94bf32a2bb1758eb478a8ecbbed07a0defe7a839
2492 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
b181c2f8c3 |
fix(#3039): clamp max/xhigh effort to high for Claude-runtime skills (#3119)
* fix(#3039): clamp max/xhigh effort to high for Claude-runtime skills effort: max in plan-phase, execute-phase, and autonomous SKILL.md frontmatter was passed through as output_config.effort, which the Anthropic API rejects when extended thinking is disabled (400: effort 'max' is not supported when thinking is disabled on this model). The frontmatter is static at install time and the installer cannot know whether thinking will be on or off at invocation. normalizeClaudeSkillEffort now clamps both 'max' and 'xhigh' to 'high' — the maximum value that works in both thinking states on all supported models. Applied in both src/runtime-artifact-conversion.cts and bin/install.js. * chore(#3039): backfill changeset PR number 3119 * fix(#3039): regenerate skills with clamped effort: high --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e7ce60fd21 |
fix(#3035): add kimi-code detection and flag to review workflow (#3115)
* fix(#3035): add kimi-code detection and flag to review workflow The kimi-code reviewer lane was declared in REVIEWER_LANES, documented in docs/COMMANDS.md, resolved via --kimi-code, and functional when reached — but review.md's detect_clis hardcoded 11 of 12 lanes (no kimi probe) and the flag-parse list omitted --kimi-code. /gsd:review --kimi-code could never reach SELECTED_REVIEWERS. Added command -v kimi detection and the --kimi-code flag to the review workflow's CLI detection and flag-parse steps. * chore(#3035): backfill changeset PR number 3115 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2061919b1a |
fix(#3029): remove redundant hooks declaration from plugin manifest (#3113)
* fix(#3029): remove redundant hooks declaration from plugin manifest .claude-plugin/plugin.json declared "hooks": "./hooks/hooks.json" explicitly. Claude Code already auto-loads hooks/hooks.json by default, so the explicit declaration caused a duplicate-rejection that silently disabled every hook the plugin ships — security guards, monitors, injection scanners. The failure had no visible signal in normal use. Removed the redundant hooks field. Updated the manifest test to assert ABSENCE of the field and that the auto-loaded file still exists on disk. * chore(#3029): backfill changeset PR number 3113 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
60cf18999b |
fix(#3026): document --pi and --gemini in installer --help (#3112)
* fix(#3026): document --pi and --gemini in installer --help --help documented 16 runtime flags but the installer accepts 18: --pi (named only in banner prose) and --gemini (invisible everywhere). Both install correctly when passed but are undiscoverable via --help. Added both to the Options section. Added a parity test that asserts every accepted runtime flag appears in --help output, with an exclusion set for legacy aliases (--both, --kimi-code). * chore(#3026): backfill changeset PR number 3112 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
10da377794 |
fix(#3021): recognize worktree-wf_* branch namespace in all guards (#3109)
* fix(#3021): recognize worktree-wf_* branch namespace in all guards The Claude-orchestration Workflow backend (#1143) creates per-plan worktrees on branches named worktree-wf_<runid>-<n>. Four independent copies of the agent branch allow-list regex (^(worktree-)?agent-...) never learned this namespace: - hooks/gsd-worktree-path-guard.js:176 — FAILED OPEN (process.exit(0)), silently disabling path containment for exactly the concurrent dispatch mode where cross-worktree writes are most likely - src/worktree-safety.cts:21 — silently dropped cleanup-wave manifest entries - agents/gsd-executor.md:503 — FATAL halt on branch check - gsd-core/references/worktree-branch-check.md:33 — same FATAL halt Extended all four to ^((worktree-)?agent-|worktree-wf_)[A-Za-z0-9._/-]+$. The path guard now correctly blocks cross-worktree writes for Workflow- backend branches instead of no-op'ing. * chore(#3021): backfill changeset PR number 3109 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bbdf34a332 |
Merge pull request #3106 from open-gsd/test/3057-wave3-negative-space
fix(#3057): reach the branches nothing could reach, and stop reaping on a PID we never probed — Wave 3 |
||
|
Tom Boucher
|
d6cce9e2c3 |
fix(#3020): verify graphify tool identity before reporting compatibility (#3107)
* fix(#3020): verify graphify tool identity before reporting compatibility checkGraphifyVersion ran 'graphify --version' on PATH and trusted any plausible version string — a foreign binary named 'graphify' that happened to print a version would silently report compatible:true with no warning. Downstream graphify work would then proceed against the wrong tool and fail later in ways that looked like GSD defects. After getting a version from the binary, verify the graphifyy Python package via importlib.metadata. If the package cannot be confirmed, emit a warning naming the mismatch regardless of version-range compatibility. The compatible flag is now correctly false for unverified tools (it was previously read by nobody — the warning is what surfaces). * chore(#3020): backfill changeset PR number 3107 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
2134d97396 |
test(#3103): expect git's separators, not the ones this machine happens to use
The Windows shard failed on the worktree-root assertion: actual C:/Users/runneradmin/AppData/Local/Temp/gsd-wt-info-Nn4hj6 expected C:\Users\runneradmin\AppData\Local\Temp\gsd-wt-info-Nn4hj6 The value comes straight from `git rev-parse --show-toplevel`, and git reports POSIX separators on every platform. The expected side was built with the native realpath, so the test encoded the separator convention of the machine it was written on. Normalising the expected side keeps the assertion exact — on POSIX the replacement is a no-op, so nothing is weakened where it already passed. This is the same assertion that was strengthened earlier today from a typeof-string-and-non-empty shape check. Pinning the exact path was right; the weaker version would have passed on Windows precisely because it asserted almost nothing. Getting a real assertion wrong on one platform is the better failure, and the Linux-only matrix could not see it — the platform shards caught it, as they did twice in the previous wave. Every other path comparison in the two new test files was swept for the same mistake. The remaining ones are safe: the base-branch tests compare against literal POSIX strings supplied to a mocked git, and the reap tests put both sides through one canonicalising helper, so they cannot disagree on separators. Drive-letter case can differ in principle at the fixed site; it did not here and no case-folding was added on speculation. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
1e844b942c |
fix(#3103): only "no such process" means the owner is dead
Review found the previous commit fixed the wrong cliff. process.kill accepts a pid up to 2147483647 and throws a TypeError above it, so 2147483648 — an ordinary finite number — sailed past the finite check, threw, and was classified dead. Measured here: 2147483646 and 2147483647 raise ESRCH, 2147483648 and above raise ERR_INVALID_ARG_TYPE. The digit-length boundary the last commit pinned was a different, earlier gate, and its tests implied it was the meaningful one. The liveness helper now treats only ESRCH as dead. EPERM, a type error from an out-of-range pid, an error with no code at all — every outcome it does not recognise returns alive, because the value feeds a forced worktree removal and an unrecognised failure must never read as permission to delete. Against a build with the old catch, a lock holding 2147483648 reaps the worktree; with this one it is skipped and the directory survives. The finite check stays. It no longer carries the safety, but it still names garbage input accurately rather than reporting it as a live owner, and its comment now says that so it is not removed as redundant. The freshness verdict is split. An unreadable lock mtime and a genuinely recent lock both reported lock_too_fresh, which tells an operator to wait when waiting cannot help — the same conflation this module already separated for parse failures. The unreadable case is now lock_age_unknown. Only this module and its tests read these strings; no workflow or command consumes them. The dependency spread in the prune path is kept as it is, with a test that kills it. A caller-supplied porcelain parser displacing the default is the intended seam, not an accident: the parser is a declared member of the dependency type and the planner already prefers an injected one, so the hard-coded key restates the same default rather than guarding against override. Previously nothing exercised that line at all. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
573d39ea60 |
fix(#3103): refuse to reap on a PID the parse could not represent
A lock file whose PID is a digit string longer than 308 characters parses to Infinity, not NaN. The default liveness helper then calls process.kill with it, which throws a TypeError rather than an errno error, and that helper's catch only recognises EPERM — so it returns false, meaning "the owner is dead", and the worktree becomes eligible to be removed. The reaper already fails closed for this exact situation. It wraps the liveness call in a catch that sets alive and does not reap, with a comment saying liveness could not be determined. That protection never fires here, because the inner catch swallowed the error first and answered confidently instead of admitting it did not know. A guard that cannot verify safety reporting success is the defect this whole epic is named for, and it was sitting inside the one function in the tree that deletes things. The guard deleted earlier on this branch tested for NaN. That test really was dead — a digits-only capture cannot parseInt to NaN — but the reachable failure is non-finite, so removing it without correcting the predicate left the hole open. The check is now for a finite value, and a malformed PID reports the same lock_owner_unknown skip as an unparseable one, since both mean the same thing: the owner is unknown, so nothing is removed. Measured, not assumed: 308 nines still parse finite, 309 are Infinity. All three of that boundary are covered, along with a 400-digit case that asserts the worktree is still on disk afterwards — the consequence, not just the verdict. Against a build with the guard removed, that case reports pid_dead_and_merged and the worktree is gone. The liveness helper's own catch still maps every non-EPERM error to "dead". The fix belongs where the value stops being trustworthy rather than at the far end of it, but that helper is worth revisiting on its own terms. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
a7fdedac6a |
test(#3103): drive the orphan reaper through its injected dependencies
Thirty-four tests covering every branch in the reaping path that no test reached, which was all of the fail-closed ones. The function has always accepted an injectable dependency bag; nothing used it. Every existing test drove real git and injected only the clock and the liveness probe, so each guard that exists for a failure — an unreadable git dir, a null directory listing, an unresolvable remote ref, a missing pointer file, an unlocked sibling, an ambiguous remote — had never executed. They are now driven by injecting exactly the fault that selects them, and each asserts its specific status and reason rather than that something happened. The last one needed no new mechanism, only the right one. It was reported as unreachable without a cross-user PID, but the default liveness helper is reachable by not injecting over it and patching process.kill, which is the deterministic injection this repo requires over real OS conditions. Its three outcomes — EPERM, ESRCH, and a clean return — now assert their verdicts. The assertions were kill-tested rather than assumed. Against mutated copies of the built module, renaming the six reason strings fails eighteen tests, neutralising the fail-closed returns fails seven more, dropping the ambiguous-remote guard fails one, and removing the prune catch and its timeout guard fails both prune tests. Flipping the EPERM arm to false turns a skip into a reap and fails that test. Four places where production folds distinct causes into one verdict are recorded in the tests rather than papered over. A lock is too fresh whether its mtime is unreadable or merely recent; a branch tip fails to resolve for three different reasons; a PID reads as alive whether the owner lives or the probe threw. Where the return value cannot separate them the tests assert the git call sequence instead, and where even that cannot, the test says so. Six weak assertions already in the older file are replaced rather than left beside the new ones: five guarded their assertions behind `if (entry)`, so a missing entry skipped the check and passed, and one asserted only that the reaper returned a non-empty array. Three JSON parses wrapped in doesNotThrow now parse directly, so a malformed payload reports its own syntax error instead of a generic message. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4926c2e904 |
fix(#3004): update Codex adapter collaboration-tool vocabulary (#3104)
* fix(#3004): update Codex adapter collaboration-tool vocabulary The generated Codex skill adapter documented stale tool vocabulary: - wait(ids) → collaboration.wait_agent(timeout_ms=...) (the real tool), with explicit disambiguation from the unrelated exec-cell functions.wait - close_agent(id) unconditional → gated on tool visibility (same schema- detection pattern already used for spawn_agent's agent_type field) - Missing required task_name field and fork_turns parameter → added alongside the existing fork_context guidance (coexist, not replace) Updated the regression test to assert the new vocabulary (wait_agent not wait(ids), functions.wait disambiguation, task_name, fork_turns, tool_search gate on close_agent). * chore(#3004): backfill changeset PR number 3104 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
63404ca45b |
test(#3103): pin every branch in the base-branch resolver to the value it returns
Thirty-eight tests, covering every previously unreached branch in this module: malformed and non-object config at each nesting level, the origin/HEAD path returning a bare remote prefix, the remote-show parse missing its HEAD line and its "(unknown)" sentinel, three catch arms, the spawn-failure path that reports unverified, the worktree probe answering something other than "true", a non-zero toplevel query, a blank toplevel, and the default diagnostic sink on both arms. The one worth naming is the local-branch fallback that was deleted earlier today as unreachable and restored. It is hard to pin because it and the empty-stdout guard above it both return null, so the return value cannot tell them apart. The test drives it through a result object whose stdout is a getter that counts reads: one read means the guard fired, two means the guard passed and the fallback ran. Deleting the line again fails the value assertion — the function would return undefined — and changing the guard to swallow whitespace fails the read count, which is the other way that branch dies. Tier attribution is now pinned. Four tests rig the lower tiers to answer with DIFFERENT branch names and assert the recorded call sequence, so a result from symref, remote-show, local-branch, or config can no longer be mistaken for another. Previously any of them could have produced the answer and the test would not have noticed. Each tier's argv and timeout are asserted exactly. The boundary trio applies to the branch listing rather than a numeric limit: zero matching lines, one, and two. Three assertions already in this file were removed rather than left beside the new ones. A doesNotThrow around the worktree probe became an exact-value test of the catch arm; a typeof-string-and-non-empty shape check on the resolved root became an equality check against the realpath; and the unverified-fallback diagnostic was asserted only to be non-empty, so it now asserts the exact sentence, which fails if the reason changes rather than only if it vanishes. Two folded bodies used try/finally for teardown and now use t.after. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c7da62b682 |
Merge pull request #3094 from open-gsd/test/3057-wave2-liveness
chore(#3057): remove tests that report coverage they do not have — Wave 2 |
||
|
sim
|
5039d49924 |
ci(#3057): shard the scoped Windows lane, the last unsharded one
The scoped Windows lane reached exactly 15m05s and was cancelled on four consecutive shas of PR #3094. A job that exceeds timeout-minutes reports as CANCELLED rather than FAILURE, which is why it first read as infrastructure noise; the giveaway is that the duration equals the cap. The Required tests rollup fans that job in, so it red-blocked merge while every other lane — including all three sharded full-Windows shards — was green. The trigger was a change to the shared test helper, which scopes the install-heavy suites into the selected list. The lane normally runs about eight minutes; with that list it does not fit. It was the only unsharded lane left in this job, so it was the only one without headroom to absorb a large scoped list. Issue #869 hit this exact cliff on the sibling lane and named the durable answer in its own follow-up: a timeout bump moves the cliff, sharding removes it. #2952 then sharded the full lane. This finishes that work. The runner already supports it — the shard partition is applied after scope selection, so it composes with a selected file list rather than only with a suite, and the partition is cost-weighted from the measured timings table. The job name template already renders a shard suffix when one is present, so the three entries name themselves. No individual matrix job is a required status check; the rollup is, and it is name-independent, so renaming these jobs does not touch branch protection. timeout-minutes stays at 15. Each shard now does roughly a third of the work, so the cap goes from binding to backstop without being raised. The lane-shape tests were generalized rather than relaxed: the complete-shard-set invariant now runs per sharded scope instead of only over the full lane, and "only the full lane is sharded" became "targeted is the only unsharded lane". A new assertion pins the shard through to the runner — without it the three shards would each run the entire selected list, triple the cost and no speedup, and every check would stay green. No LANE_COSTS entry is added for the new shards. The only recorded cost for that lane is the pre-sharding run that hit the cap, and inventing a post-sharding number would be exactly the kind of unmeasured claim the rest of that table avoids. The estimate and the reason are written down instead, to be replaced by a real measurement. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2f5b6a9b48 |
fix(#3001): indent continuation lines in serializeChangelog (#3101)
* fix(#3001): indent continuation lines in serializeChangelog serializeChangelog interpolated bullet bodies verbatim into a single - ${body} (#${pr}) line. Any embedded newline became a column-0 line; parseChangelog's continuation-fold (/^\s+/) didn't pick it up, so flushBullet terminated the bullet early — dropping the continuation text and the (#NNNN) PR trailer (recorded as pr: null). The round-trip property serialize(IR) → parse(text) === IR was false for any body containing \n. Fix: indent continuation lines (body.replace(/\n/g, '\n ')) so the parser folds them correctly. Round-trip test asserts both paragraphs' content AND the PR number survive. * chore(#3001): backfill changeset PR number 3101 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
7ef9945adb |
test(#3090): stop paying for the guard on every teardown
The Windows node-24 scoped-test job hit its 15-minute ceiling twice on this
branch and GitHub reported both as cancelled, which is how a job timeout
surfaces. The same job finishes in about two minutes twenty on next, across
four consecutive runs. cleanup() is the only hot-path change here.
It was doing up to seven filesystem calls per invocation: three probes of the
temp root, two more for each conventional temp dir, then an existence check and
a realpath of the target. On Windows fs.realpathSync.native opens a file handle
and Defender charges for each one, and this runs in the teardown of effectively
every test.
The root candidates are now memoized on the live os.tmpdir() value. The key
matters: two files in the suite override TMPDIR mid-run and restore it, so a
plain module-level hoist would go stale for them, while re-reading os.tmpdir()
costs an env lookup. Measured over 20,000 calls: 546ms unmemoized, 10ms
memoized.
The symlink-escape check is removed rather than optimized, because it was
guarding something that cannot happen. Verified directly on node v26.5.1:
fs.rmSync(link, {recursive: true, force: true})
link exists false
victim exists true
file exists true
rmSync unlinks a top-level symlink and leaves its target alone, and a symlink
nested inside a tree being recursively removed is also unlinked rather than
followed. The check cost two filesystem calls per teardown on the slowest
platform in the matrix and bought nothing. Its test asserted the victim
survived, which was true with or without the guard.
What closed the original defect is untouched: a target outside the known temp
roots is still refused, before the chdir and before the rmSync, with the roots
named in the message.
Refs #3057
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
aa7697fe97 |
fix(#2997): include phase_id_convention in resolved config (#3098)
* fix(#2997): include phase_id_convention in resolved config _baseConfig in config-loader.cts is an explicit allowlist of keys copied from parsed into the resolved config. phase_id_convention was in VALID_CONFIG_KEYS (manifest line 83) and survived the unknown-key filter, but was never copied into _baseConfig — silently dropped on a clean read. The milestone-prefix validation check could only be activated via the ROADMAP frontmatter fallback, not the documented project-config surface. Added phase_id_convention: get('phase_id_convention') ?? null to _baseConfig. 3 tests: survives resolution, null round-trips, absent resolves to null. * chore(#2997): backfill changeset PR number 3098 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
1046a721f9 | Merge remote-tracking branch 'origin/next' into test/3057-wave2-liveness | ||
|
Tom Boucher
|
5628edddda |
fix(#2991): route /gsd command output through Pi's display shape (#3097)
* fix(#2991): route /gsd command output through Pi's display shape The registerCommand('gsd') handler returned a bare string, which Pi's ExtensionAPI does not display. Changed all return paths to Pi's structured { content: [{ type: 'text', text }] } shape, matching the gsd_invoke tool's proven output contract. Updated 2 reachability tests that asserted the old bare-string return shape. * chore(#2991): backfill changeset PR number 3097 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
2e81516f1f |
test(#3090): accept the temp roots the suite actually uses, and say which ones
A fourth failure of the same guard, found by review before it reached CI: the config-schema property suite builds fixtures through a getWritableTmp() helper that returns the first writable of /private/tmp, /tmp, os.tmpdir(). On macOS that is /private/tmp while os.tmpdir() is /var/folders/.../T, so five cleanup() calls in that file were refused outright. The three earlier breakages were all spellings of one root. This one is not: the suite legitimately uses more than one temp root, so the premise was wrong rather than the encoding. tmpRootCandidates() now probes the conventional system temp dirs alongside os.tmpdir(), each included only if it exists on the host, so the accepted set stays a bounded explicit list instead of growing a patch per platform. Two corrections that follow from the same review: A root that is itself a filesystem root already ends in a separator, and appending another built `//`, which only the literal `/` satisfies — TMPDIR=/ would have refused every descendant. The separator is only appended when it is not already there. The refusal messages named os.tmpdir(), which stopped being the boundary. They now name the roots actually compared against. Every failure of this guard so far was diagnosed from that message in a CI log, so it should show what was checked rather than a stale approximation of it. The symlink-escape check keeps its refusal but drops its claim. fs.rmSync does not follow a top-level symlink — it unlinks the link and leaves the target alone — so that check was never closing a live escape, and the test asserting the victim survived would have passed with the guard removed. Both now say what is true: a defense-in-depth boundary against a future change to the deletion mechanism, with only the refusal itself load-bearing. The QA path helpers were evaluated for reuse rather than keeping a third copy of path containment. They resolve against one project directory and have no multi-root or Windows short-name handling, so they are not a drop-in; noted rather than forced. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2a77e50daf |
fix(#2989): anchor code-review diff-base grep to phase-mention convention (#3096)
* fix(#2989): anchor code-review diff-base grep to phase-mention convention The diff-base fallback in code-review.md used git log --grep with a bare phase number (unanchored substring), matching version strings, dates, issue refs, and other phases' numbers. tail -1 took the oldest match — routinely a commit from months or years before the phase existed. The fail-closed branch was dead code because a bare digit almost always matches something. Changed --grep to '[Pp]hase N\b' with --extended-regexp, anchoring to the phase-mention convention. When no commit genuinely references the phase, the derivation yields empty and the fail-closed warning fires (now reachable). All three consumers (Tier 3 file scope, fallow pre-pass, agent context) use the same corrected value. * chore(#2989): backfill changeset PR number 3096 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
cc0a4685f1 |
test(#3090): close the symlink escape, and stop the test from mirroring the guard
An isolated review found the safety precondition was not safe. Test 4 uses the repo's own tests/ directory as a path that must never be deleted, and asserted beforehand that it sits outside the temp root — but it computed that root as path.resolve(os.tmpdir()) alone, while cleanup() accepts a target under any of several root spellings. For a checkout under the realpath'd temp root the precondition reads "outside, safe to proceed" while the guard reads "inside, delete it", and rmSync runs on tests/ before the assertion fails. The commit that added it claimed it would fail loudly instead of destructively; it would have done the opposite. The fix is not a second root in the test. tmpRootCandidates() is exported and the precondition calls it, so there is one source of truth and nothing left to drift. A mirror was the defect, not its contents. The same review bounded what the refusal actually guarantees: the check is a string prefix test, so a symlink living under tmpdir but pointing outside it passes while rmSync follows the link and deletes the real directory. When the target exists its real path is now checked too, against the same predicate — factored into one function so the two comparisons cannot diverge the way the test's copy did. A realpath failure refuses rather than proceeds; a safety check that cannot verify must not report safe, which is the whole subject of this branch. Missing targets are skipped, since rmSync with force no-ops on them and realpathSync would only throw ENOENT. `const isTmpPath = true` is gone. It survived the previous commit as a way to keep the catch's `&& isTmpPath` reading as a real condition, but a constant dressed as a test states nothing; the guard clauses above throw, so the catch comment now says the invariant in words instead. The new coverage does not depend on the platform the bug lives on. The root list is asserted directly — non-empty, absolute, deduped, and containing a freshly created temp dir — and the symlink refusal runs everywhere, skipping only where symlink creation is unavailable. The previous realpath test was coverage-identical to the control on Linux, so the only lanes the matrix runs could not have verified the fix it was written for. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
b375d76a10 |
test(#3090): accept every canonical spelling of the temp root, not one platform's
Windows CI refused legitimate temp directories for the same reason macOS did, one commit earlier: cleanup() refused to remove a path outside os.tmpdir(): C:\Users\runneradmin\AppData\Local\Temp\bug-3491-7zisup GitHub's windows runners report os.tmpdir() in the 8.3 SHORT form (C:\Users\RUNNER~1\AppData\Local\Temp) while callers hold the expanded LONG form. fs.realpathSync() does not reliably expand 8.3 names; only fs.realpathSync.native() does. Casing can differ independently (C:\ vs c:\). Two platform-specific breakages from the same check is a sign the check was written against one spelling rather than the concept, so this stops patching symptoms. tmpRootCandidates() collects every variant derivable from os.tmpdir() — resolved, realpath'd, and native-realpath'd — each probe isolated in its own try/catch so an unavailable variant contributes nothing instead of crashing teardown, then deduped. A target is accepted under any of them, and the comparison folds case on win32 only, where casing genuinely varies. The error message still prints the original-case path. On this machine three variants collapse to two: /var/folders/.../T and /private/var/folders/.../T. Both spellings of a real temp dir are accepted and removed. The Linux matrix passed every one of these broken states — 30544 and then 30545, both lanes green — because /tmp has neither symlink indirection nor short names. The platform CI shards are the only thing that has caught any of it, which is worth stating plainly given what this branch is about. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2843e25bf3 |
fix(#2988): local changeset/docs lint falls back to next, not main (#3095)
* fix(#2988): local changeset/docs lint falls back to next, not main Both scripts/changeset/lint.cjs and scripts/lint-docs-required.cjs resolved their diff base as GITHUB_BASE_REF || 'main'. GITHUB_BASE_REF is set only in GitHub Actions; locally it falls back to 'main' (the release branch), which lags far behind 'next' (the integration branch every PR targets). The oversized diff range swept in every changeset fragment merged since the last release, so the lint passed on the first fragment it saw regardless of whether the current PR authored it — structurally vacuous. Changed the fallback to 'next' (DEFAULT_BASE constant, exported from both scripts for parity). CI behavior unchanged (GITHUB_BASE_REF is set there). Added a parity test asserting both lints resolve the same base. * chore(#2988): backfill changeset PR number 3095 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
164076b6ac |
test(#3090): compare both canonical forms of the temp root, not just the unresolved one
The guard added in the previous commit refused legitimate temp directories on macOS. os.tmpdir() returns a path under /var/folders/..., /var is a symlink to /private/var, and path.resolve() does not resolve symlinks. So a caller that passed the realpath'd form — via fs.realpathSync(), or via process.cwd() after chdir-ing into a temp dir, which returns the resolved path — produced /private/var/folders/... and failed a check written against /var/folders/... Measured on this machine before the fix: mkdtemp path /var/folders/.../probe-XXX allowed fs.realpathSync of the same dir /private/var/folders/.../probe-XXX REFUSED process.cwd() after chdir to it /private/var/folders/.../probe-XXX REFUSED The accepted-roots set is now built from both the resolved and realpath'd forms of os.tmpdir(), deduped — on Linux they are identical, so the set collapses to one entry and nothing changes there. realpathSync is wrapped because it throws if the root is momentarily missing, and a safety check must not become a new crash. The target itself is deliberately NOT realpath'd: cleanup() is called on already-deleted directories, where realpathSync raises ENOENT. The roots are computed per call rather than hoisted to module scope, because two test files override TMPDIR and a hoisted value would go stale for them. Worth naming: the remote matrix runs linux-node22 and linux-node24 only, and it passed 30544/30544 on the broken commit. os.tmpdir() is /tmp on Linux with no symlink indirection, so that matrix could not have caught this at any sample size. The regression test added here branches on whether realpath differs from the original path, so it exercises the real case on macOS and stays meaningful rather than vacuous on Linux. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
955655407c |
fix(#2979): document ExitError plain-text carve-out in json-errors.md (#3093)
* fix(#2979): document ExitError plain-text carve-out in json-errors.md The JSON-errors doc claimed every error emits a structured JSON envelope, but usage errors (ExitError) intentionally emit plain text with their own exit code (src/cli-exit.cts:36-39 catches ExitError before the envelope branch). Anyone following the doc's 'always parse stderr as JSON' guidance against a usage error got a parse failure. Amended the Wire format + Overview + Writing tests sections to scope the structured envelope to non-ExitError failures, stated the carve-out with a pointer to cli-exit.cts, and scoped the JSON-parse instruction to the envelope branch. Added a characterization test pinning both paths together (ExitError -> plain text + own code; non-ExitError -> JSON envelope) so the code cannot drift toward the doc's prior overstated claim. No runtime change — the test passes before and after the doc edit. Re-scoped per maintainer triage: the smart-entry --json part is already satisfied (shipped payload exposes the command token); only the doc correction + characterization test remain. * chore(#2979): backfill changeset PR number 3093 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
fba501836b |
test(#3090): let the destructive call ask the safety question the function already answers
cleanup() computed `isTmpPath` — the exact predicate for "is this path safe to delete" — and consulted it only inside the catch block, to classify a transient Windows error. The rmSync above it ran unconditionally against whatever path it was handed, with `recursive: true, force: true`. The guard existed and the destructive call never asked it. The chdir at the top of the function makes the failure mode worse rather than better: a wrong target first moves the process out of the tree, then deletes it. So the predicate moves above both, and an out-of-tmpdir path is refused before either can run. The refusal throws and names the path; returning quietly would reproduce the fail-open shape this wave exists to remove. The catch keeps its `&& isTmpPath` term. It is now always true, but it states the condition the swallow depends on rather than inheriting it from a check twenty lines up, and it stays correct if the guard is ever relaxed. All 300+ call sites resolve under os.tmpdir() today, including the two files that override TMPDIR — both create their override root through the real os.tmpdir() first — so nothing legitimate is refused. The regression test targets tests/ itself: a real directory that must never be deleted, so nothing is created and nothing needs tearing down. It asserts the throw names the path, that cwd is unchanged (the chdir hazard), and that a known file inside still exists — proving the directory was not emptied rather than merely still present. Whether tests/ is outside os.tmpdir() is environment-dependent: os.tmpdir() is /tmp on Linux, so a checkout under /tmp would put it inside, and there a correct guard would delete this directory rather than refuse it. The test asserts that precondition before calling cleanup, so that environment fails loudly instead of destructively. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
6128f73003 |
test(#3090): normalize the whole reason line, not just the category token
The allow-test-rule gate keys on identity, and the identity it records is everything after the colon on the annotation line — not the category token. Ten annotations carried the canonical category plus a trailing justification on the same line, so the recorded identity was a prose blob, and where the prose wrapped it was a sentence fragment: `source-text-is-the-product — the workflow .md content IS`. Seven of those ten are ones this branch already rewrote. That pass renamed the token and left the prose, which is the same error this wave exists to correct, one level down: the label was fixed without checking what the machine reads. Justifications move to the following comment line, which the scanner ignores because it lacks the token. No annotation gains or loses an issue reference, so no exemption changes compliance status; the allowlist goes 161 to 159 as two files' duplicate identities collapse. git-base-branch.test.cjs carried the token twice — once as the real annotation, once echoed in docblock prose that the line scanner parsed as a second exemption with a truncated identity. The echo is reworded to drop the literal token. intel.test.cjs:1360 was cut off mid-clause with an issue ref appended after the break; its sentence is restored and the ref kept on the annotation line so it stays compliant. Every remaining non-canonical identity is an ESLint RuleTester fixture inside a `code:` template literal, which the line scanner cannot tell apart from an annotation. Those two stay grandfathered. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2979f2a994 |
fix(#2978): add structural validation to roadmap validate (#3092)
* test(#2978): roadmap validate must perform structural validation Failing-first: roadmap validate returns {"warnings":[]} (exit 0) for every input — empty file, garbage, missing file, truncated frontmatter — because it performs no structural validation and its one opt-in check (W021 milestone- prefix) is off by default. Six cases: empty, garbage, missing, truncated frontmatter, well-formed (no false positive), BOM-prefixed (not corruption). * fix(#2978): add structural validation to roadmap validate roadmap validate returned {"warnings":[]} (exit 0) for every input — empty file, garbage, missing file, truncated frontmatter — because it performed no structural validation and its one opt-in check (W021 milestone-prefix) is off by default. A verb named validate that cannot produce a negative result provides false assurance. Add four structural checks, each producing a coded warning {code, message}: - V001: file missing/unreadable (was silent success) - V002: empty/whitespace-only - V003: malformed frontmatter (unterminated --- fence; BOM-tolerant per #3057) - V004: no recognizable phase entries (no ### Phase N: heading) Keep the existing W021 milestone-prefix check as-is. Exit non-zero via ExitError(1) when warnings are non-empty, per the documented contract ('exits non-zero on any error or warning'). Well-formed roadmaps (incl. BOM-prefixed, CRLF) still validate cleanly with warnings: [] and exit 0. * test(#2978): update W021 tests for non-zero exit on warnings Two existing W021 tests asserted roadmap validate exits 0 even with warnings ('roadmap validate should exit 0 even with warnings') — that was the bug. #2978 made validate exit non-zero on any warning (per its documented contract). Updated both mismatch-case tests to expect success===false and parse the JSON output from the failure path (stdout is written before the ExitError throw). * chore(#2978): add changeset fragment * chore(#2978): backfill changeset PR number 3092 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
a4560c3669 |
test(#3090): read the body Status, not the frontmatter key that shadows it
stateExtractField tries bold **Field:**, then a plain ^Field: line, first match wins. STATE.md's frontmatter carries a `status:` key that appears before the body's plain `Status:` line, so extracting "Status" from unstripped content returns the frontmatter value and never the body prose the test was written against. Scoping the lookup to stripFrontmatter() fixes it. The tempting fix was to assert the frontmatter enum instead, on the reasoning that a typed value beats matching prose. That would have been wrong and would have weakened the test: normalizeStateStatus maps both "Phase complete — ready for verification" and "Verifying Phase N" onto the same 'verifying' enum, so the enum cannot tell phase-complete from mid-verification, which is precisely the distinction this case exists to prove. Typed is not automatically stronger when the type conflates the cases under test. Case 4 carried the same collision and was passing only because normalizeStateStatus falls through to the raw text when no known pattern matches, so frontmatter and body happened to agree. Fixed alongside it rather than left for the next person to trip over. All fourteen conversions on this branch were audited against the same failure mode. The collision can only arise where the frontmatter key and the body field name are identical case-insensitively — Status is the only such field, since every other frontmatter key is snake_case against a Title Case body label. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
7dd9e59f6b |
test(#3090): stop exempting violations under categories that do not fit
An allow-test-rule annotation citing a category that does not apply is worse than no annotation, because it reads as reviewed. Eight were confirmed by reading the assertions each one covered, and auditing the rest found five more plus one refutation — a converter test whose wording described the wrong mechanism while the covered assertion genuinely was deployed-text. The instructive one used the CANONICAL string for the same mistake: STATE.md command output labelled as a deployed artifact. A canonical string is not evidence the category fits, which is why normalising strings alone would have laundered the problem rather than fixed it. Every mapping the audit had inferred rather than code-verified was spot-checked before rewriting, and the ones that turned out not to fit were re-annotated rather than relabelled. Fourteen STATE.md assertions had a typed extractor available all along and now use it; their annotations came out because nothing needs exempting. Eight assertions genuinely need a production change first — CLI stdout and stderr with no structured mode — and are tagged pending-migration-to-typed-ir citing #3090, which is what that category is for. It had zero real uses before this, while one file carried a real citation to migration issue #2974 under a non-canonical tag. Six annotations covered assertions that do no text matching at all. An exemption for a violation that does not exist is noise that makes the real ones harder to audit; those are removed. atomic-write-coverage gains the annotation it always warranted — its own docstring describes a structural-regression-guard while the file carried none. Fifty-nine non-canonical strings across roughly thirty files are normalised, and the allow-test-rule allowlist is regenerated to match. 472 annotations became 463: every one now uses a canonical category, and the two remaining non-canonical strings are ESLint RuleTester fixtures, not annotations. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
b0f1722662 |
fix(#2969): ratchet completed_plans up for gap-closure plans under deriveProgressKeys (#3091)
* test(#2969): completed_plans must ratchet up for gap-closure plans Failing-first regression: when deriveProgressKeys=true (cmdStatePlannedPhase's opt-in), applyStatePreservation restores completed_plans to its pre-growth curated value, so gap-closure plans that complete never increment it — STATE.md shows completed_plans < total_plans forever even though every PLAN has a SUMMARY. Three cases: the ratchet-up (derived 54 > curated 50), the ratchet-down protection (derived 47 < curated 50 keeps curated), and the body-only write protection (no deriveProgressKeys → wholesale restore). The existing #2440 test covers the case where derived < curated (ratchet holds); this adds the missing case where derived > curated (ratchet must release upward). * fix(#2969): ratchet completed_plans/plases up under deriveProgressKeys applyStatePreservation's deriveProgressKeys path (cmdStatePlannedPhase's opt-in) let total_plans/total_phases take the derived value but restored completed_plans/completed_phases to their pre-growth curated value — so gap-closure plans that completed after the plan count grew never incremented them, leaving STATE.md at completed_plans < total_plans forever (every PLAN had a SUMMARY). Extend the deriveProgressKeys exclusion to also let completed_plans and completed_phases take the derived value, but ratcheted UP only (never derive downward past curated) — preserving the #3242 curated-progress protection for cases unrelated to plan-count growth (e.g. a deleted SUMMARY). percent takes the derived value (the resync already recomputed it from disk counts). Scoped to deriveProgressKeys (plan-phase only); body-only writes (state.update/patch without the flag) keep the full #3242 wholesale restore. * fix(#2969): also take derived percent under deriveProgressKeys Isolated-review blocker: percent fell into the else branch and was overwritten with the stale curated value, contradicting the inline comment and leaving STATE.md incoherent (e.g. completed_plans:54/total_plans:54 at percent:93). Skip percent in the ratchet loop so the derived (resync- recomputed) value survives. * chore(#2969): add changeset fragment * chore(#2969): backfill changeset PR number 3091 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
9723d2b7e0 |
test(#3090): assert which value, not which type
Nine assertions drove a real fault through a real seam and then checked only that the call did not throw, or that a result was a string, a boolean, an array. Each passed whether the code was right or wrong. #3050 is the canonical instance of the shape: its counter-test asserted effectiveRoot was a string and never which root, so a silent misroute passed it. All nine now assert the exact verdict, derived from the production branch each one reaches and traced back to source rather than taken from the survey. One was worse than a weak assertion. The test targeting resolveWorktreeLinkage's main_worktree path used createTempGitProject, which always seeds .planning/ — so the reason was always has_local_planning and the git-dir comparison the test appears to exercise was unreachable from its own fixture. It was not asserting loosely, it was pointed at the wrong path. The fixture now builds a git project without .planning (projectDoc had to be disabled too, since it defaults to git and would have re-seeded it), and the test reaches the branch it names. Another had no reason assertion anywhere in the file while its four siblings all pinned theirs — the odd one out rather than a convention. The last is mine. The parity guard shipped in #3077 checked typeof and doesNotThrow across four ExecGitFn seams, and that PR described it as failing "the moment any site re-grows its own shape". It could not: a site returning a different value of the same type passed it. All four benign-passthrough outputs are derivable exact values, so it now asserts them and the claim is true. Test names that promised more than their assertions established are corrected to match what they prove. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
53ea8e0664 |
fix(#3057): make a guard's failure distinguishable from its benign result — Wave 1 (#3088)
* fix(#3057): refuse the write when the duplicate scan cannot complete writeManifest documents itself as a fail-closed duplicate guard: if any existing manifest shares plan_id with a different, non-terminal job_id it must refuse, because dispatching again would duplicate the external job. It could not honour that. The scan reads every sibling manifest looking for the duplicate, and an unreadable or unparseable sibling was `continue`d past. If the corrupt file was the one holding the live duplicate, the scan found nothing and a duplicate external job dispatched. The asymmetry is what gives it away: a malformed TARGET refused with malformed_existing because clobbering is unacceptable, while a malformed SIBLING was skipped — yet siblings are the only thing the duplicate check reads. Adds a scan_incomplete verdict that refuses and names the offending file, so an operator can quarantine or repair it. Fail-closed alone would let one stale corrupt manifest wedge every dispatch for that planning dir permanently; naming the file is what makes refusing survivable. malformed_existing is untouched, so the target/sibling distinction stays visible. The docstring is updated — it previously stated a rule the function did not keep. memFs() gains an optional failReads map so these branches are reachable at all; they had zero coverage because the fake could not express a per-file read fault. The signature is additive and every existing caller is unchanged. The regression is proved by a pair, not a single test. A control writes a readable sibling holding a genuine non-terminal duplicate and asserts duplicate_plan_id, establishing the scenario is real; the regression then makes that same path unreadable and asserts scan_incomplete. A first draft of this test used a corrupt-JSON fixture containing no plan_id at all while its comment claimed otherwise — it duplicated the unparseable-sibling case and proved nothing, which is the defect class this phase exists to remove. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): make a guard's failure distinguishable from its benign result Wave 1 of the negative-space backfill: the branches where a guard that could not verify something reported the same value it reports when everything is fine. That indistinguishability is the defect; every fix here makes the two states tellable apart, and every test proves it with a pair — one for the failure, one for the benign case. A single test cannot establish that two states are distinguishable, which is the whole property being fixed. state.cts phaseInventoryProvider returned null for both a real disk-scan failure and a genuinely empty phases dir, so `state rebuild` could report success while phase-table reconciliation never ran. It now returns a discriminated result and the CLI surfaces phase_inventory_scan_failed plus a reason. The reason field turned out never to have been wired into the emitted JSON at all — it existed only as an internal variable — so a test could only assert on the operator-facing note. It is a real field now. state.cts treated an unreadable lock body the same as an empty one, applying the 1-second stealable floor. A lock we cannot read is not a lock we know is stale; an unreadable body is now held to the deadman ceiling like a live holder. verification.cts findStaleVerificationSummary returned null on any fs, scan or clock failure — meaning "not stale". It now returns a discriminated StaleCheckResult and the caller records that the check was indeterminate. git-base-branch resolveBaseBranch returned 'main' both when no candidate branch existed and when every git tier timed out. A diagnostics variant now reports whether the answer was verified, and the CLI writes an unverified-fallback note to stderr. The stdout contract five workflows parse is untouched. worktree-safety snapshotWorktreeInventory left exists:true when statSync threw, so a guard that could not check reported the worktree present; exists is now tri-state and a stat failure surfaces as an 'unverified' finding. planWorktreePrune reported 'no_worktrees' for a parse failure, which is not the same as an empty list — and it drives a prune. It now reports 'parse_failed'. Fixing the inventory change exposed a second fail-open in verify.cts: the validate-health consumer silently dropped findings whose kind it did not recognise, so the new kind would have vanished. That is closed too — worth noting that the survey enumerated producers of degraded verdicts, not consumers that discard them. worktree-base-ref and state-transition gain the distinguishing signal without changing what they do: headAbsenceVerified, and a phase-inventory scan meta. Whether those guards should ACT differently is a product question this change does not answer, and both are flagged rather than quietly settled. rescueSummaryArtifacts is left alone: rescuing on an uncertain cat-file is deliberate per #2556. It now has tests proving it, and a recorded negative finding — git cat-file -e returns 128 for both "absent from HEAD" and a fatal error, so "uncertain" and "certain-and-fine" are not separable at the git level. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): assert typed values, not rendered text Ten assertions in the rebuild CLI suite matched substrings of produced output — STATE.md body fields, a markdown table row, an audit-log heading, and JSON keys read as text. CONTRIBUTING prohibits that: if the code under test produces text, the test asserts on its structured surface instead. No production surface had to be built. Every one already existed and was already compiled into bin/lib: stateExtractField for body fields, parseMarkdownTable for the phase table, collectSection for the audit-log section, and result.data.log — already a typed RebuildLogEntry[]. The tests were matching rendered text sitting next to the structured data. One of those assertions was passing for the wrong reason. `stdout.includes ('rebuilt')` matched the JSON KEY name, not a value: the dry-run path emits `mutated` and the real path emits `rebuilt`, so it would have passed whether the value was true or false. It now asserts the value. external-job's refusal already had to name the offending file — that naming is why the fail-closed variant is survivable rather than a permanent wedge — but the tests proved it by substring of a prose message. The failure result now carries offendingPath as its own field and the tests assert it by value. The human message is unchanged; operators read it. Array membership is left alone. `phaseIds.includes('99')` and `result.updated.includes('Completed Phases')` are membership checks on real arrays, not text matching, and converting them would weaken nothing and clarify nothing. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): execute acquireStateLock instead of grepping its source The non-EEXIST lock test asserted on the TEXT of the built .cjs and never called acquireStateLock. It carried an allow-test-rule: architectural-invariant exemption to permit that. A source grep proves a literal is present in a file, not that the behaviour works — it is weaker than a liveness test, which at least runs the code, and it was the only coverage the fatal-errno path had. Replaced with tests that inject the errno through fs and assert what actually happens: a fatal EACCES propagates out of acquireStateLock with zero backoff sleeps, while EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE/EPERM/EBUSY retry once and succeed. The exemption is removed and its allowlist entry with it. One old assertion is deliberately not carried over: it checked the retryable errnos were expressed as a Set rather than an inline literal. That is a shape check with no runtime signature; the behavioural tests fail if the code reverts to the old inline check, which is the regression it was really guarding. The #3057 lock-body tests move into that same file rather than a new one, which is what lint-test-file-count asks for and puts every acquireStateLock test in one place. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): surface an indeterminate staleness check to its callers An isolated review caught an inconsistency inside this wave. Two of the three "add the distinguishing signal" fixes wire through to something a user sees: git base-branch writes an unverified-fallback diagnostic to stderr, and an unverifiable worktree surfaces as a W020 finding. The third set staleCheckIndeterminate on readVerificationStatus's result and nothing read it. A signal nobody consumes leaves the fail-open exactly as silent as before: the staleness check could fail and the operator saw precisely what they would see if the answer were genuinely "not stale". That is the defect this issue exists to remove, so it is not defensible as scaffolding when its two siblings in the same change already wire through. All five callers now surface it, each through the channel it already had rather than a mechanism imposed uniformly: phase complete adds it to its existing warnings array and, on the blocked path, as an additive note on the error text; init and roadmap carry it as a field on output they already emit; the UAT report carries it without ever gating passed/blockers; workstream inventory takes an injectable writeDiagnostic mirroring the git base-branch idiom, because its return shape had nowhere to hang a per-phase field without rippling the builder's types. The routing decision is unchanged everywhere. What changes is only that a caller and an operator can now tell a failed check from a completed one. That diagnostic carries structured meta rather than being asserted by regex — the default still writes only the human message to stderr, but tests assert phaseDir and reason by value. Two earlier assertions in this branch were converted the same way; this was the last raw-text assertion left. Also records a scope correction: the completePhaseCore guards now compare stateReplaceField's result to the body instead of testing truthiness, so a field whose substitution produced identical text no longer reports as updated. That is a real behaviour fix, not the signal-only change this file was described as carrying, and its tests cover both the changed and unchanged cases. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): bound two heavy subprocesses for a loaded bench, not an idle one The remote matrix surfaced three failures unrelated to this branch's changes. All were bad tests, and a re-run would have hidden every one of them. The reviewer-flags parse block bounded bash -> node -> a full gsd-tools cold start at 5 seconds. On a bench running thirty thousand tests in parallel that is not a hang, it is a busy machine. Raised to 30s, matching the convention sibling suites already use for script invocations, with a comment saying what the budget covers so nobody tightens it back. Two further copies of the same 5-second spawn in the same file had the identical defect and are raised too — they were not in the failure report, but they will be next time. The fragment-propagation test bounded npm run regen:derived — a full build plus eight generators, the heaviest subprocess in the suite — at five minutes, and node22 was killed near the end. The captured output proves it: every generator had written its files and gen:install-tree had emitted all fifteen runtimes before the kill. Raised to fifteen minutes. That failure read as `null !== 0`, which says nothing. status null means killed, not a non-zero exit, and the two want different responses: one is a timeout to size correctly, the other is a real build break. The assertion now distinguishes them and names the signal. Neither test's assertions were weakened and no retry was added. A retry here would suppress exactly the signal the timeout exists to produce. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture fd 1 through the mock tracker, not a raw reassignment The phase suite reported zero test results on both lanes while running for five and a half minutes and exiting 1. No assertion text, no stderr, four events for the whole file: enqueue, start, dequeue, complete. That shape is not a failing assertion — it is the runner being unable to read the child at all, because it parses its event stream from the child's stdout. The cause was the capture helper reassigning fs.writeSync directly. Proven rather than assumed: a standalone probe patched fs.writeSync and called process.stdout.write, and the interception fired only when fd 1 resolved to a FILE, not when it was a pipe. The remote runner captures the event stream to a file, so a helper that was invisible against a pipe swallowed the reporter's own output on the bench. That is also why the two sibling suites wired the same way in this change pass cleanly — they use the mock tracker, the seam io.test.cjs established for this exact function. The helper now uses t.mock.method with an explicit restore after each call, so teardown belongs to node:test rather than a second hand-rolled implementation, and the interception cannot outlive the one synchronous call it wraps even if that call throws. Ten call sites thread the test context through; three test callbacks gained the parameter they lacked. The three B3 tests are untouched — same assertions, same fault injection. Only how the context reaches the helper changed. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture phase-complete output from a subprocess, not fd 1 Two attempts to make in-process fd-1 interception safe both failed on the bench. The suite reported zero test results on either lane while exiting 1 — four events for the whole file — because the runner parses its event stream from the child's stdout, and process.stdout.write routes through fs.writeSync whenever fd 1 resolves to a file, which is how the runner captures. Patching that seam anywhere in a file can therefore destroy the file's own reporting, and tightening the window only moved the runtime from 326s to 125s without recovering a single event. So the interception is gone rather than tuned. The helper now spawns gsd-tools as a real subprocess and reads stdout the way the OS already gives it to us, which is what the rest of the suite does. It asserts the command succeeded before parsing, so a genuine failure can no longer present as a JSON parse error. The two fault-injecting tests could not survive that move as written: a subprocess cannot see a mock installed in the parent. Instead of reinstating the interception they now produce the fault on disk — the summary artifact is created as a dangling symlink, so the staleness check's real statSync throws inside the child. That is a more honest fixture than a mock in any case, since it is a condition a user's tree can actually be in. Skipped on Windows, matching the existing symlink precedent in the write-guard suite. Three further call sites turned out to depend on parent-process writeFileSync mocks the subprocess could not see. Those call the CJS function directly, which is what they always wanted — they never needed stdout at all. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): one name for one signal, one encoding for one distinction Standards review found four things this branch introduced, all of them inconsistencies with itself rather than with the repo. One upstream bit reached its consumers under three names — verification_stale_check_indeterminate in two modules, the same value with "stale" dropped in a third, and stderr only in the fourth. Standardised on the long name wherever it is a field. The workstream inventory keeps its stderr channel, since its return shape has nowhere to hang a per-phase field without rippling the builder's types, but it now says the same word for the same thing. worktree-safety encoded one three-way distinction two ways in a single file: a named union for a finding's kind, and boolean|null for an inventory entry's existence. The second is now a named union too. Two assertions matched human prose because the blocked and non-blocked completion paths carried no typed field for the signal. Both now assert typed values. The first round of this fix added the field but left the regex beside it, which is the banned pattern sitting next to its own replacement; the second removed it and added an assertion on the reason enum so nothing was lost. The remaining two were reasoned away before being fixed, and both reasons were bad. "No typed surface exists" is the condition CONTRIBUTING says to fix by adding one — it took three lines. "The file already does this dozens of times" is not licence to add instance number thirty-one; a convention that violates a documented rule is debt, not precedent. Vocabulary differing across DIFFERENT modules is left alone: CONTEXT.md rejects a single shared result envelope, so per-module shapes are precedented, and a baseline smell does not outrank a documented standard. A census of every line this branch adds to a test file now finds no regex or substring assertion on produced prose: 87 strictEqual, 25 ok (all non-empty or shape guards), 12 equal, 3 throws (all typed err.code predicates), 3 deepStrictEqual, 2 notStrictEqual. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3057): backfill changeset pr number to 3088 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
589a9b29b0 |
fix(#2962): enable nullglob in for-glob shell blocks for zsh portability (#3087)
* fix(#2962): enable nullglob in for-glob shell blocks for zsh portability Workflow shell blocks are fenced bash but execute in the user's login shell (zsh on macOS). zsh's nomatch default aborts the WHOLE block on an unmatched glob in a for-list (not just skipping the command), silently bypassing every statement after it — including the verify-phase decision-coverage gate, whose optional *-CONTEXT.md lookup used the unsafe for-list form so the DECISION_RESULT= assignment on the next line never ran under zsh. Fix: prepend a portable nullglob shim to every bash block containing a for-glob loop: shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null Each command no-ops (stderr suppressed) in the shell that doesn't recognize it; the matching shell enables nullglob so an unmatched glob expands to nothing and the loop body is skipped cleanly. Verified locally: both zsh and bash now reach end-of-block (rc=0) on a no-match glob; bash matched-case behavior unchanged. 14 blocks across 7 files: verify-phase.md (4, incl. the decision-coverage gate), review.md, execute-phase.md, resume-project.md, complete-milestone.md, audit-milestone.md, gsd-integration-checker.md, gsd-plan-checker.md (3). Closes the zsh bypass of the #2770 fix. * chore(#2962): add changeset fragment * chore(#2962): backfill changeset PR number 3087 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7203011400 |
feat(#3072): ship the deferred MCP served catalog (resources + prompts) (#3083)
* test(#3072): add failing-first coverage for the mcp served catalog 55 input-class rows from the phase test matrix, across four suites: the catalog module over injected readFile/readDir seams, the protocol surface through handleMessage, the install-vs-catalog parity gate, and fast-check properties for uri round-trip, traversal refusal, and pagination partition. src/mcp-catalog.cts lands as a skeleton whose functions throw, so the suites fail on BEHAVIOR rather than on a missing module. The REASON enum is real so tests assert typed codes instead of message prose. Hostile coverage for the one client-controlled path surface (resources/read): dot-dot and backslash traversal, percent- and double-encoded traversal, absolute posix and windows paths, file:// scheme, null byte, symlink escape, unindexed sibling, non-string and empty uri, wrong root segment. IO faults are injected by monkeypatching the seam, never chmod 0o000 - root bypasses mode bits, so a permission-based test silently passes with zero coverage in root CI. Refs #3072 * feat(#3072): serve the mcp catalog as resources and prompts gsd-mcp-server now serves GSD's own content alongside its three tools: the workflow, reference and command tree as MCP resources (resources/list, cursor paginated, and resources/read over gsd://<segment>/<relpath> uris) and the 71 commands/gsd/*.md as MCP prompts keyed by bare command name. initialize advertises resources and prompts, and deliberately does not advertise subscribe or listChanged - the catalog is fixed for a server process lifetime, so declaring a notification we never send would be a lie a host acts on. Composition scope is SHARED, not re-declared. shouldCompose lives in src/mcp-catalog.cts and bin/install.js now imports it instead of carrying its own regex, so the served catalog and the installed file floor cannot drift on what gets composed. Proven behavior-preserving across all 2871 tracked paths plus windows-backslash, absolute and near-miss-prefix cases: zero mismatches. tests/mcp-catalog-parity.test.cjs asserts served text equals the installer composition-stage text over the real tree, with anti-vacuity guards requiring both a marker-bearing workflow and a non-composed file in the comparison set. Two measurements corrected the literal issue text. Composition is scoped to gsd-core/workflows/ only, because a reference or command that documents marker syntax with an unfenced example would otherwise be parsed as carrying a real marker and have that line lossily dropped. And parity is asserted at the composition stage rather than against an emitted runtime tree, since install applies per-runtime path rewrites afterwards and the catalog is host-agnostic, so byte equality with any one runtime would be false by construction. resources/read is the one client-controlled path surface and is guarded in two independent layers: the uri must be an exact key in the prebuilt index, which defeats every traversal string by construction, and the mapped path is then re-checked with validatePath so a symlink planted inside a root after indexing is still refused. Also fixes a real drift defect found while here: SERVER_VERSION was hardcoded 1.7.0 while the package is at 1.9.1. It now resolves lazily from VERSION or package.json, reusing the precedent in runtime-artifact-conversion. Closes #3072 * test(#3072): make the catalog parity gate drive the real installer Review found the parity gate vacuous: it never imported or spawned bin/install.js, and recomputed the installer side with the SAME shouldCompose and composeWorkflow the catalog calls internally. It therefore proved only that src/mcp-catalog.cts is self-consistent. The old row 52 compared shouldCompose against a regex literal frozen in the test file rather than against the installer at all. An inline divergent regex re-added to bin/install.js - the exact regression ADR-1671 asks this gate to catch - would have left the suite green. The gate now spawns a real bin/install.js and compares the composition DECISION, observed as gsd:section marker survival, against what the catalog serves for the same files. Marker presence is the right observable because the installer applies per-runtime path rewrites after composing while the catalog applies none, so raw byte equality between the two surfaces is false by construction and must not be asserted. Sensitivity was proven, not assumed: overlaying the shouldCompose export that bin/install.js imports so it always returns false makes a real spawned install leave autonomous.md's markers in place while the catalog still strips them, and the row 48 assertion diverges. Anti-vacuity guards are kept and extended - the comparison set must be non-empty, must contain a workflow that actually carries markers, must contain a file the predicate declines to compose, and the install must have emitted a non-zero file count. The marker-documenting reference case has no instance in the real tree, so it uses an overlay fixture built with the same technique workflow-fragments-emission.install.test.cjs already uses. Renamed to .install.test.cjs so it lands in the install suite it now belongs to. Refs #3072 * test(#3072): retarget the unknown-method assertion off a now-implemented method tests/gsd-mcp-server.test.cjs used 'resources/read' as its example of an UNKNOWN JSON-RPC method. The served catalog implements that method, so it now returns -32602 (no uri supplied) rather than -32601. The remote runner caught it deterministically on both linux lanes: -32602 !== -32601. The test's intent is still correct and worth keeping, so it is corrected rather than deleted or weakened. It now uses 'resources/subscribe', which the server deliberately does not implement and deliberately does not advertise in initialize's capabilities, because it never sends the corresponding notification. That turns the assertion into a real contract - the advertised capability surface and the implemented method surface agree - instead of an arbitrary method name a future feature could invalidate the same way. Swept the rest of the suite for other assertions pinning the newly implemented methods; this was the only one. Refs #3072 * chore(#3072): backfill changeset PR number 3083 * test(#3072): make the catalog fake fs separator-agnostic for windows CI caught this on windows-latest (22 and 24): every catalog fixture indexed ZERO entries, surfaced by the anti-vacuity guards as 'fixture catalog must actually index resources for this property to mean anything'. Mechanism: makeFakeFs keyed its dirMap/fileMap on POSIX-joined paths (${root}/${rel}), while production buildCatalog looks paths up with path.join, which is backslash-separated on Windows. Every lookup missed, tryReadDir returned null, and the catalog came back empty. Production is NOT at fault and is unchanged. The same CI run proves it: on windows-latest the real-filesystem tests all passed, including 'installer composition decision matches the served catalog for every file in the real installed tree' and the row-51 non-vacuity proof against a real spawned installer. A real Windows fs accepts both separators; the FAKE did not, so the fake was the unfaithful one and is what changed. Lookup keys are now normalized unconditionally with .replace(/\\/g,'/') in readDir and readFile - never path.sep-conditional, never platform-gated. The row-42/43 injected-fault wrappers got the same treatment, since they compared raw production paths against POSIX-literal fixtures. No assertion was weakened, and the anti-vacuity guards that caught this are untouched - they are the reason this surfaced as a loud failure instead of a suite that silently asserted nothing on Windows. Refs #3072 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2bc53baa03 |
fix(#2947): preserve preamble phase details when milestone section has none (#3084)
* test(#2947): milestone anchor must prefer heading with Phase details Row 1 of the test matrix: the failing-first regression test. When the phase-listing heading (## Phases) is NOT version-bearing but a later version-bearing progress heading (### v9.0 phase progress) exists, extractCurrentMilestone latches onto the progress heading and silently drops the phases (phase_count: 0, exit 0). Five cases: the regression, the version-bearing control (must keep working), the no-phase-details fallback, the closed-vs-open preference, and an end-to-end roadmap.analyze check. Reproduced locally against built lib + confirmed by maintainer triage (trek-e). The one-word control (## Phases -> ## v9.0 Phases) restores phase_count: 2. * fix(#2947): preserve preamble phase details when the milestone section has none Root cause was one layer deeper than the issue title: the anchor selection (selected = first non-closed version-bearing heading) is fine — the real drop happens in the preamble strip. When the phase list lives under a non-version-bearing ## Phases heading (the shipped greenfield template's own shape) and the selected version-bearing heading is a LATER progress/notes sub-heading with no ### Phase N: details of its own, the preamble strip removed every phase-detail heading from the pre-milestone region (intended to avoid duplication with a Phase Details section that does not exist here) — silently dropping all phases (phase_count: 0, exit 0, empty stderr). Fix: only strip preamble ### Phase N: headings when the selected milestone section (currentSection) actually contains its own phase details. When it does not, the preamble phases ARE this milestone's phases and must be preserved. Falls back to today's behavior (strip) whenever the selected section has phase details, so multi-milestone roadmaps with a dedicated Phase Details section (#730) are unaffected. Surgical: one conditional on the existing strip, no signature change, no change to computeSectionEnd or the #730 Phase Details append. Blast radius CRITICAL (84 upstream symbols) — the change is gated on currentSection's content so every existing roadmap that currently resolves phases correctly keeps doing so byte-identically. * chore(#2947): add changeset fragment * chore(#2947): backfill changeset PR number 3084 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
5719efbc6b |
fix(#2766): scan archived phases and read table-shaped Gaps/deferred entries (#3082)
* fix(#2766): scan archived phases and read table-shaped Gaps/deferred entries Three silent false negatives in cmdAuditUat and its readers, all failing in the reassuring (false-negative) direction — a UAT audit whose entire job is to catch leftover work reporting zero over real work: 1. Archived phases invisible (src/uat.cts cmdAuditUat): on milestone completion milestone.cts MOVES phase dirs into .planning/milestones/<version>-phases/ (archive-by-default since #1871), leaving .planning/phases/ empty or absent. Partial archive → false all-clear; full archive → hard error indistinguishable from a broken install. Fix: enumerate archived dirs via the canonical getArchivedPhaseDirs seam (phase-locator.cts); archived dirs deliberately bypass getMilestonePhaseFilter (which scopes to the CURRENT milestone — applying it to past-milestone dirs would discard every one and reinstate the bug). 2. Table-shaped deferred-items.md yielded zero items (splitGapsEntries keyed on bullet openers only; a GFM table row starts with |). Fix: union of bullet + numbered + table-row splits. 3. Table-shaped ## Gaps yielded zero items (same bullet-only splitter). Fix: same union walker. The table walker is deliberately NOT routed through parseMarkdownTable (ADR-2143 §3 — that reads only the first table and treats ragged/headerless shapes as errors, the wrong contract for a hand-written backstop table that must surface its rows). New additive archived_milestone field labels provenance. Fix authored by issue reporter gavin-ray and verified against the published tarball; maintainer triage (trek-e) confirmed all three findings. Cherry- picked onto fresh next after prior PR #2832 closed for staleness; re-verified under gsd-test + reviews. 21 regression tests including the negative direction (bullet-only unchanged, status: resolved still suppressed, empty phases dir still succeeds). * chore(#2766): backfill changeset PR number 3082 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
481ac7c71b |
fix(#2946): run milestone complete unstarted-phase guard independent of STATE (#3081)
* test(#2946): milestone complete unstarted-phase guard fails open on STATE desync Row 1 of the test matrix: the regression test that fails first. Adds seven cases to tests/milestone.test.cjs covering the desync, absent, no-file, --force-override, mismatch-WARNING, fresh-project-noop, and sentinel-skip behaviors. RED on next: the guard's entire scan is nested inside `if (stateVersion && stateVersion === version)`, so any STATE.md milestone: value that does not exactly equal the version argument skips the scan with no warning — functionally an implicit --force on a one-way-door operation. * fix(#2946): run milestone complete unstarted-phase guard independent of STATE The entire ROADMAP phase-directory scan was nested inside `if (stateVersion && stateVersion === version)`, so any STATE.md milestone: value that did not exactly string-equal the version argument — a desynced value, or no milestone: field at all — skipped the scan with no warning, functionally an implicit --force. The operation the guard fronts is a one-way door: ROADMAP.md and REQUIREMENTS.md are archived and phase directories are MOVED into .planning/milestones/<version>-phases/. The scan was already driven by the version argument through getMilestonePhaseFilter / extractCurrentMilestone; the STATE match was a redundant second gate that shadowed and broke it. Decouple: the scan now runs whenever --force is absent, and a present-but-mismatched STATE milestone: field emits a WARNING naming both values so the suspicious condition is visible rather than silent. A fresh project with no Phase headings in the scoped slice still yields an empty scan (no false positives) — the intent the STATE-match short-circuit was reaching for, now achieved by the scan itself. * docs(#2946): document milestone complete --force, --dry-run, and the unstarted-phase guard The CLI-TOOLS reference signature omitted --force and --dry-run entirely, and neither the unstarted-phase guard nor its override was documented anywhere user-facing. Add a flags table and a factual guard description to the Reference page (CLI-TOOLS.md), and a practical guard note to the /gsd-complete-milestone How-to (COMMANDS.md) covering what to do when the guard fires and the new STATE-mismatch WARNING (#2946). American English per CONTRIBUTING.md language policy. * fix(#2946): emit STATE-mismatch WARNING as JSON in --json-errors mode Follow-up to the guard decoupling: a structured caller using --json-errors parses stderr line-by-line as JSON, so the plain-text WARNING would break such a parser. Honor getJsonErrorMode() and emit a structured JSON object ({ ok, level, message }) in that mode, plain text otherwise — mirroring io.cts error()'s JSON shape. Addresses the isolated-review observation (~45% but credible, since --json-errors is a documented CLI flag). * fix(#2946): address review — drop JSON-mode WARNING scope creep, tighten test assertions Standards + spec review findings (code-review two-axis + isolated adversarial): 1. The --json-errors JSON WARNING branch (commit dee719404) was scope creep the issue never asked for, AND emitted ok:true for a suspicious-condition warning (a category error — a stderr JSON parser keying on ok would treat the suspicious state as success), AND its comment falsely claimed to mirror io.cts error()'s {ok:false,reason,message} shape. Dropped: the WARNING is now plain-text stderr only, matching the existing [gsd-tools] WARNING convention (state.cts). The issue asked for 'at minimum warn', not a structured JSON surface. 2. The WARNING test asserted on /WARNING/ regex (raw-text matching on stderr prose). Tightened to assert on the stable operator-facing tokens — the WARNING: marker and both version literals the operator must see — not the surrounding formatter prose. Added a paired negative test confirming no WARNING is emitted for an absent milestone: field (a missing declaration is a normal fresh-project state, not suspicious drift). * fix(#2946): sanitize STATE milestone value before stderr WARNING interpolation Security review (minor): stateVersion is read from a user-controlled file (STATE.md) and is not validated like the CLI version arg. Sanitize before interpolating into the WARNING — strip ANSI/control chars (/[\x00-\x1f\x7f]/g -> '?') and truncate to 80 chars — so a corrupted or hostile STATE.md cannot echo terminal escapes or secret-looking strings verbatim into a CI log or terminal aggregator (CONTRIBUTING.md security: secret-looking values in stderr). version is already constrained to [A-Za-z0-9._-] by ARCHIVE_VERSION_LABEL_RE upstream, so it needs no sanitization. * test(#2946): correct stale fixtures that relied on the guard being silently disabled Four pre-existing tests broke under the #2946 fix because their fixtures only passed thanks to the bug — the unstarted-phase guard was skipping on STATE mismatch, so fixtures with missing or non-matching phase directories slipped through. The tests exercise version-forwarding / version-scoping, not the guard, so give them legitimate directories: - milestone.test.cjs #3043: dirs were '103.old'/'104.old'/'108.new' (dot), which phaseTokenMatches rejects — renamed to hyphen form. The v3.6 stats scoping still yields 1 phase (getMilestonePhaseFilter scopes correctly); the guard now sees all three phases as having directories. - milestone-archive.test.cjs 'returns version in response data': ROADMAP listed Phase 1 but no directory was created. Added 01-foundation so the scan is satisfied. Per CONTRIBUTING.md, test-fixture corrections land as their own test: commit, not bundled into fix: (release hotfix cherry-pick routes by prefix). * chore(#2946): backfill changeset PR number 3081 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e6fcf14d02 |
fix(#2977): tolerate a leading UTF-8 BOM before the frontmatter fence (#3076)
* test(#2977): prove extractFrontmatter returns {} on a leading BOM Failing-first regression for #2977. extractFrontmatter's startsWith('---') byte-0 check fails on any leading byte, so a UTF-8 BOM (Windows PowerShell/Out-File, several editors) makes every frontmatter field silently disappear. Rows 1-2 assert BOM-prefixed frontmatter parses identically to no-BOM (incl. BOM+CRLF); Row 3 guards the no-frontmatter negative space; Row 4 covers STATE/PLAN/SUMMARY/UAT artifact shapes; Row 5 is the control. * fix(#2977): tolerate a leading UTF-8 BOM before the frontmatter fence extractFrontmatter's byte-0 startsWith('---') fence check failed on any leading byte, so a UTF-8 BOM (\uFEFF) — written by default by Windows PowerShell `>`/Out-File (PS 5.1) and several editors — made every frontmatter field silently disappear. STATE.md/ROADMAP.md/PLAN.md/UAT.md/SUMMARY.md all lost phase/status/name with no error, no warning. Strip a single leading BOM before the fence check. The rest of the function is unchanged — the BOM is one codepoint, and removing it restores byte-0 alignment. Negative space preserved: BOM + no-frontmatter and BOM + thematic-break Markdown both still return {} (no false diagnostic). Scope: BOM only; the generalized 'arbitrary content before the fence' fork (tolerate vs diagnose) is a separate product-intent decision, surfaced in the PR. * chore(#2977): add changeset fragment * chore(#2977): backfill changeset PR number 3076 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7e1004d89e |
test(#3056): add the in-process fault-injection adapter, and normalize execGit's shape (#3077)
* refactor(#3071): normalize execGit's call and result shape, unify ExecGitFn ExecGitFn was declared four times. Three were hand-copies of one signature and two of those were wrong: they typed exitCode as number|null when _spawnResult returns `result.status ?? 1` and can never yield null, weakened signal from NodeJS.Signals to string, and widened error from Error to unknown. Only verification.cts got it right, via `typeof execGit`. The root cause was a missing export: SpawnResultOutput was declared without `export`, so no other module could name the return type of execGit. Three authors independently hand-copied it instead. Exported now. Normalizing the type alone would have left the pressure that caused the divergence, so the function is normalized on both sides. It now ACCEPTS every call its consumers make — worktree-safety's declaration could not express an env-carrying call at all — and RETURNS every result code they need: timedOut moves into _spawnResult, so execGit, execNpm and execTool all carry it and the one extension that justified a separate type disappears. All four sites are now `typeof execGit` with nothing left to restate. timedOut reuses the existing isSpawnTimeout predicate introduced by #3050 rather than re-deriving it. That predicate checks error.code === 'ETIMEDOUT' only; the signal === 'SIGTERM' conjunct was deliberately dropped there because Windows does not reliably report SIGTERM and requiring it risks a false negative. There is no false-positive risk, and a test proves it: an externally-delivered SIGTERM leaves error null, so it is still not reported as a timeout. No dead null-checks surfaced. Every exitCode comparison in the two affected modules is === 0, !== 0 or === 128 — never a null guard — so the nullable declaration had never been written against. Closes #3071 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3056): add the in-process fault-injection adapter Adds tests/helpers/faulty-deps.cjs — makeFaultyGit() and withFaultyFs() — so a module's error branch can be driven deterministically and its degraded verdict asserted, instead of a counter-test that only proves the call did not throw. makeFaultyGit returns a value structurally assignable to `typeof execGit`, so one stub satisfies all four seams that the #3071 normalization collapsed into that single shape. A parity test drives the same stub through a real injectable entry point in each of worktree-safety, git-base-branch, worktree-base-ref and verification; it fails the moment any of them re-grows its own shape. Faults are scoped rather than global — by argv predicate and by call ordinal — because a fault adapter that faults everything looks like it works and proves nothing, and because verification.cts's two-call error handling needs to fault the second call only. Invocations are recorded so a test can assert an exact call count. The timeout fault carries error.code === 'ETIMEDOUT', and a test asserts the real isSpawnTimeout predicate matches it, so the fixture cannot drift from the production definition of a timeout. A companion test asserts an externally delivered SIGTERM with a null error is still NOT reported as a timeout — the false-positive guard for #3050's dropped conjunct. withFaultyFs restores in a finally so a throwing body still restores, patches only the named methods, and nests without clobbering an outer saved original. It never uses chmod: that no-ops under root, so the test would pass with zero coverage in root Docker/CI. The adapter is in-process via deps only. The Phase 1 process seam is documented as deliberately not a fault-injection surface — it cannot distinguish an injected timeout from a genuine bench OOM and would retry it. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3071): add changeset fragment for the execGit normalization Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3071): route the last two timeout checks through the shared predicate An isolated review found this branch had normalised the timeout verdict but left two callers still hand-rolling the fragile version of it. check-command-router's runBoundedShell computed `timedOut: r.signal === 'SIGTERM'` while the correctly-derived `r.timedOut` sat on the same result object. graphify's execGraphify branched on `result.signal === 'SIGTERM'`, with a comment asserting the very premise isSpawnTimeout exists to reject. Both fail in both directions. On Windows a genuine timeout is not reliably reported as SIGTERM, so the guard silently fails to fire — the false negative #3050 was raised for. And an externally-delivered SIGTERM is not a timeout at all, so the check also fires when it should not; isSpawnTimeout avoids that because `error` is null in that case and it keys on error.code. Both now read the derived `timedOut`, and graphify's comment states the actual rule instead of the fragile assumption. Also replaces a vacuous test: "execGitDefault now accepts env" never called execGitDefault (it is unexported), called execGit — whose signature already accepted env before this branch — and asserted only that exitCode was a number, which would pass whether or not the change under test existed. It now proves env reaches the child by asserting `git var GIT_EDITOR` returns the injected sentinel. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3071): make the graphify timeout fixture faithful to a real timeout The remote matrix failed on both Linux lanes: graphify's "returns exitCode 124 on timeout" got 1 instead of 124. The fixture was wrong, not the production change. It stubbed spawnSync as { status: null, signal: 'SIGTERM', error: undefined }. That is not a timeout. A real spawnSync timeout also sets error.code 'ETIMEDOUT'; a SIGTERM with no error is an externally delivered signal — a kill. The old `result.signal === 'SIGTERM'` check accepted it as a timeout, which is the false positive the shared predicate exists to reject, so this test was locking that bug in rather than guarding against it. The fixture now carries a real ETIMEDOUT error and all three original assertions pass unchanged. A counter-test is added alongside it: an externally delivered SIGTERM with no error must NOT be reported as a timeout. That is the assertion whose absence let the false positive live. Swept every SIGTERM/SIGKILL stub under tests/ for the same unfaithful shape. No other instance: the worktree-safety, worktree-base-ref and commit-staging fixtures already set ETIMEDOUT, and the remaining hits are either deliberate external-kill tests or feed code that never consults timedOut. Two sites keep their own signal check deliberately and are NOT changed: capability-source.cts:1301,1386 fail closed on ANY abnormal termination, which is correct — reading timedOut there would stop it failing closed on a kill and let it parse stdout from a killed process. Their reason strings, and check-latest-version.cjs:115, label any signal as "timed out", which is imprecise wording over a correct verdict, not a silent failure. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3071): backfill changeset pr number to 3077 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
5fd5c81042 |
test(#3055): add the process seam so a subprocess timeout is expressible as data (#3066)
* test(#3055): add the process seam and route runGsdTools through it Adds tests/helpers/process-seam.cjs — runNode/runGit/runHook over spawnSync, each returning a typed discriminated union { outcome, exitCode, stdout, stderr, timedOut, signal, killed, code }. Every call is timeout-bounded; there is no unbounded path. runGsdTools becomes an adapter over the seam. Its legacy { success, output, error, exitCode } shape and retry-once-on-kill behaviour are preserved byte-identically, so none of its 136 caller files change. Outcome discrimination was corrected against probed runtime behaviour rather than assumption: a timeout and a maxBuffer overflow are identical on both status (null) and signal (SIGTERM), and differ only by code (ETIMEDOUT vs ENOBUFS). Overflow is therefore classified before timeout. This fixes a live defect — the previous isKilled() treated an overflow as a kill, retried it for a second full 60s run, and then reported "host OOM or scheduler contention" for a child that had merely printed too much. Also widens the ESLint tests glob from tests/**/*.test.cjs to tests/**/*.cjs, which brought 31 previously unlinted shared helpers under the same rules their sibling test files already obey, and fixes the 5 violations that surfaced — including a bare npm invocation without shell:true in tests/helpers/emitted-runtime.cjs (DEFECT.WINDOWS-TEST-PORTABILITY), now routed through the existing portable runNpm helper. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3055): migrate every local spawn wrapper onto the process seam Replaces the spawn body of all 25 local runHook/runGuard/runGate definitions with a call to tests/helpers/process-seam.cjs. Each wrapper keeps its name, parameter list, return shape and post-processing (JSON parse, ANSI strip, env sanitising, field extraction) — only the spawn mechanism changes, so no test assertion moves. The 4 bash-driven wrappers use the seam's explicit `interpreter` option rather than a fourth primitive; it is explicit rather than inferred from the file extension, because guessing an interpreter from a path fails silently when a script's name does not match its shebang. Seven wrappers were previously unbounded and now carry an explicit timeout sized to what each actually runs, not the seam default. Two of those seven (gsd-write-guard, lint-docs-command-form) were absent from the issue's inventory entirely and were found by scanning after the migration. Adds the CONTEXT.md `### Process seam` glossary entry and a CONTRIBUTING.md reference section covering the three primitives, the discriminated union, and the two rules the seam enforces. Scope disclosure recorded in the phase design notes: the issue scoped three identifier names. A scan for local helpers that spawn AND return the spawn result finds 113 across 82 names, 71 of them unbounded, plus 122 unbounded direct git call sites. This change bounds 25 of those. The remaining surface is the same defect class and is NOT closed by this PR. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3055): classify an externally-killed child as KILLED, not EXITED Blocker found in this branch's own diff, independently confirmed by an isolated reviewer. A child killed by an external signal — a genuine bench OOM kill — makes spawnSync return { status: null, signal: 'SIGKILL' } with NO .error field. The seam's "no error implies EXITED" rule therefore classified it as a clean exit, and runGsdTools returned { success: false, exitCode: 1 } without retrying. That silently defeated the #969 kill-discrimination for precisely the case it was built for: the old isKilled() fired on `signal != null`, retried once, then threw a labelled resource-starvation error. A real OOM would have been reported as an ordinary assertion failure. Adds a fifth outcome, KILLED, for "no error but a signal is set", and makes the adapter retry on TIMED_OUT or KILLED — reproducing the old `killed || signal != null || code === 'ETIMEDOUT'` condition exactly. SPAWN_FAILED still does not retry (matching the old behaviour, where signal was null). BUFFER_OVERFLOW still does not retry, which remains a deliberate divergence: the old code retried it because signal was SIGTERM, burning a second 60s run on a child that had merely printed too much. All five outcomes verified against the live runtime rather than assumed: SIGKILL -> killed, exit 0/7 -> exited, timeout -> timed_out (ETIMEDOUT), >1MB stdout -> buffer_overflow (ENOBUFS). Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3055): address standards-review findings on this branch Three findings from the standards axis of the review, all in this branch's own diff. The CONTEXT.md glossary entry this branch introduced was already stale on the branch's own last commit: it enumerated a 4-member OUTCOME while the code had 5, because the KILLED fix did not update it. That is precisely the drift the "module changes update Domain-terms" gate exists to catch, so the entry now lists all five and explains KILLED. api-coverage-gate-e2e compared an outcome against the raw string 'exited' rather than OUTCOME.EXITED, the only such outlier; the enum is now imported and used. A sweep for the other four outcome literals found no further comparison sites. Three call sites hand the literal bash flag '-c' to the seam's first parameter, which the JSDoc described as an absolute script path. Rather than add a fourth primitive, the contract is corrected to match reality: the parameter is renamed `target` and documented as the first argv element handed to the interpreter — normally a script path, but for an interpreter invoked with an inline program it may be that interpreter's own flag. No behaviour change. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3055): assert the cross-platform timeout contract, not the macOS one The remote runner failed on both Linux lanes (node 22 and node 24, identical) while the same tests passed locally on macOS. Two assertions encoded a platform-specific behaviour as a cross-platform guarantee. When spawnSync times out, macOS preserves the child's partial stdout/stderr; Linux discards it and returns empty strings. Verified on node v26.5.1 both ways. The seam passes through whatever spawnSync hands it and cannot manufacture output that was discarded, so the production code was correct — the tests were wrong. Both tests now assert the guarantee the seam actually makes on every platform: outcome TIMED_OUT, timedOut true, and stdout/stderr always being strings rather than undefined or a Buffer. The partial-content assertions are retained behind an explicit process.platform === 'darwin' guard so the macOS coverage is not lost, and the first test is renamed to say what it now guarantees. This is the failure mode the remote matrix exists to catch: local macOS verification would have shipped it. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3055): classify a failed spawn as SPAWN_FAILED, not a timeout Windows CI caught two defects the Linux matrix could not. tests/context-predicates-query.test.cjs passes a 32K-char argv value. On Windows that exceeds the argv limit and spawnSync fails with code ENAMETOOLONG, signal null, status null. The seam's fallback rule — "otherwise, status === null implies TIMED_OUT" — swallowed it, so the adapter retried a spawn that can never succeed and then threw the resource-starvation error. The old isKilled() returned false for that shape and returned an ordinary failure result. TIMED_OUT is now identified positively: code === 'ETIMEDOUT' OR signal is set. Anything else carrying an error is SPAWN_FAILED, which covers ENAMETOOLONG, E2BIG, EACCES and ENOENT alike. The signal clause is what keeps a platform whose timeout errno differs classified correctly, so the greedy catch-all is no longer needed. The second defect is a contract regression I introduced and had claimed otherwise. That same test asserts `typeof r.exitCode === 'number'`, and toLegacyShape was returning null for BUFFER_OVERFLOW and SPAWN_FAILED, so the assertion failed on type. The old code returned `err.status ?? 1` on every non-retried failure path. The adapter now returns 1 again for both, and the comment claiming "never coerced to exitCode:1, unlike the pre-seam helper" is retracted: the seam keeps the richer truth (exitCode null plus a distinct outcome), the legacy adapter keeps the old numeric contract its callers actually depend on. Verified on this host: a 4MB argv yields E2BIG -> SPAWN_FAILED; ENOENT -> SPAWN_FAILED; timeout -> TIMED_OUT; >1MB stdout -> BUFFER_OVERFLOW; SIGKILL -> KILLED; clean exit -> EXITED. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
59b74c4e7d |
fix(#2945): roll back the REQUIREMENTS checkbox when the traceability row rejects the write (#3073)
* test(#2945): prove phase complete checkbox ignores traceability row rejection Failing-first regression for #2945. cmdPhaseComplete flips the REQUIREMENTS.md checkbox unconditionally and never rolls back when the traceability row exists but rejects the Status write (Deferred/Blocked), so a deferred requirement reads as shipped. Rows 1-2 assert the checkbox stays [ ] for Deferred/Blocked; Row 3 guards the forward-status flip; Row 4 covers the no-row boundary. * fix(#2945): roll back the REQUIREMENTS checkbox when the traceability row rejects the write cmdPhaseComplete's inline requirement-write loop flipped the REQUIREMENTS.md checkbox unconditionally and kept the flip when the traceability row existed but rejected the Status write (Out/Deferred/Blocked), so a deferred requirement read as shipped — the #2788 defect-2 fix was written into cmdRequirementsMarkComplete (milestone.cts) only, never the phase.cts inline copy. Port the rollback: capture beforeCheckbox, track tableHit in the updateTraceabilityCell callback, and when reqUpdate.ok && !tableHit (row existed but rejected the write), restore beforeCheckbox. The two surfaces can no longer silently diverge. Forward-status rows (Pending/In Progress/Gaps Found) still flip + advance; absent rows still flip (nothing to disagree with). * chore(#2945): add changeset fragment * chore(#2945): backfill changeset PR number 3073 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c97f5debb9 |
fix(#2949): exclude sentinel phase ids from stage-3 next-phase candidacy (#3070)
* test(#2949): prove phase complete stage-3 admits 0.x backlog sentinels Failing-first regression for #2949. cmdPhaseComplete's stage-3 lowest-outstanding loop has no sentinel filter, so completing the last real phase with an unchecked 0.x backlog row present selects the sentinel as next_phase, corrupting STATE.md. Row 1 asserts the 0.x sentinel is not selected; Row 2 guards the #2028 real-lower- phase out-of-order behavior; Rows 3-4 cover STATE desync and the checked-sentinel boundary. * fix(#2949): exclude sentinel phase ids from stage-3 next-phase candidacy cmdPhaseComplete's stage-3 lowest-outstanding-override loop (#2028) had no sentinel filter, so completing the last real phase with an unchecked 0.x backlog row present selected the sentinel as next_phase — comparePhaseNum("0.1","12") === -12 sorts it below every real phase — corrupting STATE.md and desyncing current_phase from current_phase_name. Add !isSentinelPhaseId(cbm[2]) to the stage-3 condition, reusing the existing zero-caller predicate (SENTINEL_RANGES = [0, 999]) so both sentinel ranges are excluded. A real lower-numbered outstanding phase is not a sentinel and is still selected, preserving #2028's out-of-order-completion behavior. Stage-3 only: PR #2815 (in-flight, #2786) covers stages 1-2; the two PRs touch disjoint code. * fix(#2949): compare next_phase numerically in Row 2 (handles padded/unpadded) Row 2's assertion /09/.test(next_phase) failed because the CLI returns the unpadded "9", not "09". Compare numerically (parseInt === 9) so the assertion holds for either form. * fix(#2949): mark Phase 11 complete in Row 1 so only the 0.x sentinel is unchecked Row 1's fixture left Phase 11 unchecked, so completing Phase 12 correctly selected Phase 11 as the real lower outstanding phase (is_last_phase=false) — the assertion is_last_phase===true was wrong for that fixture, not the code. Mark Phase 11 [x] so the ONLY unchecked lower row is the 0.x sentinel, which is the actual #2949 scenario. Confirmed locally: with Phase 11 checked + the fix, completing 12 yields is_last_phase=true, next_phase=null (sentinel excluded). * chore(#2949): add changeset fragment * chore(#2949): backfill changeset PR number 3070 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8f75e27554 |
fix(#3045): fail closed when an executor dispatch drops its resolved isolation (#3069)
* feat(#3045): deny an executor dispatch that drops its isolation flag Every isolation gate already resolved correctly. The resolved value then reached the executor through a prose instruction telling the model to substitute it into a call the model composes itself, and nothing verified the substitution. When it was dropped, the executor edited and committed in the user's primary checkout with no consent and no warning. A prose backstop would be the same class of artifact as the defect, so this is a shipped PreToolUse hook on the Agent tool. It fires at the instant of the call rather than being read once at the top of a workflow, which is the only placement the model cannot skip. The guard is inert unless it can positively establish that this is a GSD project, that the project resolves to harness isolation, and that the dispatch targets an executor. A non-GSD repo has no invariant to enforce. Where it cannot read the configuration at all, it denies rather than assuming, with its own reason -- a guard that cannot verify must not answer safe. A malformed payload allows rather than throwing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3045): extend the isolation guard to Cursor Cursor is the second of only two runtimes that resolve harness isolation, so shipping the guard for Claude alone left half the exposed surface unguarded while the changeset implied it was covered. The two runtimes fail differently. On Claude the harness flag is a per-dispatch kwarg the model must copy into a call it composes, and the defect is that it can be dropped. On Cursor the flag is --worktree, which applies to the whole session, and the subagent-start payload carries no isolation field at all. There is no flag to check, so the guard verifies the effective state instead: whether the workspace is genuinely running outside the user's primary checkout. That is a stronger check than the Claude one because it tests reality rather than intent, and it is commented so nobody later rewrites it into a flag check. Isolation is established two ways, either sufficient: the workspace resolves to a linked git worktree, or it sits under the worktree root Cursor manages. The second matters because a directory Cursor placed there is a legitimate isolated session even before it becomes a distinct git worktree, where linkage alone would report no repository. Detecting linkage required a new primitive rather than the existing context resolver. That resolver short-circuits on finding a local .planning directory before it ever compares the git directory to the common one -- and an isolation worktree normally has its own checked-out .planning. Reusing it would have read a correctly isolated session as unisolated and denied it, which is the failure direction that gets a guard switched off. The comparison is now its own shortcut-free function that the resolver delegates to after its own shortcut, so existing behavior is unchanged, and the case that would have broken is pinned. The subagent type is checked before any configuration is read, so an unreadable config cannot deny a dispatch this guard would never have enforced against. The input-schema comment on the Cursor hook documented only the fields common to every event and omitted the ones specific to this one. That omission cost a halt during this work; it now documents both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3045): enforce the resolved dispatch decision, not the host capability The guard keyed on the registry's dispatch.isolation, which says only that a runtime is CAPABLE of harness worktrees. The decision that actually governs a dispatch is the one the workflow resolves after gating, and that legitimately comes out as sequential in three documented cases: a project setting use_worktrees false, a per-plan submodule intersection, and the base-check auto-degrade. The workflow tells the model to omit the flag in exactly those cases, and the guard was denying every one of them. The third case matters most. The preceding fix made the base-check degrade on git timeouts and a missing git binary, where it had previously answered "safe". That correction is right, and it means a transient hang now degrades to sequential far more often than before -- so the two changes composed into a trap where the workflow behaved exactly as designed and the guard blocked it. The workflow already resolves isolation in shell, deterministically, which is what makes it a trustworthy source in a way the model-authored call is not. It now records that resolved value through a dedicated verb, and both guards read it first. A fresh record is authoritative, so sequential dispatches pass untouched. Absent or stale, the guards fall back to the capability check combined with the project's use_worktrees setting, which still covers the case that never reaches the workflow. Also widened the matcher to accept Task alongside Agent, since a host that names the tool Task would otherwise leave the guard silently inert while implying coverage; stopped assuming Claude when no runtime is declared, which is the shipped default and would have demanded a Claude-only argument elsewhere; and made a non-git project inert rather than denied, since advising a worktree session is not actionable without a repository. The original diagnosis never modeled sequential mode as legitimate. That omission is what let this through, and it is now recorded there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3045): record at resolution and bind the record to its dispatch Two independent reviews converged on the same failure: the guard was fail-open in a default install, so it did not catch the defect it exists to catch. A shipped project carries no runtime key, which made "runtime not confidently known" the common case rather than a corner one. A record asserting that isolation was required but carrying no flag then fell through to a capability lookup that answered "none", and the dispatch was allowed. The flag itself only arrived from a second shell block -- the same block a model dropping the argument would also skip. A test had pinned that behavior as intended. The record is now written by the resolver, as an unavoidable consequence of asking for the value, rather than by a step the model is told in prose to go and run. A guard against a prose-carried value cannot itself depend on prose. Mode, flag and identifiers are written together and atomically, so the flagless window is gone, and a record asserting isolation with no resolvable flag now denies instead of degrading. Runtime is also resolved from the installer's own recorded default, which makes confident resolution the normal case. The per-plan submodule gate degrades after the phase-level decision and never re-recorded, so a plan that legitimately ran sequentially was denied against a still-fresh phase record. It now records its own, scoped to the plan. A record also authorized any dispatch for four hours. One phase degrading to sequential could silently license an unisolated dispatch in the next. Records now carry phase and plan, the guards require them to match, and the window is minutes rather than hours -- the resolver rewrites it before every dispatch, so a long window bought nothing and only widened the hole. The flag validator rejected any value beginning with two dashes, which is exactly the form Cursor and Windsurf declare, so their real value could never have been stored. Writer and reader also derived the record path differently and diverged inside a linked worktree without local planning state. The predictable path remains a way to silence the control without leaving a trace in the diff. It grants no access an agent with shell does not already have, so it is documented as accepted rather than redesigned around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3045): correct the staleness boundary and unmask a vacuous parity test The remote runner returned twenty failures. One was a real production defect the boundary case existed to catch: a record whose age exactly equalled the staleness window was treated as fresh, so it stayed authoritative for one tick past its own expiry. Freshness is now strictly inside the window. The parity test meant to stop the two guards' executor lists from drifting could never have failed. Its project fixture was a bare directory rather than a repository, so the non-git inert branch answered before the executor list was ever consulted. It asserted agreement it never actually measured. The fixture is now a real repository, like every sibling in the file. A test also asserted that Windsurf declares the worktree flag. It does not -- Windsurf resolves to no isolation by design, having no named concurrent dispatch to isolate. The test claimed a registry fact that was never true, and a comment in the resolver repeated it. Both corrected, and the test now proves what it should have all along: that the parser accepts any bare flag value, rather than one runtime's supposed value. The new guard was missing from the bundled-hook whitelist, which is the surface that decides what actually ships, and the per-plan gate had gained calls to the launcher without the preamble those calls require. The changeset carried parenthetical product descriptions the purity rule forbids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3045): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3045): make the guard tests hold on Windows Two tests redirect HOME to control where the installer-persisted runtime default is read from. Node resolves the home directory from USERPROFILE on Windows and never consults HOME, so both silently read the real runner profile, found no recorded runtime, and asserted against a project the hook had not recognised. The production code was already correct in asking the platform rather than the variable; only the tests were wrong to assume one variable answers everywhere. The helpers now mirror the override onto both. The symlink spoofing test also created a directory symlink unconditionally, which needs elevated privileges on Windows. It survived on this runner, but it would fail on any host without them, so the creation is now attempted and the test skips explicitly when it cannot be done -- a bare return would have counted as a pass and hidden the gap. Skipping alone would have left the platform uncovered, so the behaviour it proves is now also driven in-process through an injected realpath, following the seam already used for the clock. That case no longer depends on privileges at all, and the end-to-end test keeps its original assertions wherever symlinks work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c9dd4aa951 |
fix(#2940): preserve codex config.toml content after the GSD marker on update (#3067)
* test(#2940): prove gsd-update discards codex config after the GSD marker Failing-first regression for #2940. mergeCodexConfig Case 2 (marker present) preserves content before the marker but discards everything from the marker to EOF, so user/Codex-CLI settings ([model], [mcp_servers.*], [profiles.*]) added after a fresh install are wiped on every update. Rows 1-2 assert trailing user content survives; rows 3-7 cover idempotency, the #2406 leaked-section non-regression, the [agents] namespace, both regions, and the zero-trailing boundary. * fix(#2940): preserve codex config.toml content after the GSD marker on update mergeCodexConfig Case 2 (marker present) preserved content before the marker but discarded everything from the marker to EOF, so user/Codex-CLI settings added after a fresh install ([model], [mcp_servers.*], [profiles.*]) were wiped on every gsd-update. Route the trailing region through the existing stripLeakedGsdCodexSections, which removes GSD's own managed/leaked sections (the bare [agents] table GSD regenerates, legacy [agents.gsd-*], [[agents]]) while preserving genuine user TOML. The marker comment line (and the optional codex_hooks ownership line) is stripped from the trailing region first so it is not duplicated alongside the regenerated block. #2406's de-dup still holds (leaked sections after the marker are still stripped), and re-merge is idempotent. * fix(#2940): use CRLF-safe regex in regression test (lint:ci no-crlf-fragile-split) The Row 7 blank-line assertion used a bare \n which trips local/no-crlf-fragile-split under Windows git-autocrlf. Use \r?\n. * fix(#2940): correct Row 5 to valid single-[agents] TOML shape The isolated adversarial review found Row 5 (bareAgentsAfterMarkerHandled) constructed TWO [agents] tables (the GSD-managed one + a user trailing one), which is invalid TOML — a duplicate table definition. extractCodexUserAgentsScalars reads only the first [agents] and stripLeakedGsdCodexSections removes ALL bare [agents] tables, so the user's max_threads would be silently dropped. The valid, realistic shape is the user folding max_threads INTO the managed [agents] block (which the existing spliceCodexAgentsScalars path preserves) plus a separate trailing [model]. Reword Row 5 to that shape. A duplicate-[agents] input is invalid TOML Codex itself rejects, so it is out of scope for this fix. * chore(#2940): add changeset fragment * chore(#2940): backfill changeset PR number 3067 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c899f5ada3 |
chore(#3065): build the deterministic load-bearing-fragment contract gate (#3068)
* test(#3065): build the load-bearing contract gate ADR-1671 promised Epic #1671 Phase 7. A post-merge audit of every promise in ADR-1671 against the merged tree found one mitigation asserted-but-absent and two stale records. ADR-1671 names exactly one correctness risk — trimming a load-bearing fragment, with the recorded history of a paraphrased META.RULE causing agent violations — and #2931 amended its mitigation to a deterministic contract gate that proves no load-bearing fragment was omitted or shrunk, treats a floored fragment as a success, and asserts the isolate prefix survives byte-identical, with an explicit anti-vacuity rule. That gate did not exist. What existed was tests/context-composer.test.cjs: synthetic unit tests of the composeWithinBudget primitive over invented fragments, asserting nothing about real declared strategies. The ADR asserted a mitigation that was never built, which is the promised-but-not-built shape the epic's own coverage discipline exists to catch. The gate derives its load-bearing set from declared verbatim strategies rather than a hand-maintained list, so it cannot go stale as upstream changes. It sweeps budgets from 4x total down to a quarter of total and asserts at every step that no load-bearing id appears in omitted or shrunk, that isolatePrefix is byte-identical, and that hardFailed is surfaced rather than silently passed. Both anti-vacuity guards are EXECUTABLE, not comments. One proves the empty load-bearing set guard actually throws. The other proves a sweep that never applies pressure is rejected — because a gate that only ever runs unpressured is exactly how the original mitigation went missing without anyone noticing. Measured: underPressure true at 6 of 7 budgets, false only at 4x total. Three ADR records corrected in the same change, all doc-vs-reality drift: - Decision item 2 describes a composer that trims by priority to fit a measured per-runtime cap. composeWorkflow in fact passes MAX_SAFE_INTEGER with every fragment verbatim (both verified in source), so no trimming happens there; the emitted-byte cap is a separate measure-and-fail gate and Windsurf's limit a bespoke truncation. The wording described an option as shipped behavior. - flag:--converge never reached a terminal state. #2992 withheld six atoms; five were resolved explicitly. This one was resolved in code by reusing state:plan-strategy-converge but recorded nowhere — the same gap #2995 closed for flag:--verify-only, and I closed five of six. - The open-questions list enumerated three questions while two Resolved-by blocks resolved an unlisted Question 4. It is now listed. Refs #3065 * fix(#3065): make the gate assert over production, not a copy of it The isolated review found a blocker, and it was fatal to the gate's purpose: it hand-copied applyBudget's fragment array into the test, so flipping a strategy in src/prompt-budget.cts — say roadmap from verbatim to drop — would leave the gate computing from its own untouched copy and still passing. A guard built as an instance of the very divergence class it exists to prevent (DEFECT.GENERATIVE-FIX) is worse than no guard, because it reports green. Fixed by eliminating the duplicate rather than adding a parity assertion, the same resolution used for the FAMILIES table in #2996. applyBudget's inline construction is extracted to an exported buildBudgetFragments(), which both applyBudget and the gate now call; the 1024 plan floor is exported as PLAN_FLOOR_CHARS instead of being re-declared in the test. The extraction is pure — verified behavior-preserving at budget=2000: hardFailed false, omitted ['context'], projectMd shrunk, plan truncation ~27.8%, all headers present. There is no longer a second copy to diverge from. Also fixed a vacuous assertion the same review caught: isolatePrefix was pinned across the sweep, but no production fragment sets isolate:true, so the value is always '' and the check could never fail. The pinning assertion stays, with an honest comment that nothing in production sets it today, and a second test now constructs an isolate:true fragment set and proves the prefix is non-empty and byte-identical across a roomy and a severely tight budget — which is what makes the first assertion capable of detecting a real change. Refs #3065 * chore(#3065): backfill changeset pr number to 3068 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
83a26ed1dc |
fix(#2939): honor the declared depth budget in shouldFlattenDispatch (#3063)
* test(#2939): prove shouldFlattenDispatch ignores the depth budget Failing-first regression for #2939. shouldFlattenDispatch checks only background+backgroundDispatch, never nested/subagentToolkit/maxDepth, so a maxDepth:1 descriptor (no room for a bg orchestrator plus a leaf) is told it may background. Row 1 (codex-like, maxDepth:1) asserts true (flatten) and fails today; rows 2/3 guard the unchanged depth-sufficient cases. * fix(#2939): honor the declared depth budget in shouldFlattenDispatch shouldFlattenDispatch checked only background+backgroundDispatch, never nested/subagentToolkit/maxDepth, so a maxDepth:1 descriptor (no room for a backgrounded orchestrator plus a delegated leaf) was told it may background — producing a depth-2 tree (Codex MultiAgent V2) the declared contract cannot support. canBackground now ALSO requires nested:true + subagentToolkit:"full" + a depth budget > 1 (or unbounded -1), reusing the exact predicate shape from bin/install.js _normalizeDispatchCallSpan and matching degradationFor's treatment of maxDepth===1 as flat. Non-finite/missing maxDepth fails closed to flatten. Correct the two existing pins that asserted the buggy output (bare {bg,bgDispatch} now fail-closes on missing depth; the codex-like maxDepth:1 pin flips to flatten) and add a maxDepth:2 negative-space row. * fix(#2939): propagate depth-aware flatten to all pinned descriptors + tests The isolated adversarial review found the depth-aware predicate reclassifies codex/kimi/kimi-code (previously background-eligible under the two-field rule) to flatten — the correct behavior, since each lacks what a backgrounded nesting orchestrator needs: - codex: maxDepth:1 (no room for a depth-2 leaf) - kimi: nested:false (cannot host a nesting orchestrator) - kimi-code: subagentToolkit:'built-in-only' (cannot delegate to full subagents) Only cursor (maxDepth:2) remains background-eligible. Update the three test files that pinned the old contract (host-integration-descriptors EXPECTED_FLATTEN, kimi-upgrades UPGRADE 2, trae-imperative-reference), and align the unbounded convention to maxDepth < 0 (matching degradationFor/negotiateHostCapabilities) with an accurate docstring noting the deliberate nested-check addition over _normalizeDispatchCallSpan. * fix(#2939): update dispatch-should-flatten CLI query pins for codex The depth-aware rule (a0ad0f680) reclassifies codex (maxDepth:1) to flatten, but command-routing-hub.test.cjs exercises the contract through the CLI query route (runGsdTools query dispatch-should-flatten), not a direct shouldFlattenDispatch call — so neither the reviewer's caller-search nor a grep for the symbol found it; only the full gsd-test matrix did. Update the codex query assertions to shouldFlatten=true (maxDepth:1 insufficient), preserving cursor (maxDepth:2 → false) and the backgroundDispatch:true descriptor field. * chore(#2939): add changeset fragment pr:0 placeholder backfilled with the real PR number once the PR exists. * fix(#2939): rephrase changeset for product-name-purity + opencode flatten pin Two failures from the full gsd-test matrix on the prior sha: 1. product-name-purity: changeset fragments must not include parenthetical product descriptions (they render verbatim into CHANGELOG.md). 'Codex (and kimi/kimi-code)' tripped it — rephrase to lead with the behavior, naming runtimes inline without the parenthetical. lint:ci changeset-lint does not catch this; only the test does. 2. opencode-imperative-reference: the #2087-retraction pin flipped only the two background booleans and asserted shouldFlatten:false. Under #2939 that is no longer sufficient (opencode lacks nested + full toolkit + depth budget), so the retracted axes now correctly flatten — update the pin to true with rationale. * chore(#2939): backfill changeset PR number 3063 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c547e73a71 |
fix(#2927): merge installed overlay reviewer lanes into review-lane invocation (#3062)
* test(#2927): prove overlay reviewer lanes are invisible to review-lane Failing-first regression for #2927. routeReviewLane builds its lane map from the static REVIEWER_LANES array only, so an installed overlay reviewer lane (role:"reviewer" capability) is roster-visible and disclosed at install but never selectable, plannable, or invocable. The test exercises a pure mergeReviewerLanes(firstParty, registry) helper that does not exist yet, so every row fails at the require(). * fix(#2927): merge installed overlay reviewer lanes into review-lane invocation routeReviewLane built its lane map exclusively from the frozen first-party REVIEWER_LANES array, so an installed, consented third-party reviewer lane (role:"reviewer" capability) was roster-visible and disclosed at install but never selectable, plannable, or invocable — sections/flags/plan/invoke all shared the one static map. Add a pure, total mergeReviewerLanes(firstParty, registry) helper (src/review-lane-descriptor.cts) implementing ADR-2782 D8: first-party ∪ installed overlay reviewer bodies, first-party winning on slug collision. The overlay body is field-identical to ReviewerLane per ADR-2782 D1 ("no translation layer"), so the helper MERGES rather than PROJECTS. Malformed overlays (missing/non-object body, empty or grammar-invalid slug) are skipped, never thrown — one bad third-party manifest cannot take the first-party lanes down. routeReviewLane consults loadRegistry({includeInstalled:true}) and degrades to the static set on any load failure. * test(#2927): add CLI-seam coverage for the wiring defect + normalize slug Two findings from the isolated adversarial review: 1. The test matrix's rows 9-10 (acceptance criteria #1-#3: overlay appears in sections/flags and plan resolves ok) were documented as covered but had no backing tests. The eight pure-helper tests would stay green if the one-line routeReviewLane wiring were reverted — the actual defect this PR closes had no regression guard. Add real end-to-end CLI tests that install a global-scope role:"reviewer" overlay and assert review-lane sections/flags/plan see it through loadRegistry -> mergeReviewerLanes. 2. mergeReviewerLanes trimmed the slug for the map key but stored the body with its untrimmed slug, diverging from deriveReviewerSlugs (which trims before adding to the roster). Normalize the stored lane's slug to the trimmed value so the two surfaces agree on the canonical key. * test(#2927): correct CLI-seam fixtures for reviewer manifest shape Two corrections from local CLI smoke-testing before the verification run: 1. role:"reviewer" manifests must omit feature-only fields (skills/agents/ steps/contributions/gates/hooks/runtimeCompat) — the validator rejects them. Match the shipped capabilities/lm-studio shape. 2. The plan subcommand renders an ARRAY of {slug,ok,section,transport,...} (it strips the nested invocation plan object), so assert on the array element, not a top-level object. Also drop the malformed-flag-filter assertion: the capability validator enforces flag grammar at install time, so a lane with a malformed flag cannot be installed and never reaches the flags shape filter (which is defense-in-depth, not independently reachable). * fix(#2927): drop unnecessary type assertion flagged by lint:ci The `body as object` cast inside the spread is redundant — body is already narrowed to object by the preceding typeof check. eslint no-unnecessary-type- assertion flagged it; lint:ci is a merge gate. * chore(#2927): add changeset fragment pr:0 placeholder backfilled with the real PR number once the PR exists. * fix(#2927): access runGsdTools result via .output in CLI-seam tests runGsdTools returns {success, output, exitCode, error}, not a string. The CLI tests (rows 9-10) passed the result object directly to JSON.parse/.split, which string-coerced to "[object Object]" and threw under gsd-test (3 failures). My local smoke test ran the CLI directly (string stdout), so it missed this — the helper wraps execFileSync and returns a result object. Access .output and assert .success explicitly, matching the established capability-cli.test.cjs convention. * chore(#2927): backfill changeset PR number 3062 --------- Co-authored-by: sim <sim@local> |