372 Commits

Author	SHA1	Message	Date
Tom Boucher	2f4f7538e9	fix(#4264): wire both TDD dispatch backends to phase.tdd-applicable (#4284)	2026-09-04 16:08:42 -04:00
Tom Boucher	580059251a	fix(#4040): route partially-created .planning to initialization recovery (#4283) ... * test(#4040): add failing-first regression tests for partial-init routing Red: init.progress/init.resume/init.new-project payloads carry no partial-init discriminator, and progress.md/resume-project.md/ new-project.md route an interrupted bootstrap (.planning/PROJECT.md + config.json only) to Route F / STATE reconstruction / a hard error. * fix(#4040): route partially-created .planning to initialization recovery A bootstrap interrupted after .planning/PROJECT.md (but before REQUIREMENTS.md/ROADMAP.md/STATE.md) was mis-routed three ways: progress.md read it as between-milestones (Route F) or 'no planning structure', resume-project.md offered STATE.md reconstruction, and new-project.md errored 'already initialized' — a routing loop with no recovery exit. Add a shared buildInitCompletenessFields discriminator (planning_exists / requirements_exists / milestones_exists / init_incomplete) to the init.progress, init.resume and init.new-project payloads, and branch on init_incomplete in progress.md, resume-project.md and new-project.md BEFORE the legacy branches. MILESTONES.md presence excludes the archival between-milestones state, so Route F and the STATE-reconstruction path keep working. Emitted-Drift-Ack-Growth: progress.md — deliberate #4040 growth: new init_incomplete recovery branch (routing text + guard on the no-planning and Route F branches) added ahead of the legacy init_context routes. Emitted-Drift-Ack-Growth: resume-project.md — deliberate #4040 growth: new init_incomplete branch routing an interrupted bootstrap to initialization recovery before the STATE.md-reconstruction branch. Emitted-Drift-Ack-Growth: new-project.md — deliberate #4040 growth: project_exists gate split on init_incomplete so a partial bootstrap resumes initialization instead of erroring. * chore(#4040): add changeset fragment * chore(#4040): backfill PR number in changeset fragment --------- Co-authored-by: sim <sim@local>	2026-09-04 16:03:42 -04:00
Tom Boucher	97ce61dee2	fix(#3990): state the RED/GREEN/REFACTOR cycle once, embed tdd.md conditionally (#4228) ... * test(#3990): the RED/GREEN/REFACTOR cycle is stated once, embedded conditionally * fix(#3990): state the cycle once — pointers in consumers, conditional tdd.md embeds Emitted-Drift-Ack-Growth: execute-phase.md — #3990 conditions the tdd.md embed on the dispatch being TDD * chore(#3990): changeset for the single-statement TDD cycle * chore(#3990): backfill changeset pr number * fix(#4228): linear cycle check — the lazy-span regex pinned a Windows core for the whole job cap * test(#3990): allowlist pin tracks the rebased line * fix(tests): npm-integrity gate names an empty audit output explicitly — empty stdout crashed the parse as a bare SyntaxError * fix: name an empty npm-audit stdout explicitly — it crashed the parse as a bare SyntaxError Observed on CI (several branches, all lanes): spawnSync npm ETIMEDOUT with empty stdout; the empty string survived the recovery path and surfaced as 'SyntaxError: Unexpected end of JSON input', hiding the captured error. The recovery path now requires non-empty stdout, and an empty result throws with the captured stdout/stderr/message so the actual error is on the record. Root cause of the ETIMEDOUT itself is NOT diagnosed here — this change only stops masking it. --------- Co-authored-by: sim <sim@local>	2026-09-03 21:41:14 -04:00
Tom Boucher	515191f07d	feat(#3677): quick-batch hardening and acceptance (#4240) ... * chore(#3677): checkpoint design artifacts (gitignored, dev-only) * test(#3677): add failing regression test for the crash-window duplicate-dispatch gap (RED) Independently re-traces resume-mode.md/planner-wave.md/worktree-dispatch.md/ merge-wave.md and src/quick-batch.cts's resumeBatch (lines 894-899) and confirms the prior research pass's Open Question 1: a coordinator crash between Step 6 (executor commits, SUMMARY.md written) and Step 7 (merge) leaves BATCH.json at "pending" with no STATE.md row yet (only written in Step 9), so --resume's eligibility re-derivation would dispatch a second executor into a new worktree for the same item, orphaning the first. This test asserts worktree-dispatch.md's Step 6 excludes an item whose SUMMARY.md already exists from the spawn set, mirroring planner-wave.md's existing PLAN.md-existence check one layer earlier. Fails against the current worktree-dispatch.md, which has no such guard. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §1 for the full trace and fix-location rationale. * fix(#3677): guard worktree-dispatch.md against re-dispatching an already-executed item (GREEN) worktree-dispatch.md's Step 6 re-derives eligibility every dispatch round via the same quick-batch resume call resume-mode.md uses, but had no check for "did this item already finish executing" the way planner-wave.md already checks "did this item already get planned" (PLAN.md existence) before re-planning. A coordinator crash between Step 6 (executor commits, SUMMARY.md written) and Step 7 (merge) left the item eligible for a second dispatch on --resume, orphaning the first worktree's real, already- committed work and silently losing it once the second executor's SUMMARY.md write clobbered the first at the same item_dir path. Adds a SUMMARY.md-existence exclusion before spawn-plan is computed, symmetric to planner-wave.md's PLAN.md check. The excluded item is not lost: merge-wave.md's own mergeable-wave criterion (status=pending, SUMMARY.md on disk, not yet merged) already picks it up independently of this eligible/spawn list. Workflow-prose-only fix — touches no already-merged/reviewed .cts module. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §1 for the fix-location rationale (why not resumeBatch itself). * test(#3677): add real-git coverage for worktree-ownership tampering, scope drift, and submodules Closes the three coverage gaps identified in 40-design.md §2/§3 (#3677, epic #3344 Phase 5's own AC bullets: "arbitrary-worktree ownership attempts", "scope drift", "submodules"): - Arbitrary-worktree ownership tampering: a manifest entry naming a non-agent branch is silently dropped at normalization before any git subprocess runs; a manifest entry naming a plausible agent-branch that was never actually created by this repo's own worktree.create (a genuinely foreign repo/branch) is blocked via base_mismatch. Both leave the foreign location and repoRoot's HEAD provably untouched. - Advisory scope drift: a committed path outside declared files_modified still merges successfully (advisory, never blocking) while surfacing a scope_out_of_declared warning naming the drifted path; an exact declared-scope match produces zero warnings (boundary case). - Real .gitmodules submodule integration: a repo containing a real local git submodule merges cleanly through executeWorktreeWaveCleanupPlan for an unrelated plan; a real gitlink pointer bump (declared) merges cleanly with the superproject tree reflecting the new pinned commit; an undeclared bump is advisory-only and surfaces a scope warning naming vendor/sub, same as any other undeclared modification. No src/*.cts changes — all three gaps were coverage-only; the underlying primitives already behaved correctly (independently verified against real git subprocess output before writing each assertion). * docs(#3677): document how to diagnose a preserved quick-batch worktree Extends the one-sentence "worktree is preserved (never deleted)" mention into a concrete diagnosis procedure: where the preserved directory is, how to read the executor's real commits/diff against the plan's declared files_modified, how to read the item's own SUMMARY.md independent of merge outcome, how to manually merge-and-clean-up or discard, and how to re-run --resume afterward. Also documents that a SUMMARY.md-written-but-still- pending item (the crash-window case fixed in this same PR) needs no manual intervention — --resume routes it straight to the merge step. * chore(#3677): checkpoint final acceptance-evidence mapping (gitignored, dev-only) * fix(#3677): make crash-window duplicate-dispatch guard behaviorally provable and durably recoverable Orthogonal review (Spec finding): the crash-window regression test added earlier this phase only asserted readStep('worktree-dispatch.md') + regex matches against the markdown prose — proving the DOCUMENTATION says the right thing, never that the runtime condition (pending status + on-disk SUMMARY.md + absent STATE row) is actually handled correctly. #3677's own "Alternatives considered" explicitly rejects "document recovery without fault injection" for exactly this reason. Extracts the filtering decision into a pure, independently testable function, filterAlreadyExecuted(eligibleIds, executedIds) in src/quick-batch-dispatch.cts, wired to a new `quick-batch filter-executed` CLI verb (src/quick-batch-command-router.cts) — the same pure-decision- then-CLI-wired pattern computeSpawnPlan/computeMergeOrder already establish. worktree-dispatch.md now calls this verb explicitly instead of only describing the decision in prose. A genuine fixture-based test in tests/quick-batch.test.cjs constructs a REAL BATCH.json (createBatch), writes a REAL SUMMARY.md on disk at the item's real item_dir, calls the REAL resumeBatch, and proves both that resumeBatch alone still reports the item eligible AND that filterAlreadyExecuted (fed a real filesystem check) correctly excludes it. The prior prose-assertion tests are kept — they now prove the workflow markdown is correctly WIRED to the verb — but are no longer the only proof. Self-discovered defect while building that fixture (fixed inline, not deferred): tracing merge-wave.md against /gsd:quick's own prior art (QUICK_WORKTREE_MANIFEST=$(mktemp ...), quick.md:415) showed $QUICK_BATCH_WORKTREE_MANIFEST is a fresh PER-PROCESS temp file. A resumed coordinator correctly does not re-dispatch an already-executed item (this fix), but nothing durably recorded that item's worktree_path/branch/base either — Step 7 in the resumed process would have had no data to build its cleanup-wave entry from. Adds dispatched_worktree/dispatched_branch/ dispatched_base to QuickBatchItem (src/quick-batch.cts) — deliberately NOT a reuse of the pre-existing `worktree` field, whose loadBatch validation requires the path to exist on disk (verified empirically: reusing it made the batch permanently unloadable the moment a legitimately-merged worktree was removed). worktree-dispatch.md persists the triple once a worktree is created; merge-wave.md falls back to it when the ephemeral manifest lacks an entry, clears it after a successful merge, and fails closed rather than guessing if no record exists anywhere. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §9.1 and §9.3 for the full trace, empirical verification notes, and rejected alternatives (reusing `worktree` directly). * test(#3677): prove the arbitrary-worktree-ownership boundary against two real sibling worktrees Orthogonal review (Security finding): the two existing ownership-tampering tests didn't test ownership — one was trivially rejected by WORKTREE_AGENT_BRANCH_RE's shape check before any git call (proves branch- NAME filtering, not ownership), the other pointed at a wholly separate, never-linked foreign repo, so merge-base failed immediately because the branch didn't exist as a ref at all. Neither exercised the real scenario: a manifest entry whose worktree_path/branch are swapped to point at a DIFFERENT, GENUINELY-REGISTERED sibling worktree of the SAME repoRoot, with a branch name passing the shape check and a base in allowed_bases. Investigated executeWorktreeWaveCleanupPlan (src/worktree-safety.cts) directly: this is NOT a reachable gap. Git enforces branch-per-worktree uniqueness, so a swapped-in entry.branch can only match worktree_path's ACTUAL checked-out branch if it names that sibling's own real, uniquely- generated branch name — which manifest tampering confined to one batch's own record has no way to know (branch names are agent-<quick_id>[-<timestamp>]-shaped, and quick_id allocation is collision-checked GLOBALLY across every existing quick task and batch, not merely within one batch). Adds a stronger test that empirically proves this: two REAL, concurrently- alive sibling worktrees of the same repo (both via real `git worktree add`, both WORKTREE_AGENT_BRANCH_RE-passing, both sharing one merge-base), with worktree_path/branch swapped between them in both directions. Both attempts are blocked via branch_mismatch; both real worktrees, their branches, and one sibling's real uncommitted-to-main commit survive completely untouched. Supplements (does not replace) the original two tests, which still prove distinct, real boundaries. See .gsd/phase/feat-3677-quick-batch-hardening-acceptance/40-design.md §9.2 for the full trace, including the one explicitly-documented (not fixed) trust boundary this investigation surfaced: the primitive defends against fabricated data, not a caller bug that misattributes a real-but-wrong item's own triple to a different item. * chore(#3677): checkpoint design-doc addendum for review pass 2 findings (gitignored, dev-only) * docs(#3677): add changeset for PR 4240 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-03 09:47:22 -04:00
Tom Boucher	3639ab0431	fix(#3968): measure commit claims at all three surfaces — ledger, verifier BLOCKER, porcelain HANDOFF (#4230) ... * test(#3968): commit claims must be measured against git, never narrated * fix(#3968): measure commit claims at all three surfaces Emitted-Drift-Ack-Growth: gsd-executor.md — #3968 plan commit ledger and measured commits contract (held under the agent size cap) Emitted-Drift-Ack-Growth: verify-work.md — #3968 commit-claim reconciliation with the same rev-list instrument Emitted-Drift-Ack-Growth: pause-work.md — #3968 uncommitted_files from git status --porcelain * fix(#3968): retired slash syntax, allowlist line pin, git-compare test pin * fix(#3968): persist the ledger on disk and reconcile with the same rev-list instrument Emitted-Drift-Ack-Growth: gsd-executor.md — #3968 plan commit ledger and measured commits contract (held under the agent size cap) Emitted-Drift-Ack-Growth: verify-work.md — #3968 commit-claim reconciliation with the same rev-list instrument * fix(#3968): hold the gsd-executor size cap with a compact ledger contract Emitted-Drift-Ack-Growth: gsd-executor.md — #3968 plan commit ledger and measured commits contract (held under the agent size cap) * fix(#3968): allowlist pin and HALT regex track the final prose * chore(#3968): changeset for measured commit claims * chore(#3968): backfill changeset pr number * fix(#3968): quote the BASE expansion (SC2086) * ci: raise the test-lane budget 21 to 32 minutes (measured cost grew past the cap) --------- Co-authored-by: sim <sim@local>	2026-09-03 05:43:16 -04:00
Tom Boucher	d5f8191f66	fix(#3730): quick-tasks-migrate — canonical-schema repair, auto-run on first quick (#4216) ... * test(#3730): a legacy Quick Tasks table must be migratable to canonical * fix(#3730): quick-tasks-migrate — canonical-schema repair, auto-run on first quick * fix(#3730): review fixes — usage parity, contiguous table span, collision-safe bucket, template-width delimiter Emitted-Drift-Ack-Growth: fast.md — #3730 runs quick-tasks-migrate before the first append (auto-migration on first quick run) Emitted-Drift-Ack-Growth: quick.md — #3730 replaces the match-any-format note with the migration instruction * chore(#3730): backfill changeset pr number * fix(#3730): scope the quick-batch row-48 guard to branches touching quick-batch --------- Co-authored-by: sim <sim@local>	2026-09-03 01:32:41 -04:00
Tom Boucher	2f64e6230a	feat(#3676): quick-batch command, workflow, and isolation integration (#4212) ... * test(#3676): add failing tests for quick-batch dispatch core Failing-first tests for Phase 4 of epic #3344 (ADR-1239 "Quick-batch binding"): quick-batch-dispatch.test.cjs / .property.test.cjs cover the new pure decision-logic module (arg validation, effective concurrency, deterministic merge order, spawn backpressure, verification/merge routing, cleanup-entry construction — design doc rows 3-15,24,26-28, 30-36,39; property rows 51-53). quick-batch-update-items.test.cjs covers the new updateBatchItems export on src/quick-batch.cts (rows 15,22-23, including the negative cycle-rejection case). quick-batch-command-router.test.cjs covers the new gsd-tools quick-batch CLI family (rows 46-47). These reference modules/ exports that do not exist yet. * feat(#3676): implement quick-batch dispatch core, updateBatchItems, and command router Phase 4 of epic #3344 (ADR-1239 "Quick-batch binding") CORE decision layer — CLI verbs and pure orchestration logic only; no workflow markdown, no Agent()/git-worktree I/O. - src/quick-batch-dispatch.cts (new): pure decision functions consumed by the (separate, follow-up) /gsd:quick-batch workflow markdown — parseQuickBatchArgs, computeEffectiveConcurrency, computeMergeOrder, computeSpawnPlan, routeVerificationOutcome, routeMergeOutcome, buildCleanupManifestEntry (the last parses caller-supplied plan text via the existing parsePlanDocument; no filesystem access). - src/quick-batch.cts: adds updateBatchItems, resolving the design doc's Open Question 1 as ONE additive export on this module instead of the second, independent BATCH.json writer the design doc originally proposed. Reuses the same withPlanningLock transaction shape, computeWaves, and platformWriteSync call resumeBatch/ completeQuickItem already use; fails closed without persisting on an unknown item, an unknown/self dependency, or an introduced cycle. - src/quick-batch-command-router.cts (new): gsd-tools quick-batch CLI family, wired into HOST_COMMAND_ROUTERS (gsd-core/bin/gsd-tools.cjs) as a first-party always-on command (like /gsd:quick), not the opt-in capability-registry path graphify uses. Verbs: create/update/resume/ complete (wrap quick-batch.cts) and effective-concurrency/ merge-eligible/spawn-plan/verification-routing/merge-routing/ cleanup-entry/parse-args (wrap quick-batch-dispatch.cts). Design doc rows covered: 3-15, 22-24, 26-28, 30-39, 46-47. Property rows 51-53. Rows covering workflow markdown / Agent() dispatch / `git worktree` behavior (16-21, 25, 29, 40-45, 48-50) remain for the follow-up markdown-authoring pass, per the phase brief's explicit scope boundary. * docs(#3676): register quick-batch-dispatch/command-router modules in bookkeeping surfaces New-.cts-module ripple for the two Phase 4 modules (epic #3344, ADR-1239 "Quick-batch binding"): .gitignore (compiled .cjs artifacts, ADR-457 build-at-publish), eslint.config.mjs (lint the .cts source, not the emitted .cjs), docs/INVENTORY.md + docs/INVENTORY-MANIFEST.json (via `node scripts/gen-inventory-manifest.cjs --write`, after `npm run build:lib`), and CONTEXT.md glossary entries for "Quick-Batch Dispatch Core Module" and "Quick-Batch Command Router Module", plus an update to the existing "Quick-Batch Core Primitives Module" entry documenting the new updateBatchItems export. * test(#3676): fold updateBatchItems tests into quick-batch.test.cjs (fix lint-test-file-count) scripts/lint-test-file-count.cjs buckets any quick-batch-*.test.cjs file under the quick-batch production module by longest-prefix match, and that module is already at its 2-file cap (quick-batch.test.cjs + quick-batch.property.test.cjs). The standalone tests/quick-batch-update-items.test.cjs added in the prior commit pushed it to 3 and failed `npm run lint:ci`. Fold its content into quick-batch.test.cjs (append-only — no existing test in that file is modified) and update the CONTEXT.md glossary reference to match. Surfaced while re-running `GITHUB_BASE_REF=next npm run lint:ci` after `npm ci` (this worktree previously had no local node_modules, which also made gen-scripts-cli-exit/gen-hooks-cli-exit/gen-exit-code-* unable to resolve typescript — resolved by npm ci, no code change needed there). `npm run lint:ci` and `npx tsc -p tsconfig.build.json --noEmit` are both green after this fix. * test(#3676): add failing tests for the quick-batch command/workflow markdown Failing-first tests for Phase 4's markdown-authoring pass (epic #3344, ADR-1239 "Quick-batch binding"): gsd-quick-batch-workflow.test.cjs covers commands/gsd/quick-batch.md's frontmatter/objective/process, gsd-core/workflows/quick-batch.md's byte-size boundary (row 49, ADR 1610 NEW_FILE_CAP) and step-fragment count, the isolation model (rows 20-22), the executor single-writer invariant (row 18), merge validation reusing the existing bounded primitive (row 25), the optional research/plan-checker/verification leaves (rows 16,17,19, 30,31), planning-failure blocking execution (row 29), the submodule guard (rows 36,44), and the new agents/gsd-planner.md quick-batch mode (rows 13-15). gsd-quick-batch-quick-regression.test.cjs covers row 48 (ordinary /gsd:quick stays byte-identical). Named `gsd-quick-batch-*` (not `quick-batch-*`) so lint-test-file-count's longest-prefix bucketing doesn't fold these markdown-only tests into the already-capped quick-batch/quick-batch-dispatch/ quick-batch-command-router production-module buckets from the CORE pass. These reference files that do not exist yet. * feat(#3676): author the quick-batch command, workflow, and planner mode Phase 4 markdown-authoring pass (epic #3344, ADR-1239 "Quick-batch binding") — the orchestration layer that calls into Pass 1's CLI verbs (src/quick-batch-command-router.cts). - commands/gsd/quick-batch.md (new): frontmatter/objective/process, delegates argument validation to `quick-batch parse-args` (parseQuickBatchArgs) rather than re-deriving the grammar. - gsd-core/workflows/quick-batch.md (new, 11843 bytes — under ADR 1610's 32768-byte NEW_FILE_CAP for a brand-new file) + 9 lazy-loaded step fragments under gsd-core/workflows/quick-batch/steps/: resume-mode, batch-init, research-phase (flag:--research), planner-wave (+ nested plan-checker-loop when --validate), worktree-dispatch, merge-wave, verification-wave (flag:--validate), completion. Covers design doc rows 3-45: capacity/isolation resolution (reusing dispatch-isolation-gate.md verbatim), per-DAG- layer planning with full-task-catalog prompts and always-required depends_on/files_modified frontmatter, serialized worktree create/ merge/cleanup via the existing worktree.cleanup-wave primitive, deterministic wave-order merging, verification routing (human_needed/gaps_found), the executor single-writer invariant, submodule fail-loud guard, and #1941 fork-base auto-degrade. - agents/gsd-planner.md: additive new `load_mode_context` bullet for `**Mode:** quick-batch`, pointing at the new gsd-core/references/planner-quick-batch.md reference (documents the always-required depends_on/files_modified contract, reusing the existing frontmatter grammar — no new keys). Existing modes byte-identical, only a new bullet added. - src/init.cts (+init-command-router.cts, +command-aliases.cts): cmdInitQuickBatch / `init.quick-batch` — model profiles, commit_docs, roadmap/planning existence checks, and the section_manifest field gating research-phase/verification-wave (reuses the existing flag:--research/flag:--validate WHEN_VOCABULARY atoms — no new atom needed). Rows 16-21, 25, 29, 36, 38, 39, 44, 46-50 covered structurally by the prior test(#3676) commit; rows 3-15, 22-24, 26-28, 30-35, 37, 40-43, 45 covered by construction (verb wiring, single-writer prompt constraints, crash-window resume via unmodified Phase 3 primitives). * docs(#3676): regenerate skills/inventory/section-manifest/install-tree; baseline the intentional word-splitting pattern npm run regen:derived output for the new command/workflow/reference (epic #3344, ADR-1239 "Quick-batch binding"): - skills/gsd-quick-batch/SKILL.md (generated from commands/gsd/quick-batch.md) - docs/INVENTORY.md rows for /gsd-quick-batch, quick-batch.md, planner-quick-batch.md, and the quick-batch-dispatch.cjs/ quick-batch-command-router.cjs CLI-module rows' now-live `/gsd-quick-batch` cross-reference (was "(separate, follow-up)") + docs/INVENTORY-MANIFEST.json (`node scripts/gen-inventory-manifest.cjs --write`) - gsd-core/workflows/section-manifest.json (`npm run gen:section-manifest`) — research-phase/verification-wave gsd:section entries for the new quick-batch workflow - tests/fixtures/install-tree/*.json (`npm run gen:install-tree`) — the new command/workflow/skill/reference files now ship to every runtime scripts/lint-workflow-shellcheck-baseline.json: 3 new entries for gsd-core/workflows/quick-batch.md's intentional flag-token/$ARGUMENTS word-splitting (SC2046/SC2086) — the same deliberate unquoted-optional- flag pattern gsd-core/workflows/quick.md already carries baselined (e.g. `$DISCUSS_PARAM $RESEARCH_PARAM` in quick.md's own Step 2); quoting would break the intended "omit this arg when the flag is false" splitting. * fix(#3676): close prompt-injection and argv/glob-injection gaps in quick-batch leaf dispatch Security review pass findings, both confirmed real: 1. HIGH — prompt injection, no boundaries. Every leaf-dispatch fragment interpolated the raw, attacker-influenced task ${description} (and the shared ${TASK_CATALOG_TABLE}, broadcasting every item's raw description into every planner's prompt in the layer) straight into Agent() prompt bodies with no boundary. Fixed by wrapping every such interpolation in a <security_context> + DATA_START/DATA_END boundary, matching the CONCRETE convention already implemented in this repo (agents/gsd-debug-session-manager.md, agents/gsd-debugger.md, gsd-core/workflows/debug.md) — commands/gsd/quick.md's own <security_notes> only asserts this convention in prose, so the debug-agent files are the real precedent followed here. Added a new <security_notes> block to commands/gsd/quick-batch.md (it had none) documenting both this fix and the one below. 2. MEDIUM — unquoted $ARGUMENTS -> argv/glob injection. gsd-core/workflows/quick-batch.md and commands/gsd/quick-batch.md both ran `gsd_run quick-batch parse-args --raw -- $ARGUMENTS` UNQUOTED, causing shell word-splitting and pathname expansion on raw task-list text before the parser ever saw it. Fixed at the source: added a `--text <string>` form to the `parse-args` verb (src/quick-batch-command-router.cts) that accepts the ENTIRE $ARGUMENTS as ONE quoted argv element and does the whitespace split itself, in Node — which is never glob-aware, unlike the shell. Both call sites now use `--text "$ARGUMENTS"`. The `-- <tokens>` form is kept for direct/test callers that already have a real argv array. The SC2086 baseline entry added for the original unquoted line is now stale (`node scripts/lint-workflow-shellcheck.cjs` no longer reports it) and has been removed; the two SC2046 entries for the UNRELATED, still-unquoted `$([ "$VALIDATE_MODE" = true ] && echo --validate)`-style conditional-flag splitting remain — that line only ever expands to one of a few known-safe literal strings (never raw user text), matching quick.md's own already-baselined convention exactly. Tests: quick-batch-command-router.test.cjs covers the new --text form (token splitting, glob-shaped text passing through literally unexpanded, whitespace-only input). gsd-quick-batch-workflow.test.cjs asserts the DATA_START/DATA_END boundary on every leaf prompt (research-phase/planner-wave/plan-checker-loop/verification-wave, including the shared task catalog) and the quoted --text call sites. * fix(#3676): strengthen test-depth gaps in rows 9, 18, 24, 34, 35 Spec review pass findings — the test matrix claimed "yes" coverage these assertions did not actually support: - Row 9 (--jobs 0/-1/abc hostile case): previously asserted rejection only. Added an end-to-end assertion (tests/quick-batch-command-router.test.cjs, committed alongside the security fix that touches the same file) that .planning/quick-batches/ is never created for any rejected value — createBatch is genuinely never reached. - Row 18 (--resume <unknown-batch-id>): previously only exercised a hand-corrupted BATCH.json, never a genuinely nonexistent batch directory. Added the real nonexistent-id case (also in quick-batch-command-router.test.cjs). - Row 24 (post-planning updateBatchItems racing a concurrent completeQuickItem for a different item, both through withPlanningLock): zero test existed. Added a property test (tests/quick-batch.property.test.cjs, appended — Phase 3's own file, no existing test touched) exercising both call orders and asserting no lost update in the final on-disk manifest — the same technique Phase 3's own row-15 lock-contention property test uses (sequential calls through the real lock; a working mutex makes any interleaving equivalent to some serial order, so this is the same claim a literal concurrent-thread test would make without OS-level threading). - Row 34 (worktree preserved on merge_failed) and row 35 (undeclared- deletion detection): both were previously asserted only at the pure routeMergeOutcome level. Added tests/gsd-quick-batch-merge-integration.test.cjs using the SAME real-git-fixture pattern tests/worktree-safety.test.cjs already establishes for executeWorktreeWaveCleanupPlan (real repo, real worktree, a REAL merge conflict / a REAL file deletion diffed against declared_deletions) — asserting the actual worktree directory survives on disk, not just that a pure function returns a preserveWorktree:true field. Named gsd-quick-batch-* so lint-test- file-count's bucketing doesn't fold it into any capped module bucket. Row 48 (/gsd:quick regression) intentionally left as-is per the reviewer's own framing: the byte-identity claim is already mechanically proven by the changed-path diff (git diff --name-only empty on those two paths IS byte-identity), and a genuine execution- level regression test would require actually running the workflow — out of scope for this repo's unit-test model (no other quick.md regression test in this repo does that either). * docs(#3676): add the changeset and user-facing docs the command needed Standards review pass findings — both HARD: - Missing changeset. None of the 6 prior #3676 commits touched .changeset/*. /gsd-quick-batch is a new user-facing command; CLAUDE.md/CONTRIBUTING.md require one. Added .changeset/silly-rams-caper.md (type: Added, pr: 0 placeholder — backfilled after the PR opens, matching CLAUDE.md's own documented convention and Phase 3's own precedent, #4190's .changeset/mellow-yaks-squeak.md). Uses the docs-convention hyphen form `/gsd-quick-batch` throughout, never the source-artifact colon form (`scripts/lint-docs-command-form.cjs` confirms 0 violations; that check scans docs/**, not .changeset/, so it was never actually in scope for the fragment itself, but the wording still follows the doc convention for consistency, matching how Phase 3's own fragment named the not-yet-shipped command). - Missing docs. Added docs/how-to/batch-quick-tasks.md (Diátaxis how-to, matching docs/how-to/handle-quick-and-fast-tasks.md's existing convention for /gsd-quick /gsd-fast) covering --jobs, --validate, --research, --resume, --file, the capacity/isolation interaction, and resume/failure recovery. Cross-linked from docs/README.md's how-to index and from handle-quick-and-fast-tasks.md's own "Related" section. Added a /gsd-quick-batch section to docs/COMMANDS.md (same table format as the existing /gsd-quick entry) and docs/features/quick-batch.md (REQ-QB-01..12, same frontmatter shape as docs/features/quick-mode.md) — regenerated docs/FEATURES.md (179 features) and skills/gsd-quick-batch/SKILL.md via the standard generators. * fix(#3676): close docs-parity, attribution, and generated-registry gaps gsd-test caught gsd-test's real run against 155e8975b3 found 43 failures, all rooted in this phase's own new command/workflow never being registered across ~10 independent generated/hand-maintained registries this repo keeps in parity by convention. Root-caused each, no test weakened or special-cased. - help.md ↔ commands/gsd/ bidirectional parity (docs-parity-live- registry.test.cjs): added a /gsd:quick-batch entry to gsd-core/workflows/help/modes/full.md (the real help.md content; gsd-core/workflows/help.md is a thin dispatcher) documenting every flag (--file/--jobs/--validate/--research/--resume), matching the existing /gsd:quick entry's format. - gen-section-manifest.test.cjs: quick-batch.md's `gsd_run query init.quick-batch` invocation used inline `$([ ... ] && echo --flag)` substitutions, which never satisfy the test's exact-whitespace-token / assigned-variable detection (the trailing `))` glued onto `--research` in the compound substitution broke the "exact token" match). Rewrote to the same VALIDATE_PARAM/RESEARCH_PARAM two-line pattern gsd-core/workflows/quick.md's own Step 2 already uses. - runtime-launcher-parity.test.cjs: the 8 quick-batch/steps/*.md fragments that call gsd_run each needed their OWN embedded copy of the canonical shim preamble (every workflow .md that calls gsd_run carries its own copy — reading one file does not persist shell state into another). Ran `node scripts/sync-runtime-launcher.cjs`, which inserted it before each file's first gsd_run call. plan-checker-loop.md correctly has none — it never calls gsd_run directly. - Namespace routing (skill-manifest.test.cjs, install-nested- layout.test.cjs, runtime-artifact-layout-surface.test.cjs): added `quick-batch` to commands/gsd/ns-workflow.md's `requires:` array and routing table (same namespace `quick` already routes through), and to src/clusters.cts's `utility` cluster (same cluster `quick` already belongs to). Verified by hand-running installRuntimeArtifacts + applySurface for augment/cline against a real temp install: exactly 6 top-level gsd-ns-* router dirs, gsd-quick-batch correctly nested under gsd-ns-workflow/skills/, never re-flattened. - mcp-server-catalog.test.cjs: hardcoded command count 71 -> 72 (a brand-new command is a real count change, not a bug this test should hide). - model-omit-when-inherit-guard.test.cjs: added the canonical `<!-- #2517 model-omit-on-inherit -->` marker block to gsd-core/workflows/quick-batch.md (every leaf dispatch — planner/ researcher/checker/executor/verifier — lives in a steps/ fragment, read combined with the host by this test's own readWorkflowCombined, same as quick.md's own research-phase.md carries it for its gated section). Also fixed a genuine pre-existing inconsistency in the test's own "#2711: the guarded set is derived from dispatch sites" check: its `nonDispatching` computation read the BARE host file while `derived` (the set it's checked against) reads the combined host+steps content — inconsistent with that same test file's own #2994 doc comment explaining why the combined read is necessary. quick-batch.md is the first workflow whose EVERY model="{...}" dispatch site lives in a mandatory (never gated) steps/ fragment — extracted to stay under ADR-1610's tighter NEW_FILE_CAP for a brand-new file — which is what exposed the mismatch. Fixed by using the same readWorkflowCombined read in both places. - skill-frontmatter-contract.test.cjs: shortened commands/gsd/quick-batch.md's frontmatter `description` from 107 to 91 chars (<=100 budget), and added `quick-batch.md` to the hand- maintained KNOWN_SKILLS consolidation allowlist with a #3676 justification comment (a genuinely new first-party command, not a consolidation of an existing skill). - workflow-fragments-emission.install.test.cjs: added `quick-batch.md` to the hand-maintained MARKED_WORKFLOWS set (composeWorkflow is deliberately NOT a no-op for it — its research-phase/verification- wave sections are gated). - Regenerated all downstream artifacts (npm run build:lib && npm run regen:derived && npm run gen:plugin-skills -- --write && npm run gen:features -- --write): skills/gsd-quick-batch/SKILL.md, skills/gsd-ns-workflow/SKILL.md, install-tree fixtures for augment/cline/hermes/qwen/trae/zcode. - emitted-attribution.test.cjs: agents/gsd-planner.md's #3676 addition (one new `load_mode_context` bullet pointing at the new gsd-core/references/planner-quick-batch.md reference) grew the file 124 bytes without an acknowledgment trailer. Acknowledged below — the growth is the deliberate, additive, single-bullet change from the earlier feat(#3676) commit, not drift. Verified: npm run build:lib clean, npx tsc -p tsconfig.build.json --noEmit clean, GITHUB_BASE_REF=next npm run lint:ci fully green (includes lint-workflow-shellcheck, lint-test-file-count, lint-docs-command-form). The deep install/spawn/registry tests gsd-test actually runs (docs-parity-live-registry, gen-section-manifest, runtime-launcher-parity, install-nested-layout, runtime-artifact-layout-surface, skill-manifest, skill-frontmatter- contract, mcp-server-catalog, model-omit-when-inherit-guard, workflow-fragments-emission) are not part of lint:ci — each fix above was independently verified by hand-invoking the exact production function the failing test calls (installRuntimeArtifacts, applySurface, composeWorkflow, the CLUSTERS union, the section-manifest forwarding regex) against the real repo tree and confirming the expected shape. Emitted-Drift-Ack-Growth: gsd-planner.md — additive #3676 quick-batch mode bullet in load_mode_context (one new line pointing at gsd-core/references/planner-quick-batch.md); not drift. * fix(#3676): trim the /gsd:quick-batch help.md entry to fit the LARGE tier line budget skill-frontmatter-contract.test.cjs's "feature #3039: tiered help — size budgets" enforces a SEPARATE line-count ceiling for gsd-core/workflows/help/modes/full.md (FULL_BUDGET = 844 lines, tighten-only ratchet, scripts/lib/allowlist-ratchet.cjs's assertTightCeiling) — independent of the skill-frontmatter description- length budget and consolidation allowlist I touched in the prior round; those are unrelated checks in the same test FILE, not the same check. Root cause: the /gsd:quick-batch entry I added to full.md in the docs-parity fix round was 17 lines, pushing the file from 834 to 851 lines — 7 over the 844 ceiling. Condensed the entry (merged the per-flag bullet list into one dense "Flags:" line, dropped from 3 Usage examples to 1) to 844 lines exactly — at the ceiling with zero slack, which assertTightCeiling accepts (it only fails on actualMax > ceiling, or on slack > grace when the ceiling is too LOOSE — zero slack triggers neither). Verified after trimming: full.md still contains a live /gsd:quick-batch reference (bidirectional parity) and all 5 argument-hint flags (--jobs/--validate/--research/--resume/--file) still appear as literal tokens (docs-parity-live-registry.test.cjs's own flag-coverage check, re-run by hand against the trimmed content). Verified: npm run build:lib clean, npx tsc -p tsconfig.build.json --noEmit clean, GITHUB_BASE_REF=next npm run lint:ci fully green. * docs(#3676): backfill changeset pr number to 4212 Follow-up to fix(#3676) commits — .changeset/silly-rams-caper.md's pr:0 placeholder backfilled with the real PR number now that gh api POST /pulls has returned it (#4212). Matches CLAUDE.md's PR Number Handling convention and Phase 3's own #4190 precedent (708c5a3f8c). Doc-only (root-level .changeset/*.md fragment), exempt from a fresh gsd-test run per pre-pr-gate.sh's DOC_ONLY_RE. * fix(#3676): resolve prompt-injection-scan false positive on test fixture tests/quick-batch.test.cjs:232's row 11b regression proves the task-list parser carries a prompt-injection-shaped task description through createBatch as inert data, never interpreted. The fixture has to be a real "ignore all previous instructions..." phrase or the test asserts nothing, but the full-file --diff scan flagged it once unrelated edits in the same file pulled it into the changed-file set. Add the file to prompt-injection-scan.sh's ALLOWLIST, matching the sanctioned, precedented exemption already used for other legitimate security-regression fixtures (tests/windsurf-conversion.test.cjs, tests/health-validation.test.cjs, tests/continuation-grammar-parity.test.cjs) per DEFECT.PROMPT-INJECTION-SCAN-COLLISION. --------- Co-authored-by: sim <sim@local>	2026-09-02 22:38:31 -04:00
Tom Boucher	7c52344284	fix(#4003): anchor the safe-resume gate's plan-scope greps to the milestone (#4194) ... * test(#4003): safe_resume_gate must grep an anchored padding-tolerant scope * fix(#4003): anchor the resume-gate scope greps and bound them to the milestone tag Emitted-Drift-Ack-Growth: execute-phase.md — #4003 rewrites three commit-scope greps (safe_resume_gate, TDD RED, completion spot-check) to anchored zero-pad-tolerant regexes with a milestone tag bound; growth is the fix itself * test(#4003): align shape assertions with the implemented gate text * fix: bump fast-uri past GHSA-jqff-g426-hqxp (transitive, advisory reddened next) * fix(#4003): bound the TDD RED grep to the milestone and fix tdd.md's example greps * test(#4003): the gate pin tracks the anchored scope grep * fix(#4003): trim the gate rationale to hold the 93400 margin ceiling * test(#4003): the RED-grep pin tracks the milestone-bounded invocation * chore(#4003): changeset for the anchored resume-gate scope * chore(#4003): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-02 13:45:14 -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	5c7243e54b	fix(#3995): derive the review diff base from the phase directory (#4181) ... * test(#3995): diff base keys on the phase directory, not commit subjects All three derivation sites (Tier 3, spawn_reviewer, fallow pre-pass) must anchor on the phase directory's first commit; the milestone-blind repro (an archived milestone's same-numbered phase commit capturing the base) is the failing-first row. #3191/#3503 rows reworked to the directory-anchor contract; T6 docs-parity forbids any remaining phase-scope message-grep site. * fix(#3995): derive the review diff base from the phase directory A phase number is unique within a milestone, not a repository; the message grep had no milestone bound and tail -1 deliberately selected the oldest same-numbered subject, dragging archived milestones phases into the scope (7 files to 3388 plus the >50 depth downgrade). All three lockstep sites now anchor on the first commit that added anything under the phase own directory — the same anchor class git-base-branch phaseStartCommit uses. ShellCheck baseline gains the escaped fragment shifted parse signature. Emitted-Drift-Ack-Growth: code-review.md — phase-directory anchor replaces the message-grep derivation at both sites (#3995) * chore(#3995): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-02 06:03:17 -04:00
Tom Boucher	647365faf1	fix(#4011): key the TDD runtime gate on TDD_MODE alone (#4180) ... * test(#4011): TDD gate keys on TDD_MODE alone, not the MVP intersection Contract updates: no shipped line may conjoin MVP_MODE with TDD_MODE as a gate condition, the end-of-phase escalation must not require MVP, the executor agent's gate section triggers on TDD_MODE alone, and the gate semantics reference loads without MVP_MODE. * fix(#4011): key the TDD runtime gate on TDD_MODE alone The RED-commit gate shipped as #76's MVP slice kept the paired invocation's conjunct, so workflow.tdd_mode=true was silently inert on every non-MVP phase, contradicting references/tdd.md's own contract. Drops the MVP conjunct from the per-task gate and the end-of-phase review escalation; rescopes execute-mvp-tdd.md's load condition, gsd-executor's gate section, and mvp-concepts' intersection claim. MVP remains free to imply TDD; the file is not renamed (stated assumption in the PR body). * test(#4011): scope no-conjunct detector to shell conditions; clean stale MVP+TDD phrasing Review follow-ups: the detector now only inspects if/[ condition lines so explanatory prose mentioning both flags cannot trip it; remaining 'under/outside MVP+TDD' phrases in execute-phase.md, the gate reference, and docs/INVENTORY.md now describe TDD-mode semantics. Emitted-Drift-Ack-Growth: execute-phase.md — TDD-gate decoupling comment + escalation rescoping (#4011) Emitted-Drift-Ack-Growth: gsd-executor.md — gate section trigger rescoped to TDD_MODE alone (#4011) * chore(#4011): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-02 04:24:52 -04:00
Tom Boucher	7960374d15	fix(#3962): rename the TDD-Audit trailer token to gate-status (#4174) ... * test(#3962): shipped TDD-Audit trailer token must round-trip through git Behavioral coverage: extract the trailers:key token from ship.md and prove a real git commit carrying that trailer reads back via %(trailers:key=token,valueonly). gate_status contains an underscore, which git's trailer machinery cannot tokenize, so the audit read was structurally empty. * fix(#3962): rename the TDD-Audit trailer token to gate-status Underscore is not a valid git trailer token character, so %(trailers:key=gate_status,...) could never match. Renames the token at the read, the documented aggregate write, and the section's prose/table header. Self-suppression semantics (#2431) unchanged. * test(#3962): compare outcome against the seam's literal, fix doc token Review follow-ups: the round-trip test compared outcome to 'EXITED' but the process seam emits 'exited'; and the trailer rename is propagated to docs/ship-pr-body-sections.md's read/write examples. * chore(#3962): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-02 00:01:55 -04:00
Tom Boucher	10593e2cf2	fix(#3959): give review-prompt plans citable path anchors (#4170) ... * test(#3959): plan copies carry source names, prompt carries path anchors Regression coverage: budget copies named gsd-review-plan-<plan-id>.md (not a bare padded index), no bare-index copy survives, and the Plans to Review template instructs a per-plan repo-relative #### path header. Also corrects the stale -00 assertion to the source-named copy. * fix(#3959): give review-prompt plans citable path anchors The Plans to Review template now instructs a per-plan repo-relative #### path header, and the budget copies are named gsd-review-plan-<plan-id>.md instead of a bare padded index, restoring provenance for prompt-budget's per-plan headers while keeping the trim glob intact. Emitted-Drift-Ack-Growth: review.md — per-plan path-header instruction in Plans to Review + provenance-named copy loop (#3959) * chore(#3959): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-01 22:40:23 -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	bf4485ada2	enhance(#3717): make the edge probe's shape cues language-aware via an optional text_en field (#4156) ... * test(#3717): add failing-first coverage for text_en language-aware classification Adds unit tests for the not-yet-implemented text_en field on Requirement (fallback selection, empty/whitespace/non-string rejection, shapes-override precedence), a SHAPE_CUES/VALID_SHAPES parity guard (RULESET.GENERATIVE-FIX), and workflow-prose contract tests asserting spec-phase.md Step 5.5 documents populating text_en for response_language projects. All new tests are RED until src/edge-probe.cts and the workflow docs are updated. * feat(#3717): make edge-probe shape classification read an optional text_en field Requirement gains an optional text_en; classifyShape's own signature stays untouched (a locked, directly-tested export), and the text_en ?? text selection is pushed to proposeEdges' single call site instead. text_en is validated fail-closed: an empty or whitespace-only value throws rather than silently winning the ?? fallback and degrading classification to zero shapes. This makes the #2773 doc-only translation convention an explicit, validatable field instead of an invisible instruction, per the approved Form-1 scope on #3717. * docs(#3717): document the text_en field across spec-phase, reference and how-to docs Updates Step 5.5's response_language instructions, the edge-probe reference Inputs contract, the FEATURES.md fragment, and the non-English how-to guide to describe the new text_en field: text keeps the requirement's own wording in all cases, text_en (when populated) is the engine-only English rendering the classifier prefers. * docs(#3717): record the text_en locked-surface change in CONTEXT.md and ADR-550 Updates the Edge Probe Module glossary entry to describe the text_en field and its fail-closed validation, and appends an ADR-550 amendment recording why this is additive and does not re-open the #652 LLM-classifier rejection (text_en is a plain field read by the existing deterministic regex classifier, not a new model-dependent surface). * docs(#3717): add changeset fragment and regenerate FEATURES.md pr:0 placeholder — backfilled with the real PR number after the PR opens. * docs(#3717): attribute the text_en machine check to engine-level validation, not prose tests Code-review (Spec axis) finding: the workflow-prose contract tests and the ADR-550 amendment overclaimed themselves as "the machine check the #2773 doc-only stopgap lacked." That check is actually engine-level (validateRequirement/classifyShape, covered in tests/edge-probe.test.cjs) — the prose tests are the same style of assertion #2773 already used. Reworded both to attribute the claim correctly. * fix(#3717): rewrap spec-phase.md so the id-unchanged sentence stays on one line The #3717 rewrite of Step 5.5's response_language paragraph moved a line break so "requirement `id`s" ended one physical line and "are never translated" started the next. The pre-existing #2773 regression test (tests/edge-probe-spec-phase-contract.test.cjs) asserts id + "never translated" on the SAME line (no \n in between, matching git's own line-oriented prose), so the reflow silently broke it. Rewrapped so the sentence lands on one line again, verified against every #2773/#3717 regex assertion in that test file. Emitted-Drift-Ack-Growth: spec-phase.md — #3717 adds text_en documentation to Step 5.5 (response_language paragraph + REQS_JSON heredoc comment); this growth is this PR's own diff, not incidental drift. * chore(#3717): backfill changeset PR number pr:0 -> pr:4156 now that the PR exists. --------- Co-authored-by: sim <sim@local>	2026-09-01 21:39:53 -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
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
Dennis Alexis Valin Dittrich	c9bff8d2b9	fix(#3899): resolve REVIEWS.md by quoted assignment, not an unquoted ls (#3928) ... * fix(#3899): resolve REVIEWS.md by quoted assignment, not an unquoted ls The convergence loop resolved the phase REVIEWS.md through `$(ls ${phase_dir}/... 2>/dev/null)`. The unquoted ${phase_dir} word-splits, so a project path containing a space handed `ls` arguments that do not exist, the discarded stderr hid the error, and the empty result was reported as "review agent did not produce REVIEWS.md" — blaming the agent for a quoting defect. A glob metacharacter fails worse: the pattern expands to whatever sibling directory matches, so the loop silently reads a different phase's REVIEWS.md and `ls` still exits 0. Assign the path directly, quoted, and fail closed with an error that names the expected location. The regression block extracts the bash fence from the shipped workflow and RUNS it against space-, glob- and missing-file fixtures, because every text assertion in that suite passed against the broken line. * chore(#3899): add the changelog fragment * fix(#3899): reject a non-file reviews path and anchor the gate harness Adversarial review (codex, gemini) on the first cut, three findings taken: - `[ -r ]` alone is true for a readable DIRECTORY, so a directory standing where the reviews file belongs passed the gate and reached the consumers that read it. Require a regular file as well. - the harness picked its fence by scanning the WHOLE document for a bash block assigning REVIEWS_FILE. If the real fence ever stopped assigning it and an unrelated one started, the harness would execute the wrong block and report green. Anchor the span to the verification step, between its opening sentence and the next heading. - the no-subshell assertion matched only `$(`; a backtick rewrite would reintroduce identical word-splitting unseen. The discarded-stderr assertion is scoped to lines naming REVIEWS_FILE rather than banning the redirect across the whole fence. Four findings declined: `return 1 || exit 1` (the document's own idiom is a bare `exit 1`, including the guard immediately below), `${phase_dir:-}` for `set -u` (unchanged exposure from the old line, and an unset phase_dir is a bug worth surfacing), an explicit empty-phase_dir branch (the guard already fails closed and prints the truncated path), and a downstream `[ -n "$REVIEWS_FILE" ]` hazard (no such consumer exists — the guard exits before any of them run). * chore(#3899): backfill the changeset PR number * fix(#3899): address required review feedback Use the fragment-aware workflow reader, remove the inert exemption marker, document the unreadable-arm coverage limit, align errors with the workflow convention, and name an empty phase_dir directly. Migrate the obsolete emitted-drift fragment to the current commit-trailer contract. Emitted-Drift-Ack-Growth: plan-review-convergence.md — #3899: replace unquoted $(ls ... 2>/dev/null) resolution with a quoted direct path, fail-closed file/readability checks, and an explicit phase_dir diagnostic; surrounding prose records why quoting is load-bearing. --------- 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 20:25:27 -04:00
Tom Boucher	b6dd4e2e74	fix(#3936): dispatch quick research with researcher persona and model (#4158) ... * test(#3936): quick research dispatch must use researcher persona/model Regression tests for the planner-persona leak in quick's Step 4.75 research dispatch: init quick emits researcher_model, the workflow resolves AGENT_SKILLS_RESEARCHER and parses researcher_model, and the researcher dispatch uses the researcher persona/tier while planner and executor dispatches keep their own. * fix(#3936): dispatch quick research with researcher persona and model cmdInitQuick now emits researcher_model (parity with the plan-phase init), quick.md resolves AGENT_SKILLS_RESEARCHER and parses the new field, and Step 4.75's dispatch swaps ${AGENT_SKILLS_PLANNER}/ {planner_model} for the researcher's own persona and tier. The #2517 model-omission list gains researcher_model. Emitted-Drift-Ack-Growth: quick.md — researcher persona resolution line + researcher_model parse-list entry (#3936) * chore(#3936): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-01 19:40:34 -04:00
Tom Boucher	eca9c2b590	fix(#4112): portable grep in workflow markdown + submodule commit fix under diff.ignoreSubmodules (#4149) ... * fix(#4112): ban GNU-only grep -P in workflow markdown to prevent macOS regressions Add scripts/lint-portable-grep.cjs, wire it into lint:ci, and add tests/lint-portable-grep.test.cjs. The #4112 shell-syntax fix newly exposed a pre-existing grep -oP invocation that silently resolves to "" on stock macOS's BSD grep (no -P support). This ratchet catches the same class of GNU-coreutils assumption before it merges, mirroring lint-portable-timeout.cjs. * fix(#4112): drop unneeded ls -l long-format that broke basename/dirname extraction The previous commit on this branch replaced grep -oP 'phases/\K[^/]+' (GNU-only, silently fails on macOS's BSD grep) with basename "$(dirname "$phase")"), but left ls -lt (long format, -l) in place. -l output is a full detail line (permissions, owner, size, date, path), not a bare path, so dirname on that string throws "illegal option -- r" (BSD) / errors under GNU coreutils too -- the extraction never produced a usable value, on any platform. -l was never needed here; only mtime-sort plus the bare path mattered. Dropping it to ls -t restores one-bare-path-per-line output, which basename/dirname actually requires. Confirmed on gsd-test's Linux bench: the #4112 regression test (tests/pause-work-context-detection.test.cjs) now passes phase, spike, and sketch resolution. Emitted-Drift-Ack-Growth: pause-work.md — portability fix (#4112): dropping grep -oP for a portable basename/dirname extraction is a few characters longer per line; no functional growth beyond the fix. Emitted-Drift-Ack-Growth: sync-skills.md — portability fix (#4112): replacing grep -oP '(?<=--from )\S+' with a portable sed -E capture-group equivalent (no PCRE lookbehind available) is a longer expression; growth is the direct cost of the fix, not new functionality. * fix(#3859): pin git commit itself against diff.ignoreSubmodules=all, not just the probe git 2.39.x (the exact version on the CI Linux bench) resolves a pathspec-scoped `git commit -- <path>` through the same diff.ignoreSubmodules-gated machinery as `git diff`, so a genuinely bumped submodule gitlink was silently dropped by the real commit even though the #3859 empty-diff probe correctly saw the change. The commit was misclassified as generic commit_failed because git's refusal text never says "nothing to commit". Prefix the pathspec-scoped commit invocation with -c diff.ignoreSubmodules=dirty, the same override the probe already carries, so the two can no longer disagree. Confirmed on git 2.50.1 (no-op) and git 2.39.5 via the actual gsd-tester-linux v1.8.0-node24 bench image (turns the silent refusal into a commit). * fix(#3859): carry the diff.ignoreSubmodules override via env, not argv The prior commit prepended `-c diff.ignoreSubmodules=dirty` to the real commit's argv, which shifted `commitArgs[0]` off `'commit'` and broke several pre-existing #3859 regression tests (tests/commit-files-pathspec.test.cjs) that assert on the raw argv captured at the execGit seam, e.g. `gitCalls.some((a) => a[0] === 'commit')`. Carry the same override via `GIT_CONFIG_COUNT` / `GIT_CONFIG_KEY_0` / `GIT_CONFIG_VALUE_0` env vars instead, which git honours identically and leaves argv untouched. Re-confirmed on the gsd-tester-linux v1.8.0-node24 bench (git 2.39.5): the submodule commit still succeeds. * test(#3859): pin the commitEnv GIT_CONFIG_* override to canScope directly The commitEnv/GIT_CONFIG_* override added in e935694fc/b3d37b929 was only exercised indirectly through pre-existing submodule integration tests. Add two dedicated regression tests that pin the canScope=false side of the scoping decision: a whole-index commit (no --files) and an --amend commit must both keep recording a bumped submodule gitlink under diff.ignoreSubmodules=all with no override applied, since neither shape carries a pathspec for that git internal check to consult. * fix(#4112): match grep invocations after then/do/else/elif shell keywords lint-portable-grep.cjs's GREP_INVOCATION_RE only anchored to line-start or right after `| & ; ( \` {`, so a grep call positioned right after `then`, `do`, `else`, or `elif` (e.g. `if x; then grep -oP '...'; fi`) was never flagged, letting the GNU-only grep -P defect this lint exists to catch reappear undetected in that shape. * fix(#3859): apply the diff.ignoreSubmodules override to every commit, not just scoped ones The commitEnv GIT_CONFIG_* override landed in e935694fc/b3d37b929 only when canScope was true, on the assumption that git 2.39.5's silent refusal of a bumped submodule gitlink under diff.ignoreSubmodules=all only affects a pathspec-limited `git commit -- <paths>`. Reproduced directly against the pinned CI tester image (ghcr.io/open-gsd/gsd-tester-linux:v1.8.0-node24, git 2.39.5): a bare whole-index `git commit -m ...` with no pathspec at all is refused identically when the only staged change is a submodule gitlink, since git's "nothing to commit" check is a real diff against HEAD that honours diff.ignoreSubmodules regardless of whether a pathspec narrows it. Apply the override unconditionally instead of gating it on canScope; it remains a confirmed no-op for --amend, which never hits this refusal at all. Caught by the new regression test added in 579ac9f8f, which failed against the real bench git version before this change. * fix(#3859): apply diff.ignoreSubmodules override to commit-to-subrepo and pr-subrepo cmdCommit already carries the GIT_CONFIG_* override that forces diff.ignoreSubmodules=dirty on the actual git commit call so a bumped submodule gitlink is not spuriously refused under git 2.39.5. The two sibling multi-repo commit paths, cmdCommitToSubrepo and cmdPrSubrepo, build the identical canScope-branched commit invocation but never carried the override, so the same refusal there surfaces as a generic commit_failed/error instead of a recorded gitlink. Apply the override unconditionally to both, matching the corrected cmdCommit shape. * fix(#3859): pin pr-subrepo's change detection against diff.ignoreSubmodules cmdPrSubrepo discovers what to commit via `git status --porcelain`, which (like the empty-diff probe fixed for cmdCommit) honors a local diff.ignoreSubmodules=all config. Under that config, a genuinely bumped submodule gitlink is invisible to the status scan, so changedFiles comes back empty and the function reports nothing_to_commit before ever reaching the commit call this same issue already fixed. Pin the status probe with --ignore-submodules=dirty, mirroring the flag cmdCommit's diff probe already uses, so a real gitlink bump is detected regardless of local config. Found while adding a regression test for the previous #3859 follow-up fix: the test failed not on the commit step but on this earlier detection step. * docs(#3859): update changeset to cover all three fixed commit sites * docs(#4112): backfill changeset PR number (pr:0 -> pr:4149) --------- Co-authored-by: sim <sim@local>	2026-09-01 17:31:01 -04:00
Tom Boucher	80d2236ca5	fix(#4112): drop redundant $(( subshell wrapping in pause-work.md Context Detection (#4140) ... * test(#4112): add failing-first regression coverage for pause-work.md Context Detection Extracts and executes the shipped Context Detection bash block to prove the $(( arithmetic-context misparse (SC1102/SC1106/SC2205) as a hard syntax error, plus boundary/independence coverage for the phase/spike/sketch/ deliberation resolution the fix must preserve. * test(#4112): fix false-green regression test — assert dash/sh failure, not bash -n The prior version asserted bash -n syntax validity and wrapped bash -c execution, neither of which detects this bug: bash/zsh silently fall back to a working command-substitution reading of the ambiguous $(( construct, and bash -n never evaluates arithmetic-context content at parse time. A gsd-test run against the still-broken file passed 41594/41594 with this test in place — a false green. POSIX sh/dash does not implement that fallback and throws a real "Syntax error: Missing '))'", which is what this version asserts against. * fix(#4112): remove leaked tool-call tags corrupting the regression test file A subagent's Write introduced trailing </content>/</invoke> markup at the end of tests/pause-work-context-detection.test.cjs. gsd-test's own tests/portability-rule-disable-ban.test.cjs (which parses every test file) correctly caught this as a parse error. Strips the garbage lines; no behavioral change. * fix(#4112): drop redundant $(( subshell wrapping in pause-work.md Context Detection phase=/spike=/sketch= opened with $((, which POSIX sh/dash parses as arithmetic expansion and rejects (Syntax error: Missing '))') since the enclosed text isn't valid arithmetic. bash/zsh silently retry it as command substitution, which masked the defect there. Dropping the inner grouping parens (matching the deliberation= line already in the same block) makes the construct valid under bash, zsh, and dash alike, with identical fallback-to-empty-string behavior when nothing matches. Once the surrounding syntax parses, ShellCheck can now also analyze the inner ls -lt calls it previously couldn't see past the parse failure, newly surfacing the same pre-existing SC2012 ("use find instead of ls") suggestion already baselined for the file's other ls usage. Baseline updated to reflect the true current count; no ls-vs-find behavior change made, as that is a separate, pre-existing, out-of-scope question. * fix(#4112): verify shell discrimination instead of trusting the binary name Code review finding: falling back from dash to sh could silently produce a non-discriminating test on a host where /bin/sh is bash-compatible (e.g. macOS) — such a shell never throws on the broken construct either, so the test would pass whether the bug were present or not. resolvePosixShell now verifies the candidate actually rejects a known-ambiguous $(( snippet before using it, and skips with an explicit reason when none does. The gsd-test Linux bench (dash as /bin/sh) is unaffected either way. * fix(#4112): route regression test through process-seam, splitLines, cleanup npm run lint:ci flagged 7 violations gsd-test's node:test run doesn't check: local/no-adhoc-markdown-parsing (single fence-spanning regex), local/no-crlf-fragile-split (bare \n split), local/no-unbounded-spawn (x4, hand-rolled execFileSync with no timeout), and local/no-raw-rmsync-in-tests. Rewrites the test to route every subprocess call through tests/helpers/process-seam.cjs's runHook() (bounded by construction), tests/helpers.cjs's cleanup() for temp-dir removal, and a line-by-line fence scan using the text-lines.cts splitLines() seam, matching this repo's established extraction idiom (tests/no-hardcoded-home-gsd-tools.test.cjs). No behavioral change to what is asserted. * docs(#4112): add changeset for the pause-work Context Detection fix * docs(#4112): backfill changeset PR number (pr:0 -> pr:4140) --------- Co-authored-by: sim <sim@local>	2026-09-01 13:03:33 -04:00
Tom Boucher	05092ff369	fix(#4113): add no-op to empty then-body in chunked-planning-mode.md's outline resume-check (#4125) ... * fix(#4113): add no-op to empty then-body in chunked-planning-mode.md's outline resume-check The `### 8.5.1 Outline Phase` resume-detection `if` block only contained a comment in its `then` clause, which is a syntax error under both bash and zsh if executed literally. The workflow ShellCheck lint added by #4109 already flags this (SC1009/1048/1072/1073/2105) but the findings were masked by matching entries in the pre-existing-findings baseline; those five entries are removed here so a regression re-surfaces as a new failure instead of being silently re-absorbed. * chore: backfill changeset PR number for #4125 * fix(#4113): fix second empty-then/continue-outside-loop defect surfaced by baseline cleanup Removing the 5 stale baseline entries for chunked-planning-mode.md in the prior commit unmasked a second, genuinely separate ShellCheck finding (SC2105) in a different fenced block of the same file: a bare `continue` outside any bash loop in the ### 8.5.2 per-plan resume-check. Same root cause class as the original bug (pseudocode fenced as literal bash) — fixed the same way, with a `:` no-op preserving the documentation. --------- Co-authored-by: sim <sim@local>	2026-09-01 00:55:44 -04:00
Cody Anderson	8c9265d4e5	fix(#3724): warning-only Dimension 3b findings no longer force the revision loop (#3758) ... * fix(#3724): stop advisory Dimension 3b findings from forcing the revision loop Dimension 3b (undeclared/temporal coupling, #1954) is spec'd "never a blocker" but tagged severity: warning — the tier plan-phase's revision loop counts as must-fix — and the planner is never taught the rule, so every multi-wave phase touching shared mutable state replans at least once, and intentionally coupled plans re-flag identically every iteration to the stall prompt. Three coordinated changes: - gsd-plan-checker: retag 3b to severity: info, the tier references/revision-loop.md already exempts by design; recognize a coupling_justified frontmatter declaration in the Do-NOT-flag list so deliberate pairs converge. Additions are offset by trimming 3b motivation prose — the checker sits 45 bytes under its LARGE hard cap. - plan-phase step 12: INFO-only accept — an issues block with zero BLOCKER/WARNING entries accepts the plan and surfaces the advisories instead of re-entering the revision loop. Real blockers and warnings still gate unconditionally. - gsd-planner: slim pointer in assign_waves to the new progressive-disclosure reference gsd-core/references/planner-coupling.md (the planner sits 19 chars under its own cap), which carries the shared-mutable-state rule and the coupling_justified escape hatch so first-pass plans avoid the finding when the coupling is unintentional. Documented the coupling_justified field in docs/reference/plan-md.md. Growth acks per #2914; inventory manifest and install-tree fixtures regenerated for the new reference file. Closes #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): pin Dimension 3b at severity: info The severity retag makes the old assertion (severity: warning) stale; lock the advisory tier from both directions — info must be present, warning must not — so a future edit cannot silently re-arm the revision-loop trigger. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * chore(#3724): changeset fragment for PR #3758 Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * docs(#3724): roster planner-coupling.md in docs/INVENTORY.md The new reference was enumerated in the manifest and all 19 install-tree fixtures but missing its row in the Modular Planner Decomposition table — the roster half the manifest-sync test cannot check. (Review Blocker.) Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): cover all four acceptance criteria (review round 1) - plan-checker-coupling: the 3b severity assertion is now a PARITY check deriving the exempt tier from revision-loop.md's flow instead of hardcoding info — editing either side alone reds the suite. New describe pins the other three criteria: plan-phase's INFO-only accept clause (proven failing-first), the BLOCKER + WARNING count staying intact, the coupling_justified Do-NOT-flag exemption + fix_hint, and the planner pointer + planner-coupling.md content. - ack fragment: $comment's plan-phase figure corrected to +79B; the 2775 pin note carried forward into the gsd-planner.md entry, updated for upstream's #3761/#3764 Rule-paragraph anchor (which this diff leaves verbatim). The parallel-dependent-plans re-anchor this commit originally carried was superseded by upstream #3764 during review; this branch no longer touches that file. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 2 — align the stance enumeration, complete the template contract MAJOR: <adversarial_stance>'s severity enumeration gains the INFO bullet so it agrees with Dimension 3b's 'ALWAYS INFO' mandate instead of contradicting it. Funded by extracting the inline <examples> block to the new progressive- disclosure reference gsd-core/references/plan-checker-examples.md (@-inlined from the same spot; #1949 precedent), which also restores the 3b motivation clause round 1 traded away (Nit 4) and nets the agent file SMALLER than base (49107 -> 48486) — the extraction the byte pressure was owed. MINOR: gsd-core/templates/phase-prompt.md now carries coupling_justified, and the field's shape becomes one 'plan-id: reason' string per coupled peer so a plan justified against two peers can express it; docs/reference/plan-md.md's Type column names the shape. NIT: the 3409 ack's plan-phase entry no longer calls the #1168 workflow ratchet an 'XL tier'. Acks and derived artifacts updated accordingly (checker entry removed — a shrink needs no ack; INVENTORY roster row + regen:derived for the new file). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): derive the 3b negative severity assertion (review round 2) Every severity token in the 3b span must BE the tier revision-loop.md exempts, replacing the hardcoded severity:warning negative — if the loop's exemption ever moves, the failure names the real conflict instead of blaming the agent file with a mutually-unsatisfiable pair. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): refit the planner coupling pointer under the char cap Upstream #3299 (PR #3390) grew agents/gsd-planner.md to 49146 chars at the base, leaving 5 chars of headroom where the +16-char pointer was measured against 13 more. The pointer prose shortens to 'Non-file coupling:' — 49150 chars, back under the strict 49152-char cap — and the ack figures follow. The @-path the tests pin is unchanged. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): re-home the plan-phase ack after the #3823 spent-fragment sweep Upstream #3078/#3823 deleted all fully-spent ack fragments, including 3409-unreachable-guard-arms.json, which carried this PR's plan-phase.md +79B append. Per the collision remedy that sweep added: take the deletion and home the still-live entry in this PR's own fragment. Figures re-measured at this merge base (90871 -> 90950 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): absorb the spent #3172 plan-phase fragment into this PR's ack Upstream #3825 shipped 3172-stated-failing-direction.json naming only plan-phase.md, now spent at the base — colliding with this PR's live plan-phase entry. Per the #3003 pattern the fully-spent single-path fragment is deleted and this fragment stays the path's one source; figures re-measured at this base (93073 -> 93152 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 3 — true up the ack figures, restore the wave comment The fragment's absolute sizes are re-measured and anchored to base e40e9670 (planner 47259 -> 47330 chars, checker 45537 -> 44916 B, plan-phase 91186 -> 91265 LF bytes), with a note that absolutes rot as next moves — the deltas are the durable claims. The round-1 removal of the '# Implicit dependency: files_modified overlap forces a later wave.' pseudocode comment offset headroom base drift had already returned, so it is restored (findings 2-3). Changeset gains the (#3724) backlink (finding 4). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 4 — close the verify-work surface, harden the boundaries BLOCKER: verify-work.md's verify_gap_plans is the second multi-plan consumer of the checker's sentinels, and its ISSUES FOUND handler entered revision_loop with zero severity parsing — the guaranteed replan #3724 fixed in plan-phase, alive on the gap-closure surface. The handler now counts BLOCKER + WARNING and accepts INFO-only returns with advisories displayed. The checker's INFO stance bullet is reworded to the claim that is true everywhere ('revision gates count only BLOCKER + WARNING'). Minor 1: plan-phase's iteration_count >= 3 arm recounts severities, so an INFO-only third check accepts instead of halting on a '0 issues remain' user gate. Minor 2: the coupling_justified exemption now requires the entry to NAME the other plan, closing the blanket-suppression reading. Nit 1: INVENTORY row states the extraction buys cap headroom, not context. Nit 2: the advisory display gains a concrete format on both surfaces. Ack fragment re-anchored at base ddde001a: verify-work.md +264B (new entry), plan-phase.md +395B, checker still net negative (-512B). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): pin the verify-work accept and the iteration-cap boundary (review round 4) Two wiring assertions: verify_gap_plans' ISSUES FOUND handler gates on BLOCKER + WARNING and accepts INFO-only blocks, and plan-phase's iteration_count >= 3 arm recounts severities instead of gating advisories — the limit+1 boundary of the gate this PR fixes. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 5 — fail closed at the gates, surface the advisory Blocker 1: the checker's step-10 status rule routes an INFO-only result to ## ISSUES FOUND (with a new ### Advisories (info) template section and a severity-aware recommendation) so the orchestrator receives the block and displays the advisory instead of silently accepting a bare PASSED. Blockers 2+3: all three gate surfaces (plan-phase step 12 both arms, verify-work verify_gap_plans) carry one canonical clause verbatim — an entry whose severity is missing or unrecognized counts as a BLOCKER (fail closed) — making the accept condition an explicit-INFO whitelist while keeping issue_count coherent for stall math. Major 1: the INFO stance bullet scopes its claim to the plan-phase and verify-work gates (quick mode's loop still revises on any ISSUES FOUND). Major 2: INVENTORY row and ack $comment state the extraction's real trade (readability, +0.6 KB eager runtime context), not a cap remedy. Minor 1: plan-md.md marks coupling_justified as prompt convention, unvalidated. Nit 1: ack absolutes re-anchored at base 1e67ec97; checker now +120B and acked. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): pin the round-5 contract — fail-closed parity, INFO-only return shape New: three-surface verbatim parity test for the fail-closed clause (Blockers 2+3); checker return-contract test for the INFO-only ## ISSUES FOUND route and advisories section (Blocker 1). All seven newly pinned tokens are absent at f3a5682d, so each new assertion fails pre-fix. Updated: accept-clause regexes track the explicit-INFO whitelist wording; the severity sweep scopes to the span's fenced yaml examples via yamlSeverityTiers (round-5 Minor 3, applied to the blocker negative too); the iteration-cap comment states it is a prose pin, not an executed boundary check (Minor 4); splitLines call sites document the line-pin coupling (Nit 2). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): adopt next's line wrap in the 3b motivation clause — drops a wrap-only hunk from the diff Byte-identical content; the wrap difference was an artifact of the round-1 base adaptation predating upstream's #3003 landing. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 7 — gate every checker consumer, not just the two audited ones Blocker: quick/steps/plan-checker-loop.md (issue-named in #3724) gets the same canonical fail-closed clause and explicit-INFO whitelist accept as plan-phase/verify-work — an INFO-only result proceeds instead of entering quick mode's revision loop. Major: import.md plan_validate handles the checker return by severity (INFO-only never blocks an import) and is added to agent-contracts.md's consumer enumeration, which had omitted it. The checker's INFO stance bullet drops the quick-mode carve-out — the claim is universally true again now that every consuming gate is severity-aware. Minor: an applied coupling_justified exemption is surfaced as its own info advisory so a stale one-sided declaration stays observable. Nit: plan-phase's revision-iteration Display line is explicitly conditioned on not having already proceeded to step 13. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM Emitted-Drift-Ack-Growth: import.md — #3724 round 7: the plan_validate step's checker-return handler becomes severity-aware — counts BLOCKER + WARNING failing closed and accepts an explicitly-INFO-only return with advisories displayed instead of blocking the import * test(#3724): pin the round-7 surfaces — five-gate parity, quick/import accepts, exemption visibility The verbatim fail-closed parity test extends to quick/steps/plan-checker-loop.md and import.md plan_validate; new assertions pin quick mode's INFO-only proceed, import's never-blocks accept, import.md's presence in agent-contracts.md's consumer row, and the surfaced coupling_justified exemption advisory. All four newly pinned token families are absent at the pre-fix head, so each new assertion fails first. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-31 19:08:11 -04:00
Tom Boucher	e242d85c92	fix(#4109): rewrap unquoted DISPATCH_SLUGS-shaped consumption to survive zsh (#4116) ... * test(#4109): add zsh regression coverage for gate-check and invoke_reviewers dispatch Extends tests/review-plan-coverage-manifest.test.cjs to cover the two DISPATCH_SLUGS consumption sites the existing #3301 coverage-check tests don't reach: the write_reviews gate-check block (ALL_LANES_SKIPPED / TOTAL_LANE_FAILURE counting) and invoke_reviewers' dispatch + join loops. Both extract the real shipped bash from review.md and run it under bash and zsh, same convention as the existing coverage-check rows. Expected RED under zsh pre-fix, GREEN post-fix. * fix(#4109): rewrap unquoted DISPATCH_SLUGS-shaped consumption to survive zsh Bash word-splits an unquoted scalar on IFS by default; zsh does not (no setopt SH_WORD_SPLIT anywhere in these files), so a scalar accumulated from multiple space-separated tokens and then consumed via bare `for x in $VAR` collapses onto one bogus iteration under zsh whenever it holds 2+ tokens. Fixes all four review.md sites reported by #4108/#4109 (invoke_reviewers dispatch + join loops, the gate-check block, and the #3301 coverage-check block), plus the identical pattern found by a repo-wide sweep in complete-milestone.md, code-review.md, pr-branch.md, sync-skills.md, and execute-phase/steps/per-plan-worktree-gate.md. Each site is rewrapped in unquoted command substitution (`$(printf '%s' "$VAR")`), which re-splits identically under both shells regardless of SH_WORD_SPLIT — the same mechanism that already made the accumulator-building loops in these files shell-safe. * test(#4109): warn loudly when the zsh probe fails instead of skipping silently detectShells() drops the zsh test lane whenever a live zsh probe fails (e.g. on a CI runner without zsh installed), and a dropped lane reads identically to a passing one in the suite's own output — exactly the blind spot that let #4109's bug class ship undetected. Both copies of this helper (this file and its byte-identical duplicate in review-build-prompt-optional-sections.test.cjs) now emit a greppable console.warn when the probe fails, so a run without zsh reads as "zsh coverage unknown" rather than "all lanes green". * ci(#4109): gate workflows/*.md's embedded bash blocks in lint:ci Adds scripts/lint-workflow-shellcheck.cjs, wired into npm run lint:ci, so this bug class can't land undetected a third time. Two independent checks run over every ```bash block in gsd-core/workflows/**/*.md: - ShellCheck (new `shellcheck` devDependency, downloads and caches the real koalaman/shellcheck binary) catches the general unquoted-expansion/ word-splitting family in argument position. 212 pre-existing findings across the tree are absorbed into scripts/lint-workflow-shellcheck-baseline.json (matched on {file, code, message}, not line number, so unrelated edits elsewhere in a file can't spuriously un-baseline anything) — fixing all of them is out of scope for this issue; only NEW findings fail the build. - A custom structural check specifically for #4109's own shape: ShellCheck does not flag a bare `for x in $VAR` word-list — it treats that as an intentional idiom under any ruleset (confirmed empirically). This check does, and gates the build on any occurrence, with zero tolerance (no baseline) since every known site was already swept and fixed on this branch. * test(#4109): update pr-branch cherry-pick loop test anchor for the zsh fix extractPickLoop()'s PICK_LOOP_MARKER located the create_pr_branch cherry-pick loop by the literal substring "for HASH in $INCLUDED_COMMITS", which no longer appears verbatim after this issue's fix rewrapped that loop in unquoted command substitution. Updates the anchor (and its error message, now derived from the same constant instead of duplicating stale text) to the new literal form. No behavior change to the extraction logic itself. Emitted-Drift-Ack-Growth: review.md — #4109's fix adds explanatory comments at 4 sites; net code is functionally equivalent, comment expansion grows the file Emitted-Drift-Ack-Growth: complete-milestone.md — #4109's fix adds explanatory comments at 3 sites documenting the zsh word-splitting divergence Emitted-Drift-Ack-Growth: code-review.md — #4109's fix adds an explanatory comment documenting the zsh word-splitting divergence Emitted-Drift-Ack-Growth: pr-branch.md — #4109's fix adds explanatory comments at 3 sites (TRANSIENT_DIRS, INCLUDED_COMMITS, FILTER_PATHS) Emitted-Drift-Ack-Growth: sync-skills.md — #4109's fix adds explanatory comments at 2 sites (CREATE_LIST/UPDATE_LIST, REMOVE_LIST) * test(#4109): cover lint-workflow-shellcheck parsers + add timeout Adds tests/lint-workflow-shellcheck.test.cjs covering the hand-rolled parser/logic functions exported by scripts/lint-workflow-shellcheck.cjs (stripCommandSubstitutions, stripShellComments, substitutePlaceholders, extractForLoops, findBareForLoopSplits, findingKey, partitionAgainstBaseline) that shipped with zero coverage — CLAUDE.md requires at least one fast-check property test for parsers, included here (bare/braced forms always flagged naming the variable; quoted/substituted/literal forms never are). Also bounds runShellcheck's subprocess with a 60s timeout: the `shellcheck` npm package's own API has no timeout option and internally blocks on a synchronous spawnSync, so this reimplements the binary resolve/download step via the package's own exported config/download and calls spawnSync directly with a native timeout. And guards the CLI entry point with `if (require.main === module)`, matching this repo's sibling dual-purpose lint scripts — without it, requiring the module for its exported functions (as the new test file does) also triggered a live ShellCheck run as a side effect. * docs(#4109): add changeset for the zsh word-splitting fix * chore(#4109): backfill changeset PR number (#4116) --------- Co-authored-by: sim <sim@local>	2026-08-31 18:08:06 -04:00
0xdhx	ef9ce3e598	fix(#3702): count asterisk, plus and ordered markers as deferred-items entries (#3739) ... * fix(#3702): deferred-items counts `*`, `+` and ordered markers as list items `deferred-items.md` has no template and no mandated shape, but its parser recognised only the `- ` hyphen marker. Asterisk bullets, plus bullets and dot-terminated ordered lists — all lists in CommonMark and GFM — contributed ZERO entries on both the headless and the heading-delimited path, and a mixed file dropped its non-hyphen entries while keeping their hyphenated siblings, under-reporting without ever looking empty. The restriction was a regex literal inherited from the Gaps seam, where the template genuinely mandates the hyphen YAML-lite form; nothing in the module's stated rationale distinguishes `*` from `-`. Widened on the deferred path only: - `splitGapsEntriesCore`'s entry opener, `extractGapEntryFields`' line-0 strip and `rawGapEntryText`'s line-0 strip take a `BulletMarkers` parameter that DEFAULTS to the hyphen-only set, so `## Gaps` keeps its template-mandated grammar byte-for-byte and the module still has exactly one grouping pass. - `splitDeferredHeadingEntries`' body-bullet test, `stripLeadingBulletMarker` and `acknowledgeDeferredItem`'s status-field regexes move in lockstep — widening what OPENS an entry without widening what is STRIPPED before field extraction would surface an entry that can never resolve. Unchanged, and pinned by tests: prose-only and bare headings still contribute nothing ("prose is not an item"); a table under a leaf heading still yields exactly its rows, since table lines are skipped before the body-marker flag can be set and a `|` row is not a list marker; the paren-terminated ordered form `1)` is out of this fix's scope. * docs(#3702): changeset fragment (pr: 0 placeholder pre-create) * fix(#3702): widen the forensic-audit prose entry rule to match the parser Sibling site of the same defect class, found by a defect-class sweep of the deferred-items consumers. `/gsd-progress` check 7 does NOT go through `gsd-tools query` — it globs `deferred-items.md` and has the model read entries by a prose rule that mandated "one entry per top-level `- ` line". Left as-is, the marker widening would hold on the CLI path while the one consumer that bypasses the parser kept reporting "No unresolved deferred items" for a file written with `*`, `+` or an ordered marker: the same false negative, surviving in the only place the fix could not reach by code. Also pass DEFERRED_BULLET_MARKERS explicitly where the heading path extracts fields. It was already correct — stripLeadingBulletMarker pre-strips the widened set from every line, so the default hyphen strip is a no-op there — but relying on that leaves a detection site and a strip site nominally on different marker sets, which is exactly the asymmetry the BulletMarkers doc comment warns about. Explicit is local; inferred is a trap for whoever edits the strip next. Out of scope, noted rather than fixed: forensic-audit.md globs only `.planning/phases/*/` and so misses archived milestone phases that `scanDeferredItems` covers. Pre-existing, a different defect, and not this issue's ruling. * docs(#3702): note the milestone-close halt for heading-shape non-hyphen files in the changeset A heading-delimited deferred-items.md written with */+/ordered markers previously parsed to zero and closed silently; it now yields entries whose heading shape acknowledgeDeferredItem refuses, halting complete-milestone until hand-edited. User-visible, so the fragment states it. * chore(#3702): set changeset fragment pr to 3739 * fix(#3702): CR-normalise the heading path and the acknowledge writer (review B1, M4, m2) B1 — `splitDeferredHeadingEntries` stored RAW lines; on a CRLF file every body line but the last still carried its `\r`, the `$`-anchored marker strip failed on it, the marker survived into field extraction and the field was lost — a `**Status:** resolved` that was not the file's final line resurfaced its entry as open. The heading path now stores CR-stripped lines like the headless path already did, and the strip regex tolerates a trailing CR on its own. Round 1's CRLF test put `**Status:**` on the last line, the one position `collectSection`'s `.trimEnd()` had already de-CR'd; the new tests put it first and mid-body. M4 (pre-existing on `next`) — `acknowledgeDeferredItem` found the status line on a CR-stripped copy but rewrote the raw line with a `$`-anchored `.*`, which cannot consume `\r`; `replace` returned its input, and the writer reported `ok` over byte-identical content. The rewrite now runs on a CR-stripped line. The comment that claimed `.*$` consumed the `\r` is corrected — it was the bug, stated as the design. m2 — the indent probe for an inserted `status:` line ran on the raw line and fell back to indent 0 on CRLF; it is CR-stripped too. * fix(#3702): derive every deferred-items marker regex from one source (review M3, N1, N2) M3 — round 1 carried the marker alternation in FOUR places: the `BulletMarkers` pair and two inline literals inside `acknowledgeDeferredItem`, under a doc comment saying the interface existed so a detection site and its strip site could not drift. All four now derive from `DEFERRED_MARKER_ALT`; drift is impossible rather than discouraged. A parity test pins the vocabulary against `markdown-sectionizer`'s `iterateBullets` on everything the two grammars are meant to agree on, and names the two points they deliberately differ. N1 — the ordered marker is `\d{1,9}\.` (CommonMark §5.2), not `\d+\.`. N2 — the marker is followed by `[ \t]`, not `\s`, which also accepted `\r`; the tab remains accepted (CommonMark-legal) and the divergence from `iterateBullets`' literal space is pinned rather than papered over. The four regexes are exported for the parity test only. * fix(#3702): an ordered marker opens an entry only from `1.` or inside a run (review B2, m1) B2 — `\d+\.` alone read ordinary prose as a list: "2026. was a bad year for this module" and, under `### Notes`, "3. is the number of retries we settled on." both opened an entry on round 1, the second straight through the "prose is not an item" contract that round's AC4 claimed to preserve. CommonMark §5.3 faces the same ambiguity when an ordered list would interrupt a paragraph and resolves it by requiring the list to start with 1; `matchListOpener` applies that rule wherever an ordered marker is seen, with the run carried per list (headless) or per leaf-heading body. Numbers after the first are ignored, as CommonMark ignores them. Stated cost, pinned: a hand-numbered list starting at 2 reads as prose — every ordered record in the #3702 scan starts at 1. Both reviewer cases are pinned as prose; the ruling's `1. alpha / 2. beta` shape still counts. m1 — the 9-digit boundary is pinned at both sides (`999999999.` opens, ten digits is not a marker), and the 3-vs-4-space indentation cliff is pinned as deliberately NOT applied: the parser is indent-lenient because surfacing a questionable hand-written entry beats dropping a real one. * fix(#3702): thematic breaks close the list and fenced code never opens an entry (review M1, M2) M1 — `- - -` was a phantom `"- -"` entry on base; widening the marker set added `* * *` and `+ + +` to the class, and `* * *` is the separator an author writing in the `*` style is most likely to use. A CommonMark §4.1 thematic break (plus the `+ + +` gesture, which is the same garbage as an entry name) now closes the open entry on the headless path and is dropped from the body on the heading path — neither an item nor a continuation. M2 — neither splitter was fence-aware, so `+ `-prefixed diff lines and `1.`-numbered repro steps inside a code block counted as entries; #3702's wild records carry exactly those blocks. Both splitters now classify lines by the sectionizer's own `scanFencedBlocks` (so `~~~`, indented and unterminated fences behave as `stripFencedCode` would): fence content never opens an entry, is continuation inside an open one — keeping the span invariant `acknowledgeDeferredItem` re-verifies — and is discarded before the first. * test(#3702): range the #2287 deferred-items property over marker × shape × line ending (review B3) The `#2287` property hard-coded `- ` and filtered `\r\n` out of its arbitraries, so the widened marker set — an enumerated domain, exactly what a property is for — was never under it. It now ranges over `{-, *, +, ordered}` × `{headless, heading}` × `{LF, CRLF}`, with the heading shape placing `**Status:**` first or last: the review's prescription (markers × line endings) would not have reached B1, which lives on the heading path only, so the shape axis is the load-bearing addition. Ordered entries are numbered from 1, so the B2 run rule is under the property too. A second property drives `acknowledgeDeferredItem` over every unresolved headless entry across the same marker × line-ending grid — the one that reaches M4 (a CRLF rewrite that reported `ok` and wrote nothing) and m2. * test(#3702): pin the milestone-close halt on a heading-delimited `*`/`+`/`1.` file (review m3) A heading-delimited `deferred-items.md` written with a non-hyphen marker previously parsed to zero entries and let `complete-milestone` close silently; it now yields entries whose heading shape `acknowledgeDeferredItem` refuses, which the milestone loop turns into `record_ack_failure` → exit 1. The loop is prose in a workflow, so the test drives the two CLI calls it makes: `audit-open --json` must list the entry, and `audit-open acknowledge --text <the audit's own text>` must refuse with the heading-delimited message and write nothing. * docs(#3702): changeset and forensic-audit prose carry the round-2 grammar The changeset names the CRLF fixes, the ordered start-at-1 rule, thematic breaks and fences. The `/gsd-progress` forensic-audit step is the one prose parser of this file and must state the same grammar the code has. * fix(#3702): round-review refinements — run ends at a paragraph, rejected ordinals unstripped, breaks at any indent, fenced fields, `## Gaps` scope Findings from the pre-push adversarial review of round 2, each pinned: - An ordered run ENDS at a paragraph that follows a blank line (CommonMark §5.3); a non-indented line with no blank before it is lazy continuation and keeps the run open. `1. a` / blank / `paragraph` / blank / `5. x` is one entry, not two. - The heading path strips the marker off every body line before field extraction (#3457); a line whose ordinal `matchListOpener` REJECTED must not be stripped, or "3. status: resolved" as prose loses its `3. ` and reads as a resolved field. `splitDeferredHeadingEntriesDetailed` now carries a per-line opener flag and only accepted openers are stripped — in headless regions of a heading-shaped file too. - A thematic break is recognised at any indent, matching the parser's indent-lenient reading of items; ` * * *` was a phantom `* *`. - Fenced lines carry no FIELDS either: a `status: resolved` quoted inside a code block no longer resolves its entry on either path. - Block structure (breaks, fences) is a property of the GRAMMAR, carried as `BulletMarkers.blockStructure`: the deferred set opts in, the Gaps set does not, so `## Gaps` is byte-for-byte on its `next` behaviour — the round-2 M1/M2 change had reached it through the shared splitter. * test(#3702): the property exercises the rejected-ordinal branch; the N2 control is independent Round review: the widened #2287 property numbered every ordered run from 1 and so never generated an ordinal the start-at-1 rule rejects — it could not tell round 1 from round 2 on B2. Each entry may now carry a decoy prose line beginning with a non-1 ordinal, placed where it cannot end a run (before the first headless entry; first in a heading body), followed by a `status: resolved` that must never become a field; and a decoy-only heading body must yield no entry. The N2 assertion accepted a tab, which round 1's `\s` accepted too, so a `[ \t]` → `\s` revert alone stayed green. NBSP, form-feed and vertical-tab are now asserted refused — the assertion that fails on that revert on its own, and the disclosure that `[ \t]` narrows what round 1 accepted. * fix(#3702): the splitter records its own opener flags; an opener clears the blank-line memory Round-review continuation, two state defects in the ordered-run logic: - `blankSeen` survived the headless splitter's opener branch, so an opener followed by a lazy continuation line read as "paragraph after a blank" and ended the run — `1. a` / blank / `2. b` / lazy / `3. c` folded `c` into `b`. The opener branch now clears it. - The heading path re-derived per-line opener flags for headless regions without the paragraph reset, re-accepting a rejected `3. status: resolved` under a stale run and stripping it into a field. `GapsEntrySpan` now carries the flags the splitter itself computed, and the heading path reads them; the re-derivation is deleted. * fix(#3702): ordered-run memory is per indent — nested runs resolve, nested ordinals never inherit the top-level run Round-review continuation 2: nested openers consulted the TOP-LEVEL run flag and never wrote their own, so a nested `1. / 2.` run under a hyphen entry rejected its `2. status: resolved` (round 1 resolved it), while a nested `3. status: resolved` under a nested `- ` bullet inherited an open top-level run and was stripped into a false field. `OrderedRuns` keys the memory by indent: a new opener at indent d resets every deeper level, a paragraph after a blank at indent d ends the runs at d and deeper, a thematic break or a heading clears all. Both splitters use it; the top level still decides entry boundaries, nested levels decide only which continuation lines are accepted openers for field stripping. Pinned for LF and CRLF. * fix(#3702): run levels — one top level at or above the base, CommonMark column indents, a fence ends its level's runs Round-review continuation 3: - A dedenting top-level list (` 1.` / ` 2.` / `3.`) lost its entry boundaries: the exact-indent run lookup rejected the shallower ordinals before the boundary check ran. Every indent at or shallower than the list's base is now ONE level, in both splitters. - `indentOf` counted characters, so a tab and a space aliased to one level and `\t1. nested` / ` 2. status: resolved` resolved falsely. Indent is now measured in CommonMark columns (§2.2: a tab advances to the next multiple of 4), for the run level and the entry-boundary check alike. - A nested run survived a fenced block. A fence is a non-list block: its opening delimiter ends the runs at its level and deeper, exactly as a paragraph after a blank does. * fix(#3702): the indent measure is grammar-scoped — Gaps keeps next's character count `blockStructure: false` promised the Gaps grammar byte-for-byte parity with `next`, but the CommonMark-column indent measure added for the deferred grammar was shared by the whole splitter core, so tab-indented Gaps input changed entry boundaries in BOTH directions: `\t- a` / ` - b` — next folded into one entry, HEAD split into two ` - a` / `\t- b` — next split into two, HEAD folded into one `indentWidth` now keys the measure on the grammar: columns for the deferred set, raw character count for Gaps. The opt-out covers indent semantics, not only fences and thematic breaks. Four cases pin both halves — the two flipped Gaps pairs, the two Gaps pairs that never moved, and the same tab/space pairs on the deferred path returning the opposite (column-measured) verdict by design. * fix(#3702): the acknowledge path reads and writes through one classifier Round 3, Blockers 1 and 3, and Minors 7 and 8 — one mechanism, so one commit. Every consumer of an entry's lines now reads the splitter's own per-line verdict instead of a re-derivation of it. B1. Round 2 widened the WRITER's status-line finder to the deferred marker set while `extractGapEntryFields` still de-bulleted line 0 only. A nested ` * status: pending` was therefore selectable by the writer and invisible to the reader: acknowledge rewrote it in place, returned `ok`, and the item stayed outstanding on every later audit. Measured against a `next` build, `*`, `+` and `1.` each resolved on base and stopped resolving at round 2's head — a regression, not a gap in new behaviour. The hyphen form of the same shape was already broken on `next` and is fixed here too: one classifier cannot be right for three markers and wrong for the fourth. `parseGapEntryFieldLine` is now the single place a line is classified as a field, and it reports the offset at which the VALUE begins. The rewrite happens at that offset rather than through a second regex, so a line the classifier can select is one whose rewrite it has already located — the selection and the rewrite cannot disagree. Both `DEFERRED_STATUS_FIELD_RE` and `DEFERRED_STATUS_REWRITE_RE` are deleted rather than widened. A read-back guard returns `rewrite_not_readable` rather than `ok`; it is unreachable by construction today and is the fail-loud floor under the next divergence. B3. This is the end state the round-3 review prescribed on both #3739 and #3773: #3773's shared classifier, parameterised by this PR's marker set, with this PR's two status regexes deleted. #3773 lands first. Its hyphen-only strip is consistent with `next`'s hyphen-only splitter today, so the writer/reader divergence is created by THIS merge, which is why widening every consumer belongs to the PR that widens the domain. m7. The heading path marker-stripped its lines before calling the reader, so the reader's fence scan ran over text the splitter never saw: `- ```sh` is an ordinary bullet to the splitter but strips to a fence opener, and a `**Status:** resolved` after it was suppressed as fence content — a resolved entry resurfaced as open. Stripping now happens inside the reader, after the fence scan. m8. `rawGapEntryText` stripped a marker off line 0 unconditionally, but on the heading shape line 0 is the heading TEXT: `### 1. Race in the writer` was silently renamed to `Race in the writer`, and the name is the key acknowledge matches on. Line 0 is stripped only when the splitter accepted it as an opener. Also removed: `splitDeferredHeadingEntries`, whose sole caller only null-checked it (round 3, M4 — the claim was zero callers, which was wrong; the wrapper's `.map` was waste at the one call site), and `stripLeadingBulletMarker`, which this change leaves with no callers at all. The export surface narrows to the two splitter regexes the behavioural parity test reads (M6). [PEER-ASK pr-order-12d5] q: Reviewer blocked both on merge order. I'm declaring #3773 lands first and building the end-state shape into #3739 now (both my status regexes deleted). Does that match your plan? reply: CONFIRMED - same order, derived independently. #3773 cannot carry the fold: `DEFERRED_BULLET_MARKERS`/`BulletMarkers` have zero occurrences at `next` (verified), so the prescribed end state is not executable inside #3773 without absorbing this PR's work. deadline: 03:55 UTC (answered before it) fallback: declare #3773 first, adopt end-state shape in #3739, push+comment decision: proceeded as stated; #3773 lands first, this PR carries the widening of every consumer. Refs #3740 * test(#3702): pin the detect/strip symmetry, and drop a white-box test that could not reach it Round 3, Blocker 2 and Minors 6 and 9. B2. The regression shipped green because no fixture put a marker on a nested status line. Four markers x {nested status line}, each asserting the entry READS BACK as acknowledged rather than that acknowledge merely reported `ok` — reporting `ok` over a line the reader skips is the whole defect. Plus the bare capitalised `Status:` case (the reader stores it case-sensitively, so the writer must not select it), and an idempotence test, which is the failure the defect actually produced: the item resurfaces, is acknowledged again, and never settles. Each of these was run against the pre-fix build first: all five fail there and pass here. Two further assertions in the block are labelled CONTROL because they held pre-fix — they guard the new offset-based rewrite and the opener-flag threading against regressing, and calling them regression tests for a reported defect would overclaim. M6. The round-2 parity test asserted that four writer-side regexes embedded the same source string. That is true of a detect/read asymmetry too, so it could not have caught B1 — and two of the four regexes were widened into `export =` purely to let it read them. Replaced with a behavioural test that drives the real seam: every marker that opens an entry must also resolve it through acknowledge. The structural assertion is kept for the two splitter regexes, which really are two copies of one alternation. m9. `expectedResolved` was computed and immediately voided; the loop beneath it already asserts both polarities. m7/m8 coverage lands here too: a bullet whose content is a fence opener must not suppress the entry's fields, and a heading beginning with a list marker must keep it in the entry name. * docs(#3702): document the deferred-items entry shape where the file is written Round 3, Major 5, and #3702's own item 2. The widened grammar was documented in the reader (`forensic-audit.md`) but not at the write site, where `executor-examples.md` still said only "log to deferred-items.md" — so the question the issue actually raised, which shapes count, remained unanswered anywhere a human writes the file. States what opens an entry (`-`, `*`, `+`, and `1.` when the list starts at `1.`), that `1)` is not a marker here, that a separator closes the list and fenced content is never an entry or a field, and that an entry without an explicit `status: resolved` stays open by design. * chore(#3702): regenerate the changeset through the generator Round 3, Minor 10. The fragment was hand-named against 64 generated names on `next`, and its body ran ~250 words against CONTRIBUTING's one-sentence form. Regenerated via `npm run changeset`, which is also what the random three-word name is for: concurrent PRs never collide. * fix(#3702): the fence gate lives on the seam both sides call, not just the reader Found by the pre-push adversarial review of this round, and it is a regression this round introduced rather than a pre-existing one. `extractGapEntryFields` applied `fencedLineSet` before classifying; the acknowledge writer's status-line search did not. So a `status:` line inside a fenced block was SELECTED by the writer and SKIPPED by the reader — the write produced a line nothing reads, the read-back guard refused it, and the entry became impossible to acknowledge at all: `audit acknowledge` raised an internal error and `complete-milestone` halted on it. Measured, `- alpha` / fence / ` status: pending` / fence: next ack=ok -> reads back "acknowledged" round-2 head ack=ok -> reads back "" (the B1 defect) before this ack=rewrite_not_readable -> refuses entirely (worse than next) `entryFieldLines` is now the seam — per line of an entry, the field it declares or `null`, fences included — and the reader and the writer both go through it. That makes "the writer cannot select a line the reader will not read back" structural rather than asserted, which is what the previous commit's message claimed while a second read-side filter still lived outside the classifier. Two comments corrected with it. The read-back guard is NOT "unreachable by construction": this round shipped a reachable path to it, which is precisely what an invariant asserted in a comment is worth. And the M6 replacement test put its marker only on the entry opener, so it passed against the defective build — the exact weakness it was introduced to fix in round 2's test. It now marks the nested status line too, and fails pre-fix like the rest. Round-3 tests against the pre-fix build: 10 of 12 fail there, and the 2 that hold are labelled CONTROL because they guard this round's new code rather than pin a reported defect. * fix(#3702): one end-of-file CRLF algorithm, adopting #3773's with its B4 closed Round-4 M1. Two open PRs shipped two different answers to "what line ending does an entry that ENDS THE FILE get?", and the review's ruling was that the disagreement needs one answer, not two. Neither shipped answer was that one. Measured on builds of both heads: case #3739 r3 #3773 here undelimited single entry, CRLF preamble pass FAIL pass LF-dominant list, one stray CRLF at EOF FAIL pass pass (the other five) pass pass pass This PR's content.endsWith('\r\n', matchIndexInContent) reads the terminator of the PREVIOUS line, so it propagated an isolated CRLF into an LF-dominant list -- refuted by #3773's own LF-dominant fixture, ported here. Withdrawn. #3773's crlfAtEof asks the right question -- does anything before the entry, within scope, contradict CRLF -- and fails closed. But its scope goes EMPTY for an undelimited single-entry list, because the entry-list region runs from the first entry's start to the insertion point and those coincide; crlfAtEof('') is false by its own before.length > 0 guard, so 'preamble\r\n\r\n- alpha' gained a bare \n in a CRLF document. That is #3773's B4, verified by driving its head. Adopted here with the scope widened to everything preceding the insertion point where the preferred region is empty, rather than asserting LF from no evidence. That only ever loosens a scope carrying zero information, and the predicate stays fail-closed over the wider one. An entry at offset 0 of an undelimited document has no evidence under either scope and stays LF. Tests: 10 added. Negative control, driven -- 1 of the 10 fails against this branch's own pre-fix head (the stray-CRLF fixture); B4 fails against #3773's head; the remaining 8 are the scope counterexamples ported with the function, which were regression pins in #3773 and are guards here. Each still kills a simpler algorithm: drop any one and a refuted scope passes again. Four deferred-items suites 450/450, 0 skipped. npm run lint:ci exit 0. * fix(#3702): drop the unreachable rewrite_not_readable guard (B3) Round-4 B3: the status had zero test coverage in either file. The review offered two branches -- drive it from a test, or delete it and stop carrying an untested terminal status. Taking the second, with the reason stated rather than assumed. Why it cannot be driven. Round 3 added the guard after a fenced `status:` line proved the writer could select a line the reader would not read back. Round 3 then closed that divergence STRUCTURALLY, by routing the writer's line selection and the reader's field extraction through one entryFieldLines seam. The guard now detects a state construction prevents: 21 document shapes were driven against it -- fence openers on the bullet line for every marker in the widened set, duplicate and triplicate status lines, bolded and nested variants, fences between duplicates -- and none reached it. The only seam that would is routing the internal call through the module's exports so a test could stub it, which reshapes production surface for a test. Why leaving it undriven is not free. RULESET.TESTS.mutation-score runs Stryker incrementally over changed files at an 80% threshold and says to treat a surviving mutant as a failing test specification. An undriven `if` on a changed file is exactly that, on both the condition and the .toLowerCase() comparison. What this gives up, stated rather than hidden: if a future change re-splits the writer's selection from the reader's extraction, acknowledgeDeferredItem returns ok over an item that stays outstanding -- the original #3702 defect class. One correction to the review's framing: match_verification_failed does NOT backfill it. That check runs BEFORE the write and compares the matched span to the target, so it cannot see a post-write read-back failure. The protection against re-splitting is the shared seam and the round-3 tests that pin it, not a runtime assertion. A comment at the removal site records all of this. Removing it also drops the union member from both files, which resolves the PR body's internal contradiction (it claimed no type-signature changes while adding one) and the duplicate-status surface #3773 collides on. No test changed behaviour: 450/450 across the four deferred-items suites, 149/149 across the audit suites, npm run lint:ci exit 0 -- the same figures as before the removal, which is itself the evidence that nothing exercised the branch. * fix(#3702): the deferred fence gate is indent-unbounded, like the rest of the grammar (M2) Round-4 M2. scanFencedBlocks is CommonMark, which caps a fence delimiter's indent at three spaces -- a fourth makes it an indented code block instead. This grammar had already opted out of that cliff for entry openers ([ \t]*) and for THEMATIC_BREAK_RE (^[ \t]*), but not for fences. So a fence at four spaces was not a fence to the gate, and a `status: resolved` line inside it RESOLVED the entry containing it. That is not an exotic shape. A fenced block written under a NESTED bullet sits at four spaces, so ordinary hand-written deferred-items.md files reach it. Driven before the fix at indents 4, 5, 8 and a leading tab: all four silently resolved. It is the #3702 silent-resolution defect class in a new place. gsd-core/references/executor-examples.md, added by this PR, states flatly that "nothing inside a fenced code block is an entry or a field". The review offered fixing the parser or bounding that claim in three places. Fixing it -- the claim is the one users will rely on, and the grammar had already chosen unbounded indent everywhere else. NO second fence dialect (the rule blankIndentedFenceDelimiters states). The classification is still done by scanFencedBlocks, the one exported CommonMark state machine, over a de-indented VIEW of the same lines. Run lengths, backtick vs tilde, closer-must-match-and-not-trail, info-string rules and the unterminated-at-EOF case remain that engine's answers. Indent is the only dimension hidden from it, and it is exactly the dimension this grammar has already declared it does not measure. Index alignment is 1:1 -- map preserves length -- so every returned line index still addresses the original line. Scope is the deferred grammar only. Both marker-parameterised call sites gate on markers.blockStructure, which the Gaps set does not set, so Gaps reaches an empty set. Verified, not asserted: the 47-fixture Gaps differential (marker x line-ending x separator x fence x break x key-shape x list-shape) is BYTE-IDENTICAL across this change, 8033 bytes both sides. Tests: 14 added, of which 8 fail against the pre-fix source and pass here; the other 6 are the deliberate controls -- indents 0 through 3, which must NOT move, and the Gaps opt-out guard. Four deferred-items suites green; the 58 suites touching uat/deferred/sectionizer run 6045 tests with an IDENTICAL failing set before and after this change (17 pre-existing environment failures -- installs and an unpinned GSD_EMITTED_BASE; emitted-attribution passes 259/259 in isolation with its base pinned). lint:ci exit 0. * fix(#3702): changeset, both prose parsers, and the minors (M3, M4, m1-m3, m5, n1-n2) M3 -- the changeset omitted a user-BREAKING change. Measured against next: a heading-delimited deferred-items.md written with `*`, `+` or `1.` went from "0 entries, so complete-milestone has nothing to acknowledge and closes" to "1 entry, the CLI writer refuses the heading shape, ACK_FAILURES accumulates, exit 1". The `-` form already halted and is unchanged. That is release-note material: a close that used to succeed now fails, and the correct response is to fix the file, not revert. Also names the fence-indent fix below, and adds #3740 so #3773's issue is attributed here as it is absorbed. M4 -- gsd-core/workflows/progress/steps/forensic-audit.md is a SECOND, model-executed parser of the same grammar, and prose cannot carry a parity test. Its widened text stated the start-at-1 rule, fences and separators but not the `1)` exclusion nor the nine-digit ordinal cap, both enforced in code with pinned tests. Both stated now, along with the round-4 fence-indent rule. (No ack fragment: the size ratchet's currentSizes does a NON-recursive readdirSync of gsd-core/workflows and agents, so a file under workflows/progress/steps/ is outside its scope -- verified by reading the helper, not by the green.) n1 -- executor-examples.md documented that the BOLDED status key is matched case-insensitively and left the bare key's rule to inference. Driven: bare `Status: resolved` is NOT read, so the entry stays open with no warning, while `**Status:**` is. Stated explicitly, with the digit cap and the any-indent fence rule (n2). m1 -- boundary coverage was 2/3. limit (999999999.) and limit+1 (1234567890.) were pinned; limit-1 (12345678.) added, per RULESET.TESTS.boundary-coverage. m2 -- THEMATIC_BREAK_RE and the tab-expanding indent counter are hand-rolled CommonMark rules with no in-repo peer to compare against, so the parity assertion is against the SPEC: eight positive and five negative fixtures, plus the two DELIBERATE divergences pinned as deliberate (`+` is a separator here but not in CommonMark, because `+` is a list marker in this grammar and `+ + +` would otherwise be a phantom entry; indent is unbounded). One fixture was initially wrong -- `-- -` IS a CommonMark break, since the spec allows free spacing between the three characters -- and the parser was right. m3 -- the result union is hand-duplicated in audit.cts as part of a deliberate structural view of uat.cjs, so the fix is not to delete a copy but to make drift observable. Every REACHABLE status is now driven from a fixture; four of the six (ambiguous, unsupported_heading_shape, already_resolved, match_verification_failed) had no assertion anywhere in the suite before this. match_verification_failed is still undriven and the test says so rather than omitting it. m5 -- DECLINED, with the measurement. The review is right that `(\s*)` in the opener and `/^[ \t]*/` in the reader disagree about \f, \v and NBSP, but its prescribed narrowing was implemented, driven and REVERTED: as shipped, an entry indented with any of those surfaces, parses its status field, acknowledges, and reads back acknowledged -- a complete round-trip. Narrowing turns all three into SILENTLY DROPPED entries, which is the #3702 defect class itself and the opposite of this file's stated fail-safe rule. A latent inconsistency in the safe direction is not worth a live regression in the unsafe one. Pinned by three round-trip tests so the prescription cannot be re-applied silently; if it is ever closed, the direction is to make the readers agree with the opener, not to make the opener reject lines it accepts today. Four deferred-items suites 475/475, 0 skipped. lint:ci and lint:changeset exit 0. The 47-fixture Gaps differential is byte-identical at 8033 bytes. * fix(#3702): the pinned `## Gaps` phantom now cites its issue (m4) Round-4 m4. The second assertion in the Gaps byte-for-byte test pins a real defect as expected output: a spaced hyphen thematic break in `## Gaps` is read as an ITEM, so `- - -` surfaces a phantom open gap named `- -`. Reproduced on pristine next at 389bc86e0 across nine separator shapes -- every spaced hyphen form is affected, `---`/`----`/`* * *`/`___` are not, and the dividing line is a space after the first hyphen (the Gaps opener is /^(\s*)(-)\s/ with no thematic-break concept at all). Filed as open-gsd/gsd-core#3898. The pin stays: scope-limiting Gaps is the point of the blockStructure opt-out, and this assertion is the only thing that would notice the Gaps path moving. What was missing was the tracking -- a pinned defect with no issue behind it reads as intended behaviour to the next reader. The comment now says which it is and what the expectation becomes when #3898 lands. * fix(#3702): an unterminated fence runs to the end of its entry, never past it (B1, B2) Round 4 de-indented every line before `scanFencedBlocks`, so a fence opened at any indent — and `scanFencedBlocks` runs an unterminated fence to end-of-document — so one stray delimiter swallowed every entry after it into the entry before it. `- a` / blank / four-space ``` / blank / `- b` yielded ONE entry where `next` yields two: a widening that made an already-counted item vanish, on the mixed-file shape #3702 exists to close. Reproduces at indent 0 as well. The bound is the entry. CommonMark closes a fence with its container and a container at the next item at its level; this parser extends that to a document-level stray delimiter, where CommonMark would swallow to EOF and the fail-safe rule (surface, don't drop) will not. `scanFencesFrom` reports the unterminated opener and the walk supplies the bound — the next line shaped like a top-level item — then RESCANS from it, so a later delimiter is read on its own terms. Still one fence dialect: every block boundary is `scanFencedBlocks`' answer. Entry-scoped `fencedLineSet` (the field reader) already ran an unterminated fence to the end of its lines, so reader and walk agree by construction. Tests: the M2 pin that asserted `[]` for a stray fence before an item flips (the item counts); the round-4 "runs to end-of-file, exactly as CommonMark says" test is retitled — its assertion stands because the bound is the entry — and extended with the next entry; a new block pins the review reproduction at both indents, a terminated deep fence still gating, the gated status inside the bounded fence, the rescan case, and the heading-tokenizer caveat (at indent 0 the tokenizer applies CommonMark's own fence rule, so a heading after a stray delimiter is body text there, exactly as on `next`). Reverted in isolation against the final tree: 3 named tests fail. * fix(#3702): `0.` starts an ordered list (M1) The start-at-1 rule applied unconditionally dropped ONLY the first item of a `0.`-numbered list — the run then started at `1.` — which is the mixed under-report that looks like a clean parse. CommonMark §5.2 permits any 1-9-digit start and a `0.` list is ordinary; a sentence opening with "0." is not a shape anyone writes. The threshold is now `> 1`. The cost is restated accurately in the doc comment and pinned: a list starting at 2 or more, at a paragraph position, reads as prose until its first `0.`/`1.` line — the prefix, not the whole list. Boundary tests at the threshold itself: `0.`, `1.`, `2.` starts, `00.`/ `01.`, and the prefix-loss case. Reverted in isolation: 1 named test fails. * fix(#3702): a non-1 ordinal is an item wherever a list is already open at its level (M2) The per-indent run memory recorded whether the previous opener was ORDERED, so a bullet item closed the run and `1. a` / `- b` / `5. c` folded `5. c` into `b` — another mixed-file under-report. In CommonMark `5. c` there opens a fresh ordered list (start=5): a non-1 start is refused only where it would interrupt a PARAGRAPH (§5.3), and after a list item it interrupts nothing. `ListRuns` now records "a list is open here"; the start rule applies where no list is open at the line's level — the positions a sentence can occupy — so the round-2 B2 pins (doc start, after a heading, after a paragraph) hold unchanged. Two round-2 pins move with it, both CommonMark-backed: `1. alpha` / `- beta` / `2. gamma` is three items, and a nested `3. status:` after a nested bullet is a nested item (a field line, as `- status:` would be); the "rejected ordinal is not stripped" pin is re-anchored at a paragraph position, where it still holds. Reverted in isolation: 4 named tests fail. * docs(#3702): the two runtime-loaded docs state the grammar the parser ships (B3, M4 parity) `executor-examples.md` (the write-site doc) and `forensic-audit.md` check 7 (the model-executed parser) both asserted "never silently drops a possibly-open item" over a grammar that dropped three measured shapes. Both now carry the round-5 grammar — `0.`/`1.` starts, a non-1 ordinal inside an open list, an unclosed fence ending with its own entry — and the fail-safe sentence is kept with what it does NOT cover named beside it: a fenced line, a separator, and an ordered list numbered from `2.` upward at a paragraph position, and nothing else. * docs(#3702): changeset reflects the merged contract The "Breaking, and deliberate: … HALTS complete-milestone" paragraph described a refusal that #3781 removed from `next`; a heading-shaped file written with a newly recognised marker now surfaces its entries and `complete-milestone` acknowledges them in place. The fragment cites #3702 alone — #3740 and #3775 closed on `next` through #3940 and #3989; this PR's shared reader/writer classifier subsumes both fixes rather than closing either issue. The round-5 grammar (ordered start, unclosed fence bound) is stated in the user-facing sentence. * fix(#3702): the heading-shape insert lands on a line the reader reads, and keeps a closing `#` sequence Found by the round's pre-push adversarial review. An entry whose body ends in a fenced block — closed, or unclosed and therefore running to the entry's end — received `status: acknowledged` AFTER its last non-blank line, i.e. as fence content: the writer returned `ok` and the reader never saw the marker, the item stayed outstanding. That is the #3702 class itself (a write nothing reads), on the shape #3781 just opened. The insert now walks back over blank AND fenced lines, classified by the reader's own `fencedLineSet`, so the marker lands on a line the reader reads; pinned as round-trips for an unclosed fence, a closed fence, and a pending entry ending in an unclosed fence before a heading. Reverted in isolation: the round-trip test fails. Separately, the leaf line-0 rewrite (`### status: open ###`) dropped the closing `#` sequence; it is kept now. Cosmetic, pinned. * docs(#3702): the prose parser states the bare-key case rule; both docs say what an unclosed fence does, no more `forensic-audit.md` check 7 called `status: resolved` case-insensitive where the code reads a bare key lower-case only (the bolded form in any case; the value case-insensitively) — `executor-examples.md` already said so, the model-executed parser did not. And both docs claimed "a stray delimiter cannot hide the entries after it", which overstates B1: an UNCLOSED fence ends with its entry; a closed pair of delimiters is a fence, whatever sits between them, as CommonMark reads it. Found by the round's pre-push review. * fix(#3702): a heading whose text is a fence delimiter is a heading, not a fence Second finding of the round's pre-push review, one door over from the first: for a leaf headed `### ```` (or `~~~`) the entry-level fence scan read line 0 — the heading TEXT, not a Markdown line — as a fence opener, so every body line was fenced: the reader read no field under it, and the writer's marker (placed by the same scan) landed on a line nothing reads — `ok`, item outstanding. `entryFencedLines` now owns the entry's fence view for reader and writer alike, and a leaf's line 0 never opens a fence (the leaf tell is `openerFlags[0] === false`; a pending or headless entry's line 0 is a marker line, never a delimiter). Pinned for both delimiters, read and write; reverted in isolation the pin fails. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-31 13:44:47 -04:00
Tom Boucher	996196fe02	fix(#4099): stop word-splitting an unquoted PLAN_IDS string (bash vs zsh diverge) (#4102) ... * fix(review): stop word-splitting an unquoted PLAN_IDS string (bash vs zsh diverge) #3301's plan-coverage-manifest block built a space-separated PLAN_IDS accumulator and re-split it via unquoted `for x in $PLAN_IDS`. Bash word-splits an unquoted scalar on IFS by default; zsh does not, so every plan id collapsed onto one iteration under zsh whenever a review had 2+ plans, undercounting the manifest and merging bullets onto one line. This is exactly what was failing origin/next's CI on every push. Derive PLAN_COUNT and the bullet list directly from the same *-PLAN.md glob loop used for copying — glob iteration, unlike re-splitting a stored string, is identical under bash and zsh, so it never needs word-splitting. Closes #4099 Emitted-Drift-Ack-Growth: review.md — fix comment documents the bash/zsh word-splitting divergence (gsd-core#4099); net code is smaller, comment expansion grows the file * docs(#4099): add changeset for the plan-coverage-manifest zsh fix * docs(#4099): fix changeset body to match the bold-dash-explanation format Code review (standards axis) flagged the changeset body as not matching CONTRIBUTING.md's canonical `**<Bold>** — <explanation>.` format — the bold clause had its own trailing period instead of being followed directly by the em-dash separator. * chore(#4099): backfill changeset PR number (#4102) --------- Co-authored-by: sim <sim@local>	2026-08-31 11:08:22 -04:00
Tom Boucher	6eea00b707	enhance(#3301): tell reviewers the plan ids and total count, grade coverage (#4084) ... * test(#3301): add failing-first plan coverage manifest tests Failing-first regression tests for the plan-id manifest, the updated Review Instructions, and the mechanical per-reviewer coverage check, ahead of the review.md implementation. RED baseline before the fix lands. * test(#3301): raise allow-test-rule-refs unverified ceiling for new marker Adding tests/review-plan-coverage-manifest.test.cjs's source-text-is-the-product marker grows the unverified-exemption pool by one (282 -> 283), the same documented growth path scripts/lint-allow-test-rule-refs.cjs's own failure output names. Confirmed clean via 'npm run lint:allow-test-rule-refs' locally. * feat(#3301): tell reviewers the plan ids and total count, grade coverage build_prompt now derives a plan-id manifest from each *-PLAN.md filename (stripping the -PLAN.md suffix) and appends it, with the total plan count, to both gsd-review-instructions.md and gsd-review-prompt.md. The Review Instructions prose requires one heading-verbatim section per id before any cross-plan or overall-risk content. write_reviews grades each dispatched lane's real (non-stub, non-empty) review against that same manifest and records an optional plan_coverage: frontmatter block, present only when a lane is incomplete. The match escapes regex metacharacters in the id and excludes a preceding/trailing hyphen or word character as a boundary, closing the two traps named in the issue (a decimal phase like 12.6 satisfied by 12X6-01; a threat id like T-04-07 registering as coverage of plan 04-07). CodeRabbit is exempt, since it never receives the source-grounding prompt carrying the manifest. This closes the gap where a review that silently covers only some plans in a multi-plan phase is indistinguishable from one that covers all of them. * docs(#3301): add changeset fragment * test(#3301): use t.after() instead of try/finally for cleanup CONTRIBUTING.md bans try/finally inside test bodies. Code review caught this in the new coverage-manifest test file; switch every fixture-cleanup site to the approved t.after() pattern. * test: use t.after() instead of try/finally in #3300's build_prompt tests Pre-existing try/finally-for-cleanup pattern in this file (landed for #3300) violates CONTRIBUTING.md's explicit ban on try/finally inside test bodies. Surfaced incidentally while reviewing #3301's diff, which cites this file as its extraction-pattern precedent; fixed inline per the no-defer rule rather than deferred to a separate PR. * test(#3301): anchor coverage-check extraction on the fence line, not prose `.plans-manifest.md` also appears in write_reviews' own prose ahead of the ```bash fence, so indexOf found that occurrence first and the backward-walk-to-fence-open landed on the earlier, unrelated gate-check block instead. gsd-test caught this: coverage-check tests expecting a real verdict got null, because the wrong block ran and never writes .plan-coverage-<slug>.json. Anchor on the fence-only bash assignment line instead. Emitted-Drift-Ack-Growth: review.md — #3301 adds the plan-coverage manifest and mechanical coverage check to build_prompt/write_reviews. * fix(#3301): route id escaping through the canonical pattern seam ADR-3212 (epic #3212) consolidated ~44 hand-rolled regex-escape copies into one owner, src/pattern.cts's escapeRegex, specifically to stop this exact class of duplication. My coverage-check node -e script hand-rolled the identical metachar-escape regex — invisible to eslint-rules/no-adhoc-regex-escape.cjs only because it lives inside a workflow markdown file, not a .cts/.cjs source file the shape-matching guard scans. Require the compiled seam (gsd-core/bin/lib/pattern.cjs) instead, matching the established node -e-requires-a-compiled-lib idiom already used elsewhere in this workflow (code-review.md's code-review-flags.cjs/code-review-depth.cjs calls). Verified both named traps from the issue still resolve correctly under escapeRegex's RegExp.escape-backed implementation, which differs in escaped-text shape (hex-escapes hyphens/leading chars) but not match result. * test(#3301): run coverage-check block with cwd at the repo root The block's node -e now requires ./gsd-core/bin/lib/pattern.cjs, a path relative to the repo root (correct for production, which always runs from there). The test harness ran it with cwd at the fixture's own temp dir instead, so the require failed. Add an optional cwd param to runScript (default: root, unchanged for the plan-copy-block tests) and pass the real repo root for every coverage-check call site. Manually verified end-to-end before spending another remote run: the extracted block now produces the expected {complete:true} verdict. * docs(#3301): backfill changeset pr number (pr:0 -> pr:4084) --------- Co-authored-by: sim <sim@local>	2026-08-30 14:25:24 -04:00
Dennis Kim	8487f0ed42	enhance(#3552): warn on additional protected branches beyond the resolved base branch (#3648) ... * test(01-01): add failing protected-branch warning coverage - pin configured, absent, and malformed branch-list behavior - require opposite CLI and execute warning outcomes * feat(01-01): warn on configured protected branches - resolve the base branch union configured protected branch names - expose exact boolean CLI comparison output for workflow callers - keep execute-phase warning advisory and within its byte budget * test(01-01): add failing protected branch config coverage - cover valid list persistence and null unset - reject hostile shapes while preserving the prior value * feat(01-01): validate protected branch configuration - register git.protected_branches as a canonical config key - require a non-empty array of non-blank branch names * test(01-02): add failing ship protected-branch controls - Execute both workflow warning blocks with exact predicate arguments - Require true and false results to produce opposite warning outcomes - Preserve the none-strategy feature-branch offer contract * feat(01-02): warn at ship on protected branches - Reuse the typed protected-branch predicate in ship preflight - Keep raw base resolution for PR targeting and advisory branch creation - Prove execute and ship warning blocks with opposite-result controls * test(01-02): add failing protected-branch docs parity - Require the canonical schema key in both English config references - Pin the non-empty string-array type and absent default - Require synchronized multi-branch examples and advisory semantics * feat(01-02): publish protected branch configuration contract - Document the optional non-empty string-array field in both references - Explain resolved-base union and absent-field compatibility - Keep execute and ship warnings advisory under branching_strategy none * fix(01): CR-01 honor active workstream branch policy * fix(01): WR-01 assert protected config path selection * docs: add changeset fragment for #3648 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CteVPJt4BkPmroMPGajYx * fix(#3648): resolve base_branch precedence inversion and round-1 findings Blocker 1/2: production config resolution was flat-first, so a project that migrated to git.base_branch but still carried a stale flat base_branch got the old value back. Add base_branch to normalizeLegacyKeys (mirrors the existing branching_strategy/sub_repos pattern: canonical nested wins) and route readEffectiveGitConfig's test seam through the same normalization so it can't silently diverge from production again. Adds a regression test with both keys set that fails without the fix. Blocker 3/4/5: restore the handle_branching case-selector prose and "none" contract sentence that #3389's tests anchor on, and revert the unrelated prose/comment compaction in the same step — both were drive-by edits outside #3552's scope. Also addresses review majors/minors: delete readConfigBaseBranch and readConfigProtectedBranches (dead in production, only self-tested); --is-protected now fails closed (reports protected) instead of silently answering false when the base branch can't be verified; trim configured protected-branch names; fix HOME-without-USERPROFILE vacuous isolation on Windows; correct the drift-ack's byte accounting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S44stkuQbhD3jTCtKzte5N * test(#3648): add failing legacy-key hoist safety coverage Round-2 review found normalizeLegacyKeys block 5 records a normalization carrying the DISCARDED flat value on the canonical-wins branch. Probing that turned up a second, unreported defect in the same helper shape: blocks 1, 2 and 5 all spread result['git'] / result['planning'] with no object guard, so a config whose section key holds a string is spread into index keys — {"git":"main","base_branch":"release"} -> {"git":{"0":"m","1":"a","2":"i","3":"n","base_branch":"release"}} The resolved value is accidentally still correct, so nothing fails and no diagnostic fires. But normalizations.length > 0 sets configDirty, and config-loader then serializes that shape back into the user's config.json — a read that silently corrupts config. The deleted #3057 W3 suite covered {"git":"main","base_branch":"release"} explicitly; this is the input it would have caught. Covers both defects across blocks 1 and 5, with object/array/null negative controls that must stay green in both phases, and a fast-check property over arbitrary `git` values. * test(#3648): pin fail-closed handling of malformed protected_branches Replaces the test that pinned the fail-OPEN behaviour. The old assertion — ['develop', 42] yields isProtected === false for 'develop' — locked in the exact failure #3552 exists to close: config-set validation is bypassable by a direct edit of .planning/config.json, so a user who believes 'develop' is protected got a silent false and no warning. It was also inconsistent with the fail-CLOSED direction twelve lines away, where an unverified base reports protected and writes a diagnostic. A protection predicate must not have two opposite failure directions depending on which input is bad (#3648 review Blocker 3). New coverage: a bad element drops only itself, a non-array contributes no names, an empty list is well-formed rather than malformed, and --is-protected surfaces the rejection. Both negative controls — a clean list reports nothing rejected and writes no diagnostic — must stay green in either phase, so the reject channel cannot fire unconditionally. * fix(#3648): drop only invalid protected_branches and report them Partition git.protected_branches instead of discarding the whole list on one bad element, and carry the rejections out through ProtectedBranchStatus so --is-protected can name them on stderr. Valid names keep protecting; the user finds out the rest were ignored. A non-array value still contributes no names — a bare string is not a list of branch names — but is now reported rather than swallowed. An empty array stays silent: declaring no extra protected branches is a valid choice, not a misconfiguration. writeDiagnostic is hoisted out of the unverified-base branch since both arms now use it. * test(#3648): prove the predicate diagnostic survives both call sites The workflow bash stub now emits a stderr diagnostic the way the real command does, which is what makes a swallowed `2>/dev/null` visible to a test — previously the stub was silent on stderr, so discarding it changed no observable behaviour and the call sites could drop the explanation undetected. Adds the Minor 2 binding check as well: ship must expose the predicate result as IS_PROTECTED rather than only echoing a warning, asserted by running the extracted bash and reading the bound value, not by grepping the workflow source. Both tests carry opposite-outcome controls — an empty diagnostic must leave the text absent, and a false predicate must bind false. * fix(#3648): surface the predicate diagnostic and bind ship's result Drop `2>/dev/null` from the --is-protected call at both call sites. The fail-closed explanation and the new rejected-entry warning both go to stderr, so discarding it left the user with a bare "protected branch" warning on a branch that is not protected and no way to tell a real match from a degraded-git guess. `git branch --show-current` keeps its own redirect — that one is genuine noise. ship.md binds IS_PROTECTED and its prose now branches on the variable, so the following steps have evaluable state instead of having to infer it from warning text in tool output. execute-phase.md byte accounting refreshed: 92326 -> 92645, net growth 319 bytes (was 331 before the redirect came out). Baseline re-verified against the current rebase base by blob id; the ceiling check passes with 755 bytes of margin. * test(#3648): restore negative space for the readFile config seam The #3057 W3 suite was deleted with readConfigBaseBranch, but every arm it pinned survives verbatim in readEffectiveGitConfig's readFile branch — the JSON.parse catch, the non-object guard, the git-section object guard, .trim() and blank-string rejection — and the four surviving readFile injections were positive-path only. protected_branches was never driven through this seam at all. Restores nine cases against the seam, including protected_branches partitioning, plus a control proving loadConfig still wins when both seams are supplied. Records honestly what the suite pins. Mutating the built lib shows .trim() is KILLED, while the non-object guard and the blank-string rejection SURVIVE — both are unreachable through this entry point for the same reasons the deleted suite documented against its own equivalents: a JSON-parsed non-object carries no relevant own-property either way, and a blank value is rejected a second time downstream by the resolver's truthiness check. They stay as defence-in-depth and are labelled known-unkillable rather than left looking like coverage this suite does not provide. * test(#3648): distinguish detached HEAD from a missing branch argument `args[1] ?? ''` collapsed two different situations into one: a detached HEAD, where `git branch --show-current` legitimately prints nothing, and the flag being called with no argument at all. Both answered false, so the right outcome arrived by an unintentional path and a caller bug was indistinguishable from normal operation. Asserts the detached case stays silent and the missing-argument case reports, with a control that the two diagnostics differ. * fix(#3648): report a missing --is-protected branch argument Answer false either way, but say so when the flag arrives with no argument. A detached HEAD passes an explicit empty string and stays silent, since that is a normal state rather than a misconfiguration. * docs(#3648): state exact-name matching and per-entry rejection isProtected is exact string equality, so a git-flow project must enumerate every release/* and hotfix/* by name. #3552 only asked for an integration-branch field, so the implementation satisfies the letter of the issue while leaving its git-flow motivation partly unserved — say so where users will meet it rather than leaving them to discover it. Also documents the Blocker 3 behaviour change: an invalid entry is ignored with a warning naming it and the remaining names still apply. Both statements land in docs/CONFIGURATION.md and gsd-core/references/planning-config.md, and the config-field-docs parity test asserts each in both so the two cannot drift. * refactor(#3648): extract isValidProtectedBranches for cross-surface pinning The `git.protected_branches` check inside `cmdConfigSet` and the resolver's per-entry filter in `git-base-branch.cts` are deliberately different shapes — all-or-nothing on write, per-entry on read, so a hand-edited config.json cannot fail the guard open. Nothing structural keeps their two definitions of "usable branch name" in step. Lifting the write-side check into a named, exported predicate lets a property test ask both surfaces about the same value and assert they agree, which is the fast-check gap the round-2 review flagged. No behaviour change: the predicate is the same expression, called from the same place. * fix(#3648): stop --is-protected rewriting the config it is asking about `gsd_run query git.base-branch --is-protected` runs on every execute-phase and every ship. It resolved config through `loadConfig`, whose normalize-then-write path rewrites `.planning/config.json` whenever any legacy key normalizes — so a boolean question was silently editing the user's checked-in config. This PR had widened the trigger by adding a fifth normalization block (top-level `base_branch` -> `git.base_branch`), making it fire for exactly the projects the feature targets. `loadConfigResolved` gains `options.persist` (opt-OUT, default true): resolution is unchanged, only the two write-back side effects are suppressed. The predicate passes `persist: false`; the ~30 other callers are untouched, so a legacy config is still migrated by ordinary use. Asserted on BYTES rather than parsed shape, because the rewrite reorders keys and reflows whitespace even when the values are equivalent. Three tests, each with its own control: the end-to-end CLI leaves the file byte-identical while still answering `true` from the legacy key (proving the config WAS read); an ordinary persisting load of the same fixture DOES change the bytes (proving the fixture is live rather than inert); and `persist:false` vs default over one directory returns deep-equal config while differing on the write. Reverting the one-line `persist: false` fails the first of those and only that one. Also from the review: - `readEffectiveGitConfig`'s comment claimed the readFile branch routed "through the same precedence authority production uses". It does not, and cannot — it reproduces two of production's steps over a single file. The comment now names what the seam covers and what it does NOT (root/workstream deep merge, builtin and global defaults, federated merge), and the seam now applies production's flat-then-nested lookup so it stops disagreeing about a surviving flat key. - The missing-argument diagnostic promised "answering false", which the fail-closed guard on the same call can contradict by printing `true`. It now states what it did with the argument and leaves the answer to stdout. * test(#3648): re-pin block 5 on #3760's refusal contract #3767 landed on next while this PR was in review and fixed the non-object config-section defect properly: a present-but-non-object section now BLOCKS its own migration — value preserved, no Normalization pushed, refusal reported via `skipped[]` — rather than being rebuilt from a plain-object view. That supersedes this branch's round-2 `hoistLegacyKey`, which prevented the character-key spread but still dropped the section value silently, and which the round-3 review correctly called out as destruction in place of corruption. The rebase drops that commit and routes block 5 through the upstream helper. This file's tests asserted the superseded design, so they are rewritten to pin block 5 — `base_branch` -> `git.base_branch`, which did not exist when #3760's suite was written — against the contract that now governs it: ordinary hoist into an absent/null/object section, canonical-nested-wins, and refusal for each of string/number/boolean/array sections with the exact `skipped` entry. Two controls keep it from passing vacuously: the refusal must be scoped to block 5 (an unrelated block still normalizes in the same call), and a property over arbitrary `git` values asserts hoist and refusal are exhaustive AND mutually exclusive per key, that a refusal leaves both the section and the legacy key untouched, and that a hoist manufactures no index key the input did not carry. * docs(#3648): correct the Git Query and Config Loader module contracts CONTEXT.md's Git Query Module still described base-branch tier 1 as a direct `.planning/config.json` read. Since this PR it is the EFFECTIVE configuration resolved by the Config Loader — a materially different authority, carrying the root/workstream deep merge, flat-then-nested lookup and builtin/federated defaults. The `--is-protected` predicate, `git.protected_branches`, and the two invariants that distinguish the predicate from the plain query (fails closed on an unverified base; must not write) were undocumented entirely. The Config Loader entry now states that loading is not side-effect-free by default and documents `options.persist`. docs/INVENTORY.md's `git-base-branch.cjs` row carried the same stale ladder and no mention of the predicate. `node scripts/gen-inventory-manifest.cjs --write` was run and produced no diff: the manifest indexes roster NAMES, not row prose, so a description edit cannot move it. Also closes the global-defaults minor: `git.protected_branches` is inert in `~/.gsd/defaults.json`, but so is every other `git.*` key — no branch-policy key appears in `_globalBaseCfg` or `GLOBAL_DEFAULTS_RESOLUTION_KEYS`. That is section-wide and predates this PR, so the fix is to state the scope where users meet it rather than to quietly extend the resolution set for two new keys. * fix(#3648): close four defects found by the round-4 external review Two external reviewers (codex, antigravity/Gemini 3.1 Pro) were run adversarially against this branch. Four findings reproduced against source; each is fixed with a failing-first test and a control, and each fix was verified by reverting it and watching exactly the intended test fail. 1. `persist:false` was DROPPED by the workstream fallback (codex). Blocker 1 was only half closed. `loadConfigResolved` re-enters itself with a bare `{ workstream: null }` when a workstream has no config.json of its own, and that literal discarded every other option — so the recursive pass ran at the DEFAULT persistence and rewrote the ROOT config. Reproduced: with GSD_WORKSTREAM=alpha and a legacy flat `base_branch`, `--is-protected` rewrote `.planning/config.json` despite `persist:false`. Both recursions now forward `options` and override only `workstream`; the explicit override still wins the hasOwnProperty check, so spreading cannot let `workstreamContext` reintroduce a workstream. 2. Both workflow call sites failed OPEN, and aborted under `set -e` (both reviewers, independently). `IS_PROTECTED=$(gsd_run ...)` yields an empty string when the query fails, so `[ "$X" = true ]` was simply false: no warning, no trace — a silent hole in the guard whose only job is to warn. The bare assignment also aborted the step under `set -e`. Both sites now degrade VISIBLY: `|| IS_PROTECTED=""`, then an explicit empty-string arm that says the check did not run. Deliberately not fail-closed — claiming "protected" on no evidence would warn on every branch whenever gsd-tools is unavailable. 3. `isValidProtectedBranches` and the resolver disagreed on a sparse array (antigravity). `.every()` skips holes; the resolver's `for...of` yields `undefined` for them, so `["main", , "develop"]` was accepted by config-set and rejected by the resolver. The cross-surface property passed only because `fc.array` cannot generate a hole. The predicate now indexes, and the generator punches holes so that axis is actually falsifiable. JSON cannot express a hole, so this is unreachable in production — but two definitions of one predicate must not contradict each other. 4. A top-level `protected_branches` silently outranked `git.protected_branches` (antigravity). Routing the key through `get(key, {section, field})` gave it flat-then-nested precedence, which is back-compat for keys `normalizeLegacyKeys` migrates. `protected_branches` is new in #3552 and has no legacy form, so that invented an undocumented alias. It now resolves nested-only through a new `getNested`, in production and in the test seam. `base_branch` keeps flat-then-nested — it HAS a legacy spelling that #3760's refusal path can leave behind — and a control pins that distinction. Also narrows a CONTEXT.md claim this round introduced. The predicate fails closed only when a git query TIMED OUT or could not be spawned (#3057 B4's `verified`); a git command that runs and exits non-zero counts as a clean negative, so a cwd that is not a repository answers `false`, not `true`. Verified pre-existing on next @ 738f42f4, so the documentation was over-claiming rather than the code regressing — but an over-broad contract is exactly what the module docs must not carry. Both workflow byte figures re-derived after the call-site change: execute-phase.md 92356 -> 92865 (+509), ship.md 36784 -> 37227 (+443). * test(#3648): pin git config read parity * docs(#3648): document git query contracts * fix(#3648): expose protected branch default * test(#3648): snapshot planning tree for read-only query * test(#3648): pin planning snapshot stray-write detection * fix(#3648): resolve merge conflict from #3078's ack-fragment sweep next swept the fully-spent 2818/3003 ack fragments this branch had appended to (#3078, a84f7563). Rebased onto upstream/next and took the deletions on both, then moved the #3552 append into a new fragment of its own. Rebasing onto the current base also left execute-phase.md only 34 bytes under the frozen ADR-857 Phase 6 margin ceiling (93400 bytes) — intervening next PRs consumed the rest while this PR was in review. Extracted the "none" arm's protected-branch-warning bash block into gsd-core/workflows/execute-phase/steps/protected-branch.md (content unchanged, matching the existing steps/ extraction pattern used elsewhere in this file) so the inline growth is a one-line pointer instead of the full block. 93366 -> 93385 bytes (+19), 15 bytes inside the ceiling. * fix(#3648): drop stale ack entry for the new step file The extracted execute-phase/steps/protected-branch.md needed no acknowledgment of its own — the differential-attribution check flagged the entry as stale once the build ran, so removed it and kept the two growth entries (execute-phase.md, ship.md) that actually needed one. * fix(#3648): follow the step-file reference in the bash-extraction test helper extractProtectedBranchWarningBash() read the "none" arm's bash block directly out of execute-phase.md. That block now lives in execute-phase/steps/protected-branch.md (byte-ceiling extraction); the helper follows the step-file reference and extracts from there when no inline block is found, so the three execute-phase tests that execute this bash for real keep exercising the actual behavior. * fix(#3648): regenerate INVENTORY-MANIFEST.json and satisfy the CRLF-fragile lint rule - gen-inventory-manifest.cjs --write to pick up the new execute-phase/steps/protected-branch.md entry (already covered by docs/INVENTORY.md's generic workflow_steps wildcard row, so no INVENTORY.md edit is needed). - Reworked the step-file-reference lookup in extractProtectedBranchWarningBash() to avoid a bare-\n regex split on file content (local/no-crlf-fragile-split), using the same line-array scan the function already uses elsewhere. * fix(#3648): regenerate golden install-tree fixtures for the new step file npm run gen:install-tree, adding gsd-core/workflows/execute-phase/ steps/protected-branch.md to all 19 runtime install-tree fixtures. CI's tests/golden-install-tree.test.cjs caught this on push — I'd verified the differential-attribution and INVENTORY-MANIFEST checks but missed this separate golden-fixture check for the new file. * fix(#3648): add the canonical gsd_run preamble to the new step file CI's runtime-launcher-parity suite requires exactly one canonical resolver preamble in every workflow .md that calls gsd_run. The inline "none"-arm block never needed one (execute-phase.md already carried a preamble elsewhere in the same file), but the extracted execute-phase/steps/protected-branch.md is now its own file with no preamble of its own. Ran node scripts/sync-runtime-launcher.cjs to insert it (execute-phase.md itself is untouched — still 93385 bytes, inside the ADR-857 ceiling). That preamble defines its own gsd_run(), which shadows the mock tests/git-base-branch.test.cjs injects for the three #3648 tests that execute this bash for real — without stripping it, those tests reached the real gsd-tools.cjs on the machine running them instead of the test's fixture. Preamble correctness is already covered by tests/runtime-launcher-parity.test.cjs, so extractProtectedBranchWarningBash() now strips the preamble line before handing the block to the harness; it only needs to exercise the #3552 warning logic. * fix(#3552): address PR 3648 review feedback on protected branch warnings - Fix execute-phase handle_branching branching_strategy=none instruction to "Read and execute execute-phase/steps/protected-branch.md" - Use io.error(..., ERROR_REASON.USAGE) for cmdGitBaseBranch usage errors - Align git.protected_branches schema default to (none) without fallback [] - Relocate CONTEXT.md forward-referencing sentence into module body - Sanitize control and ANSI characters in renderRejected diagnostics - Clean up out-of-scope whitespace hunks in gsd-tools.cjs Emitted-Drift-Ack-Growth: execute-phase.md — #3552: execute-phase handle_branching adds a pointer to execute-phase/steps/protected-branch.md for branching_strategy=none so the protected-branch check executes while keeping execute-phase.md within the ADR-857 Phase 6 margin ceiling (93400 bytes). 93392 bytes, 8 bytes inside the ceiling. Emitted-Drift-Ack-Growth: ship.md — #3552: ship preflight step 3 now asks the same typed git.base-branch --is-protected predicate as execute-phase, binding IS_PROTECTED and warning without refusing execution or blocking the branching_strategy=none feature-branch offer; it degrades visibly (rather than silently reading an empty result as "not protected") when the query itself fails to run. 36841 bytes, well inside the XL cap (98304, tests/workflow-size-budget.test.cjs). --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-29 17:00:52 -04:00
0xdhx	472f585f7c	fix(#3726)!: require --confirm before milestone complete mutates (#3774) ... * fix(#3726): require --confirm before milestone complete mutates `milestone complete <version>` is a one-way door — ROADMAP.md and REQUIREMENTS.md archived, every phase directory in the milestone MOVED, STATE.md rewritten — and ran unconditionally on first invocation through every invocation path, including `query milestone.complete <version>`, whose `query` meta-prefix reads as a read-only namespace but performs no filtering (#167's invocation-compatibility shim + #3243's dotted-form normalization). The gate lives on the destructive command itself, not on the `query` prefix (the prefix is an intentional invocation mechanism, not a permission boundary — restricting it would break dozens of shipped workflow callers). Without --confirm and without --dry-run the command now refuses via error() before reading anything beyond its arg checks, so an unconfirmed invocation is a guaranteed no-op on disk. --dry-run still previews with no confirmation needed and is now documented in the usage block (it was only documented for the sibling archive-quick). --force keeps its narrow meaning — bypassing the TRUNCATED-scope and unstarted-phase guards — and does not double as the mutation opt-in. --confirm follows the existing `phases clear --confirm` idiom in the same module. complete-milestone.md's two invocations pass --confirm (the workflow has gathered explicit user intent by that step). Existing tests get --confirm appended — pre-change behavior is exactly confirmed behavior — and a #3726 regression block covers: refusal + full-tree byte-identity on both invocation forms, --force not satisfying the gate, --dry-run still passing without confirmation, and --confirm proceeding. The refusal tests fail against pre-fix code (negative control run). Fixes #3726 * docs(#3726): document the --confirm requirement in CLI-TOOLS and COMMANDS Cross-AI review of the fix diff (codex, pre-create) caught three shipped doc sites still instructing the now-refused bare invocation: the CLI-TOOLS.md milestone-complete synopsis + flag table, and COMMANDS.md's two guard-override instructions (`--force` alone now refuses without --confirm). Localized CLI-TOOLS copies already lag the English synopsis (no --force/--dry-run either) and follow the translation pipeline, not this fix. * chore(#3726): set changeset fragment pr to 3774 * test(#3726): confirm-gate CI repairs — QA scenario caller + growth ack Two CI reds from the --confirm gate, both this branch's own misses: - tests/qa/scenarios/milestone-rollover.json invoked `milestone complete 1.0 --force` as a JSON arg-array fixture — a caller shape the test sweep (which grepped runGsdTools/runSdkQuery in tests/*.cjs) never enumerated. Adds --confirm; the scenario's boundary-crossing contract is otherwise untouched. - complete-milestone.md's +420-byte --confirm note trips the emitted-attribution growth ratchet. Acknowledged as a #3726 append to the existing complete-milestone.md entry in 3409-unreachable-guard-arms.json (two ack sources may never name the same path, per that fragment's own precedent). Local: lint-emitted-drift-ack ok; loop-walk.qa 115/115 green sandboxed. * docs(#3726): CLI-TOOLS.md guard-override sentences say --force --confirm Review Major 1: the truncated-window and unstarted-phase guard paragraphs still told the reader to "Pass `--force` to override", which now refuses (--force alone does not satisfy the confirmation gate), while the flag table 470 lines later said the opposite. Mirror the docs/COMMANDS.md pair so the file no longer contradicts itself. * docs(#3726): synopsis renders --confirm and --dry-run as alternatives Review Nit 1: `milestone complete <version> --confirm [--dry-run]` read as "a dry run still needs --confirm", the opposite of AC 3. Render the pair as `(--confirm | --dry-run)` in the CLI-TOOLS.md synopsis and the usage docblock, and let the flag rows carry the rule. * test(#3726): pass --confirm in base-added milestone fixtures; re-file the growth ack Rebase onto next (26 commits) surfaced three tests the gate now refuses: the #3685 write-flag contract pair in tests/milestone.test.cjs and the `milestone complete` boundary fixture in tests/state-contract.test.cjs all invoke the command bare. Each now passes --confirm (a mutating run is exactly what they assert on). The +420 byte complete-milestone.md growth ack rode on 3409-unreachable-guard-arms.json, which #3078 swept from next as fully spent — hence the modify/delete conflict. Re-filed under a fresh fragment named for this issue, never resurrecting the swept one. * test(#3726): pin the present-but-falsy arm of the confirmation gate Review Minor 1: the boundary triple covered absent and present but not present-but-falsy. The gate is an exact-token match, so --confirm=false and --confirm=0 refuse today — pinned (canonical + query forms, whole .planning/ tree byte-identical) so a future `=`-aware or prefix-matching parser cannot silently turn --confirm=false into a confirmed run of an irreversible command. * test(#3726): drop --confirm from dry-run-only invocations Review Nit 2: --confirm was mass-appended to 14 pre-existing --dry-run invocations that never needed it, so each stopped standing as incidental proof that a preview needs no confirmation. Reverted to the pre-PR form; the dedicated AC-3 test carries the explicit assertion. * docs(#3726): sync the localized CLI-TOOLS synopsis with the confirm gate REQ-I18N-02 (docs/features/internationalized-documentation.md) requires translations to stay synchronized with the English source. The four localized CLI-TOOLS.md guides still advertised a bare `milestone complete <version>`, which now exits 1. Render the English synopsis verbatim — `(--confirm | --dry-run)` plus the `[--force]` and `[--archive-quick]` flags the translations had also fallen behind on. * test(#3726): drop --confirm from the remaining preview-only invocations Round 2 reverted the --confirm appends on --dry-run-only invocations in tests/milestone.test.cjs, but four more sat in two files the sweep missed: tests/milestone-archive.test.cjs (three) and tests/milestone-window-single-owner.test.cjs (one). Each is a preview run whose whole purpose is to document that a preview mutates nothing, so `--dry-run ... --confirm` contradicted the semantics the test exists to pin. Dropping the token restores each as incidental proof that a preview needs no confirmation; the dedicated AC-3 test keeps the explicit assertion. No assertion added, relaxed, or removed — the change is four tokens. * chore(#3726): migrate the emitted-drift ack from a fragment to a commit trailer #3954 (ADR-3942) moved emitted-drift acknowledgments out of tests/emitted-drift-acks/ and into git commit trailers, and the fragment directory no longer exists on next. The reason this PR's fragment carried moves verbatim into the Emitted-Drift-Ack-Growth trailer on this commit; the fragment file is removed rather than resurrected. Emitted-Drift-Ack-Growth: complete-milestone.md — #3726: +420 bytes (40186 -> 40606). The archive_milestone step's two `milestone complete` invocations now pass the required --confirm flag (the command refuses to mutate without it — the archive is irreversible), with a note explaining the flag and pointing at --dry-run for previews. Deliberate runtime-loaded workflow text for the new gate, not converter drift. * fix(#3726): name --confirm in the version-required refusal The documented arg-discovery path (gsd-tools.cjs top-level usage: invoke the command without args and the error lists what is required) stopped at `version required for milestone complete (e.g., v1.0)` — one required argument short. Discovering --confirm took a second round trip through the gate. The refusal now reads `… — and --confirm to mutate`, pinned by a test that also asserts the version-less invocation leaves .planning/ untouched. * test(#3726): pin the milestone complete docs against a silent regression The changeset is `type: Fixed`, which the docs-required lint exempts, so nothing in CI would notice a later edit that reinstated the bare-`--force` override prose or dropped `--confirm` from the synopsis. Four tests in tests/milestone.test.cjs now pin: the synopsis line in docs/CLI-TOOLS.md and its four localized mirrors; the `--confirm` flag row; both guard-override instructions in docs/CLI-TOOLS.md and docs/COMMANDS.md, by guard name (a substring match on each instruction's `--force --confirm` text); and — as an identity ratchet over the milestone-complete sections — every `--force` sentence or clause that lacks `--confirm`, so a new bare instruction in its own sentence or clause fails whatever its wording. Named residual: a bare instruction spliced into the same clause as a compliant one coalesces with it and passes the ratchet; the by-name pins are what keep the four known instructions from losing the pairing that way. The file is registered in scripts/docs-guard-registry.cjs so the pin runs on the PR that changes those docs, not only after merge. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-29 17:00:45 -04:00
Tom Boucher	400db94e02	fix(#3894): quick path honors workflow.research_before_questions; key resolves from global defaults (#4047) ... * test(#3894): research_before_questions must resolve globally and order quick.md (failing first) * fix(#3894): quick path honors workflow.research_before_questions; key resolves from global defaults Two layers, one key. The quick workflow ran its discussion phase before its research phase unconditionally — neither quick.md nor its steps ever read workflow.research_before_questions, though the key is documented, schema-registered, /gsd-settings-writable, and honored by /gsd-discuss-phase and /gsd-new-project. A gray-area answer given without research is then written to <quick_id>-CONTEXT.md as a locked decision downstream agents are told not to revisit — an evidence-free choice made unfalsifiable (the reporter's #3714 misresolution). - quick.md Step 4 now carries the same research-before-questions check the two honoring paths make: when enabled, research-phase executes before discussion-phase; false/unset keeps the written order. Both sections stay section-manifest gated. - src/config-loader.cts forwarded workflow.post_planning_gaps from ~/.gsd/defaults.json but silently dropped this key — same file, same nesting, one resolved and one didn't. Now forwarded with the same flat + nested-alias fallback shape, added to the resolution-keys lockstep canary and the #3532 shadowed-warning set (nested alias reporting generalized over both keys). Emitted-Drift-Ack-Growth: quick.md — #3894: +Step 4 ordering rule (the research-before-questions check the discuss-phase and new-project paths already make); a real behavioral gate, not incidental bloat. * fix(#3894): review fold-ins — gate the CONTEXT.md reference, colon slash-forms - quick/steps/research-phase.md directed the researcher subagent to read <quick_id>-CONTEXT.md under DISCUSS_MODE with no existence hedge — but under the new ordering (research BEFORE discussion) the file cannot exist yet when the researcher is dispatched. The reference now says read-only-if-present with the #3894 reason; the alignment purpose still applies on the default ordering. - quick.md's new rule used the hyphen slash forms (/gsd-discuss-phase, /gsd-new-project); source artifacts under gsd-core/workflows must author the colon form the install-time converters key on — the same file already uses /gsd:new-project and /gsd:quick elsewhere. * docs(#3894): planning-config row names the flat CONFIG_DEFAULTS alias config-field-docs requires every CONFIG_DEFAULTS key to appear in the doc; the row documented the canonical namespaced form only. Adds the same alias sentence post_planning_gaps's row carries, plus the #3894 quick-path note. * chore(#3894): changeset fragment (pr number backfilled after PR creation) * chore(#3894): backfill changeset PR number (4047) --------- Co-authored-by: sim <sim@local>	2026-08-29 13:51:42 -04:00
Tom Boucher	3c08315a5e	fix(#3827): new-mode routing gate — classify approval no longer authorizes scaffold writes (#4037) ... * test(#3827): new-mode routing approval gate before roadmapper (failing first) * fix(#3827): new-mode routing gate — classify approval no longer authorizes scaffold writes The route_new_mode step delegated to gsd-roadmapper (PROJECT.md, REQUIREMENTS.md, ROADMAP.md, STATE.md creation + commit) with no gate: the discovery gate approved classification only, and the zero-conflict branch said 'proceed to routing silently'. Merge mode previews its diff and gates via approve-revise-abort; new mode now shows the exact destinations and requires Create planning setup | Keep synthesized intel only | Abort, per the skill contract's routing-gate requirement. The keep-intel-only choice is the analysis-only path the issue asks for: no roadmapper, no destination writes, intel preserved; finalize labels it 'new (intel only)' and points at /gsd:new-project instead of plan-phase. Ambiguous gate answers re-ask once, then treat as Abort — never infer Create. Zero-conflict wording is mode-aware: silence about conflicts is not authorization to write. Emitted-Drift-Ack-Growth: ingest-docs.md — #3827: +routing gate display block, AskUserQuestion contract, three disposition branches (incl. the intel-only no-write path), ambiguous-answer rule, intel-only finalize line, mode-aware zero-conflict pointer; a real behavioral gate, not incidental bloat. * chore(#3827): changeset fragment (pr number backfilled after PR creation) * chore(#3827): backfill changeset PR number (4037) --------- Co-authored-by: sim <sim@local>	2026-08-29 09:41:00 -04:00
Tom Boucher	51ca9f39ba	fix(#3801): register inline_plan_threshold in the defaults manifest and correct the docs (#4019) ... * fix(#3801): register inline_plan_threshold in the defaults manifest (default 2) and correct settings-advanced * chore(#3801): changeset fragment (pr number backfilled after PR creation) * chore(#3801): backfill changeset PR number (4019) * test(#3801): parse the defaults table with the shared markdown-table parser --------- Co-authored-by: sim <sim@local>	2026-08-28 23:48:42 -04:00
Tom Boucher	004c7532b6	fix(#3796): write the audit report to the single-version filename every reader expects (#4007) ... * test(#3796): the audit report writer and readers must agree on the filename * fix(#3796): write the audit report to the single-version filename every reader expects * chore(#3796): changeset fragment (pr number backfilled after PR creation) * chore(#3796): backfill changeset PR number (4007) --------- Co-authored-by: sim <sim@local>	2026-08-28 14:44:01 -04:00
Tom Boucher	4d151e46b6	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>	2026-08-28 13:50:37 -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	c3e667df34	fix(#3786): authorize mutable scope from live observation only in the quick planner (#4005) ... * test(#3786): the quick planner constraints must carry a mutable-scope authority rule * fix(#3786): authorize mutable scope from live observation only in the quick planner The quick planner could commit HISTORICAL scope as authorized edit or verification scope before any live observation of the mutable state: a minimized probe used cached PR-diff paths (65 of them, "pending replacement") or broadened verification to the PR integration surface in 2 of 3 trials (#3786). One explicit authority requirement reduced that to 0 of 3. The new planner constraint authorizes mutable-state scope ONLY from a live observation made at planning time (for conflict resolution, the fresh merge index via git diff --name-only --diff-filter=U) or keeps files/verify CONDITIONAL on it; historical STATE.md entries, recovery notes, and cached PR/base diffs may guide investigation only. A structural guard pins the rule in the shipped constraints block. Emitted-Drift-Ack-Growth: quick.md — #3786: +620 bytes — one MUTABLE-SCOPE AUTHORITY constraint bullet added to the planner <constraints> block * chore(#3786): changeset fragment (pr number backfilled after PR creation) * chore(#3786): backfill changeset PR number (4005) --------- Co-authored-by: sim <sim@local>	2026-08-28 12:31:02 -04:00
Tom Boucher	c8f08b61fb	fix(#3782): segment verification debt by the archived_milestone stamp in progress step 1.6 (#4001) ... * test(#3782): step 1.6 must segment verification debt by the archived stamp * fix(#3782): segment verification debt by the archived_milestone stamp in progress step 1.6 progress.md step 1.6 read the cross-population summary.total_items as current-milestone debt while claiming the whole query respects milestone boundaries. The active tree is milestone-filtered; archived trees are deliberately unfiltered (each result stamped archived_milestone), so a healthy current milestone presented six shipped milestones' still-open items as CURRENT debt on every /gsd-progress run (#3782). Step 1.6 now computes CURRENT_DEBT/ARCHIVED_DEBT via jq selects on the stamp, tracks archived_debt separately (visible with its own labeled header — segmented, never hidden), corrects the scoping claim, and unwraps the CLI's @file: large-payload redirect before jq so counters cannot silently read 0. A structural guard test pins the segmentation. parse_gap_files stays deliberately cross-population. Emitted-Drift-Ack-Growth: progress.md — #3782: +1406 bytes — step 1.6 gains the CURRENT_DEBT/ARCHIVED_DEBT segmentation jq, the @file: unwrap, and the archived-visibility paragraph; parse-gap paragraph reworded to match * chore(#3782): changeset fragment (pr number backfilled after PR creation) * chore(#3782): backfill changeset PR number (4001) --------- Co-authored-by: sim <sim@local>	2026-08-28 11:40:34 -04:00
Tom Boucher	15af0f5536	enhance(#3951): B6+B7 — widen two unreachable lint rules and make the guard ledger true (#3965) ... * fix(#3951): two lint rules that could not reach the code they govern B6 names two widenings. Measuring them first turned up a defect the criterion did not know about, and refuted the reason it gave for one of them. 1. no-adhoc-markdown-parsing self-gates on its own filename. Lines 107-110 short-circuit create() to {} unless the path matches /(?:^|\/)src\/[^/]+\.cts$/. B6 says to widen the files: glob in eslint.config.mjs - but doing only that ships an INERT rule, because the gate still returns {} for every new path. Both halves have to change, and the gate is the load-bearing one. That same regex hides a live hole: [^/]+ is FLAT-ONLY, so it requires the file to sit directly in src/. The registered glob is src/**/*.cts, which includes subdirectories. 28 .cts files - health-diagnostic-rules/ (10), installer-migrations/ (11), observability/ (3), host-integration-adapters/ (2), vendor/ (2) - are inside the registered glob and silently skipped. Measured with the gate neutralized: 0 violations there today. The hole is hiding nothing right now, and is fixed anyway, because "no violations today" is not a property that keeps holding. The fix is not invented: require-subprocess-timeout.cjs:196 already carries the correct form of this guard, /(?:^|\/)src\/.*\.cts$/ with .*, one directory over. Checked the other 21 rules for the same bug - no-adhoc-regex-escape and no-private-binary-resolution short-circuit only to exempt their own seam file, which is the right shape, and no-crlf-fragile-split has no filename gate at all. This bug is unique to the one rule. 2. no-adhoc-regex-escape could not see the shape that actually occurs. Line 396 gated the whole UNSAFE-NEW-REGEXP arm on arg.type === 'Identifier'. Every check below it - the _SOURCE provenance check, the isSoleReturnOfOwnParameter shape - lives inside that branch, so new RegExp(obj['key']) and new RegExp(cfg.pattern) were never examined at all. Runtime data arrives as a property access far more often than as a bare identifier, which is exactly why this rule never fired on the #3477 ReDoS. Widened to MemberExpression, measured by AST walk across all five registered blocks rather than by grep. 27 sites, zero TSAsExpression: 18 safe new RegExp(X.source, flags) -> exempted, keyed strictly on the PROPERTY being `source`, never on the object. Keying on the object would wave through X.anything and buy nothing. B6 estimated ~10; that was an undercount. 3 _SOURCE-suffixed constants reached through a required module namespace (phaseId.BRACKET_PHASE_TOKEN_SOURCE) -> the same provenance-exempt class the rule already recognizes for bare identifiers, extended to reach them. Without this the widening produces 3 false flags. 6 real findings -> marked, each a test extracting a pattern from a shipped file at test time, where the runtime contract IS the product. Deliberately the NARROW MemberExpression form. The rule's own isSoleReturnOfOwnParameter doc comment records that an earlier broad "any non-literal identifier" heuristic produced ~25 false positives and was rejected; a re-run of the census after this change flags exactly the 6 above and nothing else. Verified by execution, not by reading: the gate now accepts src/<subdir>/x.cts, still accepts flat src/x.cts, and still exempts paths outside src/ - each pinned by a test proven to fail against the old regex. build:lib, lint and lint:ci all exit 0. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3951): give no-adhoc-markdown-parsing its reach, and fix the 80 parses it finds The rule self-gates on filename AND is registered on one glob, so widening either half alone is inert. Both move here: the gate now accepts tests/**/*.cjs and scripts/**/*.cjs alongside src/**/*.cts, and eslint.config.mjs registers it on the same two. A test pins that the gate and the registration AGREE, in both directions. The original defect was a gate narrower than its registration; the failure mode of this fix is a gate wider than its registration. Both are silent, so the test asserts the pair rather than either half. 80 violations across 43 files, all in tests/, zero in scripts/. 70 are routed through the existing seams - scanFencedBlocks, collectSection, stripFencedCode, tokenizeHeadings from markdown-sectionizer; splitTableRow, parseMarkdownTable, findTableWithColumns from markdown-table. Headerless STATE.md tables use splitTableRow per line, because parseMarkdownTable needs a real delimiter row. 10 are suppressed, 12.5%, well under the third that would have meant the rule is mis-scoped for tests/ rather than the tests carrying debt. Each names its reason: three regression guards (#3873 / bug-#21) are deliberately independent of the generator's own fence handling, and routing them through the seam would have them test the generator against itself; one is a negative-text probe that extracts nothing; six are a shell-pipe-to-jq detector whose regex coincidentally matches the table fingerprint and is not markdown parsing at all. All ten sit in tests whose subject is .md content, which is normally a reason to prefer the seam. The marker used is allow-adhoc-markdown, distinct from no-source-grep's allow-test-rule, and lint:ci's lint-allow-test-rule-refs reports the same 280/280 unverified count as before - checked rather than assumed, because those two markers are easy to conflate. The widening earned its keep immediately: it found a test that passed for the wrong reason. tests/config-field-docs.test.cjs asserted notEqual(<cell>, '600') against the TYPE column instead of the DEFAULT column. notEqual('number', '600') is true forever, so the guard against workflow.subagent_timeout regressing to the old seconds default could never fire. docs/CONFIGURATION.md:434 is `| workflow.subagent_timeout | number | 300000 | ... |`, so the default is cell index 2; the assertion is now row-scoped through splitTableRow and reads 300000. That is the argument for the widening in one case: the violation was invisible to lint, the suite was green, and the assertion was vacuous. A rule that cannot reach a file cannot tell you the file is lying. Not fixed here, and recorded rather than assumed: #3426/#3239 are NOT reachable by this widening. tests/package-legitimacy-gate.test.cjs yields zero violations even with the gate bypassed - its hand-rolled scans are real, but built from line filters and split('|') rather than the regex-literal fingerprints this rule detects. They need new detectors. The epic assumed a wider glob would catch them. build:lib, lint and lint:ci all exit 0; the post-fix census across tests/** and scripts/** is 0 violations. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3951): B7 — and #3356's defects were still live in the code B7 asks that each closed child be driven fail-first with a behavioral identity test at the CONSUMER's output. Four of eleven children had no test citing their issue number. Auditing them by BEHAVIOR rather than by number-grep changed the answer for three of the four. #3364 and #2540 — traceability only. Both were implemented by #3941 and their consumer-output tests exist and were shown failing-first; neither cited its originating issue, so an audit that greps for the number reports them uncovered. Tagged the specific asserting test in each file, following the citation form those files already use. #3372 — covered, but only at helper level, and the triage narrowed it. Of the four commands the issue names, only estimate-cli's collectCalibrationSamples actually enumerates phase dirs from disk; smart-entry, audit and roadmap-upgrade derive from ROADMAP/body text and never reach the sentinel path, so they are benign by construction and were left alone rather than "fixed" into churn. The existing #3882 rows asserted the helper's return value. Added a consumer-output test driving `query estimate-calibrate` and asserting sample_count and the persisted document. RED proof: reverted collectCalibrationSamples to a raw readdirSync and ran the real CLI - sample_count 3, sentinel leaked; restored - sample_count 2. #3356 — NOT covered, and BOTH halves of the defect were still live in source. The issue is closed; the bug was not fixed. Fixed here rather than writing tests that document a bug as correct. Defect 1, the contradicted row. quick.md:627 claimed `quick-tasks-append` performs "the equivalent write" to the Step 7c row. It did not: the `#` cell was a positional ordinal and `Directory` read `—`, because the route had no way to receive a quick id or task directory. Added OPTIONAL `--quick-id` / `--slug` / `--directory`. A caller with neither - fast.md, the original #2133 caller - omits them and gets the byte-identical prior row, so nothing existing changes. A caller that HAS a real id and directory now gets the canonical row quick.md:632 renders. The false-equivalence sentence itself is corrected rather than left to mislead the next reader. Defect 2, the forced re-derive. The route called readModifyWriteStateMd with no options, so a body-only append to the Quick Tasks table triggered a full re-derive of the disk-derived progress.* frontmatter. Every other body-only writer passes { resync: false } - src/state.cts's own docstring prescribes it - and this route was the lone outlier. RED proof: reverted the option, seeded a project with 2 real phase dirs and a curated total_phases of 25, ran quick-tasks-append; total_phases collapsed to 2. Restored; it stayed 25. That second one is the shape this epic exists to close: a silent write that replaces curated state with a re-derivation nobody asked for, exit 0 throughout. build:lib, lint and lint:ci all exit 0. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3951): amend B6's ledger to what was measured, and document the new flags The ADR gains a ledger amendment in its own correction style - the sixth wrong premise it records, found the same way as the other five, by measuring before building. B6 says the net guard count must fall. It rose: 62 -> 69, +7, measured from the epic's filing commit to origin/next. The attribution is the point, though. Five of the seven came from PRs unrelated to this epic, one was added by a phase of it, and the epic did retire something sub-file - #3884 removed a detector with an explicit "net: -1 detector, 0 added" ledger. Every named casualty is load-bearing, two already carry retractions in this same document, and a sweep of all 22 rules plus every scripts/lint-* found no provably dead guard. There is no honest way to make the count fall; forcing it would trade coverage for a number, which is the Goodhart outcome Decision 6 exists to prevent. The amendment also records that B6's own prescribed fix for one widening was inert. no-adhoc-markdown-parsing self-gates on its filename, so widening only the files: glob - which is what the criterion says to do - ships a rule that still returns {} for every new path. And #3426/#3239 are not reachable by that widening at all; their scans use line filters and split('|'), not the regex fingerprints the rule detects. The roster row tracked them against the wrong mechanism. Three roster rows updated from aspiration to fact: the two widenings are DONE with their measured counts, and lint-phase-enumeration-drift is marked RETAINED rather than "expected casualty - verify before retiring", because Phase 5 verified it and kept it. The rule Decision 6 should carry forward is stated plainly: a guard ledger is a claim about COVERAGE, not about COUNT. "Net count must fall" is measurable and wrong. "Every guard is reachable, and each retirement names what makes its defect unrepresentable" is the property that was actually wanted. CLI-TOOLS.md documents the optional --quick-id/--slug/--directory flags and says plainly that omitting them keeps the pre-#3356 row byte-identical, plus that the append no longer re-derives progress frontmatter. New features fragment (id 3951); FEATURES.md regenerated rather than hand-edited. Changeset is Changed, pr:0 pending backfill. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): correct four rows that pinned the lint rule's old narrow reach The remote suite came back RED with 5 failures, all in tests/eslint-rules.test.cjs. They are stale tests, not a regression: four rows assert that no-adhoc-markdown-parsing is inert outside src/*.cts, which is exactly the contract this deliverable changes. Confirmed by reading rather than inferred from the names - the row at :1981 used filename: 'tests/some.test.cjs' and filename: 'scripts/helper.cjs', the two roots the rule now covers on purpose. Worth recording WHY local gates missed this. npm run lint and lint:ci were green, and the touched test files passed standalone. Lint only reports violations in real files; these rows assert the rule's REACH using synthetic RuleTester filenames, so nothing but the full suite could see them. Local green on a rule change says nothing about the rule's own tests. Each row is rewritten with BOTH halves rather than flipped from valid to invalid: - the same fingerprint under tests/ or scripts/ is now flagged, with the right messageId - the negative space is preserved - the same fingerprint under a path outside all three roots (gsd-core/bin/lib/foo.cjs) is still NOT flagged The second half is the one that matters. Without it the rule has no boundary and nothing would catch an over-wide gate later, which is the mirror image of the bug this deliverable just fixed. Each row is renamed to state the current contract; the old names said "non-src/*.cts ... is not flagged" and would have been actively misleading once the bodies changed. Proven to test the widening rather than restate it: every flagged half was run against HEAD~2's pre-widening rule and does NOT fire there, then against the current rule and does. 12/12 on that probe; the full file is 178/178. Swept for the same staleness elsewhere and found none. require-subprocess-timeout's own "inert outside src/*.cts" row is untouched - that rule's gate was not widened here - and no-adhoc-regex-escape's test file already carries correctly-targeted rows. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): acknowledge the quick.md growth the attribution guard reported The full suite came back RED with one failure, and it is mine: 1 file(s) grew without an acknowledgment: quick.md grew 364 bytes gsd-core/workflows/quick.md is runtime-loaded emitted content, so correcting its false 'performs the equivalent write' claim trips emitted-attribution by construction. This is the acknowledgment, not a workaround - there is nothing to regenerate. The fragment names ONE path, which is the only one the guard reported. The four spent acknowledgments it also listed (audit-uat, plan-phase, progress, review) belong to other fragments whose ripple the base already absorbs; they are inert, not failures, and are deliberately NOT copied here - naming paths I did not change would make this record false in the other direction. Byte figure corrected before committing: the guard reported 37220 -> 37584 (+364), but origin/next has since moved and quick.md is 37232 there now, so the measured delta is +352. The reason text says so and names the base as a moving figure rather than pinning a number that is already stale. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): move the quick.md growth ack to a trailer, delete the obsolete fragment The acknowledgment mechanism changed under this branch. Merging next brought in the redesign - it also deleted .github/workflows/ack-fragment-sweep.yml, which was in the merge status and which I did not register at the time - and the guard now says so directly: Add a trailer to a commit in this PR (never a new file). Emitted-Drift-Ack-Growth: quick.md - <why this growth is deliberate> So tests/emitted-drift-acks/3951-quick-append-equivalence.json is obsolete on arrival. A fragment file is no longer read by anything, and leaving it would be a dead record that looks like an active one. It is deleted here rather than kept "just in case". The byte figure moved again with the merge: 37232 -> 37596, +364. The earlier fragment said +352, measured before the merge auto-merged quick.md itself. The trailer carries no number, which is the better design - the figure was stale twice in two attempts. Refs #3951 Emitted-Drift-Ack-Growth: quick.md — #3356/#3951 replaces a false claim with an accurate one. Line 627 said the `quick-tasks-append` shortcut "performs the equivalent write" to the Step 7c row rendered above it; it did not, and that was the documented half of #3356 — with no quick id or task directory the route emitted a positional ordinal in `#` and an em-dash in `Directory`, a visibly different row. The corrected sentence has to carry three facts the original elided: what the shortcut actually writes when it has neither input, that this is honest behavior for its real caller (`fast.md`, which has neither), and how a caller with both now gets the byte-identical canonical row via the new optional `--quick-id`/`--slug`/`--directory` flags. Prose is the product here — an executing agent reads this line to decide whether the shortcut is safe for its case, and a shorter correction would either drop the flags (leaving the reader unable to act on the fix) or drop the limitation (recreating the false claim in gentler words). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3951): backfill changeset pr number Refs #3951 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-27 23:10:49 -04:00
Tom Boucher	52b11ee811	fix(#3763): pass --raw at every shipped config-get bash call site (#3961) ... * test(#3763): guard every shipped config-get substitution on --raw * fix(#3763): pass --raw at every shipped config-get bash call site config-get without --raw prints JSON.stringify(value), so string-typed values reach bash with literal quotes and every string comparison silently never matches (#3763). --raw added at 75 command-substitution sites across shipped content; four JSON consumers (default_reviewers, sub_repos, pr_body_sections, code_review_depth_overrides) deliberately keep default JSON output. Emitted-Drift-Ack-Growth: ai-integration-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: audit-fix.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: autonomous.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: cleanup.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: code-review.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: complete-milestone.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: discuss-phase-assumptions.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: do.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: eval-review.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: execute-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: execute-plan.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: fast.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: graduation.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: gsd-executor.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: health.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: import.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: inbox.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: ingest-docs.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: mvp-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: new-milestone.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: next.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: plan-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: plan-review-convergence.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: plant-seed.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: profile-user.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: progress.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: quick.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: remove-workspace.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: secure-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: settings-integrations.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: settings.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: ship.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: sketch-wrap-up.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: sketch.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: smart-entry.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: spike-wrap-up.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: spike.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: ui-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: ui-review.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: undo.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted Emitted-Drift-Ack-Growth: validate-phase.md — #3763: bytes from '--raw' at config-get call sites so string-typed config values reach bash comparisons unquoted * chore(#3763): changeset fragment (pr number backfilled after PR creation) * chore(#3763): backfill changeset PR number (3961) --------- Co-authored-by: sim <sim@local>	2026-08-27 19:55:34 -04:00
Tom Boucher	929e02cb2c	enhance(#3885): no silent swallow, and no verdict manufactured from dropped data (#3925) ... * test(#3885): failing-first coverage for the depth bound and the manufactured wave verdict ADR-3473 §8.5 says a swallowed failure may not become an authoritative-looking answer. Three families do exactly that today; this commit pins each one RED. Measured on this tree, 2026-08-27: intel query, .planning/intel/file-roles.json nested 12000 deep -> exit 1, "Error: Maximum call stack size exceeded" searchJsonEntries / matchesInValue carry no depth parameter at all. The MAX_JSON_SEARCH_DEPTH = 48 bound existed in the retired SDK lineage (sdk/src/query/intel.ts at 11918dcc3^) and the surviving .cts lineage never received it. same fixture nested 48 and 49 deep -> both return total=1 at exit 0, truncated=undefined Nothing distinguishes "searched to the bottom" from "stopped looking". query phase-plan-index, a plan whose depends_on names an unresolvable token -> warnings: ["Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"] The token is never mentioned. computeDependencyLevels drops the edge with `if (!resolvedDep) continue;`, every plan becomes a root, and the tool then reports the author's correct wave: as the thing that is wrong. countPhasePlansAndSummaries with fs.readdirSync throwing EACCES -> hasContext:false, indistinguishable from a phase that simply has no CONTEXT.md. context_read_error is undefined. The shapes these tests assert against, chosen here so the implementation has a target rather than inventing one later: `truncated: boolean` on the intel query result, `unresolved: Array<{plan, token}>` from computeDependencyLevels, and `context_read_error: string | null` per analyzed phase. Deliberately green, and they must stay that way — each stops the fix from over-firing: depth 48 is found and NOT flagged truncated (the ceiling is inclusive) a shallow miss reports no truncation (noise control, N1) 10,000 siblings at depth 2 are unaffected (the bound is DEPTH, N2) a genuine wave: mismatch on a fully-resolved DAG still warns (N3) a genuinely missing directory is absent, not an error the emitted depends_on display mapping still passes an unresolved token through verbatim — already pinned by the existing #3785 test, so no duplicate was added T31 asserts at the consumer's output per ADR-3180 Decision 4(b): it runs the real CLI and reads the emitted JSON, because a unit assertion on computeDependencyLevels would have passed throughout #3427's life. Design: .gsd/phase/feat-3885-no-silent-swallow/40-design.md Test matrix: .gsd/phase/feat-3885-no-silent-swallow/50-test-matrix.md Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3885): no silent swallow, and no verdict manufactured from dropped data Implements ADR-3473 §8.5. A failure or a gap in the input stops being absorbed into an output that reads as authoritative. The recursion bound, restored but NOT verbatim (src/intel.cts) MAX_JSON_SEARCH_DEPTH = 48 is threaded through searchJsonEntries and matchesInValue, which carried no depth parameter at all. The bound existed in the retired SDK lineage (sdk/src/query/intel.ts at 11918dcc3^) and the surviving .cts lineage never received it — §8.3's "a consolidation may not delete an invariant along with the surface that held it", demonstrated. Measured before: a .planning/intel file nested 12000 deep exits 1 with "Error: Maximum call stack size exceeded". Reachable from a project document. The original returned a bare `false` at the ceiling. Restoring that verbatim would trade a crash for a silent "no match" when the truth is "I stopped looking" — the same class this epic exists to close, and ADR-3473 Decision 4 forbids it. So the bound carries a truncation signal: nesting 47 -> found, truncated false nesting 48 -> found, truncated false (the ceiling is inclusive) nesting 49 -> not found, truncated TRUE nesting 12000 -> exit 0, truncated TRUE, no RangeError A shallow document that simply has no match reports truncated FALSE — the flag means "I stopped early", never "I found nothing", or it would be noise. The bound is on DEPTH: 10,000 siblings at depth 2 are unaffected. The dropped edge is named, and stops being blamed on the author (src/phase.cts) computeDependencyLevels dropped every unresolvable depends_on token with a bare `continue`. Each drop makes a plan a root, so the whole phase collapses to wave 1 — and cmdPhasePlanIndex then reported the author's CORRECT wave: as the thing that was wrong. Before: warnings: ["Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"] After: warnings: ["Plan 03-02: depends_on token \"nonexistent-token-3427\" does not resolve to any plan in this phase — edge dropped, wave placement for this plan may be unreliable"] The suppression is PER PLAN, never blanket: a plan with a fully-resolved DAG and a genuinely wrong wave: still gets the mismatch warning. resolveDependencyId stays two-tier — the shortFormToId third tier is §8.3/Phase 6's rule and is deliberately not built here. The emitted depends_on display mapping still passes an unresolved token through verbatim (#3785). No artifact from failed inputs (gsd-core/workflows/review.md, #3352) A failed lane leaves no result file, so "every lane failed" is exactly "the aggregate JSONL has zero lines" — the gate condition already existed as a byproduct. REVIEWS.md is no longer written in that case, and the commit step is skipped with it. A budget-SKIPPED lane also leaves no file and is NOT counted as a failure. Per-lane output and non-empty .err are preserved to .review-diagnostics/ before `rm -rf "{run_dir}"` destroys the only record that the lanes failed at all; the commit step names one file, never a glob, so the diagnostics are not swept in. Unreadable is not absent (roadmap.cts, gap-checker.cts, init.cts x2) Four callers collapsed an EACCES on a phase directory into [] and reported hasContext:false — byte-identical to a phase that simply has no CONTEXT.md. Each now names the directory it could not read. A genuinely missing directory stays absent rather than becoming an error, which is what keeps the fix from over-firing. Fatal errno folded into a retry set: audited, no defect found Reported as a verified negative rather than padded with a change. withPlanningLock was fixed by #1884/PR #3472; acquireStateLock by #3776; atomicRenameWithRetry and estimate-cli's renameWithRetry are correct by construction — bounded set {EPERM,EBUSY,EACCES}, bounded attempts, and they return or rethrow the final error rather than swallowing it. estimate-cli's sole caller surfaces that rethrow as write_error in its JSON output. Manufacturing a diff to make the checkbox look worked-on is the Goodhart outcome Decision 6 exists to prevent. Disclosed: R46 (the commit step names one file, never a glob) is a real regression guard but is NOT independently failing-first — the commit fence is byte-identical pre- and post-fix, so it only fails pre-fix through its shared extraction dependency. Recorded rather than claimed as fail-first. Design: .gsd/phase/feat-3885-no-silent-swallow/40-design.md Test matrix: .gsd/phase/feat-3885-no-silent-swallow/50-test-matrix.md Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3885): escape untrusted tokens, and stop cleanup destroying unpreserved evidence Two review findings, both real, both in my own change. An isolated adversarial review found the evidence-preservation block never checked mkdir/cp exit status while `rm -rf "{run_dir}"` ran unconditionally in a SEPARATE fenced block. A disk-full or unwritable phase directory therefore still destroyed the only copy of the failed lanes' output — reintroducing the exact #3352 data loss this item exists to stop, inside the fix for it. Preservation and cleanup are now one block, because each fenced block is a separate execution and a shell variable cannot carry between them. mkdir -p and each cp are exit-checked; cleanup runs only when preservation succeeded, and a failure warns naming the intact run directory. "Nothing to preserve" is not a failure and still cleans up. Driven three ways: success removes run_dir, failure leaves it intact with the warning, nothing-to-preserve removes it. The failure is induced by a file-vs-directory conflict rather than chmod 0o000, which root bypasses. The new unresolved-depends_on warning embedded a user-authored token verbatim: warnings: ["Plan 03-02: depends_on token \"evil Plan 03-01: FORGED WARNING\" does not resolve ..."] The JSON wire form is safe, and the security reviewer judged it non-exploitable for that reason. It is escaped anyway through formatDiagnosticToken — the helper #3884 added one phase earlier for exactly this class. warnings[] is an array a consumer naturally prints line by line, and not reusing the sibling fix is the generative-fix-divergence shape this epic exists to close. The same treatment is applied to context_read_error / phase_dir_read_error, which embed a phase directory path a repository can choose, and to the fs error message, which echoes the raw path itself. Known limit L5 recorded: the bound is on DEPTH only. A 300,000-element shallow array yields a 14.5MB reply with truncated:false. Correct per §8.5 and per negative space N2, disclosed rather than left to be discovered. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3885): unreadable is not absent in intel.cts either, and a corrupt snapshot is not "no snapshot" Blocker from the round-2 isolated review, and it is my own inconsistency: this phase applied "unreadable is not absent" to phase directories and left it broken in the file it was already editing. chmod 000 .planning/intel/file-roles.json gsd-tools intel query <term> -> {"matches":[],"total":0,"truncated":false} exit 0 safeReadJson swallowed every read failure and returned null, so an EACCES was byte-indistinguishable from an absent file AND from a genuine no-match. Now it separates three states: ENOENT stays silently absent, because not every project has every intel file and intelQuery loops over all of them expecting misses; EACCES/EIO and malformed JSON are both surfaced naming the file. A corrupt intel file previously read as "no matches" too — same defect, same fix. Threading that outcome through the other three callers found something worse than the reported case. intelDiff returned no_baseline:true for a corrupt or unreadable snapshot — not a silent failure but an actively FALSE verdict, telling the caller they never took a snapshot when they did. That is §8.5's headline case, so it is fixed and tested rather than noted. intelStatus and intelApiSurface collapsed the same way; intelApiSurface additionally printed a "not yet populated" banner that was simply untrue. Every row is failing-first, including the absent-file ones — the field is new, so it does not exist pre-fix at all. Those rows are not pre-fix pins; they pin that the fix does not OVER-fire on the ordinary absent case, which is what would turn this into noise on every project lacking an intel file. IO failure is injected by monkeypatching fs and restoring in finally, never chmod 0o000 — root bypasses mode bits, so the reviewer's manual chmod repro is not reproducible as a test. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): build the pathological intel fixture as text, not by stringifying a nested object The remote runner came back red on Linux with two failures, both T4: deeplyNestedIntelDoesNotOverflowTheStack, while the same test passed on macOS. The product was never at fault. writeNestedFixture(12000) built a 12,000-deep JavaScript OBJECT and then JSON.stringify'd it. JSON.stringify recurses once per level, so it overflowed the TEST PROCESS's stack — the error was thrown before the CLI was ever spawned. Linux's container stack is smaller than macOS's, which is the whole of the platform difference. Measured, with the same document built as JSON TEXT so nothing in the building process recurses: depth=100 rc=0 truncated=true depth=5000 rc=0 truncated=true depth=12000 rc=0 truncated=true depth=60000 rc=0 truncated=true V8 parses this shape iteratively; only stringify recurses. The bound works at every depth tried. The fixture is now built by string concatenation. That is also the more faithful input — a real deeply nested JSON document on disk is exactly what the bound guards, where a stringified object was only ever a way to produce one. The depth stays 12000. Lowering it would have made the test pass by weakening it to accommodate a fixture bug, and 12000 is a legitimate pathological input the product handles. T4 remains a genuine fail-first: rebuilt against the parent of the commit that added the bound, the string-built depth-12000 fixture still drives the CLI to rc=1 with "Error: Maximum call stack size exceeded". A comment records why the fixture is text, so it is not "simplified" back into a macOS-green / Linux-red test. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3885): backfill the changeset PR number Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): normalize path separators before splicing into the workflow's bash CI red on one lane — test (windows-latest, 24, shard 3/3). macOS, Linux and the remote runner were all green. AssertionError: commit must name the single REVIEWS.md file; got: --files C:UsersRUNNER~1AppDataLocalTempgsd-3352-phasedir-mOKmuy/03-REVIEWS.md Every backslash in C:\Users\RUNNER~1\AppData\Local\Temp\... was eaten. The harness spliced an OS-native temp path into the extracted bash, and bash consumes \U, \A, \L and \T as escapes on an unquoted expansion. The same loss broke RUN_DIR, so "rm -rf" targeted a path that never existed and the run directory survived — which is the other two assertions. This is a fixture defect, not a product one, and that was checked rather than assumed. In production the phase directory is toPosixPath-normalized at every call site that serializes it (bin/lib/init.cjs:951, 1381, 1461, 1529, 1595), and the run directory is created by "mktemp -d" running inside the bash block itself (gsd-core/workflows/review.md:163), which emits POSIX-style output even under Git-Bash on Windows. Neither ever carries a backslash where the workflow reads it. The file's pre-existing #3034 harness splices raw native paths too, but only ever inside double-quoted assignments, so it never tripped this — my new harness followed that convention faithfully into the one place where it does not hold. Both now splice through toPosixPath from shell-command-projection, the established seam, which is a no-op on POSIX and mirrors what production does. No assertion was weakened. "commit must name the single REVIEWS.md file" and "the run dir must still be destroyed" still assert exactly that; only how the fixture supplies its path changed. Nothing is skipped on Windows — a t.skip() here would have hidden the question of whether the exposure was real, which is the question that mattered. Driven both ways: a synthetic C:\Users\RUNNER~1\... input reproduces the exact CI string when unfixed and yields C:/Users/RUNNER~1/... when fixed; a POSIX input produces a byte-identical shape, proving the normalization is idempotent. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): stop the harness making the deleted run dir its own cwd Windows shard 3/3 stayed red after the separator fix, on two assertions the separator fix never touched: AssertionError: the run dir must still be destroyed AssertionError: nothing to preserve is not a failure — run dir must still be removed The separators were a real bug and fixing them fixed the --files assertion. They were not this bug, and two CI cycles went into the wrong axis before I stopped converting path forms and looked at what the harness actually does. runWriteReviewsFlow passed cwd: runDir to runHook, so the child bash process's working directory WAS the directory the block under test then removes with rm -rf "$RUN_DIR". POSIX allows a process to delete its own cwd — verified locally, cd "$d"; rm -rf "$d" removes it cleanly — and Windows does not: a live process's working directory cannot be removed. So on Windows the directory survived and both assertions failed, on macOS and Linux it vanished and they passed. Nothing to do with slashes. Harness-only. Production never cd's into the run directory; every reference is by absolute path, and RUN_DIR is created by mktemp -d inside the bash block itself (gsd-core/workflows/review.md:165) rather than injected. review.md is unchanged. Fix: the child now runs with its cwd in an unrelated temp directory that the block under test never deletes. Neither assertion was weakened, and nothing is skipped on Windows — the tests in this file carry no platform guard and run there unconditionally, which is how this surfaced at all. Honest limit: the Windows failure mode cannot be reproduced on macOS, because POSIX permits the very thing Windows refuses. The diagnosis is grounded in that documented divergence and in the fact that only the Windows lane failed, but the green outcome on windows-latest is unverified until CI runs it. Refs #3885 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-27 04:12:47 -04:00
Tom Boucher	832dcbb751	fix(#3707): surface UAT rows audit-uat silently dropped, and never report a clean result for a file it could not read (#3887) ... * test(#3707): failing-first coverage for the three parseUatItems false negatives Nine tests that must be red and three controls that must already be green. The controls are the point of the split. `result: pass` staying unsurfaced is what stops the fix inverting the filter so eagerly that every passing test becomes an outstanding item, and the classic single-line shape is the no-churn control for rewriting the adjacency regex. Both were confirmed green against the current build before being written down; a control that is red today would be a second bug, not a control. Each failing fixture was run through the built parser first and returns [] for its stated cause — the issue row matched then filtered, the block-scalar and wrapped rows never matched at all, the all-unparseable file vanishing whole. That evidence is in 50-test-matrix.md rather than asserted. Tests target ../gsd-core/bin/lib/uat.cjs, the built live module, and drive the real CLI through runGsdTools. #3706 lost a full RED/GREEN cycle to tests that imported a different copy of the function under test, so the import target was verified before anything was written. * fix(#3707): stop parseUatItems dropping outstanding UAT rows Three independent false negatives, all in the audit path, plus one the issue did not mention. The matcher no longer requires `expected:` and `result:` to be adjacent single lines. It slices each `### N.` block to the next heading and reads the first `result:` line within it, taking `expected:` from parseExpectedFromTestBlock — the seam that already parsed both the block-scalar and inline forms correctly and was sitting unused two hundred lines away. Two parsers in one module read the same field with different grammars; now there is one. The result filter is inverted from an inclusion list of three to an exclusion of a minimal PASS set. This was the issue's one open design question, which the reporter explicitly declined to answer for the maintainer; it was asked and decided deliberately. The fail-safe direction is what parseGapsItems documents seventy lines below for this same false-negative class (#2286): a token nobody recognised surfaces rather than vanishing. The trade is a visible, correctable false positive if a project invents a novel pass-word, against today's silent and invisible drop. `issue` also needed a category. It is template-sanctioned with its own `issues:` counter, but categorizeItem fell through to `unknown` — surfacing it in the wrong bucket would have been a half-fix. Finally, a file parsing to zero items no longer vanishes with its frontmatter `status:`. One with a non-terminal status is reported with `parse_gap: true`, so the reader gets a cue to look; a `complete` one stays omitted as before. That is what made the first two defects dangerous rather than merely lossy — the audit omitted the phase instead of under-counting it. * fix(#3707): close the review blockers, including a regression I introduced The remote suite was RED on the previous commit and both reviews found real defects. Everything below was verified by execution, not by reading. I introduced a regression against origin/next. The rewritten result matcher was END-anchored where the old one was not, so `result: pending (blocked on staging)`, `result: [skipped] # no device` and `result: blocked - waiting` all returned a row before this branch and returned nothing on it — me reproducing the exact defect class this issue exists to kill, in the fix for it. The anchor is gone and each shape has a regression test; trailing text now falls back to `reason` when the block has none. `parse_gap` was inferred from the wrong signal. It fired for ANY zero-item file whose status was not `complete`, which asserted something false about a perfectly-parsed all-pass file, swept in archived phases left at `testing`, and is what turned the #2286 Gaps tests red — a control this change was supposed to keep green. It now derives from headings SEEN BUT UNYIELDED, reported by a new parseUatItemsWithStats, so an all-pass file and a Gaps-only file are not parse gaps and a file whose blocks carry no `result:` line is. The fix was also invisible end to end, which both reviewers caught independently. parse_gap entries carry no items, and both audit-uat.md and progress.md gate on `total_items === 0` — so the headline symptom, the phase vanishing, still reproduced for a user and only the raw JSON had changed. There is now a `parse_gap_files` counter and both workflows gate and report on it. Also: categorizeItem compared case-sensitively while the new PASS check lowercased, so `result: PENDING` surfaced as `unknown`; blocks are bounded at the next heading of any level, so a trailing `## Gaps` entry no longer bleeds its `reason` onto the preceding test; dead unreachable fallbacks removed; and the all-pass control was strengthened, since asserting only `total_items === 0` let it stay green through the bogus parse_gap entry. * chore(#3707): acknowledge the workflow growth the fix required The emitted-attribution guard went red because audit-uat.md and progress.md grew, and it is right to ask: runtime-loaded workflow prose is the product, so growth there is a real change to what an executing agent reads. The growth is not incidental to this fix, it IS the fix reaching a user. Both reviewers found independently that emitting `parse_gap` in the JSON changed nothing observable, because both workflows gated their output on `total_items === 0` and parse-gap entries carry no items — so a phase whose rows could not be parsed still printed "All Clear" and still vanished from the progress report. The widened gates and the branches that name the unparsed files with their phase and path are what close that. Acks exactly the two paths the guard reported, keyed on the bare filename. The three spent acknowledgments it also listed are inert by its own description — the base already absorbs them — so they are left alone rather than swept up here, where they would just add unrelated churn to this diff. * fix(#3707): close the mixed-file blocker and the second false-clean surface The suite was GREEN and the isolated review still found a blocker, which is the useful part: none of this was covered by a test. A MIXED file dropped its unparseable rows silently. `parse_gap` sat behind an `else if` on `items.length > 0`, so one parseable row was enough to discard `headingsSeen` entirely — a file with one pending row and two unreadable blocks reported one item and no gap. That is the exact class this issue exists to kill, reappearing inside its own fix for the third time. The flag is now set independently of item count and the entry carries `unparsed_blocks`, so the count is quantified rather than merely flagged. A `result:` inside a fenced code block was being read as real, so a PASSING test could be reported as outstanding from a value in a code sample — another regression against origin/next, whose adjacency regex ignored it. Field scans now run against a fence-stripped copy while `expected:` still reads the raw block, since a block scalar may legitimately contain fenced-looking text. The workflow report was still unreachable whenever anything else was outstanding: the unparsed table lived in the all-clear branch, so a project with one pending row in phase 01 and an unreadable phase 02 rendered phase 02 nowhere. It now fires on `parse_gap_files > 0` from the `present` step. planning-inspect was the second surface making a false-clean claim — for exactly the files audit-uat now flags it emitted `scope: 'complete'` with an empty unresolved list, positively asserting completeness over a file it could not read. It consumes the stats now and reports SCOPE.TRUNCATED with a `uat_unreadable` diagnostic, reusing the vocabulary already used two lines above for an unreadable file rather than inventing a token. Also: headings with no name no longer vanish whole; the trailing-text-to-reason synthesis I had added is removed, since it was never required by the blocker and silently changed categorization for `result: [skipped] # no device`; the emitted `result` token is normalized to lower case so it agrees with `category` in a published contract; and an O(n^2) indexOf is gone from the heading loop. * fix(#3707): stop rows stealing each other's fields, on all three surfaces The suite was green when the security review found these. Two are blocker-severity and one of them is a direct hit on my own verification. A `### N.` line indented two spaces inside an `expected: |` scalar is a valid ATX heading, so it became a phantom row that STOLE the real row's result token while the real row vanished. I had probed this shape and declared it fixed — my probe asserted the item COUNT and the result token, both of which the phantom satisfied, so it passed for exactly the reason it should have failed. Block scalar bodies are now masked to blank lines (line count preserved, so offsets still line up) before headings are tokenized, and the tests assert row IDENTITY — number and name — not presence. Feeding parseExpectedFromTestBlock the raw slice let one row publish another row's `expected:` from inside a fence the stripped view had correctly excluded. Blocks handed to it are now clipped at the first fence opener. This was not cosmetic on the render-checkpoint path: a checkpoint banner a HUMAN reads and answers was rendering a different row's expected text. A balanced fence pair straddling a test block made that heading invisible, so an outstanding row disappeared with no item, no gap and no count — the exact false-clean this issue exists to close, and a regression against origin/next. Suppressed `### N.` lines now count toward headingsSeen so the file is flagged. An unterminated fence swallowed the rest of the document including `## Gaps`, producing a whole-file false clean. Such a file is now treated as a parse gap, following what uat-predicate already does. Found and fixed inline while there: parseExpectedFromTestBlock's scalar opener required a bare newline, so a CRLF `expected: |` fell through to the inline arm and published `expected: "|"`, silently discarding the entire value. The same fall-through hit `|-` and `|+`. parseFirstPendingTest had the identical exposure on the render-checkpoint path and now shares the same masking and clipping. Five legitimate fixtures — inline expected, a real block scalar, CRLF, bracketed pending, and a first-pending that is not the first test — are byte-identical before and after. Also from the code review: the admit condition disagreed with the terminal status guard, so a `status: complete` file with an unparseable block was emitted as an empty entry that rendered nowhere but inflated total_files; a control test was vacuous because its fixture filename did not match its phase dir, so #3511 scoping meant the file was never opened — and that vacuity is why the admit regression shipped green; an unterminated fence discarded the flag that would have caught it; `### 1.2.3` parsed as test 1; and planning-inspect did not share the terminal-status rule. * test(#3707): assert what the render-checkpoint fix actually does The suite went red on three of my own tests and the source was right — the assertions were wrong, in a way worth naming. One forbade the rendered checkpoint from containing `### 3. Fake Row`. But in that fixture the string IS row 1's legitimate `expected:` block-scalar value; a heading-shaped line inside a scalar is inert text and rendering it is correct. The test was forbidding correct output. It now asserts row IDENTITY — the checkpoint is for test 1 named Alpha and never test 3 named Fake Row — which is the property that actually distinguishes the fix from the bug. The other expected success where the correct outcome is a clean error: row 1 in that fixture has no `expected:` of its own and only ever appeared to have one by stealing row 2's from inside a fence. Depending on the bug to produce a pass is how a test ends up pinning the defect. The fixture now gives row 1 its own value and asserts the checkpoint carries it and never the fence-hidden text, and the error path gets its own test asserting it fails cleanly without leaking. All three were checked against the real rendered output before the assertion was written, and each was reasoned through for whether it can fail: the identity test breaks if a phantom row is parsed, the clipping test breaks if the raw block is read again, and the error test would pass-not-fail under the old stealing behavior. * fix(#3707): correct the scalar masking frame and cover every YAML block opener Two reviews independently found the same blocker, and it is the sharpest defect on this issue: maskBlockScalarBodies computed line offsets in UTF-16 units but spliced them into Array.from(content), a CODE POINT array. One emoji anywhere earlier in the file shifted every later mask write, so the mask blanked the wrong characters and spilled past line ends. Measured: at two astral characters a result token truncated `pending` to `pendi` and recategorized to unknown; at six the real row vanished; at twelve the FOLLOWING row's `result: blocked` disappeared and the file reported clean. That is the false-clean class this issue exists to close, reintroduced by the mitigation written to prevent it, and defeating both new detectors at once. The mask is rebuilt line by line now, which is frame-agnostic and length-preserving by construction. The opener grammar was also incomplete. YAML block scalar headers take an optional indentation indicator and an optional chomping indicator in either order, so `|2`, `|2-`, `|-2`, `>2`, `>2+` are all valid — and none were matched. An unmasked `expected: |2` body meant a `### N.` line inside the value became a real heading: reproduced, row 1 disappeared and a fabricated row 2 named "Phantom" took its identity. Fixing that exposed a third instance of the same family, found by my own probe rather than by review: the value extractor understood only the `|` openers, so every `>` folded scalar published the LITERAL OPENER as its value — `expected` came back as ">" or ">2+" and the whole scalar was discarded. The extractor now shares the opener grammar and implements real folding, joining paragraph lines with a space and turning a blank line into a newline, rather than pretending `>` means `|`. Also from the reviews: the shortfall counter scanned the masked copy but not a fence-stripped one, so a `### N.`-shaped line inside a properly closed documentation fence — the ordinary way to document the row format inside a UAT file — counted as a suppressed row and flagged the file against nothing; and clipping at the first fence discarded a legitimate `expected:` that appeared after a closed fence, which is silent field loss. Every opener now verified for both row identity and exact extracted value, in LF and CRLF, alongside the emoji fixtures at 1/2/6/12. * refactor(#3707): replace the scalar masking with a column-0 heading rule The fix had grown to five helpers whose only job was undoing one over-permissive rule: tokenizeHeadings treats a heading indented up to three spaces as real, so a `### N.` inside an `expected: |` body was parsed as a row and stole the real row's identity. Every blocker in the last three review rounds came out of that machinery rather than the reported bug — worst of all a UTF-16-versus-code-point frame mismatch that corrupted any document containing an emoji. A UAT test heading is at column 0. The shipped template puts all of them there, no `*UAT*.md` in the repo has an indented one, and the only indented `### N.` lines in the tree are the adversarial fixtures that must not parse. Requiring column 0 makes a scalar-interior heading a non-heading by construction, so maskBlockScalarBodies, indentWidthOf and BLOCK_SCALAR_OPENER_RE are gone along with the mask-invariant test that existed only to guard them. The frame bug is now structurally unreachable: no code-point array or offset splicing remains. The premise was incomplete and the reviewer caught it rather than forcing it through. Masking had been doing double duty — it also hid indented FENCE delimiters from the tokenizer, so removing it let a two-space fence inside a scalar body swallow a later column-0 row. The alternative on offer was to rewrite that test to assert the row is merely counted, which is a behavior regression dressed as a passing suite. Instead there is a small line-based pass that blanks only indented fence delimiters — same "column 0 is structure" rule extended consistently, no YAML knowledge, and line-based by construction so the frame bug cannot come back. It was proven load-bearing by a negative control: reverting the wiring reproduces the regression exactly. Kept, because they fix defects column-0 does not touch: the fence clipping that stops one row reading another's `expected:` from inside a fence, and the folded scalar handling that stopped `expected: >` publishing the literal ">". Also corrects a comment left pointing at a symbol this commit deletes. * fix(#3707): blank neutralized fence bodies, and give both parse paths one grammar Reviewing the simplification found two more, and the first is row theft again — the sixth time this class has surfaced on this issue, and the second time inside a mitigation written to stop it. Neutralizing an indented fence blanked only its DELIMITER lines. If the block's body held a column-0 `### N.`, un-hiding the delimiters made that line a real heading, which then took the preceding row's fields: an `### 1. Alpha` document came back as a single row 9 named Phantom, with Alpha gone. Neutralized blocks are now blanked open-to-close, body included, which is the honest reading of the intent — an indented fence inside a scalar is content, so nothing in it should be able to produce structure. The raw block is still what the expected extractor reads, so a legitimate `expected: |` carrying a fenced code sample keeps its full text. The two parse paths also disagreed about what a test row IS. parseFirstPendingTest filtered on `^\d+\.\s+` while parseUatItemsWithStats used `^\d+\.(?!\d)`, so `### 3.Foo` was a row when audited and not a row when resumed. Both now share one predicate and one extractor. The extractor mattered as much as the filter: the checkpoint path's name-mandatory pattern would have skipped exactly the shapes the widened filter admits, so fixing the filter alone would have moved the divergence down a line rather than closing it. Also from the security pass: parseUatItems had become an export with no callers and no direct test once both consumers moved to the stats form. It stays, since deleting an exported symbol from a shipped module is a contract change and not this issue's business, but it is now documented as the items-only wrapper and has a test. And the PASS check lowercased a value that extraction had already lowercased; normalization now happens once. * fix(#3707): revert the whole-block fence blanking, and pin the rule instead My previous commit over-corrected and the suite caught it. Blanking a neutralized fence block open-to-close destroys content legitimately living between the delimiters, and on an UNTERMINATED opener it blanks to EOF and deletes every later row. No framing makes that correct, and it is what turned two earlier tests red. The "blocker" that prompted it was my own misreading. This change adopted the rule that column 0 is structure and indentation is content. Under that rule an indented fence delimiter is not a fence, so a column-0 `### 9.` sitting between two indented delimiters genuinely IS a heading, and a `result:` after it genuinely belongs to it. That document is malformed and the parser reading it that way is consistent, not stealing. Nothing is silently lost either: the row whose result was taken surfaces as the parse gap. So the blanking is back to delimiters only, and rather than leaving the question open, the behavior is now pinned by a test asserting the rows by identity, with the rule stated at the site — so the next person does not oscillate the way I just did. Kept from the reverted commit: the shared row-heading grammar and extractor across both parse paths, the parseUatItems wrapper documentation and its test, and the single point of lowercasing. * fix(#3707): scope the shortfall scan, and stop neutralized content becoming structure The security review found a HIGH that is the earlier frame-mismatch bug wearing different clothes. The shortfall scan compared a SECTION-scoped raw line count — the `## Tests` body — against a DOCUMENT-wide token count. So a single legal `### N.` row anywhere outside `## Tests` decremented the shortfall and switched the fence-straddle detector off: an identical `## Tests` section went from `headingsSeen 1, parse_gap true` to `headingsSeen 0, no gap` purely because a `## Prior` section existed. A document whose rows render as ordinary blocked rows in any CommonMark renderer audited as totally clean. Both sides of the comparison now come from the same surface, by filtering tokens to the scan span rather than re-tokenizing, so there is no second offset basis to keep in step. Neutralizing a fence could also promote its former CONTENT into structure: a column-0 delimiter run inside an indented pair became an opener once the enclosing delimiters were blanked, hiding every later heading to EOF. Column-0 delimiter-shaped lines inside a neutralized block are now blanked too — and only those, so a column-0 heading between neutralized delimiters is still a heading (the pinned behaviour) and the field lines of a row living between two scalars still survive. The reviewer corrected my repro while fixing it: an even number of inner runs re-pairs and hides nothing, so the live shape needs an odd one, and both are now tests. Two more from that pass. A legal scalar header carrying a trailing comment (`expected: | # sample`) failed the end-anchored grammar, publishing the literal header and raising a false gap. And the indented-row counter walked backwards per row: 3.6 seconds at sixteen thousand rows, now 15ms, via one forward pass — though the reviewer also established my example was not the quadratic shape, which needs an uninterrupted scalar body. Carried in from the previous round: the indented-row counter keys on any block scalar rather than only `expected:`, so a template-sanctioned `reported: |` holding user prose with a heading-shaped line no longer raises a false gap; and `reason:`/`blocked_by:` read block scalars through the same shared extractor instead of publishing the literal `"|"`, which also means a multi-line reason can finally reach categorizeItem — a `reason:` mentioning a server now categorizes as server_blocked, which was impossible while the value was thrown away. * fix(#3707): make the shortfall scan whole-document on both sides Second HIGH in this area, and the diagnosis is the useful part: I closed the first one by making the two sides agree, but I did it by NARROWING the token side to the `## Tests` span while the parse side stayed whole-document. Rows outside that section are still parsed and surfaced when visible, so when a fence straddled one it fell through both sides of the comparison — no item, no gap, file never entered the results at all. A `## Regression Tests` section, or a second `## Tests` (collectSection takes the first), audited as totally clean while origin/next surfaced those rows. Both sides are whole-document now. Symmetry is the property that matters here; every attempt to be clever about which scope to compare has produced one of these, twice at HIGH severity. That reinstates a known over-report, deliberately: a `### N.`-shaped line inside a closed fence in a `## Notes` section — the ordinary way to document the row format — counts as a suppressed row and raises a gap on a file with nothing missing. Noisy, but visible and fail-safe, against two silent false-cleans on the other side of the trade. This issue exists to eliminate false cleans, so the trade goes that way, and the reasoning is written at the site so it does not get optimized back. Three existing tests encoded the retired scoping and are replaced rather than worked around: two now assert the accepted over-report, and one asserting a 4-space row is "not counted" was already contradicted by widening the counter to any indentation — refusing to PARSE a 4-space heading is right, refusing to COUNT it reopened the hole the counter exists to close. Also in this commit, from the same review round: the inner-delimiter sweep tested a column-0-anchored pattern, so an INDENTED delimiter inside a neutralized block was still promoted to structure and lost a row; it is indent-tolerant now. A refinement was identified and deliberately not taken — keying the documentation-sample exemption on the fence info string rather than on section scope. It is content-based and symmetric, so it would not reintroduce the asymmetry, but it belongs in its own change rather than riding this one. * docs(#3707): correct two claims in the over-report justification Both from review, both comment-only, and both matter because they would mislead the next person into "fixing" something correct. The over-report note called the triggering shape "the ordinary way to document the row format". It is narrower than that: the scan requires literal digits, so the conventional placeholder `### N. Name` does not trigger it at all — only a sample written with real numbers does, and no phase UAT file in-tree has one, only the shipped template, which selectPhaseUatFiles never scans. A maintainer who tested the documented placeholder form would find no over-report and could reasonably conclude the pin was stale. That is now stated, and it also makes the trade look better than I claimed: the real-world frequency is lower. The attribution guard is described as structural rather than positional. It is positional in one respect: the walk stops at the nearest column-0 line, so a block scalar nested inside a `## Gaps` bullet is transparent to it and a heading-shaped line in that value gets counted. Same accepted over-report, reached by a path the comment did not mention — recorded so it is not later mistaken for a new defect. * fix(#3707): a complete status no longer switches off the parse-gap detector The security review named this as the last silent-clean path in the change, and its phrase is the right one: a self-declared kill switch over the very detector this issue built. A file whose frontmatter said `status: complete` was omitted unconditionally, so one containing a fence-straddled `result: blocked` computed headingsSeen = 1 — the detector fired — and then emitted no entry at all. The audit reported nothing. The predicate is now status-independent: a file is surfaced when blocks were seen but yielded nothing, whatever it claims about itself. A terminal status is an assertion by the author, and an assertion is exactly what must not be allowed to suppress the signal that would contradict it. What does not change is the thing the status is actually for — a complete file with nothing to parse, and a complete file whose rows all parse and all pass, both stay silent, verified through the real CLI. I replaced a control test of mine, and it is worth saying why that is not a weakening: its name was already false. "A zero-item file with a complete status is still omitted" used a fixture with a `### 1.` block carrying no `result:` line, so headingsSeen was 1 — it was never a zero-item file, it was the kill switch itself, pinned. The intent it claimed is now covered by two stricter tests, one for a file with no blocks and one for a file where every row parses and passes, each asserting both that no entry exists and that no items are counted, where the old test asserted only the former. Everything else is byte-identical: 61 regression cases and the non-complete equivalents of all four shapes produce exactly the same output as before, with the delta confined to the two cells this change is meant to move. * fix(#3707): close the moved kill switch, and keep the archive out of the live gate Both reviewers independently found that closing the kill switch on one surface left it standing on the other. cmdAuditUat dropped the terminal-status guard, but buildUatRows in planning-inspect kept it — and its comment justified that by claiming to mirror a guard cmdAuditUat no longer had. One byte-identical file with `status: complete` and a fence-straddled `result: blocked` reported parse_gap through audit-uat while planning-inspect published `uat.scope: "complete"` with no diagnostic at all. That is the repo's own generative-fix-divergence class, and no test pinned that arm, which is why it survived. The clause and the false comment are gone and the arm now has tests. Removing it exposed a MAJOR the security pass had not reached: archived phase dirs are deliberately not milestone-filtered and archived UAT files are `complete` by definition, so status-independence newly admitted the entire project archive. One live pending row plus four signed-off milestones produced parse_gap_files 4 — and since progress.md gates Verification Debt on that counter, a mature project would have warned on every run, forever, about closed history no user action can clear. Warning fatigue that buries the next real gap is the feature defeating itself. So the counter is split rather than suppressed: `parse_gap_files` counts live phases only and remains the gate, `archived_parse_gap_files` carries the rest, and every archived entry stays in `results` with its parse_gap and its milestone. Nothing became silent; the live signal stayed actionable. Both workflows report the archived bucket as closed history rather than as something to act on. The scope cascade is also decoupled, on the security reviewer's advice that it is load-bearing here rather than a follow-up: `uat.scope` still reports TRUNCATED honestly so no completeness is claimed over an unread row, while the accepted fence-shortfall over-report no longer flips the aggregate fold that withholds a phase's percentage. A genuinely unreadable file degrades as before. Also corrects a frequency claim of mine: "no phase UAT file in-tree triggers this" was true over a sample of zero, since the only UAT file in the tree is the shipped template. The comment now says the shape is uncommon, which is what I can actually support. * fix(#3707): state that the live/archived split does not extend to outstanding_debt Review MINOR: the split's rationale read as though it governed every counter, but `summary.total_items` was never split — so a single archived `result: pending` row re-trips the same Verification Debt warning the split exists to stop. The asymmetry is deliberate: an archived parse gap is a row nobody can read, so the warning can never be cleared, whereas an archived pending row is legible work someone can still pay down by retesting. Debt that can be settled stays counted. The prose now says so at the point of the claim, instead of leaving the next reader to file it as a miss. Also rewrites the changeset, which described only the secondary fixes and omitted all three defects the issue actually reports: `result: issue` dropped, any wrapped or block-scalar `expected:` never matched at all, and the phase vanishing outright. * fix(#3707): count every parse gap, dropping the live/archived split The split had two regressions, both reproduced through the real CLI, and its premise was false. uat.cts carries #2766's rationale ~190 lines above the code I added: 'Outstanding UAT items do not stop mattering when a milestone closes: a deferred human-UAT scenario or a skipped live-stack test is exactly what gets archived still-open.' So 'archived UAT files are complete by definition' was never true, and the split rested on it. Regression 1: archived-ness was inferred from path shape alone. A phase in the CURRENT milestone, status in_progress, filed under .planning/milestones/v1.1-phases/ was classified archived and demoted out of the gate — live work reported as closed history that needs no action. Regression 2: the split was one-sided. total_items has no archived split, so an archived outstanding row that PARSES gates Verification Debt while the identical row that fails to parse was informational. The parse failure was what buried the debt — the exact bug class this issue exists to fix, re-created one surface over. parse_gap_files counts every parse_gap entry again, archived or not, so it agrees with total_items on what archived means. The pre-existing archived_milestone field and archived-phase scanning are untouched. Regression tests added for both cases. * fix(#3707): correct the changeset clause left behind by the split revert The changeset was rewritten before the split was removed, so its final clause still claimed archived parse gaps are counted separately because signed-off history is not work anyone can act on. There is one counter now, and that premise is the one uat.cts refutes and the revert was made over. This text lands in CHANGELOG.md verbatim, so it would have shipped a description of behavior the code does not have. * chore(#3707): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-26 09:16:22 -04:00
Tom Boucher	86fa2917d7	enh(#3866): dispatch step and contribution hooks at verify:pre (#3869) ... * test(#3866): pin that verify:pre must dispatch every hook kind verify-work.md's verify_pre_hooks step dispatches only `kind == "gate"`, so getWiredKinds reports verify:pre -> {gate} and gen-capability-registry rejects any capability declaring a step or contribution there. The verify lane is therefore closed to capabilities that want to contribute to what UAT covers rather than refuse to let it start. Failing-first: the step, contribution, and exact-kind-set rows are RED; the pre-existing gate row is a green regression pin so the new arms cannot orphan the arm verify:pre already had. Refs #3866 * feat(#3866): dispatch step and contribution hooks at verify:pre verify_pre_hooks dispatched `kind == "gate"` only, so getWiredKinds reported verify:pre -> {gate} and gen-capability-registry's validateHooksWired rejected any capability declaring a step or contribution there. A capability could refuse to let UAT start; it could not contribute to what UAT covers. Add contribution and step arms mirroring execute:wave:post, deferring to references/loop-hook-dispatch.md and carrying its ref.command in-context validation guard ahead of any shell-use prose. A verify:pre step is advisory: it never blocks the start of UAT and an erroring step is routed by its own onError. The gate arm and its check guard are untouched. Give extract_tests an additive consumption seam for the artefacts those steps declare via the existing steps[].produces field -- no new registry field, no new ordering, no invented filename. Manifest-supplied artefact names are validated in-context against an allowlist and resolved only inside PHASE_DIR. With no producing step the derivation is unchanged, pinned by test rather than asserted in prose. Review findings folded in: the artefact-name allowlist (isolated adversarial pass), the artefact-shape contract and the seam-inertness tests (spec axis), and the reference/how-to split so one constraint has one source of truth (standards axis). Closes #3866 * chore(#3866): backfill changeset PR number --------- Co-authored-by: sim <sim@local>	2026-08-25 18:07:29 -04:00
Tom Boucher	fb2d122d7f	feat(#3841): assert gsd-tools identity on every state-mutating verb (#3848) ... * feat(#3841): assert gsd-tools identity before any state-mutating verb only this package publishes. The path-based branches — a project-local install, a runtime config directory — had no such guarantee; they trusted their configured location. This closes them. Mechanism: once resolution finishes, and before any verb runs, the preamble probes the tool it picked with `runtime-identity --raw` and matches the answer with a shell `case` pattern ANCHORED to the start of the compact payload (`{"packageName":"@opengsd/gsd-core"`). An unanchored substring match accepts the decoy `{"packageName":"get-shit-done-cc","note":"@opengsd/gsd-core"}`, which any colliding package could publish. The outcome is exported as the two-valued `GSD_IDENTITY_STATUS` (`ok`/`unverified`), so the gate is asserted on a VALUE rather than on warning prose. Rollout is warn-then-fail per the #3146 ruling: `unverified` prints one line naming BOTH causes and continues, because `no_identity_verb` cannot tell a foreign package from an `@opengsd/gsd-core` older than the verb, and at rollout the old-version case is the common one. The blocker was byte budget, not design. The preamble is inlined into 112 shipped files and several sat within single-digit bytes of frozen ceilings (`gsd-verifier.md` 16 bytes, `gsd-executor.md` 33, `execute-phase.md` 234); a first attempt broke five of them. What made room was collapsing the resolver's twenty near-identical `elif [ -f … ]` arms into one candidate-list helper (`_gsd_at`), which buys far more than the assertion costs. The preamble is now 2,624 bytes against 4,500 — a net 1,876 bytes SMALLER per inlined file, so every capped file moved away from its ceiling rather than toward it. No cap raised, no size-budget exception added, no override token emitted. Resolution order, every runtime-home probe, the `unset -f gsd_run` re-source fix, the fail-closed `exit 1`, and the `CLAUDE_ENV_FILE` persistence are all preserved byte-for-byte in substring terms; the snippet still begins with `_GSD_SHIM_NAME=` and still ends with `fi`, which the parity extractors anchor on. `gsd-core/references/gsd-run-resolver.md` is re-synced byte-equal. Also fixes two stale claims found in passing: CONTEXT.md and FEATURES.md both described an `[ -x ]` guard as the load-bearing re-source defense. That guard was tried and REMOVED in #3831 — it rejected the bare function name, fell through every branch, and hit `exit 1`, which kills a sourced caller's shell. `unset -f gsd_run` is the actual mechanism. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3841): pair the anchor's brace by requiring a closed identity payload The matrix went red on `tests/new-project-mvp-prompt.test.cjs` — "new-project.md has unbalanced braces: net depth 2" — plus a knock-on report from its parent `bug #1516` describe, which is the same failure counted once at the child and once at the block. Root cause: that guard (:182-189, mirroring #3784 bd53925f) walks characters and increments on `{`, decrements on `}`, with no awareness of shell quoting. It scans `new-project.md` PLUS every `new-project/steps/*.md`, and both `new-project.md` and `steps/auto-mode-config.md` carry one inlined preamble copy — hence net 2 from a snippet that was off by exactly one. The unpaired brace was the `{` inside the single-quoted `case` pattern of the identity anchor, which is correct shell and invisible to a text scanner. Fix in the snippet, not the guard. The pattern now anchors at BOTH ends: `'{"packageName":"@opengsd/gsd-core"'*'}'`. That balances 51/51 with a brace that does real work rather than a cosmetic pair — a truncated payload whose prefix matches now fails too, where before it verified. Safe for any future additive field: a JSON object's own closing brace is always the last character, whatever type the last value has, which is pinned by two negative-space tests (a nested object and an array-valued last key must both still verify). Cost: +3 bytes, against the 1,873 the resolver fold already gave back. The alternative considered and rejected was dropping the literal `{` for a `?` glob. It balances too, but weakens the anchor from "must be an opening brace" to "must be any one character", and the anchor is the entire point. Two guards added so this cannot recur silently: - runtime-launcher-parity (F0) pins brace balance at the SNIPPET, so the next edit to that pattern fails on the file it broke instead of surfacing three files downstream in a test whose name mentions neither the launcher nor this issue. It also asserts depth never goes negative, since a `}` preceding its `{` nets to zero while being unbalanced at every prefix. - runtime-identity gains behavioral truncated-payload and trailing-garbage fixtures, so the added `}` is proven load-bearing rather than merely present. Verified: snippet 51/51 braces; new-project combined net depth 0; the seven other preamble-bearing files with nonzero depth are unchanged from merged next (their own prose, not the preamble, and not in any guard's scan set); all 112 inlined copies and the resolver reference re-synced byte-equal; sync:launcher idempotent on the second run. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3841): backfill changeset PR number 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-25 01:05:53 -04:00
Tom Boucher	63abcface9	feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools (#3831) ... * feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools The predecessor package get-shit-done-cc publishes a colliding gsd-tools bin whose phases.clear DELETES where this package's ARCHIVES, and both print success-shaped output against a gitignored .planning/ -- which is how #3129 cost a user 43 phase directories with no error and nothing recoverable from git. The launcher's PATH branch now resolves gsd_run, published only by this package and self-locating via its own symlink chain to the sibling shim, instead of the colliding gsd-tools. A foreign handler becomes unreachable from PATH, and when no gsd_run is reachable the resolver fails closed rather than falling back -- that fallback was the vulnerability. This is smaller than the branch it replaces, which matters: the preamble is inlined into 113 shipped files and agents/gsd-verifier.md sits 2 bytes under a red-line size cap. unset -f gsd_run leads the preamble so a re-source is idempotent. Without it, command -v finds the shell function, returns a bare name, and the resolver falls through to an exit 1 that kills a sourced caller's shell. Adds gsd-tools runtime-identity, a manual diagnostic reporting this runtime's package coordinates over the baked package-identity (#498) and readHostVersion, with a strict total classifier: only a JSON object with an exact packageName verifies, since JSON.parse admits 0/"str"/[]/null/true. An inlined identity assertion was built and reviewed first, then withdrawn -- it breaks five frozen size ceilings and no assertion fits in 2 bytes. Closes #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3146): stop sync:launcher relocating a deliberate preamble placement Pre-existing defect, surfaced by this PR because sync is a no-op unless the snippet content actually changes. transformFile inserts the preamble into the first block that CALLS gsd_run, but gsd-core/workflows/explore.md deliberately places it in a bootstrap-only block that DEFINES gsd_run without calling it -- its own comment explains why: declining the research offer must not leave Step 5's commit call unbootstrapped. Stripping empties that block of calls, so the preamble migrated forward and broke the define-before-use invariant tests/explore-command.test.cjs pins. Reproduced on a pristine origin/next checkout with the base snippet and base file, so this was not introduced here. The insertion target now honours a block that already carried the preamble, falling back to the first calling block for files that have none yet. Adds a behavioral regression test over a two-block fixture. Also updates three runtime-launcher-parity tests that pinned the removed PATH fallback to gsd-tools. Their intent is preserved -- the PATH stub is renamed gsd_run so it is reachable by the new resolver, and the RUNTIME_DIR-wins test still asserts the stub is never invoked. Fixture shebangs move to an absolute /bin/sh, because the fixture PATH is deliberately restricted and #!/usr/bin/env sh could not resolve. Refs #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3146): backfill changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3146): document the FEATURES.md section-numbering practice The monotonically increasing section number in docs/FEATURES.md is the most frequent merge-conflict source in this repo, and it has TWO conflict cells, not one: the ### N. heading and the hand-maintained table of contents. Two PRs adding differently numbered features still collide on the TOC, so renumbering alone does not make a branch safe. This branch alone was renumbered 165 -> 166 -> 167 -> 168 across successive rebases. Adds a CONTRIBUTING section stating the practice: allocate the number last, never pre-emptively renumber, take max+1 after a rebase and update the TOC in the same commit, and never renumber someone else's section. Fork contributors are told explicitly they may leave the number to a maintainer at merge rather than chasing the counter. Agents are told to lease the allocation and to include the file in their published touched set. Records the durable fix as planned rather than pretending it exists: FEATURES.md should be generated from per-feature fragments the way CHANGELOG.md is generated from .changeset/, and the way tests/emitted-drift-acks/ works (#2914). Also renumbers this branch's own section to 168, leaving 167 to the PR already in flight. Refs #3146 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-24 20:57:16 -04:00
Tom Boucher	c933184b97	enhance(#3172): require a stated failing direction for every automated acceptance command (#3825) ... * test(#3172): failing-first suite for the stated failing-direction probe Pins the <fails_when> pairing walk, placeholder denylist, MISSING sentinel exemption, degraded-read contract, CLI arm and the plan-authoring contract text. RED by construction: the module exports it requires do not exist yet. Executed on the remote runner. * feat(#3172): require a stated failing direction for every automated acceptance command Every runnable <automated> command now carries a <fails_when> sibling naming what output constitutes failure. A command with no expressible failure mode is not an acceptance test: it reads as rigour and is not falsifiable. - verify-command-grounding gains a failing-direction probe sharing the existing <automated> grammar, MISSING sentinel and walk guard rather than copying them - gsd-tools check verify-failure-directions <N> backs it; plan-phase dispatches it and hands the JSON to gsd-plan-checker check 8f - Dimension 8 detail extracted to references to stay under the agent size cap Verified on the remote runner. * fix(#3172): close four review findings in the failing-direction probe - MISSING_SENTINEL_RE matched an env-var assignment prefix (MISSING=1 cmd), so a real command was exempted from the new blocking gate. Tightened the SHARED constant rather than adding a second copy. - Both token regexes scanned to EOF on unclosed openers (O(n^2), 1562ms at 40k). Bodies are now non-crossing; 1ms, byte-identical on well-formed input. The pre-existing AUTOMATED_BLOCK_RE carried the same defect and is fixed here too. - probePhaseFailingDirections reported status 'ok' when one plan was unreadable, conflating 'could not look' with 'nothing to report'. - Extracted the phase-resolution block both check arms had copied verbatim. Also corrects a docs/AGENTS.md dimension list stale since #2401. Verified on the remote runner. * fix(#3172): project the planner rule onto the spawn contract, settle emitted bookkeeping The remote runner refuted the planner-side edit. agents/gsd-planner.md is frozen under a 49152-LF-char cap asserted by four suites and sat at 49,146 — six chars of headroom — so the +537 of authoring rule blew it. #3297/#3645 already settled where such a rule goes: the planner spawn contract in plan-phase.md, beside <tracked_source_paths>. The agent file is reverted to origin/next verbatim. - plan-phase.md gains <failing_direction_contract>; tests row 30 now asserts the contract there and row 30b guards the freeze in both directions - plan-phase.md growth acknowledged by APPENDING to the 3409 fragment, per the precedent that two ack sources may never name the same path - install-tree fixtures regenerated for the three new reference files Verified on the remote runner. * chore(#3172): backfill PR number into the changeset fragment pr:0 -> pr:3825 now that the PR exists. --------- Co-authored-by: sim <sim@local>	2026-08-24 19:05:11 -04:00
Tom Boucher	a2387a0545	feat(#3034): add opt-in parallel reviewer lanes (#3822) ... * test(#3034): failing-first coverage for opt-in parallel reviewer lanes Executes the real invoke_reviewers dispatch block from review.md against a stubbed gsd_run seam rather than pattern-matching the workflow text, so the two properties that actually carry risk are observable: that every lane is joined before aggregation, and that concurrent lanes cannot tear a line in gsd-review-lane-results.jsonl. Concurrency is proven by a barrier fixture, not by elapsed time -- each stub lane blocks until all lanes have checked in, which can only complete if they overlap. Red against the current sequential dispatch, by design. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3034): add opt-in parallel reviewer lanes Reviewer lanes within one review pass inspect the same immutable plan snapshot and have no dependency on one another, but were dispatched strictly one at a time, so a multi-reviewer pass cost roughly the sum of its lanes. The serialization is a deliberate protection against provider rate limits, so it stays the default; review.parallel_lanes opts a project out of it. The loop body is hoisted into run_review_lane so the sequential and concurrent paths share one body -- two hand-synced dispatch bodies is the divergence class ADR-2782 spent a phase deleting. Each lane writes a slug-scoped result file, concatenated in selection order after the join: concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter, so a torn line is a broken REVIEWS.md rather than a cosmetic log defect. Aggregating in selection order also keeps the artifact byte-identical between the two paths. The guard is strict equality on "true" and falls back to sequential when config-get fails -- the opposite polarity from the commit_docs guard, because failing open here fires the very requests the default prevents. Also corrects docs/COMMANDS.md and its four locale mirrors, which described --all as running every configured reviewer in parallel when dispatch was in fact sequential. Closes #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3034): de-duplicate dispatch slugs and scope lane locals Review finding (Standards axis): a slug repeated in SELECTED_REVIEWERS would put two concurrent background jobs on the same > -truncated per-lane result file. The shared-append form this replaced could not corrupt itself that way, so de-duplicating is what keeps the concurrent path no worse than the sequential one. Selection de-dupes today -- the roster is a Set and review.default_reviewers normalizes lowercase-unique -- but reachability analysis is not a contract, which is the same reason the roster derivation itself is guarded. Splitting once into DISPATCH_SLUGS also removes the duplicated tr-split the same review flagged: the dispatch and aggregation loops now share one list, which is what guarantees they walk the same slugs in the same order. A plain string accumulator rather than an array, because zsh and bash disagree on array indexing and this block runs under both. Also scopes run_review_lane's locals. Not a live fix -- each dispatched call already forks its own subshell -- but it makes the isolation a property of the function rather than of the dispatch mechanism happening to fork. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3034): acknowledge review.md growth, drop spent 2295 ack The differential attribution gate reported review.md growing 4173 bytes (30712 -> 34885) with no live acknowledgment. Adds the per-PR fragment it asks for, naming only the one path it reported. Deleting tests/emitted-drift-acks/2295-resolved-model.json is required, not opportunistic. That fragment declared review.md and nothing else, and its ripple is already absorbed into the base, so it is spent -- it can no longer clear anything, which is why the gate still reported review.md as unacknowledged. It could not simply be left alone either: two ack sources may never name the same path, so it blocked this PR's fragment outright. CONTRIBUTING is explicit that a fragment whose last entry is removed gets deleted with it, because an empty fragment signals nothing while its presence reads as a live alarm. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3034): backfill changeset PR number Refs #3034 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-24 15:25:22 -04:00
Tom Boucher	8442d984b9	fix(#3809): route runtime-loaded markdown through the gsd_run launcher (#3815) ... * test(#3809): generalize dead-ref guard into a rule table (failing first) The #2020 guard hardcoded `sdk/(src|dist|handlers)/` — the three dead paths that had caused that storm. That proved those three paths were gone and said nothing about the class, so #3809 reproduced the identical Windows find.exe storm under a different token and the guard could not see it. Replaces the single regex with a rule table over the same runtime-loaded markdown surface, adds `commands/` to the scan set (previously uncovered), and adds rule B: the runtime shim filename must never appear in command position, because it is not a PATH command and an agent that meets it falls back to locating the file. Rule B's matcher is deliberately lenient — the launcher's own resolver assignment, `node <path>/<shim>` calls, bare paths, and prose that names the file all stay unflagged, each pinned by a negative-space row. This commit is expected to FAIL: 50 offenders across 23 files remain in the tree. The remediation lands next. Refs #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3809): route every workflow call through the gsd_run launcher 50 places across 23 runtime-loaded workflow, agent, reference, and command files instructed the agent to run the runtime shim by filename. That filename is not on PATH under any name -- package.json ships gsd-core, gsd-tools, gsd_run and gsd-mcp-server -- so the call exited 127, the file-shaped token sent the agent looking for the file, and on Git Bash for Windows the resulting `find /` walked the entire drive (7268 CPU-seconds in the report) until somebody killed it by hand. CONTEXT.md -> Runtime Launcher Module already makes gsd_run the single entry point: "Canonical space-safe shell preamble (`gsd_run`) used by every workflow bash block to invoke the GSD runtime CLI." These sites predate that rule -- they trace to 0e6907050 (docs(#195): migrate workflow markdown off gsd-sdk query), which swapped one non-PATH token for another. Two further instances of the same class surfaced during remediation and are fixed here rather than left for later: - references/model-profiles.md prescribed `node <shim> effort sync` with no path at all; node resolves a bare filename against cwd, so it fails the same way. - references/universal-anti-patterns.md rule 25 instructed every agent to "use <shim>" when shelling out. That rule did not contain the defect, it prescribed it repo-wide. Five "(or legacy <shim>)" parentheticals left dangling by the substitution are removed; after the rewrite they offered the non-resolving form as an alternative. The guard from the previous commit now passes. Its node-prefix exemption was tightened to require a path separator, which is what exposed model-profiles. Fixes #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3809): key the guard on the CLI's whole verb roster, not observed usage Review found the first cut of rule B repeating the very mistake it exists to prevent. Its verb set held query, commit and effort -- the verbs that happened to appear in the tree -- so it could not see `<shim> phase add`, `<shim> state load`, `<shim> verify ...` or twenty-odd other real single-word subcommands. A guard that only recognises yesterday's offenders is not a guard. The set is now the CLI's full advertised roster, unioned from the usage banner and HOST_COMMAND_ROUTERS (which carries verification, planning, uat, stats, todo and windows, all absent from the banner). Widening it immediately caught a live offender the first pass had missed: references/planning-config.md prescribed `node <shim> worktree set-baseref` with no path. Fixed here. Also drops the "a hyphen or a dot means subcommand" heuristic, which was unsound for prose -- it flagged `built-in` and `v1.2`. Detection now keys entirely on the roster, testing the first dot-segment so that phase.add and state.patch still match while prose does not. Both false positives are pinned as negative-space rows. Guard verified against the pre-fix tree at origin/next: 52 offenders across 25 files, and 0 after this branch's remediation. Refs #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3809): derive the verb roster from the router; repair launcher parity Standards review caught the guard repeating the defect it exists to prevent. Its verb list was a hand-copied literal -- and worse, transcribed from an INSTALLED older binary, so it was missing 22 verbs this tree actually ships (websearch, windows, state-snapshot, context-predicates and the dispatch-* family among them). gsd-tools.cjs already carries three hand-maintained rosters whose drift is a named defect pinned by the parity test in tests/commands.test.cjs; a hand-copied fourth was that same defect wearing a guard's clothes. The roster is now derived from HOST_COMMAND_ROUTERS + TOP_LEVEL_USAGE, lazily and memoised, with `query` supplemented explicitly -- it dispatches through the routing hub ahead of the host-router table, so it appears in neither export, yet 45 of the 50 offenders used it. A parity test pins the derivation. Two regressions this branch introduced, both caught by the remote runner: - runtime-launcher-parity: rewriting a comment in gsd-research-synthesizer.md put a `gsd_run` token at line 65 while the canonical preamble sits at 158, breaking "exactly ONE preamble, before the first gsd_run call". The comment is descriptive and needs no command token at all; it now names none. - The #2751 guard's PROSE_ALLOWLIST entry for that same line went stale once the line stopped carrying a bare mention. Pruned, exactly as that guard's own stale-entry test instructs. Also corrects git-planning-commit.md, where the first pass rewrote only the trailing "legacy" clause and left the sentence reading backwards. Note the #2751 guard and this one are complementary, not duplicates: its regex requires whitespace immediately after `gsd-tools`, so it cannot match the `.cjs` form, and this one only matches the `.cjs` form. Refs #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2751): extend the bare-command guard to references/ and commands/ The #2751 guard has only ever scanned agents/ and gsd-core/workflows/. Two runtime-loaded directories were never in its scan set, and 47 bare `gsd-tools <verb>` calls had accumulated there unseen -- the same defect that guard exists to catch, in the rooms it never entered. - gsd-core/references/: 37 calls, all rewritten to gsd_run. references are fragments inlined into a parent that defines the launcher, which is why 21 of the 22 files already using gsd_run carry no local preamble. - commands/gsd/: 10 operative calls rewritten. The remaining 10 are descriptive prose ("resolved inside the workflow via ...") and are allowlisted with reasons, bringing PROSE_ALLOWLIST to 15. commands/ also came under launcher propagation. sync-runtime-launcher.cjs walked only WORKFLOWS_DIR and AGENTS_DIR, so every preamble under commands/ was a hand-pasted copy nothing propagated and no test checked -- graphify.md had accumulated five. It now walks COMMANDS_DIR too, which collapses those five to the canonical one-per-file, and runtime-launcher-parity gains a (B-commands) arm mirroring (B-agents) exactly so the placement stays honest. The parity arm keys on shell blocks, so commands/gsd/workstreams.md and config.md -- which name gsd_run only in inline backtick prose -- are exempt, as they should be. gsd_run is itself a shipped npm bin, so those inline instructions resolve from PATH exactly as the gsd-tools form they replace did. skills/ is deliberately NOT added to either guard's scan set: it is generated from commands/ and pinned by lint:generated-sync, so guarding the source guards both, and scanning the mirror would double-report every future offender. Regenerated here. Refs #2751, #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3809): acknowledge the one emitted file this change grows The emitted-attribution gate failed on the previous sha: gsd-research-synthesizer.md grew 3 bytes (13847 -> 13850) with no acknowledgment. The substitution SHRANK the other 19 emitted files, which is why the growth arm was not expected to fire at all. The 3 bytes are unavoidable. Line 65 is a descriptive comment inside a fenced block; naming any command there puts a gsd_run token ahead of the file's canonical preamble at line 158, which runtime-launcher-parity's (B-agents) arm correctly rejects. So the comment names no command and says where the config is actually loaded instead, which reads longer than the token it replaced. Acks only the path the gate reported, per the fragment rules. Refs #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * revert(#2751): drop the commands/ half — three contracts pin it in place The remote runner refuted the commands/ extension outright. Reverting it and keeping the references/ conversion, which passed. What broke, all of it caused by bringing commands/ under launcher propagation: - graphify.md's five per-block preambles are LOAD-BEARING, not accumulated drift. tests/graphify-visualization.test.cjs extracts individual Step-3 shell chains and executes them standalone, so each fenced block needs its own definition of gsd_run. Collapsing them to the canonical one-per-file produced `bash: gsd_run: command not found`, exit 127, across four tests. The "define once per file" contract holds for workflows and agents because nothing extracts their blocks in isolation; commands/ is not like that. - explore.md broke "the preamble that DEFINES gsd_run must appear before the first USE of gsd_run anywhere in the file". - tests/gsd-tools-path-refs.test.cjs (#1766) ASSERTS that commands/gsd/workstreams.md contains the literal string `gsd-tools query workstream.list`. Rewriting it to gsd_run contradicts a test that pins the opposite, so the two guards disagree about that file by construction. So commands/ is not a scan-set widening. It needs those contracts reconciled first, and that is its own change. SCAN_DIRS keeps gsd-core/references/ and drops commands/, the ten commands/ allowlist entries go with it (back to 5), and the reasoning is recorded in the guard itself so the next person does not rediscover it by burning a matrix run. commands/gsd/import.md keeps its #3809 fix — that one is the .cjs form this PR exists to remove, and it is untouched by any of the above. Refs #2751, #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * revert(#3809): restore explore.md's Step 1 preamble placement Running the launcher sync script processed workflows/ and agents/ too, not just the commands/ directory the run was aimed at, and it MOVED gsd-core/workflows/explore.md's preamble from Step 1 down to Step 3. The script inserts into the first bash block that USES gsd_run. explore.md's Step 1 block only DEFINES it, and that placement is deliberate -- the file says so on the line above: "Placed in Step 1 rather than Step 3 so declining the research offer cannot leave Step 5's commit call unbootstrapped." tests/explore-command.test.cjs pins it. explore.md carried no #3809 offender, so reverting it costs this fix nothing. This was collateral from invoking the sync script at all, not from the COMMANDS_DIR change, which is why the earlier commands/ revert did not catch it. Refs #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3809): backfill PR number into changeset fragments pr:0 -> pr:3815 for both fragments now that the PR exists. Refs #3809 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3809): drop the hand-rolled regex escaper CodeQL flagged CodeQL raised js/incomplete-sanitization (HIGH) on the guard's pattern build: `SHIM.replace(/\./g, '\\.')` escapes the dot and nothing else, so it does not escape backslashes. It blocked PR #3815. The repo already bans this shape -- local/no-adhoc-regex-escape exists exactly to stop hand-rolled escapers, with the canonical one in src/pattern.cts. Rather than reach for that helper, the pattern now carries no escaping logic at all: SHIM is a compile-time constant whose only metacharacter is the dot, so the regex source is spelled out literally. The generated source string is byte-identical to what the replace() produced, verified before and after -- 0 offenders on this tree, 52 against origin/next, unchanged. A drift pin asserts SHIM_PATTERN still matches SHIM exactly, and that the dot is escaped rather than acting as a wildcard, so the two cannot separate. Refs #3809 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-24 11:47:37 -04:00
Tom Boucher	8ed105c8a4	fix(#3684): resume verified-unmarked phases at update_roadmap (#3814) ... * test(#3684): failing-first rows for the verified-unmarked resume * fix(#3684): resume verified-unmarked phases at update_roadmap * test(#3684): heading-shaped roadmap fixture, plain phase.complete calls * fix(#3684): fit under the pre-phase-6 margin, fix pins and verify call * fix(#3684): padding-normalize the marked-complete join, assert STATE idempotency * test(#3684): anchor fixes, node jq mirror, characterized STATE delta * chore(#3684): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-24 10:58:04 -04:00
Tom Boucher	4b84be1da4	fix(#3683): wire gated learnings extraction into completion, align copy path (#3810) ... * test(#3683): failing-first rows for learnings source resolution and wiring pins * fix(#3683): wire gated learnings extraction into completion, align copy path * test(#3683): register the learnings suite in the docs-guard lane, drop unverified markers * fix(#3683): close review findings — per-item parsing, readdir guards, docs paths * fix(#3683): route phase enumeration through the locator seam, fix assertion targets * fix(#3683): merge execute-phase ack into the 3003 fragment, fix fidelity targets * fix(#3663): replace the spent execute-phase ack entry with the 3683 re-arm * chore(#3683): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-24 09:23:01 -04:00
Tom Boucher	31fcb833ec	fix(#3679): gate pr-branch verify on planning-tree deletions (#3803) ... * test(#3679): failing-first rows pinning planning preservation and the verify deletion gate * fix(#3679): gate pr-branch verify on planning-tree deletions * test(#3679): extract hashes via rev-parse and de-vacuate the pure-code pin * fix(#3679): close review findings — merged ack, pinned prose gate * fix(#3679): close two-axis review findings — no-renames gate, structural pin * chore(#3679): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-24 03:54:02 -04:00