e2eed1c58ae030acc159be7e4daead43b085c614
2523 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
0xdhx
|
e2eed1c58a |
test(#2665): ship the hermeticity guard at report level, not fatal
Its first CI run found PRE-EXISTING leaks on the Windows lane — C:\Users\runneradmin\.claude\gsd-core and skills\gsd-dev-preferences — with all 1196 Windows tests otherwise passing. os.homedir() reads USERPROFILE on Windows, and ~190 test sites across 31 files sandbox HOME alone, so the suite has been installing GSD into the runner's real home directory invisibly. That is exactly the class the guard exists to surface, and exactly the class this PR's review said CI could never catch. It is also a different defect from the one #2665 closes, and too large to fold in here. A brand-new gate that immediately reds an unrelated lane gets bypassed or reverted rather than obeyed, so the guard reports by default and fails only under GSD_STRICT_LIVE_CONFIG_GUARD=1. This is the repo's own established ratchet, not a hedge: the local/no-source-grep ESLint rule shipped at `warn` and was promoted to `error` after its cleanup sweep (ADR 452). Promote this the same way once the USERPROFILE sweep lands. |
||
|
0xdhx
|
5863351281 |
test(#2665): separator-safe containment in the regression assertion
startsWith(ambientConfigDir) also matches a sibling like <tmp>/ambient-live-config-2, so it can report a leak that did not happen. Use path.relative and check for '..' or an absolute result, the repo's usual shape. The readdirSync assertion already carried the test, so this is cosmetic. Addresses review finding: Nit 9. |
||
|
0xdhx
|
771980b661 |
test(#2665): restore fallback-branch coverage in the #2003 regression
This PR fixed the test's real defect -- it compared the child's answer against the PARENT process's getGlobalConfigDir(), two different environments, agreeing only because the child inherited the developer's ambient CLAUDE_CONFIG_DIR -- but fixed it by INJECTING CLAUDE_CONFIG_DIR, which moved the test onto the env-first branch and silently dropped the only coverage #2003 had of the home-derived fallback. Sandbox HOME/USERPROFILE and leave the config vars blank instead: the expectation stays test-controlled AND the branch under test is unchanged. Drops the notStrictEqual against the codex dir. It could not fail whenever the strictEqual on the line above passed. Addresses review finding: Minor 7. |
||
|
0xdhx
|
a4efa4deda |
test(#2665): cover the derivation and the in-process scrub
The single regression test exercised CLAUDE_CONFIG_DIR only, so deleting GSD_RUNTIME or CODEX_HOME from any literal broke nothing -- a mutation of 24 of the 27 added key-value pairs survived. Four tests here: every configHome env var the registry declares is scrubbed; every scrubbed key is blanked rather than merely present; the four non-registry vars are named explicitly so deleting one is a failure rather than a silent narrowing; and scrubConfigLocationEnv round-trips both a set and an unset var (restoring an originally-unset var as '' would itself be a leak). Both derivation tests assert a floor on the registry first, so a renamed registry shape fails loudly instead of making the assertions vacuously true. Negative-controlled against the hand-written 3-key list this PR shipped: the parity test fails there and names all 19 missing vars. Addresses review findings: Major 6, Minor 8. |
||
|
0xdhx
|
a02462e050 |
test(#2665): fail the suite when it writes into a live config dir
The recurrence guard, and #2665's own "Optional hardening". This class is silent by construction: TEST_ENV_BASE cannot see an in-process caller, and CI cannot see the class at all because CI never has these env vars set. It damages the developer's machine and reports nothing -- which is how two prior authors each diagnosed it and fixed only the instance in front of them. run-tests.cjs snapshots GSD's install footprint in every live runtime config dir before the suite and re-checks it after, failing the run on a create or a modify. Roots come from the product's own getGlobalConfigDir, so the guard watches wherever the product actually points, including through an ambient var. Scope is ownership-based, not whole-root: the top-level install footprint plus gsd-prefixed children of dirs GSD shares with the host agent. A config root like ~/.claude is shared, and watching it wholesale would false-positive on the host's own history.jsonl or settings.json -- a guard that cries wolf gets disabled, and then catches nothing. The prefix test is load-bearing: the first version watched only the three top-level entries and MISSED a real leak into skills/gsd-*. It earned its place immediately -- it is what found the fifth in-process leak in runtime-artifact-layout.test.cjs, which no amount of reading the review would have surfaced. Known gap documented in the module: a write to a file GSD does not own is out of scope by construction. Lives in scripts/, deliberately NOT scripts/lib/ -- the installer copies that dir into every user's config dir wholesale while uninstall removes only an allowlist, so a test-only module there would ship to users and survive uninstall. Addresses review finding: Minor 8. |
||
|
0xdhx
|
36f4f9479a |
fix(#2665): scrub config-location env on raw spawns that sandbox only HOME
Same class as the in-process leak, on the child-spawn side. These call spawnSync
directly rather than through runGsdTools, so TEST_ENV_BASE never applies to them
and an ambient config-location var survives into the child.
- install-runtime-artifacts: the dev-preferences writer resolved env-first and
wrote SKILL.md into the live config dir instead of its tmp HOME. This also
fixes a test that FAILS today on any machine with CLAUDE_CONFIG_DIR set.
- issue-766: the `claude` CLI is third-party and bootstraps its own config into
whatever CLAUDE_CONFIG_DIR names, so even a bare --version probe wrote there.
It gets an explicit throwaway dir rather than a blank value -- GSD's resolvers
treat '' as falsy and fall back to the home dir, but a third-party binary
offers no such guarantee (blanking it produced a stray backups/ in the repo
root).
These two were the last writers standing between the suite and #2665's stated
acceptance criterion.
|
||
|
0xdhx
|
466db2f1a3 |
fix(#2665): scrub config-location env on the parent for in-process install()
The blocker the review said decides this PR. These tests call the real installer IN-PROCESS with only HOME/USERPROFILE sandboxed. getGlobalConfigDir is env-first, so an ambient CLAUDE_CONFIG_DIR beats the sandbox and a complete global install -- agents/, commands/, skills/, gsd-core/, manifest, settings -- lands in the developer's live config dir. No child-env scrub can reach it; only clearing the parent's env can. The review named two describe blocks in install.test.cjs. A sweep for the shape found four there (bug #3571 and bug #3288 each appear twice in the file), and the post-suite guard added later in this series found a fifth in runtime-artifact-layout.test.cjs, which the review did not name. All five now save/clear/restore via scrubConfigLocationEnv(). Verified: codebuddy-install, cline-install and codex-config already guard their own config-location var around in-process install(), so the class is closed. Addresses review finding: Blocker 1. |
||
|
0xdhx
|
bc5c362610 |
fix(#2665): replace the hand-synced TEST_ENV_BASE copies with the canonical import
Eight declarations were kept in sync by hand with no parity assertion. The drift was already in the tree: api-coverage-gate-e2e and representative-corpus declared TERM_SESSION, but the real variable is TERM_SESSION_ID, so both scrubbed nothing for that slot and leaked TERM_SESSION_ID into every child. Deleting the copies removes the dead key with them -- there is no longer a second place to get wrong. run-tests-harness keeps its local session-identity literal: that helper mirrors the production runGsdTools to prove its CONTRACT, so it must not re-import what it is testing. The config-LOCATION keys are a safety scrub rather than part of that contract, so it spreads the canonical derived set and keeps the rest local. Addresses review findings: Blocker 2, Blocker 3. |
||
|
0xdhx
|
0f97a26f76 |
fix(#2665): derive the config-location scrub set from the capability registry
TEST_ENV_BASE listed three config-location vars by hand. The resolver reaches 25: every runtime descriptor's configHome.env, plus GROK_AGENTS_HOME (a hardcoded branch of getGlobalConfigDir), GSD_RUNTIME, and GSD_PROJECT/GSD_WORKSTREAM (planningDir). A hand-written list can only ever be as complete as the author's recall, and every one of those resolvers is env-FIRST, so a missing key is a live escape hatch rather than a cosmetic gap -- which is why this bug has now been diagnosed three times. Derive the set from the same registry the resolver reads. The scrub list becomes structurally incapable of being narrower than the surface it guards: adding a capability that declares a new configHome env var extends it in the same commit. Also export TEST_ENV_BASE (a parity test could not previously import the canonical copy) and add scrubConfigLocationEnv(), the in-process counterpart -- TEST_ENV_BASE only ever reaches child processes. Addresses review findings: Blocker 2, Major 4 (GSD_WORKSTREAM/GSD_PROJECT), Major 5 (XDG_CONFIG_HOME). |
||
|
0xdhx
|
08021b02c0 |
fix(#2665): scrub config-location env vars in every TEST_ENV_BASE declaration
TEST_ENV_BASE blanks session-identity variables but none of the three that
decide WHERE a child process writes: CLAUDE_CONFIG_DIR, GSD_RUNTIME and
CODEX_HOME. The config-home resolver is env-first (runtime-homes.cts, the
dot-home case consults the env var before the home-derived fallback), so an
ambient CLAUDE_CONFIG_DIR in the developer's shell beats a call site that
sandboxes only HOME. The suite then writes into the developer's real config
directory -- including a registered skill under <configDir>/skills/ whose
body carries behavioural directives that load into later sessions.
Blank all three alongside the session-identity vars. `...env` still spreads
last, so the five call sites that already constrain these locally keep
winning with their explicit values.
TEST_ENV_BASE is re-declared in nine files, so the three lines are added
nine times rather than once. Consolidating the nine into a single exported
constant -- and fixing the TERM_SESSION / TERM_SESSION_ID drift between the
copies -- is deliberately left out of this change; see the PR body.
One call site needed adjusting. capability-state.test.cjs's
`capability state --runtime claude` CLI test passed no env at all and
compared the CHILD's resolved config dir against the PARENT process's
getGlobalConfigDir('claude'). That agreed only because the child inherited
the developer's ambient CLAUDE_CONFIG_DIR -- i.e. it passed *because of*
the leak. It now redirects both runtime homes into the sandbox and asserts
against values the test controls, so it is hermetic with the variable set
or unset.
Regression case folded into the owning module's test file rather than a new
bug-NNNN file, per scripts/lint-regression-test-names.cjs. It sets the
variable on the PARENT process, which is the actual vector; setting it in
the per-call env argument would exercise a path that was never broken.
|
||
|
Tom Boucher
|
343835facc |
refactor(#3183): route live-plan counting through scanPhasePlans (#3199)
* refactor(#3183): route live-plan counting through scanPhasePlans scanPhasePlans becomes the sole owner of the live-plan derivation. Twenty-one independent re-derivations across seven modules now route through it, and scripts/lint-plan-count-drift.cjs reports zero, scanning the whole repo rather than an allowlist (ADR-3180 Decision 4a). The epic scoped this at three copies. A whole-repo guard found twenty-six sites across nine files, so Phase 1 absorbs every live-plan re-derivation and Phase 3 narrows to window plus sentinel enumeration. Two sites are exempt with a documented reason rather than a bare allowlist: audit.cts scans one quick task's own directory for a single completion record, and gsd2-import.cts reads a foreign GSD-2 tasks/ layout during a one-time import. Neither is a phase directory. scanPhasePlans gains allPlanFiles (pre-supersession) alongside planFiles so one owner answers both questions: verify.cts's numbering-gap check wants every plan on disk, its pairing check wants the live set. Both fields are additive. Highest-severity fix: cmdPhasePlanIndex, which feeds execute-phase wave scheduling, was scheduling status:superseded plans into waves and reporting zero plans for the post-#3139 nested layout. filterPlanFiles and filterSummaryFiles are deleted; getPhaseFileStats orphaned them and only their own tests still called them. New leaf module src/planning-scope.cts carries the frozen SCOPE discriminator, with its six-gate ripple closed: gitignore, inventory manifest, INVENTORY.md and the CONTEXT.md glossary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * docs(#3183): amend ADR-3180 for the Phase 1/3 boundary re-slice The contract held; the phase boundary did not. The whole-repo drift guard found 26 re-derivations across 9 files against the epic's estimate of 3, and cmdProgressRender re-derives both enumeration and plan counting on adjacent lines, so DW4 was unsatisfiable within Phase 1's original file scope. Records the amended scope, scanPhasePlans's new allPlanFiles field, findOrphanSummaries, the two documented exemptions, the re-derived Tier-2 table, and the describeNonCanonicalPlans trap for later phases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * fix(#3183): complete the canonical pairing rule and gate the naming diagnostic The remote runner went red with 13 deterministic failures on both lanes, and they were right: replacing verify.cts's canonicalPlanStem pairing with summaryCandidates dropped a case the bespoke rule covered. A plan carrying a descriptive slug after its id (68-01-scaffolding-PLAN.md) pairs with its canonical-stem summary (68-01-SUMMARY.md), and summaryCandidates generated no such candidate, so the plan read unsummarized. The fix is to complete the one rule rather than restore a second: summaryCandidates gains a canonical-id candidate, narrowed to fire only when an id pair was actually extracted. countMatchedSummaries, findUnsummarizedPlans and findOrphanSummaries all inherit it. The two-plans-one-summary collision behaviour of the original rule is preserved deliberately and documented in place. Second defect, independently root-caused while verifying: routing the #2893 naming diagnostic through scanPhasePlans exposed it to the loose /PLAN/i fallback, which is correct for counting and wrong for a naming check — a non-canonically-named file was accepted as a valid plan and the diagnostic went silent. cmdPhasesList, cmdFindPhase and cmdPhasePlanIndex now intersect with a strict isCanonicalPlanFile predicate before reporting names. Same class as the describeNonCanonicalPlans trap already recorded in ADR-3180: a question about file naming wants the physical, strictly-matched set; only a question about outstanding work wants the live set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * chore(#3183): register planning-scope.cjs in the eslint migration list tests/repo-invariants.test.cjs asserts every bin/lib/*.cjs is linted xor ignored per its ADR-457 migration state. The new planning-scope module closed five of the six .cts ripple gates - gitignore, inventory manifest, INVENTORY.md and the CONTEXT.md glossary - but not eslint, because that one is enforced by a test rather than by lint:ci, so the local pipeline stayed green while it was missing. Generated from src/planning-scope.cts, so the .cjs is ignored and the .cts is linted, matching every other migrated module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * fix(#3183): replace the plan-count drift detector with a literal tokenizer CodeQL reported 4 high-severity js/redos alerts on REGEX_LITERAL_MD_RE, the backtracking regex that finds "a regex literal mentioning PLAN/SUMMARY and an escaped \.md". Five review rounds found it had two defects, not one: - EXPONENTIAL, then CUBIC. Its "any char" atom `(?:\\.|[^/\r\n])` let a `\.` pair be consumed either as one escape or as two class characters, which is exponential backtracking: 27,464ms on `"/\.mdplan" + "\.".repeat(28) + "X"`. Excluding `\` from the class killed that but left a cubic path — 23ms at N=200, 172ms at N=400, 1362ms at N=800 on `"/" + "PLAN\.md".repeat(N)` with no closing `/`. This guard is the last stage of `npm run lint:ci`, which CI runs on fork pull requests, so a crafted src/*.cts could stall the job. - A DETECTION HOLE. A character class holding a bare, unescaped `/` — e.g. `/SUMMARY[^/]*\.md$/`, an ordinary path-excluding filter — terminated the literal at that `/`, so the scan never reached `\.md` and the guard missed it entirely. (Classes holding an ESCAPED `\/` were already matched; the tests cover those separately as parity, not as regressions.) Both defects have one root cause: regex-literal grammar — `\x` escapes, and `/` inside `[...]` not terminating — is not expressible in a backtracking regex. So the detector is now a tokenizer, not a regex. readRegexLiteralAt reads the literal at a given `/` in a single left-to-right pass with no backtracking, treating escapes as two-character units and suppressing the `/` terminator inside a character class. findRegexLiteralMdMatch restarts it at every `/` on the line, preserving the old "find anywhere" behaviour; MAX_REGEX_LITERAL_LEN (400) bounds each read — including the trailing-flag scan — which keeps the whole-line cost linear. Results: cubic shape flat at 0.06-0.39ms out to N=3200 (25KB), exponential shape 0.01ms at 28 reps and 0.00ms at 64, and the bare-`/` class shapes are now caught. Differential against the old regex over 28,474 lines (those matching FILENAME_TEST_RE but not PLAN_SUMMARY_LITERAL_RE, across src/tests/scripts/ gsd-core/bin/eslint-rules, excluding 265 lines with >6 backslashes on which the old regex hangs): 6 differences, all the tokenizer returning the fuller or newly-correct literal, 0 old-only misses. The `\.md` token stays case-insensitive, matching the `/i` the old regex carried. Also closes three holes in the same new file: - walk() tested entry.isFile(), false for a symlink, so a symlinked src/*.cts was silently unscanned — an evasion of a guard whose stated principle (ADR-3180 Decision 4a) is whole-repo discovery with no allowlist. It now resolves symlinks, but confined: file links must resolve inside the repo root, directory links inside the scanned dir itself. Every sibling drift guard in scripts/ uses the Dirent classification and never follows links, so following them unconfined would have made this the only linter able to read outside the tree — on fork PRs an arbitrary out-of-repo read whose matched fragments reach a public CI log. The narrower directory rule additionally stops `src/up -> ..` from sweeping the whole repo, and the skip list is now checked against resolved paths so `src/g -> ../.git` cannot reach .git/** or node_modules/**. Real paths are de-duplicated and files reported canonically, so a symlink alias cannot shift which FUNCTION_SCOPED_EXEMPTIONS key applies. - Both the reported fragment and the reported FILE PATH are attacker- controlled source text written straight to a CI log, and git permits control bytes in a filename. Both are now escaped — C0/C1/DEL plus the bidi and zero-width controls — so a crafted literal or filename cannot recolour the log, overwrite a line with CR, or fabricate a line that looks like this guard's own success output. Regression coverage in tests/plan-count-single-owner.test.cjs: a child-process probe over both pathological shapes (catastrophic backtracking is synchronous and would freeze the suite rather than fail one test), the bare-`/` class shapes verified to fail against the parent-commit blob, root-confinement tests covering the outside-file, outside-directory, cycle, broken-link and duplicate cases, direct isInsideRoot coverage including the sibling-prefix case that a bare startsWith would let through, sanitizeForReport coverage, and limit-1/limit/limit+1 coverage of MAX_REGEX_LITERAL_LEN derived from the exported constant. The earlier structural assertion was dropped — it checked for the substring `[^/`, which respelling the class as `[^\r\n/]` defeats while staying exponential. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * chore(#3183): backfill changeset PR number Restores b77931869, which a force-push during the ReDoS remediation dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
9faacc0c15 |
test(#3148): bound the long tail and delete the unbounded-spawn allowlist (#3192)
* test(#3148): bound the long tail and delete the allowlist Migrates the final 170 unbounded sync spawn sites across 49 files, then removes the allowlist entirely. local/no-unbounded-spawn now runs with no exemption surface across tests/**: there is no file to add a name to. drift-detection's throw-native git() helper routes to gitOrThrow -- bare runGit would have taken 16 call sites quiet on failure. commands.test.cjs has two independently-scoped runGsdTools/runCli helpers, one already bounded and one not; they are kept distinct rather than unified, the same trap as the two same-named git() helpers in Wave 1. runNpm's bound was erasable. Its options spread callerOptions after the defaults, so an explicit timeout:undefined silently dropped the 180000ms bound -- the rule flagged it and was right; it was not a false positive. Fixed by destructuring with a default, with a test that fails when the default is removed. Two sites stay on a raw spawn with an explicit timeout because the seam cannot express them: one needs shell:true for npm.cmd on Windows, one redirects stdout to a real fd. Both are the rule's own documented second option, not an escape from it. Closure verified rather than asserted: the derivation scan reports 0 unbounded spawn helpers and 0 unbounded direct git call sites, and a temporary file carrying an unbounded spawn still errors with the allowlist gone. Closes #3064. * test(#3148): close a hole in the guard's own eslint-disable ban The ban listed only the top level of tests/, so it was blind to 37 .cjs files under tests/helpers, qa, observability, fixtures and dispatch. With the allowlist deleted this test is the sole remaining way to detect someone silencing the rule inline, so the gap was load-bearing: a nested file could carry an unbounded spawn plus an eslint-disable and pass everything. Proven before and after. A probe planted under tests/helpers with both was invisible to the guard and clean under eslint; after making the listing recursive the guard fails on it. The scanned set goes from 771 files to 808. Pre-existing since the guard shipped, but this wave is what promoted it to sole defense, so it is fixed here rather than filed. Also converts the last hand-rolled throw check to throwIfFailed and the last re-derived legacy shape to compose toLegacyResult, which makes the epic's none-remain claim true rather than nearly true. toLegacyResult itself is not widened -- eight callers depend on its shape and one consumer does not justify changing a shared contract. * fix(#3148): correct seam incoherence at the bound and a slow review-lane error path Two real failures from the remote runner, both fixed at the cause. The seam could return outcome TIMED_OUT together with exitCode 0. At the exact bound spawnSync reports ETIMEDOUT while the child has already exited with a real status, and toSeamResult classified on the error code while passing status straight through -- an incoherent pair its own boundary test was written to catch, and did. A status that is not null is direct evidence the child exited on its own, so it now decides the outcome before the error-code branches run. process-seam.cjs was deliberately untouched by every earlier wave; this is a defect in the module itself, kept surgical, with a unit test that fails against the old logic. review-lane with an unknown subcommand fell through to its usage error only after loading the capability registry and building a per-lane plan, which spawns one child process per lane -- up to twelve. The error path took ~1288ms instead of ~119ms, and under bench load it outran a caller's spawn timeout and was killed before writing anything, which is the empty stdout and stderr CI saw. It now fails fast before any of that work begins. This is the epic's first production change. It is user-facing, so it carries a changeset rather than a no-changelog label. * test(#3148): replace a real-race timeout test with a deterministic one E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a warm container git finishes first, spawnSync returns status 0 with no error at all, the seam correctly classifies EXITED, and gitOrThrow correctly does not throw -- so the test failed on both lanes. A probe confirms a genuine timeout always carries status null, so this was never the seam misbehaving. Raising the bound would only lengthen the odds, which is the same defect with better luck. The test now drives gitOrThrow against a stubbed runGit that returns a synthetic TIMED_OUT result, so it asserts exactly what it always meant to -- that a timeout propagates as a throw -- with no timing dependence. Five consecutive runs are identical where the old one varied. I wrote this test in Wave 0; it is a real-race test by construction and CLAUDE.md says to replace those rather than re-run them. * chore(#3148): backfill changeset PR number 3192 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
664d49e513 |
docs(#3182): ADR-3180 — planning semantic model single owner (#3196)
* docs(#3182): ADR-3180 — planning semantic model single owner Phase 0 design lock for epic #3180. Names one canonical owner per semantic derivation, specifies the frozen-enum scope contract that distinguishes a genuinely-empty computation from a truncated or unscoped one, and locks the drift-guard contract. Ships no production code. Phases 1-5 execute against this ADR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * test(#3182): prune real issue 3182 from phantom-ref guard, fix empty-list regex The guard's own header documents that its list rots: entries are phantom only until the repo's shared issue/PR counter reaches them, and once the counter passes an entry it must be deleted. Creating the Phase-0 sub-issue advanced the counter past 3182, so the guard began rejecting a legitimate citation of a real issue - the failure its header already records happening twice, with PRs 2551 and 2361. 3182 was the last entry, and removing it exposed a latent bug: the regex builder interpolated the list unconditionally, so an empty list yields (?:#(?:)\b)|(?:issues/(?:)\b), whose empty alternation matches every issue reference in the repo. Following the file's own maintenance instruction would have turned a green guard into one failing on nearly every file. buildRefRe() now returns null for an empty list and is exported, with boundary coverage at 0/1/2 entries plus word-boundary and bare-digit negative cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
3f349e551d |
fix(#3024): route sync-skills through the shipped gsd-tools instead of an unshipped install.js (#3195)
* fix(#3024): sync-skills workflow uses gsd-tools query skills-root instead of unshipped install.js The sync-skills workflow Step 2 shelled out to gsd-core/bin/install.js --skills-root, but install.js is not shipped in installed trees (only in the npm tarball root bin/). Every /gsd-update --sync invocation failed with MODULE_NOT_FOUND. Fix: added 'gsd-tools query skills-root <runtime>' subcommand (gsd-tools IS shipped) that calls the same getGlobalSkillsBase function install.js used. Updated the workflow to call gsd_run query skills-root instead of the dead install.js path. Also documented the #3025 verbatim-cp limitation in Step 5 with a workaround. * test(#3024): failing-first guards for the three defects in the adopted fix The cherry-picked commit came from an aborted run that never executed its own tests. Its raw-path assertion fails as written, which is the clearest evidence the work never reached verification. Covers: - --raw must emit a bare path, not JSON (output() takes a third rawValue arg that routeSkillsRoot omits, so the raw branch never fires) - an unknown, empty, whitespace, traversing, or metacharacter-bearing runtime must be rejected, not silently resolved to claude's skills root - sync-skills.md must contain zero references to the unshipped install.js, including the guard's remediation text — the issue's second reported defect - parity across every runtime in the registry, not three hardcoded ones, so the two entry points cannot drift Also converts the adopted tests off a hand-rolled spawnSync onto the bounded process seam, per CONTRIBUTING. Fails before the fix. Verified via the remote runner. * fix(#3024): make the skills-root query actually work and reach non-Claude runtimes The cherry-picked commit never ran its own tests. Six defects, all fixed here. --raw was ignored: output() is output(result, raw, rawValue) and the third argument was omitted, so the raw branch never fired and the workflow captured a JSON blob as SRC_SKILLS_ROOT. Every downstream cp -r then resolved against a nonexistent path — the command would have shipped still broken. An unknown runtime silently resolved to claude's skills root, because getGlobalSkillsBase falls back rather than returning null, leaving the existing === null guard dead. The runtime id is now validated at the CLI boundary against the shipped registry, so a typo'd --from/--to fails instead of reading from or writing into the wrong runtime's tree. getGlobalSkillsBase('vscode') threw a raw TypeError. vscode is non-installable by descriptor, so it has no skills root — null is the answer, not a crash. The resolver now short-circuits configHome.kind 'none', which also fixes the same latent crash in install.js --skills-root vscode. Every caller already gates on === null. sync-skills.md used gsd_run WITHOUT the canonical launcher preamble, so gsd_run was undefined on non-Claude runtimes — the fix would have been dead in exactly the place the original bug bit. Preamble propagated via sync-runtime-launcher. Also registers skills-root in TOP_LEVEL_USAGE (the help/dispatch parity guard caught it), removes the last two install.js references including the guard's remediation text (the issue's second reported defect), and updates the stale assertion that still described the removed contract. Verified on the remote runner. * fix(#3024): align the documented runtime list with the registry and gate both entry points Isolated review returned BLOCK on two findings. The workflow's Supported-runtimes list and its --to all expansion named grok and gemini, neither of which is a registered runtime. Once this branch added validation, --to all — a documented first-class feature — aborted. The list was hand-copied prose shadowing the registry, so correcting it alone would drift again; a parity assertion now fails in BOTH directions if the doc and the registry disagree. vscode is excluded by name: it is installSurface 'none', so syncing skills to it is meaningless and would abort. bin/install.js --skills-root reached getGlobalSkillsBase with no own-property gate, so --skills-root __proto__ silently resolved to claude's skills root. This branch had just hardened the OTHER entry point to the same function; leaving one of two parallel surfaces open is the same divergence class as the first finding. Both now call one shared isRegisteredRuntimeId() rather than a copied check, and the parity test covers the hostile ids so the two can never disagree again. Also guards the workflow's root resolution: neither command substitution checked its exit status and only the source had an existence guard, so a failed destination resolution left DEST_ROOT empty and turned rm -rf "$DEST_ROOT/$SKILL" into an absolute path at filesystem root. Both resolutions are now checked, and Step 5 requires both roots to be non-empty and absolute before any destructive command. Verified on the remote runner. * test(#3024): anchor the runtime-list parity extractor to the list span The extractor captured (.+) to end of line, so it swallowed the em-dash prose that explains the vscode exclusion — and that sentence contains backticked `runtimes` and `null`, which is where the three phantom ids came from. The documented list was correct; the test was reading its own explanation back as data. Anchored to the id-list span. Both directions still fail as intended: proven by injecting a bogus id and by removing a registered one. * test(#3024): anchor the --to all extractor and fail loudly on empty captures The workflow has three TO_RUNTIMES= assignments and the regex matched the first one — an empty array initializer at line 28 — so the extractor captured nothing and the assertion diffed [] against 18 ids as if that were data. That is the same failure twice, so the fix is the general one: every extractor in this test now asserts it captured a plausible list before comparing, naming which extractor found nothing and what it was looking for. An extractor that silently yields [] is a confident wrong answer, and a parity guard that reports it as a data mismatch teaches the reader to loosen the assertion. Verified against the real workflow and against doctored copies with each target construct removed, plus both teeth directions. * fix(#3024): merge duplicate process-seam import after rebase The rebase applied cleanly but left runNode declared twice: next had gained its own import of the seam while this branch added one carrying OUTCOME. A clean rebase is not a correct one — the file no longer parsed. Merged into a single import providing both. * fix(#3024): bind DEST_ROOT per destination instead of a dangling map Step 2 stored each destination's root into DEST_SKILLS_ROOTS, which nothing ever read, while Steps 3 and 5 used a scalar DEST_ROOT that nothing ever assigned. The array was also never declare -A'd, so on bash 3.2 — macOS system bash, which this repo supports — every destination collapsed onto index 0. The absolute-path guard added earlier was the only thing standing between that and rm -rf "/$SKILL"; it turned a silent disaster into a hard stop, but the feature still could not complete. Each destination now binds its own DEST_ROOT where it is used, and the unread map is gone rather than replaced. Step 2 keeps eager validation, so a bad runtime id in a multi-destination --to aborts before any destination is written rather than after some already have been. Verified on bash 3.2 with a two-destination run binding distinct roots, and with a bad id aborting before any destructive call. * fix(#3024): restore grok support broken by the registry gate The registry gate added earlier rejected grok, and that was my error. I confirmed grok was absent from the capability registry and concluded the hardcoded branch was dead — without checking what it resolved to. It resolves to ~/.agents/skills, a real grok-specific path, exactly as the pre-fix workflow documented ('grok uses the ~/.agents layout'), and there is a support discussion doc for it. So a working, documented runtime silently lost --skills-root and sync-skills support as a side effect of prototype-pollution hardening — and the parity test I added locked that in as correct. gemini is the one that really was dead: it fell through to CLAUDE's skills root, so rejecting it is right and it stays rejected, as do bogus ids, __proto__, empty, whitespace and traversal. The validator's real question is 'does this id have a genuine runtime-specific resolution', not 'is it in the registry map'. Registry membership was a proxy that happened to miss grok. Legacy non-registry runtimes with dedicated resolution branches are now a named, documented set; enumerating every hardcoded branch in getGlobalConfigDir against the registry confirms grok is the only one. The new tests assert grok resolves UNDER .agents and specifically not to claude's root. Allow-listing an id proves nothing about whether it resolves correctly — that assertion is what would have caught my mistake. Also uses the shared PROBE_TIMEOUT_MS instead of a duplicate literal, and guards Step 3's DEST_ROOT re-resolution, which contradicted the file's own stated guarantee. Verified on the remote runner. * test(#3024): guard against LEGACY_NON_REGISTRY_RUNTIME_IDS drifting The named legacy set is a second hand-maintained proxy for the same predicate the registry check got wrong — 'does this id resolve runtime-specifically'. Nothing stopped a third hardcoded branch being added to getGlobalConfigDir without updating the Set, reproducing the exact class of bug that broke grok. Production stays explicit and greppable; the test derives the truth instead. It resolves a sentinel id to learn the generic fallback, classifies every candidate against it, and fails in both directions — an id resolving runtime-specifically that is in neither the registry nor the Set, or a Set entry that no longer earns its exemption. The failure message names the remedy. Confirms grok resolves runtime-specifically and gemini does not, which is the distinction the original registry check could not see. Also reverts the shared-timeout swap: SKILLS_ROOT_PROBE_TIMEOUT_MS is pre-existing on next and arrived by rebase, so changing it here was scope creep into another issue's territory. Verified on the remote runner. * chore(#3024): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cbd180c5cd |
test(#3147): bound the lint/changeset/docs cluster onto the process seam (#3181)
* test(#3147): bound the lint/changeset/docs cluster onto the process seam Migrates 69 unbounded sync spawn sites across 24 files. Allowlist 73 to 49. Two shared helpers move: tests/helpers/graphify.cjs (6 importing suites) and tests/fixtures/index.cjs, whose three quoted-argument shell strings became single argv elements rather than whitespace splits. changeset-lint's throw-native git() helper routes to gitOrThrow; migrating it to bare runGit would have silently swallowed a failure that is loud today. ingest-docs goes the other way -- its catch never rethrew, it degraded failure into data every call site asserts on, so throwIfFailed would have thrown where the original returned. The design doc said otherwise and was corrected. tsconfig-noemit runs a real tsc --noEmit and takes a bespoke 180000ms per the ensure-runtime-build precedent, not the 30000ms build-hooks norm -- that norm is for a file copy, and sizing against a label rather than the work is the same error in the opposite direction. * test(#3147): add toLegacyResult and settle review findings The seam exposed a throwing adapter (throwIfFailed) but no non-throwing one, so eight files independently re-derived the same unwrap back to the legacy {status, stdout, stderr} shape. That is the third time this epic produced N copies of one mechanism -- seven throw wrappers in Wave 1, fifty-two timeout constants in Wave 2, eight result adapters here. The pattern is that whenever the seam does not expose a mechanism, every suite re-derives it. toLegacyResult now sits beside throwIfFailed, with its own tests. Two sites are deliberately NOT converted: changeset-cli's runRender and runRenderIn return {status, report, stderr} from parsed JSON and never a raw stdout, so they are a different shape family. lint-legacy-dir-name keeps its local GUARD_TIMEOUT_MS: 30000 matches the build norm numerically but bounds a lint probe, not hooks bundling, and importing it would encode a coincidence as a relationship. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3fac6e629f |
test(#3145): bound the installer/runtime cluster onto the process seam (#3176)
* test(#3145): bound the installer/runtime cluster onto the process seam Migrates 156 unbounded sync spawn sites across 47 files. Allowlist 120 to 73. Timeouts are sized from evidence already in the tree rather than a house default, because this wave spawns installers rather than git plumbing and an undersized bound does not catch a hang -- it manufactures CI flake, which is worse, since a flake gets re-run instead of investigated. install.test.cjs records a real spawnSync ETIMEDOUT at a 60000ms cap on a loaded bench while another lane passed the same commit in 12.7s, so full installs are bound at 120000ms against that recorded incident. Also adds an auditable escape to the guard's timeout ceiling. The 600000ms cap was set in #3143 from partial evidence, but fragment-single-edit- propagation carries a documented, load-tested 900000ms bound on a run that chains a full build plus eight generators -- the guard would have rejected a correct timeout the moment that file left the allowlist. A value above the ceiling is now permitted only with an inline allow-spawn-timeout-ceiling marker carrying a non-empty reason. It raises the ceiling; it never waives the requirement for a bound, which is asserted directly. install-shared.cjs keeps its hand-rolled assert rather than routing through throwIfFailed: its message embeds both streams, and throwIfFailed carries only a trimmed stderr. The message now also names the outcome, so a bounded timeout reads as such across its 38 importers instead of as expected null to equal 0. * test(#3145): extract class-norm timeouts and correct the build-hooks sizing A pre-PR review found 52 copies of four class-norm timeout constants across this wave. These are not per-suite fixture bindings -- they are shared facts about how long a class of subprocess takes, derived from a recorded bench incident. That norm already moved once (60000 to 120000 after a real ETIMEDOUT), and 52 copies would have drifted the next time it moved. Extracts tests/helpers/timeouts.cjs, where each norm is justified once, and converts the copies. A site that genuinely differs -- a real tsc compile, or regen:derived -- keeps its own local constant with its own justification. Also corrects a misclassification: scripts/build-hooks.js was sized as a build at 120000 in twelve places and 60000 in another, but it compiles and bundles nothing. Its own header says no bundling needed; it copies pre-built files and syntax-checks them with vm. Three different values bounded one script; now there is one. * test(#3145): fix red CI — lint self-match and a Windows chunk overrun Two failures on PR 3176. lint-allow-test-rule-refs read a RuleTester fixture as a real exemption. The fixture exists to prove an unrelated marker does NOT suppress the rule, so it carries that marker's literal text as test data. Split via concatenation, the same idiom no-unbounded-spawn-allowlist.test.cjs already uses for its own self-match problem. The explanatory comment needed the same treatment. The Windows shard 3/3 chunk was killed at its 600000ms budget. Output stopped seven minutes before the kill, so this was an overrun rather than a slow chunk: regenDerivedPropagatesSingleFragmentEditWithNoSecondSourceSurface runs regen:derived bounded at 900000ms, which is larger than the whole chunk budget, so the chunk killer always fires first and it can never complete there. Both the test and that bound predate this change; modifying the file pulled it into the Windows targeted set and exposed it. Skipped on Windows with the reason recorded; the Linux lanes cover it. The 900000 bound and its ceiling marker are unchanged -- they are correct. * test(#3145): refresh the stale test-timings cost table The Windows shard was killed at its 600000ms per-chunk budget. run-tests.cjs packs chunks by measured duration from tests/test-timings.json, and an unknown file falls back to the table's median weight -- advisory by design, but it silently underweights exactly the files that matter. Four of the failing chunk's 22 files were absent from the table, including the two heaviest: fragment-single-edit-propagation.install.test.cjs at 230s (it runs regen:derived) and agent-fragments-emission.install.test.cjs at 79s. Both were weighted as average, so the chunk's total weight read 53.68 against a budget of 60 and the packer produced a single chunk. Regenerated from a passing full-suite run, per the remedy the script itself documents. 700 to 770 entries, 70 added, 0 dropped -- verified, since gen-test-timings.cjs replaces the table wholesale rather than merging. Proven against the real packer: the same 22 files now weigh 103.91 and split into two chunks. No logic, budget, or timeout was changed; raising a budget to make a red gate pass is not a fix. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
27aa40f65e |
fix(#3023): stage pi's shared hook bundle outside pi's reserved hooks/ directory (#3175)
* test(#3023): failing-first guard — pi must not stage hooks in its reserved dir pi reserves <configDir>/hooks as its deprecated extension location and warns on every startup when it exists. Assert a pi install stages the shared hook bundle under gsd-hooks/ instead, manifests it there, and never creates hooks/. Also adds pi to the local-scope dir table in install-shared.cjs: pi was in RUNTIME_META but not LOCAL_DIR_NAME, so scope:'local' resolved path.join(root, undefined) and no local pi install could be exercised. Fails before the fix. Verified via the remote runner. * fix(#3023): stage pi's shared hook bundle outside pi's reserved hooks/ dir pi reserves <configDir>/hooks as its now-deprecated extension location and warns on every startup when that directory merely exists — checkDeprecatedExtensionDirs() guards the warning with a bare existsSync(), unlike its tools/ sibling. GSD staged its shared hook bundle exactly there, and pi's advised remediation (move it to extensions/) would break the adapter's paths and expose GSD's .js helpers to pi's extension auto-discovery. The bundle directory name is now runtime-descriptor-driven: hostBehaviors .sharedHooksDirName, defaulting to 'hooks' so all 18 other runtimes are byte-identical. pi sets 'gsd-hooks'. The name is validated as a single path segment — separators, dot-only segments, trailing dots, absolute paths, NUL, and Windows reserved device names all fall back to the default, because the value is joined onto a user's config root and written to. Renamed in place rather than relocated: hook scripts resolve siblings via __dirname/.., so a depth change would silently break them. - install / uninstall / manifest sites all read the resolved name - pi/gsd.cjs probes gsd-hooks then hooks, so dev checkouts and half-upgraded trees still resolve; the never-throws contract is preserved - new migration 009 retires the legacy pi hooks/ dir on upgrade, using a new non-recursive remove-empty-dir engine primitive (rmdirSync only, symlink-refusing, containment-guarded); ADR-0008 amended accordingly - fixes two latent name-dependencies the rename exposed: the stale-hook scan and the injection scanner's self-exclusion both hardcoded 'hooks' Verified on the remote runner. Closes #3023 * fix(#3023): close review findings and align emitted provenance with the rename Adversarial review found two defects, and the remote runner found four failure clusters. All fixed here. Review BLOCKER — detect-custom-files was blind to the renamed bundle. GSD_PREFIX_MANAGED_DIRS in gsd-tools.cjs hardcoded 'hooks', so for pi the whole gsd-hooks/ tree was invisible to the custom-file scan and user-added files there were never backed up before the next update's clean-install wipe. The dir set now resolves via the .gsd-runtime marker plus the shipped capability registry (never bin/install.js, which is not shipped into installed trees), and falls back to scanning every known candidate when the runtime cannot be determined — over-scanning is safe, under-scanning is the data loss. Review MAJOR — the pi adapter bound to an empty bundle. resolveSharedHooksDir accepted any directory, so an interrupted install left gsd-hooks/ winning over a fully-staged legacy hooks/ and every hook silently no-opped. A candidate now qualifies only if it is non-empty. Remote-runner clusters: - emitted-provenance had no rule for the gsd-hooks/ family; added two pi-scoped rules pointing at the same sources the existing hooks/ rules use. The table is total, so an unattributed family is a hard failure by design. - pi tests in install-minimal-hooks and the install integration suite asserted the old layout; updated to derive the dir name from the descriptor rather than hardcoding either name. - 19 unrelated-looking failures on node22 only were a leaked fs mock: t.after() runs in registration order, cleanup was registered before mock.restoreAll(), and node22's JS rimraf calls the public fs.rmdirSync while node24's native path does not — so the EACCES stub leaked process-wide on one lane. Restore now runs first. Verified on the remote runner. * fix(#3023): honor PI_CODING_AGENT_DIR, ack the rename ripple, fix expandTilde pi resolves its agent dir as PI_CODING_AGENT_DIR ?? ~/<CONFIG_DIR_NAME>/agent (packages/coding-agent/src/config.ts). GSD's pi descriptor declared an empty configHome.env, so a user with that variable set had GSD installed where pi never looks. Added the env name; the dot-home-nested resolver already handled the override, so no resolver logic changed. Also fixes expandTilde in the shared runtime-homes resolver, found while adding that: it hardcoded os.homedir() and ignored the opts.home every caller threads, so EVERY runtime's tilde-valued env override (claude, antigravity, windsurf, pi) silently resolved against the real home. That is a correctness bug and a test-escape hazard — a sandboxed test asserting on a tilde override reached the developer's actual home directory. Now threaded through every branch; behavior with no injected home is unchanged. Adds the emitted-drift ack fragment for the 58 pi paths whose emitted location moved with the rename. The provenance rules satisfy the totality gate; the differential gate needs the ack because the hook sources are byte-unchanged — only the installer's target directory moved. The two hook files this branch genuinely edits stay attributed and are not double-acked. Note on piConfig.configDir: it is read from pi's OWN installed package.json (getPackageDir walks up from pi's __dirname), alongside piConfig.name — a white-label setting for a redistributed pi fork, not a per-project user setting. Documented accordingly rather than treated as an unsupported override. Verified on the remote runner. * fix(#3023): reject blank env overrides, pin adapter/descriptor parity Three review findings, all fixed. A whitespace-only config-dir override was accepted verbatim: the guard was `if (val)`, falsy only for the empty string, so PI_CODING_AGENT_DIR=' ' resolved to a literal three-space directory name instead of falling back to the descriptor default. Fixed across every env-consuming branch — dot-home, dot-home-nested, all three xdg steps, and generic-agents-root — not just pi's. Non-blank values are still never trimmed, so '~/My Agent Dir' keeps working. pi/gsd.cjs's probe list and the descriptor were two independent sources of truth for the bundle directory name; a future rename would have desynced them silently and left every pi hook quiet with no error. The probe list stays deliberate — it must resolve in a dev checkout and a half-upgraded tree, where the registry's answer would be wrong — so this adds the parity assertion the repo's generative-fix-divergence rule calls for: the descriptor value must be the FIRST candidate, and the default must remain present. Changeset body rewritten to cover the two later user-facing fixes it had not caught up with. Verified on the remote runner. * chore(#3023): backfill changeset PR number * fix(#3023): anchor injection-scan patterns and fix a macOS detection hole CI's security job flagged CONTEXT.md:124 — pre-existing prose reading 'not the same fact as a genuinely empty or absent one'. The match was the 'act as a' INSIDE 'f-act as a': the pattern had no left word boundary, so any word ending in act tripped it (fact, impact, contract, artifact, interact, redact, abstract). My four-line CONTEXT.md edit dragged the latent false positive into this PR because the scan is diff-scoped by file but reads whole files. Anchored with (^|[^[:alnum:]]) rather than rewording maintainer-owned prose, which would have left the class alive for the next PR touching any file saying 'fact as a'. Auditing the rest of the list for the same class surfaced a real detection hole: the eval/exec/Function patterns matched a quote via \x27, a GNU-grep-only hex escape. BSD/macOS grep reads it as four literal characters, so single-quoted eval('...')/exec('...') payloads were NEVER detected there while passing on GNU-grep CI. Replaced with a literal apostrophe class. Boundaries were added only where a real word-suffix collision exists; exec, jailbreak, developer mode and the role-manipulation family were audited and deliberately left unanchored. 22 new cases cover both directions — the false positives now scan clean, and every real payload still fires, including the quote/punctuation/start-of-line boundary forms. Also builds this branch's injection test fixture at runtime instead of carrying the literal phrase, so the payload keeps its teeth without tripping the scan. Verified on the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1d208e5af6 |
test(#3144): bound the git/worktree cluster onto the process seam (#3152)
* test(#3144): bound the git/worktree cluster onto the process seam Migrates 180 unbounded sync spawn sites across 19 files. Every previously unbounded call now carries an explicit timeout with a comment giving the number and why. The migration is not a callee swap. execSync and execFileSync throw on a non-zero exit and the seam never does, so each site was classified first: sites that rely on the throw route to gitOrThrow, and sites that already read .status to detect an EXPECTED non-zero -- an intended cherry-pick conflict, a rev-parse outside a repo driving a skip -- route to the never-throwing runGit instead, which would otherwise throw on exactly the exit being probed for. Two same-named git() helpers in worktree-cleanup.test.cjs have different return contracts, one trimmed and one raw; both are preserved rather than unified. Collapses five hand-rolled throw wrappers onto one throwIfFailed in git-fixture.cjs, which gitOrThrow now also uses so the shape cannot drift. Allowlist drops 139 to 120; BASELINE lowered to match. * test(#3144): fix pre-PR review findings Documents throwIfFailed in the CONTEXT.md glossary and CONTRIBUTING.md -- it became the shared throw mechanism without either doc naming it. Routes the sixth and seventh hand-rolled copies of the throw shape through throwIfFailed (worktree-baseref-install, worktree-safety-reap); the first consolidation missed both. Converts ci-rebase-check's 8 fixture-setup calls from unchecked runGit to gitOrThrow so a failed setup step aborts where it fails rather than surfacing later as a confusing failure against the wrong subject. Adds 12 direct unit tests for throwIfFailed, which until now was only exercised transitively. Splits verify.test.cjs's non-git grep/sed bound off GIT_TIMEOUT_MS. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cfb37ccdfc |
Merge pull request #3154 from open-gsd/feat/3149-cmdinitdebug
enhance(#3149): dedicated init.debug entry point for /gsd:debug |
||
|
sim
|
8a0c1bce2e |
test(#3149): correct stale tdd_mode assertion and drop a marker-token collision
Two failures from the remote runner on
|
||
|
Tom Boucher
|
2afe17bbdb |
test(#3143): add the no-unbounded-spawn guard and throw-preserving git fixture (#3150)
* test(#3143): add no-unbounded-spawn guard and throw-preserving git fixture Adds the ESLint rule local/no-unbounded-spawn, wired into the tests/**/*.cjs block, plus an allowlist that only ratchets down: a listed file with zero violations reports its own entry as stale. The rule resolves renamed destructures and chained requires rather than matching literal callee names -- both forms exist in the suite today and a name-only matcher leaves them permanently invisible. It resolves an options object held in a single-write const, which is what keeps process-seam.cjs, the bounded reference implementation, from flagging itself. timeout: 0 and anything above the 600000ms ceiling are rejected as only nominally bounded. Adds tests/helpers/git-fixture.cjs so a migrated execSync call site keeps its throw-on-non-zero contract; process-seam.cjs is unchanged. * test(#3143): prove the allowlist guards can actually fail Extracts the D4/D6/D7/D8 checks into pure helpers and drives each against a synthetic fixture carrying an injected violation. Without this the suite only proved that today's clean data passes, which a deleted check would also satisfy. * fix(#3143): close two ceiling and alias escapes found in review Nested arithmetic bypassed the ceiling entirely: the numeric evaluator only resolved a flat literal, so `timeout: 60 * 60 * 1000` (3600000ms, six times the ceiling) fell through to trusted and reported nothing. The evaluator now recurses through arithmetic and unary signs with a depth cap. Alias resolution was traversal-order dependent, not scope dependent: a call textually above its own require destructure saw an empty alias map and reported clean. The map is now built in a Program pre-pass. Also: an explicit timeoutMs:undefined no longer overwrites the git fixture default via spread, adds the missing seam-routed rule test, and de-duplicates the repeated try/catch in the fixture tests. --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
2bead6ca1d |
feat(#3149): add dedicated init.debug entry point for /gsd:debug
/gsd:debug was one of the last workflows with no cmdInit* of its own: its Step 0 made three separate round-trips (state.load, resolve-model gsd-debugger, config-get workflow.tdd_mode) to assemble one context. Because no debug-scoped fact was computed at any entry point, ADR-1671 admission gate (2) could never be satisfied for debug — an applicability atom naming such a fact would evaluate FALSE forever and silently exclude its section. Adds cmdInitDebug (init.debug), registers it in the init router and the command-alias table, and collapses debug.md Step 0 to one call. Every field resolves through the same primitive the call it replaces used: loadConfig for commit_docs, withProjectRoot for response_language (#2402), planningPaths for debug_dir, resolveModelInternal for debugger_model, and the existing Boolean(workflow.tdd_mode) idiom for tdd_mode. PlanningPaths gains a debug field so state.load and init.debug share ONE debug-directory expression rather than two kept in sync by hand. state.load keeps emitting debug_dir: it is a shipped query surface with its own test anchor, so narrowing it would break unseen consumers for no gain. No WHEN_VOCABULARY atom and no gsd:section marker: gate (1), a consuming section of at least 400 bytes, belongs to the change that adds the section. Closes #3149 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
0d93a5cfe5 |
test(#3149): failing-first coverage for the init.debug entry point
Adds the behavioral matrix for a dedicated init.debug handler before the handler exists: equivalence cross-checks against the three calls debug.md makes today (state.load, resolve-model, config-get workflow.tdd_mode), bundle shape, the CLI negative/hostile argv matrix, section_manifest null-vs-[] degradation, planningPaths.debug, and a guard that WHEN_VOCABULARY stays closed at 29 entries. Currently RED: the init router reports "Unknown init workflow: debug". Matrix: .gsd/phase/feat-3149-cmdinitdebug/50-test-matrix.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4b66bf4560 |
fix(#3086): apply #2667 .cmd-shim gate to deps.spawn + surface errorCode in review lanes (#3142)
* fix(#3086): apply #2667 .cmd-shim gate to deps.spawn + surface errorCode in review lanes deps.spawn used shell:false with a bare binary name — on Windows, npm-installed CLIs (gemini, codex, etc.) are .cmd shims that CreateProcess cannot start, producing ENOENT + empty stderr. The review path then wrote an empty err file and emitted a generic 'failed or returned empty output' stub. Two fixes: 1. deps.spawn: detect .cmd/.bat on win32 and mediate through cmd.exe /d /s /c (same gate as runWithTimeout #2667, same explicit argv array). 2. runSpawnLane: surface errorCode (ENOENT, ETIMEDOUT) in the err file so the stub explains WHY the lane produced nothing. * chore(#3086): backfill changeset PR number 3142 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7ab4556395 |
fix(#3079): query commit no longer resurrects deleted phase branches via silent switch (#3141)
* fix(#3079: query commit no longer resurrects deleted phase branches via silent switch git checkout -b both created AND switched HEAD, so when branching_strategy: 'phase' deletes its branch on merge, a later query commit silently recreated it and moved HEAD there. The commit landed on the wrong branch. Fix: replace checkout -b with git rev-parse --verify + git branch (create- only, no switch). The commit always lands on the current branch. Callers that want to be on the phase branch use execute-phase's handle_branching. Updated 3 existing tests that asserted the old switch behavior. * chore(#3079): backfill changeset PR number 3141 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
80ec0791eb |
fix(#3052): preserve frontmatter last_activity_desc on same-date body prose conflict (#3140)
* fix(#3052): preserve frontmatter last_activity_desc on same-date body prose conflict preferNewerLastActivity only preserved last_activity_desc when the derived date was OLDER than the existing frontmatter date. When the dates matched (same-date), the derived body prose (potentially stale) overwrote the authoritative frontmatter desc. Fix: when derDate === exDate, also preserve the frontmatter desc. * chore(#3052): backfill changeset PR number 3140 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3c1e358a2d |
fix(#3099): emit LAST_ACTIVITY_UNPARSEABLE diagnostic for unusable last_activity (#3139)
* fix(#3099): emit LAST_ACTIVITY_UNPARSEABLE diagnostic when last_activity is present but unparseable parseActivityTimestamp returned null for both absent AND present-but-unusable last_activity values, silently suppressing the idle-stranded recommendation. Per ADR-1411's amendment (corrupt is not absent), the fallback stays but a diagnostic is now emitted via the warnUnusableInput seam. - Added LAST_ACTIVITY_UNPARSEABLE to UNUSABLE_REASON enum + prose - Wired warnUnusableInput into detectSignals when lastActivityRaw is truthy but parseActivityTimestamp returned null - Updated UNUSABLE_REASON lock test - Added 5 regression tests (unusable→diagnostic, absent→silent, well-formed→silent, dedup) * chore(#3099): add changeset fragment * chore(#3099): backfill changeset PR number 3139 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3146ff36aa |
fix(#3132): realign retired covered/backstop-as-status vocab to resolved+verification (#3138)
* fix(#3132): realign spec/plan/ui-phase workflow prose from retired covered/backstop-as-status to resolved+verification The edge-probe resolution model splits status (resolved|dismissed|unresolved) from verification (explicit|backstop). The workflow prose in three files still used the pre-re-cut covered/backstop-as-status vocabulary that validateResolution rejects. Swept all three prose surfaces: - spec-phase.md: Step 5.5 resolution options, --auto mode + log line, comment, Step 6 row list - plan-phase.md: lift rule (L778/L780), comments (L564/L706), quality gate (L826-827) - ui-phase.md: resolution loop (L391), --auto mode (L405-409), write-back format (L415) Added regression test in edge-probe-spec-phase-contract.test.cjs asserting the retired vocab is absent and resolved+verification is used instead. * chore(#3132): add changeset + emitted-drift ack for workflow vocab realignment * fix(#3132): update planner contract tests for resolved+verification vocabulary RR-02 and RR-03 tests asserted the old covered/backstop-as-status vocab. Updated to match the realigned prose (resolved edge → must_haves). * fix(#3132): fix specless-probe-fallback test assertion + merge duplicate ack Test assertion was too strict (expected auto-resolved + verification:explicit on same line). Split into two independent assertions. Merged plan-phase.md ack into existing #2658 fragment to resolve duplicate-path rule violation. * fix(#3132): use bare filenames in ack keys (size map keys are bare, not full paths) * fix(#3132): amend existing acks instead of duplicating — remove plan-phase from #2658, spec-phase from #3132, append #3132 reason to #0000 and #2650 * chore(#3132): backfill changeset PR number 3138 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
a731a45cd6 |
fix(#3116): strip trailing CR per line in parseFrontmatterStrict for CRLF WINDOWS.md (#3137)
* test(#3116): failing-first — parseLedger throws on CRLF WINDOWS.md On repos with core.autocrlf=true (Windows default), .planning/WINDOWS.md is checked out CRLF. The \n--- close-fence scan leaves the last frontmatter line's CR attached, and the key:value regex's . doesn't match CR, so the parser throws WINDOWS_LEDGER_MALFORMED on the last key. * fix(#3116): strip trailing CR per line in parseFrontmatterStrict The \n--- close-fence scan lands on the LF of the last frontmatter line's CRLF, so yamlBody ends with a bare \r. split(/\r?\n/) strips CR from interior lines but the last line's \r survives. The key:value regex fails because . doesn't match CR. Fix: strip \r per line (rawLine.replace(/\r$/, '')) rather than normalizing raw — the writer round-trips raw byte-exact. * chore(#3116): add changeset fragment * chore(#3116): backfill changeset PR number 3137 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
0e6fa2e2cf |
enhance(#3118): close the dead injectables and the shell projection follow-on — Wave 4 (#3124)
* test(#3118): failing-first coverage for the dead injectables and the shell projection Adds the counter-tests Wave 4 closes against, before any fix: - antigravityWatermark had zero test references. The four existing tests that look like watermark coverage hand the fallback a literal mark and never call the producer, so nothing pinned whether a real run's mark is correct. Covers all six branches plus the non-object cache classes. - Pins the fail-open: a transcript read that throws reports lines:0, indistinguishable from a genuinely empty transcript, and the consumer then replays a previous run's review as this run's. - Pins the export-line escaping across the repair, persist and win32 bash lanes, including the parity assertion that they must not diverge. - sliceCurrentPositionSection: empty-vs-absent, fenced heading, second occurrence, H3, CRLF. - Proves deps.progressProvider is inert by supplying a throwing stub to all ten transition intents. Verification through the remote runner only. Refs #3118 * fix(#3118): distinguish an unreadable transcript from an empty one antigravityWatermark's final read can throw on a transcript that indisputably exists. It returned lines:0, which is the same value a genuinely empty transcript produces, so the caller could not tell the two apart. antigravityTranscriptFallback derives its skip from that count. A mark of {convId:'c1', lines:0} for a conversation that pre-dates the run makes it skip nothing and return the last PLANNER_RESPONSE in a transcript written before this run started — a previous review presented as this one's, which is exactly what the function's own 'never stale' docstring promises cannot happen. The unreadable case now sets unreadable:true and the fallback declines for a same-conv-id unreadable mark. An absent or empty transcript is untouched: those genuinely have zero prior lines. * fix(#3118): escape the export line for the file it lands in, not the echo Three lanes emit export PATH="<dir>:$PATH". repair escaped it with escapePosixDoubleQuoted; persist and the win32 Git Bash lane escaped it with escapeSingleQuotedShellLiteral instead. The single-quoting is correct for the echo, so nothing runs when the user pastes the command. But the bytes appended to ~/.bashrc are the export line itself, and inside double quotes in an rc file a $(...) or a backtick in the directory name is command substitution that runs on every new shell. Those characters are legal in a path on both POSIX and Windows, so the path was reachable. projectPathExportLine is now the single source of that line and escapes for its final rc-file context; each lane still applies its own transport escaping on top. fish keeps the single-quote escaper — its value really does stay single-quoted. The cmd.exe lane interpolated into a cmd double-quoted string with no cmd-level escaping, so a quote closed the region and &cmd& ran. A quote is reserved on Windows and cannot appear in a real path, so there is no correct command to suggest: the win32 lanes now fail closed for one. Metacharacter-free paths render byte-identically on every lane. * fix(#3118): drop a stray carriage return and a deps field nobody reads locateCurrentPosition subtracted a fixed one byte to exclude the newline before the next heading, which assumes LF. On a CRLF document the slice kept an unpaired trailing carriage return. It now walks back over the newline and over a preceding carriage return if there is one. StateTransitionDeps also required a progressProvider that 33 sites supplied and no site ever called. A required field nothing reads widens the module's interface without changing its implementation, which is the shape epic #3051 cites as its reason for refusing blanket injection. Removed along with the ProgressRecord alias that existed only as its return type; state-document.cts's unrelated interface of the same name is untouched. * fix(#3118): stop an empty span duplicating bytes, and name the empty results Three findings from the isolated review pass. locateCurrentPosition could return end < start when the section was empty and the next heading followed with no blank line between. Every mutator splices with slice(0,start) + body + slice(end), so an inverted span duplicated the region between them — a blank line silently inserted into STATE.md on every transition, two bytes on CRLF. The span is now clamped, and an empty section is a zero-length span, which is what it always meant. The win32 fail-closed path left the installer printing 'Add it with one of:' with nothing under it. An empty shellActions folded two different facts together, so projectPathActionProjection now carries a frozen PATH_ACTION_REASON and the installer branches on it. Two empty results with different causes staying distinguishable is the subject of the epic this belongs to. fish_add_path parses a leading dash as an option, so a directory named -v printed 'No paths to add' instead of being added. Verified against fish 4.8.1: the end-of-options separator fixes it. Replaces the console-prose test the second fix first arrived with — a regex over captured stdout is what CONTRIBUTING prohibits, and the typed reason is the surface it asks for instead. * fix(#3118): escape TOML control characters, and stop a test name overstating Five findings from the two review axes. escapeTomlDoubleQuotedString escaped only backslash and quote. TOML basic strings also require U+0000-U+0008, U+000A-U+001F and U+007F to be escaped, so a value carrying a raw newline or NUL wrote a config.toml no parser accepts — rejecting the whole file, not just that value. Four of its call sites write real config. Tab stays raw; the grammar exempts it. The byte-identity test claimed every lane was unchanged for an ordinary path, which is false: fish now takes the end-of-options separator on every path, not only hostile ones. Renamed, and the one intended delta now has its own named test instead of hiding inside a claim that read as broader than it was. Also: exact-equality assertions in place of substring checks that could pass on a subtly wrong escape, newline and null-byte cases for all five quoting primitives, and a temp dir registered with t.after so it is removed when an assertion fails. * docs(#3118): add the changeset fragments * fix(#3118): degrade instead of throwing on a null conversation cache A cache file whose whole content is the literal null — what a truncated or zeroed write leaves behind — made both antigravityWatermark and antigravityTranscriptFallback throw. JSON.parse('null') succeeds, so the try/catch wrapping the parse never fired, and resolveConvId then called hasOwnProperty on null. Both functions advertise the opposite; the existing test next to them is named 'a missing cache or transcript degrades to empty, never throws'. Parsing successfully is not the same fact as the payload being usable, and a guard that only wraps the parse cannot tell them apart. resolveConvId is now total for any non-object input, so one guard covers both callers. Caught by the null case in this wave's own cache matrix. * test(#3118): correct a stale fish expectation and a parity comparison The pre-existing 'POSIX persist mode escapes single quotes' test pinned fish_add_path without the end-of-options separator this wave adds, so it asserted behavior that is no longer correct. A repo-wide scan found one such hardcoded expectation; every other site derives its expectation from the projection. The new parity test compared the token from a POSIX path against the win32 lane, which posix-normalizes its input first — two different inputs, so the tokens differed for a reason that had nothing to do with the parity it claims to check. It now derives the win32 expectation from the same input the lane receives. * docs(#3118): reword a comment the injection scanner reads as an instruction The scanner pattern act\s+as\s+(?:a|an|the)\s+ carries no word boundary, so 'the same fact as the payload' matched on the tail of 'fact'. Reworded per the documented remedy for this collision. The missing boundary is a scanner defect rather than a prose problem — any contributor writing 'fact as the' trips it — but the pattern is gate plumbing, which the sibling epic owns, so it is surfaced rather than changed here. * chore(#3118): backfill changeset pr number to 3124 * chore(#3118): backfill changeset pr number to 3124 * fix(#2784): make the negation scan single-pass and index it correctly Three defects in the negation suppression added by #3127, all in one block, none of which had a test. The pair scan was verbs.some(nouns.some(...)) with a slice and a split per pair, so it grew cubically with clause length: 1.1ms before that PR and 8462ms after, on 800 verb+noun pairs in one clause. api-coverage's property test generates documents large enough to reach the runner's 600s file cap, which is why it hangs as 'fail 0, cancelled 1' rather than failing an assertion. Every (verb, noun) window is a subset of the single widest one, so one scan of that window answers the same question in a linear pass. Verified equivalent against the old predicate over 20,000 generated clauses. Both checks also subtracted clause.start from offsets that collectTerm- Matches already returns clause-local. The first clause on a line has start 0 so it worked there and nowhere else: later clauses went negative, and slice reads a negative index from the end, so suppression silently examined unrelated text. The comment claimed 'without any API integration' was suppressed. It is not — the qualifier sits outside the two-word lookback and the noun precedes the verb. Widening the window would trade a false positive that costs one declaration line for a false negative that slips a real integration past a blocking gate, so the behavior stands and the comment now says so. Pinned by a test. The qualifier sets were also rebuilt for every line of every document. |
||
|
Tom Boucher
|
0ccc18dd3c |
fix(#2665): scrub config-location env vars in TEST_ENV_BASE (#3134)
* fix(#2665): scrub config-location env vars in TEST_ENV_BASE TEST_ENV_BASE scrubbed 14 session-identity vars but omitted CLAUDE_CONFIG_DIR, GSD_RUNTIME, and CODEX_HOME. The config-home resolver (runtime-homes.cts) consults these env vars BEFORE the HOME-derived fallback, so an ambient value won unconditionally over a sandboxed HOME. npm test wrote fixtures into the developer's live config directory when any of these were set. One leaked fixture was a registered skill (gsd-dev-preferences/SKILL.md) carrying behavioral directives that loaded into subsequent sessions. All three config-location vars are now blanked in TEST_ENV_BASE. Per-site overrides still win (env is spread last in the child-env merge). * chore(#2665): backfill changeset PR number 3134 * ci: retry shard timeout flake (#2665) * ci: retry shard-2 timeout flake (#2665) --------- Co-authored-by: sim <sim@local> |
||
|
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 |