178 Commits

Author	SHA1	Message	Date
Dennis Alexis Valin Dittrich	18c899def5	enhance(#4209): optional external source reviewer lanes for /gsd:code-review (#4323) ... * test(01-01): define reviewer-support trait contract Add failing coverage for step.supportsReviewerLanes (#4209 DISP-02): validator rejects non-boolean values with an exact field path, accepts missing/true/false, and the real code-review capability.json steps must declare supportsReviewerLanes: true. Add loop-resolver projection coverage proving the trait reaches activeHooks verbatim for a provider-neutral synthetic step (not code-review-specific), and that omitted/false values stay inert (no key on the active hook). All 8 new assertions fail today: the validator has no such field, and loop-resolver has nothing to project. RED before GREEN. * feat(01-01): declare reviewer-capable steps Add step.supportsReviewerLanes (#4209 DISP-02): a strict optional boolean opt-in trait, step-scoped (not capability-wide). Only a literal true validates and projects; false/omitted stay inert (no key on the projected active hook), and every non-boolean type fails capability-validator.cjs with an exact field-path error. Opt both existing code-review steps (execute:post, execute:wave:post) into the trait in capabilities/code-review/capability.json. Project the validated field through src/loop-resolver.cts into activeHooks so a provider-neutral generic interpreter can read it without any code-review-specific knowledge. Document the field in docs/reference/capability-manifest.md and regenerate gsd-core/bin/lib/capability-registry.cjs via the generator (never hand-edited). Makes all 8 RED assertions from the prior commit pass. * test(01-02): define shared reviewer dispatch - Add tests/reviewer-step-dispatch.test.cjs covering dispatchReviewerLanes: inert when the supportsReviewerLanes trait is off or nothing is selected, exactly-once plan/invoke per selected lane, duplicate-alias dedup, the bounded metadata-only source-review prompt (repo root, paths+baseSha, depth, four fixed prohibitions), and capability-neutral reuse via a second synthetic step context. - RED: module under test (src/reviewer-step-dispatch.cts) does not exist yet, so require() fails and every assertion is unreached. * feat(01-02): dispatch reviewers for opted-in steps - Add src/reviewer-step-dispatch.cts: dispatchReviewerLanes(input, deps), ONE interpreter for a step's supportsReviewerLanes trait. Reuses resolveReviewerSelection for selection and resolveLanePlan for planning (both already-existing, pure building blocks); invocation is the one required, caller-injected seam (deps.invoke) since runLane needs OS-aware spawn plumbing this module does not own. - trait !== true, or a selection resolving to zero lanes, dispatches nothing (zero plan/invoke calls). Each selected lane is planned and invoked exactly once, in the selector's deduped/sorted order. - buildSourceReviewPrompt assembles a metadata-only bounded prompt (repo root, canonical paths + base SHA, depth, four fixed prohibitions) — never file contents — written once per dispatch and shared across every invoked lane. - GREEN: tests/reviewer-step-dispatch.test.cjs now passes. * test(01-02): define reviewer dispatch failures - Extend tests/reviewer-step-dispatch.test.cjs with the fail-closed matrix: an explicitly requested lane the selector could not resolve still lets the OTHER resolved lane run, but the aggregate result must never read as a clean success (and 'every explicit lane unavailable' must be distinguishable from the plain no-flags-passed inert case); request-level validation (path traversal, absolute paths outside repoRoot, empty/non-string paths, missing depth/base SHA) halts the whole dispatch before any lane is planned or invoked; a per-lane prompt-budget overflow hard-fails only that lane before invoke while its sibling still runs. - RED: src/reviewer-step-dispatch.cts does not yet implement any of these guards, so 9 of the new assertions fail against the current (Task 1) implementation. * fix(01-02): fail closed in reviewer dispatch - src/reviewer-step-dispatch.cts: add the fail-closed guards the prior commit deliberately left out. An explicitly requested lane the selector could not resolve no longer lets the aggregate read as a clean success — lanes that DID resolve still run and keep their results (never narrow the requested set), but selection.errors now flips the aggregate ok to false, and 'every explicit lane unavailable' is now distinguishable (SELECTION_FAILED) from the plain no-flags-passed inert case (NO_LANES_SELECTED). - Add request-level validation (validatePaths, depth/baseSha presence) that halts the WHOLE dispatch before any lane is planned or invoked: path traversal, absolute paths outside repoRoot, empty/non-string paths, and missing provenance are all rejected up front. - Add per-lane prompt-budget enforcement (resolveBudget, mirroring gsd-tools.cjs's budgetFor convention including budget 0 = unbounded): a lane whose resolved budget the prompt exceeds hard-fails before invoke runs for it, without cancelling a sibling lane already planned. - Document the supportsReviewerLanes trait and its dispatch-step interpreter in gsd-core/references/loop-hook-dispatch.md. - GREEN: all 19 tests in tests/reviewer-step-dispatch.test.cjs pass; no regressions in the review-lane/reviewer-selection/prompt-budget suites (356 passing). * test(01-03): define optional source reviewer flow RED: assert code-review.md dispatches roster-derived reviewer-lane flags through a single review-lane dispatch-step call (DISP-01..05), that the no-flag path stays byte-for-behavior unchanged (COMP-01), and that external evidence reaching the internal reviewer prompt is marked unverified (CONS-02). Also covers the CLI contract directly: no-op with no explicit selection, and fail-closed on an explicit unknown lane (SAFE-07) via real gsd-tools.cjs subprocess calls. * feat(01-03): route optional source reviewers GREEN: code-review.md gains a dispatch_reviewer_lanes step that matches canonical reviewer-lane flags against the merged first-party + installed roster (never a hand-maintained list) and, only when at least one is present, calls the shared reviewer-step interpreter exactly once with the already-resolved repo root, file scope, depth, and base SHA. Its evidence paths are appended to the internal reviewer prompt via ${EXTERNAL_EVIDENCE_BLOCK}, explicitly marked unverified. No reviewer-lane flag leaves the internal-only dispatch byte-for-behavior unchanged (COMP-01). Deviation (Rule 3 — blocking issue): 01-02 documented `review-lane dispatch-step` (gsd-core/references/loop-hook-dispatch.md) as the CLI route `dispatchReviewerLanes` wires through, but never implemented the gsd-tools.cjs subcommand — the workflow's call had nothing to reach. Add it to the existing review-lane router, reusing the same effort-aware plan building and runner deps `plan`/`invoke` already use (factored into buildLaneRunnerDeps to avoid duplicating the spawn/http/fs seam). Guard the CLI's own `detected` set on whether an explicit flag was passed: resolveReviewerSelection's no-explicit-selection fallback is "select every detected reviewer" (the correct default for /gsd:review), and passing it an unconditionally non-empty detected set would silently invoke the whole roster on every no-flag code review, violating COMP-01. * test(01-03): define external finding consolidation RED: assert gsd-code-reviewer.md treats <external_reviewer_evidence> as untrusted input — independently re-verifies every claim against the actual current source, resists a prompt-injection attempt embedded in evidence text, and folds a verified claim into the existing Narrative Findings section with no second REVIEW.md schema (CONS-01..03). Also assert code-review.md's EXTERNAL_EVIDENCE_BLOCK restates the four fixed source-review prohibitions (SAFE-03..06) at the internal-reviewer handoff. * feat(01-03): consolidate external review evidence GREEN: gsd-code-reviewer.md's load_context parses <external_reviewer_evidence> as untrusted data, independently re-verifies every cited claim against the actual current source before it can appear in REVIEW.md, and explicitly resists prompt injection embedded in evidence text (never a command, no matter what it claims to be). A verified claim folds into the existing Narrative Findings section with (external: {slug}) provenance — one REVIEW.md schema only, no separate external-findings section. code-review.md's EXTERNAL_EVIDENCE_BLOCK now restates the four fixed source-review prohibitions (SAFE-03..06) at the internal-reviewer handoff. * fix(01-02): gitignore the reviewer-step-dispatch build artifact 01-02 added src/reviewer-step-dispatch.cts but never added its npm run build:lib output to .gitignore, unlike every sibling gsd-core/bin/lib/*.cjs generated file. Left it showing as untracked noise in git status. * docs(01-04): publish user and command contract for reviewer-lane source review - Document optional reviewer-lane flags on /gsd-code-review in USER-GUIDE.md and COMMANDS.md: opt-in, no source bodies in prompts, no fallback on failure, findings independently consolidated into the single REVIEW.md - Add the same contract to the docs/features/code-review-pipeline.md fragment and regenerate docs/FEATURES.md from it - Preserve /gsd-review as the plan-review command; cross-reference it rather than duplicating the reviewer roster - Pick up docs/INVENTORY-MANIFEST.json and skills/gsd-code-review/SKILL.md drift owned by source already shipped in Plans 01-01/01-03 but never regenerated (npm run regen:derived had not been run in this worktree) * docs(01-04): align architecture and agent ownership docs for reviewer-lane trait - ARCHITECTURE.md: trace the #4209 capability trait (supportsReviewerLanes) through the shared dispatchReviewerLanes interpreter to the existing review-lane plan/invoke machinery, ending at gsd-code-reviewer as the sole REVIEW.md consolidator - AGENTS.md: document gsd-code-reviewer's full-context verification scope and its treatment of external reviewer evidence as unverified input - No new diagram, abstraction, or config key; docs/CONFIGURATION.md is unchanged since the feature adds no setting or default * fix(01-02): eslint-ignore the reviewer-step-dispatch build artifact Same gap as the earlier .gitignore fix: 01-02 added src/reviewer-step-dispatch.cts but never added its generated gsd-core/bin/lib/reviewer-step-dispatch.cjs output to eslint.config.mjs's ignore list like every sibling generated file, so tsc's emitted __importDefault CommonJS-interop var tripped no-var. * fix(01-04): add the reviewer-step-dispatch.cjs roster row to docs/INVENTORY.md 01-04 regenerated docs/INVENTORY-MANIFEST.json (which now lists cli_modules/reviewer-step-dispatch.cjs) but the hand-written roster row in docs/INVENTORY.md — required by design, since a role sentence cannot be generated — was never added. * fix(01-01): update the code-review capability-step fixture for supportsReviewerLanes refactor-trigger-cli.test.cjs's preservesCodeReviewHookShapeAlongsideRefactorHook strict-deep-equals the code-review step's exact shape at execute:post; 01-01 added supportsReviewerLanes: true to that step and this fixture was not updated. * chore(01-03): acknowledge emitted-doc growth for code-review.md and gsd-code-reviewer.md Both files grew as a direct, intended consequence of wiring optional reviewer lanes into /gsd:code-review (the new dispatch_reviewer_lanes step and the untrusted-evidence consolidation contract) — not incidental drift. Emitted-Drift-Ack-Growth: code-review.md — new dispatch_reviewer_lanes step and EXTERNAL_EVIDENCE_BLOCK wiring for optional reviewer lanes (#4209) Emitted-Drift-Ack-Growth: gsd-code-reviewer.md — untrusted external-evidence consolidation contract for optional reviewer lanes (#4209) * test(01-05): define WR-01/WR-02 reliability contract for dispatchReviewerLanes From internal code review: dispatched must be false when zero lanes actually reached plan(), and a throwing plan()/invoke() for one lane must not discard results already collected for a sibling lane — matching the fail-closed pattern gsd-tools.cjs already uses for the same resolveLanePlan call (#2494/#2605/#1698/#1936/#2073/#2176/#2589/#2794). Refs: gsd-core-dks.16, gsd-core-dks.17 * fix(01-05): close WR-01/WR-02/IN-01/IN-02 from internal review - WR-01: dispatched now tracks whether any lane actually reached plan(), not results.length — an unresolvable selected slug no longer reports dispatched:true. - WR-02: plan()/writePromptFile()/invoke() wrapped per-lane so a throw for one lane can never discard results already collected for a sibling lane, matching the same guard gsd-tools.cjs already has around the identical resolveLanePlan call. - IN-01: documents the intentional budget===0-is-unbounded convention (#2797) the caller already relies on. - IN-02: review-lane dispatch-step no longer blocks indefinitely on an un-piped interactive TTY; fails closed to empty paths instead. Refs: gsd-core-dks.16, gsd-core-dks.17 * docs(01-05): add changeset fragment for PR #17 * fix(01-03): allowlist prompt-injection-scan false positive on the untrusted-evidence contract agents/gsd-code-reviewer.md's untrusted-evidence section and its pinning regression test both quote injection phrases as the exact attack they defend against/detect — same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class as the existing allowlist entries, not an actual injection vector. * test(01-05): extend WR-02 coverage to writePromptFile/invoke throws; DIFF_BASE-empty skip From CodeRabbit review: WR-02's earlier fix only wrapped plan() — writePromptFile()/deps.invoke() still ran unguarded, so a throw there still aborted every later selected lane. Also covers the dispatch_reviewer_lanes DIFF_BASE-empty-provenance gap (explicit lanes silently not running when no prior review and no phase-start commit exist). * fix(01-05): skip dispatch_reviewer_lanes with a clear warning when DIFF_BASE cannot be resolved Previously an explicit reviewer-lane request with no prior review and no resolvable phase-start commit reached dispatch-step with an empty --base-sha, which fails closed via missing_provenance — correct, but silent about why explicitly requested lanes didn't run. Now skip dispatch entirely in that case with a stderr warning naming the actual cause. * fix(01-05): wrap writePromptFile/invoke in the same per-lane try/catch as plan() WR-02's original fix only guarded plan() — a throw from writePromptFile() or deps.invoke() still aborted the whole dispatch, discarding results already collected for lanes processed earlier in the loop. CodeRabbit caught the gap; WR-02b/WR-02c pin it. * fix(01-05): WR-02b mock must throw only on the first writePromptFile() call The committed mock threw unconditionally, so codex's retry also threw and failed for the same reason as claude's — the test could not distinguish 'sibling still runs' from 'sibling also breaks'. Gate the throw to the first call, matching WR-02/WR-02c's single-failure intent. * fix(#4209): close review findings from adversarial + critical-code-reviewer pass Two independent reviews (agy adversarial review, Opus critical-code-reviewer + ponytail) found 6 Blocking and 7 Required issues in the reviewer-lane dispatch wiring around dispatchReviewerLanes. All 13 tracked in gsd-core-dks.18-30 and fixed here: - dispatch-step's reducer silently swallowed whole-dispatch rejections (invalid paths, missing provenance, etc); it now checks parsed.ok/reason. - spawn_reviewer recomputed its own stale DIFF_BASE, diverging from the LAST_REVIEW_COMMIT-aware value dispatch_reviewer_lanes uses on re-review; now shares the single compute_file_scope derivation. - the external reviewer prompt had no actual review request or citation requirement, only prohibitions; added both. - removed the supportsReviewerLanes trait plumbing (capability registry, validator, loop-resolver, docs, tests) — it was never consulted by the real dispatch path, which gates on explicit CLI flags instead. - flag-resolution require() was a fragile cwd-relative literal that failed silently on non-vendored installs; now resolves via GSD_TOOLS's own directory and warns instead of swallowing failure. - reducer didn't unwrap the @file: overflow protocol for large payloads. - deduplicated resolveBudget/budgetFor into one resolveLaneBudget. - lane artifacts now write to a mktemp run dir instead of $PHASE_DIR, so a second dispatch can't overwrite prior evidence. - validatePaths rejects control characters, closing a markdown-injection vector into the external prompt via crafted filenames. - reworded the one line that tripped prompt-injection-scan.sh instead of allowlisting the whole production prompt file. - fixed a stale docstring range and a dispatched-field ordering bug. - added 3 integration tests executing the actual reducer against synthetic dispatch-step JSON, replacing markdown-substring-only assertions. 771/771 tests pass across every touched suite; tsc --noEmit clean. * fix(#4209): wire supportsReviewerLanes as the maintainer's required reusable trait The maintainer's approval on issue #4209 explicitly redirected implementation shape: reviewer-lane dispatch must be a reusable capability/step-dispatch trait ("supportsReviewerLanes"), not code-review.md hand-wiring the call itself. My previous commit (e2558326) deleted that trait entirely after finding it declared-but-never-consulted, which was backwards — the fix was to wire it, not remove it. Restores the trait (capability.json, generated registry, validator, loop-resolver.cts, docs, tests) and wires it for real: dispatch_reviewer_lanes now resolves its own active hook via `gsd_run loop render-hooks` for the configured workflow.code_review_point and only proceeds to CLI-flag matching when supportsReviewerLanes reads true. Explicit flags no longer bypass the trait; a matching flag with the trait false resolves zero slugs (proven by a new integration test executing the real fence with both trait states). Emitted-Drift-Ack-Growth: gsd-core/workflows/code-review.md — the dispatch_reviewer_lanes step grows a trait-resolution fence (#4209 maintainer redirect requires the capability layer, not the workflow, own the opt-in decision). * fix(#4209): dispatch-step self-verifies the reviewer-lane trait via --cap-id/--point Both an agy adversarial review and an Opus critical-code-reviewer pass independently found the same gap in my previous commit (9b2c3773d): the trait check I wired into code-review.md only protected code-review's OWN invocation — gsd-tools.cjs's dispatch-step handler still hardcoded `trait: true` unconditionally, so a second capability declaring supportsReviewerLanes would get zero enforcement from the shared CLI unless it correctly re-implemented the ~15-line render-hooks scrape itself. That is exactly the "each workflow.md hand-wiring the call" the maintainer's redirect said to eliminate. Moves the trait check into dispatch-step itself: given --cap-id/--point, it self-invokes `loop render-hooks <point>` (relocating the one subprocess code-review.md used to spawn for this, not adding a new one) and derives the real trait from that capId's active hook, rather than trusting a caller-passed boolean. code-review.md now only passes --cap-id code-review --point "$CODE_REVIEW_POINT" and no longer resolves or gates on the trait itself — the ~20-line scrape it previously carried is gone. Any other capability opts into the identical enforcement by declaring the trait and passing the same two flags. Replaced the two tests that stipulated SUPPORTS_REVIEWER_LANES as an input variable (they proved a bash branch honors a variable, not that the variable reflects the real capability manifest) with three integration tests that invoke the real dispatch-step CLI against the real first-party capability registry: the real code-review trait resolves true, an unknown --cap-id resolves false (trait_not_enabled, fail-closed), and omitting --cap-id/--point entirely resolves false (no context means no opt-in). Also: reject \x7f/U+2028/U+2029 in validatePaths' control-character check (agy-F1 was incomplete), and delete the promptWritten per-lane coupling flag — the prompt write is idempotent, so writing it once per lane instead of gating on "did any lane write it yet" removes a latent bug where a deps.plan override that ever varies promptPath per lane would silently skip writing for a later lane. Emitted-Drift-Ack-Growth: gsd-core/workflows/code-review.md — net line count drops (the trait scrape moved into dispatch-step), but the file still grew this session across multiple commits; acknowledging per the growth-tracking convention. * fix(#4209): remove per-run token waste from the shipped prompts Runtime prompt content, not session tokens: two real, per-invocation token costs in the code that ships. 1. agents/gsd-code-reviewer.md's critical_rules restated nearly all of load_context step 5's ~180-word untrusted-evidence contract in ~90 more words, breaking this section's own established terse one-liner style (every other rule here is 1-2 sentences). This prompt loads fresh on every /gsd:code-review invocation. Shrunk to a one-line cross-reference, matching how write_review's own reference to step 5 already does it. 2. buildSourceReviewPrompt repeated the base SHA on every single file line even though it is identical for every file and already stated once at the top of the prompt — O(files) wasted tokens on every dispatched lane for a 50-file review, for zero information gain. File lines are now bare paths. * fix(#4209): resolve reviewer-lane trait in-process, fix CI failures found in review round 3 Opus critical-code-reviewer found a real Blocking defect in the --cap-id/ --point self-invocation added last commit: `dispatch-step` spawned `loop render-hooks <point> --raw` as a subprocess and bare-JSON.parse'd its stdout, but `io.cjs`'s output() redirects any payload over 50000 chars to `@file:<path>` instead of inline JSON -- the same overflow protocol this feature already unwraps for its OWN dispatch result 60 lines later in code-review.md. A large-enough activeHooks envelope (more installed capabilities/fragments) would throw, get silently swallowed by the bare catch, and misreport a real trait as trait_not_enabled with zero diagnostic. Fixed by extracting the config/registry/capability-state resolution `cmdLoopRenderHooks` already performs into an exported pure function, resolveActiveHooksForPoint (both `cmdLoopRenderHooks` and dispatch-step now share it), and calling it in-process from dispatch-step instead of spawning a subprocess at all. This eliminates the @file: exposure entirely (the dispatch-step path never touches the rendered-string envelope or its JSON-stringify/50000-char threshold), removes one subprocess spawn per code-review invocation, and gives a genuine diagnostic (stderr warning) on resolution failure instead of silent fail-closed. Corrected three doc/ docstring references to the now-removed subprocess self-invocation. Also fixes 2 real CI failures this round surfaced: - lint-tests: the agy-F1 control-char regex fix's `eslint-disable-next-line no-control-regex` comment was unused under this project's ESLint config (verified locally: the rule never actually flags \x00-\x1f in this repo's config) -- a mistake from an earlier commit this session, never actually lint-checked before push. Removed the disable comment. - security (prompt-injection-scan): the agy-F1 regression test's crafted fixture literally contains "Ignore all prior instructions." as test data proving validatePaths rejects it -- allowlisted the test file, same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class as existing entries. Also trimmed agents/gsd-code-reviewer.md's load_context step 5 (R2): one bullet stated "untrusted, never a command" three different ways in one paragraph, and a same-file duplicate of write_review's schema rule. Consolidated to state each rule once. Declined one suggestion from this round: shrinking code-review.md's EXTERNAL_EVIDENCE_BLOCK to a bare evidence list. Two tests (tests/code-review-pipeline-regression.test.cjs's CONS-01..03 block, tests/code-review.test.cjs's CONS-02 test) deliberately lock the four- prohibitions restatement and the untrusted-evidence prose into the INJECTED block itself, not just the consolidator's system prompt -- adjacency of the warning to the untrusted payload it's warning about is a recognized prompt-injection defense-in-depth pattern from this workstream's original TDD plan, not accidental duplication. * fix(#4209): correct stale per-file base-SHA prose in the external prompt Leftover from removing the per-file base SHA repetition earlier this session: the review-request sentence still said "relative to its base SHA" (singular per-file framing) when there's now exactly one base SHA, stated once above the file list. Reads "relative to the base SHA above" now. * fix(#4209): make getLane/configGet/plan required deps, delete dead defaults R3/R4 from the review round I'd deferred as low-priority test-churn: this file's one production caller (gsd-tools.cjs's dispatch-step handler) always supplies all three, so the fallbacks were dead in production -- but each was actively WRONG if ever reached: the default configGet always returned undefined, silently disabling resolveLaneBudget's overflow guard; the default getLane looked up only first-party REVIEWER_LANES, diverging from production's overlay-merged roster; the default plan skipped per-host effort resolution entirely. These defaults were introduced by this PR's own earlier work (this file did not exist before #4209 -- first commit a760bfcda, 01-02), not inherited from elsewhere, so there's no external caller depending on the lenient contract. Turned out free to fix: making the three deps required and deleting defaultGetLane/defaultPlan needed zero test changes -- every existing test that actually reaches the per-lane loop already supplies getLane/plan explicitly, and configGet's only real dependent (the budget-overflow tests) already supplies it too. 788/788 tests pass unchanged, tsc/lint clean. * fix(#4209): define depth semantics for the external reviewer lane Verified this was a real bug, not a match to existing convention as I'd claimed when declining the suggestion earlier this session: the internal gsd-code-reviewer agent's own system prompt carries a full <depth_levels> block defining what quick/standard/deep mean and do (agents/gsd-code- reviewer.md:68-99). The external reviewer lane has no access to that persona at all -- it only ever sees buildSourceReviewPrompt's bounded text, which sent the bare depth label with zero definition to a third-party CLI with no other source of truth for what "standard" means. Added depthMeaning(), condensed from the internal reviewer's own <depth_levels> definitions so the two stay consistent, and interpolated it into the review-request sentence. 150/150 tests pass, tsc/lint clean. * fix(#4209): merge dispatch_reviewer_lanes' split fences into one shell invocation CR-01 (Opus critical-code-reviewer, confirmed by direct execution): the roster-matching fence set EXPLICIT_JOINED/EXPLICIT_REVIEWER_SLUGS, and a SEPARATE later fence read them via ${#EXPLICIT_REVIEWER_SLUGS[@]} to decide whether to dispatch at all. This file's own documented rule (its depth-resolution guard, stated explicitly a few hundred lines earlier) is that a guard and the extraction it protects must run as one shell control-flow decision, because markdown-fenced blocks do not share shell state -- this step violated its own file's rule for the entire feature's gating condition. Merged the roster-resolution fence and the dispatch-decision fence into one continuous bash block, removing the intervening prose that split them. Fixed the stderr-based failure detection in the same edit (RQ-01: checking whether stderr is non-empty misfires on any benign Node warning; now checks the actual exit status of the roster-resolution command). Verified by extracting the merged fence and executing it standalone, driving both branches: --codex resolves EXPLICIT_JOINED=codex, SLUGS_COUNT=1, and a real dispatch-step call succeeds; no flags resolves EXPLICIT_JOINED empty, SLUGS_COUNT=0, dispatch-step never invoked (COMP-01). 141/141 workflow tests pass, tsc/lint clean. * fix(#4209): depthMeaning accuracy, injection defense on all embedded fields, hoisted prompt write Batch of Required/Suggestion fixes from the Opus critical-code-reviewer + writing-for-agents pass: - CR-02/CR-03: depthMeaning() dropped real categories from quick (empty catch blocks, commented-out code) and deep (error propagation, state mutation consistency, circular dependencies) relative to the real <depth_levels> block, and had zero test coverage. Restored full accuracy and added tests that read the real agents/gsd-code-reviewer.md file directly, so drift between the two can't recur silently. Unrecognised depth now normalizes to standard's definition, matching that agent's own documented rule, instead of rendering an undefined bare label. - RQ-04: depth/baseSha/repoRoot/runDir land in the same markdown prompt `paths` does, but weren't checked for control characters like paths were (agy-F1's original finding). Hoisted CONTROL_CHAR to module scope and applied it to all four fields at the same provenance-check boundary. runDir previously had zero validation at all. - S1: deleted the dead `identity` parameter on `invoke` -- the one production caller already ignores it, no test read it by name. - S2: hoisted the shared prompt write above the per-lane loop -- promptPath is derived from runDir alone (constant across lanes by construction), so writing it once is both correct and cheaper than the per-lane write R1 introduced earlier this session. Discovered and fixed a real regression from the naive version of this hoist: an unguarded throw would have escaped dispatchReviewerLanes as an uncaught exception instead of a clean per-lane failure. Added a new PROMPT_WRITE_FAILED whole-dispatch reason, matching the existing validatePaths/MISSING_PROVENANCE halt pattern, with a dedicated regression test. - S3: moved `planned = true` past the budget-overflow gate, so `dispatched` only reports true once a lane has cleared BOTH plan and budget checks. - S5: relayed gsd-code-reviewer.md's own "performance issues are out of scope unless also correctness issues" policy into the external-lane prompt, which previously had no such guidance and could return findings the internal reviewer's own contract excludes. - RQ-05 (partial): shrunk this file's own header docstring's restatement of the trait-reuse architecture to a pointer at gsd-core/references/loop-hook-dispatch.md, the canonical home. 234/234 tests pass across the full reviewer-lane test suite, tsc/lint clean. * fix(#4209): dedupe roster-merge logic, consolidate trait architecture prose, add step completion criterion RQ-02: added a `review-lane explicit-from-argv` subcommand that reuses the SAME merged-roster logic (`laneBySlug`) `dispatch-step`/`plan`/`invoke` already share. code-review.md's ~18-line inline `node -e` reimplementing `loadRegistry`+`mergeReviewerLanes` (a rename-only copy of the block in gsd-tools.cjs) is now a single call to this subcommand -- the exact violation code-review-flags.cjs's own header warns against ("this is the canonical flag-parsing surface -- do not replicate inline bash parsing"). RQ-03: an empty --cap-id XOR --point now warns distinctly from the legitimate no-context opt-out (both absent) -- a caller that named a capability without its point was silently indistinguishable from a correct opt-out. Also hardened the CODE_REVIEW_POINT config-get fallback: it only ever fires when the config-get COMMAND ITSELF fails (config-get already resolves the manifest's own schema default in the normal case), but that failure was previously silent. RQ-05/W-01/W-12/W-13: the "supportsReviewerLanes is a reusable trait resolved inside dispatch-step" explanation was restated in full in 5 places across this session's own review cycles. Consolidated to ONE canonical statement in gsd-core/references/loop-hook-dispatch.md; the other 4 (this file's own header, gsd-tools.cjs's comment, docs/ARCHITECTURE.md, code-review.md's step-opening comment) now point at it instead. W-05/W-06: loop-hook-dispatch.md described "false or non-boolean" as two inert cases when capability-validator.cjs already rejects non-boolean at load -- restated as the two cases that actually reach this code. Removed a "do not hand-roll trait resolution" prohibition whose target no longer exists once the positive description precedes it. W-04: deleted a no-op sentence in agents/gsd-code-reviewer.md ("missing block means proceed as normal") -- an absent optional block already means proceed as normal without being told. W-08/W-09: replaced longhand "zero selection/plan/invoke calls" and the made-up compound "byte-for-behavior [un]changed" with the token this session's own docs already coined for this concept (inert) and the word that means what byte-for-behavior was reaching for (unchanged). W-10: dispatch_reviewer_lanes had no completion criterion -- added one sentence naming the checkable end state (EXTERNAL_EVIDENCE_BLOCK is set, either populated or empty). This exact sentence would have caught the cross-fence bug fixed two commits ago at authoring time. Declined from this round, with reasoning: W-02/W-03 (trim the untrusted-evidence restatement in EXTERNAL_EVIDENCE_BLOCK/critical_rules) -- two tests deliberately lock this as intentional adjacency-based prompt-injection defense-in-depth, not accidental duplication (see this branch's own earlier commit). S4 (wrap LANE_RUN_DIR in a creation-site `trap ... EXIT`) -- would fire at the end of the CREATING fence, before spawn_reviewer's agent ever reads the evidence files, given this file's own documented fenced-block execution model; the existing named cross-reference between creation and cleanup already satisfies the co-location concern without introducing that regression. 853/853 tests pass across the full reviewer-lane test suite, tsc/lint clean. * fix(#4209): merge CODE_REVIEW_POINT into dispatch_reviewer_lanes' one fence, stop test from spawning real codex Round-5 review (agy) found the same cross-fence-split bug CR-01 already fixed for EXPLICIT_JOINED/EXPLICIT_REVIEWER_SLUGS: CODE_REVIEW_POINT's config-get fallback lived in an earlier, separate fence from the fence that consumes it via --point, split only by prose (not a guard, per this step's own documented rule). Merged into the single continuous fence and added a structural test asserting exactly one bash fence in the step. The new end-to-end regression test for this used --codex, which drives the fence's real `review-lane dispatch-step` call and, with the codex binary present on PATH, spawns the real external CLI — which then blocks on interactive auth with no stdin (BL-01). Stubbed gsd_run for `review-lane dispatch-step` only (captures argv instead of executing), keeping the real config-get/explicit-from-argv calls the test is actually about. * fix(#4209): split control-char vs missing provenance reason, realpath-check path escapes, stale comment Round-5 review (Opus) warning-tier findings: - WR-04: MISSING_PROVENANCE covered both "field absent" and "field present but a control-character injection attempt" — a caller distinguishing a config problem from a security event couldn't tell them apart. Split into MISSING_PROVENANCE (absent) and INVALID_PROVENANCE (present but invalid). - WR-05: validatePaths' containment check was lexical only (path.resolve), so a symlink whose own path sits inside repoRoot could still point outside it. Added an fs.realpathSync check (ENOENT-tolerant — a git-diff path can legitimately name a file already deleted in a stale worktree), realpathing repoRoot itself too so a symlinked repoRoot (e.g. /tmp on macOS) doesn't false-positive-reject its own real children. - WR-08: a comment in the per-lane loop still said a throwing writePromptFile() was caught there — stale since the prompt write was hoisted above the loop in an earlier round. WR-03 (validate depth against the quick/standard/deep enum) was considered and declined: this dispatcher is deliberately capability-neutral (see the existing "synthetic step context" test, which passes a non-code-review depth label on purpose to prove no code-review-specific special-casing exists). WR-01 (double registry load), WR-02 (trim-vs-hard-fail budget semantics), and WR-07 (reason omitted on the aggregate return) were verified against source and are not bugs — see review notes. * docs(#4209): document LANE_RUN_DIR's early-exit trade-off as accepted, not a gap Round-5 review (Opus, BL-03) flagged that an early exit between dispatch_reviewer_lanes and commit_review leaks the run-scoped temp dir. A trap-based cleanup was considered and rejected: if a step genuinely runs as a separate process, a trap set at creation time would fire at the end of that SAME fence, deleting the directory before spawn_reviewer/commit_review ever read it — worse than the leak it would fix. review.md's own gather_context/cleanup pair for the identical resource class (a run-scoped reviewer temp dir) already makes and documents this exact trade-off: cleanup runs only on a documented success path, and a leftover $TMPDIR entry is explicitly called cheaper than destroyed evidence. Recording that precedent here so this isn't re-raised as a live gap in a future review. * fix(#4209): register the WR-05 symlink-escape test's synthetic docs/ path reviewer-step-dispatch.test.cjs's "capability-neutral reuse" fixture passes paths: ['docs/spec.md'] as a synthetic, never-read path proving the dispatcher has no code-review-specific special-casing. lint-docs-guard- registration correctly flagged this as an unregistered docs/ path reference — add the docs-guard-exempt marker and its pinned baseline entry, the same pattern every other synthetic docs/ literal in this test suite already uses. * fix(#4209): backfill changeset pr: field with the real upstream PR number changeset-lint's fail_pr_field_drift caught the fragment still pointing at the fork PR (17) instead of the upstream one (open-gsd/gsd-core#4323) this branch is now also open against. * docs(#4209): amend ADR-2782 for the supportsReviewerLanes step-trait seam trek-e's review (2026-09-07, gsd-core#4323) found a real ADR gap: every decision in ADR-2782 (D1-D9) and every prior dated amendment governs the `role: "reviewer"` capability body and its one consumer, /gsd:review. This PR's actual new seam - a `supportsReviewerLanes: true` trait on an ordinary feature capability's `steps[]` entry, projected through loop-resolver.cts and resolved in-process via resolveActiveHooksForPoint - is a different capability axis (steps/gates/contributions) that the ADR's own scope note explicitly places out of reach. Per docs/contributor-standards.md's "Amending an accepted ADR", an in-place dated section is the established, lighter-weight path for an addition that stays within the ADR's existing decisions - used twice already in this same file - so this appends a third dated entry documenting the new seam, its consumer, and why it reuses the existing D1-D9-governed plan/invoke machinery rather than adding a second one. No decision is reversed; no new Amends/Amended-by pair is needed since the steps/gates/contributions axis already carries reciprocal links to ADR-857 and ADR-894. * fix(#4209): close two test-quality gaps trek-e's review found Minor 1: validatePaths (a path-shape parser guarding the prompt- injection/path-traversal trust boundary) had only example-based coverage, violating ADR-456's rule that parsers/budget limits carry at least one fast-check property test. Adds three: safe-segment paths are never rejected, a single leading "../" always escapes the one-segment repoRoot, and a control character anywhere is always rejected - one property per rejection reason validatePaths owns. Minor 2: the budget-overflow check (`estimatedTokens > budget`) was only ever exercised far below budget or at budget:0 (unbounded), never at the exact threshold crossing where a `>` vs `>=` off-by-one would hide. Adds three exact-boundary tests using the real estimateTokens/ buildSourceReviewPrompt the module calls internally, so the resolved token count is exact rather than approximated: budget == estimate (must pass), budget == estimate - 1 (must fail), budget == estimate + 1 (must pass). Also extracts okPlan()'s fixture timeoutMs into a named constant - local/no-adhoc-timeout-literal (#4446) landed on next after this branch was authored and flagged the pre-existing literal on rebase; it is fixture data for a synthetic plan object dispatchReviewerLanes never waits on, a distinct class from tests/helpers/timeouts.cjs's real subprocess norms. * fix(#4209): update docs-guard-registration baseline for the new ADR citation reviewer-step-dispatch.test.cjs's new fast-check property tests cite docs/adr/456-test-rigor-architecture.md in a justifying comment (never a real read). lint-docs-guard-registration fingerprints every docs/ path string an exempted test file mentions and fails on drift so a human re-confirms the exemption still holds - re-confirmed, and the baseline is updated to match. * fix(#4209): point changeset pr: field at the fork PR for CI validation changeset-lint's fail_pr_field_drift check compares the fragment's pr: field against the PR the CI run is actually attached to (GITHUB_EVENT_PATH), not a fixed target. Rehearsing this branch on fork PR davdittrich/gsd-core#17 needs pr: 17 to pass that check; the prior commit's pr: 4323 (the real open-gsd upstream PR number) is correct for that PR but fails here. Backfill to 4323 happens again, as the last commit, immediately before the approved push to open-gsd#4323 - never leaving pr: 17 on the branch that ships upstream. * fix(#4209): reject promptChannel:none lanes from source-review dispatch CodeRabbit found a real scope mismatch: coderabbit's lane declares promptChannel: 'none' and reviews the working tree on its own terms, fed nothing (review.md:367). Silently dispatching it through dispatchReviewerLanes would ignore the bounded paths/depth/baseSha scope buildSourceReviewPrompt promises and let the lane review whatever it independently sees fit, violating this interpreter's own scoped, metadata-only contract. Reject before plan()/invoke(), same as an unresolved slug. * fix(#4209): scope CONS-02 test to the evidence-block line, not the whole file CodeRabbit found the whole-file match on workflowContent would still pass if UNVERIFIED and re-open/reopen appeared in two unrelated parts of this 1000+-line workflow, proving nothing about the actual evidence block's contract. Line-filtered via splitLines (not a bare-\n regex spanning readFileSync content) so this stays CRLF-portable and passes local/no-unbounded-quantifier and local/no-crlf-fragile-split. * fix(#4209): guard DISPATCH_JSON substitution and capture its stderr CodeRabbit found the dispatch-step command substitution unguarded: a non-zero exit could leave DISPATCH_JSON empty (or halt the step under errexit with no warning), and the downstream reducer would only ever report the generic unparseable_dispatch_output reason, discarding the command's own diagnostic. Guarded like the existing CODE_REVIEW_POINT/ EXPLICIT_JOINED calls above it: capture stderr to a temp file, surface it in a warning on failure, and fall back to a parseable dispatch_ command_failed JSON stub so the reducer's existing reason-reporting path still fires. * docs(#4209): fix byte-for-behavior wording and missing colon, regenerate CodeRabbit found "byte-for-behavior" should read "byte-for-byte" (the established repo term for output-identical unchanged behavior) and a missing colon after the bold "Optional external reviewer lanes (#4209)" lead-in in docs/features/code-review-pipeline.md. Fixed in the two hand-authored sources (commands/gsd/code-review.md, docs/features/ code-review-pipeline.md) and regenerated the two derived projections (skills/gsd-code-review/SKILL.md via gen-plugin-skills.cjs, docs/ FEATURES.md via gen-features.cjs) so they stay in sync. * fix(#4209): drop the fabricated DISPATCH_JSON fallback stub (Windows CI) The prior fix's fallback `DISPATCH_JSON='{"ok":false,...}'` embeds double-quoted JSON keys inside a single-quoted shell literal. That extra quote density, inside an already quote-heavy ~8KB driver string, passed bash -n and the full local suite on Linux but broke Windows Git-Bash: `dispatch_reviewer_lanes computes CODE_REVIEW_POINT ... end to end (#4209 round 5)` failed on two Windows CI shards with `bash -c: unexpected EOF while looking for matching '''` — a Windows argv-to- command-line re-quoting edge case, reproducible on rerun, not a flake. Root-caused via gh api job logs plus a byte-identical local reconstruction of the test's own driver script. Fix: drop the fabricated stub. The downstream node -e reducer already falls back to reason `unparseable_dispatch_output` on any JSON.parse failure, so an empty/partial DISPATCH_JSON on command failure is still handled correctly, with zero new quoting risk. * revert(#4209): drop the DISPATCH_JSON stderr-guard nitpick (Windows CI) Two materially different mechanisms for the same CodeRabbit Nitpick ("Trivial | Quick win") both broke Windows Git-Bash reproducibly: a single-quoted JSON-literal fallback ("bash -c: unexpected EOF ... matching '''") and, after removing that, a plain `head -1 "$VAR"` inside a nested command substitution ("unexpected EOF ... matching '"'"). Both passed bash -n and the full local suite on Linux every time; both failed the SAME test deterministically on Windows CI. Two attempts at the same class of fix (nested-quote construction near this exact step) is the retry limit - reverting to the original, already-shipped, Windows-verified unguarded form rather than continuing to guess at a third quoting mechanism for a Trivial- severity nitpick. Logged as bug-221/bug-222 in .wolf/buglog.json for anyone attempting this again: the fix belongs outside this specific markdown-fence-driver test harness (e.g., a real .sh helper script) if it's worth doing at all. * fix(#4209): backfill changeset pr: field to the real upstream PR before push Fork validation (davdittrich/gsd-core#17) needed pr: 17 to satisfy changeset-lint's PR-number check while rehearsing there; this is the last commit before the approved push to the real upstream PR (open-gsd/gsd-core#4323), so the field points at that PR number again. --------- Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-07 22:52:33 -04:00
Tom Boucher	03738824de	enhance(#2586): stop installing Codex context-monitor hooks without metrics (#4367)	2026-09-06 05:46:49 -04:00
github-actions[bot]	b3906c66f6	chore: sync next package version to 1.13.0	2026-09-06 02:10:29 +00:00
Tom Boucher	2e1ede6d99	fix(#4093): give advance-plan's zero-labeled-fields failure a disk-derived recovery decline (#4318) ... * test(#4093): regression matrix for advance-plan zero-labeled-fields decline * fix(#4093): give advance-plan's zero-labeled-fields failure a disk-derived recovery decline * refactor(#4093): collapse IIFE to a plain block (review finding) * docs(#4093): document the advance-plan recovery decline + changeset * chore(#4093): backfill PR number in changeset * fix(#4093): budget lint-compiled-artifact-sync's tsc compile as a compile, not a probe --------- Co-authored-by: sim <sim@local>	2026-09-05 10:46:37 -04:00
Behruz Nassre Esfahani	5ad9a36f35	fix(#4255): resolve reviewer-lane effort from the lane, not from gsd-plan-checker (#4275) ... `review-lane plan` resolved every cross-AI reviewer lane's reasoning effort by spawning `query resolve-execution gsd-plan-checker --host <slug>`. The agent id was a hardcoded literal, so `--host` chose only the argv RENDERING while the LEVEL always came from the installed plan-checker's frontmatter — `low` under every shipped model profile. Every prompt-fed lane therefore ran at a fast structural verifier's effort, and because the rendered argument is a CLI config override it silently beat the effort the operator had configured for that CLI. At `low` a large source-grounded prompt makes a model end its turn with no final message, so the lane came back empty and its stub read as a crash. Effort is a property of the review, so the lane declares it. Two new fields on ReviewerLane — `effortConfigKey` (`review.effort.<slug>`) and `defaultEffort` — carried through each capability manifest and the generated registry, set on the three lanes with an argv effort channel and null on the other nine. A new pure `resolveLaneEffort()` resolves config key -> lane default -> nothing, where "nothing" emits no effort argument at all and the reviewer CLI's own configuration decides; `inherit` selects that path explicitly and an unrecognized level falls back to the lane default rather than being forwarded to a CLI that would reject it. The host's negotiated effortSurface still gates the rendering, so ADR-1239/#2481's trust boundary holds on this path too. Resolving in-process also removes up to twelve subprocess spawns per review. The empty-output stub now names the effort the lane ran at and distinguishes a clean exit from a timeout kill, a non-zero exit, and a process that never ran — `status` is null for both a timeout and a signal, so those were indistinguishable before. The hint is hedged: a clean empty exit is most often a model stopping short, but it is also consistent with a CLI writing its output elsewhere. Also: the capability validator now knows both fields, rejects a malformed key or an out-of-vocabulary default, and rejects a default declared without a config key (a level the operator could never override). An existing end-to-end row in tests/effort-surface-axis.test.cjs asserted the old coupling; it now configures the lane's own key and pins the decoupling in the same real spawn, with the agent execution tier set to a level that must not appear. Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort; leaving the new key undocumented there is the same invisibility that made the plan-checker coupling survive this long. Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort, so leaving the new key undocumented there is the same invisibility that let the plan-checker coupling survive. Claude-Session: https://claude.ai/code/session_01CRMEuzNMWn3gs5uUW2ghcF Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-05 05:25:44 -04:00
Dennis Alexis Valin Dittrich	925a363879	enhance(#4032): apply configured agent tool grants (#4238) ... * test(4032): add failing installed-agent grants contract Cover global and project agent_tools precedence at the real Claude installer seam before adding implementation. * feat(4032): apply configured agent tool grants during staging Resolve selector-level global and project config once per staging call, then append validated grants before runtime conversion. * test(4032): cover host grant and quoted MCP contracts Exercise installed host artifacts and prove ZCode must treat quoted MCP scalars like plain MCP grants. * feat(4032): apply configured agent tool grants across runtimes Move augmentation and scalar identity into the converter seam so every staged artifact preserves host policy. * fix(4032): register agent tool grants in configuration Accept documented agent_tools config without unknown-key warnings.\n\nKeep installer fixtures on the shared temporary-directory helper. * fix(4032): translate configured MCP grants for Kilo Reuse the converter-owned scalar decoder so quoted canonical grants reach Kilo's native permission keys without altering other host policies. * fix(4032): decode YAML-escaped tool grants * fix(4032): emit valid inline agent tool grants * fix(4032): reject invalid trailing-colon grants * test(#4032): cover cross-review remediation gaps * fix(#4032): close cross-runtime grant gaps * test(#4032): expose Kimi global project context * fix(#4032): preserve Kimi project config context * chore(#4032): add release note * test(#4032): expose fork review regressions * fix(#4032): address fork review findings * test(#4032): make byte-stability assertion portable Compare repeat installs at one root so platform-specific path rendering cannot masquerade as an agent_tools behavior change. * chore(#4032): bind changeset to upstream PR 4238 * fix(#4032): address trek-e review findings (2,3,4,5,6,7,8) Fixes fail-closed decode-failure handling in ZCode's mcp__ stripper, a comment-only `tools:` header mis-parse that silently dropped configured grants, and a naive comma-split that could tear a quoted scalar containing a literal comma. Documents Kilo's inherent `{server}_{tool}` MCP-permission-key collision (external, fixed format — not ours to widen) and locks the existing first-seen-wins resolution in with a regression test. Opts kimi/kimi-code out of the ADR-1235 pre-converter path-rewrite step: routing Kimi through that pipeline (needed so project-scoped agent_tools selectors reach it) was short-circuiting Kimi's own neutralizeKimiAgentPrompt, which expects the original ~/.claude/gsd-core text rather than a pre-rewritten Kimi path. Extends the fast-check token pool and per-runtime install coverage with the missing comment/comma/broad-runtime cases the prior review flagged as untested. * docs(#4032): add CONTEXT.md glossary entries for agent_tools resolver + pre-converter step Documents readGsdEffectiveAgentTools (Install Model Override Resolver Module) and the appendAgentTools pre-converter pipeline step (Runtime Artifact Conversion Module), per contributor-standards.md's new-seam glossary requirement (finding 1). * fix(#4032): address agy adversarial review findings An agy (gemini-3.8-flash-high) adversarial pass over the prior review-fix commit found the fixes for findings 3, 4, 6 and 8 had unfixed sibling gaps, plus a genuine new regression and two CONTEXT.md inaccuracies: - ZCode's comment-only `tools: # note` header matched the inline-value branch instead of falling through to the block-list scan, so a following mcp__* item leaked through unstripped — the exact defect finding 4 fixed in appendAgentTools, unfixed in this sibling function. - Reverted capabilities/kimi-code/capability.json's noPathRewrite: true. kimi-code uses the standard 'agents' kind with converter: null (not kimi-agents — confirmed by reading the descriptor, not its prose description), so it never went through the pipeline change finding 5 fixed, and disabling its path rewrite broke every ~/.claude/ embed in its shipped agents instead. - decodeToolScalar never stripped a trailing ` # comment` from a bare (unquoted) scalar, so a comment after a block-list item, or after an appended grant on an inline line, became part of the "tool name" — fixed at the source (one call site fixes every consumer). - appendAgentTools's comment-index scan wasn't quote-aware, so a `#` inside a quoted scalar (`"mcp__server #1"`) was mistaken for a comment start and corrupted the quote. - parseFrontmatterTools (Kimi/Qwen's tool-list reader, downstream of appendAgentTools's own output) had the same naive comma-split and comment-only-header gaps as findings 4 and 6, unpatched. - The all-runtime smoke test's presence assertion was built on a guessed omit-list; empirically only 7 of 17 runtimes keep an arbitrary mcp__ grant recognizable, replaced with a verified allowlist. - CONTEXT.md claimed a `project:<agent>` selector prefix that does not exist (project override is a same-key merge across two config files) and mislabeled stageAgentsForRuntimeWithConverter's module. * fix(#4032): address full-PR review (Opus critical/ponytail + agy) A whole-PR pass (critical-code-reviewer + ponytail-review on Opus, plus a second agy full-source adversarial pass) surfaced defects the earlier finding-scoped passes couldn't reach: - appendAgentTools corrupted a `tools:` line whose ENTIRE value is a leading quoted scalar (`tools: "Read"` -> `tools: "Read", Write`, invalid YAML) — there is no safe line-surgical rewrite here, so it now refuses to touch that shape instead of emitting broken frontmatter. - decodeToolScalar's malformed-trailing-quote check ran BEFORE comment stripping, so a bare tool name with a quote inside its own trailing comment (`Bash # note: "internal"`) was wrongly rejected. Reordered. - findUnquotedCommentIndex (added in the prior remediation commit) was built on a wrong model of YAML: a `#` after whitespace starts a real comment in a plain scalar regardless of nearby quote characters — verified against the actual parser. The one case that DOES need protection (a leading quoted scalar) is now refused outright above, so the quote-tracking scan was dead weight solving a problem that no longer reaches it. Removed; reverted to the plain `[ \t]#` scan. - Kilo has a SEPARATE agent-frontmatter parser (convertClaudeToKiloFrontmatter, distinct from the buildKiloAgentPermissionBlock fixed earlier) with the same comment-only-header and naive-comma-split gaps as findings 4 and 6 — unfixed in both its src/ and bin/install.js copies. Fixed in both, exporting splitToolScalars for bin/install.js to reuse rather than reimplementing it. - Pipeline docstring in stageAgentsForRuntimeWithConverter still listed 5 steps, omitting appendAgentTools (now step 3 of 6). - docs/CONFIGURATION.md didn't state that a --global install still discovers agent_tools from the cwd's .planning/config.json (confirmed intentional and already covered by a dedicated test, not a bug). - Removed install-engine.cts's deps.cwd injection seam: zero callers or tests ever populated it. Two claims from this round were verified and rejected, not fixed: prototype pollution via a `__proto__` selector key (empirically confirmed `Object.prototype` is never touched — only reassigns the resolver's own local object's prototype, with no observable effect), and a `*` grant value crashing YAML parsing as an alias reference (empirically confirmed it parses as plain scalar text, no crash). A pre-existing, unrelated defect (extractFrontmatterField returns null for block-list `tools:` on Copilot/Antigravity/Cursor/Codex/Qwen, affecting two shipped agents today) was filed as a follow-up rather than fixed here — it predates #4032 and isn't caused or worsened by this PR. * fix(#4032): update stale slug-derivation-drift-guard fixture line normalizeKimiSkillName's real closing brace moved from line 616 to 635 as a side effect of this PR's edits to runtime-artifact-conversion.cts; the MAJOR-1 fixture's hardcoded realEndLine had gone stale. * fix(#4032): address CodeRabbit findings on projectDir threading and flow-sequence tools bin/install.js's installAgentsKindStandalone call site omitted the projectDir argument the function already supports, so a global install through this legacy branch silently fell back to the runtime config dir instead of process.cwd() when resolving project-scoped agent_tools grants — inconsistent with the sibling installOpencodeFamilyArtifacts call site, which already threads it correctly. appendAgentTools' leading-quoted-scalar bailout did not cover a YAML flow sequence (`tools: [Bash, Read]`): splitToolScalars tore it apart on the in-sequence commas and appended past its closing bracket, producing invalid frontmatter. Extended the bailout regex to also refuse a value starting with `[`, matching the same "whole node, nothing may follow" reasoning already applied to quoted scalars. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-05 04:52:45 -04:00
Dennis Alexis Valin Dittrich	a262ad6b61	fix(#4148): dispatch wave-pre step hooks (#4185) ... * fix(#4148): dispatch wave-pre step hooks External capabilities can render step hooks before a wave, but the execute workflow consumed only contributions and silently skipped every step. Reuse the shared dispatch contract before executor spawning and pin the capability-validator boundary with a red-first regression. Emitted-Drift-Ack-Growth: execute-phase.md — wave-pre now carries the missing generic step-dispatch contract before executor spawning * test(#4148): pin wave-pre dispatch ordering * test(#4148): pin wave-pre dispatch contract * chore(#4148): bind upstream changeset PR * chore(#4148): restore fork changeset identity * fix(#4148): align wave-pre dispatch contract Mirror the sibling wave-post all-shapes clarification while pruning redundant prose so the rebased workflow remains below its frozen byte ceiling. Emitted-Drift-Ack-Growth: execute-phase.md — wave-pre now carries the missing generic step-dispatch contract before executor spawning * chore(#4148): restore upstream changeset identity * fix(#4148): align wave-pre capability guidance * docs(#4148): identify wave-pre manifest input Name the third-party manifest trust origin at the wave-pre dispatch boundary so the reviewer-requested validation guidance matches wave-post. Emitted-Drift-Ack-Growth: execute-phase.md — wave-pre now carries the missing generic step-dispatch contract before executor spawning * docs(#4148): preserve execute-phase byte budget Remove a redundant advisory label while retaining the non-blocking contract, keeping the reviewer-required trust-boundary wording at the enforced 93,400-byte ceiling. * fix(#4148): mark wave-pre manifest-input validation as security-relevant Reviewer nit on PR #4185: wave-pre's step-dispatch sentence had the (third-party manifest input) parenthetical but dropped the ⚠ marker that wave-post's parallel sentence (execute-phase.md:1044) carries, losing the visual flag that this validation is security-motivated. Trims the redundant "of one" from "not one shape of one" to reclaim the 4 bytes the marker adds — the ADR-857 byte-margin gate (tests/claude-orchestration.test.cjs) leaves zero slack at the 93,400-byte ceiling. * fix(#4148): trim wave-pre step-dispatch prose to clear ADR-857 byte ceiling Merging next's unrelated growth (#3990's TDD_APPLICABLE conditional) pushed execute-phase.md 116 bytes past the 93,400-byte ceiling, failing CI on all three platforms. The security-relevant ⚠ marker and ref.command validation call-out (added per prior reviewer nit) are preserved verbatim per the pinned regression test in capability-registry.test.cjs; only the non-pinned connective prose is trimmed. * fix(#4148): recalibrate execute-phase.md self-imposed margin, restore security marker next grew execute-phase.md by ~230 bytes across two unrelated merges during this fix (#3990's TDD_APPLICABLE conditional, then a further step-extraction commit), consuming this test's own self-imposed 93,400 safety buffer under ADR-857's actual, unmodified 93,600 ceiling (docs/adr/857-capability-system.md:22). The wave-pre step-dispatch sentence cannot shrink further without dropping one of the pinned substrings this same test file asserts on (kind=="step", loop-hook-dispatch, never blocks or redirects executor spawning, Validate `ref.command`). Raises the self-imposed margin to 93,550 (still 50 bytes under the real, untouched ADR ceiling) and restores the ⚠ marker the prior reviewer round required for the ref.command validation call-out, which byte pressure had dropped. * fix(#4148): restore full ref.command validation wording, drop self-imposed margin Adversarial review (agy/gemini-3.8-flash-high) flagged two issues in the prior CI-recovery commit: 1. Trimming "in-context before any shell use" from the step-dispatch warning weakened the inline operational instruction (the reader is told WHAT to validate but not the specific in-context-not-shell mechanism the referenced loop-hook-dispatch.md:45-51 threat model requires). Restored it - the merge with next since the last commit freed enough real margin (77 bytes under the untouched 93,600 ADR-857 ceiling) to afford it without any margin change. 2. The prior commit self-imposed margin bump (93400 to 93550) was, on reflection, the wrong lever: it is a number this PR invented, not an ADR value, and re-bumping it every time next grows execute-phase.md is a losing pattern (already needed twice in one session). Removed the redundant assertion; the same line existing bytes-under-93600 check against the real, frozen ADR-857 ceiling (docs/adr/857-capability-system.md:22) is the actual invariant and is untouched. workflow-size-budget.test.cjs tier hard cap (98304 bytes, extract-not-bump by design) remains the correct backstop for runaway growth. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-05 02:56:37 -04:00
Tom Boucher	acb903c2e8	enhance(#3661): make the code-review hook point configurable (#4159) ... * feat(#3661): make the code-review hook point configurable Add `workflow.code_review_point` (`execute:post` default, or `execute:wave:post`) so a multi-wave phase can run code review once per wave instead of once at the end, scoped to what changed since the phase's prior review. The code-review capability now declares its step at both loop points via a new generic `pointFrom` step field: `pointFrom` names an enum config key, and the step is only active at its own `point` when that key resolves to a matching value. `_resolvePointGate` (capability-activation.cts) is the single shared implementation consumed identically by loop-resolver.cts and capability-state.cts, and capability-validator.cjs enforces that `pointFrom` references an enum key whose values cover the declaring step's own point. code-review.md's manual-invocation gate now reads `workflow.code_review` directly instead of probing registry presence at the hardcoded execute:post point (so manual `/gsd-code-review` keeps working regardless of which automatic point is configured), and its file-scope tiers narrow to what changed since the phase's last review commit when one exists. execute-phase.md's wave-post step dispatch gets a small, precedented carve-out so the code-review skill still receives its required phase argument when dispatched generically (caught by the isolated spec review). Closes #3661 Emitted-Drift-Ack-Growth: code-review.md — #3661 adds a point-aware config gate check and LAST_REVIEW_COMMIT-based incremental scoping to the file-scope tiers. Emitted-Drift-Ack-Growth: execute-phase.md — #3661 adds one carve-out sentence so the wave-post generic step dispatch passes PHASE_NUMBER to the code-review skill. * docs: backfill changeset PR number for #3661 (#4159) * fix: scope tests/io.test.cjs's fs.writeSync fault-injection mocks by fd Five fault-injection mocks in the "bug #1008" describe blocks intercepted every fs.writeSync call regardless of file descriptor, and several threw or truncated unconditionally on the first call. This surfaced as an intermittent macOS CI failure: node:test's own IPC channel back to the parent process (which also goes through fs.writeSync internally) could get a bogus injected error or truncated write if node's internal machinery called it while one of these mocks was active, corrupting the message frame the parent tried to deserialize ("Unable to deserialize cloned data.", location tests/io.test.cjs:1:1, uncaughtException — a whole-file IPC crash, not a test assertion failure). Root cause confirmed by a working counter-example already in the same file: the "#3912 A6" mocks gate on `fd !== 2` before any fault injection and were never implicated. Applied the same fd-scoped pattern to the five unscoped mocks (four output()-targeting tests gate on fd 1, one error()-targeting test gates on fd 2), and added a regression test proving an unrelated fd passes through untouched while the fault-injection mock is active. Found while verifying #3661; unrelated to that change's own diff. --------- Co-authored-by: sim <sim@local>	2026-09-02 11:01:54 -04:00
Tom Boucher	ff0361071d	feat(#3653): add review.models.cursor — wire modelArg/modelConfigKey for the cursor reviewer lane (#4160) ... * feat(#3653): add review.models.cursor — wire modelArg/modelConfigKey for the cursor reviewer lane cursor-agent exposes --model (204 selectable models) but the cursor lane declared modelArg: null / modelConfigKey: null, so review.models.cursor was rejected as an unknown config key and the #1517 reviewer-instances escape hatch silently discarded a configured model at modelExpansion. Wire the lane the same way codex already is: inject {{model}} into args right after -p, set modelArg to --model, and declare modelConfigKey as review.models.cursor plus its config schema entry. An unconfigured lane still invokes byte-identically to today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3653): add changeset for review.models.cursor Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3653): update co-change surfaces that assumed cursor has no model key gsd-test surfaced three surfaces still hardcoding "cursor declares no modelConfigKey", broken by wiring review.models.cursor: - tests/reviewer-config-federation.test.cjs: the #3691-narrows-#2797 invariant test listed cursor among lanes that must own no model key. - tests/settings-integrations.test.cjs: the #3651 keyless-lane test listed cursor as keyless, including a live config-set assertion that now correctly succeeds instead of failing (swapped to qwen). - gsd-core/workflows/settings-integrations.md: the settable-keys enumeration and two prose call-outs still named cursor as keyless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3653): acknowledge deliberate growth of settings-integrations.md settings-integrations.md grew 4 bytes because it now enumerates review.models.cursor as a settable key alongside the other reviewer lanes, matching the modelConfigKey wired for cursor in this PR. Emitted-Drift-Ack-Growth: settings-integrations.md — adds review.models.cursor to the settable-keys enumeration and removes cursor from the two keyless-lane call-outs, matching #3653's modelConfigKey change Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3653): fix malformed Emitted-Drift-Ack-Growth trailer The previous commit's trailer was separated from Co-Authored-By by a blank line, splitting it into an earlier, non-trailer paragraph — git's trailer parser only recognizes the last contiguous block. Restating it here immediately adjacent to Co-Authored-By so both parse as trailers. Emitted-Drift-Ack-Growth: settings-integrations.md — adds review.models.cursor to the settable-keys enumeration and removes cursor from the two keyless-lane call-outs, matching #3653's modelConfigKey change Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3653): backfill changeset pr number pr:0 -> pr:4160 now that https://github.com/open-gsd/gsd-core/pull/4160 exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-01 21:40:20 -04:00
Tom Boucher	4dfc46bbe7	enhance(#3348): add a context-drift pre-check gate to plan-phase (#4147) ... * test(#3348): add failing-first coverage for the context-drift gate * feat(#3348): add context-drift pre-check gate for plan-phase Compares each phase's *-RESEARCH.md/*-PATTERNS.md/*-VALIDATION.md/*-SPEC.md effective last-changed time (git commit time, falling back to mtime for uncommitted edits) against *-CONTEXT.md's, so plan-phase no longer silently reuses an upstream artifact that predates a decision added to CONTEXT.md after that artifact was derived from it. Deterministic, no model call. New `gsd_run verify context-drift <phase>` command, sibling to the existing verify.codebase-drift/verify.schema-drift gates in the drift capability. Warn-only by default (workflow.context_drift_precheck), with an opt-in workflow.context_drift_action: block escape hatch. Wired at plan:pre in plan-phase.md, before both the RESEARCH.md and PATTERNS.md reuse decisions. * fix(#3348): address code-review findings — raw-text-match, stale comment, import placement, duplicated phase resolution * fix(#3859): pin the real commit's diff.ignoreSubmodules to match the empty-diff probe The #3859 empty-diff guard decides whether a submodule bump would land using `--ignore-submodules=dirty`, overriding the caller's `diff.ignoreSubmodules` config. The real `git commit -- <paths>` that follows was never given the same override, so under a bare `diff.ignoreSubmodules=all` repo config the two calculations disagree: driven on git 2.39.5 (Debian bookworm, the linux-node24 test-matrix image), the guard correctly stands aside but the scoped commit itself then silently fails (exit 1, no error text) for a gitlink bump it had just confirmed would be recorded, surfacing as commit_failed instead of committed:true. Pin `-c diff.ignoreSubmodules=dirty` onto the scoped commit call too, so the probe and the commit it protects can never diverge. Harmless when no submodule path is involved (driven: identical outcome on an ordinary scoped file, with and without the flag). * fix(#3348): guard resolvePhaseDirByToken's exact-match fallback against path traversal * fix(#3348): retarget phase-enumeration-drift exemption to the consolidated resolvePhaseDirByToken helper cmdVerifySchemaDrift's inline readdirSync was already function-scoped-exempt in lint-phase-enumeration-drift.cjs as a single-phase LOOKUP (not a current-milestone enumeration). This PR's refactor pass lifted that block into a shared helper, resolvePhaseDirByToken, also used by the new cmdVerifyContextDrift — the guard tracks exemptions by enclosing function name, so the readdirSync now lives in an unexempted function and started firing. Move the exemption to resolvePhaseDirByToken (same written reason, now covering both callers) instead of migrating to listAllPhaseDirs, which would introduce two real behavior deltas here: it catches readdirSync failures internally (old code let them throw) and sorts results by phase number before matchPhaseDirs picks matches[0] (old code used raw, OS-dependent readdirSync order). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3348): satisfy lint:ci — slash form, capability registry regen - docs/features/context-drift-gate.md used the deprecated /gsd: colon form; docs are never passed through the install-time slash-form converters, so lint-docs-command-form requires the hyphen form. Regenerated docs/FEATURES.md from the corrected fragment. - Regenerated gsd-core/bin/lib/capability-registry.cjs after editing capabilities/drift/capability.json (lint:generated-sync). * fix(#3859): pin the real commit's diff.ignoreSubmodules via env, not argv -c The prior fix pinned `-c diff.ignoreSubmodules=dirty` onto the scoped commit's argv via `commitArgs.unshift(...)`. `-c key=val` must precede the `commit` subcommand, so this shifted `commitArgs[0]` from `'commit'` to `'-c'` for every scoped commit call, breaking 17 position-based assertions in the commit-files pathspec regression suite that read `a[0] === 'commit'` to find the commit invocation among recorded git calls. `execGit` already accepts an `env` option merged onto `process.env` before spawning. Git honors `GIT_CONFIG_COUNT`/`GIT_CONFIG_KEY_0`/`GIT_CONFIG_VALUE_0` as a per-invocation config override functionally identical to `-c key=val`, expressed via env instead of argv. Passing that env alongside the existing commitArgs (still `['commit', ..., '--', ...stagedPaths]`, argv unchanged) fixes the real commit's effective diff.ignoreSubmodules to match the empty-diff guard's probe without moving anything in argv position 0. Scoped to exactly the canScope branch, matching the probe's own preconditions and leaving no behavior change for commits the probe never evaluated. No test file changes needed — the 17 previously-failing assertions test argv[0] against the array passed into execGit, which never changes. * fix(#3348): register verify-context-drift in the check subcommand router The drift capability's new plan:pre gate declares check.query "verify.context-drift", which normalizes to `check verify-context-drift`, but no such subcommand was routed — phase6-capstone-conformance's uniform-block-field test failed with "Unknown check subcommand" for every declared gate query. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3348): extend #1592's exact-key-list snapshot for the new context-drift config keys tests/capability-registry.test.cjs asserted an exact, hardcoded snapshot of the drift capability's config keys. #3348 legitimately adds two new keys (workflow.context_drift_precheck, workflow.context_drift_action) for its own plan:pre context-drift gate — extend the expected set (and clarify the assertion message) without weakening the test's exactness. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3348): reconcile E2's exemption-migration pin with the resolvePhaseDirByToken extraction #3348 (an earlier commit on this branch, e4b80ad81) extracted cmdVerifySchemaDrift's inline phasesDir readdirSync/matchPhaseDirs block into the shared resolvePhaseDirByToken helper (also used by the new cmdVerifyContextDrift), and retargeted lint-phase-enumeration-drift.cjs's function-scoped exemption from cmdVerifySchemaDrift to resolvePhaseDirByToken accordingly — cmdVerifySchemaDrift no longer contains a line the guard's detectors match, so it needs no exemption. tests/phase-locator.test.cjs's E2 test still pinned the exemption to the old name (cmdVerifySchemaDrift), unaware of the migration. Update E2 to match the same "migrated call site's exemption must move, not duplicate" pattern the test already applies to cmdRoadmapAnalyze and cmdInitMilestoneOp just below it: drop cmdVerifySchemaDrift from the still-exempt list and add symmetric assertions that it no longer carries the exemption while resolvePhaseDirByToken now does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3348): fix two self-contradicting/nondeterministic tests in context-drift.test.cjs 'always exits 0 (query command contract)' included the no-phase-arg case, which contradicts the file's own earlier 'errors with usage message on missing phase arg' test (that case legitimately exits 1 via the Usage error) — drop it from the always-exits-0 cases. 'degrades to mtime comparison outside a git repo' and '...in a repo with no commits' relied on real wall-clock ordering between two back-to-back writeFileSync calls to prove CONTEXT.md is newer than RESEARCH.md; on a fast filesystem both can land in the same mtime tick, producing a tie that computeContextDrift's strict `<` correctly treats as not-stale, so stale_artifacts comes back empty. Make both tests deterministic via explicit fs.utimesSync instead of relying on timing (CONTRIBUTING.md: never assert elapsed wall-clock time). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3348): add context_drift_precheck:false to the plan:pre all-off fixture The "all plan:pre when-keys false" fixture explicitly disables every known workflow.* plan:pre toggle, but didn't yet know about the new workflow.context_drift_precheck key (defaults to true), so the new drift context-drift gate stayed active and broke the empty-activeHooks assertion. Emitted-Drift-Ack-Growth: plan-phase.md — adds the #3348 context-drift plan:pre pre-check section (new ## 4.6); this PR's own diff, not incidental drift. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3348): backfill changeset PR number (pr:0 -> 4147) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-01 21:39:37 -04:00
Tom Boucher	c0fd2e3f4c	feat(#3673): add dispatch.maxConcurrency axis and dispatch-capacity query (#4162) ... * test(#3673): add failing tests for dispatch.maxConcurrency axis and dispatch-capacity CLI route Extends tests/host-integration.test.cjs with negotiateHostCapabilities maxConcurrency negotiation coverage (test matrix rows 1-15, including a fast-check property test) and a new #3673 dispatch-capacity CLI route describe block spawning the real gsd-tools.cjs (rows 16-25). Extends tests/host-integration-validator-parity.test.cjs with an all-19-descriptor maxConcurrency presence/validity sweep (row 26) and adds a hostile-input validator test to host-integration.test.cjs (row 27). The dispatch.maxConcurrency field does not exist yet, so these tests fail. * feat(#3673): add dispatch.maxConcurrency axis, negotiation, validator parity, and the dispatch-capacity query Adds a numeric dispatch.maxConcurrency sub-field to the Host-Integration Interface (ADR-1239 Phase 1), following the existing dispatch.isolation sub-field pattern: DispatchCapability interface, SAFE_DEFAULTS/PROFILE_BASELINES floors, and a negotiateHostCapabilities branch that passes through a positive safe integer and fails closed to 1 otherwise (no engine-side reduction, per the design doc's explicit rejection of a min(host,engine) rule). capability-validator.cjs gains parity validation for the new field (optional, positive safe integer or the "undocumented" sentinel — mirroring isolation's "added after existing descriptors" treatment). gsd-tools.cjs gains a new `query dispatch-capacity` route, a pure-read sibling of `dispatch-isolation` with no side effects: live env (GSD_DISPATCH_MAX_CONCURRENCY) > descriptor > fallback-to-1 precedence. All 19 capabilities/*/capability.json descriptors now declare dispatch.maxConcurrency: claude carries the one cited value (20, per code.claude.com/docs/en/sub-agents); the other 18 carry "undocumented" (not yet researched for this axis). * docs(#3673): document dispatch.maxConcurrency and add its citation row to the capability matrix Updates docs/reference/host-integration-interface.md's dispatch struct entry (also backfilling the previously-undocumented isolation/backgroundDispatch sub-fields found stale in the same table) and adds fail-closed/live-transport precedence prose for the new maxConcurrency field. Adds a dispatch.maxConcurrency row (with citation) to all 19 host sections in docs/reference/host-integration-capability-matrix.md — required for tests/host-integration-descriptors.test.cjs's kimi-code matrix-parity check, which asserts every declared dispatch sub-axis is documented there. Updates docs/how-to/add-or-update-a-host-integration.md's dispatch checklist and example descriptor block to mention maxConcurrency (and, likewise backfilling a stale gap, isolation/backgroundDispatch). * fix(#3673): extract shared maxConcurrency validator, drop dead reserved-name check Exports isPositiveSafeInteger from src/host-integration.cts as the single source of truth for the dispatch.maxConcurrency positive-safe-integer contract; negotiateHostCapabilities and gsd-tools.cjs's routeDispatchCapacity now both call it instead of independently reimplementing the same predicate. Also removes the __proto__/constructor/prototype reserved-name branch from capability-validator.cjs's maxConcurrency check — copy-pasted from the string-enum fields above it, but unreachable for a numeric field (the generic positive-safe-integer branch already rejects any string) and absent from maxDepth, the field the code's own comment claims to mirror. --------- Co-authored-by: sim <sim@local>	2026-09-01 21:39:12 -04:00
Dennis Alexis Valin Dittrich	b848b23861	feat(#3778): dispatch plan:pre planner contributions before quick planning (#3934) ... * feat(#3778): dispatch plan:pre planner contributions in quick.md - Add plan:pre capability gate to quick.md Step 5, mirroring plan-phase.md's existing render + generic contribution dispatch pattern - Inject planner-targeted contribution fragments into the planner prompt, after AGENT_SKILLS_PLANNER, matching D-08 ordering - Add tests/quick-plan-pre-capabilities.test.cjs proving the dispatch is generic (D-01) via real scanWiredKinds/coveredKindsInRegion functions - Record quick.md's byte-growth rationale in this commit trailer for every gsd-core-verbatim runtime Emitted-Drift-Ack-Growth: quick.md — #3778: Step 5 (Spawn planner, quick mode) gains a `plan:pre` capability gate, mirroring `plan-phase.md:420-424` and `:797`. This is shipped shell and prose read by an agent at runtime, not compiled, so the reasoning has to travel with the feature rather than being deferred to a reference doc: (1) the dispatch paragraph phrases role routing possessively ("the role each entry's `into` names") rather than as an `into ==` equality, because `coveredKindsInRegion` (scripts/gen-loop-host-contract.cjs) voids a segment's `kind == "contribution"` coverage credit when a role or capability equality shares that same segment — an equality phrasing here would silently fail the generic-dispatch proof required by D-01; (2) `activeHooks` is read directly in-context from `PLAN_PRE_HOOKS_JSON`/`HOOKS_JSON` and the unfiltered `rendered` digest is explicitly forbidden from being pasted, because `rendered` carries every kind and role — including non-planner-targeted contributions such as a `into: "checker"` twin — and pasting it would leak checker-scoped guidance into the planner's prompt (T-01-02 in the threat model); (3) the injection block sits inside `<planning_context>` AFTER `${AGENT_SKILLS_PLANNER}` and after the Project skills line, matching plan-phase's `:741` -> `:797` ordering (D-08), so agent-skills content is never shadowed by capability-contributed prose. No prose was moved into an eagerly `@`-imported reference to shrink the measured file — @gsd-core/references/loop-hook-dispatch.md already existed before this change and is deferred to for the generic contract only, exactly as plan-phase.md already does. * test(#3778): expand quick.md plan:pre dispatch coverage to all nine locked conditions Extend tests/quick-plan-pre-capabilities.test.cjs with D-02 (silent omit-when-empty), D-03 (single shared planner spawn), D-06 (array-order dispatch phrasing), D-07 (planner-only into filter), and D-08 (render call < agent-skills placeholder < injection block < spawn ordering) assertions, all extracted via a brace-bounded slice anchored on the literal injection instruction rather than a naive first-brace scan (${AGENT_SKILLS_PLANNER} and the surrounding prompt's ${VALIDATE_MODE ? ...} ternaries also contain brace pairs). Add a capability-registry.test.cjs describe block proving the registry-wide D-07 exclusion is meaningful: at least one plan:pre contribution exists, every plan:pre contribution has a non-empty into/fragment.inline, and the registry as a whole carries at least one non-planner-into contribution. Add a loop-host-contract.test.cjs regression pin for D-09: quick.md stays absent from STEP_WORKFLOWS, parseLoopHostBlock still throws on quick.md's real content, and buildContract() still yields exactly 5 entries. Verified red-without-Task-1 by temporarily reverting quick.md to its pre-f30de9cc content and re-running these three suites (D-08 failed as expected), then restored via git checkout and re-confirmed green. * docs(#3778): note quick planning also renders plan:pre in the tutorial The tutorial's Step 6 named only /gsd-plan-phase as the trigger for the plan:pre hook set. Since quick.md now dispatches the same hook set (f30de9cc), the sentence understated the capability's real reach. * feat(#3778): add changeset fragment * chore(#3778): reference the upstream issue in the changeset fragment The fragment was the only one of 81 in .changeset/ without a trailing (#NNNN) reference or a bold lead-in. serializeChangelog auto-appends only the pr: field, so the rendered CHANGELOG entry carried no link back to issue #3778. * test(#3778): scope the D-07 registry assertion to what it actually proves The registry-wide non-planner check was named "D-07 exclusion is meaningful", which overclaims: it proves only that `into` takes non-planner values somewhere in the registry, not that anything is excluded at plan:pre. Every plan:pre contribution is currently into: "planner", so the filter is a forward-looking safeguard there. Narrowing the assertion to plan:pre (as review suggested) would fail today. Asserting plan:pre is all-planner would be brittle — it would break the day a legitimate non-planner plan:pre contribution lands, which is exactly when the safeguard starts doing work. So the assertion is unchanged and only the name and comment are corrected. * chore(#3778): point the changeset fragment at the upstream PR The fragment carried pr: 3, the fork staging PR. changeset lint derives the real PR number from GITHUB_EVENT_PATH, so on the upstream PR that would read as pr-field drift. Point it at open-gsd/gsd-core#3934. * test(#3778): require contributions in Quick revision prompts * test(loop-host): require Quick auxiliary registration * fix(#3778): preserve contributions in Quick plan revisions * fix(#3778): validate Quick as a planner contribution host * fix(#3778): tighten Quick contribution contract * test(#3778): drop unnecessary source-contract exemption * fix(#3778): require Quick planner target coverage * docs(#3778): describe targeted auxiliary coverage --------- Co-authored-by: davdittrich <davdittrich@gmail.com> Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-01 21:16:57 -04:00
Tom Boucher	62b0d939b6	feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey (#4083) ... * feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey Add an optional `timeoutConfigKey` field to the reviewer lane descriptor, resolved in `resolveLanePlan` at invocation time and falling back to the frozen `timeoutFloorMs` when unset or invalid, in the same spirit as the existing `promptBudgetKey`/`modelConfigKey` fields. All 12 shipped lanes declare `review.timeouts.<slug>` on both surfaces (the descriptor and their capability.json manifest), validated by capability-validator.cjs. For the antigravity lane, the native `agy --print-timeout` flag — previously a second hardcoded literal (`540s`) independent of the outer cap — is now derived from the same resolved outer timeout in `antigravityArgv`, preserving the existing 60-second buffer relationship (ADR-2782 D6: the outer bound is declared data, the inner one is handler-owned). The antigravity default timeoutFloorMs stays at 600s per the maintainer's disposition; users raise it through the new config key instead. * docs(#3274): document review.timeouts.* and extract resolveTimeoutMs helper Address code-review findings on the timeoutConfigKey change: extract the inline timeout-resolution logic into a named, exported, directly-tested resolveTimeoutMs helper (matching the file's existing configString/ normalizeHost convention); document the new review.timeouts.* federated config keys in docs/CONFIGURATION.md, docs/reference/capability-manifest.md, and docs/how-to/ship-a-reviewer-lane.md; add the changeset fragment. * fix(#3274): resolve native antigravity timeout in resolveLanePlan, not the runner gsd-test caught two design mistakes in the prior commits: 1. SpawnPlan.argv is documented and tested as fully resolved by resolveLanePlan (model/effort/output/prompt already folded in) — leaving the antigravity '{{nativeTimeout}}' marker unresolved until the runner's antigravityArgv violated that contract and broke tests that read plan.argv directly (tests/antigravity-reviewer.test.cjs, tests/review-default-reviewers-workflow.test.cjs). Fix: '{{nativeTimeout}}' is now a fifth ARGV_PLACEHOLDER member, resolved by resolveLanePlan itself via the new nativeTimeoutToken() helper, exactly like the other four. antigravityArgv reverts to its pre-#3274 four-argument form. Also missed updating capabilities/antigravity/capability.json's invoke.args to match the descriptor, which broke the manifest/descriptor parity test. 2. tests/reviewer-config-federation.test.cjs enforces a deliberate, narrow invariant (#3691 narrows #2797): qwen, cursor, and coderabbit — the three lanes with neither a model flag nor a host — may own no config key beyond their own prompt-budget key. Adding review.timeouts.<slug> to all 12 lanes violated it. Fix: those three keep timeoutConfigKey: null and own no review.timeouts.* key, matching their existing modelConfigKey: null. The other 9 lanes are unaffected. * chore(#3274): backfill changeset PR number (pr:0 -> 4083) --------- Co-authored-by: sim <sim@local>	2026-08-30 14:26:02 -04:00
github-actions[bot]	4a705cf19f	chore: sync next package version to 1.12.0	2026-08-30 03:07:03 +00:00
Tom Boucher	1051c6d8d4	fix(#3799): scope legacy cleanup to the install's resolved config dir and add --no-legacy-cleanup (#4013) ... * fix(#3795): read the interrupted agent id before clearing the stale marker (#4006) * test(#3795): the interrupted-agent read must precede the stale-id clear * fix(#3795): read the interrupted agent id before clearing the stale marker execute-plan's init_agent_tracking step ran `rm -f .planning/current-agent-id.txt` BEFORE the existence check that read it, so the interrupted-agent branch and the Task resume prompt it exists to offer were unreachable (#3795) — a kill -9 mid-executor left the file, and the next run deleted it before looking. The read now precedes the clear; fresh-run semantics (no stale id leaking into the new spawn) are preserved. A structural guard pins the order. Emitted-Drift-Ack-Growth: execute-plan.md — #3795: +bytes from reordering the interrupted-agent read before the rm plus the explaining comment * chore(#3795): changeset fragment (pr number backfilled after PR creation) * chore(#3795): backfill changeset PR number (4006) --------- Co-authored-by: sim <sim@local> * fix(#3799): scope legacy cleanup to the install's resolved config dir and add --no-legacy-cleanup * chore(#3799): changeset fragment (pr number backfilled after PR creation) * chore(#3799): backfill changeset PR number (4013) * test(#3799): mark the legacy-name fixtures with gsd-allow-legacy-name --------- Co-authored-by: sim <sim@local>	2026-08-28 20:16:50 -04:00
Tom Boucher	dd4f179672	feat(#3970): per-task external-tracker content-resolution seam (#4000) ... * feat(#3970): per-task external-tracker content-resolution seam Implements ADR-3646 (Phase 1, #3970): a `<task tracker-id="...">` attribute plus a new optional `taskContentResolver` capability-manifest field let a capability resolve a task's action/verify/acceptance-criteria/read_first/done content from an external issue tracker instead of PLAN.md's inline body. - src/plan-document.cts: parses the `tracker-id` attribute into `PlanTask.trackerId` - src/task-content-resolution.cts: new leaf module — split/find/build/resolve, with a hard-halt (throw) contract on ambiguous/failed/timeout/malformed resolution, never a silent fallback to possibly-stale inline text - src/task-command-router.cts: new `task resolve-content --plan --task-id --raw` CLI verb wiring the module into a real process exit code - gsd-core/bin/lib/capability-validator.cjs: validates the new `taskContentResolver` manifest field (feature-role only, cross-capability trackerPrefix uniqueness) - gsd-core/workflows/execute-plan.md, gsd-core/references/loop-hook-dispatch.md, docs/reference/capability-manifest.md: wire the seam into the per-task loop and document it as a new `execute:task` point outside the existing contribution/step/gate vocabulary (unconditional in autonomous mode) Closes #3970 * fix(#3970): gate checkpoint tasks out of content resolution, close trackerPrefix grammar parity gap, cover path-traversal guard Standards/Spec code-review pass on the task-content-resolution seam (ADR-3646 Phase 1) found three defects: 1. execute-plan.md's task-content-resolution bullet fired on any tracker-id-bearing task with no check that it wasn't type="checkpoint:*", contradicting ADR-3646 Decision 1 (a checkpoint task must never enter resolve-content). plan-document.cts already parses trackerId: null unconditionally for checkpoint tasks; only the workflow prose needed the fix, so the bullet now explicitly excludes checkpoint tasks. 2. task-content-resolution.cts's parseResolverDeclaration accepted any non-empty trackerPrefix with no grammar check, while capability- validator.cjs's KEBAB_RE enforces kebab-case at install time — a Generative Fix Divergence gap. Added the same grammar (as a literal regex, documented as intentionally not shared across the .cts/.cjs build boundary) plus a parity test asserting the two surfaces agree across a valid/invalid trackerPrefix table. 3. task-command-router.cts's routeResolveContent path-traversal guard on --plan had zero test coverage. Added a test exercising a ../../../etc/passwit-shaped path and asserting the USAGE rejection names the offending path. * fix(#3970): sanitize resolver diagnostics and cap resolver timeoutMs Two findings caught by an isolated security-review pass on the task content resolution seam: - ResolverFailedError/ResolverMalformedOutputError embedded raw, unsanitized subprocess stderr/stdout (attacker/model-influenced via the tracker-id argv token) into .message. A hostile or buggy resolver could smuggle a newline plus a forged "Error: " line, or terminal escape sequences, into a diagnostic io.cjs's error() writes verbatim to stderr. Fixed at the constructor (task-content-resolution.cts) via io.cjs's existing formatDiagnosticToken(), so every caller of resolveTaskContent gets a safe .message by construction. - capability-validator.cjs's validateTaskContentResolverFields had no upper bound on taskContentResolver.invoke.timeoutMs, letting a manifest declare an effectively unbounded value and defeat the "bounded subprocess" design intent. Added a 120000ms ceiling specific to this field, without touching the shared isPositiveIntegerMs() helper (still used unbounded by the reviewer lane's timeoutFloorMs and probe timeoutMs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3970): fix gsd-test failures — stale prose allowlist line and stderr-vs-message assertion gsd-test (remote dockerized matrix) came back red with 5 failures on this PR; all five are real defects, fixed here. - tests/no-bare-gsd-tools-command-position.test.cjs: PROSE_ALLOWLIST's execute-plan.md entry pointed at line 415, which ffc190df4's checkpoint-exclusion caveat (added near line 221) shifted down by one line. The actual "validated downstream by gsd-tools uat classify-coverage" descriptive mention now sits at line 416. Updated the allowlist entry's line number to match. - tests/task-command-router-resolve-content.test.cjs: the path-traversal test asserted the outside-project-scope diagnostic against the thrown ExitError's own .message. io.cts's error() (ADR-3889) writes its human-readable message to fd 2 via writeAllSync and then throws a bare `new ExitError(1)` with no message argument — by design, so the exception carries no duplicate text and the thrown ExitError's message defaults to "process exit 1" (cli-exit.cts's ExitError constructor). Root cause was the test, not the source: task-command-router.cjs's outside-project-scope rejection already calls error() correctly and the diagnostic text is genuinely emitted, just on fd 2, not on the exception. Fixed the test to capture fd-2 writes (mirroring tests/estimate-calibrate.test.cjs's runCalibrateExpectError and this same file's own captureStdout for fd 1) and assert against the captured stderr text instead of err.message. This was masked locally because a manual `node -e` sanity check that only inspects the caught exception's .message cannot see what the real node:test run actually failed on. Emitted-Drift-Ack-Growth: execute-plan.md — adds the ADR-3646 task-content-resolution bullet and checkpoint-exclusion caveat to the per-task execute loop; a real behavioral prose addition, not incidental bloat. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3970): backfill changeset PR number (pr:0 -> pr:4000) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-08-28 13:17:04 -04:00
Tom Boucher	d98b55562c	enhance(#3910): the raw terminator is banned by construction (#3980) ... * enhance(#3910): move the last src/ terminators onto the seam Phase 6 bans the raw terminator by construction, which it cannot do while violations stand. A census found 12 sites the rule would flag; nine of the ten unsanctioned ones were owned by no phase of the epic at all — a coverage hole in the decomposition, since P0-P2 are infra, P3 the gate modules, P4 the scanners, P5 the fragments, P7 the hooks, P8 io.cts, and P6 itself only adds the rule. `src/**/*.cts` now holds exactly 2 raw exits, both inside `terminateNow`, the single sanctioned site. `io.cts`'s `error()` is the interesting one. It was first called substantive on "dozens of callers, contract risk" — asserted, not measured, and the measurement refuted it: 289 call sites, zero inside a try whose catch would swallow a throw. The real obstacle was structural instead: `terminateNow` cannot emit exit 1, because ADR-3889 §1 makes 0 and 1 unallocatable and `nameForExitCode(1)` throws. So the only route is `ExitError` under `runMain`, which sets exitCode and writes stderr only when the error carries a user message — keeping the existing stderr write and throwing a message-less ExitError is observably identical. That census was still too narrow, and running the CLI proved it. It asked whether the CALL sits in a try/catch; the two regressions that surfaced were interceptors elsewhere on the stack: - `command-routing-hub.cts`'s `dispatch()` swallowed the ExitError into a HandlerFailure, so the caller emitted a duplicated, wrong stderr line on every Hub-routed path. It now rethrows ExitError explicitly — the same shape `gsd-tools.cjs` already used at two dispatch sites, so this follows an established idiom rather than inventing one. - the profile-pipeline router's deliberately un-awaited `.catch(e => error(...))` turned an ExitError rejection into an uncaught exception; it now mirrors runMain's handling. `edge-probe` and `ui-consideration-probe` gained `runMain` wrappers because probe-core's new throwing default would otherwise have escaped them. A follow-up sweep of every dispatcher — 19 command routers, the Hub, the gsd-tools dispatch seams — found no further swallowing catch. The admitted bound: ~1260 non-rethrowing catches repo-wide were scanned structurally but not individually classified. Both real regressions were found by execution, not by reading, so the suite is the detector that matters here. `gsd-tools.cjs:253` stays a raw exit deliberately: it is the ensureRuntimeBuild bootstrap, which runs before cli-exit is required, so the seam does not yet exist. It needs a second allowlist entry, which means #3910's "single allowlist entry" criterion is unachievable as written. Verification runs on the remote runner. Refs #3910 * enhance(#3910): ban the raw terminator by construction Adds local/require-registered-exit and registers it on all four globs: src/**/*.cts, scripts/**/*.cjs, hooks/**/*.js, gsd-core/bin/**/*.cjs. Registering on the .cts glob is load-bearing, not redundant — the emitted .cjs mirrors are globally eslint-ignored, so a rule registered only on the emitted globs is blind to the sources. That is the #3496 lesson, and it is how the previous guard became invisible: n/no-process-exit was 'error' in one block yet fired zero times on all three surfaces that mattered. The dead n/no-process-exit: 'off' block for hooks is deleted in the same PR. Phase 7 migrated every hook, so the exemption now protects nothing. Two allowlist entries, not the one #3910 anticipated. terminateNow's body is detected STRUCTURALLY — a process.exit lexically inside a function of that name — rather than by a path and line number that rots. The second is gsd-tools.cjs's ensureRuntimeBuild bootstrap, an inline disable with its reason at the call site: it runs before ./lib/cli-exit.cjs is required, so the seam does not exist yet and no migration is possible. #3910's 'single allowlist entry' criterion is therefore unachievable as written, and is amended with the measurement rather than quietly missed. The rule is proven able to FAIL, per glob: four positive controls, one for each registered glob. A guard that cannot be shown to fire is not a guard. Four matching negative controls pin process.exitCode as never-flagged — conflating it with process.exit is what inflated this epic's original census 2x. An allowlist case and a near-miss (same shape, different function name) fix the structural detection in place. Verification runs on the remote runner. Refs #3910 * fix(#3910): stop the detached catch from throwing, and scope the allowlist Review findings, one of them a regression the previous fix introduced. _handlePipelineRejection called error() from inside a DETACHED .catch(). error() now throws, so that throw became an unhandled promise rejection — and on Node >=15 with --unhandled-rejections=throw, Node dumps a raw stack trace with absolute paths on top of the clean Error: line. That was impossible before this branch, because process.exit(1) terminated synchronously before any rejection machinery could observe it. The handler now writes byte-identical stderr itself, in both plain and --json-errors form, and sets exitCode in place. This was the THIRD interceptor found, and like the first two it surfaced by running the CLI rather than by reading code. The rule's terminateNow allowlist had no path constraint, so any function anywhere named terminateNow across all four globs inherited it. It now requires the structural nesting check AND a cli-exit.cts basename — still no line numbers to rot. The four per-glob positive controls only varied a filename inside RuleTester, which never resolves eslint.config.mjs. Since the rule is filename-agnostic, all four exercised identical logic and none proved the rule was WIRED — this epic's own failure mode. A registration test now asserts the rule resolves for a real path in each glob, and it is proven able to fail: removing one glob's registration flips the resolved value from [2] to undefined. Three evasions the rule cannot catch (computed member, aliasing, .call/.apply) are documented in its header and pinned by tests, labelled as known limits rather than endorsed, so a future change that starts catching them is a deliberate diff. Refs #3910 * docs(#3910): document the raw-terminator ban Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it, not hand-edited). How-To: docs/how-to/resolve-a-raw-terminator-finding.md, indexed from docs/README.md — a contributor whose code trips the rule picks among three replacements by surface (runMain/ExitError for a CLI path, terminateNow for a hook, process.exitCode where the process should drain), and needs to know why process.exitCode is correct and never flagged, since conflating the two is what inflated this epic's original census 2x. The page also names the three patterns the rule cannot catch and says plainly that using one to dodge it is a review finding, not a fix — documenting them without that sentence would read as a sanctioned workaround. docs/INVENTORY.md deliberately untouched: eslint-rules/ is not a tracked family in the manifest (verified — a regen produced a zero diff), so a hand-written row would desync the table from the family it claims to belong to. Refs #3910 * fix(#3910): a catch that sniffs the message swallows an ExitError The remote run returned 41 failures, and one of them was a live production regression rather than a test artifact. `cmdMilestoneComplete`'s unstarted-phase guard re-threw only when `e.message.startsWith('Cannot mark milestone complete:')`. `error()` used to `process.exit(1)`, uncatchable, so the guard always fired. It now throws an ExitError carrying no message, the string test fails, and the ExitError was silently swallowed — the guard stopped blocking milestone completion entirely. Proven against the real CLI: pre-fix, a milestone with an unstarted phase archived at exit 0; post-fix it is blocked at exit 1 with the intended message. That is a guard that silently stopped guarding, which is this epic's thesis appearing inside the phase meant to enforce it. Worth stating plainly: an earlier census DID examine this site, saw a `throw e`, and classified it as rethrowing. It was wrong — the rethrow is conditional, and a conditional rethrow on an inspected message is indistinguishable from an unconditional one unless you read the predicate. So the class was swept rather than patched where it was tripped over. An AST census of every CatchClause across src/, gsd-core/bin/ and scripts/ found 38 conditional rethrows. Two more had the same defect and are fixed the same way: `config.cts`'s `'No config.json'` sniff and `gsd-tools.cjs`'s `e.name === 'WindowsError'`. The remaining 25 are provably unreachable — every one wraps a bare fs, YAML, manifest-require or git-exec primitive that cannot throw ExitError — and two were scanner false positives, both explained. Each fix is an unconditional `instanceof ExitError` rethrow placed BEFORE any inspection, matching the idiom command-routing-hub and gsd-tools already used. Residual bound, stated rather than implied: zero known-reachable unfixed sites, contingent only on error() never later being called inside one of those 25 primitive try blocks. The remaining failures were harness artifacts, and the harnesses were corrected to the new contract rather than the assertions weakened. Tests that mocked `process.exit` to observe termination now catch ExitError and assert its code; tests parsing stderr as a single JSON object still assert exactly that, with their ad-hoc `node -e` scripts wrapped in runMain so it is true. milestone and phase-resolution-parity needed no test change — they were correctly written against the real bug and are what caught it. Verification runs on the remote runner. Refs #3910 * chore(#3910): backfill the changeset PR number Also reframes the fragment to lead with the user-visible change — the milestone guard blocking again — rather than the narrowest of the three fixes. Refs #3910 --------- Co-authored-by: sim <sim@local>	2026-08-28 03:15:39 -04:00
Tom Boucher	2ea5efc151	enhance(#3911): hooks declare their crash policy (#3960) ... * enhance(#3911): give hooks an exit seam that needs no build ADR-3889 Phase 7 foundation. The 19 shipped enforcement hooks hold 91 of the epic's 128 terminators and cannot reach `terminateNow` today. The obvious route — requiring `gsd-core/bin/lib/cli-exit.cjs`, as gsd-agent-isolation-guard.js already does for two other modules — is rejected. That precedent carries its own warning (#3582): those files are tsc output, gitignored and absent on a raw plugin-marketplace or git-clone install, so the hook must first call ensureRuntimeBuild() to self-heal. Making the module a hook needs IN ORDER TO TERMINATE depend on a build inverts the dependency, and its failure mode is precisely the fail-open this phase exists to remove: a guard that cannot terminate cannot deny. `lint-hooks-runtime-build-seam` already encodes that concern, and Design B would have had to add an ensureRuntimeBuild() call to all 19 hooks to satisfy it. So `hooks/lib/` becomes a third emit location for cli-exit and a fifth for the registry, preserving the invariant `src/cli-exit.cts`'s own header states: it imports nothing but node:fs and its sibling registry, and the generator dual-emits that sibling alongside each copy so a relative require resolves next to whichever copy loaded it. Shipping needed no change — build-hooks.js already declares HOOKS_SUBDIRS_TO_COPY = ['lib']. Proven, not asserted: the two files are copied into an otherwise-empty tmpdir and a child process requires them and terminates — PASS exits 0, HOOK_DENY exits 2 with the payload on both stdout and stderr. That test fails the moment the hooks copy gains a require reaching outside hooks/lib/. Also fixed inline: the registry's fifth target let any `--write` test overwrite the real committed hooks/lib/exit-code-registry.js, because the test helper derived only three of the other output paths. It now redirects all five, and a regression test asserts every committed artifact is byte-identical after a redirected write. Install-tree goldens pick up the two new shipped paths across 11 runtimes — insertions only, no removals. lint:ci was green while they were stale, so this was found by regenerating rather than by a gate. Verification runs on the remote runner. Refs #3911 * enhance(#3911): declare a crash policy, and migrate the write guard Adds `hooks/lib/hook-exit.js` — the hook-facing vocabulary over `terminateNow`, hand-written because the cli-exit copy beside it is generated: allow(payload) exit 0 deny(payload, stderr?) exit 2 crash(onCrash, payload) whichever the hook DECLARED `crash()` takes the policy as a required argument with no default, which is the whole mechanism: fail-open by accident stops being expressible. A hook must name ALLOW or DENY at the call site, and an unrecognized value terminates INTERNAL rather than guessing. Fail-open stays legal; fail-open by omission does not. `gsd-write-guard.js` is the first hook migrated, all 12 sites, and it exposed a gap in the seam. `terminateNow`'s doc comment justified its fd-2 write by citing this hook's `emitBlock` — but modeled it as sending the same bytes to both streams, when `emitBlock` actually sends full JSON to stdout and only the bare `reason` string to stderr, because Kimi's hook bus feeds stderr verbatim back to the model. Migrating as written would have turned a readable sentence into a JSON blob for Kimi-backed agents. #3911 requires both "all 19 hooks terminate through terminateNow" and "no hook's effective default changes". Those are jointly satisfiable only by teaching the seam to carry a distinct stderr payload, so `terminateNow` gains an optional third argument: omitted, behavior is byte-for-byte what it was; a string is written raw, which is exactly the Kimi case. The doc comment's inaccurate claim about emitBlock is corrected in place. Proven rather than asserted: the pre-migration file is reconstructed from HEAD and driven with the same catastrophic-shrink payload as the migrated one — exit code, stdout and stderr all byte-identical. Verification runs on the remote runner. Refs #3911 * enhance(#3911): all 19 hooks terminate through the seam Migrates the remaining 18 enforcement hooks onto allow/deny/crash. An AST walk now reports zero `process.exit(` call sites across every `hooks/*.js` — down from the 91 the census measured. Each hook with an outer catch declares its policy once, at module top, with the reason that policy is right for that specific guard: a read guard that cannot scan must not block the read; a statusline that renders every prompt must degrade rather than crash; an injection scanner must not retroactively block a result already returned. Those sentences are the deliverable — they are what turns fail-open-by-accident into fail-open-on-purpose. No hook's effective default changed. Wiring exposed two defects, both fixed here rather than noted. A SECOND stdout/stderr-splitting site turned up in `gsd-workflow-guard.js`'s `emitForceAddBlock`, matching the pattern already known from the write guard — full JSON to stdout, bare reason to stderr for the Kimi bus. It uses the `stderrPayload` argument added in the previous commit, which is now carrying its second real caller rather than one special case. More seriously, `terminateNow` emitted both streams inside ONE try, so a payload that failed to serialize aborted before the stderr write ever ran. The two windsurf guards write nothing to stdout on a block and only a reason string to stderr, so `deny(undefined, reason)` exited 2 with EMPTY stderr — a deny that silently loses its reason, which is the exact "fails with success" class this epic exists to close. The streams are now emitted independently, each with its own guard, and `undefined` means "nothing to write for this stream" rather than an error. Regression tests inject a throwing write on one fd and assert the other still receives its payload; they fail against the single-try version. Byte-identity was proven per hook, not assumed: each pre-change file is reconstructed from HEAD and driven side by side with the migrated one across its normal path, its deny path, malformed stdin and empty stdin — exit code, stdout and stderr compared. Verification runs on the remote runner. Refs #3911 * enhance(#3911): harden the three shell hooks, and pin every hook's policy `gsd-phase-boundary.sh`, `gsd-session-state.sh` and `gsd-validate-commit.sh` gain `set -euo pipefail`. The expected hazard did not materialize, and that is worth recording: every intentionally-non-zero command in all three is already the condition of an `if`/`elif`, which `set -e` never fires on, and none of them reads a possibly-unset variable or pipes through a grep that may legitimately match nothing. No `|| true` guards were needed. Each hook was still checked command-by-command before the flags went in rather than after. Twenty-one before/after cases across the three hooks — disabled and enabled, planning and non-planning, missing STATE.md, malformed JSON, the Kimi payload shape, quoted and unquoted `-m`, valid and over-long Conventional Commits — all match on exit code, stdout and stderr. The hardening is shown to actually fire, not merely added: with a stubbed `node` that fails at the JSON-emit step, phase-boundary and session-state go from silently exiting 0 with empty stdout to failing visibly with the error surfaced. No such case could be constructed for `gsd-validate-commit.sh`, whose every statement already sits inside an if-condition — recorded as unproven rather than claimed. `tests/hooks-crash-policy.test.cjs` adds the per-hook coverage the issue asks for, table-driven over all 19 hooks rather than 76 hand-written cases: normal allow, deny where a deny path exists, crash-honors-the-declared-policy, and an unclosed-stdin case — the one `process.exitCode` structurally cannot serve. The deny assertions encode each hook's ACTUAL stream split rather than a uniform shape, since four of the six deliberately differ. A drift guard enumerates `hooks/*.js` and fails if a terminating hook is ever added without a row. Writing those tests surfaced two hooks that emit a block decision in their JSON body and exit 0. Both were checked rather than assumed, and neither is a fails-with-success: `gsd-read-injection-scanner.js` is PostToolUse, where the tool has already run and exit 2 has no meaning, and `gsd-cursor-subagent-start.js` follows Cursor's JSON-body protocol. They are deliberately left alone — a mechanical sweep to `deny()` would have broken exactly these two. Verification runs on the remote runner. Refs #3911 * fix(#3838): the commit validator says when it could not validate #3911 claims to subsume #3838. Measurement said otherwise, so this closes it for real rather than by assertion. `set -euo pipefail`, added earlier on this branch, does NOT fix #3838: bash exempts a command used as an `if` condition from `set -e`, and all three of the hook's swallow-and-pass sites are exactly that shape. Verified against the hardened hook with a node shim that fails only the classifier call — a non-conforming commit still exited 0 with empty stdout AND empty stderr, indistinguishable from "your commit conforms". That is the defect verbatim. All three sites named in #3838 now capture the real exit status instead of consuming it as a condition, and each distinguishes its genuine negative from "could not run": - the classifier: 0 = is a git commit, 1 = genuinely not one, anything else = could not classify. Its `node -e` now wraps the require and the call in try/catch and exits 3 on a throw, so a broken require chain can never be mistaken for `isGitSubcommand` legitimately returning false — which is the arm that matters, since `token-scanner.cjs` is a gitignored build artifact and a fresh checkout lands there. - the opt-in config read and the JSON command extraction get the same treatment. On "could not run" the hook emits a diagnostic to stderr naming which check failed and why, then exits 0. The issue confirms this is safe — it is a PreToolUse hook, so stderr does not disturb the JSON protocol — and ranks it the smallest sufficient fix. The gate still fails open, but it can no longer do so silently, which is the whole complaint: a validator that disables itself quietly costs more than one that is absent, because it is trusted. Both controls are unchanged and pinned by tests: a conforming commit still passes silently, a non-conforming one still exits 2 with its existing block payload. The defect test asserts stderr is non-empty and names the failure; it fails against the pre-fix hook. Verification runs on the remote runner. Refs #3911, #3838 * docs(#3911): document the hook crash-policy contract Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it), INVENTORY rows for the three new hooks/lib files, and an ARCHITECTURE note on the hooks section. How-To: docs/how-to/declare-a-hook-crash-policy.md, indexed from docs/README.md — a hook author now has to choose and declare a crash policy, which is more than one step and crosses into which harness protocol their hook speaks. It covers allow/deny/crash, writing an ON_CRASH reason that is actually useful, when a deny needs a distinct stderr payload, the two hooks whose harness reads a JSON-body decision and must NOT use deny(), and what to do when a check cannot run at all — with #3838 as the worked example. Refs #3911 * test(#3911): prove the seam actually ships, and stop hand-rolling temp cleanup Two review findings. The acceptance criterion 'hooks/dist/** stays in parity via the build seam (lint:hooks-runtime-build-seam)' was misstated and unmet: that lint checks something else — that a hook requiring a compiled gsd-core/bin/lib module also calls ensureRuntimeBuild(). Nothing exercised that the three new hooks/lib files reach hooks/dist/lib at all. That gap is not theoretical: #770 is a recorded ship-blocking bug where a new hook never shipped because a copy list missed it. The suite now builds dist through the repo's own ensureBuiltHooks(), byte-compares each shipped copy against its source, and spawns a child that requires the SHIPPED dist copy and denies — which is what catches a copy that exists but cannot resolve its sibling registry. gsd-validate-commit.sh hand-duplicated mktemp/run/rm three times; one idempotent trap on EXIT replaces them, guarded so cleanup cannot alter the exit status. Behavior-neutral across five cases, with temp-file counts taken before and after each run. Refs #3911 * fix(#3911): stage transitive hook lib requires, not just one level The remote run returned 7 failures across 3 real causes. The important one is a PRODUCTION bug this phase exposed rather than caused. `writeCursorHooksJson` scanned each hook script for `./lib/X` requires exactly one level deep and never re-scanned the lib files it staged for their own sibling requires. Nothing had a transitive lib dependency before, so the gap was invisible. Adding hook-exit.js -> cli-exit.js -> exit-code-registry.js made real Cursor installs ship a bundle that dies at require time with MODULE_NOT_FOUND. It now walks to a fixed point, and a real installed Cursor hook runs to completion. The staging harness in shared-hooks-dir-resolution hand-copied its fixture, so the injection scanner crashed at require time and its exit-1 was being read as a policy decision. Migrated to copyScriptWithDeps, which walks the require graph — the repo's recorded rule for this class, since adding another copyFileSync keeps it alive for the next person. The missing-lib-source test in cursor-hook-workspace-roots hardcoded which lib file it expected to be named in the abort message; the same throw now fires for a different file first. Its assertion is unchanged in substance — staging still must abort rather than ship a broken hook — only the name is no longer pinned. The last one was my own test asserting an uppercase reason code. Measured against origin/next: the pre-change hook emits the same lowercase 'config_unreadable', so the test was wrong, not the migration. Corrected to the real value rather than making the code match the test. Verification runs on the remote runner. Refs #3911 * chore(#3911): regenerate the cursor install-tree golden The staging fix means a Cursor install now correctly carries the two transitive lib files it was silently missing. Additive only — no path was removed. The golden diff is the evidence the packaging defect was real. Refs #3911 * chore(#3911): backfill the changeset PR number Refs #3911 * fix(#3911): a git probe that timed out is not a negative A macOS CI lane failed three deny cases at 2084ms, 2112ms and 2177ms — just past the 2000ms budget these hooks give their git probes. The three that passed took 72ms, 595ms and 651ms. Under shard contention `git rev-parse` overruns, the hook reads the non-zero result as "not a git repo", and allows with exit 0 and empty stdout AND empty stderr. Under load, the guards silently stop guarding. That is ADR-3889's thesis exactly, sitting inside the security hooks this phase is about. The repo had already recognized the class in one place — gsd-cursor-subagent-start.js fail-closed-denies on `git_timed_out` (#3045) — but nowhere else. `hooks/lib/git-probe.js` classifies a probe's outcome, distinguishing a real non-zero exit from ETIMEDOUT, a signal kill, and a spawn failure, rather than folding all four into `status !== 0`. Three guards route their eight git probes through it. The resolution is the same shape #3838 took, and the same one that issue endorsed as smallest-sufficient: fail open, but loudly. **No exit code changes on any path** — a developer on a loaded machine is still not blocked, which keeps #3911's declaration-pass contract intact for exit codes. What changes is that the hook now says on stderr which probe could not answer, instead of presenting silence as a clean verdict. Scope was checked across every hooks/*.js, not just the three that failed: gsd-agent-isolation-guard spawns no git; gsd-statusline's two probes gate only a cosmetic display segment, not an allow/deny decision, and are left alone. The C2 deny assertion was a real-race test — it demanded exit 2 while a slow git legitimately yields 0. It now requires the hook to either deny, or allow with a diagnostic naming the probe that could not run; a silent allow still fails, so the assertion is not vacuous. A deterministic regression stubs git on PATH to sleep past the budget rather than waiting for load to reproduce it. Verification runs on the remote runner. Refs #3911 * test(#3911): a PATH shim cannot intercept the hooks' git spawn on Windows The deterministic timeout regression stubbed git on PATH and asserted the guard reports rather than silently allows. It passes on Linux and macOS and failed on Windows in 83ms and 176ms — the stub was never invoked at all. Mechanism: the hooks call spawnSync('git', args) with no shell:true, so on Windows CreateProcess resolves git.exe only and never a PATH .cmd shim. The git.cmd branch could not have worked and is removed rather than left implying a Windows path that does. Adding shell:true to the hooks to serve a test would change product behavior and widen an injection surface, so the case is skipped on win32 only, with the mechanism written into the skip reason so a future reader does not 'fix' it that way. Linux and macOS keep the coverage, and macOS is where the underlying fail-open was actually caught. Refs #3911 --------- Co-authored-by: sim <sim@local>	2026-08-27 22:21:10 -04:00
Tom Boucher	03b7125293	enhance(#3909): a probe that could not run no longer asserts a verdict (#3944) ... * test(#3909): failing-first suite for the fabricated probe fallbacks Binds the four fabrication sites found by executing the surfaces (ADR-3889 failure class (c)), each with a positive control so an over-firing fix goes red: - the blocking api-coverage.verify-pre gate certifying "no external-API integration" from a zero-byte phase scope - the assumption-delta query route scanning an unresolvable phase section as the empty string and reporting it as an examined negative - both capability fragments' probe fallbacks, which append a fabricated verdict rather than replacing, and fire on the legitimate exit-1 negative Verification runs on the remote runner. Refs #3909 * enhance(#3909): a probe that could not run no longer asserts a verdict ADR-3889 Phase 5. Four sites turned a failed or unexamined probe into a confident negative; each now reports what it could not establish. - check api-coverage.verify-pre: a phase with no plan body and no roadmap section ran detection over zero bytes and PASSED the blocking seal gate, certifying "no external-API integration" from input it never read. It now holds with scope_unavailable. The discriminator is bytes examined, never signals found, so a phase whose plans are real and simply carry no API vocabulary passes exactly as before. - query assumption-delta scan: an unresolvable phase section was scanned as the empty string and reported as an examined negative. It now returns {skipped, reason: phase_unresolved}, still at exit 0 — an ADR-2980 degraded result in the payload, leaving the gsd-tools exit projection to P8. - both capability fragments: `|| echo '{"detected":false}'` appended rather than replaced, and fired on the legitimate exit-1 negative, so a correct answer and an honest skip both arrived as two concatenated objects. They now keep the probe's own payload and manufacture only an explicit probe_unavailable skip when the probe produced nothing at all. Every registered outcome is more restrictive on a blocking gate, so this can turn a false green red and never a red green. Docs: FEATURES 156, CONFIGURATION (both keys), references/api-coverage.md seal-time outcome table, and a new how-to for the reason-code vocabulary. Verification runs on the remote runner. Closes #3909 * test(#3909): correct the stale unknown-phase assertion `unknown phase → detected:false, no throw (graceful)` scanned phase 999 against a two-phase roadmap and asserted `detected === false`. That pinned the fabrication as intended behavior: the phase does not exist, so the detector was handed the empty string and its "no core assumption changed" answer described nothing that was ever read. It now asserts the skipped-with-reason shape. The graceful-degradation contract the test was actually protecting — the query succeeds and does not throw on an unknown phase — is unchanged. Found by code review, not by the author. Refs #3909 * docs(#3909): author the FEATURES entry in its generator source `docs/FEATURES.md` is generated by `scripts/gen-features.cjs` from the per-feature fragments in `docs/features/`. The API-coverage entry was edited in the generated file, so the next regeneration silently dropped it. The text now lives in `docs/features/api-coverage-gate.md` and `docs/FEATURES.md` is regenerated from it, leaving the shipped file byte-identical and its content actually derivable. Caught by `lint:generated-sync`. Refs #3909 * test(#3909): bind the skip to "not found", and pin the discriminator The first verification run went red on one case, and the case was wrong rather than the code. `getRoadmapPhaseWithFallback` returns `null` for an unknown phase and for a missing ROADMAP.md, but for a section whose body is whitespace-only it returns the heading line alone — which is not empty. So a body-less section WAS found, and reporting `detected:false` over its heading is a real negative, not a fabrication. The test had assumed the resolver yielded `''` there. Correcting the test rather than the resolver keeps `skipped` bound to the distinction the issue asks for — found versus not found — and avoids diverging `assumption-delta scan` from `roadmap.get-phase`, which the fragment documents as sharing one resolver. Also adds the seeded property the test matrix had promised: for any plan body, the scope read back is whitespace-only exactly when the body was. That pins the gate's discriminator to bytes examined, so it cannot quietly become "no signals found", across unicode whitespace and CRLF. `docs/INVENTORY.md` picks up the reference doc's new seal-time outcome table — surfaced by the co-change gate, not by a lint failure. Refs #3909 * chore(#3909): backfill the changeset PR number Refs #3909 --------- Co-authored-by: sim <sim@local>	2026-08-27 15:50:12 -04:00
Tom Boucher	c5f2b94b27	enhance(#3907): gates report no-input instead of a verdict they never reached (#3932) ... * feat(#3907): gates report no-input instead of asserting a verdict they never reached The three stdin-reading gates bound 2 to a stdin read error only, with no arm for stdin closed at zero bytes - so empty input flowed into the detector, found nothing, and exited 1, which each module's own comment defines as a negative verdict. An unset PHASE_SECTION made the UI gate assert the phase has no UI. Empty and whitespace-only input now exit NO_INPUT, and a read error exits UNAVAILABLE rather than a locally-invented 2, both resolved through the registry and delivered by terminateNow. The exit code was only half of it: under --json the same input emitted {detected:false}, byte-identical to the fabricated payload #3909 exists to fix, and the blocking coverage gate reads that payload. Empty input now emits the in-tree {skipped:true,reason} form with no detected key at all. teams-status is excluded: it never reads stdin and has no invented 2, so the four-module framing in the issue and ADR is wrong. The dead root bin/lib/ui-safety-gate.cjs is deleted - no installer reference, no workflow invocation, and the live fallback chains are for other modules. Its removal restores the unit tests to the module that actually ships; they had been asserting the stale copy's two-field shape, which is why it drifted unnoticed. * fix(#3907): drive gate tests through the process seam, and make removed-but-needed basename-precise CONTRIBUTING requires every subprocess go through tests/helpers/process-seam.cjs; two of the three gate suites hand-rolled spawnSync while the third, added in the same change, used runNode correctly for the identical injection case. Converted the blocks this change added, leaving pre-existing ones alone. Deleting one of two files sharing a basename made lint-removed-but-needed report 14 references that were all to the surviving canonical module - the false-positive class its own docstring names. It now matches on the deleted file's full path when a surviving file shares its basename, which is more precise rather than weaker: a genuine full-path reference still fails, and behaviour is unchanged when no basename collides. It immediately caught a docstring on this branch that spelled the deleted path. * test(#3907): update the one existing assertion that pinned the old empty-stdin verdict A pre-existing test asserted exit 1 on empty stdin - the defect this phase removes - and was missed because the change added new blocks without auditing existing ones pinning the old contract. Audited the rest: the other three status-1 assertions in that file all feed real input and are the genuine-negative controls that must keep returning 1, so exactly one was stale. The retired 2 is gone from the describe's contract comment too. * chore(#3907): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-27 11:37:31 -04:00
Tom Boucher	941b62249e	enhance(#3906): two terminators over one registry, with a versioned exit projection (#3924) ... * feat(#3906): two terminators over one registry, with a versioned projection Adds terminateNow (write-then-terminate, for callers that cannot wait for the event loop) beside runMain (drain-then-exit), both projecting through one shared function so they cannot disagree - the parity the ADR makes mandatory. A failed write does not change the exit code: letting it propagate would fail a hook open, which is what the fail-closed branches exist to prevent. The projection is versioned. v1 reproduces today's integers, including keeping a payload-carried degraded result at exit 0 - ADR-2980 ratified that across 60 sites and declined normalizing it on measured blast radius. v2 applies the registry. --exit-contract=v2 or GSD_EXIT_CONTRACT=v2 selects it; an unrecognized version throws rather than silently defaulting. The registry is now emitted beside both copies of the exit module, so it resolves as a sibling in the built tree and in the committed scripts/ copy that must load on an unbuilt clone. * fix(#3906): actually restrict code 2 to terminateNow, and generate the registry's type The claim that terminateNow is the only place 2 can be produced was false: runMain's outcome arm applied no guard, so runMain(()=>'HOOK_DENY') set exitCode 2 through the drain path - and the parity matrix demonstrated it while calling it parity. runMain now refuses any outcome projecting to the hook-protocol code, gated on the code rather than the name so an alias cannot slip past, and the matrix asserts the restriction instead of contradicting it. The ambient type for the generated registry was hand-written with no gate against the generator's actual output - the declared-surface-diverges-from-runtime defect class this epic exists to close, reintroduced inside it. It is now a third generated artifact covered by the same --check. Also converts every test-body try/finally to t.after(). * test(#3906): derive the glossary fixture's dependencies instead of hand-listing them Adding a require to scripts/lib/cli-exit.cjs broke 31 tests in one suite that built its fixture from a hand-written dependency list, so the new sibling was absent and the copied script could not load. copyScriptWithDeps walks the require graph and exists for exactly this class - #3412 paid the same bill when one new require broke 82 tests across two suites. Migrating rather than adding another copyFileSync line keeps the class closed. The other nine suites referencing that path were triaged; none copies-and-spawns, so none needed migrating. * fix(#3906): enumerate the new shipped file, drop a vendor name from shipped data, and fix three test defects install: scripts/lib/exit-code-registry.cjs was missing from GSD_SCRIPTS_LIB_FILES, so it shipped to every install and orphaned on uninstall. The registry gave HOOK_DENY a meaning naming one harness, and that string ships into every runtime's tree - a guard correctly caught it leaking into the hermes and qwen installs. The registry is runtime-neutral infrastructure; the vendor name belongs in the ADR, not in shipped data. Two more fixture harnesses built their trees from hand-listed dependencies and broke on the new require; both migrated to the derived helper, and all 23 copy-and-spawn candidates were enumerated so the class is closed rather than patched. One generator test used a fixture code that collided with a real allocation, so the generator correctly reported a duplicate where the test expected drift. The large-payload test embedded a 256KB literal in the child's argv, exceeding Linux's 128KiB MAX_ARG_STRLEN so the child never started - it now builds the payload inside the child. * chore(#3906): backfill changeset pr number * docs(#3906): document the exit-code contract selector P2 is the first phase of this epic with a user-invocable surface, so the flag and env var owe a reference entry. Records what actually differs between v1 and v2 today (one outcome), that an unrecognized value is rejected rather than silently defaulted, and the fail-safe property that makes switching safe. --------- Co-authored-by: sim <sim@local>	2026-08-27 03:31:02 -04:00
Tom Boucher	39673ae9ff	fix(#3738): antigravity global skills/agents install to ~/.gemini/config (#3921) ... * test(#3738): antigravity global skills/agents must resolve under ~/.gemini/config Regression tests (RED first): --skills-root and gsd-tools query surfaces, install-plan dest dirs, and converter skills-path rewrite. * fix(#3738): antigravity global skills/agents install to ~/.gemini/config Antigravity's machine-local discovery scans ~/.gemini/config/{skills,agents}; the configHome (~/.gemini/antigravity) is deprecated for artifacts. Declare the ADR-1239 skills/agents 'home' override on the antigravity global layout — the same mechanism codex uses (.agents) — and divert ~/.claude/skills/ references in converted global content to ~/.gemini/config/skills/. configHome, settings, probe/migration semantics, and the local .agents layout are unchanged. * fix(#3738): retire deprecated configHome artifacts via installer migration 010 Next install converges an existing antigravity install: manifest-managed skills/gsd-*/ and agents/gsd-*.md under the configHome (a location AGY does not scan) are removed — modified files backed up first, unmanifested and non-gsd entries preserved — and now-empty containers retired. Global scope only; the local .agents surface is live. Docs + inventory updated. * fix(#3738): converter sync in bin/install.js, harness emit-root coverage, migration baseline - bin/install.js converter gains the same ~/.claude/skills → ~/.gemini/config/ rewrite as src (ADR-1508 dual copy must stay in sync). - Parity-manifest walk covers home-override emit roots (extraEmitRootsFor) so antigravity's emitted skills/agents stay differential-visible at their new install root; install-tree fixture regen confirms an unchanged key set. - skills-from-commands rule declares the antigravity converter as a runtime-scoped transform; one ack fragment covers the identity-classed workflow whose antigravity copy embeds the old skills path. - Migration 010 checksum baseline + home-override set doc updated; existing tests updated to the #3738 contract (global dest, golden parity via layout dest, integration expectations). * fix(#3738): tolerate an absent extra emit root on baseline-side measurement The base tree's installer predates the home override, so <HOME>/.gemini/config does not exist there; walk() threw ENOENT and the in-job baseline build failed. An absent extra root is the legitimate pre-override shape — skip it. * fix(#3738): review findings — manifest agents root, bare skills-path rewrite, guard comment - writeManifest resolves the agents-kind home override (_kindDestDirSafe), so the manifest records agents at their actual install root and drift detection keeps working (isolated review finding 1, major). - Converter bare forms ~/.claude/skills and $HOME/.claude/skills (no trailing slash) divert to ~/.gemini/config/skills instead of falling through to the retired configHome path (finding 2). - real-home-guard comment updated: antigravity's global agents kind is the first agents-kind home override (finding 3, doc-only). - Regression tests for both behavioral findings. * chore(#3738): changeset fragment (pr number backfilled after PR creation) * chore(#3738): backfill changeset PR number (3921) * fix(#3738): sandbox HOME in tests that install antigravity global artifacts antigravity is the first home-override runtime in the golden-parity and skills-wrapper suites (codex is not in their runtime lists), so those tests never needed HOME sandboxing — the real-home guard now (correctly) refuses their un-sandboxed global installs on CI, where HOME is the passwd home. * fix(#3738): stop the K3 sequential-sandbox env leak; sandbox L2's home-override plans K3's two back-to-back sandboxHome calls leave HOME pointing at the first sandbox once the after-hooks restore (each call saves the env as it found it, so the second saves the first's sandbox as 'original'). On the windows matrix that leaked gsd-k3-qwen-* home into the L2 property, whose antigravity/global run then (correctly) refused via the #3712 real-home guard — antigravity is the runtime that made L2's plan escape into os.homedir(). K3 now manages the env with a single restore; L2 sandboxes HOME per run, mirroring L1. * fix(#3738): L2 property's HOME sandbox must exist on disk The #3712 guard's sandbox exemption fails closed when identify(effectiveHome) is 'absent' — L2 never created its configDir, so on the windows matrix (tmpdir under the real home) the antigravity/global run refused even with HOME sandboxed. Create the per-run sandbox dir and clean it up. --------- Co-authored-by: sim <sim@local>	2026-08-27 02:24:03 -04:00
Tom Boucher	878f25025c	enhance(#3905): the exit-code registry — one number, one meaning, enforced at build (#3920) ... * feat(#3905): the exit-code registry — one number, one meaning, enforced at build A generated registry replaces locally-invented exit codes. Every entry records code, name, meaning, owning module and the decision that authorized it. The generator refuses to build a table where two entries claim one code, two claim one name, a code falls in a range Node or the shell reserves, 2 is claimed by anything but the hook adapter, or an allocation carries no justification. exitCodeFor is pure and total: it throws rather than returning undefined, including for prototype-chain names. Inert by design — nothing emits a registered code until #3906. Every registered code is non-zero, asserted over the whole table, so a caller testing for failure behaves identically for pass and trips for everything else. * feat(#3905): make the registry generator's failures machine-readable Adds a --json mode carrying {ok, reason, context, detail}, where context is a typed payload naming the specifics the prose embedded - which code collided and under which names, which band rejected a code, which field was missing. The tests now assert on that structure instead of regex-matching the generator's stderr, which CONTRIBUTING prohibits, and the CONTEXT.md glossary gains the entry the issue's scope requires. * test(#3905): refresh the install-tree fixtures for the new declaration The registry declaration ships in the install tree, so all 19 golden fixtures needed regenerating. Caught by the remote matrix, not by lint:ci - the install-tree goldens are verified by a test rather than a lint, so a newly shipped file clears every local gate and fails only under the suite. * chore(#3905): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-27 00:10:11 -04:00
Tom Boucher	6b7df61938	enhance(#3881): one YAML parser — vendored js-yaml replaces the hand-rolled dialect (#3888) ... * docs(#3881): answer §8.1's open question and correct three wrong premises ADR-3473 §8.1 carries a blocking open question with a forcing function: it must be answered before any implementation PR for the rule opens. Answered here as (a), a string-coercing adapter, with the measurement that settles it. The sequencing note bet that §8.8's schema would make (b) tractable. Measured against merged reality it does not: only 33 of extractFrontmatter's 78 non-test call sites read STATE.md, and two of the five compensating mechanisms §8.1 lists survive real types, leaving ~31 lines across 3 call sites as the actual prize. Also corrects three claims verified false while answering it. §8.1's justifying sentence names #3349 and #3360 as defects a real parser would fix; both are already fixed on next, confirmed by executing the compiled parser rather than reading it. The guard roster calls lint-frontmatter-scalar-broad-grep.cjs an expected casualty of this rule, but it guards shell grep idioms in workflow bash fences and never touches our parser. The same roster calls lint-vendored-deps.cjs reusable as-is; it is hardcoded to re2js throughout. The last two were caught by applying the rule this amendment records -- a factual claim in this ADR is a hypothesis until the implementing phase executes it -- on its first use. Refs #3881 * docs(#3881): record that §8.1's fork is ill-posed and (a) is not implementable An adversarial pass on the Phase 4 design established by execution that extractFrontmatter is not a YAML parser but a line-oriented scanner whose output is a function of raw source text. Four spellings of the same value collapse to one js-yaml tree but produce four distinct legacy strings, one of them mangled. No adapter over a tree can choose among outputs the tree does not distinguish, so fork (a) -- keep a string-coercing adapter so the existing contract holds -- cannot be built. For any document with a non-scalar value, (a) collapses into (b); about 26 percent of frontmatter-carrying documents have one. Also records three design defects and one new attack surface, all confirmed by execution: catching a parse failure and returning {} would delete the frontmatter block on the next write at eight call sites that conflate empty with unparseable; an empty value yields null where legacy yields {}, and reconstructFrontmatter omits null-valued keys, so the shipped state template's empty progress key would vanish; the #1882 truncation probe is parseYamlRegion itself rather than a pre-parse heuristic, so it cannot both stay unchanged and survive that deletion; and FAILSAFE_SCHEMA still resolves aliases, expanding seven lines to 22.8 MB. The rule is not deferred. The measurement is the deliverable and the re-scoping is recorded as an open question with a forcing function, per section 8's own rule. Refs #3881 * test(#3881): failing-first rows for block scalars, unicode keys and the missing #3594 matrix Creates tests/feat-3594-parser-adversarial-frontmatter.test.cjs, the file the fixture README instructs contributors to register fixtures in but which never existed. Section C: table-driven ownership check over tests/fixtures/adversarial/frontmatter/ so a fixture with no matrix entry fails loudly; six existing fixtures (duplicate-keys, crlf-mixed, unclosed-block, unicode-keys-and-values, null-byte-value, huge-bounded) each get the invariant its README states. B1 blockScalarValueIsNotTheBlockIndicator: parsing commands/gsd/add-tests.md must give argument-instructions the instruction text, not the literal '|'. RED today. B2 blockScalarDoesNotInventATopLevelKey: same parse must not produce a top-level Example key scraped from inside the block body. RED today. B3 unicodeKeyRoundTripsAsIs: the 相 key in unicode-keys-and-values.md must survive parsing; today it is silently dropped. RED today. Refs #3881 * chore(#3881): vendor js-yaml and generalize the vendored-deps guard to a manifest Packaging step for ADR-3473 §8.1: makes js-yaml available to gsd-core/bin/** without promoting it out of devDependencies (promoting broke every installed tree, #3496). gsd-core/bin/lib/vendor/js-yaml.cjs is a verbatim copy of node_modules/js-yaml/dist/js-yaml.js (the self-contained UMD dist bundle, not index.js), exposing load/dump/FAILSAFE_SCHEMA/YAMLException with zero require() calls of its own. src/vendor/js-yaml.d.cts is hand-authored, not copied, because js-yaml ships no upstream .d.ts and @types/js-yaml is not installed. It is deliberately narrow, declaring only the four symbols in use, so anchors/aliases/custom types/loadAll are unreachable from typed code -- a compile-time enforcement of ADR-3473 §8.1's refusal to expand alias resolution for security reasons. Because it has no upstream counterpart it is excluded from the byte-compare. scripts/lint-vendored-deps.cjs is refactored from a script hardcoded to re2js into a table-driven VENDORED manifest (one row per package: upstream/vendored .cjs paths, optional .d.cts paths, twin kind upstream-verbatim vs hand-authored) so a second vendored package does not require a second hardcoded check block, per ADR-3473 §8.3 'one implementation per rule'. The four existing re2js checks (vendored .cjs vs node_modules, vendored .d.cts vs node_modules, src/vendor twin vs bin-side twin, devDependency version pin vs installed version) are preserved unchanged; verified pass/fail identical before and after the refactor, and the guard's ability to fail was re-proven with a deliberate one-byte append to both re2js.cjs and js-yaml.cjs, then restored. docs/INVENTORY.md and docs/INVENTORY-MANIFEST.json (via gen-inventory-manifest.cjs --write, run after build:lib) register vendor/js-yaml.cjs. gsd-core/bin/lib/vendor/README.md documents both vendored packages and the two twin kinds. Refs #3881 * feat(#3881): parse .planning frontmatter with the vendored js-yaml ADR-3473 §8.1: extractFrontmatter's read path is no longer a hand-rolled line scanner. parseYamlRegion, escapeDoubleQuoted, unescapeDoubleQuoted and parseQuotedScalar are deleted (not patched); parsing now goes through the vendored js-yaml (./vendor/js-yaml.cjs) under { schema: FAILSAFE_SCHEMA, json: true }. Everything js-yaml does not do is layered on top, in one place, carrying the seven design-doc consequences: 1. Empty value: a null js-yaml value is coerced to {} (matching legacy's own empty-value contract) so reconstructFrontmatter — which omits null-valued keys — still round-trips a bare `key:` line instead of deleting it. Verified live: progress: with no value survives parse -> reconstruct -> re-parse. 2. Unparseable no longer collapses to a bare {}: a new FRONTMATTER_UNPARSEABLE Symbol (exported), keyed exactly like the existing #3257 FULL_LINE_COMMENTS channel, is carried on the {} returned for malformed/refused YAML. Invisible to Object.keys/entries/JSON.stringify/for-in, so the 70 call sites that never inspect it are unaffected; wiring the 8 hasFrontmatter sites to consult it is a separate change, not done here. 3. Non-scalar object-list items (the four spellings of `- test: a b` that js-yaml collapses into one tree shape) are rendered as a canonical `key: value[, key2: value2]` string per item, keeping the existing array-of-strings value SHAPE. A full corpus differential over all 1702 tracked markdown files found 11 residual divergences from the legacy parser (enumerated in the PR/report), most of them the parser now being MORE correct (a dropped quoted top-level key, the block-scalar/phantom-key defect, a dropped Unicode key). 4. The #1882 truncation probe still runs the one real parser, but derives its key count from js-yaml's own thrown error and mark.line when the whole region doesn't parse cleanly (the dominant real truncation shape: fence opened, well-formed keys, no closing fence). Verified against both the clean-parse and the exception-fallback path. 5. The #3257 comment channel now attributes each pending column-0 comment against js-yaml's own parsed top-level key list (matched by literal key text, in document order) instead of the legacy ASCII-only key regex, so a comment above a Unicode key attaches correctly. 6. Anchors, aliases and merge keys are refused outright (a raw-text pre-scan, since FAILSAFE_SCHEMA still resolves them) — corpus occurrences today: zero. A 7-line billion-laughs fixture is verified refused rather than expanded. 7. A literal U+0000 is swapped for a private-use sentinel before the parse and restored in every resulting string afterward, since js-yaml rejects NUL unconditionally under every schema. escapeDoubleQuoted is deleted and reimplemented via js-yaml's dump() (forced double-quoted style), with control-char hex escapes lowercased to keep serialized output byte-stable (#1779 emitted lowercase); it keeps its exported name and signature for its two other call sites (commands.cts, runtime-artifact-conversion.cts), which need no change. frontmatterDeepEqual, the comment channel, sliceTopLevelFrontmatterSegments, regenerateFrontmatterKey's guard, noOpObjectListSetError and parseMustHavesBlock are all unchanged — retiring them is fork (b) and is not this phase. Refs #3881 * fix(#3881): quote template placeholders and preserve unparseable frontmatter SECURITY.md/UI-SPEC.md/VALIDATION.md wrote frontmatter placeholders as bare {N}/{phase-slug}/{date}, which is valid YAML flow-mapping syntax under the vendored js-yaml parser, not the literal placeholder text intended. Quote them so they parse as strings. Wire the FRONTMATTER_UNPARSEABLE Symbol (exported but unused) at the 8 call sites in state.cts/state-transition.cts that compute hasFrontmatter via Object.keys(extractFrontmatter(...)).length > 0 and reassemble the document without a frontmatter block when false. That check conflated 'no frontmatter' with 'unparseable frontmatter' (both parse to {}), so a document with a merge-conflict marker or refused alias in its frontmatter had that block silently dropped on write. Each site now preserves the exact raw bytes stripFrontmatter removed when the marker is set, leaving the genuinely-empty case unchanged. Refs #3881 * test(#3881): consequence and boundary coverage for the js-yaml migration Rows: A1 emptyValuedKeySurvivesAWrite, A2 unparseableDocumentKeepsItsFrontmatterBlock, A3 unparseableIsDistinguishableFromEmpty, A4 nonScalarValuesCanonicalize, A5 truncationProbeStillFiresOnAnOpenFence, A6 commentsStayOnTheirOwnKey, A7 anchorsAndAliasesAreRefused, A8 aliasExpansionCannotExhaustMemory, F1 UNTERMINATED_KEY_THRESHOLD boundary, F2 alias/nesting refusal bound, F3 frontmatter size boundary (huge-bounded.md + larger). Adds tests/fixtures/adversarial/frontmatter/anchor-alias-bomb.md and its entry in the feat-3594 fixture matrix. Refs #3881 * docs(#3881): document the vendored parser, correct a stale rationale, add a vendoring how-to Refs #3881 * docs(#3881): correct the frontmatter glossary entry Two errors in the entry as first written: it named parseYamlRegion as part of the read path when that function is deleted, and it recorded the eight hasFrontmatter call sites as unwired follow-on work when they were wired in e35ac2a2c. Also records the scope caveat that the CLI write path rebuilds the frontmatter block independently, so the marker binds at the transform layer. Refs #3881 * docs(#3881): record the semantic-migration decision and the counted guard ledger The maintainer chose the full semantic migration over splitting the rule into its own epic or patching the scanner, so section 8.1 is answered as "the fork was ill-posed and the migration is semantic" rather than as (a) or (b). Also replaces the pre-implementation guess that this phase would shrink the guard surface with the counted result: excluding vendored third-party lines the hand-maintained surface is net +307, and frontmatter.cts grew by 68 lines despite four functions being deleted, because the compatibility layer over js-yaml is larger than the scanner it replaced. Section 8.1's stated benefit is therefore not delivered as written; what improved is the kind of code maintained, not the amount. Decision 6 requires recording that rather than netting it away. Refs #3881 * chore(#3881): changeset for the vendored YAML parser migration Refs #3881 * test(#3881): golden parity, round-trip property and packaging coverage Refs #3881 * fix(#3881): refuse anchors structurally and fold in review findings ADR-3473 §8.1 review findings, addressed inline: Finding 1 (BLOCKER): refuseAnchorsAndAliases was a raw-line regex that matched only the bare-key spelling (key: &x). A quoted key ("a": &x), a flow mapping ({b: &x}) and a flow sequence ([&x, *x]) all define/use the SAME anchor mechanics while never matching that line shape, so the exact expansion the guard exists to stop went straight through unrefused (a 303-byte quoted-key bomb expanded to ~35.8MB). Replaced with js-yaml's own `load` `listener` callback, which reports `state.anchor` for every event belonging to an anchored node in every spelling, and throws from inside the callback to abort before any expansion (~1-2ms vs full expand-then-discard). A merge key with an alias is still refused (merge always requires a previously anchored node, so the alias itself trips the listener); a bare merge key with NO alias is no longer separately refused, documented as intentional: FAILSAFE_SCHEMA never resolves `!!merge`, so it carries no expansion risk. Table-driven tests added for all four bypass spellings + merge key, plus a quoted-key-spelled billion-laughs fixture registered in the adversarial matrix and README. Finding 2: src/vendor/js-yaml.d.cts's docblock falsely claimed anchors/ aliases were "simply UNREACHABLE from typed code" through the twin. Corrected to state the truth: anchor/alias resolution is document-level `load` mechanics reachable through exactly the declared surface, and refusal is enforced at RUNTIME (Finding 1's listener), not by the type surface. Finding 3 (MAJOR): the null-byte sentinel (U+E000) round-trip was non-injective — restoreNullBytesDeep rewrote every U+E000 in the parsed tree back to NUL, including one the document author legitimately wrote, silently corrupting it. Now refuses outright whenever the raw region already contains U+E000 (consistent with the existing anchor/merge-key refusal path), making the substitution provably injective. Tests added for a real NUL alone (preserved), a pre-existing U+E000 alone (refused, not corrupted), and both together (refused, not merged into one byte). Finding 4 (MAJOR): scripts/lint-vendored-deps.cjs's `srcTwin` field was dead for a hand-authored row (only read inside the upstream-verbatim branch) — exactly how Finding 2's stale docblock drifted unnoticed. Added checkHandAuthoredTwin: every value-level export the twin DECLARES must be an actual own property of the vendored runtime module at require-time. Tests added, including a sensor that a declared-but-nonexistent export IS caught. Finding 5: the existingFm/hasFrontmatter/stripFrontmatter/fmPrefix/ unparseableFm/reassemble preamble, copy-pasted at 7 sites in state-transition.cts plus a sixth hand-inlined copy in state.cts's cmdStateCompletePhase, is now one exported helper (beginFrontmatterReassembly) every site routes through, including the hand-inlined one. Three call sites (beginPhaseCore, patchCore, updateCore) keep a literal `body = stripFrontmatter(content)` assignment alongside the helper call so scripts/lint-state-write-path-drift.cjs's single-hop backward scan (which does not chase aliases) still sees the strip; stripFrontmatter is pure/idempotent so the extra call changes nothing observable. Finding 6: corrected the frontmatter.cts docblock's stale "wiring is a separate change" claim (the 8 call sites are wired on this branch) and the changeset's backlink from (#3473) to (#3881). Finding 7: fixed the lint:ci failures blocking the gate — an @typescript-eslint/only-throw-error violation from throwing a bare Symbol as the anchor-detected signal (now a real Error subclass), unused-var warnings left over from the Finding 5 refactor, a lint-test-file-count cap exceeded by two migration-specific test files (allowlisted with justification), and the lint-state-write-path-drift false positive from Finding 5's helper (fixed above). tests/frontmatter-golden-parity.test.cjs:117's execFileSync already carried an explicit timeout; no change was needed there. Golden fixture: added a golden entry for the new anchor-alias-bomb-quoted.md fixture ({} — matches what the legacy line scanner would also produce, since it independently dropped every quoted top-level key). No other corpus document diverges: real .planning/ documents carry zero anchors/aliases/merge keys/U+E000 today. Refs #3881 * fix(#3881): fold in second-round review findings Finding 1 (BLOCKER): tests/frontmatter.test.cjs pinned the pre-migration ASCII-only key regex for the Unicode fixture; updated to require the 相 key's value now that js-yaml has no such restriction. Audited the rest of the file for other pre-migration pins (block scalars, quoted keys, flattened values, empty values, duplicate keys, unclosed blocks, null bytes) by execution against real fixtures; found none regressed. Finding 2: parseYamlRegion and escapeDoubleQuoted renamed to parseGuardedYamlRegion and escapeDoubleQuotedScalar in src/frontmatter.cts so no function still answers to the deleted hand-rolled scanner's name (ADR-3473 §8.1 "deleted, not patched"). escapeDoubleQuotedScalar's three external call sites (src/commands.cts, src/runtime-artifact-conversion.cts) updated in the same change — a mechanical rename, not an ADR-amendment matter. Finding 3 (BLOCKER): fixed a real crash and a silent data-loss bug found by execution. A top-level key named constructor/__proto__/toString/ valueOf/hasOwnProperty crashed reconstructFrontmatter (bracket read resolving an inherited Object.prototype member); a key literally named __proto__ was silently DROPPED entirely (bracket assignment on an ordinary {} invoked the inherited __proto__ setter instead of creating a data property). Fixed by building every parsed Frontmatter object with Object.create(null), and replacing an `in` check with hasOwnProperty.call in propagateCommentChannel. Added round-trip tests for all five hostile keys, each with its own leading comment. Finding 4 (MAJOR): escapeDoubleQuotedScalar's docstring falsely claimed full byte-stability across the migration. Verified by execution: BEL/NUL/ NEL/NBSP/LS/PS/BOM now emit YAML-named escapes instead of the old hex/raw- literal forms. Proved round-trip equivalence (each escape re-parses to the exact source codepoint) and corrected the docstring. Found and fixed a related real defect while verifying: a lone UTF-16 surrogate was emitted BARE (scalarNeedsDoubleQuoting didn't trigger), producing genuinely unparseable YAML that silently collapsed to {} on re-read — extended scalarNeedsDoubleQuoting to route surrogates through the quoted+escaped path. Finding 5 (MAJOR): countKeysBeforeTruncation went silent on 4 real truncation shapes (unquoted colon, open flow collection, mis-indented sibling key, refused anchor). Root cause: the mark-based prefix recovery excluded the very line whose key needed counting, and a mark-less refusal never entered the recovery branch at all. Fixed by taking the max of two lower bounds: the longest parser-verified line-prefix, and a raw-text count of key-shaped lines (reusing the same key-shape pattern this file already uses for isFrontmatterShaped). Extended test-matrix row A5 table-driven over all 4 regressed shapes. Finding 6: the design doc's claim that no test owned the #3594 adversarial fixture corpus was false — consolidation epic #1969 had already folded it into tests/frontmatter.test.cjs. An earlier commit on this branch re-created a standalone duplicate under that false premise; folded its genuinely-new coverage (fixture-ownership check, anchor-bomb fixtures, block-scalar B1/B2 rows) into frontmatter.test.cjs and deleted the duplicate file. Corrected the false claims in 40-design.md §3.3.1 and the ADR's §8.1 note, including the roadmap-sibling claim (no such file exists). Finding 7: the golden serializer sorted object keys, making it structurally blind to the key-order-parity invariant ADR-3473 §8.1 actually claims. Made it order-preserving and regenerated the golden fixture from a standalone compile of the legacy (pre-#3881) parser at ddde001af; the current parser matches it with zero undocumented divergences, confirming key-order parity genuinely holds. Extended row A2 table-driven across 6 of the remaining 7 transitionCore kinds (all pass) plus documented, by execution, a newly-discovered 8th-site regression: state.cts's cmdStateCompletePhase calls the same preservation helper but its result is clobbered by a later unconditional resync — filed as a distinct finding rather than fixed here (touches syncAndPreserveStateMd, outside this change's verified scope). Refs #3881 * fix(#3881): preserve unparseable frontmatter through the CLI write path Characterization (executed, before/after shown): case (b), not (a). The frontmatter FENCE survives — `state complete-phase` on a conflict-marked STATE.md returns success and a well-formed, freshly-derived frontmatter block, not a document with no frontmatter at all. But the block's actual content (the merge-conflict markers, and with them any signal to a human that the document was in conflict) is silently discarded and replaced. Root cause was two clobber sites, not one: 1. syncStateFrontmatter (src/state.cts) re-parses the already-preserved `transformedContent` from readModifyWriteStateMd, finds {} + the FRONTMATTER_UNPARSEABLE marker, and unconditionally rebuilt a fresh frontmatter block from the body anyway. 2. Even after (1) is fixed, applyPostSyncPreservation's own postFm/applyStatePreservation/authoritativeFm-reassertion machinery re-extracts frontmatter from syncedContent, restores curated fields from the pre-write snapshot, and reconstructs a NEW block again — confirmed live via `state begin-phase`, which still lost the markers after fixing (1) alone. Both are now guarded by the same predicate (isUnparseableFrontmatter, checking FRONTMATTER_UNPARSEABLE): when the ORIGINAL frontmatter did not parse and the caller is not on ADR-3408 §8.3's closed "body wins" list, both functions return their input content unchanged rather than re-deriving over it. The closed list (cmdStateSync #905, /gsd-health --repair's REGENERATE_STATE, both routed only through writeStateMd, which never reaches applyPostSyncPreservation and passes sanctionedPermanentEmptyFallback=true to syncStateFrontmatter) is untouched — neither widened nor narrowed; verified by execution that `state sync` still overwrites the conflict-marked block exactly as before. Other verbs sharing the same readModifyWriteStateMd path were checked and were equally affected before this fix: state update, query state.patch, and state begin-phase all lost the conflict markers (RED, shown by execution), and all three now preserve them (GREEN). Covered table-driven in tests/feat-3881-yaml-parser-consequences.test.cjs's new A2b describe block, which drives the real CLI verbs via runGsdTools — not just the pure transitionCore layer the earlier A2 rows exercised — plus a control asserting state sync's body-wins contract is unchanged. Refs #3881 * fix(#3881): restore the parse surface's prototype and fix remote-runner failures Root cause of the bulk of the 88 remote-runner failures: extractFrontmatter/parseGuardedYamlRegion handed back Object.create(null) trees for prototype-pollution safety, but assert.deepStrictEqual compares prototypes, so every assertion against a plain object literal failed (57 frontmatter.unit.test.cjs + 5 frontmatter.test.cjs + others). Fixed by keeping the internal construction null-prototype (unchanged) and converting to a plain-prototype tree via Object.defineProperty (never bracket assignment, so __proto__/constructor/toString keys stay safe) at the parseGuardedYamlRegion/unparseableResult return boundary only; the internal FULL_LINE_COMMENTS Symbol channel is copied by reference, not recursed, so its own __proto__-safety is untouched. Per-class fixes: (1) bomAcrossArtifactTypes was the same prototype bug, no separate code change needed. (2) frontmatter-cli #1660: added objectListFieldWouldLoseData, a broader lossy-field detector alongside the existing byte-identical noOpObjectListSetError -- js-yaml's flattenObjectListItem now correctly includes every sub-key of an object-list item (a real bug fix over the legacy scanner, which silently dropped every field but the first), so a set that drops that now-included data is no longer byte-identical to the original and needs its own guard. (3) uat.test.cjs: updated the pinned expectation for the human_verification quote-stripping artifact -- js-yaml resolves quoting correctly where the legacy regex left an unbalanced quote; documented as an intentional, non-lossy behavior change. (4) smart-entry: added a fallback-only loadWithAmbiguousColonRepair so a column-0 key: value line whose value itself contains an unquoted colon (the #2571 hand-edited-STATE.md shape) round-trips instead of failing the whole frontmatter block closed. (5) frontmatter.unit.test.cjs bracket-array leniency: added a second fallback, repairMalformedInlineArrays, restoring the legacy scanner's tolerant inline-array handling (consecutive/blank commas, unclosed bracket) -- both repairs run ONLY after the primary parse already threw, so well-formed documents are unaffected. (6) prompt-injection-scan: src/frontmatter.cts had a literal U+FEFF BOM embedded in a comment illustrating the #2977 fix; replaced with the U+FEFF text escape. (7) eslint-glob-coverage: allowlisted the new src/vendor/js-yaml.d.cts vendored type declaration, same precedent as the existing re2js.d.cts entry. (8) frontmatter-golden-parity: git ls-files *.md now runs with -c safe.directory=* (process-scoped) so it survives the remote runner's dubious-ownership check without a persistent git config write. Refs #3881 * chore(#3881): backfill changeset PR number Refs #3881 * test(#3881): make golden parity resistant to unrelated tree churn A corpus-wide snapshot keyed to every tracked *.md file was coupled to mutable-by-design files: .changeset/*.md's pr:0 -> real-PR-number backfill is a required workflow step, not a parser change, yet it turned this suite red. Training people to 'just regenerate the golden' on that kind of failure defeats the point of the snapshot. Exclude .changeset/** from the golden corpus entirely, tolerate tracked *.md files with no golden entry (they postdate the capture) instead of failing on them, keep hard failures for a golden entry whose file has vanished from the tree and for any real parity divergence, and add a coverage floor so the enumeration cannot quietly degrade to comparing a handful of files. Golden regenerated by recompiling the legacy pre-migration parser (git show ddde001af:src/frontmatter.cts) standalone, independent of the current parser, over the same non-changeset corpus. Refs #3881 * test(#3881): make the parser golden hermetic instead of tree-keyed This repo merges ~21 commits/day; a 14-day sample measured 937 touches of the exact files (commands/gsd/*.md, gsd-core/workflows/*.md, agents/*.md, docs/*.md) the prior golden pinned by tracked path. Any PR editing one of those files' frontmatter for reasons unrelated to the parser (an argument-hint addition, an allowed-tools tweak) turned the suite red, and the reflex fix -- "regenerate the golden" -- overwrote the very snapshot meant to catch a real regression. Excluding .changeset/** was not enough; the design itself was wrong: a regression fixture must not be keyed to mutable repo paths, and a single 376-entry JSON every such PR touches is also a guaranteed merge-conflict surface. Rebuilt the fixture to carry its own documents: each of 51 entries stores a stable id, literal documentText (shrunk from a real ddde001af-era corpus document), and an expectedParse captured independently from the pre-migration legacy parser (git show ddde001af:src/frontmatter.cts, compiled standalone against its byte-identical sibling modules). The test reads no tracked path, shells out to no git command, and enumerates no tree -- a PR editing commands/gsd/help.md cannot affect it. Every entry's reconstruction was verified at capture time to reproduce both the current and legacy parser's output on the original document; 0 of 51 candidates were dropped by that check (1, the deliberately-unterminated unclosed-block.md adversarial fixture, has no closing fence to truncate at and is stored unshrunk). Kept the 5 documented DIVERGENCES rows (now diverges:true entries) and the D2 order-preserving structural serializer that keeps the comparison from passing vacuously; dropped the tree-enumeration helpers, the coverage floor, the post-capture-skip logic, and the vanished-file check -- all artifacts of the path-keyed design. Refs #3881 * fix(#3881): resolve vendored-deps paths independently of cwd shape Five rows in tests/lint-vendored-deps-manifest.test.cjs failed on windows-latest CI: the test passed absolute scratch-file paths into compareFiles()/checkRow(), whose helpers joined every input onto ROOT via path.join(ROOT, rel), producing garbage when the input was already absolute. It surfaced on windows-latest specifically because GitHub's Windows runners checkout the repo on a different drive than TEMP, so path.relative(REPO_ROOT, tmpFile) returned the absolute path unchanged (no relative traversal is representable across drives) rather than the relative form the test assumed. The remote gsd-test runner this repo gates pushes on is Linux-only and could never have caught this; GitHub CI's windows-latest job is the only signal that does, and it did. Fixed the helper itself (scripts/lint-vendored-deps.cjs's new resolvePath()) to treat an already-absolute input as absolute-in, absolute-out instead of silently mis-joining it, and updated the test to pass the scratch file's absolute path directly rather than relying on a relative conversion that is not always representable. Kept every mutation-sensor assertion intact and added coverage proving resolvePath is a no-op for relative inputs and correctly passes absolute ones through unchanged. Refs #3881 * fix(#3881): warn when state sync regenerates over unparseable frontmatter state sync (ADR-3408 §8.3's sanctioned regenerate path) correctly overwrites an unparseable frontmatter block per its 'body wins' contract — that overwrite behavior is unchanged here. The defect was the silence: synced:true/exit 0 gave no signal that the existing block (including git merge-conflict markers) could not be parsed and was destroyed, per ADR-3473 §8.5 ('a derived conclusion may not be reported as authoritative when the derivation dropped input it could not resolve') and §8.4 ('failure is a value'). Adds a gsd: warning — ... (#3881) line on stderr, matching the existing #3573 precedent, and surfaces the same disclosure in the JSON result's existing changes[] array so a machine consumer sees it too. Exit code and synced:true are left unchanged — sync did what its contract says. REGENERATE_STATE (/gsd-health --repair's sibling on the same sanctioned-regenerate list) is DESTRUCTIVE-risk and unconditionally refused by applyRepairs's dispatcher before runRepairAction ever runs (src/health-diagnostic.cts), so it is not a live path today and is not in scope for this fix. Refs #3881 * fix(#3881): exit non-zero when a state command returns an error Refs #3881 * chore(#3881): changeset for the state exit-code fix Refs #3881 * fix(#3881): honor the documented --project-dir flag Refs #3881 * revert(#3881): restore exit-0 result envelopes for state errors Reverts 9638f2936 and its changeset. The change was wrong and the revert is the correction. This repo distinguishes two error mechanisms deliberately. error() in src/io.cts writes to stderr and calls process.exit(1) -- the hard-failure path. output({error: ...}) writes a JSON result envelope to stdout and returns normally with exit 0. The reverted commit converted 23 result-envelope sites into hard failures, which is a different contract, not a bug fix. tests/state-contract.test.cjs's errorPathDoesNotPublish asserts the envelope contract directly -- a failing command exits 0 with a JSON error envelope and must not publish state.json -- and the remote matrix run caught it along with four cases in the QA scenario walk. Thirteen tests in tests/state.test.cjs that the original commit rewrote were encoding that real contract, not the bug it claimed; they are restored. Whether an error envelope on stdout with exit 0 is the right CLI design is a genuine question, and it is section 8.4's rule ('failure is a value') with its own phase. It is not something to flip inside this PR. Refs #3881 * chore(#3881): backfill changeset PR number for the project-dir fix Refs #3881 * test(#3881): keep the frontmatter mutation shard inside its time budget The Stryker (frontmatter) shard hit the documented 15-minute (900s) shard cap. Root cause is NOT row-level spawn overhead (contrast the #2790/ core-utils precedent): the three shard test files' own logic runs in ~413ms total (356+30+27ms) with all 392 assertions passing. Instead, src/frontmatter.cts grew from ~825 to 1496 lines (+671/-187) migrating to the vendored YAML parser, proportionally growing the mutant count Stryker generates for gsd-core/bin/lib/frontmatter.cjs. Stryker's command runner bills the full 'node --test <3 files>' invocation once per mutant, and node:test's default per-file process isolation forks a child process for each of the three files on every one of those invocations — pure fork overhead multiplied by a much larger mutant population. Fix: scripts/mutation-matrix.cjs COVERED.frontmatter now declares isolation: 'none', and .github/workflows/mutation.yml passes --test-isolation=${{ matrix.isolation }} (defaulting to 'process' — i.e. unchanged behavior — for the other 8 shards, which were not individually audited for cross-file state leakage under shared-process execution). Measured locally via node:test's run() API on the exact 3-file set: isolation:'process' took ~593ms vs isolation:'none' ~478ms for the same 392 passing assertions. The true CI-shard number can only be confirmed on the GitHub Actions run (Stryker cannot run locally, and 'node --test' is hard-blocked in this environment). Refs #3881 * test(#3881): register the vendored-parser tests in the frontmatter mutation shard stryker.config.mjs's own rule ("Keep this list in sync with the tests arrays in scripts/mutation-matrix.cjs COVERED") was violated: #3881 grew src/frontmatter.cts from ~825 to 1496 lines but its new tests (tests/feat-3881-yaml-parser-consequences.test.cjs, tests/frontmatter-golden-parity.test.cjs, tests/frontmatter-roundtrip.property.test.cjs, and +167 lines in tests/frontmatter.test.cjs) were never added to the frontmatter shard's tests array, so Stryker's mutants in the new vendored-js-yaml adapter had nothing constraining them. PR #3888 measured 55.8% against the 65 floor (748 killed / 593 survived / 17 timeout) and the shard was separately cancelled at 15m04s against the 15-minute per-shard cap. Registers all four files (each earns its slot on evidence of a unique constraining assertion, documented inline), gives the shard a measured/projected 180-minute budget via a new per-module timeoutMinutes field threaded through mutation.yml's job-level timeout-minutes the same way isolation is threaded, and removes the prior isolation:'none' override (re-measured at this file-set size, its savings are within run-to-run noise, not worth the unaudited cross-file-state-leakage risk). Refs #3881 * feat(#3881): derive the mutation test list and ratchet the score floor Refs #3881 * test(#3881): ratchet five stale mutation floors and close the frontmatter gap Raised five module minScore floors per CI run 33012034388 (floor(achieved)-1): config-schema 75.51%->74, prompt-budget 88.95%->87, context-composer 79.92%->78, context-utilization 92.31%->91, active-workstream-store 87.42%->86. Updated both scripts/mutation-matrix.cjs COVERED entries and tests/mutation-matrix-ratchet.test.cjs RATCHET_BASELINE in the same diff per the ratchet's own contract. Closed the frontmatter shard's 63.03%-vs-65 gap with new behavioral tests in tests/feat-3881-yaml-parser-consequences.test.cjs, each paired with a documented near-miss: frontmatterDeepEqual's array-order/length/type-mismatch/key-order semantics (via spliceFrontmatter's no-op guard), scalarNeedsDoubleQuoting's leading/trailing-whitespace and dash/surrogate triggers (via reconstructFrontmatter), repairAmbiguousColonValues' already-quoted vs ambiguous-colon repair paths (via extractFrontmatter), and the null-byte sentinel round-trip surviving at region offset 1. Did not lower minScore. Refs #3881 * test(#3881): decouple the ratchet test from real module floors The CLI end-to-end rows in tests/mutation-score-ratchet.test.cjs hardcoded config-schema's real floor (52), which commit 973321541 legitimately ratcheted to 74 -- breaking a test pinned to the exact value the mechanism under test exists to change. Add an injectable --matrix seam to scripts/check-mutation-score-ratchet.cjs and point the CLI rows at a synthetic module + synthetic floor built via a temp fixture, so the rows are indifferent to any real module's floor moving while still exercising the same fail/pass behaviour. Refs #3881 * refactor(#3881): parse must_haves with the vendored parser and drop re-implemented leniency Refs #3881 * fix(#3881): restore the ambiguous-colon repair its hand-edited-STATE.md contract needs A tracked-document sweep of 910 *.md files cannot see this dependent: repairAmbiguousColonValues's one real caller is user hand-edited STATE.md content that never lives in this repo's tree, only on end users' machines, and is pinned by tests/smart-entry.unit.test.cjs. Restores the function plus its post-throw fallback path (loadWithAmbiguousColonRepair) only; repairMalformedInlineArrays and splitLegacyInlineArrayItems stay deleted, reverified against the full frontmatter test shard. Adds a frontmatter-level regression row in tests/feat-3881-yaml-parser-consequences.test.cjs so the dependency is visible where the function lives. Closes #2571 Refs #3881 --------- Co-authored-by: sim <sim@local>	2026-08-26 19:29:32 -04:00
Tom Boucher	a3d5841117	fix(#3714): deliver an explicitly pinned model to the Codex worktree executor, and drop an unusable one (#3891) ... * test(#3714): failing-first coverage for the dropped Codex worktree model override Pins the argv contract for the orchestrator-worktree process dispatch: an explicit model_overrides pin must reach the child as --model, while an unpinned, empty, inherit, or profile-only configuration must emit no flag at all. Five of the eight matrix rows are CONTROLS that pass before the fix. They carry the weight here because this is an over-emission bug waiting to happen: resolve-model returns 'sonnet' for the unpinned, empty and profile-only cases, so a fix that threads its return value into argv would satisfy the positive row and emit --model sonnet to Codex on every unpinned install -- the documented 400 that ADR-2313 exists to prevent. The controls are what separate the correct fix from the obvious one. * fix(#3714): deliver an explicitly pinned model to the Codex worktree executor resolveOrchestratorExec had no model input at all -- the descriptor carried only command/args/cwdFlag/promptFlag -- so a resolved override had no way to reach the spawned process even in principle. The baked gsd-executor.toml could not compensate because this path spawns a process rather than dispatching a named agent. Three parts, and the third is the load-bearing one. The descriptor gains modelFlag (codex: --model), keeping the per-host knowledge as descriptor data exactly as cwdFlag and promptFlag already are, so the scheduler grows no per-host branch. No other runtime declares it. The seam appends [modelFlag, model] and stays MECHANICAL: it does not know the inherit sentinel, does not know which models Codex rejects, and reads no config. Its sibling codex-agent-toml states that rule outright -- callers decide what to strip. Argv order is baseArgs, model, cwd, prompt so the prompt remains the final positional token. Omitting the model is byte-identical to before. A model starting with '-' now fails closed as unsafe_leading_dash_model, the same hazard the prompt and cwd guards already reject and which was silently accepted before. The policy lives at the caller and passes ONLY an explicit, non-sentinel per-agent pin. Passing null as the runtime resolver is what keeps profile and tier derived models out of argv, which is what Codex's session-only model posture requires: the model resolver returns 'sonnet' for the unpinned, empty and profile-only cases, and emitting that revives the documented 400 from #2310/#2311 that ADR-2313 removed. So the gate is the presence of an explicit pin, never that a value came back. * fix(#3714): enforce the real-Codex value policy the sibling surface already applies Review found one root cause behind a BLOCKER, two MAJORs, two MINORs and an argv-injection finding: the dispatch path gated on the PRESENCE of an explicit pin but never applied the VALUE policy that generateCodexAgentToml already applies to the same config key. Textbook generative divergence -- and the parity row I wrote tested resolver parity, not this policy, so it could never have caught it. BLOCKER: a global ~/.gsd/defaults.json model_overrides.gsd-executor of 'sonnet', 'opus' or 'claude-sonnet-4-5' reached codex exec --model verbatim. That is the documented 400 from #2310/#2311, arriving through the explicit-pin door rather than the tier door. The issue asks for an explicit REAL-CODEX pin; real-Codex was unenforced. Now dropped with a warning via the isAnthropicFlavoredModel predicate #3241 single-sourced for exactly this reason. Also: values are trimmed, so a whitespace-only pin is blank rather than --model " "; 'inherit' is matched case- and whitespace-insensitively, so 'Inherit' and ' inherit ' no longer reach the wire; and a value outside a model-id charset is dropped with a warning. That last one closes the injection surface -- .planning/config.json travels with a clone, and values like 'gpt-5 -c approval_policy=never' or a command-substitution value previously reached argv verbatim, where the spawner is an agent writing bash. Every rejection DROPS AND WARNS rather than failing closed. An unusable exec is not degraded, it is fatal: the dispatch step halts the wave after the worktree already exists, so a config typo would have aborted execute-phase. The stale comment claiming it degrades to sequential is corrected. Separately, the seam's own empty-model handling contradicted the committed contract and failed four tests on the remote runner. An absent, null or empty model is not an error -- it means use the host default, the same degradation cwdFlag:null already expresses. Unlike a prompt, where empty is a hang rather than a degraded run, so that one stays fail-closed. Non-string values still fail closed. Tests: the CLI rows were not HOME-hermetic and read the developer's real ~/.gsd/defaults.json, which is precisely the file the BLOCKER is about; HOME and USERPROFILE are now sandboxed per call. Adds the global-pin regression, the injection shapes, the case-variant inherit rows, and a real cross-surface divergence guard. * fix(#3714): close the flag-shaped pin, the case-variant alias, and the warning sink Round two of review found three more defects, two of which both engines reached independently, and all three were mine. The charset allowlist put the dash INSIDE the character class, so a value made only of allowed characters passed the pin policy silently and then tripped the seam's leading-dash guard, producing exec:null. That is the wave-fatal path the whole drop-and-warn design exists to avoid, reachable from a committed config file: -c, --config, -p and --dangerously-skip-permissions all reproduced it. It also regressed hosts with no model flag at all, where a dash pin turned a previously working kimi-code dispatch into exec:null. The first character is now anchored, so a flag-shaped value is dropped and warned like every other rejection, and the comment that claimed this path was unreachable is corrected. isAnthropicFlavoredModel folded case on its substring arm but not on its alias-set arm, so SONNET, Sonnet, OPUS and HAIKU all reached Codex argv while lowercase sonnet was correctly dropped -- the same 400 the drop exists to prevent. The predicate is the one #3241 single-sourced so these surfaces cannot diverge, so folding case there fixes the install-side .toml surface too. The warning wrote the rejected value RAW to stderr. Every value that fails the charset test contains by definition the characters the charset excludes, so it was a guaranteed-reachable raw-to-terminal sink: an OSC sequence in a committed config reached the operator's terminal byte for byte, and truncation could sever an escape before its reset. The value is now sanitized before truncation. Also from review: the allowlist rejected Vertex version pins like text-bison@002, a false positive on a real model id; the policy ran host-neutrally so hosts with no model flag printed a misleading drop warning on every dispatch; the invalid_model branch had no test at all; and the changeset disclosed only that a pin is delivered, not that an unusable one is now dropped with a warning. * fix(#3714): single-source the model-id charset, bound the pin, keep the flag diagnosis Round three found no blocking findings on either engine. These are the three correctness items left in code I added. The charset existed TWICE -- once to accept a pin, once to render a rejected one in the warning -- and the two copies had already drifted inside a single commit: '@' was added to the accept class and not the render class, so a Vertex-shaped value rejected for some other reason rendered as text-bison?002. Both are now derived from one definition, with a parity test asserting every character the matcher accepts survives the sanitizer unchanged, so they cannot drift again. A pin reached argv unbounded. CLAUDE.md documents the hazard: execFileSync aborts on Windows above 32,767 characters of argv. A model id has no reason to be long, so a pin over 200 characters is dropped and warned rather than truncated -- a truncated model id is a different model id. Boundary rows at 199, 200 and 201. The first character is now required to be alphanumeric, so '@evil' and '/c' no longer reach argv. Security rates both inert on codex today, so this is hardening rather than a live defect; it is here because it is one character of regex and resolveOrchestratorExec documents itself as a general descriptor-to-argv seam that other hosts may adopt. Tightening the anchor made the leading-dash branch unreachable and, with it, regressed the diagnosis: '-c' began reporting 'unsafe characters' instead of 'looks like a flag/option'. The dash check now runs before the charset test, which both restores the actionable message for the most likely user typo and keeps the branch live. A test pins the distinction between the three rejection messages so the branch cannot silently die again. * test(#3714): make the charset parity guard actually guard, and remove a false-green trap Review proved by mutation that my parity test could not do what its own comment claimed. It bound the expected character set to a local that was assigned and discarded, and the shared definition was not exported, so widening that definition left the test green. A comment overstating what a test guards is worse than no comment, because the next reader trusts it. The shared body is now exported and asserted equal, and I watched the assertion fail against a widened definition before keeping it. The sanitizer regex was module-scope, carried the g flag and was exported. Production only uses it with replace, which resets lastIndex, so there was no live bug -- but test() on a g-flagged regex alternates between calls, so any future test reaching for it would false-green. The g-flagged copy is now internal to the one call site that needs it and the exported companion carries no flags, which removes the footgun rather than documenting it. Also notes at the shared definition that it is interpolated into both a positive and a negated character class, so only plain characters and ranges are safe to add. No runtime behavior changes: the accept set, the anchor, the length cap, the sanitizer output, the rejection ordering and all three message texts are byte-identical, re-verified through the real CLI. * chore(#3714): backfill changeset pr number * chore(#3714): re-trigger CI after the GitHub Actions outage The workflow runs for this branch were created during the Actions major outage on 2026-08-26 and never got scheduled. They are wedged: GitHub reports them queued, refuses to cancel them, and refuses to rerun them because it believes the workflow is already running. The Tests run is also pinned to a superseded sha, so no test run exists for the current head at all. Actions is operational again and the repo-wide queue has drained, so a fresh push is what creates schedulable runs. This commit is empty on purpose: nothing about the change is being altered, and the verified content is byte-identical to 7860c6ccc. --------- Co-authored-by: sim <sim@local>	2026-08-26 17:01:09 -04:00
Tom Boucher	aaf47c5fc2	fix(#3691): let every reviewer lane take a prompt cap, and make the documented global resolve (#3832) ... * test(#3691): failing-first coverage for the reviewer prompt budget No prompt cap can reach any CLI reviewer lane, by any configuration. Two independent defects compound: all nine `transport: spawn` lanes declare `promptBudgetKey: null`, so `budgetFor` returns on its first line; and the documented global `review.max_prompt_tokens` is advertised in the schema manifest but declared nowhere, so the resolver never materializes it and `budgetFor`'s fallback is dead code. Adds to tests/reviewer-config-federation.test.cjs, which already owns the per-reviewer budget config-set/config-get idiom: - a CLI lane inherits the global cap (RED: reports null) - an http lane with the -1 sentinel inherits the global cap (RED: reports null) - the resolved review surface carries max_prompt_tokens at all (RED: absent) - per-lane overrides the global on a CLI lane - the sentinel boundary: -1 inherits, 0 means do-not-trim and must NOT read as unset, 1 is the smallest real budget — the regression budgetFor's own comment warns about - anti-tightening pins that must stay green: an empty config leaves every lane null, the three existing budgeted lanes are unchanged, and config-set still rejects a per-reviewer key naming something that is not a declared lane - a fast-check property over the resolution contract itself, with -1, 0 and non-finite inputs generated explicitly rather than left to chance Every row was reproduced by hand against the real CLI before being written, so the RED/GREEN split is observed rather than predicted. Refs #3691 * fix(#3691): let every reviewer lane take a prompt cap, and make the global resolve No prompt cap could reach any CLI reviewer lane, by any configuration. Two independent defects compounded. The nine spawn-transport lanes — claude, coderabbit, antigravity, cursor, gemini, codex, kimi-code, opencode, qwen — declared `promptBudgetKey: null`, so `budgetFor` returned on its first line and `review-lane plan` reported `promptBudget: null` no matter what was configured. Each now declares `review.max_prompt_tokens_per_reviewer.<slug>` with the same `-1`-is-unset sentinel the three local-server lanes already use. Separately, the central `review.max_prompt_tokens` was listed in the schema manifest's validKeys and documented as a supported setting, but declared nowhere — the resolved surface is built from capability declarations plus the defaults manifest, and neither carried it. `configGet` returned undefined and `budgetFor`'s documented fallback was dead code. It is now declared with a `null` default, exactly as docs/CONFIGURATION.md already specified, so the default behavior is unchanged: nothing configured means nothing trims. Two things the diagnosis had not predicted, found and fixed while implementing: - `REVIEWER_LANES` in src/review-lane-descriptor.cts is a second, hardcoded registration site that `mergeReviewerLanes` prefers over the capability registry on a slug collision. Editing only the capability files left every CLI lane still null. Both sites now agree. - The generated `gsd-core/bin/lib/capability-registry.cjs` was stale and masked the capability edits; regenerated with `npm run gen:capability-registry` rather than hand-edited. docs/CONFIGURATION.md said "Only lanes that declare a budget key accept one — today ollama, lm_studio and llama_cpp". That is false as of this change and is corrected rather than left to rot. The trim-versus-refuse question the issue raises is deliberately not taken up here: the refusal path already exists for the case that matters — a reviewer whose minimum set exceeds its budget is skipped rather than sent a misleading prompt — and trimming above that floor is the documented, shipped design of the feature. Changing it would alter behavior for the three lanes that already work, which is not what the issue asks for. Fixes #3691 * fix(#3691): document the new global and narrow an invariant this change obsoleted The full suite surfaced two consequences of giving every CLI lane a budget key. `review.max_prompt_tokens` entered CONFIG_DEFAULTS without a matching entry in the planning-config reference, which config-field-docs guards. Documented, including the sentinel semantics a reader needs: a per-lane value overrides the global, `-1` means unset and inherits it, and `0` means "do not trim that lane" and is not unset. The #2797 federation guard asserted that "a lane with no model flag and no host owns no config keys". That held only because budget keys existed solely on the three local-server lanes, all of which have hosts. A lane can now legitimately own a config key for a third reason, so qwen tripped it. The assertion is narrowed rather than weakened: such a lane must still own no model key and no host key, and may own at most its own `review.max_prompt_tokens_per_reviewer.<slug>` — never another lane's. That is strictly more specific in the dimensions that still matter. Proven to still bite: hypothetically giving qwen a `review.models.qwen` key fails it with `model/host: review.models.qwen`. The name and comment cite #3691 for why the premise changed, so a reader sees a deliberate narrowing, not erosion. Checked the sibling assertions in that describe block; the other three do not rest on the obsolete premise and are untouched. Refs #3691 * fix(#3685): port the write-flag content-change contract to its three sibling sites #3685 fixed `phase complete`'s `roadmap_updated` / `state_updated`, which reported `fs.existsSync(path)` rather than whether the transaction wrote anything. Three sibling sites carried the identical defect and are ported here. - `cmdPhaseRemove` reported `roadmap_updated: true`, hardcoded. `updateRoadmapAfterPhaseRemoval` now returns whether the content changed and the flag reports it. #2640/#2974 already fixed `state_updated` at this same call site and left this one behind, so the correct shape was adjacent. - `cmdMilestoneComplete` reported `state_updated: fs.existsSync(statePath)` — byte-identical to #3685's bug in a different command. - `cmdMilestoneComplete` reported `milestones_updated: true`, hardcoded, never consulting the MILESTONES.md write. `gsd-core/workflows/remove-phase.md:100` extracts `roadmap_updated` for display and never branches on it, so the flip from always-true to content-based changes no workflow behavior. Verified by reading the step, not assumed. One trap found while implementing: the obvious in-memory `finalContent !== originalStateContent` comparison — copying `cmdPhaseComplete`'s shipped shape verbatim — gives a FALSE POSITIVE for milestone completion. `platformWriteSync` normalizes Markdown at write time, and the milestone-closure transform regenerates `## Current Position` fresh on every call, so its pre-normalize output always differs from the already-normalized file on disk even when the persisted bytes are identical. The comparison is therefore made against the post-write on-disk content. `cmdPhaseComplete`'s own comparisons are left untouched — their repeat-no-op tests pass, so they are not exposed to this artifact. `milestones_updated` has no reachable no-op: the MILESTONES.md write unconditionally appends an entry every call. Only the true direction is pinned, documented inline rather than faked with a passing test. Refs #3685 * fix(#3685): compare write-flag content through the writer's own normalizer An independent reviewer disproved a claim made while porting #3685's contract to its sibling sites: that `cmdPhaseComplete`'s comparisons were not exposed to the Markdown-normalization artifact already diagnosed in `cmdMilestoneComplete`. `platformWriteSync` normalizes on write — CRLF stripped, blank-line runs collapsed, a blank line inserted after a heading, a single trailing newline enforced. Every flag that compares the PRE-normalization in-memory string against the on-disk pre-image can therefore report a change when the persisted bytes are identical. `cmdMilestoneComplete` had been worked around by re-reading the file after the write; the other sites compared raw strings. All of them now go through one exported seam, `contentChangedAfterNormalize(filePath, before, after)`, which normalizes both sides exactly as the writer does. That removes the extra disk read the milestone workaround needed, and makes the sites agree by construction rather than by four independent implementations of one rule — the divergence the repo names as an anti-pattern. Reachability, stated precisely rather than uniformly: the seam is load-bearing at `cmdPhaseComplete`'s `roadmapUpdated`, `requirementsUpdated` and `stateUpdated`, where section-rewrite logic genuinely regenerates content into a different-but-normalization-equivalent shape. At `updateRoadmapAfterPhaseRemoval` it is defense-in-depth: the no-match branch never reassigns `content`, so the raw comparison was already correct there. The first analysis claimed the reverse; this is the corrected finding. Also fixes an unsound test premise the remote suite caught. The byte-identity precondition in `roadmap_updated is false when ROADMAP.md comes out byte-identical` asserted against a hand-authored, un-normalized fixture — so the very first write reformatted it and the file could not come back identical. The fixture is now written already-normalized, so the assertion compares a normalized pre-image against a normalized post-image and still fails if the flag regresses to a hardcoded `true`. Not platform-specific; it reproduces on macOS too, and the earlier local check simply never exercised it. The sibling true-direction and milestone tests were checked for the same premise and do not share it — they assert `notEqual`, or compare two post-write states produced through the same normalizing seam. Refs #3685 * chore(changeset): backfill PR number for #3691 fragment --------- Co-authored-by: sim <sim@local>	2026-08-24 19:39:51 -04:00
Tom Boucher	2f86278b5e	fix(#3003): opt-in mechanism for intentional deletions in worktree.cleanup-wave (#3757) ... * test(#3003): failing-first suite for declared deletions in cleanup-wave Binds the guard's opt-in before it exists, so the suite is RED against next. The rows that carry the weight are the over-authorization set: a directory declaration must not authorize its children, a glob declaration must authorize nothing, and a declaration must not act as a string prefix of another path. Each of those BLOCKS, and each would PASS under a prefix, glob, or startsWith matcher — which is how a path list quietly degrades into the boolean opt-in #3003 explicitly rejected. The glob row matters most: declaredScopePrefix already returns null ("matches everything") for a glob-leading pattern, correct for the advisory it serves and catastrophic for a gate. Also pinned: a failed deletion check blocks on its own reason rather than being filtered into a pass; the block detail names only the undeclared residue so the operator is not misdirected by paths that were fine; an entry with no declaration blocks exactly as before; junk and non-array declarations do not authorize; and a blocked entry still isolates rather than aborting the wave (#2852, which must stay fixed). Two advisory rows cover an interaction found while designing: git diff --name-only includes deleted paths, so without unioning the declaration into the #2596 scope check, authorizing a deletion would raise SCOPE_OUT_OF_DECLARED against the very path just authorized. A seeded property states the whole invariant the three over-authorization rows sample: a deletion merges iff its normalized path is in the declared set. * feat(#3003): declared deletions opt-in for the cleanup-wave guard The deletions guard blocked the merge-back of any executor branch whose diff removed a file, with no way to say a removal was intended. A plan that folded one test file into a sibling could not be merged by the tool meant to merge it, forcing a manual --no-ff outside the tool -- strictly less safe than what the guard protects against. A plan now declares removals in its own frontmatter (files_deleted), and that list rides the same path files_modified already travels: plan-document parse -> phase plan JSON -> the per-plan worktree gate -> record-agent/create --deletions -> declared_deletions on the manifest entry -> the guard. The guard blocks only the deletions NOT in that list. A path list rather than a boolean, per the pinned decision: a boolean disarms the guard for the whole entry, so an unexpected deletion riding along with a declared one would pass unnoticed. Matching is exact after the module's shared normalizer -- never a prefix, never a glob. Both would let one declaration authorize a whole set, which is the mass-deletion accident the guard exists to catch. That also means declaredScopePrefix is deliberately NOT reused here: it returns null ("matches everything") for a glob-leading pattern, which is right for the advisory it serves and would silently disarm a gate. The block detail now carries only the undeclared residue, so an operator is not sent looking at paths that were fine. A failed deletion check still blocks on its own reason and is never filtered into a pass. A blocked entry still isolates rather than aborting the wave (#2852). The #2596 scope advisory unions the declaration into its declared set -- git diff --name-only includes deleted paths, so without that, authorizing a deletion would immediately warn that the same path was out of declared scope. Optional and additive throughout: files_deleted is absent from PLAN_REQUIRED_FIELDS, a manifest entry without declared_deletions keeps the original unconditional block, and omitting --deletions leaves the on-disk entry shape untouched. Supersedes the spent #2856 emitted-drift ack entry for execute-phase.md, the same supersede that entry performed on #3370 and #3370 on #3324. * fix(#3003): wire --deletions on every dispatch surface, not just one Review found the feature inert on two of three dispatch paths. execute-phase.md (harness inline) passed --deletions, but the orchestrator-worktree path (executor-isolation-dispatch.md, worktree.create) and the Fleet-parallel batch path (capabilities/claude-orchestration/fragments/execute-wave-pre.md, worktree.record-agent) still passed only --files. A plan declaring files_deleted would have merged on one path and been blocked on the other two -- the exact bug #3003 exists to fix, left unfixed where most of the isolation actually runs. Worse, per-plan-worktree-gate.md already claimed --deletions was passed 'on the same worktree.record-agent / worktree.create calls', which was false for both untouched sites. A doc asserting coverage that does not exist is how a gap survives review. All four surfaces now pass the flag, verified by sweeping every .md under gsd-core/, capabilities/, commands/, skills/ and agents/ that invokes worktree.record-agent or worktree.create: each one that passes --files now also passes --deletions. The isolation-dispatch note explains why this flag, unlike --files, is not advisory -- omitting it does not skip a check, it blocks a merge the plan declared. Regenerates capability-registry.cjs, which the fragment edit made stale. Neither newly-grown file needs an emitted-drift ack: executor-isolation-dispatch.md sits under workflows/execute-phase/steps/ and execute-wave-pre.md under capabilities/, both outside currentSizes()'s non-recursive scan of gsd-core/workflows/ and agents/. * docs(#3003): document files_deleted where a plan author will actually find it The feature's entire user surface is one plan-frontmatter field, and the canonical reference for that frontmatter -- docs/reference/plan-md.md, the table that documents every other key -- never mentioned it. A field nobody can discover ships as a field nobody uses. Adds the files_deleted row and an example entry in all five locales (en, ja-JP, zh-CN, ko-KR, pt-BR), stating the property that makes the opt-in safe: matching is exact per path after separator normalization, with no globs and no directory prefixes, so a declaration can never authorize more than it literally lists, and omitting the field keeps the guard's original unconditional block. Also corrects two claims in the scope-conformance how-to that this change made false. Its opening paragraph described the recorded declared scope as files_modified alone; declared_deletions is now unioned into that comparison. Its "Renames are not detected specially" bullet asserted the deletions guard blocks any entry whose diff contains a deletion, full stop -- which was the whole point of #3003 and is no longer true. Reworked to say what now decides a rename's fate: declare the old path in files_deleted and both halves become ordinary paths for the advisory check, which is also why the old path needs no separate files_modified entry. Documentation that describes the pre-change behavior of the thing being changed is worse than no documentation, because a reader trusts it. * fix(#3003): close every review finding on the declared-deletions opt-in Two independent isolated reviewers, correctness and security. Neither found a blocker; both found real defects, and the directive treats a finding at any severity as blocking. All of them are fixed here. MAJOR -- the submodule worktree gate could not see a deletion-only plan. per-plan-worktree-gate.md intersected $SUBMODULE_PATHS against $PLAN_FILES alone, while $PLAN_DELETIONS was extracted and then never used. Before files_deleted existed, a path had to appear in files_modified to be planned at all, so the gate saw it; the new field plus the new docs telling authors a deleted path needs no files_modified entry opened a hole where a plan whose only submodule touch is a removal kept worktree isolation on -- the exact case #2772 disabled it for. Both channels now feed the intersection. Note the posture is deliberately the OPPOSITE of the cleanup-wave guard: there the channels stay apart because a deletion AUTHORIZATION must never be inferred; here they merge because a safety fallback must never MISS a touch. MAJOR -- same-wave conflict detection could not see a deletion. The planner's implicit-dependency rule compared files_modified only, so plan A editing src/x.ts and plan B declaring files_deleted: [src/x.ts] scored as conflict-free and ran in parallel: one branch removing what the other is writing, which is the sharpest conflict there is. Overlap is now computed across both channels. MINOR (both reviewers, one root cause) -- the advisory union gave one field two matching rules. declared_deletions was unioned into the scope list handed to planWaveScopeConformance, which reads it with prefix-and-glob semantics. So a field that is exact-match-only at the gate silently became wider at the advisory: ["*.md"], inert at the gate, yielded a null prefix meaning "matches everything" and muted the advisory completely, and ["src"] muted all of src/. The union also activated the advisory on plans that declared no modification scope at all, warning on every modified path. Replaced with subtraction from the findings, gated on files_modified alone. One field, one rule, everywhere. MINOR -- core.quotepath made the feature silently inert for non-ASCII paths. git emits "tests/\303\251.ts" C-escaped and quoted, which never equals the declared plain path, so a correctly declared deletion of tests/é.ts would block forever with nothing pointing at the encoding. Both diffs now pass -c core.quotepath=false. NIT -- flag() consumed a following flag as a value, so --deletions --files x swallowed --files and dropped both. Now treated as a missing declaration, which fails closed. Fixed at both call sites; the helper is duplicated verbatim in cmdWorktreeRecordAgent and cmdWorktreeCreate and leaving one would reintroduce it. TEST -- one test passed for the wrong reason. "a declared deletion is in scope for the advisory" asserted only that warnings omit the deleted path; under a full revert the entry blocks first, warnings come back empty, and the negative assertion passes anyway. It now asserts the entry actually merged, which is the load-bearing half. Four regressions added, one per fix above. Docs corrected rather than extended. The rename bullet in the scope-conformance how-to claimed a rename whose delete side is undeclared never reaches the advisory. Verified false: git's rename detection is on by default, so a pure rename is a single R entry that appears in no --diff-filter=D output and was never gated, before or after #3003. Only a rename that edits enough to fall below the similarity threshold decomposes into add+delete. The pre-existing sentence made the same wrong claim; this restates it correctly instead of sharpening the error. The localized plan-md.md reference edits are reverted: the PR template requires docs content added here to be English, and the translations already lag by three fields, so English-only is the repo's standing posture, not an oversight. Agent-file size caps respected: gsd-planner.md is XL-tier by bytes but carries a separate 49152-LF-CHAR cap asserted by four suites, so its edit is deliberately terse and lands at 49141 with 11 chars of headroom, with the rationale moved to docs/reference/plan-md.md, which has no cap. gsd-plan-checker.md lands at 49107 bytes, 45 under the LARGE cap. Both acks merged into the existing fragments that already name those paths, since two ack sources may never name the same path. * fix(#3003): decode git's path quoting instead of changing the git argv The previous commit's non-ASCII fix turned the remote suite red: 44 failures, 42 of them "unexpected git call: -c core.quotepath=false diff --diff-filter=D --name-only ...". The suite's git mocks match on exact argv, so adding two flags to the deletions diff and the advisory diff invalidated every existing fixture in tests/worktree-safety.test.cjs. Rewriting dozens of fixtures to accommodate one flag would be paying a large Hyrum's-law bill to fix a small defect. Both execGit calls are reverted to their original argv. The C-quoting is now decoded in normalizeScopePath instead, via a new decodeGitQuotedPath helper. That is the better fix on its own merits, not merely the cheaper one: the git argv is untouched so no fixture moves, the decode lands on the ONE normalizer already applied to both sides of the comparison so the declared and reported paths cannot disagree, and it holds regardless of the user's own core.quotepath setting rather than only when we remember to override it. A value not wrapped in a leading AND trailing quote is returned completely untouched, so the plain-ASCII path -- the overwhelmingly common case -- is byte-identical to before. Escapes decode to BYTES collected into a Buffer and UTF-8 decoded only at the end, because \303\251 is two bytes forming one character and decoding them separately yields mojibake. Malformed input never throws: a trailing lone backslash or a short octal escape degrades to the literal character, since one bad path must not take down a cleanup wave. Caught while reviewing the helper: the non-escape branch pushed a UTF-16 code unit rather than UTF-8 bytes. Git always escapes non-ASCII so its own output was fine, but this normalizer runs on the DECLARED side too, and an author may write a quoted path holding a literal é -- pushing 0xE9 alone is invalid UTF-8, so the declaration would decode to a replacement character and silently stop matching. That is precisely the failure this change removes, reintroduced on the other side of the comparison. Now converts whole code points, surrogate pairs intact. The other 2 failures: tests/parallel-dependent-plans.test.cjs pins the exact unbackticked substring "files_modified overlap" in gsd-planner.md, and rewording that comment to "declared-scope overlap" deleted it. The comment is restored verbatim and the files_deleted change rides in the pseudocode and the Rule sentence instead. Recorded in the ack fragment so the next contributor does not rediscover it the same way. Four regression tests cover the decode through the public cleanup-wave seam (the helper is module-private): a declared non-ASCII deletion merges against a C-quoted git report, the symmetric case where the DECLARATION is the quoted form, an undeclared non-ASCII deletion still blocks with the residue naming the decoded path an operator can act on, and a path merely containing a quote is left alone. Plain ASCII was already covered and is not duplicated. * fix(#3003): revert the leading-dash flag guard, the review nit was wrong The remote suite came back with 2 failures, down from 44, and both point at the same thing: tests/worktree-safety.test.cjs:7045 already pins the opposite contract, deliberately. test('a flag-shaped --files value is not re-parsed as a flag', ...) recordAgent(['--files', '--branch']) -> files_modified === ['--branch'] -> branch === 'worktree-agent-a1' ("the real --branch value must be untouched") So consuming the next argv element positionally, whatever its shape, is the tested intent of this parser, not an oversight. The security reviewer's nit claimed --deletions --files x would "swallow --files and drop both". It does not: each flag runs its own indexOf, so --deletions records the literal '--files' while --files independently still resolves to x. And that literal is a path git never reports as deleted, so it authorizes nothing -- already fail-closed with no guard at all. The guard bought no safety and silently changed --files behavior along the way, outside this issue's scope. Reverted at both call sites, which are byte-identical again, along with the test asserting the reverted behavior and the docs sentence describing it. The nit is recorded as REJECTED in the review artifact with the reasoning above, rather than as fixed -- a finding that turns out to be wrong should leave a trace of why, or the next reviewer files it again. docs/CLI-TOOLS.md now states the positional-read behavior plainly instead, so the next person meets it as documented intent rather than rediscovering it through a red suite. * chore(#3003): backfill changeset pr number to 3757 * test(#3003): cover parsePlanDocument's filesDeleted branch to clear the mutation gate CI's Stryker shard for plan-document failed at 73.28 against a break threshold of 75: 170 killed, 62 survived, 232 total. Eight of those survivors are the filesDeleted block this issue added to parsePlanDocument, which shipped with no direct coverage at all -- the field was exercised end to end through the cleanup-wave tests, but the parser itself was never called with a plan that declares it, so every mutant in the block lived. Four tests, each pinned to specific mutants rather than written for coverage percentage: - absent key yields exactly [] -- kills the array-literal seed (["Stryker was here"]) and the `fmDeleted = true` conditional, which would otherwise produce ["true"] - a scalar underscore `files_deleted:` wraps into a one-element array -- kills `fmDeleted = false`, the `&&` logical-operator swap, the `fm[""]` string mutation on the first operand, the emptied if-block, and the ternary's non-array branch - an array-valued hyphenated `files-deleted:` maps element-wise -- kills the `fm[""]` mutation on the SECOND operand (only reachable when the legacy hyphen alias is the one carrying the value) and the ternary's array branch - an empty list yields [] -- boundary case, and a genuinely distinct one from the absent key: [] is truthy in JS so it ENTERS the if, and only Array.isArray's true branch mapping over nothing produces the same [] Threshold arithmetic: 174 of 232 are needed for 75%, and these take it to about 178, so the shard clears with margin rather than landing on the line. Every expected value was confirmed by executing the built parser before being asserted, not inferred from reading the source. --------- Co-authored-by: sim <sim@local>	2026-08-22 13:17:51 -04:00
Tom Boucher	14679b866b	enhance(#2856): add default-off live-DOM UAT capability (#3716) ... * test(#2856): add failing-first suite for the live-dom-uat capability Binds the approved triage shape before any of it exists: - containment — the execute:wave:post hook must not render unless workflow.live_dom_uat is true AND the capability resolves active (fail-closed on a missing state entry, and on a non-boolean value) - criterion 4 — agents/gsd-executor.md carries no browser MCP family; asserted as an absence, which is the only way it is observable - Hyrum guard — the pre-existing mcp__playwright__* branch must stay outside the key-gated block, or upgrading silently removes working automated UI verification for every current Playwright-MCP user - parity — the browser glob list now lives in two surfaces (agent frontmatter + workflow detection block); the assertion fails if either gains or loses a family without the other Red by construction: the capability, agent and workflow block do not exist yet. Verified on the remote runner. Refs #2856 * enhance(#2856): add default-off live-DOM UAT capability A phase whose acceptance criteria needed a live DOM could not be finished by the agent that executed it: gsd-executor carries no browser tools, so it correctly returned checkpoint:human-action even though the work was not human-only, just tool-less. Every such phase degraded to "executed, then finished by hand in the orchestrator", and autonomous: false could not distinguish "a human must judge this" from "the executor lacks the tool". Implements the shape approved at triage, not the one reported. The executor's tools: line is NOT widened, in any configuration: for a first-party agent the static list is the only control that exists (ADR-1244 D2, ADR-857 D4, no per-dispatch override). Instead one default-off capability owns the key, the agent, and the step: - capabilities/live-dom-uat/ — activationKey workflow.live_dom_uat (boolean, default false), one additive step at execute:wave:post (onError: skip, gates: []), so it can never halt a wave - agents/gsd-dom-verifier.md — the only GSD agent carrying browser MCP globs, in its own tools: line, with no Bash - verify-work automated_ui_verification — a gsd:live-dom-families block naming both new families AND the key; presence alone never activates Two independent fail-closed gates: isCapabilityActive renders a hook only on state.active === true, plus the step's own `when`. The pre-existing mcp__playwright__* branch keeps the gating it already had and stays outside the new block. Pulling it behind a default-off key would have silently removed working automated UI verification from every current Playwright-MCP user on upgrade. Also closes a host gap this surfaced: execute:wave:post dispatched only contribution + gate, so ANY registered step was declared and silently never run — exactly the single-kind hand-roll loop-hook-dispatch.md names. Step 5.75 now dispatches every kind == "step". The browser-profile lock is tolerated, not coordinated: --isolated is a flag on the operator's own MCP-server registration that GSD neither launches nor parameterizes, so the verifier reports could_not_look / profile_locked, names the flag, and stops. DOM-VERIFY.md keeps could_not_look and nothing_to_report distinct behind a closed reason enum — collapsing them is the ambiguous-run-notes defect reported. Verified on the remote runner. Closes #2856 * fix(#2856): apply review findings from the orthogonal passes Correctness pass (blocker): - delete detectionBlockIsCrlfSafe. It was pass-always: it read the file, replaced LF with CRLF, then indexOf'd marker strings that contain no newline, so the replacement could not change the result and the assertion could never fail for the reason it stated. There is no real CRLF risk on this surface either — the gsd:live-dom-families block has no parser, only human and agent readers. Deleted rather than replaced, per the repo's pass-always-test rule. Isolated security pass (two minors, both real): - execute-phase.md step 5.75: this change is what first activates kind == "step" dispatch at execute:wave:post, which newly opens the ref.command shell path at that loop point. Our own step uses ref.agent and never touches it, but the door is now open, so the step-dispatch line carries the same in-context validate-before-shell warning the sibling gate-dispatch line directly below it already carries. - gsd-dom-verifier: quoted page text in DOM-VERIFY.md is attacker influenced. Require it wrapped in inline code or a fence, kept short, and never left reading as a directive to the next reader. Verified on the remote runner. Refs #2856 * fix(#2856): settle the new-agent roster ripple Checkpoint 2 returned 28 failures, none in the new suite — all of them the guards that exist to make adding an agent a deliberate act. Each is a real boundary that had to move: - docs/AGENTS.md: Tools row must copy the frontmatter verbatim (#2526), so the browser globs lose their backticks; primary-agent counts 21->22, roster 33/34->34/35, Verifiers category 1->2 - docs/INVENTORY.md: roster completeness requires every agents/gsd-*.md to be classified exactly once - gsd-dom-verifier: add the anti-heredoc instruction and the commented hooks: frontmatter pattern both agent gates require - gsd-core/bin/shared/model-catalog.json: every shipped agent needs a profile entry (#3229) - copilot-install / kilo-upgrades / qwen-upgrades: expected agent list and the 34->35 roster boundary - execute-wave-post-gate-pipeline-e2e: execute:wave:post legitimately carries one step now. Asserted as an exact shape — one step, capId live-dom-uat, ref.agent gsd-dom-verifier, onError skip — so it stays a real guard against accidental change rather than being relaxed Two findings worth naming: mcp-tool-inheritance (#2526) rejected the agent for documenting mcp__playwright__* while its tools: line withholds it — a dead instruction that invites the agent to claim a path it cannot take. The prose now names the Playwright MCP family without the dispatchable token, in both the agent and the capability fragment. runtime-launcher-parity rejected the new gsd_run call: each fenced block is its own shell, so a workflow step file invoking gsd_run needs its own canonical preamble. Propagated with scripts/sync-runtime-launcher.cjs. That script also normalizes explore.md, which is unrelated pre-existing drift the parity check tolerates, so it is reverted to keep this diff scoped. The emitted-drift ack supersedes the spent #3370 entry for execute-phase.md — it is merged into next, so its ripple is absorbed at the base and it can no longer clear anything. That is the same supersede the #3370 entry itself performed on the spent #3324 fragment. Its unrelated execute-plan.md entry is untouched. Verified on the remote runner. Refs #2856 * fix(#2856): drop the stale emitted-drift ack entry The automated-ui-verification.md entry was written speculatively rather than from a reported growth, and the check names that precisely: an ack "written or reworded in THIS diff, but nothing here needed it, so it explains nothing". The growth tier keys on the bare filename as it appears under gsd-core/workflows/ or agents/. automated-ui-verification.md is nested under verify-work/steps/, so it was never in the tracked set — only execute-phase.md was ever reported, both before and after the launcher preamble landed. Only ack what the check actually reports. Verified on the remote runner. Refs #2856 * chore(#2856): backfill changeset pr number pr:0 -> 3716. The placeholder fails both changeset-lint (fail_invalid_fragment) and docs-lint (fail_malformed_fragment) by design and can only be resolved once the PR number exists. Both now report ok against GITHUB_BASE_REF=next. Refs #2856 --------- Co-authored-by: sim <sim@local>	2026-08-20 15:07:21 -04:00
Tom Boucher	ea594300d9	fix(#3606): validate hook-kind coverage at call sites and dispatch generically (#3687) ... * test(#3606): pin hook-kind coverage in the wired guard * fix(#3606): validate hook-kind coverage at call sites and dispatch generically * fix(#3606): address review - segment-granular narrowing, zero-coverage diagnosis, quick.md, fragment extraction * fix(#3606): drop stale shrink-ack, export HOOK_GROUP_KINDS, dedupe scanner regex * chore(#3606): regenerate install-tree fixtures for new wave-post fragment * chore(#3606): sync canonical launcher preamble into new fragment * fix(#3606): keep fragment preamble ahead of first gsd_run mention * fix(#3606): revert sync script's preamble move in explore.md * chore(#3606): regenerate derived manifests post-rebase * chore(#3606): allowlist peer test files - base was red on the count lane * chore(#3606): regenerate inventory for peer's verify-command-grounding doc * chore(#3606): grounding test maps to its own module by longest prefix * chore(#3606): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-19 16:41:27 -04:00
github-actions[bot]	97ca69a53e	chore: sync next package version to 1.11.0	2026-08-19 13:51:49 +00:00
Tom Boucher	9e4f0e99ad	fix(#3631): exclude only __pycache__-resident bytecode from the consent digest (#3650) ... * test(3631): failing-first coverage for bytecode-cache in the consent hash bundleContentHash digests a walk with no exclusion, so a routine 'python3 -m unittest' inside a Python-backed capability bundle writes __pycache__ under the bundle, the recomputed hash stops matching the consent record, and the capability silently goes inactive — no error, no warning, and loop render-hooks then omits its step and gate. Two distinct triggers, and the second is the sharper one: collectBundleEntries pushes a {kind:'dir'} entry for EVERY directory and the digest emits a TAG_DIR marker for it, so an EMPTY __pycache__/ flips the hash before a single .pyc is written. A fix filtering only *.pyc would leave that live. Verified by execution against the built lib: 5 of 7 probe rows diverge from intent today, including the empty-directory row. The anti-regression rows are the point of the shape: editing a real scripts/m.py and adding node_modules/pkg/index.js must BOTH still change the hash. node_modules is deliberately not excludable — its contents are required at runtime, so dropping it from the digest would stop consent binding executable content. The symlink row pins ordering: exclusion must apply after the lstat fail-closed rejection, never before. Refs #3631 * fix(3631): exclude derived bytecode caches from the consent digest RED proven at e5ba8f1fe on the remote runner: 8 failures, exactly the rows predicted to fail, with the four anti-regression rows already green. collectBundleEntries now skips a hardcoded, gitignore-independent set from the DIGEST: basenames __pycache__, .pytest_cache, .DS_Store, and any .pyc/.pyo file. Matching is byte-exact on the raw Buffer name (the walk never utf8-decodes) and case-sensitive, so the digest does not vary with how a name happens to be spelled on a case-insensitive volume. Three properties were preserved deliberately, each pinned by a test: - The filter runs AFTER the lstat symlink/non-regular fail-closed rejection. Filtering first would have turned the exclusion into a way to smuggle a symlink past the check; a symlink named x.pyc still throws. - Excluded entries still count toward BUNDLE_MAX_FILES and BUNDLE_MAX_TOTAL_BYTES. The caps guard the WALK; the digest answers a different question, and exclusion must not become an unbounded-bytes hole. - An excluded DIRECTORY is neither emitted as a TAG_DIR marker nor recursed into. The directory marker was the sharper half of this bug: an empty __pycache__ flipped the hash before any .pyc existed, so a *.pyc-only filter would have left it live. The issue proposed either a gitignore-aware walk or a list including node_modules. Both are rejected. A consent binding must not delegate its scope to a .gitignore the bundle author does not control — one line there would drop arbitrary executable content out of the hash. And node_modules holds code that is required at runtime; excluding it would stop consent binding executable content, turning a usability bug into a supply-chain hole. What makes __pycache__ different is that CPython validates each .pyc against its sibling source, which remains hashed, so a real code change still invalidates consent. Docs: CONTEXT.md's 'EVERY regular file AND directory' claim is corrected in place. ADR-2363's residual-gap section said the walk had 'no exclusions' — per docs/adr/README.md ('ADRs are append-only') that is corrected by a dated amendment rather than an in-place edit. Its D4 argument is unaffected: skill bodies are .md and stay bound. Fixes #3631 * fix(3631): narrow the digest exclusion after two isolated security reviews The first cut of this fix passed the full suite and was still wrong. Both orthogonal reviews rejected it, and the second one found a hole that has nothing to do with Python. HIGH — an excluded DIRECTORY was 'continue'd before recursion, so its whole subtree was permanently outside the digest. Declared hook script paths allow '_', '.' and '/' with no directory or extension rule, so hooks:[{script:'__pycache__/run.js'}] installed, executed via node, and its bytes could be rewritten forever without moving the hash. Ship benign v1, collect consent, then own the machine. No Python involved. FALSE RATIONALE — the justification I wrote into the code, CONTEXT.md, the ADR amendment and the changeset claimed CPython validates a cached .pyc against its sibling source, so the source staying hashed kept consent honest. That is not true, and I proved it by execution rather than argument: default timestamp invalidation compares only the source's mtime and size, both settable by anyone who can write the bundle. A forged pyc ran while the .py was byte-identical. Also wrong: '*.pyc' matched anywhere, but a legacy sourceless scripts/x.pyc IS importable, so excluding it was a live vector. Narrowed to what is actually defensible: - a DIRECTORY named __pycache__/.pytest_cache has only its TAG_DIR marker suppressed; the walk still recurses and hashes every non-excluded child. - .pyc/.pyo are excluded ONLY when the parent basename is exactly __pycache__. - a regular FILE named __pycache__, and a DIRECTORY named x.pyc, stay bound. - declared hook paths containing a __pycache__/.pytest_cache segment or a .pyc/.pyo basename are now rejected in both validator copies — a file named .pyc can contain perfectly valid JavaScript, so the exclusion must not be reachable from a declared surface. Accepted residual risk, stated plainly in ADR-2363 and CONTEXT.md instead of explained away: a forged __pycache__/mod.pyc matching an unmodified, still-hashed mod.py executes without moving the digest. Before this change that write was detected. It is accepted to stop routine bytecode caching from silently deactivating capabilities, and it is bounded — the attacker needs post-consent write access, everything outside __pycache__/*.pyc stays hashed, and no declared surface can point into the excluded space. Known limitation, not papered over: .pytest_cache CONTENTS still move the digest. Only the directory marker is suppressed. Excluding that subtree would reopen the HIGH finding. Refs #3631 * fix(3631): drop the .DS_Store exclusion and pin what the caps actually bind Second round of isolated review findings. The hardening closed the two original holes — both re-reviews confirmed that by execution — but it introduced a new one of the same shape, and left three claims unbacked. HIGH, self-inflicted: .DS_Store was excluded from the digest at any depth, but the hook path validator was hardened only for __pycache__/.pytest_cache/.pyc/.pyo. So script:'hooks/.DS_Store' was ACCEPTED, runnableHookCommand emits the bare quoted path for a non-.js name (the branch .sh hooks already use), and capability-source copies it with its mode bit intact. Ship it +x with a benign shebang, take consent, then rewrite it forever — the digest never moves. Fixed by DELETING the .DS_Store exclusion rather than teaching the validator about it: .DS_Store has nothing to do with this issue's Python bytecode symptom, and an excluded filename is a permanently unhashed name. The narrower the exclusion, the smaller the hole. The residual-risk bound in ADR-2363 and CONTEXT.md claimed declared surfaces cannot reach excluded space. That is false and is now stated correctly: node resolves an unregistered extension through the default .js handler, so a hashed, consent-covered hooks/run.js that requires '../__pycache__/mod.pyc' reaches it in one hop. The validator guard raises the bar for DECLARED surfaces; it does not contain the risk. The two bounds that are real — post-consent write access required, everything outside __pycache__/*.pyc still hashed — are kept. The BUNDLE_MAX_FILES boundary test had gone vacuous: it padded with root-level *.pyc, which the hardening made non-excluded, so it no longer proved anything about excluded entries while the ADR claimed the caps were test-pinned. It now pads __pycache__/f{i}.pyc, with the arithmetic re-derived by execution (capability.json + the still-counted __pycache__ dir + N). BUNDLE_MAX_TOTAL_BYTES had zero coverage at all and is now pinned by a sparse 32 MiB __pycache__/big.pyc that must still trip the size cap — the test that proves exclusion did not become an unbounded-bytes hole. Added the parity assertion CLAUDE.md's Generative Fix Divergence rule requires for the two isSafeHookScriptPath copies, and proved it can fail: mutating one BUILT copy to drop .pyo made the parity check report the divergence. Also pinned semantics that were correct but untested and would have survived mutation — __pycache__/sub/x.pyc stays hashed (the parent resets to sub, which is the recursion threading itself), .pytest_cache/y.pyc stays hashed, and .pyo in both directions, which was a free surviving mutant. Changeset rewritten: it still described the rejected wholesale-exclusion semantics. Refs #3631 * chore(3631): backfill changeset PR number (#3650) --------- Co-authored-by: sim <sim@local>	2026-08-18 22:35:54 -04:00
Tom Boucher	3ab0007164	enh(#2875): materialization primitives — durable user-artifact staging and descriptor-authoritative agents (#3600) ... * fix(#2875): stage user artifacts durably across install wipes (#1874-F19) preserveUserArtifacts held user files only in an in-memory Map across the wipe, so any process death between preserve and restore lost them outright. Seven call sites, not the four the issue records. Three of them never called the helper at all - they open-coded the same read/wipe/write - so searching for callers under-counted by construction; the extra sites were found by sweeping for the pattern instead. The worst is the mainline install path, where the crash window spans the entire gsd-core tree copy rather than a single rmSync. Adds src/user-artifact-staging.cts: durable on-disk staging with a record written after the copies land as the commit point, plus recovery of orphaned batches on the next run - without recovery the staged bytes survive but the user's file is still gone, which would pass its own test while delivering nothing. Routes copyPreservingSymlink through installFs() so staging cannot bypass the install fs seam, and reunites its symlink-safety docblock with the function it documents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-3574 with four claims disproved by implementation Implementing Phase 6 disproved four statements the ADR rests on. The central decision - no single materializer - is unaffected and stands. Corrected: decision 3 was already satisfied, so nothing was extracted; the agents-bypass runtime set omitted claude, kilo and opencode, and closing it needed three new pieces of descriptor contract rather than proceeding on its own terms; three of the four blockers the layout comment names were already stale; and F19 is seven call sites, not four. Records the generalizable lesson: the defect is the pattern of holding user data in memory across a wipe, not the helper, so searching for callers of the helper under-counts by construction. Also resolves the ADR's open question on USER_OWNED_ARTIFACTS membership, and notes that copyPreservingSymlink needed routing through the install fs seam before it could be reused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close dangling-symlink blind spot and harden staging recovery An adversarial review found the F19 staging work shipped red and unsafe. Root cause, shared by two arbitrary-write findings: hasExistingSymlinkBetween missed dangling symlinks in both its root check and its per-segment walk, because it probed with existsSync, which is false for a link whose target does not exist. Fixing only the new module would have reused a guard that was itself blind. This guard protects the whole install tree. Recovery no longer throws: it degrades per entry and per file, so one bad batch cannot block the others. Previously an unrecoverable entry propagated out of the first statement of install and uninstall, before the cleanup that would have removed it - wedging the installer permanently. Partial fs adapters now throw on any omitted method instead of silently reaching the real filesystem, closing the trap that let a test poison list pass while real IO happened. Staged names must be flat, recovery refuses a dangling destination symlink, and a batch whose recovery genuinely failed is no longer swept - it was discarding the only durable copy of the file it had just failed to restore. Replaces three tests that could not fail, including the one labelled negative proof. Known limitation, documented not closed: concurrent installs sharing a staging key can still lose a batch. A real fix needs a cross-process lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enh(#2875): make the descriptor authoritative for the agents kind Deletes the inline agent-staging loop in bin/install.js and the _DESCRIPTOR_AGENTS_RUNTIMES set, so every runtime materializes agents from its capability descriptor instead of an inline hostBehaviors dispatch. Closing it needed three pieces of contract the descriptor pipeline never had, all reducible to one missing input - per-agent resolution context: a frontmatter-extensions step for claude's effort and disallowedTools, per-agent model-override resolution for kilo and opencode, and a named branding converter for hermes, whose rewrite data was already declared. Seven runtimes were on the loop, not the six the design recorded - kimi-code was found by a golden fixture, not by analysis. claude-local and kimi-code both silently lost their agents mid-change; the fixtures caught both and the cause was fixed rather than the fixtures regenerated. A parity harness gates the migration: both pipelines over identical inputs, byte-identical output including filenames, per runtime. It is demonstrated red before being trusted. Surface and install paths converge for all seven, which also fixes surface previously writing no agents for these runtimes. Codex's config.toml strip stays put - it mutates host config, which no descriptor kind models. Also routes install-model-override-resolver and install-effort-resolver through the install fs seam. Both leaked real filesystem IO from the install call tree; the stricter adapter is what exposed them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): record the agents-descriptor migration and correct the ADR count The _DESCRIPTOR_AGENTS_RUNTIMES allow-list no longer exists, so the host integration guide told readers to join a set that is gone. Replaces that with what is now true - declare an agents entry and it installs, on the surface path as well as install - and points anyone needing a per-agent transform at the three extension points rather than at a new inline branch. Corrects the ADR amendment: seven runtimes were on the inline loop, not six. kimi-code was found by a golden fixture going red, not by reading. That is the third short count this phase, all from enumerating by symbol or set membership when the thing that matters is a behavior. Adds the Changed changeset for the surface-path convergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-2866 - claude global always wrote agents on disk The claude row's global=[skills] described what capability.json declared, not what the installer wrote. bin/install.js's inline agent-staging loop was never scope-gated and never consulted the descriptor, so a claude --global install has always written agents/gsd-*.md. Phase 6 closes the gap by deleting that loop and declaring agents on claude's descriptor at global scope. On-disk bytes are unchanged - the golden fixtures did not move, which is the evidence that the descriptor, not the installer, was incomplete. #2218 is unaffected: agents are not trigger-bearing, so the wider row does not introduce a new shadowing case. Records the warning that an incomplete descriptor is invisible while a second code path silently does its work, and only surfaces when the two are forced into agreement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close review findings across staging, agents and the parity harness Two independent reviews of this branch found defects the local gates missed. Security: a dangling symlink at a migration destination allowed writing outside configDir - the same class this change claimed to close, missed at the terminal write of the flow being added. The staging-root resolver threw as the first statement of install and uninstall, so a hostile symlink bricked both, and symlinked-configDir users lost uninstall as well as install; it now degrades instead of aborting. Recovery gained a source-side symlink check and now refuses a relative destDir, which resolved against cwd. Converter dispatch gained a runtime allowlist - lint-time validation stopped mattering once this branch promoted that dispatch from the surface path to real installs. Correctness: claude --local --minimal exited 1 because the minimal profile legitimately yields zero agents and the new path treated that as a failure. cline --local silently lost its agents - its descriptor declared none while the deleted loop wrote them unconditionally. The agents prune was widened to any gsd-* entry and destroyed user files it never owned. The parity harness, on which the migration's safety argument rested, drove a synthetic registry and never byte-compared the shipped descriptors; two of its trap rows could not fail. It now drives the real registry across 13 runtime-scope rows including kimi-code and cline-local, and its red-proof is demonstrated by corrupting a live capability.json. Three goldens that had encoded the cline regression as expected behavior were corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close findings from both mandated review engines /security-review found the staging source-side walk honouring GSD_ALLOW_SYMLINKED_DEST, an opt-in documented as relaxing only the write destination. A symlinked files/ component dereferenced because copyPreservingSymlink lstats the leaf only, so an intermediate link is followed. The source walk no longer honours the opt-in; the destination check still does. /code-review spec axis found this branch had reintroduced its own bug: migrateLegacyDevPreferencesToSkill's new symlink refusal threw unguarded after the legacy dir was wiped and before the staged batch was restored, so a planted symlink bricked uninstall permanently and orphaned the batch. Refusal kept, abort removed. kimi-code local silently lost its agents, the same class as the cline bug, and the parity harness recorded that exclusion as intentional - the third test in this branch to pin a regression as correct. --minimal now creates an empty agents/ dir that never existed. Behaviour restored rather than softening the changeset, so its byte-identical claim stays true. Standards axis: try/finally removed from twelve test bodies, fast-check properties added for parseOwnerPid, boundary coverage at the grace window and the ancestor-probe depth, a parity assertion for the staging-root helper duplicated across two files, and the 8-deep config walk deduplicated. Records 60-review.json with every finding and disposition from five passes, including the smells left unfixed and why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): prune stale agents unconditionally in minimal mode The previous round stopped an empty agents/ directory being created when the resolved profile yields no agents. That was implemented by skipping the agents kind entirely, which also skipped its stale-agent prune - so a full to minimal downgrade left stale gsd-* agents behind. The deleted inline loop pruned unconditionally and only skipped writing. Those are three separate conditions, not one: prune always, write only when there is something to write, create the directory only when writing. Both call sites now run _removeGsdEntries before the empty-staged early exit. The symlink-escape guard moved with it, since the prune also touches dest. Codex .toml agents and the config.toml stanzas are cleaned again, and user-owned agents are still preserved. The agents/ directory is left in place after a prune empties it, matching every sibling kind - none of them remove the destination directory itself. Golden fixtures confirmed byte-identical: the prune is a no-op on a fresh install, so fixture generation is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): document interrupted-install recovery for user-owned files The durable-staging fix is invisible to the user it protects. Someone whose install died mid-flight has no way to know USER-PROFILE.md was staged before the delete, that the next run restores it, or that recovery happens at the start of that run rather than in the background. Written as the task the user has - finish the interrupted command - rather than as a description of the mechanism, and states what it will not do: overwrite a file already present, or touch staging belonging to another install still running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2875): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2875): assert the J8 model override without building a regex CodeQL flagged incomplete string escaping: the assertion interpolated the override value into a RegExp while escaping only forward slashes, which is meaningless in a constructor, leaving real metacharacters unescaped. The failure direction was the dangerous one - a metacharacter would have made the match more permissive, so the row would pass when it should fail. That matters here because J8 exists precisely because an earlier revision was a tautology; the rewrite reintroduced a different way for the same assertion to stop discriminating. Replaced with a line-wise exact match, so no regex is constructed at all. Swept the other test files this branch adds; no sibling instances. lint:ci passed on the original - lint-no-adhoc-regex-escape matches a full metachar-escape copy, so a single slash replace slipped under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-17 17:25:53 -04:00
Tom Boucher	71180983a0	fix(#3423): standardize on <required_reading>, retire the files_to_read emit tag (#3432) ... * fix(#3423): standardize on required_reading, retire files_to_read emit tag * test(#3423): flip tag assertions, extend consistency guard to spawner surfaces * fix(#3423): sweep capabilities fragments, regen registry+skills, anchor executor test * chore(#3423): acknowledge tag-rename emitted ripples and workflow growth * chore(#3423): broaden emitted-ripple acknowledgment to all embedders * chore(#3423): settle emitted-drift acks post-rebase (merge 3004/1689-owned keys) * chore(#3423): drop stale ripple acks, ack execute-phase growth * chore(#3423): restore pristine 3004 fragment, keep only consumed appends * chore(#3423): backfill changeset pr number * chore(#3423): settle emitted-drift acks post-merge (move code-review-fix ripple into 3190, tag-rename ripples into 3191/3297) * chore(#3423): re-arm 3324 ack for execute-phase.md tag-rename ripple * fix(#3423): trim 8 bytes from execute-phase model note to hold ADR-857 margin, re-arm 3370 ack for net +4 growth --------- Co-authored-by: sim <sim@local>	2026-08-14 16:03:48 -04:00
Tom Boucher	895d9df96d	fix(#3477): run untrusted key_links patterns on a linear-time engine (#3496) ... `cmdVerifyKeyLinks` compiled `must_haves.key_links[].pattern` from plan frontmatter with `new RegExp()` and tested it against whole file contents, so a nested-quantifier pattern such as `(a+)+$` hung `verify-phase` indefinitely (CWE-1333). JavaScript has no regex-execution timeout. Untrusted patterns now run on RE2 (re2js), whose match time is linear in input length — the class is closed by the engine, not by a heuristic screen. The screen lost in the ADR-0174 consolidation was deliberately NOT restored: it never worked, since `(a|a)*$`, `((a+))+$`, `(a+){2,}$` and `(a{1,3})+$` all evade it. A refused pattern's matcher returns false for every input, so it cannot report a match no matter what the caller does. The engine is vendored at gsd-core/bin/lib/vendor/re2js.cjs because gsd-core/bin/** is copied into installed trees with no node_modules; runtime dependencies are unchanged. New ESLint rule local/no-external-require-in-bin enforces that invariant, which had been documented in a comment since the #3024/#2071 bug class and enforced nowhere. Backreferences and look-around are unsupported by RE2 by construction — disclosed in a Changed changeset. Closes #3477 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-14 14:34:36 -04:00
Tom Boucher	967bddba37	fix(#3384): strip mcp__* tool grants from zcode-installed subagents (#3483) ... * fix(#3384): strip mcp__* tool grants from zcode-installed subagents ZCode's dispatcher treats every mcp__<server>__* entry in an agent's tools: frontmatter as a required MCP server and hard-fails the subagent spawn (CONFIGURATION_ERROR) when it is not connected, whereas Claude Code treats the same grants as an optional allowlist. ZCode shared Claude's verbatim agents copy (converter: null), so all 8 MCP-granted agents failed to spawn out of the box with zero MCP servers configured. Add convertClaudeAgentToZcodeAgent — a line-surgical converter that filters mcp__* entries out of the frontmatter tools: grant list (both inline comma and YAML block-list shapes) and preserves every other byte. Declare it on both of zcode's capability.json agents entries and cut zcode over to the descriptor-driven agents path (_DESCRIPTOR_AGENTS_RUNTIMES) so the legacy inline loop stops deleting+re-copying the converted agents raw. Claude Code, Kimi, and Gemini install behavior is unchanged. * chore(#3384): link changeset fragment to pr 3483 --------- Co-authored-by: sim <sim@local>	2026-08-14 11:49:01 -04:00
Tom Boucher	9dd240f634	fix(#3302): return per-agent worktree metadata from workflow scripts (#3450) ... * fix(#3302): return per-agent worktree metadata from workflow scripts * fix(#3302): link changeset fragment to pr 3450 --------- Co-authored-by: sim <sim@local>	2026-08-14 09:28:22 -04:00
0xdhx	0396d9cab1	enhance(#2483): stop the claude reviewer lane from inheriting CLAUDE.md + auto-memory (#2493) ... * enhance(#2483): env-guard the claude reviewer leg against CLAUDE.md injection The claude reviewer in workflows/review.md was a bare headless `claude -p` spawn run from the project cwd, so it inherited the invoking user's global CLAUDE.md, the project CLAUDE.md, and Claude Code auto-memory. That made it the only reviewer leg seeing anything beyond the prompt file. gather_context assembles PROJECT.md, the roadmap section, every PLAN file, CONTEXT.md, RESEARCH.md and REQUIREMENTS.md into the prompt before any reviewer runs; the gemini leg receives only that prompt and the codex leg runs --ephemeral. Beyond the measured ~4k tokens/spawn, the asymmetry cuts at the workflow's own premise: "independent review" meant something different for the claude leg than for the other two. Guard both dispatch lines with a per-invocation `env CLAUDE_CODE_DISABLE_CLAUDE_MDS=1`. `env`, never `export` — the flag must not leak into the orchestrating session (which may itself be Claude Code on the SELF_CLI="auto" path) or into any later spawn. review.md is the only claude -p call site in the installed tree, so this is two lines on one surface. The self-skip logic is untouched. * enhance(#2483): fix CRLF-fragile split and regenerate workflow baselines Two CI failures from the first push, both mine: 1. lint-tests: the new regression test split readFileSync content on a literal "\n". On a Windows git-autocrlf checkout that leaves a trailing "\r" on every line (local/no-crlf-fragile-split). Use .split(/\r?\n/). 2. golden-install-parity / workflow-size-budget / workflow-compat: editing gsd-core/workflows/review.md changes its content hash and byte size, and both are pinned in committed baselines. Regenerated via the repo's own generators (npm run size:baseline, npm run gen:golden). The regenerated diffs are review.md-only: exactly one hash line per golden-install-parity fixture and one size entry in workflow-size-baseline — no unrelated drift swept in. Full suite now green locally: 2113 pass, 0 fail, 3 skipped (run with HOME and CLAUDE_CONFIG_DIR overridden to throwaway dirs; live profile verified untouched afterward). * enhance(#2483): adapt guard-test matcher to the effort-args dispatch reshape The effortSurface wiring (#2481) reshaped the bare-model dispatch to `claude $CLAUDE_EFFORT_ARGS -p -`; the invocation matcher's dash-first form could no longer see it, and the count assertion failed exactly as designed. The matcher now tolerates variable expansions between `claude` and its first literal flag. Negative-controlled both ways: a stripped guard and a deleted dispatch line each still fail. * enhance(#2483): also guard the claude leg against auto-memory injection CLAUDE_CODE_DISABLE_CLAUDE_MDS suppresses CLAUDE.md file loading; auto-memory is an independently-toggled mechanism with its own flag. Add CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 to both dispatch lines, correct the docs/COMMANDS.md and changeset claims that credited the first flag with covering auto-memory, and extend the regression test to require both flags on every claude invocation (negative-controlled: 2/4 assertions fail with the new flag removed). * enhance(#2483): match the claude binary in command position, not argument position The line-oriented invocation matcher counted any line where the token `claude` was followed by a flag. #2589 (landed on next as 920a5f3f) reshaped the effort-args lookup from --host claude 2>/dev/null | jq -r '.effort_argv_string // ""' to --host claude --pick effort_argv_string which put a flag immediately after `claude` and made the config query read as a third claude dispatch, failing the count assertion. The defect class is a binary name in *argument* position being read as a command. Fixed at the class rather than the instance: tokenise the line and skip any `claude` whose preceding token is a flag. That also covers the latent sibling one line away in review.md (`command -v claude`), which escaped today only because its next token is a redirect. Negative-controlled four ways: stripping CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 fails, stripping the whole env guard fails, adding a genuine third unguarded dispatch (`timeout 900 claude --output-format text -p -`) still fails — so the narrowing did not blind the matcher to reshapes, which is the property the count assertion exists for — and the pre-#2589 jq form of the lookup still passes, so the matcher is not pinned to today's base. * enhance(#2483): carry the claude reviewer's memory guard as declared lane data ADR-2782 Phase 5b replaced the hand-authored per-CLI dispatch legs in review.md with the declared lane table, so the two `env`-prefixed shell lines this PR previously added no longer have a surface to live on. The guard is reimplemented where the lane contract now lives. `SpawnInvoke` gains an optional `env`, the claude lane declares the pair, the resolver folds own string-valued entries into `SpawnPlan.env` (absent or empty resolves to null, so the runner has one shape to test), and the runner passes it to spawn. Production merges it OVER `process.env` into a fresh object for that one child, so nothing reaches the orchestrating session or any other lane in the run. Declared data rather than a handler (D6): the pairs are static per lane, which is precisely what the manifest vocabulary is for. The capability manifest carries the same field, because the lane-fidelity test compares manifest and descriptor over the union of `invoke`'s keys. The regression test is rewritten against the resolver and runner rather than review.md's text. It gains the property the source-text assertions could only approximate: that `process.env` is never mutated. Scope boundary, asserted rather than left in prose: `env` is not part of the trust-disclosure surface, which is safe only while no manifest body reaches the resolver — the registry's reviewer bodies contribute slugs to the parity check and execution resolves from `REVIEWER_LANES`. The new test fails first if that ever changes. * enhance(#2483): restate the guard's mechanism in the docs and changeset Both described the fix as two `env`-prefixed dispatch lines, which is the surface ADR-2782 Phase 5b removed. The user-visible behaviour is unchanged; the carrier is not, and a changeset that ships a description of a mechanism the tree does not have is a CHANGELOG entry nobody can verify against the code. * enhance(#2483): cover the production spawn wiring end to end The unit tests stop at the runner's `deps.spawn` seam — every one injects a spy. Production supplies that seam in `gsd-core/bin/gsd-tools.cjs` as a hand-written object no test constructs, so the chain could be correct all the way to `SpawnPlan.env` and the merge could still be wrong or absent with the suite green. Deleting those four lines was the one mutation that left every other control silent. This runs the real `spawnSync` through `gsd-tools review-lane invoke`, with a `claude` shim on PATH that records the environment it was handed. It asserts both halves in one test: the pair arrives, and an unrelated inherited variable survives — a wiring that REPLACED the environment rather than merging over it would satisfy the first and break every lane's PATH and HOME. POSIX-only; mediating a Windows `.cmd` shim is a separate concern the repo already tests on its own. Noted rather than fixed: `timeout`, `killSignal`, `maxBuffer` and `shell: false` on that same object are equally uncovered. That is the epic's gap, not this change's, and closing it is not in scope here. * enhance(#2483): validate the invoke.env shape and register it as spawn-only `env` was the one spawn-invoke field with no shape enforcement: every sibling in `validateSpawnInvoke` is checked, and a manifest declaring `env` as an array, a string, a number, or an object with non-string values passed validation in silence. That matters more than an ordinary schema gap here, because `resolveLanePlan` DROPS a non-string value rather than coercing it — so an unvalidated manifest declares a pair that never reaches the spawn, which is the failure a memory guard can least afford. Two registrations, not one. `env` was also absent from `SPAWN_ONLY_INVOKE_FIELDS`, which is the list the openai-http arm rejects against — so `invoke.env` was accepted on a transport that issues an HTTP POST and has no child environment at all. It was the only spawn-shaped field accepted there; the other six each produce two errors. Self-found while sweeping the class, not raised in review. Keys are held to the portable POSIX environment-name grammar. That is a policy, not a claim about what an environment can hold: measured, only NUL is actually rejected by `spawnSync`, while `=`, a leading digit, a dash and a space are all carried through to the child (an `A=B` key arrives as the raw entry `A=B=value`). They are refused because a name outside the grammar is not portably addressable by the program meant to read it. `__proto__` is refused for a different and concrete reason. It passes that grammar and is a real own key once a manifest is JSON-parsed, but assigning it onto a plain accumulator goes through the inherited `__proto__` setter rather than creating an own property — and for the string values this field permits the setter is a no-op that does not even change the prototype. The pair would validate and then simply vanish before the spawn. (An environment CAN carry a literal `__proto__` entry; this is about the resolver's accumulator, and the error message says so.) Deliberately narrower than the sibling reserved-name guards in this file, which also reject `constructor`/`prototype`: those guard bracket lookups that resolve prototype members, whereas this reads via `Object.keys` plus an own-value read, where `constructor` assigns as an ordinary key the spawn could carry. `effortChannel` is deliberately left in neither field list: ADR-2782 D2 defines it for both transports, so it is shared rather than spawn-only. Reversion-controlled, three mutations, all three fire a named test: dropping `env` from the discriminator fails `httpTransportRejectsEnv`; removing the `__proto__` arm fails `envRejectsProtoKeyThatWouldSilentlyVanish`; disabling the block fails four. (#2483) * enhance(#2483): amend ADR-2782 D2 for the invoke.env vocabulary widening D2 records the spawn `invoke` shape as a closed vocabulary, and its Amendments section carries a dated entry for every prior widening (Phase 1 #2794, Phase 2 corrections #2795, Phase 5b #2799). This change extended that vocabulary in code without touching the ADR governing it, so the ADR contradicted the implementation — and the repo's own convention, recorded in CONTEXT.md, is that the ADR is amended in the same PR precisely because the prior widenings did it correctly. Adds the `invoke.env` row to the D2 table and a dated Amendments entry. The entry also corrects the authority this change cited. The source comment pointed at D6, which governs the closed `handler` enum — imperative behavior admitted first-party — and says nothing about the `invoke` field vocabulary. That is D2's territory, so the citation never covered the gap. Two claims are corrected rather than restated, both about the trust boundary that justifies leaving `env` out of the D5 disclosure signature: - The regression test does not enforce that boundary. On one forged lane it shows the resolver folds whatever it is handed, so a future path feeding it manifest lanes would not make any assertion in that test fail. Its comment claimed it "will fail first"; that was wrong, and both the comment and the ADR now say the boundary is a property of the production call chain instead. - The ADR is internally inconsistent on whether third-party manifest lanes execute at all: Consequences says adding a reviewer needs "no core patch", while `gsd-tools.cjs` rejects every slug absent from the first-party REVIEWER_LANES map. CONTEXT.md, `workflows/review.md` and the resolver's own header take the first view. #2483 did not create that inconsistency and does not resolve it; the entry records it rather than settling it in its own favour. (#2483) * enhance(#2483): document invoke.env in the capability-manifest reference ADR-2782 points capability and plugin authors at `docs/reference/capability-manifest.md` as where the lane vocabulary must be visible, and its `invoke` row enumerates the spawn sub-shape field by field. `env` was absent from that table while being part of the real shape, so the one document a third-party capability author would actually consult to learn the field exists did not mention it. Squarely Diataxis reference material — a field-by-field schema description — so it goes here rather than in the user-facing prose, which was already updated. States the constraints a manifest author can actually trip, and is explicit that the name grammar is a portability policy rather than an OS limit, so a reader does not take it for a claim about what an environment can hold. (#2483) * enhance(#2483): disclose and sign the reviewer lane's env and residual invoke fields `invoke.env` was undisclosed at install time. That was defensible while manifest lanes could not execute — the premise this PR's own ADR amendment recorded — and #2927/#3062 retired it: `routeReviewLane` now merges installed overlay `reviewer` bodies into its lane map via `mergeReviewerLanes`, which is a field-identical merge by ADR-2782 D1 and deliberately does not deep-validate. An overlay's whole `invoke` therefore reaches `resolveLanePlan`, and `env` reaches the spawned child. A consented third-party capability could set `NODE_OPTIONS=--require ./evil.js` on a reviewer lane with no install-time disclosure and no re-consent. The same file already decided what `env` means in a manifest: MCP servers fold it into the disclosure signature and render each key and value in the consent prompt, with an inline rationale naming this exact shape. Reviewer lanes get the identical treatment. `env` was the ninth unsigned invoke field, not the first. `defaultHost` (the manifest's OWN fallback egress host, used whenever the config key resolves to nothing), `path`, `outputChannel`/`outputArg`, `modelArg`, `effortChannel` and `modelDiscovery` all reach `resolveLanePlan` and none was bound. Enumerating a ninth name leaves the tenth open, so the lane signature carries a RESIDUAL of every other declared `invoke` key — the completeness backstop `rawConfig` already gives the MCP line (#1459 finding 5), and the "sign the whole object" remedy the recorded decision on this class prefers. `defaultHost` is also rendered: `resolvedHost` comes from user config, so a lane whose key is unset displayed "(unresolved …)" — which reads as "no destination" — while the runtime egresses the plan and review text to the address the manifest picked. D4.5 is preserved one level down: the extra element is appended ONLY when the lane declares something beyond the eight already-bound fields, so an env-free lane's signature stays byte-identical and no already-consented capability is re-prompted for a field it does not use. A lane that does declare one re-consents, which is the point. Execution-primitive env names are FLAGGED in the prompt, not refused in the validator. A denylist cannot be the boundary here: `PATH` alone is a complete execution primitive for a spawn lane and can never be refused, the child is an arbitrary third-party binary so the true set spans every interpreter's injection vars, and the MCP `env` this mirrors refuses nothing and discloses everything. Missing a name costs a quieter line, never a boundary. Refs #2483. * enhance(#2483): exercise the real overlay merge path in the guard test The test named for the manifest/first-party boundary did not test it. It built a forged lane locally, handed it straight to `resolveLanePlan`, and asserted that `REVIEWER_LANES` did not contain it — so no assertion in it depended on the claim its name made, and a code path that fed manifest lanes to the resolver would not have made it fail. Its own comment said as much, and named the production chain as the real carrier of the guarantee: "gsd-tools.cjs builds its lane map solely from REVIEWER_LANES". That sentence is now false. #3062 merged overlay reviewer bodies into that map, so the test's premise and its subject both moved. The replacement routes through `mergeReviewerLanes` — the real helper the production path calls — and asserts the overlay lane is admitted, resolves, and carries its `env` into `SpawnPlan.env`. That makes the security property falsifiable instead of narrated. It then asserts what now backs it: the env is disclosed on the surface, rendered key and value in the consent prompt, flagged when the name is an execution primitive, and bound to the signature so a value change, an addition, or a removal each force re-consent. Three further cases, because the finding's generative half is what stops it recurring: the residual backstop is asserted against five fields including one that does not exist (`aFieldThatDoesNotExistYet`), so a future vocabulary widening cannot silently re-open this; a fully-enumerated lane is pinned to its original 8-tuple, which is what keeps the fix from re-prompting every consented capability; and an http lane's manifest-declared `defaultHost` is asserted to reach both the prompt and the signature. Reversion-controlled, seven mutations, all seven fail a named test: env dropped from the surface, the prompt's env line removed, the execution-primitive warning removed, the signature's extra element never appended, the residual emptied, the defaultHost line removed, and the declares-something test un-widened. The last of those was SILENT on its first run and its test was written in response, then the control re-run. Refs #2483. * enhance(#2483): correct the ADR amendment's manifest-lane premise The amendment argued `env` needed no D5 disclosure because a manifest's `invoke` fields never reach `resolveLanePlan`. That was true when written and #3062 retired it 22 hours after this branch's last commit: `routeReviewLane` now builds its lane map from `mergeReviewerLanes(REVIEWER_LANES, loadRegistry({includeInstalled: true}))`, and D1's no-translation-layer rule makes that a field-identical merge, so an overlay's whole `invoke` reaches the resolver and executes. The entry had named this exact trigger — "were manifest lanes ever made executable, `env` must join the disclosed surface in that change, and nothing here will trip if it does not." Nothing tripped. The premise is rewritten to current truth rather than annotated, because an ADR is read in fragments and a superseded paragraph left standing reads as live reasoning to the next author; a one-line dated tombstone points at git for the withdrawn text. The rewritten entry records four things the first draft could not: that the enumeration itself was the defect (`env` was the ninth unbound `invoke` field, and `defaultHost` and `path` are egress-relevant on their own), that the residual is what closes the class, that D4.5's byte-identical-signature property is preserved by appending the residual only when a lane declares something beyond the eight bound fields, and that consent — not shape validation — is the boundary, since no honest env denylist can exclude `PATH`. It also closes the internal inconsistency the previous entry could only record. This ADR, `CONTEXT.md`, `gsd-core/workflows/review.md` and `resolveLanePlan`'s own header all said overlay lanes reach the resolver while the runtime said otherwise; #3062 resolved that in the documents' favour, which is what makes the disclosure mandatory rather than defensive. Refs #2483. * enhance(#2483): record in the manifest reference that invoke fields are consent-bound `docs/reference/capability-manifest.md` is the field table ADR-2782 points capability authors at, and it described `invoke` purely as a schema. A third-party author reading it could not learn that everything they declare there is shown to the user at install and bound to the consent signature — which is exactly what they need to know now that an overlay reviewer lane executes (#2927/#3062). States the two things the schema alone cannot: that `env` and `defaultHost` are named in the consent prompt and the rest is covered by a residual, so any change to a declared `invoke` field forces re-consent; and that `env`'s validation is a portability policy rather than a safety boundary, since `PATH` is a complete execution primitive and cannot be refused. Names that are execution primitives are highlighted in the prompt instead. Refs #2483. * enhance(#2483): add a Security changeset for the reviewer-lane disclosure The existing fragment describes the enhancement this PR was opened for and stays as it is. The disclosure fix is a separate user-visible change of a different type: a capability declaring `invoke.env` or `invoke.defaultHost` will ask for consent once more, and users are entitled to read why in the changelog rather than discover it as an unexplained prompt. Type is `Security` rather than `Changed` because the entry describes a closed code-execution disclosure gap, not a behaviour adjustment. Refs #2483. * enhance(#2483): correct this round's own claim about who gets re-prompted Self-found while auditing the round's claims before publishing them. The changeset and the ADR entry both stated that a capability declaring `invoke.env` or `defaultHost` "will ask for consent once more". That is wrong, and it overstated the cost of the fix in the one direction a maintainer would have had to take on trust. A code change to `disclosureSignature` re-prompts nobody. `hasProjectConsent` matches on the recomputed bundle `contentHash` — the signature has not been the security binding since #1459 CB-1/CB-2 — and the upgrade path's `executableSetChanged(old, new)` compares two disclosures both computed by the CURRENT code, so widening the signature moves both sides of that comparison equally. First-party capabilities never reach the path at all: the install flow blocks a first-party id before trust evaluation. What the widening actually buys is forward-looking, and is the real argument for it: an upgrade whose manifest edits a declared `invoke` field now registers as an executable-surface change and re-consents, where before it could change what the lane runs in silence. Also measured and recorded, because the D4.5 property was stated more strongly than it deserved: of the twelve first-party reviewer capabilities, ZERO are in the byte-identical-signature class — every real lane declares at least `effortChannel`. The property is a guarantee about minimal lanes, not a description of the fleet, and the ADR now says so. Refs #2483. * enhance(#2483): sign and disclose the probe binary and the lane's outer fields Found by this round's own adversarial review, and it is the same defect one level out: the `invoke` residual cannot reach the lane body's OUTER fields, and `probeLane` SPAWNS `probe.binary` with `--help` before dispatch (`review-lane-runner.cts`, the `command-exists`/`command-capability` arms). An overlay naming an arbitrary probe binary therefore executes it — unsigned and undisclosed, exactly as `invoke.env` was, and reachable on the same #3062 path. The lane element now carries a second residual over the outer fields, and the probe binary is shown in the consent prompt when it differs from the dispatch binary — it is a program that runs, and the user is entitled to see it. TWO fields stay excluded, and that is a decision rather than an omission: `reviewsSection` and `timeoutFloorMs` are ADR-2782's cosmetic carve-outs (matrix A10/A13), where re-consenting would present a prompt carrying no security information. A test pins that they remain excluded, so a later widening cannot quietly reverse D4.5 while claiming to complete this fix. Also corrects a miscount introduced by the previous commit: the source comment said the enumeration had fallen behind by "seven fields" and omitted `fallbackModel`, while asserting `env` was the ninth. `resolveLanePlan` reads twelve `inv.*` fields and four were bound, so the number is eight. The comment now states the derivation rather than just the total. Reversion-controlled: emptying the outer residual fails "repointing the probe binary must force re-consent"; removing the render line fails its own named assertion. Refs #2483. * enhance(#2483): refuse execution-primitive env names as defence in depth Adopts the review's B5 after this round's own adversarial pass refuted my reason for declining it. I had argued a denylist was worthless because `PATH` can never be refused. That was wrong on the facts: no shipped reviewer manifest declares `PATH`, so it can be refused, and it is the most complete primitive in the set — repoint it at a directory holding a fake binary and the declared `invoke.binary` is irrelevant. A list that cannot be exhaustive can still close the highest-confidence, lowest-legitimacy routes. So the validator now rejects `PATH`, `NODE_OPTIONS`, `LD_PRELOAD`, `DYLD_INSERT_LIBRARIES`, `BASH_ENV`, `PYTHONPATH`, `PERL5OPT`, `RUBYOPT`, `GIT_SSH_COMMAND`, `JAVA_TOOL_OPTIONS` and their siblings on a reviewer lane. A lane needing a specific executable declares an absolute `invoke.binary` instead of reshaping the child's environment. The comment states plainly that this is defence in depth and NOT the boundary — the boundary is install-time consent, which discloses every declared pair and binds it to the signature, so an unlisted name is still SEEN before it runs. That framing is load-bearing: a future reader who mistakes the denylist for the control will under-invest in the one that is, which is the failure mode I was trying to avoid by declining it outright. Two tests: the rejection itself across ten names, and a guard asserting no shipped reviewer capability declares a denied key — so if the list ever outgrows its evidence, that surfaces as a decision rather than a silent removal. Refs #2483. * enhance(#2483): fix two stale D5 enumerations elsewhere in the ADR The previous commit rewrote the amendment's premise but swept only the amendment. Two normative passages earlier in the same ADR still enumerated the old closed field list and now contradicted it: the `executableSetChanged` trigger list, and the split-binding note asserting the seven manifest-derived fields were "everything that is SHA-pinned". That is the failure the rewrite-don't-annotate rule exists to prevent, one section over — an ADR is read in fragments, and a fragment carries no supersession marker, so a reader landing on either passage would have taken the superseded enumeration as current. Both now name the residual as the mechanism rather than restating a list, which is also what stops them going stale the next time the vocabulary widens. Found by this round's adversarial review, which grepped the whole document rather than the section under edit. Refs #2483. * enhance(#2483): stop the probe disclosure claiming a spawn that does not happen The probe line added one commit ago rendered "probes by running: <binary> --help" for every lane. That is false for `kind: "command-exists"`, which only calls `hasBinary` — a PATH/filesystem scan that starts no process. Only `command-capability` spawns. A false statement in a consent prompt is worse than a missing one: the prompt is the surface a user is asked to trust, and this one overstated what a lane does. Worse, the test I wrote to prove the fix used `command-exists` — the kind that does NOT spawn — so it pinned the wrong claim and would have kept the error green forever. The surface now carries `probeKind` and the two kinds render differently: a spawn is described as a spawn, a presence check as a presence check. The test exercises both, and asserts the `command-exists` path never emits the spawn wording. Also corrects the field-count parenthetical to state its derivation unambiguously — `resolveLanePlan` reads thirteen `inv.*` fields including `env` (twelve before this PR), four were bound, so eight were unbound before `env` and nine including it. The bare "twelve" was true only of the pre-PR tree and read as a claim about the current one. And retires two comments that argued AGAINST the validator denylist this round then shipped. Leaving them would have handed the next reader the reasoning for removing it. Reversion-controlled: conflating the two probe kinds fails a named test. Refs #2483. * enhance(#2483): match the reviewer-lane env denylist case-insensitively The denylist added one commit ago compared exact case, so `Path`, `path`, `node_options` and `Node_Options` all passed it. Windows environment lookup is case-insensitive, so those reach the child as `PATH` and `NODE_OPTIONS` — the exact inputs the list names. An exactly-cased denylist is worse than none: it reads as a control while admitting the input it was written to refuse, and the next reader has no reason to doubt it. Members are stored uppercase and the key is folded before lookup; the name grammar already constrains keys to ASCII, so a plain fold is sufficient. Reversion-controlled: restoring the exact-case compare fails `envDenylistIsCaseInsensitive` on `Path`. Refs #2483. * enhance(#2483): correct the docs that still described the denylist as absent Both the ADR and the manifest reference still said `env` carries no denylist and that `PATH` "can never be refused" — written when that was this round's position, and left standing after the round reversed it. A reader landing on either passage would have taken the superseded argument as current, which is precisely the failure the rewrite-don't- annotate rule exists to prevent. Both now describe the denylist, name `PATH`'s inclusion and the case-insensitive match, and keep the limit explicit: the list cannot be complete against an arbitrary child and disclosure runs before validation, so consent remains the boundary. The ADR's byte-identical-signature claim is also corrected rather than softened. With the outer residual in place, a lane producing no residual is one the validator rejects — it declares no `flags`, `probe`, `emptyOutput`, `evidenceClass`, `requiresBinaries` or `promptBudgetKey`. So the property is about the ENCODING, not a claim that any real signature is unchanged, and it is not the argument for the change being safe. That argument is that consent binds to the bundle contentHash and no existing consent is invalidated at all. Refs #2483. * test(#2483): cover the three new lane disclosure fields in the injection-safety parity guard The PARITY test in section N exists to catch a renderer field that skips `renderValueForPrompt` (#3248). Its payload manifest is hand-maintained, so it covers the fields that existed when it was written — slug, binary, args, hostConfigKey, handler — and none of the fields this PR adds. This PR renders three further manifest-supplied values into consent-prompt lines: `invoke.env` (keys and values), `invoke.defaultHost` and `probe.binary`. The gap was silent rather than theoretical: with the lane env line reverted to the pre-#3248 raw form, the whole 948-test lane/capability/trust-disclosure suite stayed green. Two manifests, because the shapes render disjoint lines — `defaultHost` only on the openai-http branch, `env`/`probe` only where declared, and the probe line only when the probe binary differs from the dispatch binary. Non-vacuity is asserted on the typed disclosure object and on structural line counts, not by substring-matching rendered prose: CONTRIBUTING.md forbids raw text matching on test output, and this section's own header promises structural assertions only, so a prose match here would have made that promise false. Negative-controlled three ways against the merged tree, each producing exactly one named failure: env rendered raw, defaultHost rendered raw, probe binary rendered raw. * docs(#2483): extend the #3248 render-site comment to the fields this PR adds The comment enumerates every manifest-supplied value that must pass through `renderValueForPrompt`, and it stopped at `handler` — the reviewer-lane fields that existed when #3248 landed. This PR renders three more (`defaultHost`, the probe binary, and the env keys and values), so the list understated its own contract in the one place a future author would check before adding a fourth. A comment enumerating a closed set is a set that can silently fall behind the code it describes; the parity test added alongside is what makes the omission fail loudly rather than read as deliberate. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-11 17:42:41 -04:00
Tom Boucher	cf6de5e1c0	feat(#2871): resolve triggers and host precedence, not just placement (#3291) ... * test(#2871): failing-first suite for trigger-surface resolution 23 tests over the 50-test-matrix rows. RED by construction: resolveTriggerSurface and DEFAULT_TRIGGER_PRECEDENCE do not exist yet, and the validator silently ignores triggerPrecedence today. Written in the per-runtime describe idiom the other four runtime-artifact-layout suites use, not a table. The rows that carry the weight: windsurf must NOT report a shadow it does not have, since its global scope emits only agents and agents are not trigger-bearing; agents and kimi-agents must be absent from the output for every runtime; and reordering a runtime's triggerPrecedence must flip the winner, which is the only assertion that proves the axis is read rather than decorative. Stems are injected, never scanned, so the surface is assertable with no filesystem. * feat(#2871): resolve triggers and host precedence, not just placement resolveTriggerSurface(runtime, scopes) returns every /gsd-<name> trigger a runtime emits, with the scope and kind that produced it, whether the host registers it directly or only through a router, and which artifact shadows it. resolveRuntimeArtifactLayout is untouched -- its 7 callers need placement only and the issue requires them unchanged. AGENTS ARE NOT TRIGGER-BEARING, and ADR-2866 said they were. The host-integration matrix models command and dispatch as separate interface points: an agent is invoked through the Agent tool's subagent_type, not by typing a slash trigger, and _copyStaged never applies the kind prefix to an agents entry. So agents and kimi-agents are excluded from the surface entirely, and this commit amends ADR-2866 with a dated correction. #2218's conclusion is unchanged -- the collision is strictly commands-vs-skills, and claude's local /gsd-* trigger surface is still fully shadowed -- but the ADR implied the local agents surface was lost too, and it is not. That correction is what makes windsurf come out right. Its global scope emits only agents, so it has no global trigger and its local commands are unshadowed. Model agents as trigger-bearing and windsurf falsely reports a full shadow. The triggerPrecedence axis lands on all 19 descriptors as an ordered kind list, one value with one owner, rather than a numeric rank spread across N kind entries with nothing keeping them consistent. Validation uses a required-with-default shape that has no precedent in this validator -- every existing axis is hard-required -- so a third-party capability.json omitting the field still validates, which is what ADR-894's additive-only contract promises. Winner resolution reads Phase 1's scope rank first, then the kind ordering. A test reorders the axis and asserts the winner flips, since an axis that is added, validated and never consulted would pass every other assertion. shadowedBy ships unread. Phase 4 (#2873) is its first consumer, per this issue's out-of-scope note. Verified via the remote runner. * fix(#2871): single-source namespacedByDir and close two test gaps Four findings from the isolated adversarial review. The namespacedByDir rule had reached three copies -- install-engine, surface, and the new trigger resolver -- one of which carried a hand-written keep-in-sync comment and no assertion. That is this repo's generative-fix-divergence class. Extracted to one exported predicate all three now call. Verified by diverging one copy deliberately: the existing #816 parity test failed, and passes again on revert. The omission test was vacuous. Row 16 asserted that a descriptor without triggerPrecedence still validates, but built its fixture from claude's shipped descriptor -- which this PR had just added the axis to. It now clones and deletes the key, following the shippedDescriptorWithout pattern, and asserts both that validation passes and that the resolver still picks the right winner from the default. The second half is what makes it prove anything. resolveTriggerSurface silently dropped an unrecognized scope while every sibling in this epic throws. Two phases of one epic should not disagree about whether an invalid scope is an error, so it now rejects through the same shared validator; an empty scope list still returns empty rather than throwing. The ADR amendment had been spliced into the middle of the References list, orphaning its last bullet. Moved to the top, after the header block, which is where ADR-3660 and ADR-1016 both put dated amendments. No lint checks markdown structure, so this was green while malformed. * fix(#2871): single-source the command filename composition too The earlier fix shared the namespacedByDir boolean but left the filename composition around it written twice -- once in _copyStaged as what actually gets written, once in resolveTriggerSurface as what gets predicted. The predictor could go stale silently. One exported helper now composes it for both. The entry.name asymmetry that looked like it would block extraction does not: entry.name is filtered to end in .md and stem is entry.name minus those three characters, so the two branches are the same string by construction. Divergence proven to fail: injecting a marker into the helper broke the trigger-surface suite; reverting restored 25/25. The four sibling layout suites hold at 227 unchanged. * docs(#2871): correct the ADR timing notes that this phase makes stale The Amended by back-links on ADR-3660 and ADR-1016 were written in Phase 0, when the widenings they describe had not shipped. Each carried a forward-looking clause -- "the module changes at Phase 2, not before, until then this module resolves placement only" -- which becomes false the moment this PR merges. ADR-2866's own Amends header and its reciprocal-notes section carried the same tense. All four now describe what shipped. This is a tense and status correction on Accepted ADRs, not a change to any decision. Worth stating because it is the failure mode this epic keeps meeting: gen-adr-index.cjs tracks only Supersedes and Subsumes, so nothing in CI would have caught either the missing back-link in Phase 0 or these stale clauses now. They stay correct only because someone checks. * chore(#2871): backfill changeset PR number --------- Co-authored-by: sim <sim@local>	2026-08-09 22:25:42 -04:00
Tom Boucher	b901d1e06f	feat(#1953): complexity-triggered refactor extension point (execute:post) (#3261) ... * test(#1953): failing-first suite for the complexity-triggered refactor hook 60 behavioral cases against src/complexity-trigger.cts, which does not exist yet: decision-point counting, the comment/literal stripping leak surface, threshold and jump-delta boundaries at limit-1/limit/limit+1, stable-anchor baseline semantics, and fs fault injection via mock.method. Two fast-check properties assert that stripping never manufactures a decision point and that comments and string literals are score-neutral. Also registers the refactor-trigger capability manifest (inert until refactor.trigger_enabled) and regenerates the capability registry and matrix. Verified RED on the remote runner before any implementation exists. * feat(#1953): complexity-triggered refactor extension point Adds the opt-in refactor-trigger capability. After a phase executes, an execute:post step measures per-function complexity for the files the phase touched and writes a scoped refactor proposal when a function crosses the configured threshold or drifts past its recorded anchor. Design notes worth carrying: - The signal is computed in-core (decision-point counting over comment- and literal-stripped source, Node builtins only) rather than via Memtrace or a shelled-out analyzer. The hook fires as a deterministic CLI, not an agent with MCP tools, and core takes no external dependencies — this is the only option a behavioral test can bind to. The metric sits behind a seam. - The baseline is a stable anchor, not a rolling value: set on first observation, moved only on disposition. A rolling baseline makes the delta the single-phase change, so a function creeping +2 per phase never trips a delta of 5 and the jump check adds nothing over the absolute threshold. - Strict mode records an open deviation window in the broken-windows ledger rather than declaring its own ship:pre gate. ship.md has no generic ship:pre gate dispatch — only two hardcoded branches — so a third gate of any kind would be declared and never evaluated. - The gate clears on the proposal being dispositioned, never on the score improving. A blocking complexity number is one an executor can satisfy by splitting a coherent function in two. execute-phase.md gains a generic execute:post step-dispatch contract; it previously matched only ref.skill == "code-review", so any other step registered there was declared and never run. The code-review branch is unchanged. Full rationale in ADR-1953. Closes #1953 * fix(#1953): close git option injection and symlink escape in the refactor hook Three findings from the isolated security review, all fixed inline. HIGH — changedFilesSince interpolated the --since value into a revision token placed before the -- separator. A -- only stops PATHSPEC parsing of arguments after it; git still option-parses what comes before. So --since '--output=/tmp/x' became --output=/tmp/x..HEAD, which git accepts as --output=<file> and uses to redirect diff output — an arbitrary write. Fixed with --end-of-options before the revision range plus a conservative ref validator. The validator deliberately permits ~ ^ @ { } because those are legitimate git REVISION syntax (HEAD~1, main@{yesterday}) as distinct from ref-NAME syntax; --end-of-options is the actual barrier. The doc comment asserting the trailing -- was sufficient was wrong and is corrected. MEDIUM — resolveConfinedPath confined by string prefix only, so a symlink committed inside the repo passed the check (its own path is under cwd) and readFileSync then followed it outside the root. Now lstat-checks for a regular file and skips anything else with REFACTOR_FILE_UNREADABLE, so one bad path skips one file and the run continues. LOW — the new execute:post dispatch contract showed the gsd_run example before the rule requiring ref.command be validated first. That prose is executed by an agent, so textual order is execution order. Reordered. Refs #1953 * fix(#1953): make the analyzer able to see TypeScript at all Found by running the shipped analyzer over its own source: it reported functions=1 for a 940-line module with 24 function forms. A return-type annotation or a generic parameter list made a function invisible — `function f(a): number {}` and `function f<T>(a: T): T {}` both detected as zero. Since gsd-core is written in .cts and the capability declares .ts/.cts/.mts analyzable, the feature silently found nothing in this repo's own primary language while reporting success. A safety net that reports "all clear" because it cannot see is worse than no safety net. All 98 tests passed over this, because every fixture was plain JS — the exact failure the test matrix's own "assert against the shape production uses" warning describes. Adds a TypeScript-shapes suite covering return types (including unions, generics, object literals and type predicates), generic parameter lists (constrained and defaulted), export/async/generator combinations, annotated arrows, class-method modifiers, and optional/ default/rest params — plus the two traps: an overload signature has no body and must not count, and `a < b && c > d` is a comparison, not a generic. Detection now reports 24/37/21 functions for the three source files, which matches a hand count exactly. Also from review: - The strict-mode ledger dedup identified entries by parsing a prose description string. That is banned by CONTRIBUTING's raw-text-matching rule and was a real bug: the "exactly one window per untriaged proposal" guarantee rested on prose matching, so rewording a description or editing WINDOWS.md by hand silently produced duplicates. Now matches structurally on kind + phase + file + line. - A property test asserted on the stripper's output text. Reframed to assert the same invariant through analyzeSource's score. - nextBaseline's `candidates` parameter has been dead since the anchor change; removed from the signature and all call sites. - Extracted the duplicated require-or-degrade and capability-check boilerplate. - ADR-1953's Implementation bullet still named a `refactor.ship-gate` in check-command-router.cts — a leftover from the design cut D6 rejects. That file is untouched and no such gate exists. Removed. Refs #1953 * fix(#1953): keep execute-phase.md under its byte ceiling; un-vacuum the large-file test Five of the seven remote-runner failures were one cause: the execute:post dispatch contract, written out inline, grew execute-phase.md 1876 bytes (93,400 -> 95,276) against a frozen PRE_PHASE6 ceiling of 93,600. A drift-ack does not clear that — tests/phase6-capstone-conformance.test.cjs and tests/fix-2285-claude-orchestration-wiring.test.cjs assert the file is literally under the cap. The contract now lives in gsd-core/references/loop-hook-dispatch.md, which already claimed to be the point-agnostic dispatch reference and already documented ref.skill and ref.agent. It gains the ref.command shape, its in-context validation rule, the advisory-by-construction statement, and a note that a point whose workflow hand-rolls one kind is not implementing this contract. execute-phase.md now defers to it in one line: 145 bytes of growth, 55 B of headroom under the cap. Better placement than the first cut — the reference was overstating its coverage, and this makes the claim true rather than duplicating prose next to it. Acknowledged by appending to tests/emitted-drift-acks/2930-*.json rather than a new 1953-*.json: two ack sources may never name the same path, and that fragment is already the accumulating ack for this file. Sixth and seventh failures: analyzesLargeFileWithinBounds tripped its own vacuity guard — the fixture generated ~480 KB against a `> 500000` assert, so the guard fired and the three assertions after it never ran. The test has been vacuous since it was written. The matrix row specifies ~1 MB, so N goes 8000 -> 20000 (1.17 MB, 17% margin) and the guard to > 1_000_000. Verified by reproducing the exact body against the compiled module: 1168888 bytes, 118 ms, all four assertions hold. Refs #1953 * fix(#1953): fold the execute:post step deferral into the existing resolve line The remaining two failures were one test: execute-phase.md carries a SECOND, tighter assertion than the 93,600 ceiling — `<=93400`, which is exactly its current size. The file cannot grow by a single byte. My previous fix got it under 93,600 but not under 93,400, so it still failed. ("H." in the report is just the parent describe of that same test, not a separate defect.) Rather than add a paragraph, the deferral now REPLACES the existing hook resolution line. It read: Resolve active step hooks from `EXECUTE_POST_HOOKS_JSON` where `kind == "step"` and `ref.skill == "code-review"`. which is the bug itself written down — only code-review was ever dispatched. It now reads: Dispatch each `kind == "step"` hook per @gsd-core/references/loop-hook-dispatch.md. For `code-review`: The following prose already begins "If no active code-review step hook exists", so it reads correctly and the code-review handling is untouched. Net effect on the file is -11 bytes: 93,400 -> 93,389, under the margin assertion rather than merely under the ceiling. That also removes the need for a drift-ack: the file shrank, so there is no growth to acknowledge, and the append to the shared 2930-*.json fragment is reverted. Leaving it would have shipped a claim of "145 bytes of growth" that is no longer true, on a file six other issues share. The test's own comment states the principle this ended up honoring: "the host loop must stay small — optional-feature detail belongs in the capability fragment, not the host workflow." Putting the dispatch contract in the reference rather than inline is that rule, applied. Refs #1953 * fix(#1953): keep the code-review hook literal the workflow test requires tests/code-review.test.cjs extracts the <step name="code_review_gate"> block and asserts it contains `ref.skill == "code-review"` verbatim. The previous commit replaced the line carrying that literal, so the token vanished and the test went red — a fair assertion: code-review IS the bespoke branch there and the workflow should still name it. Restored inside the same one-line deferral, which now reads: Dispatch `kind == "step"` hooks per @gsd-core/references/loop-hook-dispatch.md. `ref.skill == "code-review"`: 93,396 bytes — still under the `<=93400` margin assertion and 4 bytes below the base, so the file continues to shrink rather than grow. Because three consecutive runs were each reddened by a different assertion on this one file, this change was verified by sweeping ALL of them at once rather than one run at a time: every test under tests/ that reads execute-phase.md or references/loop-hook-dispatch.md was located by resolving its path constants, and each content/size assertion was evaluated directly against the working tree — 22 assertions, plus two real executions (gen-section-manifest --check, and emitted-attribution's full real-tree differential). All pass. That sweep also confirms the earlier judgement call: the net change to execute-phase.md is a SHRINK, and the size ratchet only gates growth, so reverting the append to the shared 2930-*.json ack fragment was correct — an ack would have been both unnecessary and factually wrong. Refs #1953 * chore(#1953): backfill changeset pr number to 3261 * docs(#1953): add the missing how-to for acting on a refactor proposal Reference and explanation shipped (COMMANDS.md, CONFIGURATION.md, FEATURES.md 159, ADR-1953) but the Diataxis how-to quadrant did not, and that is the one a user reaches for. CONTRIBUTING's required-docs table is 'new command -> COMMANDS.md + FEATURES.md', so CI was green on a gap. Enabling this feature is genuinely multi-step and no single page walked it: turn it on, tune the threshold, understand advisory vs strict, discover that strict needs a SECOND toggle on a DIFFERENT capability, and know what to do when a proposal appears. The two-toggle subtlety in particular was a footnote in a config table; here it is a section with both commands. Follows the shape of its closest siblings, resolve-edge-coverage-findings and resolve-prohibition-findings — both 'the loop surfaced a finding, here is what to do with it'. Includes a reason-code table for the silent cases, since the analyzer is deliberately quiet in six situations and a user who expected a proposal needs to tell 'nothing to report' from 'could not look'. Indexed from docs/README.md beside the other loop how-tos. Docs-only: exempt from the push gate, no re-verification, pass marker on 2af188b4 untouched. Refs #1953 * feat(#1953): warn when strict mode is on but nothing will actually block Closes acceptance criterion 5, which I had wrongly marked satisfied. refactor.trigger_strict records an untriaged proposal as an open deviation window, but a ship only STOPS if workflow.windows_enforce is also on — a toggle owned by the broken-windows capability that this feature neither sets nor requires. So a user could enable strict, believe ship was gated, and find out otherwise at ship time. The split itself stays: requires:["broken-windows"] would force-install the ledger on advisory users who never enable strict, and a ship:pre gate of our own would never fire because ship.md has no generic ship:pre gate dispatch. What was missing was discoverability, so that is what this fixes. `refactor evaluate` now emits a typed REFACTOR_STRICT_NOT_ENFORCING warning, naming the exact remediation command, whenever strict is on and either workflow.windows_enforce is off or broken-windows is unavailable. It fires only on a run that produced a candidate — with nothing to block on there is nothing to warn about, and warning every run would be noise. Reads workflow.windows_enforce through the same resolveConfigKey walk the router already uses for its own keys rather than a second config reader. Four tests cover the matrix: strict+enforce-off warns, strict+enforce-on does not, strict+ledger-absent warns, strict-off never warns. Also corrects a user-facing message in this same file that told the user to run `gsd-tools config-set` — the wrong form. docs/CONFIGURATION.md and the broken-windows capability both use `gsd config-set`, and gsd-tools is invoked as `node gsd-tools.cjs`, so the bare form may not resolve. The two adjacent messages in this file now agree. Refs #1953 --------- Co-authored-by: sim <sim@local>	2026-08-09 19:52:47 -04:00
Tom Boucher	653f95e39f	chore(#2801): remove the hostBehaviors.reviewerCli deprecated alias (#3272) ... * test(#2801): failing-first suite for the hostBehaviors.reviewerCli alias removal Inverts the Phase 5a rows that assert the derived legacy alias still contributes a reviewer slug, and adds the removal-warning coverage the alias's exit needs (ADR-2782 D9). RED against unmodified production code, by design: the six shipped manifests still declare the key and collectReviewerWarnings emits nothing for hostBehaviors. Refs #2801 * chore(#2801): remove the hostBehaviors.reviewerCli deprecated alias ADR-2782 D9, Phase 7 — the final phase of epic #2782. The derived legacy alias survived one release (Phase 5a shipped in 1.9.0; 1.9.1 and 1.10.0 have since gone out), so it goes. A declared reviewer body is now the only route onto the reviewer roster. - deriveReviewerSlugs no longer reads runtime.hostBehaviors.reviewerCli - the key is stripped from the six manifests that carried it; each already declares a reviewer body whose slug equals its capability id, so the derived roster is unchanged at the same twelve slugs - collectReviewerWarnings emits a presence-based, non-fatal removal notice for any manifest still declaring the key, reaching both the build-time registry generation and the third-party overlay load path. The check runs before the reviewer-body early-return, because the manifest it exists for is the alias-only one that has no body. - hostBehaviors stays an open, unvalidated bag for its other 59 keys; this adds one keyed removal notice, not general validation Refs #2801 * refactor(#2801): give the reviewer-warning channel a typed IR Review finding: the new tests asserted with String#includes() on the warning prose, which CONTRIBUTING.md's 'Prohibited: Raw Text Matching on Test Outputs' bans in favor of a typed intermediate representation. Adds the IR beside the renderer rather than replacing it, which is the shape that section prescribes and bin/verify-reapply-patches.cjs already models: - REVIEWER_WARNING, a frozen code enum - REMOVED_REVIEWER_CLI_FIELD, so the emitting site and its test share one symbol instead of duplicating a literal - collectReviewerWarningRecords(cap), returning typed records collectReviewerWarnings(cap) keeps its exact string[] contract as a thin map over the records, so both production consumers are untouched. Every section-K row now asserts on record.code/field/capId and none on the rendered message. Locks the code surface, asserts the renderer stays one-to-one with the records, and migrates the pre-existing Phase 2 test on the same channel off prose matching. Refs #2801 * test(#2801): invert the section F alias fall-through regression row Caught by the remote runner: 2 unique failures on both Node lanes out of 31,692. tests/reviewer-lane-declarations.test.cjs section F — Phase 5a's isolated-security-review regressions — asserted that a blank reviewer.slug falls through to the hostBehaviors.reviewerCli alias rather than dropping the lane. That is the direct inverse of this phase's contract. The original rationale held only while the alias existed. With it gone there is nothing to fall through to: a blank body is not a declaration, and a declaration is the only route onto the roster. Inverted rather than deleted — the row carries the adversarial-review provenance for the slug trim, and removing a security regression guard to make a change pass is backwards. The duplicate row added earlier in section C is dropped instead; section F is its canonical home. Also corrects two count strings Phase 5b left at eleven while asserting twelve, which would misreport on failure. Refs #2801 * docs(#2801): give the removed reviewerCli flag a migration path The Reference edit alone satisfied CI — a file under docs/ moved, so lint-docs-required.cjs was green — while the task-oriented quadrant said nothing about the removal. A maintainer whose lane had just gone silent would have found the field documented as removed and no page telling them what to do about it. Adds a migration section to the how-to: the symptom, the verbatim warning they will see, the before/after manifest, and the note to keep the reviewer slug equal to the capability id so existing review.default_reviewers entries and --<slug> flags survive. Refs #2801 * chore(#2801): backfill changeset pr number to 3272 * feat(#2801): close the runtime.hostBehaviors vocabulary ADR-1016 closes twelve descriptor axes and rejects an open escape hatch in the descriptor. It never mentioned runtime.hostBehaviors, and that silence was read as permission: 59 keys across 18 manifests, 39 of them set by a single capability, validated by nothing. The reference docs went further and attributed the open seam to ADR-1016, which does not mention the field at all. KNOWN_HOST_BEHAVIORS enumerates the vocabulary. An undeclared key yields a non-fatal UNKNOWN_HOST_BEHAVIOR record on the same D4.3 channel as the alias removal notice, reaching both build-time generation and overlay install. Warning, never error, for the reason this phase exists: an error would hard-break an out-of-tree descriptor carrying a bespoke key with no deprecation window, which is what reviewerCli was given a release to avoid. Escalation is a separate decision. reviewerCli is excluded from the unknown-key sweep so it keeps its own notice with the migration pointer rather than drawing two records. A parity test binds the vocabulary to the shipped manifests in both directions, and a second asserts no shipped capability draws a notice, so the closure is provably inert in-tree. Records the decision and the miscitation as an ADR-1016 amendment. Refs #2801 * fix(#2801): bound and sanitize the unknown-key diagnostics Two findings from an isolated adversarial review of the closure commit, both proven by execution rather than asserted. MAJOR, introduced by the closure: the new Object.keys(hostBehaviors) sweep had no ceiling. An installed third-party manifest is bounded only by MANIFEST_MAX_BYTES, and an 8.69MB manifest with 800,000 keys produced 800,000 records and ~139MB of message text, retained for the registry's lifetime in OverlayMeta.diagnostics. Now capped at ten records plus a summary carrying omittedCount, mirroring capability-loader's existing slice(0,3) idiom. The same manifest now yields 11 records and 1748 chars. MINOR, newly reachable: manifest-supplied key names were interpolated raw. Unlike cap.id, which validateCapability gates on KEBAB_RE before these diagnostics run, hostBehaviors keys have no grammar check anywhere, so ANSI escapes and CRLF reached stderr and OverlayMeta.warnings intact. New describeKey replaces C0/C1 controls and clips at 80 chars. The file already had describeValue for this and applied it only to values. Both fixes land on the pre-existing reviewer.* sweep too — it carried the identical pair, and fixing only the new copy would leave the same defect one screen from its own fix. Refs #2801 --------- Co-authored-by: sim <sim@local>	2026-08-09 19:18:56 -04:00
github-actions[bot]	4f2a4932ce	chore: sync next package version to 1.10.0	2026-08-08 05:07:32 +00:00
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>	2026-08-07 13:41:21 -04:00
Dennis Kim	178ec00040	fix(#2787): clarify broken-windows ship blocking enforcement (#2814) ... * fix(#2787): clarify broken-windows ship blocking enforcement * fix(#2787): update renderLedger header to clarify opt-in enforcement * fix(#2787): address maintainer scope and wording review --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-03 12:39:16 -04:00
𝚌𝚕𝚎𝚣𝚌𝚘𝚍𝚒𝚗𝚐	88f6d9bd1b	fix(#2644): deduplicate Cursor slash menu (#2812) ... * fix(#2644): deduplicate Cursor slash menu * fix: preserve installer executable mode * chore: add changeset for PR #2812 * test(#2644): acknowledge Cursor emission changes * test(#2644): drop spent emitted drift acknowledgments * fix(#2644): remove retired Cursor command converter --------- Co-authored-by: clezcoding <clezcoding@users.noreply.github.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-03 12:05:45 -04:00
Tom Boucher	07de60523c	fix(#2657): untrack the nine ADR-457 migration-gap compiled artifacts (#3011) ... * test(#2657): add failing-first regression for tracked bin/lib compiled artifacts Nine gsd-core/bin/lib/*.cjs artifacts are tracked in git despite having src/*.cts sources, violating ADR-457's build-at-publish contract. This regression test asserts the ADR-457 end state (untracked, gitignored, empty-set reported by the #2656 sync guard) and fails until the tracking is fixed. * fix(#2657): untrack the nine ADR-457 migration-gap compiled artifacts Nine gsd-core/bin/lib/*.cjs artifacts (api-coverage, assumption-delta, claude-orchestration, claude-orchestration-command-router, external-job, markdown-table, runtime-artifact-install-plan, state-transition, write-set) were tracked in git despite each having a matching src/*.cts source, letting the committed bytes drift silently from source (#2653 demonstrated this for api-coverage.cjs). Seven had no .gitignore entry at all; two (markdown-table.cjs, write-set.cjs) had a pattern added by #2248 but were never git rm --cached. Both gaps produce the same tracked-file symptom. Untracks all nine and adds the seven missing .gitignore entries next to their two siblings, reaching ADR-457's end state: bin/lib/*.cjs is a gitignored build artifact built via prepare/pretest/prepublishOnly, never checked-in source of truth. The #2656 artifact-sync guard is regime-agnostic by design and needed no code change; it now reports the empty-set end state. * test(#2657): consolidate repeated still-tracked/unmatched assertion shape Code-review finding (Standards axis, Duplicated Code): the three 'none of the nine should still be in bad state X' checks shared an identical filter-then-assert-empty shape. Extracted assertNoneStillBad() as a shared helper; behavior is unchanged. * fix(#2657): make the .gitignore-match assertion existence-independent The regression test's check-ignore assertion used --no-index, which locally exercises the pattern correctly but was reported failing on gsd-test's fresh shallow clone. Switched to plain 'git check-ignore -q' (no --no-index): verified via a real git worktree checkout at both origin/next (fails: all nine report not-ignored, since check-ignore correctly special-cases the still-tracked pre-fix state) and this branch's tip (passes: all nine report ignored). Plain check-ignore is also semantically stronger than --no-index here, since it honors the 'a tracked path is never reported ignored' rule that --no-index bypasses -- exactly the property under test for the two paths whose .gitignore pattern predates this fix (#2248) but were never untracked. Also reconciled the .gitignore comment: it previously read 'these seven' beside seven new lines with no indication of the other two (of nine total) that already had a pattern from #2248. Annotated both groups so the count is unambiguous at the point of the diff. * test(#2657): make every git invocation self-diagnosing b6f915bc0 failed in the runner with a shape that turned out not to be about .gitignore content or the merge: two of the five failures in this file were 'Command failed' / 'Got unwanted exception' -- git itself erroring, not answering. The old code used execFileSync + try/catch, which conflates 'git said no' with 'git could not run' -- both looked like the same negative result to the test, exactly the failure mode that produced 'unmatched: <all nine>' twice on two different assertions for two different reasons. Switched every git invocation to spawnSync (never throws) and made every assertion check the exit status explicitly before interpreting output: - git ls-files: must exit 0, or the assertion fails loud with cwd, exit status, and stderr instead of silently reading an error as 'nothing tracked'. - git check-ignore -q: only exit 0 (ignored) and exit 1 (not ignored) are legitimate answers per check-ignore(1); any other status is now a thrown infrastructure failure, never read as 'not ignored'. - trackedCompiledArtifacts(): a thrown error is now reported as what it is (its internal git call failed), not swallowed into a 'still tracked' verdict. - the sync-guard subprocess check now reports cwd/stderr/stdout on a non-zero exit instead of a bare doesNotThrow. This is a genuine, independent test defect (a test that reads a failed command's empty output as a meaningful answer can pass or fail for the wrong reason) as well as the mechanism for finally surfacing why b6f915bc0 failed in the runner: the next run's assertion messages will show the resolved cwd and git's actual stderr instead of an opaque 'unmatched: <all nine>'. * fix(#2657): trust the repo root for git calls under dubious-ownership Root cause of the failing runner verdict, harvested from the diagnostics commit: every git invocation in the container exits 128 with 'fatal: detected dubious ownership in repository at /work' -- the checkout there is owned by a different uid than the process running the tests, and git refuses to operate at all. The old assertions read that hard failure as 'not ignored' / 'still tracked', producing the all-nine symptom seen on both b6f915bc0 (plain check-ignore) and 34052f836 (--no-index). Nothing was ever wrong with the untracking, the .gitignore content, or the merge -- confirmed by exhaustive local reproduction (git worktree, real shallow clone, the runner's exact clone+checkout+merge sequence from its own Go source) that could never surface the bug because this machine owns its own checkouts. Fixed at both git() call sites in this exact seam by passing '-c safe.directory=<repo root>' per-invocation (never written to any config file, so trust is scoped to the single call): - tests/fix-2657-untrack-compiled-artifacts.test.cjs - scripts/lint-compiled-artifact-sync.cjs -- a SHIPPED script with the identical defect (its own git ls-files failed the same way in the same run), which would fail identically for any containerized CI lane whose checkout uid differs from the running user, not just this branch. Folded in under the no-defer rule rather than filed separately, since it sits in the exact tracked-compiled-artifact guard this issue is about. The status-code guards added in 31858818e stay in place -- they are what turned an unexplainable 'unmatched: <all nine>' into a one-line diagnosis, and they must keep any future infrastructure fault from silently reading as a substantive result. * chore(#2657): backfill changeset PR number to 3011 * chore(#2657): backfill changeset PR number to 3011 --------- Co-authored-by: sim <sim@local>	2026-08-02 21:22:43 -04:00
Tom Boucher	51f32d2d40	fix(#2658): detect trae runtime and resolve its instruction file to a concrete rules file (#3006) ... * test(#2658): add failing-first regression for trae runtime detection and instruction path Covers all three collided defects reported in #2658 plus a fourth instance of defect 1 (ingest-docs.md) found while diagnosing it: missing trae detection in workflow runtime-detection blocks, the CLAUDE.md path-mutilation bug in both the js/cjs and md install-time converters, and the missing projectInstructionFile capability declaration. Fails against current source; the next commit fixes it. * fix(#2658): detect trae runtime and resolve its instruction file to a concrete rules file Three defects collided to produce the reported ".claude/.trae/rules/" path: 1. new-project.md and ingest-docs.md's runtime-detection blocks only recognized codex/gemini/opencode and fell through to RUNTIME=claude for trae. Both now recognize the /.trae/ execution-context path and the TRAE_CONFIG_DIR env var before the claude fallback. 2. RUNTIME_CONTENT_DISPATCH.trae.js (bin/install.js) replaced bare "CLAUDE.md" before the ".claude/" prefix was handled, mutilating ".claude/CLAUDE.md" into ".claude/.trae/rules/". Now replaces the full ".claude/CLAUDE.md" path first, and targets a concrete file. 3. convertClaudeToTraeMarkdown (mirrored in bin/install.js and src/runtime-artifact-conversion.cts per the #2094 output-parity test) had the same class of bug with a different wrong output (".trae/.trae/rules/", from its generic ".claude/" rewrite firing after the bare CLAUDE.md rewrite). Both mirrors now match full-path forms before the bare/generic patterns, converging on the same concrete file as the js/cjs converter. 4. capabilities/trae/capability.json didn't declare hostBehaviors.projectInstructionFile, so getProjectInstructionFile fell through to the generic AGENTS.md default even when RUNTIME=trae was resolved correctly. Now declares ".trae/rules/rules.md", regenerated into gsd-core/bin/lib/capability-registry.cjs via npm run gen:capability-registry. Closes #2658 * chore(#2658): add changeset * fix(#2658): preserve arbitrary runtime-dir prefixes in the trae path rewrite Found by the end-to-end --trae install regression test (not by static trace) across two verification runs: 1. copyWithPathReplacement runs a generic ~/.claude/, $HOME/.claude/, and ./.claude/ -> runtime-dir rewrite on every .md file BEFORE calling convertClaudeToTraeMarkdown. The prior fix's .claude/CLAUDE.md-specific patterns never fire on that already-rewritten text, and the bare fallback still doubled whatever prefix the generic pass substituted. A first attempt handled only the fixed "./.trae/" shape and missed the $HOME/.claude/ and ~/.claude/ forms gsd-core/workflows/profile-user.md actually uses, which post-rewrite become an arbitrary absolute local-install-root path, not the fixed relative shape. Fixed with a prefix-preserving pattern that captures whatever precedes a ".trae/" tail and fixes only the filename suffix, instead of assuming one fixed shape. 2. The fix's own explanatory comments literally spelled out the malformed strings and the instruction filename as contiguous text. Since these two files ship verbatim into local --trae installs, where they are themselves run through the same find/replace, the comments got "fixed" right along with the real code, leaking the malformed string into the installed tree. Rewrote every comment in both mirror copies to never spell either the instruction filename or a malformed shape as one contiguous token. Adds an emitted-drift-ack fragment: the corrected replacement target for every CLAUDE.md mention (bare directory -> concrete file) changes trae-emitted output for every repo file that mentions CLAUDE.md, not only the ones that hit the originally reported bug. * test(#2658): extend parity test with arbitrary-prefix .trae/ inputs The bin/install.js vs runtime-artifact-conversion.cjs parity assertion for convertClaudeToTraeMarkdown only fed the pre-existing bare .claude/CLAUDE.md input through both implementations. Feed the prefix-preserving cases (relative, nested-absolute, tilde, $HOME, backtick-wrapped) plus a property-based check through both, so a future edit to only one copy of the .trae/-tail regex fails this test instead of silently diverging. * chore(#2658): backfill changeset PR number to 3006 --------- Co-authored-by: sim <sim@local>	2026-08-02 18:29:47 -04:00
kyle-the-dev	ce38d44811	fix(#2777): remove stale codex local home metadata (#2831) ... * fix(#2777): remove stale codex local home metadata * chore(#2777): add changeset for codex local layout metadata --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-02 01:00:52 -04:00
github-actions[bot]	854c93533c	chore: sync next package version to 1.9.1	2026-07-31 13:12:09 +00:00
github-actions[bot]	4232a79396	chore: sync next package version to 1.9.0	2026-07-31 03:14:04 +00:00
0xdhx	82ca13f5a5	fix(#2736): write current_phase_name from the transition intent, not the lossy prose round-trip (#2821) ... * fix(#2736): intent-first current_phase_name on transitions; dash-first prose precedence Primary: completePhase (adapter) and beginPhase (via readModifyWriteStateMd options) pass the intent-held display name to syncStateFrontmatter as an authoritative override, applied after every derive/preserve/carry-forward step — so the lossy prose round-trip can never destroy a name the transition just resolved. Names containing a parenthetical (`Closer-ruling measurement (D1a)`) now land in frontmatter verbatim instead of collapsing to the parenthetical (`D1a`). Secondary (#1695 AC #3 residual): parsePhaseFromProse prefers the em-dash name when it is a genuine name (not a status keyword, not a `Milestone:` tail), else falls back to the parenthetical — satisfying both first-party writer shapes (`N — Name (aside)` and `N (Name) — EXECUTING`). Still lossy for paren-containing names, which is why the intent-first override is the primary fix. plannedPhase carries no name in its intent, so it is naturally out of scope. Fixes #2736 * docs(changeset): backfill PR number for #2736 fragment * fix(#2736): drop an unnecessary type assertion on result.data StateTransitionResult.data is already `Record<string, unknown> | undefined`, so the cast was a no-op and tripped @typescript-eslint/no-unnecessary-type- assertion (CI lint-tests red on the first push; every test lane was green). --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-07-30 13:53:39 -04:00