7c5234428420350796e5356dccf628013e5ca367
488 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
2131fe13f3 |
enhance(#3464): exec() detection widening, citation-debt cleanup — Phase 8 (#4171)
* feat(#3464): widen no-source-grep to detect regex.exec() on tracked text Adds an execCall kind alongside the existing regexTest detection -- regex.exec(tracked) was invisible to the rule while regex.test(tracked) was already caught, despite both reading a source-derived string through a regex. Measured: 4 previously-invisible sites across 2 files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#3464): migrate 4 sites newly flagged by the exec() widening docs-hooks-table-parity.test.cjs's three regex-extraction loops are site-scoped marked (source-text-is-the-product) -- the dynamic preToolEvent/postToolEvent dialect branching they mirror is explicitly documented as not statically parseable, so a literal-pattern mirror is the practical minimum-cost check. no-bare-gsd-tools-command-position.test.cjs's readRouterVerbs() now requires HOST_COMMAND_ROUTERS directly instead of regex-walking gsd-tools.cjs's source text -- the same accessor three other suites already use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3464): pay down 6 grandfathered uncited allow-test-rule markers Two were genuinely load-bearing (suppressing a real detected violation) and just needed a citation added -- phase6-capstone-conformance.test.cjs, runtime-name-policy.test.cjs, both now (#3464). Four were dead-weight file-header markers suppressing nothing -- each file's real effective sites are covered by separate, already-cited markers elsewhere in the same file. Deleted outright rather than cited, per Phase 1's own precedent (remove non-load-bearing markers instead of grandfathering them forever) -- codex-config.test.cjs (two copies), gsd-check-update-worker-platform-gate.test.cjs, orphaned-hooks.test.cjs, settings-jsonc.test.cjs. allowlist.json: 134 -> 128 entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3464): re-baseline effective-exemption ceiling to 84 The exec() widening's 3 newly-marked sites are now suppressed and counted; ceiling rises 81 -> 84, the exact measured high-water mark. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3464): correct citation and restore a wrongly-deleted marker Two review corrections, both found by the orthogonal review pass: - docs-hooks-table-parity.test.cjs's 3 new exec() markers cited #3464 (mechanically "the phase that widened the rule") when the file's own established, correct reference is #3839 (the issue this whole test exists to enforce, already cited in its file header) -- fixed to match. - gsd-check-update-worker-platform-gate.test.cjs's deleted file-header marker was NOT dead weight: its codeOnly() helper wraps readFileSync and is called inline as an assert argument, a genuine source-grep pattern on real .cjs/.js source that the rule cannot currently see (helper-function indirection is a distinct blind spot from anything Phase 7/8 measured) -- CONTRIBUTING.md is explicit that "unverified" is not the same as "vestigial." Restored, site-scoped this time (directly above codeOnly(), not as an inert file-header comment) and cited (#3103, the issue the file's own docstring already references). codex-config.test.cjs's two deletions and orphaned-hooks.test.cjs's / settings-jsonc.test.cjs's deletions were independently re-verified and stand: their flagged lines read generated .toml/.json OUTPUT, not source, or have no residual pattern at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Carlos Cativo
|
9b77320580 |
fix(#4076): add missing gsd-hook-version header to gsd-node-runner.sh (#4092)
* fix(#4076): add missing gsd-hook-version header to gsd-node-runner.sh gsd-node-runner.sh was registered in MANAGED_HOOKS but shipped without a gsd-hook-version header, so gsd-check-update-worker.js always classified it as 'definitely stale' (a missing header is indistinguishable from a pre-version-tracking file). Every install on an otherwise up-to-date version showed a permanent, unclearable '⚠ stale hooks — run /gsd-update' warning naming this one file. Root cause: the build-hooks.js comment claimed the file is 'not a registered hook' and 'staged verbatim — no templating', but it IS in MANAGED_HOOKS (managed-hooks-registry.cjs:34) and install.js already stamps {{GSD_VERSION}} into every .sh hook unconditionally, gsd-node-runner.sh included. The comment contradicted both the registry and the installer's actual behavior, and the header line itself was simply never added. Fix: add the header (matching every other managed .sh hook's format) and correct the comment so it no longer asserts the opposite of what the registry and installer actually do. Adds a regression test that iterates every MANAGED_HOOKS entry and asserts it carries a header matching the worker's own detection regex, so a future hook added to the registry without one fails CI instead of shipping silently. Fixes #4076 * chore(#4076): add changeset fragment for PR #4092 * fix(#4076): address review nits — drop unneeded exemption, fix blank line Per @trek-e's review on #4092: - tests/managed-hooks.test.cjs:96: the readFileSync call uses a loop variable (entry-derived hookPath), not a literal path, so local/no-source-grep's static literal-path detector never flags it — the allow-test-rule exemption comment was unnecessary. Replaced with a plain note explaining the source-read rationale. - tests/managed-hooks.test.cjs:121-122: dropped a stray extra blank line before the bug #2136 section divider. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
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> |
||
|
Tom Boucher
|
f16ff7d1b3 |
enhance(#3545): widen no-source-grep with fold+hooks, migrate 76 sites (#4161)
* feat(#3545): widen no-source-grep with one-hop path-fold and hooks dir Resolve a readFileSync() path argument that is a bare Identifier one hop back to its VariableDeclarator initializer before classification, and recognize `hooks` as a source directory alongside bin/lib/gsd-core/src. Measured (epic #3464 phase 7): fold+hooks together newly flag 76 unsuppressed sites across 18 files that were previously invisible to identifier-indirected or hooks/-rooted source reads. Neither widening alone is sufficient — hooks-only surfaces 0 new sites, confirming #3520's prior finding that the identifier-indirection gap must close first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#3545): migrate 76 sites newly flagged by the fold+hooks widening Per-site classification: rewrite behaviorally (require() the real module, assert on its actual exported behavior) wherever the read was a proxy for code behavior; add a site-scoped `// allow-test-rule: <reason> (#3545)` marker only where the raw source text genuinely is the product under test (codex-config.test.cjs's adapter-header-contract checks, install.js structural-wiring guards with no exported symbol, AST-parse fixture inputs, etc.) — each marker cites an existing repo-sanctioned category from CONTRIBUTING.md's allow-test-rule exception table. Also converts two try/finally test bodies (introduced during this same migration) to the required t.after() cleanup pattern per CONTRIBUTING.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3545): re-baseline effective-exemption ceiling to 81 The fold+hooks widening's own newly-detected sites are now suppressed by site-scoped markers, moving them from invisible into the tightly-ratcheted effective-exemption count. Ceiling rises from 10 to 81 (the exact measured high-water mark, grace unchanged at 2) — a deliberate, measured re-baseline per the widening working as intended, not an ordinary ceiling bump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3545): use canonical allow-test-rule category tokens 4 markers added during migration cited an issue ref correctly but didn't use one of CONTRIBUTING.md's seven recognized category tokens, unlike every other marker in this change. Cosmetic only — same suppression lines, same effective/live counts (81/81, 0 live). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3545): correct stale phase-artifact path in test comment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
Tom Boucher
|
cad70f4f3e |
fix(#4120): replace shellcheck npm dep with dependency-free downloader (#4121)
* fix(#4120): replace shellcheck npm dep with dependency-free downloader The `shellcheck` devDependency (added in #4109) pulled in decompress@4.2.1 for archive extraction, which carries an unpatched CRITICAL zip-slip vulnerability (GHSA-mp2f-45pm-3cg9, CVSS 9.1) plus two moderate findings. decompress's latest published version IS the vulnerable one -- no patched release exists upstream, so npm audit fix cannot resolve this by upgrading. Removes the shellcheck package entirely and replaces its role with scripts/lib/shellcheck-fetch.cjs: a small downloader using only Node's built-in https/zlib plus a hand-written tar-entry reader, fetching a pinned koalaman/shellcheck release directly from GitHub releases. The reader never uses an archive-supplied name as a filesystem path (the exact defect class decompress had) -- it only returns the matched entry's bytes; the caller writes those bytes to a path it constructs itself. Bounds the download with a 30s-per-hop timeout, consistent with the ShellCheck subprocess's own timeout. Covers linux/darwin on x86_64/aarch64, matching this repo's actual CI (lint-tests runs only on ubuntu-latest) and local dev needs; Windows fails with a clear, honest error rather than silently misbehaving. Adds tests/lint-workflow-shellcheck-fetch.test.cjs covering the tar-parser (unit cases plus a fast-check property test per CLAUDE.md's parser-testing requirement), a security behavioral pin confirming traversal-style entry names are treated as opaque strings never filesystem paths, and boundary coverage for the redirect-following logic's MAX_REDIRECTS limit (limit-1/limit/limit+1, via an injectable transport, no real network I/O). npm audit: 0 vulnerabilities (was 1 critical + 5 moderate). The lint script reproduces the identical result against the current tree: "212 pre-existing finding(s) from baseline, 0 new" -- no behavior regression, no baseline changes needed. * fix(#4120): register shellcheck-fetch.cjs with the installer scripts/lib/shellcheck-fetch.cjs shipped without being added to GSD_SCRIPTS_LIB_FILES in bin/install.js, which would have left it orphaned on uninstall and broken the golden install-tree fixtures for every runtime. Adds the entry and regenerates the 19 affected fixtures via npm run gen:install-tree. * docs(#4120): add changeset for the decompress CVE fix --------- Co-authored-by: sim <sim@local> |
||
|
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> |
||
|
BeeHiggs
|
1a358ce0fd |
feat(#2761): bracket-tolerant read path — roadmap/validate/verify/state recognize bracket ids (epic #612 PR-2) (#2867)
* feat(#2761): gated heading-intro selection + one bracket identity grammar Foundation. Two owner-level changes plus a federated convention resolver; no reader consumes them yet. 1. GATED SELECTION, not an ungated widening. Widening every heading matcher requires the claim "no legacy ROADMAP contains a `[CODE.MM]` bracket followed by a digit", and that is false: `### [RFC.2119] 5:`, `### [v1.0] 2024:`, `### [ADR.612] 3:` and `### [ISO.8601] 2026:` are ordinary headings, and a widened reader claims each as a phase — moving phase_count and total_phases and adding W006 on projects that never opted in. No narrowing rescues it: the premise is about documents we do not control. `phaseHeadingPrefixSrcFor(baseline, convention, capturing?)` selects the pattern SOURCE at construction time. A project whose resolved `phase_id_convention` is not exactly 'bracket' compiles the same source string it compiled before. `baseline` is explicit because whether a site spells the any-bracket prefix or a bare `Phase\s+` is a fact about that site's history: handing the wider grammar to a bare site retro-grants tolerance it never had, in both directions — warnings appear, and a warning that fires today vanishes. Both bracket forms CAPTURE. `[GSD.999] Phase 07:` previously matched through the base alternative, which captures nothing, so a reader saw no bracket, fell back to the legacy token rule, and counted a labeled icebox heading while excluding the label-less one beside it — two derivations of one ROADMAP disagreeing. 2. ONE bracket identity grammar, one width rule. The milestone width is reconciled with the emit validator: pad2 output, so two digits or 3+ with no leading zero. Earlier spellings diverged in both directions — admitting `002`, which the validator rejects, and a bare `0` pad2 never produces — and the section recognizers accepted `[GSD.2]`, which SCOPED a milestone no phase heading could then resolve into, recreating the on-disk-count fallback this epic removes. An unpadded bracket is now uniformly malformed: it scopes nothing, bounds nothing, sections nothing. W005 on its directories is the surfacing signal. The milestone field is boundary-anchored, so a malformed run cannot match by its prefix (`GSD.002-01` read as sentinel `00`). Recognition stays case-insensitive because readers compile `/i`, but identity helpers match `[A-Z]`, so a captured id is folded first — otherwise `### [gsd.999] 07:` failed every sentinel test. The qualified key shares the width, the `(?=-|$)` boundary and the single-sub-phase shape of the directory token, because phaseTokenMatches returns unconditionally on a qualified hit: a key matching a directory isPhaseDirName rejects would be a final wrong answer. 3. resolvePhaseIdConvention federates workstream -> root exactly as config-loader does — including that root is a fallback only when a WORKSTREAM is active, so a project-scoped directory stands alone. loadConfig cannot serve this: it merges against CONFIG_DEFAULTS and drops keys it does not know, and this key is not among them. It governs the bracket-selection reads ONLY. PHASE_HEADING_PREFIX_SRC is left byte-identical: PR-1 shipped it, nothing consumes it, and it is superseded rather than redefined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): roadmap.cts selects its heading grammar from the convention Six matchers build their intro through the gated selector, and cmdRoadmapAnalyze / cmdRoadmapGetPhase / getRoadmapPhaseWithFallback each resolve the convention ONCE per command and thread it down. Three sites take the any-bracket baseline (they already tolerated `[anything] Phase N`); three take label-only (they spelled a bare `Phase\s+`). Handing the wider grammar to a label-only site retro-grants tolerance it never had — and not only by adding matches: on a legacy repo an unchecked `- [ ] **[v1.0] Phase 05: Thing**` bullet would start SUPPRESSING the W006 that fires today. Sentinel handling under bracket ADDS a rule rather than replacing one: a bracketed heading is a sentinel when its bracket milestone is reserved (`### [GSD.999] 01:`) OR when its token is, so the engine-wide 0/999 backlog convention keeps applying to `### [GSD.02] 999:`. Replacing the token rule let a mid-migration ROADMAP — bracket headings plus a legacy backlog block, exactly the content this epic targets — add entries to the progress denominator. The captured id is folded before the identity test, so a lowercase `### [gsd.999] 07:` is excluded too. The DIRECTORY read is threaded too. `cmdRoadmapAnalyze` resolves the convention once and hands it to all four of its heading/checklist patterns, but the single `phaseTokenMatches` call that decides `disk_status`, `plan_count`, `summary_count`, `has_context` and `has_research` was left two-argument — so every canonical `{CODE}.{MM}-{PP}-slug` directory read as `no_directory` with zero counts, on the PR's own headline verb, while the SAME build resolved those same directories correctly in three other places on the same repo (W006/W007 via phaseTokenFromDir, `state json` via the milestone filter, and the W021 milestone-complete read through this very helper's three-argument form). It failed ONLY for the directory shape the convention exists to name: a mid-migration bracket repo carrying legacy `01-one` dirs resolved fine, which is why nothing caught it. Measured, bracket vs its flat-legacy twin: `[["01","no_directory",0,0],["02","no_directory",0,0]]` against `[["01","complete",1,1],["02","planned",1,0]]`. The oracle is the twin, computed in the same test run, plus exact literals — `grep disk_status tests/adr-612-*` was zero hits before this, so neither the fix nor a future regression had any gate at all. Disclosed: a ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted. Silent invisibility during the migration window is the deliberate trade against claiming phases on projects that never opted in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): validate.cts selects its grammar; gated directory recognition The W006/W007 feeders take the resolved convention as a threaded parameter. These sites carry the letter-tolerant `[\w][\w.-]*` capture, which makes them where an ungated widening does the most damage: `### [RFC.2119] 5:` enters roadmapPhases as a phantom and becomes a W007 "in ROADMAP.md but no directory on disk" on a project that never opted in. buildRoadmapPhaseVariants also surfaces the tokens borne ONLY by sentinel-bracket headings. Surfaced rather than filtered in place because roadmapPhases feeds both a membership check and a missing-directory warning, and only the latter should ignore an icebox item. That set is OCCURRENCE-AWARE, and the subtlety is load-bearing: roadmapPhases is a TOKEN set, so `[GSD.999] 01` and `[GSD.02] 01` collapse to one entry. Keying suppression on the token alone let an icebox heading silence a REAL phase that happens to share its number — a false negative strictly worse than the warning it removed. A token is suppressed only when no non-sentinel heading bears it. Directory recognition is added as gated FUNCTIONS beside the exported RegExp constants, which stay byte-identical: the `{CODE}.{MM}-` prefix is string-indistinguishable from the letter-prefixed-decimal family this repo documents as ambiguous, and folding a branch in changes those constants' answers on exactly that family. A RegExp constant has nowhere to attach a gate. The recognizer mirrors the emit grammar and delegates the token to the canonical owner, so recognizer and resolver agree on rejected input as well as accepted. Both functions throw on a non-string, matching the call pattern they replace. buildRoadmapPhaseVariants' CHECKLIST scan is capturing, like its heading twin and like the sibling checklist scan in roadmap.cts, and for the reason that one states: the bracket id has to ride along or the sentinel filter is blind to `- [ ] **[GSD.999] 01: Icebox**`. Left un-capturing, the scan called every checklist token REAL, and the occurrence-aware un-suppression loop then deleted the icebox token the HEADING scan had correctly marked sentinel — so `validate consistency` warned that a bracket ICEBOX phase had no directory, in the HOUSE ROADMAP shape where an icebox appears as both a bold bullet and a detail heading. `validate health` stayed silent on that same repo, so the two verbs disagreed — which is the disagreement `sentinelPhases` exists to close. Both directions are pinned, because the failure mode of a careless fix here is the opposite one: a real phase sharing a sentinel's token must still warn. It does, in all four shapes that attack it (sentinel heading + real bullet, lowercase sentinel, sentinel after the real heading, colon-less bullet). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): count bracket headings, and retire them, in both derivations Both `total_phases` derivations select their grammar from the resolved convention, in one commit — cmdStateSync already carries the comment that it mirrors buildStateFrontmatter "so both report consistent percents (#3242 Bug B)", so teaching one and not the other ships that divergence. The #1514 retirement filter widens WITH the counter it protects. The canonical gesture strikes the checklist BULLET and leaves the detail heading intact, so a bracket-form retirement went undetected and the phase stayed in the denominator forever. That is half a fix alone: the retired key is compared against phaseKeyFromDir, which called extractPhaseToken with no convention. Both halves land here. Under bracket the sentinel token rule composes as the full engine set {0, 999}, so this counter agrees with `roadmap analyze`, which has always excluded both — otherwise the two derivations report different numbers for one ROADMAP and the changeset's "excluded from every count" is false as written. The LEGACY path keeps its pre-existing 999-only rule: widening it there would move legacy totals, so the two stay split off the bracket path exactly as they are today. The sync-side assertion reads the PERCENT sync writes into the STATE.md body, not the frontmatter total_phases. Sync's own counter never reaches that field — the read derivation writes it — so asserting the frontmatter after a sync measures the read path twice and lets a mutation to the write-path guard survive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): verify.cts bracket-coherence W021 + selected milestone-complete read The shipped milestone-prefixed W021 gate keeps its ROOT-only config read, verbatim base semantics. Federating it silently moved a legacy convention's answer in BOTH directions on workstream repos — a W021 that fires at base vanishing, and one that is silent at base firing. resolvePhaseIdConvention governs the new bracket-selection reads only. B6, the milestone-complete check, keeps its ungated POSTURE (bug-557 pins it with an empty config) but selects its grammar from the convention. Inferring 'bracket' from the shape of a matched bracket ran a repo-failing check against a legacy ROADMAP that merely contained `### [RFC.2119] 5:`. Directory resolution widens with the heading read, so a bracket repo whose phases are on disk stays silent, and a bracket sentinel is not reported as unstarted. checkBracketCoherence is advisory and gated. Anchored to tokenizeHeadings so fenced examples cannot warn and heading level is structural. Its scope rules each close a way it silently did nothing or fired wrongly: only a genuine MILESTONE heading opens or closes a section (a `### Notes` used to reset scope and disable both sub-checks); a legacy `## v3.0` DOES close it; an M-NN or letter-suffixed phase heading raises missing-bracket and CONTINUES; a bare `#### 2026:` is not a phase; the full h2-h6 range is processed. Its section recognizer shares the one milestone width, so an unpadded `### [GSD.3] 05:` can no longer be a phase to the id grammar and a section to the section grammar at once, silently re-scoping every warning after it. validate consistency suppresses bracket sentinels in its missing-directory warning — the two verbs disagreed, health suppressing via notStartedPhases while consistency did not. The legacy reading is untouched, including its pre-existing wart that `### Phase 999:` still warns there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): scope the milestone by its bracket; select the disk-side filter Two roadmap-parser reads, both of which made a bracket project's totals track the disk instead of the ROADMAP. The ADR pins the bracket milestone heading as `## [GSD.02] Foundation` — a name, no version — but scoping matched STATE's `milestone: v2.0` STRING against a heading, so the canonical form matched nothing and total_phases fell back to the directory count. The rule was re-derived in THREE places: extractCurrentMilestone plus two `milestoneBounded` guards; fixing one left the others falling back regardless, so they are now one gated helper. It matches the CANONICAL padded spelling only — accepting `0*N` bounded a milestone whose phases were invisible, which un-suppressed a progress percent computed off an unscoped disk count. getMilestonePhaseFilter's heading scan becomes the 14th selected read. On a bracket ROADMAP it collected nothing, so the filter degraded to pass-all and buildStateFrontmatter counted every other milestone's directories — making the bracket convention strictly worse than the M-NN one it supersedes on the property that matters most: totals must track the ROADMAP, not the disk. The DIRECTORY side of that same filter is selected with it. Teaching only the heading scan was half a fix and a worse one: `milestonePhaseNums` became non-empty, so the pass-all degrade stopped firing, but no bracket directory could satisfy the three legacy dir checks (numericRe fails on `GSD.02-05-five`, the custom-id match captures the project code `GSD`, and stripProjectCodePrefix does not strip a dotted prefix). Every bracket directory was rejected, and completed_phases / total_plans / completed_plans / percent all collapsed to 0 while `state sync` went on writing a percent off the unfiltered disk — `state json` reporting 0% on the same repo, in the same second, that STATE.md's body called 67%. That is the #3242 Bug B divergence this PR exists to avoid, and total_phases could not show it: `Math.max(phaseDirs.length, roadmapPhaseCount)` floors it at the ROADMAP count no matter how many directories are rejected. The dir side matches on the milestone-QUALIFIED id, delegated to the owner's gated `phaseTokenMatches(dir, id, 'bracket')`, not on the bare token: READING-B puts the milestone in the bracket, so `GSD.01-01-old-one` and `GSD.02-01-one` share the token `01` and only the qualified key separates them. The qualified ids are kept in their own set — a hyphen in `milestonePhaseNums` would flip `roadmapUsesHyphenedIds` and silently move the LEGACY dir path on a bracket repo — and the branch is ADDITIVE: on a miss it falls through to the three legacy checks, so a bracket project carrying legacy-shaped directories reads unchanged. Both are resolved lazily and gated, so the legacy path pays neither a config read nor a second scan and cannot change answer. The scoping call is also GUARDED: resolvePhaseIdConvention reaches planningDir, which throws a plain Error for a GSD_PROJECT/GSD_WORKSTREAM segment carrying `/`, `\` or `..`. At base the only planningDir call in extractCurrentMilestone sits inside the STATE-read try, so the function returned normally on such an environment; an unguarded one here let that escape and broke the never-throws invariant that getRoadmapPhaseInternal and getMilestoneInfo three hundred lines below carry #2245 / ADR-227 notes about. Unreachable through the CLI — GSD_WORKSTREAM is rejected up front by the workstream-name policy and GSD_PROJECT throws identically at base — but reachable by any in-process embedder, which is precisely who that invariant is for. The filter's own resolve call was already inside its try and is unaffected. The milestone-qualified key is formed only for a token that is itself a bracket phase token. `${bracketId}-${token}` is a string SPLICE, so a mid-migration heading carrying an M-NN label — `### [GSD.02] Phase 02-01:` — spliced to `GSD.02-02-01`, which the qualified-key grammar reads as milestone 02 / phase 02: the `-01` truncated, both such headings collapsing to one key, and the heading claiming `GSD.02-02-two`, the directory it does NOT name, while rejecting `GSD.02-01-one`, the one it does. The guard drops those headings back to the unqualified legacy path, restoring the base ACCEPTANCE VECTOR exactly — pinned against the milestone-prefixed reading of the same ROADMAP, which is base-identical on this shape. Scoped precisely, because the fixture moves one number that the guard does not touch: `total_phases` on it reads 1 at base and 2 here. That is the bracket heading COUNT this PR exists to add, not the splice — measured identical with and without the guard, and identical to what the canonical `### [GSD.02] 01:` spelling does on the same fixture (both read 2 with zero directories on disk, where base reads 0). The claim is base-equivalent ACCEPTANCE, not a base-equivalent reading. One consequence is stated rather than fixed: a heading whose token carries a hyphen still puts that hyphen into milestonePhaseNums and so still flips `roadmapUsesHyphenedIds`. Base does the same for that spelling, so preserving it is what keeps the shape base-equivalent; excluding the token would have moved answers versus base on malformed input. The comment at the qualified-set declaration is corrected to claim only what is true — it keeps QUALIFIED IDS out of that flag's input, not hyphens in general. The oracles ship with it, and they are the five numbers, not the one: the parity gate now asserts total_phases, completed_phases, total_plans, completed_plans AND percent, on both derivations, on two fixture shapes (one milestone; two milestones with stale prior-milestone directories on disk). The oracle is the flat-legacy twin, built in the same test run and compared number for number, plus exact literals so a shared wrong answer cannot pass. The oracle SUBSTITUTION is itself pinned. The M-NN spelling of these shapes could not serve, because buildStateFrontmatter's #2445 de-dup key captures only a directory's leading integer and collapses `02-01-one` / `02-02-two` / `02-03-three` to one — measured [3,0,1,0,0] against the flat-legacy twin's [3,2,3,2,67], identically at base and before this fix, and structurally unreachable from the bracket key space. That reasoning is only sound while it stays true, so a characterization test holds the M-NN reading down on the two numbers that do not depend on which directory wins the mtime race. Widen the de-dup key and it fails, instead of quietly invalidating the changeset's disclosure. Also adds the call-site pin. The structural table pins transcription against the selector; it cannot see a call site whose BASELINE ARGUMENT is wrong. Flipping verify.cts's milestone-complete site to the wider baseline grants a fires-on-every-repo check tolerance it has never had, and every behavioural test still passed. The pin reads the shipped sources and asserts the mode at each of the 14 sites, count-exact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): pin the bracket read surfaces in the parity gate This gate exists because #2043 fixed one bug across five hand-edited copies of a rule and #2232 was the residual that survived, because a later reader could not tell the copies were one rule. PR-2 adds two consumers, so they belong here. Surface 7 — the heading read and the directory read must agree about WHICH phase a `MM-<seg>` pair names, across the shared width corpus, and the bracket and legacy spellings of one heading must yield the same token. Surface 8 — the two bracket directory readers, in BOTH directions. Agreement on ACCEPTED input was already pinned; agreement on REJECTED input is where they actually diverged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2761): changeset Disclosures for the PR body (deliberate, not defects): - phase_id_convention is not a CONFIG_DEFAULTS key, so loadConfig drops it and cannot serve as the convention resolver however the file is federated. This PR ships its own workstream->root resolver; adding the key and its value enum is later-slice work. - Convention matching is strictly === 'bracket'. A misspelled value reads as not-configured and the project keeps legacy behaviour silently. - An UNPADDED bracket milestone (`[GSD.2]`) is malformed: it scopes nothing, bounds nothing, sections nothing, and is not a phase id. W005 on its directories is the surfacing signal. - WIDTH UNIFICATION MOVED FOUR MERGED PR-1 EXPORT ANSWERS on non-canonical inputs, none of which toDir can emit and none of which had a bracket caller at base: isSentinelPhaseId('GSD.0-01', 'bracket') true -> false isSentinelPhaseId('GSD.0999-01', 'bracket') true -> false getMilestoneFromPhaseId('GSD.2-01', 'bracket') 'v2.0' -> null getMilestoneFromPhaseId('GSD.002-01', 'bracket') 'v2.0' -> null The canonical pad2 sentinel spelling `[GSD.00]` still tests true. - FLAG TO MAINTAINER: docs/adr/612:132 reads "Sentinel behavior (0.x / 999.x -> milestone null) is preserved". After the unification that holds for the canonical `00` spelling only, not for a bare `[GSD.0]`. ADR wording is yours; flagging the tension rather than editing it. - The bracket sentinel rule COMPOSES with the legacy one — a bracketed heading is a sentinel when its bracket milestone OR its token is reserved. Under bracket the state-side token rule is the full {0, 999} set so both derivations agree; the LEGACY path keeps its pre-existing 999-only rule, unchanged. - validate consistency's legacy reading is untouched, including the pre-existing wart that `### Phase 999:` warns there while validate health suppresses it. - find-phase still cannot resolve a bracket phase directory. phase-locator.cts is outside this PR's module set. Sibling PR #2559's matchPhaseDirs calls phaseTokenMatches without a convention, so whichever slice lands second must thread it through. - Four of the five bracket readers scan raw ROADMAP content, so a bracket heading inside a fenced code block is read as a phase. Pre-existing for the legacy spelling; parity, not a new class. - roadmapPhaseLookupSources gained no bracket source: nothing emits a milestone-qualified query into it yet. - roadmap validate remains a separate, unfederated convention reader. Pre-existing and base-identical, but two verbs can disagree about the active convention on one project. - _diskScanCache keys on cwd while the values it caches are now convention-dependent. Not reproducible through the CLI; pre-existing for the workstream dimension, widened here. Stated as inconclusive. - A ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted — the deliberate migration-window trade. - THE READ AND WRITE PERCENTS STILL DIVERGE ON A MULTI-MILESTONE REPO, and that divergence is MIRRORED under bracket rather than closed. buildStateFrontmatter applies the milestone filter; cmdStateSync does its own fs.readdirSync and never calls it, so on a repo carrying prior-milestone directories the read path reports the SCOPED percent and the sync body reports the WHOLE-DISK one. Measured on the true base build ( |
||
|
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> |
||
|
Tom Boucher
|
86452da7cb |
fix(#4070): reserve shard 1's aux-suite cost out of the LPT unit-test packer (#4072)
* test(#4070): failing-first regression for shard1 aux-suite budget imbalance - selectShard has no way to reserve virtual weight on a bin, so the LPT unit-test packer cannot account for shard 1's fixed aux-suite cost (integration/security/install/slow all pinned to shard 1/3). - test.yml wires no such reserve into the workflow. - ci-test-job-timeout-budget.test.cjs's LANE_COSTS entry for job `test` was stale (7m12s from run 30677442953, predating the aux-suite growth); corrected to the real evidence cited in #4070 (13m48s / cancelled at ~14m51s), which now honestly fails the file's own 1.5x headroom policy against the current 15-minute cap. All three are expected RED on this commit; see .gsd/bug/fix-4070-shard1-aux-suite-budget/50-test-matrix.md. * fix(#4070): reserve shard 1's aux-suite cost out of the LPT unit-test packer selectShard now accepts an optional initialWeights array giving one or more bins a virtual head start before any file is placed, so LPT converges each bin's FINAL total (assigned weight + head start) toward equal instead of raw assigned weight alone. test.yml wires RUN_TESTS_SHARD_RESERVE=1:77 into the full-scope unit-test step (gated on matrix.scope == 'full', so the unrelated windows lane is unaffected) -- 77 weight units is the empirical conversion of the aux suites' ~220s measured fixed cost, derived against the real tests/test-timings.json (see the diagnosis artifact for the full computation). Also corrects two pieces of now-stale bookkeeping this issue exposed: - ci-test-job-timeout-budget.test.cjs's LANE_COSTS entry for job `test` carried a 7m12s figure that predated the aux-suite growth; corrected to the real pre-fix evidence (13m48s / cancelled at ~14m51s, issue #4070), which requires raising timeout-minutes from 15 to 21 (1.5x headroom over the real worst-case measurement) to satisfy the file's own policy. - test.yml's job-header and matrix comments, which still claimed the aux suites cost "~1m35s combined" (they now measure ~216-224s). Closes the gap the previous commit's failing-first tests proved: selectShard had no reserve-capacity mechanism and test.yml wired none in. * fix(#4070): correct the reserved-weight property bound; cover main()'s reserve bounds check Isolated code review found a genuine gap and gsd-test's real run confirmed a real test bug it exposed: - The fast-check property "no shard exceeds average(+reserve) + heaviest file" was falsified by gsd-test itself (weights=[1,1,1], total=2, reserve=6 on bin 0): selectShard is correct, the BOUND was wrong. A reserve large enough that its bin never receives a real item stays at exactly that reserve forever -- no amount of routing real items elsewhere can dilute a fixed head start below itself -- so the true bound is max(reserve, the classic Graham term), not the Graham term alone. Verified the corrected bound against the exact counterexample plus 20,000 additional random trials (zero violations) before re-running gsd-test. - Isolated review (MAJOR): the shard-total bounds check on RUN_TESTS_SHARD_RESERVE and its console.error fallback in main() were untested end-to-end -- parseShardReserve itself has no concept of the shard total, so only main() enforces that guard, and nothing exercised it through the subprocess seam. Added an E2E harness test that sets RUN_TESTS_SHARD_RESERVE to an out-of-range index via the real CLI, asserts the fallback warning fires, AND asserts the resulting file selection is byte-identical to a no-reserve control run against the same injected timings table -- proving the reserve was actually ignored, not just that a warning printed. * chore(#4070): backfill changeset PR number pr:0 -> pr:4072 * fix(#4070): strip leaked RUN_TESTS_SHARD_RESERVE from the harness test's child env Real GH Actions CI on this PR (run 33288554040, ubuntu shard 2/3) failed 7 tests in the shard-partitioning describe block, all with the same symptom: `run-tests: no tests in suite "all"` where a real file count was expected. gsd-test's own dockerized bench run never showed this, and ubuntu shards 1/3 and 3/3 (which run the same test.yml step) passed clean -- the discrepancy is the tell: only shard 2/3 happened to schedule this specific test FILE for that run, and the outer CI job's own environment is where the leak lives. Root cause: test.yml's "Run unit tests" step now sets RUN_TESTS_SHARD_RESERVE=1:77 (this issue's own reserve mechanism) on the OUTER job that runs `npm run test:coverage:unit:raw -- --shard N`. The harness test file's runHarness() helper spawns run-tests.cjs as a CHILD of that same job via `{...process.env, ...extraEnv}`, so every pre-existing --shard test in this describe block silently inherited the ambient reserve -- even though none of them know it exists. A reserve of 77 weight units utterly dwarfs the ~0.3 total weight of the 9-file synthetic fixtures these tests use (none are in the real timings table, so all fall back to the same tiny median weight), so shard index 1 is routed zero files every time -- exactly the observed "no tests" failures, and exactly the skewed 5/4 split observed on the shard-2 test that expected a plain 3/3/3 round-robin. Reproduced locally end to end (set RUN_TESTS_SHARD_RESERVE=1:77, spawn the old runHarness against a synthetic 9-file fixture, --shard 1/3 -- reproduces the exact "no tests in suite \"all\"" stderr) and confirmed the fix (env stripped unless a test opts in via extraEnv, as the #4070 E2E bounds-check test already does) resolves it, before re-running gsd-test. This is a genuine bug this PR introduced -- a new ambient env var that a pre-existing subprocess-spawning test helper didn't know to isolate against -- not a pre-existing flake and not resource contention. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8793d307f6 |
fix(#4060): drive repo-baseline lint check in-process, not via a subprocess timeout race (#4065)
* test(#4060): failing-first regression for repo-baseline subtest timeout race The "repo baseline passes" subtest in lint-allow-test-rule-refs.test.cjs drives the script under test via a spawnSync subprocess with a fixed 30s timeout, which races the script's real wall-clock completion against unbounded CI-load contention -- it has already died at this race twice (#4060, and once before at a lower bound). Rewrites the subtest to call the script's `main` directly, in-process, removing the subprocess timeout race entirely. This commit only changes the test (main is not yet exported), so it fails first with `TypeError: scriptUnderTest.main is not a function`. * fix(#4060): export lint-allow-test-rule-refs main() for in-process drive The "repo baseline passes" subtest previously drove this script via a spawnSync subprocess with a fixed 30s timeout, racing the script's real completion time against unbounded CI-load contention -- it has now died at that race twice (#4060, and once before at a lower bound). A fixed timeout racing unbounded contention has no value that is both tight and safe, so raising it again would not fix the mechanism, only its odds. Parameterizes main() to accept an explicit argv (defaulting to process.argv.slice(2) only when omitted, so the CLI entrypoint is unaffected) and exports it, so the test can call it directly, in-process -- removing the subprocess and its spawnSync timeout kill race entirely for this one row. * fix(#4060): capture stderr too in the in-process repo-baseline subtest Code-review finding: the in-process rewrite captured only console.log, but main()'s real failure path throws a bare, messageless ExitError -- all diagnostic detail goes to process.stderr.write. The old subprocess-based assertion embedded both stdout and stderr in its failure message; this silently dropped that debuggability. Captures process.stderr.write the same way (restored in finally) and surfaces both streams in assertion failure messages and in a wrapped re-thrown error on an unexpected throw from main(). --------- Co-authored-by: sim <sim@local> |
||
|
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> |
||
|
Tom Boucher
|
370cfc6680 |
enhance(#4036): persist CI shard/job timeout-vs-cap trending, warn at 90% (#4043)
* feat(#4036): persist CI shard/job timeout-vs-cap trending, warn at 90% Adds two new mechanisms plus an audit-coverage extension: - scripts/lib/ci-job-timing.cjs: shared elapsed-vs-cap arithmetic - scripts/ci-check-job-near-cap.cjs: in-job advisory near-cap check, wired into test/test-full/mutate/smoke as each job's last step - scripts/ci-timeout-report.cjs + .github/workflows/ci-timeout-report.yml: scheduled REST-API poll that appends new records to tests/ci-timeout-budget-history.jsonl and opens a small data-only PR - tests/ci-test-job-timeout-budget.test.cjs: extended to cover mutate (mutation.yml) and smoke (install-smoke.yml), which previously had no headroom-factor gate coverage at all Does not change any timeout-minutes value, shard composition, or shard-1 contents — those stay maintainer policy calls per the issue's own scope. * fix(#4036): address two-orthogonal-review findings - Parity tests guarding the two hand-duplicated literals this design cannot single-source through GH Actions YAML: CI_JOB_TIMEOUT_MINUTES vs each job's own timeout-minutes, and ci-timeout-report.cjs's JOB_RULES name-prefixes vs each job's actual name: template. - Thread run.event through as runEvent on every persisted record, so PR-context and push-context install-smoke timings (genuinely different matrix shape) are distinguishable in the history rather than silently conflated under one job name. - Replace the Windows near-cap start-time step's ambiguous PowerShell +/>> precedence with GitHub's documented string-interpolation form. - Move github.run_id out of direct ${{ }} shell interpolation into an env: var in the new scheduled workflow, per this repo's own expression-injection-safe convention. * test(#4036): regenerate golden install-tree fixtures for scripts/lib/ci-job-timing.cjs npm run gen:install-tree — scripts/ ships wholesale into the installed package (per ADR/known-defect precedent from #4012's own PR history: a new scripts/lib/*.cjs file needs its golden entry regenerated or every runtime's install-tree test fails). Confirmed via gsd-test: this was the sole cause of the first real verification run's 25 failures (all in tests/golden-install-tree.test.cjs, one per runtime). Top-level scripts/*.cjs files (ci-check-job-near-cap.cjs, ci-timeout-report.cjs) are not individually tracked in these fixtures — consistent with every other existing top-level scripts/*.cjs file, so no entry was expected or added for those two. * fix(#4036): register new lib file with installer, fix H1 shell policy - bin/install.js: add ci-job-timing.cjs to GSD_SCRIPTS_LIB_FILES (a hand-maintained registry, not generated — tests/install.test.cjs asserts every scripts/lib/ file is enumerated here) - test.yml: replace the two OS-conditional "Record job start time" step pairs (test + test-full jobs) with a single unconditional `node -e` step. The prior pair's Windows variant declared an explicit shell: pwsh, which scripts/workflow-policy.cjs's H1 checker statically flags against every OS a job's matrix can realize, independent of the step's own if: gate. A single Node one-liner needs no shell override at all — it's syntactically valid and behaves identically under bash, zsh, and pwsh — which is both H1 compliant and removes the last OS-specific shell syntax from this change entirely. Both defects were found by a real gsd-test run, not local gates — lint:ci and build:lib were clean throughout because neither the scripts/lib/ install-manifest parity check nor the H1 shell-policy baseline runs as part of lint:ci; both are gsd-test-only suites. * docs(#4036): how-to for reading CI timeout budget signals The phase-gate docs check correctly flagged the enablement sequence as 3 real steps (read the near-cap warning, find the accumulated trend file, pick the right maintainer lever) — a reference table can't carry a sequence. Adds docs/how-to/read-ci-timeout-signals.md, indexed from docs/README.md. * chore(#4036): backfill changeset PR number (4043) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fd63889d1f |
fix(#3854): write normalization preserves tight multi-line lists (#4049)
* test(#3854): write normalization must preserve tight multi-line lists (failing first) * fix(#3854): no blank before a bullet whose previous line is an indented continuation _normalizeMd's 'separate a list from a preceding paragraph' rule inserted a blank before any bullet whose previous line wasn't a bullet — but an INDENTED CONTINUATION of the previous multi-line item also isn't a bullet. Every .md write (phase.complete in the report, but any write through platformWriteSync) therefore converted tight lists to loose ones: +61 blank lines on the reporter's 1015-line ROADMAP, one before each bullet following a wrapped item. Tight and loose lists render differently, so this was a rendering change plus misleading diff noise; one-shot (idempotent afterwards), which is why integrity checks on headings/content passed. The guard is the mirror image of the after-a-bullet rule two lines below, which already excludes indented next lines. Paragraph→list and heading→list separations — the rule's purpose — are pinned unchanged by the new suite. * fix(#3854): review fold-ins — ceiling tracks next's 281 + this branch's marker (282), header/require nits The ceiling is not ratcheted but must track the tree: origin/next raised it to 281 (sibling branch's marker file); this tree adds one more (shell-command-projection-md-normalize), so 282/282. Also fixes the test header's stale pre-rename filename and hoists the inline require to the file's single import. * chore(#3854): changeset fragment (pr number backfilled after PR creation) * chore(#3854): backfill changeset PR number (4049) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
0bf778c352 |
fix(#3849): phase allocation counts numbers held by sibling git worktrees (#4042)
* test(#3849): phase allocation must skip numbers held by sibling worktrees (failing first) * fix(#3849): phase allocation counts numbers held by sibling git worktrees Both allocators (cmdPhaseAdd, cmdPhaseAddBatch) chose max+1 over numbers gathered from ONE checkout — headers, bullets (add only), on-disk dirs. Every sibling git worktree carries its own .planning/ on its own branch, so a phase minted there was invisible and the same number was allocated twice (the reported incident: two Phase 441s, one with six written plans, surfaced a day late by human memory). New shared horizon collectSiblingWorktreePhaseNums: one git worktree list --porcelain, then per sibling — phase-dir names (the cheap scan that would have caught the incident) and the WHOLE sibling ROADMAP.md headers (a row can predate its dir; milestone-scoping would be wrong — a number used under any milestone on another branch is taken). Widen, never refuse: unreadable sibling / no .planning / not a git repo / git unavailable each contribute nothing and allocation is unchanged. Reuses isSentinelPhaseId and the allocators' own patterns. Secondary (#1229 never reached batch): cmdPhaseAddBatch now also scans roadmap bullets — a bullet-only 'Phase N' row was invisible to batch allocation, exactly the condition #1229 was filed for. Also fixes the two new tests' result-key access (output.phases, not output.results). * fix(#3849): review fold-ins — subprocess band, bounded test git, linked-worktree fixture - execFileSync options now match the repo's git band (10s window, windowsHide, 4MiB maxBuffer) — a spurious 4s timeout silently reverted to the pre-fix collision. - test git helper bounded (15s) per local/no-unbounded-spawn. - new fixture: allocation FROM a linked worktree counts the main checkout — the incident's actual topology direction. - fixture-setup rmSync carries the sanctioned lint-disable (setup, not teardown; cleanup() still owns directory removal). * fix(#3849): exempt the sibling-worktree scan from the enumeration-drift guard collectSiblingWorktreePhaseNums reads a SIBLING checkout's phases dir — a different question from the cwd-scoped listMilestonePhaseDirs the guard routes everything to (which cannot see another worktree's .planning at all). Function-scoped, per ADR-3180 Decision 4(a): any other re-derivation in phase.cts is still caught. The GREEN bench caught the omission. * chore(#3849): changeset fragment (pr number backfilled after PR creation) * chore(#3849): backfill changeset PR number (4042) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
519ac23ebb |
fix(#3839): hook tables say PreToolUse (validate-commit) and SessionStart (session-state) (#4041)
* test(#3839): docs hook tables must match surface registrations (failing first) * docs(#3839): hook tables say PreToolUse for validate-commit, SessionStart for session-state gsd-validate-commit.sh is registered PreToolUse (src/runtime-hooks-surface.cts; its exit-2 block IS the contract — a post-tool hook cannot prevent a commit) and gsd-session-state.sh is registered SessionStart (session orientation, not post-tool tracking). Both rows said PostToolUse in ARCHITECTURE.md and the three INVENTORY locales; the issue asked for a neighbouring-row scan, which is how the session-state row was found. All other rows in the four tables verify against the surface. * fix(#3839): review fold-ins — 10 more wrong rows in ko-KR/pt-BR/zh-CN, parser authority + drift pins Adversarial review found the same two wrong rows shipped in five more files the issue's table missed (ko-KR ARCHITECTURE+INVENTORY, pt-BR ARCHITECTURE+INVENTORY, zh-CN ARCHITECTURE) — all fixed; DOC_TABLES now covers all ten shipped tables. The parity parser unioned only the Kimi mirror list, silently exempting agent-isolation-guard (registered via the dynamic preToolEvent push): probes are now parsed too, with bare hook names resolved against hooks/ ground truth and dynamic event variables resolved to their canonical (non-Gemini) events; an exact-set pin replaces the loose size guard. allow-test-rule marker carries the issue ref; unverified-ceiling 280→281 (audited: the new marker is legitimate — the suite reads product docs whose text is the contract). * fix(#3839): register the hook-table parity suite in the docs-guard lane The new suite reads ten docs/ paths, so lint-docs-guard-registration requires it in the docs-guard registry — the first GREEN bench run caught the omission (the RED run's docs-guard failures were the same signal, previously misread as marker fallout). * chore(#3839): changeset fragment (pr number backfilled after PR creation) * chore(#3839): backfill changeset PR number (4041) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
331747ea99 |
fix(#3817): count the truncation remainder — display truncates, counting must not (#4034)
* test(#3817): audit-open counts must include the truncation remainder * fix(#3817): count the truncation remainder — display truncates, counting must not * chore(#3817): changeset fragment (pr number backfilled after PR creation) * chore(#3817): backfill changeset PR number (4034) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ac0eed1267 | Merge pull request #4015 from open-gsd/fix/3889-instrument-chunk-timeout | ||
|
Tom Boucher
|
213a2fff63 |
chore(#3813): delete the caller-less listMilestoneArchiveDirs seam; #1883 contract now pins the live path (#4029)
* test(#3813): pin the #1883 unreadable-milestones contract on the live planning-snapshot path * fix(#3813): delete the caller-less listMilestoneArchiveDirs seam; #1883 contract now pins the live path * chore(#3813): changeset fragment (pr number backfilled after PR creation) * chore(#3813): backfill changeset PR number (4029) * chore(#3813): docs-exempt marker — internal dead-code removal --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3a4c3cb83e |
fix(#3805): audit-uat honours the audit_acknowledged marker via the shared predicate (#4025)
* test(#3805): audit-uat must honour the audit_acknowledged marker * fix(#3805): route audit-uat's UAT and VERIFICATION scans through the shared acknowledged predicate * chore(#3805): changeset fragment (pr number backfilled after PR creation) * chore(#3805): backfill changeset PR number (4025) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
f4fefb0bef |
fix(#3804): audit-uat enumerates all three phase-archive layouts (#4022)
* test(#3804): audit-uat must see all three phase-archive layouts * fix(#3804): enumerate all three phase-archive layouts (flat, workstream-archived, workstream-active) * chore(#3804): changeset fragment (pr number backfilled after PR creation) * chore(#3804): backfill changeset PR number (4022) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
80de48c319 |
enhance(#3914): every phase records a truthful guard ledger (#4018)
* fix(#3914): retire n/no-process-exit where its successor governs
Epic #3889 criterion 5 — no phase closes with a guard added and its
predecessor left standing — is violated in the tree by the epic that wrote it.
local/require-registered-exit was registered on gsd-core/bin/**/*.cjs and
scripts/**/*.cjs, while n/no-process-exit stayed 'error' over a nine-glob block
covering those same two. Only the hooks 'off' exemption ever came down; the
predecessor's registration never did. Both rules have been enforcing the same
property on the same surfaces since P6.
Narrowed, not deleted. Seven of those nine globs have NO successor —
eslint-rules/, bin/lib/, pi/, examples/, vscode/, .kilo/, .opencode/ — so
deleting the rule outright would silently drop enforcement on all seven. That
is the inversion this epic has already hit three times: removing a coarse guard
because a narrower one exists somewhere it does not reach. Flat config is
last-match-wins and both successor blocks come after the nine-glob block, so
'n/no-process-exit': 'off' in exactly those two retires the predecessor
precisely where the successor governs and nowhere else.
The successor is strictly more precise: it permits process.exit only inside
terminateNow in cli-exit.cts, the single sanctioned terminator (ADR-3889 §3),
where n/no-process-exit permits none and would flag terminateNow's own
generated copy.
Asserted at the consumer's altitude via ESLint.calculateConfigForFile on real
paths, with the positive control that matters: n/no-process-exit is still
'error' on six of the seven successor-less globs, so a future edit that turns
this into a blanket disable goes red. bin/lib/ has no file in this checkout and
is reported as untested rather than given an invented path. Severity is
normalized across the string/numeric/array forms the API can return, and the
normalized value asserted — not truthiness.
Verified by running calculateConfigForFile myself on both superseded globs and
four controls before trusting the test.
Found and fixed inline: the change made an eslint-disable directive at
gsd-tools.cjs:257 partially unused, which --max-warnings 0 rejects; narrowed to
the one rule still in force.
Verification runs on the remote runner.
Refs #3914
* docs(#3914): the epic added three guards, it did not remove one
The audit reconciled the epic ledger against what actually landed. The net is
+3, not -1: four lint:generated-sync --check arms (gen-scripts-cli-exit,
gen-hooks-cli-exit, gen-exit-code-registry, gen-exit-code-docs) plus one rule,
against two retirements.
An epic whose thesis was consolidation ended with a larger guard surface than
it started with. The additions are each defensible; the claim that the total
fell was never true.
Two of the three prior errors in this amendment are mine. It said "Net -1 by
count" above terms reading -1 -1 +1 +1 +1, which sums to +1 — an arithmetic
error in the paragraph directly below the sentence arguing that an ADR about
honest accounting must not pad its own ledger. And the term list omitted two of
the four --check arms, which is what turns that +1 into the real +3.
Recorded rather than quietly rewritten. This ledger has now been wrong three
times — the original -2, the -1 that replaced it, and #3914's own table, which
states -1 above terms summing to 0 — and a written claim nobody checked against
the thing it describes is the exact failure this epic exists to close.
Refs #3914
* fix(#3914): make the successor actually supersede before retiring the predecessor
An isolated security review found that the previous commit turned off a guard
that was still doing work. Reproduced by executing both rules against a
fixture, not inferred:
const exit = 'exit';
process[exit](1);
n/no-process-exit flags it; local/require-registered-exit did not, because it
early-returned on callee.computed. So retiring the predecessor on
gsd-core/bin/**/*.cjs and scripts/**/*.cjs un-guarded that shape on precisely
the two globs this epic's exit contract cares most about.
This is the third time in this epic I have removed a coarse guard on the claim
that a narrower one covered it, without checking construct-level parity — after
the allowlist key-to-prefix-to-exact-membership sequence and the band
ranges-to-categories one. The rule is the same every time: a narrower guard
supersedes a coarser one only where it demonstrably reaches at least as far,
and "demonstrably" means executing both against the constructs, not reading
either.
The successor now resolves computed property access for the statically
determinable cases — a string Literal, and an Identifier bound once to a string
Literal, resolved through scope — and leaves genuinely dynamic properties
alone so the rule does not over-fire. Measured after the fix: plain
process.exit flagged, process['exit']() flagged, process[exit]() flagged,
process[globalThis.k]() not flagged. That makes it a strict superset of the
predecessor on these globs, since process['exit']() was caught by NEITHER rule
before.
The second finding is worse than the first, because it was reasoning rather
than oversight. My justification comment claimed n/no-process-exit "would flag
terminateNow's own generated copy here". It would not — that file is in the
global ignore list, so neither rule ever lints it. There was no conflict to
resolve; I wrote a rationale I had not checked, in a change whose entire
subject is written claims nobody verified. Both comment blocks now state the
real basis.
The tests that should have caught this asserted only rule SEVERITY per glob and
never construct REACH, which is exactly how a coverage hole passed. A parity
matrix now pins all five shapes, including a RED/GREEN regression pin against
an inlined reproduction of the pre-fix rule — inlined rather than loaded from
HEAD, because HEAD resolves to the fixed commit under the remote runner and
would silently stop testing anything.
Verification runs on the remote runner.
Refs #3914
* fix(#3914): the two exit rules are complementary — keep both
Reverts this branch's retirement of n/no-process-exit. The premise was wrong
twice, and the second review proved the change itself was wrong.
I claimed local/require-registered-exit was a strict superset on
gsd-core/bin/**/*.cjs and scripts/**/*.cjs. Measured, successor vs predecessor:
function f(exit) { process[exit](1); } 0 vs 1
let exit='exit'; exit='exit'; process[exit]() 0 vs 1
const { exit } = ...; process[exit](1) 0 vs 1
plus for-of bindings, let-then-assign, var redeclaration, catch params, and an
undeclared global named exit. The predecessor matches any identifier NAMED
exit however it is bound; the successor resolves only a string literal or a
single-write const. It never was a superset — I asserted the relationship after
fixing one construct and did not re-check the rest.
The justification was independently false: all three generated cli-exit copies
are in the global ignore list, so n/no-process-exit was never flagging
terminateNow. There was no conflict to resolve. I wrote a rationale I had not
verified, in the phase whose subject is written claims nobody checked.
So criterion 5 does not apply to this pair. They are not predecessor and
successor — they are complementary, each catching constructs the other misses.
The epic's criterion assumed a replacement relationship that does not exist
here, and retiring either rule loses real coverage. The ADR ledger now says so
with the measured shapes.
What survives is the genuine improvement: the computed-property strengthening.
local/require-registered-exit now catches process['exit'](1) and optional-chain
terminators like process?.[k]?.(1), which NEITHER rule caught before, while
correctly ignoring a genuinely dynamic property so it does not over-fire.
The parity tests are rewritten to assert what is true rather than what I wanted
to be true: a bidirectional matrix where each rule is shown catching shapes the
other misses. The previous matrix tested only the four shapes where the
successor wins, which is precisely why the regression shipped — a test set
selected to confirm the thesis.
Also corrected: a stale ADR sentence claiming a third wrong ledger version that
does not exist (the table it described now reads +3 over terms summing to +3),
and a changeset whose stated motivation was the false generated-copy conflict.
Verification runs on the remote runner.
Refs #3914
* fix(#3914): the exemption term was a no-op — the net is +4
Fourth correction to this ledger, and a fourth error of the same kind.
Every version counted removing the n/no-process-exit 'off' entry from the hooks
block as -1. Measured: calculateConfigForFile returns undefined for that rule on
hooks/**. It was never registered there, and no broader block sets it globally,
so the 'off' entry overrode nothing and removing it changed no enforcement at
all. A no-op removal, not a guard removal — the same category error as counting
baseline acknowledgement entries: a thing that is not a guard, in guard units.
It is misattributed too; that block came down in
|
||
|
Tom Boucher
|
ac7587287b |
fix(#3812): document how Current Position actually resolves a duplicate field (#4017)
* docs(#3812): say that Current Position is single-valued, and pin the behavior that makes it true #3812 shipped CLOSED with half its acceptance unmet. #3873 delivered cardinality for FRONTMATTER keys - current_phase/current_plan render as optional at docs/reference/state-md.md:89,91, covered by tests/gen-state-md-docs.test.cjs:374. The issue's actual ask was the ## Current Position BODY section, and that never landed. Surfaced by an /adr-phase-coverage audit of epic #3473; the issue was reopened rather than noted. The section now states three things: every field is single-valued, the section is overwritten rather than appended to, and a duplicate resolves to the FIRST occurrence with no warning - so a line appended in good faith is silently ignored rather than winning. Progress history belongs in ## Performance Metrics, two headings down, and the text now points there. The third claim is a behavioral promise about the reader, so it was VERIFIED BY EXECUTION before being written rather than inferred from the issue title: stateExtractField(<"Phase: 1 of 5 (First)" ... "Phase: 9 of 9 (Appended later)">, "Phase") -> "1 of 5 (First)" The mechanism is state-document.cjs:405 - the plain-line pattern ^<field>:[ \t]*(.+) carries flags im with NO g, so String.match returns the first hit. Writing "first wins" without running it would have repeated the exact error I had to retract twice in this epic already. A test pins the reader, not the prose. Three rows in tests/state.test.cjs: T1 (load-bearing) asserts the duplicated case resolves first; T2 asserts the ordinary single-field case still works, so a fix that only functions when duplicated cannot pass; T3 puts a Plan: line BETWEEN the two Phase: lines and asserts it resolves independently - negative space, because a reader returning the first line of the SECTION rather than the first matching FIELD would satisfy T1 alone. Proven to discriminate: a last-match variant returns "9 of 9 (Appended later)" and T1 reds. No assertion checks that the document contains a sentence. That is what local/no-source-grep exists to stop, and it would pin wording that is allowed to improve. The point of the test is that if that regex ever gains g and a last-match walk, the test fails - instead of the documentation quietly becoming a lie with nothing to notice. Prose only, no new heading. docs-state-md-locale-parity compares heading-level sequences by LCS rather than text, so added paragraphs cannot fail it while an added HEADING would fail all four locales. The constraint is structural, not stylistic - confirmed by running that comparison after the edit. The four locale copies are translated rather than left stale. They are not gate-enforced for prose, so "nothing fails" was available and is not the same as correct: leaving four documents asserting something the English one now contradicts is a correctness problem. Code spans and the anchor link stay untranslated - they name real tokens. The whole approach rests on one fact, checked first: ## Current Position at :196-208 sits OUTSIDE every generated marker region (:81-104, :138-151), so a hand edit survives --write. Re-confirmed after all five edits - gen-state-md-docs --check reports all 6 targets up to date. Had that been false the fix would have belonged in the generator, and a hand edit would have been silently reverted. One real gate failure fixed inline rather than reported: the new test's comments referenced docs/reference/state-md.md, which was not in that file's registered exempt-docs paths, and lint-docs-guard-registration failed lint:ci correctly. Registered. Known limit, named rather than folded in: gsd-tools validate/health still do NOT warn on a duplicated Phase:. #3812 records that as a "consider", not a requirement, and confirms none of the nine rules in src/health-diagnostic-rules/{state-consistency,phase-structure}.cts counts occurrences. Documenting the silent first-match is the delivered scope; making it loud is new scope and stays unclaimed. Closes #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3812): the rule I documented was false — replace it with the measured one An isolated review returned two blockers. Both mine, and the first is the worse kind: I wrote a falsifiable rule into a reference page and got it wrong. 1. "A duplicate resolves to the FIRST occurrence" is FALSE. stateExtractField (src/state-document.cts:401-419) tries BOLD `**F:**` across the whole input, THEN plain `^F:`, THEN a pipe-table row. Form precedence beats document order. Measured against the built reader, all intra-section: Phase: A (plain) / **Phase:** B (bold, later) -> B LATER WINS Phase: A (indented) / Phase: B (plain, later) -> B LATER WINS Phase: A (plain) / | Phase | T (table) | -> A first wins My original verification tested plain-versus-plain, saw first-wins, and generalized to all forms. Measuring one case and claiming the general rule is the same error I have had to retract twice already in this epic. It is also worse than silence. The sentence told authors an appended line is safely ignored; a bold line appended "for emphasis" silently overrides the original. Someone trusting the doc would have corrupted their own state file. And #3812 never asked for a resolution rule - it asked for single-valued, overwrite-not-append, and where history goes. The rule was my unrequested addition. Replaced with the measured truth: resolution is by FORM (bold anywhere, then plain at line-start, then table row), and only WITHIN the winning form does the first occurrence win. Both consequences stated plainly - a higher-ranked form wins regardless of position, and an indented `Phase:` is invisible to the plain form. All five claims in the new paragraph verified by execution before being written, including the two I had wrong. 2. The tests tested the wrong case and passed for the wrong reason. T1/T3 put the second `Phase:` under `## Somewhere else` - the INTER-section case, which #2956 already fixed by scoping. #3812 says verbatim that #2956 "fixed the inter-section case and never addressed intra-section duplication", so the case the new prose describes was untested, and the fixtures passed because of section scoping rather than field resolution. They also called bare stateExtractField rather than the production chain, T2 could not discriminate first from last at all, and no fixture mixed forms - which is precisely why the false claim survived to review. Rewritten as four rows, all intra-section, all through the real stateCurrentPositionSlice -> stateExtractField path: plain-then-plain (first wins within a form), plain-then-bold (the bold LATER value wins - the row whose absence let the false claim ship), indented-then-plain (indented invisible), and sibling-field independence. Each proven to fail against a reader that disagrees. 3. Two dead anchors. pt-BR and zh-CN linked `#performance-metrics` while their own headings are `### Métricas de Desempenho` and `### 性能指标`. Both fixed to the anchor their own heading generates. ja-JP/ko-KR kept the English heading, so theirs already resolved. 4. A ja/ko sentence inverted its own meaning. Both rendered "which is the section designed to grow" with a bare demonstrative whose nearest referent read as Current Position - saying the opposite of the point. Rewritten so the clause attaches unambiguously to `## Performance Metrics`. 5. Cross-locale drift, flagged by the implementing agent rather than by me: after fixing EN, the four locales still stated the OLD false rule. Four documents asserting something measured to be wrong is worse than four saying nothing. All four now carry a faithful translation of the corrected paragraph, with code spans, each file's own anchor, and the ja/ko referent fix preserved. Verified: all five claims executed against the built reader; every rewritten test row proven to discriminate; gen-state-md-docs --check reports all 6 targets up to date, so the edits stay outside the generated marker regions; locale heading parity unaffected (prose only, no headings added); build:lib, lint and lint:ci all exit 0. Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3812): second false rule on the same page — scope the ranking to the section A second isolated review found a second false falsifiable claim, and the failure mode is the same one twice in a row: attempt 1: verified plain-vs-plain, wrote a claim about ALL FORMS attempt 2: verified bare stateExtractField, wrote a claim about THE DOCUMENT Both times the claim covered a wider surface than what was actually executed. The fix each time was not a better sentence, it was executing the surface the sentence describes. BLOCKER — "bold `**Phase:**` anywhere in the DOCUMENT wins" is false. ## Current Position / Phase: 1 of 5 + ## Archive / **Phase:** 88 bare stateExtractField(whole doc) -> "88 (other section)" PRODUCTION (slice then extract) -> "1 of 5 (in section)" #2956's section slice means production never hands another section to the matcher; a bold line in `## Archive`, or in the YAML frontmatter, is simply not seen. The ranking is real but scoped: it applies WITHIN `## Current Position`. I verified against the bare function and wrote a claim about the system. Every existing test placed its bold line inside the section, which is exactly why nothing contradicted the claim. T5 now puts a bold `**Phase:**` in `## Archive` and asserts production returns the in-section plain value, with the unscoped reader asserted to DISAGREE so the row proves the scoping rather than assuming it. BLOCKER — the changeset still shipped the ORIGINAL retracted claim. I corrected the page and left the release note saying "resolves to the first occurrence ... a second entry added in good faith is silently ignored". The note contradicted the page it announces, and the release note is what most people actually read. Rewritten to the corrected rule. MEDIUM — the concession was inverted. It read "wins even if it comes FIRST in the file", which is the vacuous direction; the surprising case, and the one the very next clause illustrates with an APPENDED bold line, is "even if it comes LAST". All four locales reproduced the inversion faithfully, so it was an EN-source defect rather than translation drift. Two sharp edges now named, both measured: a bold `**Phase:**` followed only by trailing spaces resolves to an EMPTY STRING and does not fall through to a valid plain line below (T6 pins it); and `| **Phase:** | 3 of 4 |` short-circuits to the bold form and returns the literal `"| 3 of 4 |"`. A page that teaches form ranking has to say where the ranking bites. Also fixed: all five files labelled the link `## Performance Metrics` while the heading is `### Performance Metrics`. Anchors resolved correctly everywhere; only the label's level was wrong. Every clause in the final paragraph re-verified through the PRODUCTION chain (stateCurrentPositionSlice -> stateExtractField), clause by clause, before being written: bold in another section does not win; bold in frontmatter does not win; bold appended last does win; first wins within one form; trailing-space bold yields empty. All four locales carry the same corrected rule. gen-state-md-docs --check reports all 6 targets up to date; heading counts unchanged at 20/20 across all five files, so locale heading-parity is untouched; build:lib, lint and lint:ci all exit 0. Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3812): backfill changeset pr number Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
bf8905fcc9 |
fix(#4012): a hang never emits the events I was listening for
The init marker settled it. The diagnostic reported "THE REPORTER LOADED BUT RECORDED NO TEST EVENTS — the events file contains only the reporter's own reporter:init marker", which refutes the reporter-never-loaded hypothesis and leaves exactly one explanation. The runner spawns a child process per test file and surfaces a subtest's test:start / test:pass / test:fail to the parent's reporter only once the child REPORTS that test — which happens when it completes. The fixture hangs forever, so it never completes, so it never reports. I had recorded exactly those three event types: the precise set that a hang guarantees you will never see. The feature could not have worked for the case it was built for. test:enqueue and test:dequeue are emitted by the runner as it queues and begins each file, independent of anything inside finishing. test:dequeue is what actually means "in flight", and it is now the primary signal, with test:start kept as a secondary one. A file is in flight when it has been dequeued and has no terminal event. The four branches now describe states that are all real: the events file absent (reporter never loaded); the init marker alone (the runner dequeued nothing at all — genuinely surprising now rather than the expected outcome); everything dequeued and terminated (the files finished and the process hung afterwards, a handle leak); and one or more dequeued-but-unterminated files, named, which is the case this whole feature exists to report. Verified against the exact shape the real hang produces, by executing analyzeChunkEvents on a synthetic events file: init + enqueue + dequeue with no terminal event reports hangs.test.cjs as in flight, and appending a test:pass clears it. Four more unit tests cover the ordering and multi-file cases with no subprocess, so this logic is now checkable without a runner round-trip — which matters, because every defect in this feature so far was visible only remotely. T1 is untouched and should now pass for the right reason. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
444137e63a |
fix(#4012): make the artifact say whether the reporter ever loaded
Down to 3 remote failures, all one chain. The explicit no-events reporting is working — the diagnostic now states the events file does not exist, instead of silently printing the generic message. But it then ASSERTED a cause: "the child was killed before the reporter wrote even one event (process/spawn startup stall, not a test hang)". That was a guess dressed as a finding, and the fixture contradicts it: it starts a real test, so test:start should fire in milliseconds against a 2000ms budget. Two hypotheses remained and I could not separate them locally, because the local test runner is hook-blocked here: either the custom reporter never LOADS in the child, or it loads and no event reaches it before the SIGKILL. Rather than guess a third time, the artifact now answers it. The reporter appends a reporter:init line as its first action, before consuming anything, so the file's contents discriminate: absent means the reporter never loaded; init-only means it loaded and saw no test events; init plus events means it works. The diagnostic has a branch for each and, where the cause is genuinely unresolved, names both possibilities instead of picking one. Also passes the reporter as a file:// URL via pathToFileURL. Node documents the --test-reporter value as an import()-style specifier, and a bare absolute path is not a portable one — notably on Windows. That is a correctness fix whichever hypothesis holds, and it is a live candidate for the first. FIXED_OVERHEAD is derived by reducing over the actual argv strings, so the longer URL is accounted automatically. Verified by execution, not assumption: composing the reporter against an EMPTY event stream writes exactly one line, the init marker. That is the whole point of the marker, so it is pinned by a test rather than left to inspection. T1 stays red and untouched. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
5504724d7a |
fix(#4012): the reporter body must return nully, not an iterable
Fourth failure on this feature, and this one was caused by the previous fix's lint workaround. TypeError [ERR_INVALID_RETURN_VALUE]: Expected nully to be returned from the "body" function but got an instance of Array. When stream.compose is given an async FUNCTION as the body, that function must return nully. The reporter ended with 'return []' under a comment asserting Node "still requires the exported function to return an iterable" — exactly backwards, and the direct cause of 41 failures across every run-tests.cjs invocation. That return existed only to dodge ESLint's require-yield after the previous commit converted async function* to async function. A lint workaround became a runtime crash, and the comment written to justify it stated the opposite of the contract. Both are now corrected to what the runtime actually does. Verified by EXECUTION rather than by reading: composing the real reporter against a fake event stream completes with no error and leaves both handled events durably on disk, with the ignored event type skipped. The failing form was reproduced the same way first, so the diagnosis is not inferred from the stack trace alone. The new unit test requires the reporter directly and asserts the returned value is nully — the assertion that would have caught this before it reached the runner — plus the exact NDJSON written. No subprocess, so this half of the feature is verifiable without a full runner pass, which matters because every defect in this feature so far has only been observable remotely. T1 remains untouched and red. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
7f2af28639 |
fix(#4012): the reporter sink must be a regular file, not devNull
Third failure on this feature, and this one broke everything rather than just the diagnostic: 43 failures across every run-tests.cjs invocation. Error: EINVAL: invalid argument, fsync Emitted 'error' event on WriteStream instance Node opens a WriteStream for a --test-reporter-destination and FSYNCS it on close. fsync on /dev/null is EINVAL — it is a character device, not a regular file. So os.devNull is not a usable reporter destination at all, and every chunk crashed on exit. The sink only ever needed to be a regular file that stays empty, since the reporter writes its real output through appendFileSync to the path in GSD_RUN_TESTS_EVENTS_FILE. It is now one fixed file inside the existing events dir, pre-created rather than relying on the stream's create-on-open, and left empty by design. One sink for the whole run, not per chunk, so its path length stays constant — reporterOverhead feeds FIXED_OVERHEAD, which is computed once before chunking, and a variable-length path would silently mis-account the Windows argv ceiling. The comment that named devNull now says why the destination must be a regular file, so this does not get re-optimized back into the same crash. Reaching for devNull was the mistake: it looks like the obviously correct way to discard output, and it is, for a pipe or an fd — but not for something Node is going to fsync. Each of the three failures on this feature was a different edge of the same assumption, that a reporter destination behaves like ordinary output. The regression test asserts the closest externally observable consequence — a normal run must not surface the EINVAL/fsync text. The argv construction lives inside main() with no exported seam, and the sink is swept before a test could stat it; adding a seam purely to assert that is left out rather than reshaping production code for the test. Stated plainly rather than implied. The failing T1 is untouched and still red. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
0abd137ec7 |
fix(#4012): the events reporter has to survive SIGKILL
The remote run proved the instrumentation did not work. Timing landed — "chunk 1/1 was killed after 2006ms" — but the in-flight-file naming produced nothing and fell through to the pre-existing generic message. The feature I wrote to diagnose a kill was itself destroyed by the kill. Root cause, confirmed rather than assumed. The reporter yielded strings, which node pipes into the --test-reporter-destination WriteStream. That stream BUFFERS. execFileSync's timeout sends SIGKILL, which is uncatchable and gives nothing a chance to flush, so the events sat in a buffer that died with the child. The parent's own timer reported correctly because it lives in the parent — which is exactly why half the feature looked fine. The reporter now writes each event with fs.appendFileSync, unbuffered and durable at the moment it happens, to a path passed through GSD_RUN_TESTS_EVENTS_FILE. Env vars do not count toward the Windows 32,767-char argv ceiling, so moving the path out of argv also REDUCES FIXED_OVERHEAD; the accounting moved with it rather than being left stale. The destination is now a fixed devNull sink that stays empty by design. Silence was the reason this was invisible for a whole run. Failing to read the events file now says so explicitly, and distinguishes a file that could not be read at all from one that exists but is empty — the generic fallback firing quietly is what let a broken feature look like a working one. A write is unbuffered but not atomic, so a kill can still interleave a partial line; the reader tolerates exactly one unparsable trailing line and reports the complete ones before it. The failing T1 was left red and untouched rather than weakened to pass. Three new unit tests cover the reader directly, with no subprocess, so the parsing half is verifiable without a full runner pass: missing file, existing-but-empty file, and a truncated final line. Also adds ndjson-reporter.cjs to GSD_SCRIPTS_LIB_FILES in bin/install.js — scripts/lib/ ships, and omitting it meant the file would install everywhere and orphan on uninstall. That single omission caused 4 of the 7 remote failures. Verification runs on the remote runner. Refs #4012 |
||
|
sim
|
8497833a15 |
fix(#4012): a killed chunk now names the file that was hanging
The per-chunk timeout fired correctly but reported almost nothing, so every diagnosis cost a CI round-trip. run-tests.cjs logged chunk START only — no timestamp, no duration, no end line — then on a kill printed all ~55 basenames and asked the operator to work out whether output kept flowing (slow) or stopped early (hang). It could not name the in-flight file because the child is spawned with stdio inherit, deliberately, per #3597/#1051. Three additions. Per-chunk elapsed timing on every path, not just failures, so drift toward the cap is visible before it becomes a kill. Every timing number in the investigation behind this had to be reconstructed by hand from GitHub log timestamps. A second, machine-readable reporter running ALONGSIDE the human one, writing NDJSON to its own file. On a kill that file is read back and the files with a test:start and no matching completion are named, with the staleness of the last event, so "stopped 480s ago at X" reads differently from "still emitting at kill". stdio stays inherit and nothing is piped or tee'd — the maxBuffer and live-output risks that shaped the original design are untouched. Ranking of the killed chunk's files by known weight, flagging any absent from tests/test-timings.json, since an unweighted file is an unknown quantity. Two details that are correct rather than lucky. Passing --test-reporter at all replaces node's implicit default, so the human reporter is now named explicitly and reproduces node's own selection (spec on a TTY, tap otherwise) — visible output is unchanged. And the destination path's chunk index is zero-padded to a fixed width because FIXED_OVERHEAD is computed ONCE before chunking; a variable-length path would have silently mis-accounted the Windows 32,767-char argv ceiling and reintroduced #3597. The reporter flags are added to FIXED_OVERHEAD exactly as --test-force-exit is. The multi-reporter pairing and the stream.compose reporter contract were confirmed against Node's v24 documentation, not recalled — the first draft carried them as an unverified assumption and said so. Also corrects a stale comment claiming the 600s cap sits "below the 20m job cap". The lane is sharded 3x at timeout-minutes: 45; the windows shards were at 19m when chunk 1/5 was killed on |
||
|
Tom Boucher
|
83273f9642 |
fix(#3798): the profile closure follows command references into workflow spawn surfaces (#4009)
* test(#3798): tiered profiles must install the agents their workflows spawn * fix(#3798): the profile closure follows command references into workflow spawn surfaces * chore(#3798): changeset fragment (pr number backfilled after PR creation) * chore(#3798): backfill changeset PR number (4009) --------- Co-authored-by: sim <sim@local> |
||
|
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> |
||
|
Tom Boucher
|
ab69b9ce56 |
enhance(#3987): guard slug re-derivation and the swallowed-precondition shape — §8.5 was guardable after all (#3999)
* feat(#3987): guard slug re-derivation, and record why the swallow shape cannot be guarded Epic #3473's Decision 1 requires the wrong call site be UNREPRESENTABLE. #3984 measured that two of the nine §8 rules had no guard at all and recorded both as "Shipped - test-covered". This closes one of them, proves the other cannot be closed the same way, and corrects two false claims I merged yesterday. 1. §8.3 - scripts/lint-slug-derivation-drift.cjs. generateSlugInternal (src/core-utils.cts) is the canonical owner; #3883 removed 11 inline copies. Nothing prevented a twelfth: no slug guard existed in scripts/ or eslint-rules/. The detector is STATEMENT-scoped and matches the shape the real copies took - one statement carrying BOTH .replace(<negated class>, '-') and .replace(/^-+|-+$/, ''). Statement scoping is what buys the precision: the loose LINE-level form yields 18 hits with 7 unrelated, a material false-positive rate. Measured on the tree: 5 flags, 2 TRUE, 3 SANCTIONED, 0 FALSE. The three sanctioned sites are allowlisted with a reason each, following lint-phase-enumeration-drift's form rather than a bare denylist. The owner itself is listed explicitly even though it escapes by construction - an implicit escape is a latent bug, and the next person to touch line 192 would not know the guard depended on it. 2. Both TRUE positives were live defects, not style. scripts/qa-smell-ratchet.cjs reproduced the canonical formula including the 60-cap but trimmed BEFORE truncating - the #2849 bug - and never transliterated. The divergence is total, not cosmetic: canonical "privet-mir-privet-mir-privet-mir-privet-mir-privet-mir-prive" inline "tail" Cyrillic collapsed to nothing and only the ASCII remainder survived, so the ratchet was keying on wrong identifiers for any non-ASCII input. tests/planning-inspect.test.cjs carried a helper whose comment claimed parity with getPhaseDirFromPhaseId. That function now transliterates; the helper did not, so the test asserted against a stale formula while looking correct. Both now route through the seam. 3. §8.5 - measured, and deliberately NOT shipped. A candidate detector (swallowing catch + errno-retry-set test in the same function) gives 26 flags across 11 functions: 0 TRUE, 26 FALSE. Every one is best-effort unlink/rm/close cleanup, lost-rename-race backoff, or a deliberate null fallback. The file-scoped variant is worse at 71. Worse than the noise: the only known true instance was removed by #3885, so there is NO POSITIVE CONTROL - the guard cannot be shown capable of failing, which this repo requires of every drift guard. Shipping it would add a guard nobody can trust and nobody can test. The ADR now records the measurement and the reason, keeps §8.5 at "Shipped - test-covered", and points at the #1884 regression test as what actually enforces it. An honest "not detectable at acceptable precision" beats a guard that only ever passes. 4. Two claims I merged into the ADR yesterday were wrong. §8.9 said 17 of 19 subsumed children have a test citing their issue number, and that #3364 and #3812 have none. Both halves are false, and the claim came from a NUMBER-GREP - inside an amendment whose own subject is that a text match is not a fact. #3364 IS cited: tests/runtime-marker-resolution.test.cjs:107, T3 installMarkerResolvesWhenEnvAndConfigAbsent_3897 (#3364), asserting at :115-119. #3812 IS covered: tests/gen-state-md-docs.test.cjs:374, asserting at :382. Corrected to 19 of 19. #3812 does carry a real finding, though a different one: it is PARTIALLY DELIVERED on a CLOSED issue. The shipped fix declares cardinality for frontmatter keys, but #3812's stated acceptance was about the ## Current Position BODY section, and docs/reference/state-md.md:196-208 still has no normative single-valued/overwrite sentence and no pointer to ## Performance Metrics for history. Recorded in the ADR and left for #3812 to re-open - fixing it here would bury a scope question inside an unrelated PR. Note on B6: this ADDS a guard, and B6 said the net count must fall. #3951 already amended that clause - a guard ledger is a claim about COVERAGE, not count - which is what makes adding this one honest rather than contradictory. Verified: the guard flags 0 on the fixed tree, and PROVES IT CAN FAIL - a fresh inline copy planted in src/ makes it exit 1 naming the exact statement. All three sanctioned sites were confirmed exempt BY the allowlist, not by accident of the pattern, by re-attributing each to a non-exempt path and watching it flag. build:lib, lint and lint:ci all exit 0. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3987): add the changeset fragment Doc-only, so it carries forward from the verified sha rather than costing a second matrix run. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): §8.5 IS guardable — I was wrong, and the guard found a live defect Two orthogonal reviews. The correctness review overturned my central judgment, and it was right. 1. I concluded §8.5 was "not detectable at acceptable precision" and recorded that in the ADR. False. My evidence was 26 flags / 0 TRUE / 26 FALSE. The reviewer pointed out what I had not: all 26 false positives are CLEANUP verbs - rmSync 54, unlinkSync 43, closeSync 17, chmodSync 12 - and the obvious narrower predicate was never tried. A swallowed cleanup is legitimate best-effort. A swallowed CREATION is a precondition silently lost, which is exactly the #1884 shape. Measured properly, in three stages: swallowing catch 911 + try-block calls a CREATION verb 24 + enclosing function references a *_ERRNOS set 0 0 flags, 0 false positives. The `*_ERRNOS` naming key is empirically total - all 10 retry/tolerate sets in src/ follow it. My second claim was worse. I wrote that no positive control exists because #3885 removed the only true instance, so the guard "cannot be shown capable of failing". That is self-refuting: this very PR's slug guard proves-it-can-fail on a synthetic tree, and the pre-#3885 blob is available as exactly such a fixture. It is now the control, and it works in both directions - the rule flags 0c43d853e^:src/planning-workspace.cts at line 210, the line the fix commit's own message cites, and reports zero on the post-fix code. I stopped at the first negative result on the option that meant less work. Shipped as eslint-rules/no-swallowed-precondition.cjs, wired into the existing src/**/*.cts ESLint block rather than a scripts/lint-*-drift.cjs: no script in scripts/ requires typescript/espree/acorn, and scripts/ ships to consumers, so a .cts-parsing standalone guard would add a devDep at consumer runtime. The ESLint block already parses .cts for free. 2. The guard immediately found a live defect of the same class. src/capability-lock.cts swallowed a mkdirSync on the lock directory, then acquireLock classified the follow-on failure as `code !== 'EEXIST' → return null`. A real EACCES/EROFS makes openSync(lockPath,'wx') fail ENOENT, which is not EEXIST - so a fatal filesystem error was laundered into "lock unavailable". Same defect as #1884, different laundering target. Fixed the way #3885 fixed #1884: the creation failure propagates. Regression test proven fail-first by hand - with the fix stashed, EACCES was laundered to null; restored, it throws. The strict rule does NOT catch this shape (its errno classification is an inline literal, not a named set). The rule is deliberately left strict: the broadened form had 2 false positives - capability-lock.cts:408, the deliberate EEXIST steal protocol, and commonjs-marker.cts:131, which returns a distinct documented outcome. The gap is noted in code rather than papered over with a noisy predicate. 3. The security review found the slug guard's exemption FAILED OPEN. currentFunction was never reset, and only a column-0 `function` declaration updated it, so exemption bled from an allowlisted declaration to the next one. generateSlugInternal exempted 50 lines for an 11-line function. A re-derivation planted anywhere in that window was silently exempt - the same fail-open shape that produced a blocker in #3897, and an allowlist is a SUBTRACTION so a mismatch fails open by construction. Extent is now tracked by real brace depth, and a test plants a violation after each allowlisted function's real closing brace and asserts it IS flagged. 4. Also from the security review: the guard was a CI-DoS and narrower than I claimed. Its unbounded [^\]]* was re-scanned from every `.replace(/[^` start: 54.3s on a 1.28MB line. It imported MAX_REGEX_LITERAL_LEN and never called readRegexLiteralAt - the bounded tokenizer that exists for exactly this. Now routed through it with a 2MB file cap: ~200ms. 15 of 25 genuine re-derivations evaded. Widened to catch replaceAll, {1,}, \s*-wrapped classes, escaped ], literal new RegExp(...), five trim spellings, .split().join(), and multi-line .replace( args - still 0 false positives. Two forms still evade and are documented as deliberate gaps with negative tests: the two-statement/temp-var form and new RegExp built from a variable. Both need data flow, and guessing at it is how a guard becomes noisy. Also fixed: // inside a string truncated the line, a ; inside the collapse regex split the statement (a one-character bypass), and SCAN_EXT omitted .mjs/.tsx/.jsx. 5. A regression I introduced, caught by the same review. qa-smell-ratchet.cjs top-level-required a build output that is not git-tracked, so the script hard-failed MODULE_NOT_FOUND before build:lib - including for --help, which previously had no build dependency. The require is now lazy at the point of use. 6. Four of my own tests were vacuous or weak. T9's input yielded an identical string under the buggy formula, so it passed on the implementation it was meant to catch. T12 compared maxLen null vs 60 on an 18-char name, where they agree trivially. T9-T12 all asserted generateSlugInternal directly, so they would pass unchanged if both call-site fixes were reverted. And prove-it-can-fail was scoped to scanRepo, never the CLI - dropping main()'s exit-code line would have kept every row green. All rewritten with discriminating inputs, per-call-site rows that red when the fix is reverted, 59/60/61 boundaries, an entirely-non-alphanumeric row, and a CLI row asserting the real subprocess exit code and both sanitizeForReport sites. Verified: both guards flag 0 on the tree and both prove they can fail. The swallow rule's control is confirmed in both directions - pre-#1884 shape flagged, post-#3885 shape clean. build:lib, lint and lint:ci all exit 0. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3987): record that §8.5 IS guardable, and correct a correction that made a ledger worse Three ADR corrections, two of them to text this branch wrote hours ago. §8.5 advances to Enforced. Its previous entry said the rule was not detectable at acceptable precision. That was wrong twice: the 26 false positives were uniformly CLEANUP verbs, which is a reason to narrow the predicate rather than abandon it, and the claim that no positive control exists was self-refuting - the pre-#3885 blob is available as a fixture and this repo's own guards prove-it-can- fail on synthetic trees. Narrowed to creation verbs plus a *_ERRNOS reference: 911 -> 24 -> 0 flags, 0 false positives, control confirmed in both directions. The entry keeps the wrong reasoning visible, because a high false-positive count being evidence the predicate is wrong - not evidence the rule is unguardable - is the transferable part, and the first negative result is most seductive when it is also the answer that means less work. §8.9's correction is itself corrected. The original 17-of-19 claim was CORRECT for the predicate it stated; this branch silently swapped cited -> covered and declared 19 of 19. #3812 appears in zero test files. Changing what a word means to make a ledger read better is a worse failure than the miscount it claimed to repair. Both predicates are now reported separately - 18 of 19 cited, 19 of 19 covered - because §8.9 asks for a test NAMING each child, so 18 is the number that answers it. #3812 is also re-opened for real, rather than the first draft's promise that it could be. §8.3 stays Shipped - test-covered rather than advancing. The slug guard catches the copy-paste class and a dozen variants, but two forms still evade by decision (temp-var split, new RegExp from a variable) because both need data flow. Naming them keeps the status honest: the wrong call site is much harder to write, not unrepresentable. Closes #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3987): backfill changeset pr number Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): replace my own wall-clock assertion, and close the guard that let me write it CI went red on ubuntu shard 2/3. The failing test was mine, and the failure was the test, not the code. a 1.28MB line ... scans in well under a second (was 54.3s pre-fix) 7368ms It asserted ELAPSED TIME. ~200ms locally, 7.4s on a shared CI runner. The bound introduced for the MAJOR-2 DoS fix works - 7.4s against a 54.3s pre-fix baseline is the fix doing its job - but an absolute wall-clock threshold on shared hardware is a race, not an assertion. CLAUDE.md says so directly: "Clock Seams: Do not assert on wall-clock time." I wrote the anti-pattern the project bans, in a PR about guards. Raising the threshold would only move the flake. The row now asserts a DETERMINISTIC bound instead: an instrumentation seam on drift-scan.cjs counts readRegexLiteralAt calls and characters examined, and the test asserts charsExamined stays under an absolute ceiling. Measured on the same 1.28MB fixture: 120,000 calls, 48,000,000 chars - two orders under the ceiling. The pathological fixture is kept; only the thing being asserted changed. Proven to still discriminate: with MAX_REGEX_LITERAL_LEN raised to simulate the unbounded pre-fix behavior, the same fixture does not complete in 120 seconds, versus ~0.3s bounded. It is a real regression test, not a tautology. Then the second half, which is the same defect class as the rest of this PR. eslint-rules/no-elapsed-assertion.cjs matched only the EXACT identifiers ^(elapsed|duration|took|ms)$. I used `elapsedMs`. It evaded the rule entirely. tookMs, durationMs, elapsedTime and msElapsed evade the same way. A guard that cannot see the violation it exists to catch is exactly what this PR is about - it just happened to be an existing rule rather than one of the two I came here for, and it was found because I committed the violation it should have blocked. Widened to /^(?:elapsed|duration|took|ms)(?:[A-Z]\w*)?$/ plus a narrow start/endMs delta pair. Deliberately NOT a blanket *Ms suffix: a first draft did that and produced 2 false positives on `timeoutMs` in plan-phase-stall-detection, which is a configured timeout and not a measurement. Verified negative on params, items, forms, terms, dirnames, timeoutMs, cacheTtlMs and staleAfterMs. Measured over the five files carrying camelCase timing identifiers: 0 true positives beyond my own, so nothing else needed rewriting. The rule's own test file gains a row asserting `elapsedMs` flags, proven to fail against the pre-widening rule - the same prove-it-can-fail standard both new guards in this PR are held to. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): a comment I added leaked a Claude reference into every runtime install The runner went red with 4 failures in tests/install.test.cjs: Leaking: .hermes/scripts/lib/drift-scan.cjs Leaking: .qwen/scripts/lib/drift-scan.cjs The instrumentation seam added for the deterministic bound carried a comment naming CLAUDE.md as the source of the no-wall-clock-assertions rule. scripts/ SHIPS to consumers, so that comment was installed verbatim into hermes and qwen trees, and the install suite scans for exactly this - a Claude-specific reference reaching a non-Claude runtime. The rule is real and worth citing; the filename is not portable. The comment now says "this repo's test rules" and states the rule inline, which is what a reader of an installed tree actually needs anyway. Worth noting what caught it: not lint, and not the two guards this PR adds - the install suite's full-tree scan, which exists precisely because a shipped file is read by runtimes that have never heard of CLAUDE.md. Same lesson as the rest of this PR from the other direction: the check that matters is the one that can see the surface where the defect actually lands. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): a test fixture swallowed 46 git exit codes and produced a silent false negative CI red on ubuntu shard 3/3: tests/health-validation.test.cjs:2029 expected exactly one W024, got [{"code":"W006", ...}] Not caused by this branch, and the evidence is decisive rather than a hunch: the SIBLING test at :2039 builds the IDENTICAL fixture with the identical commitsAhead and asserts the same thing, and it PASSED in the same process, same file, same run. Same input, both outcomes - which rules out logic, ordering, sharding and environment, and leaves a per-invocation nondeterministic failure inside one fixture build. The mechanism is an unchecked exit code, 46 times over. The W024 fixture performs ~46 runGit spawns and never checks a single one. runGit returns failures as DATA and never throws, so one silently-failed `git commit` yields 19 commits instead of 20, or a silently-empty `git rev-parse HEAD` yields a blank state_head. Either drops readStateHeadFreshness below the advisory threshold, W024 never fires, and only W006 remains. Reproduced exactly: 20 commits -> ["W006","W024"]; 19 -> ["W006"]; blank state_head -> ["W006"] - byte-identical to the CI assertion dump. The arithmetic is what hid it. At threshold-1 and threshold+1 a lost commit still produces the asserted answer; only the exactly-at-threshold cases sit one commit from a false negative. Two of the seven tests are in that position, and CI hit one. That is why it had never been seen before, and why it surfaced now: this branch adds three test files, which reshuffles the cost-weighted shard partition and moved this file into a chunk where the latent flake fired. My files were checked as suspects first and cleared: all fixtures mkdtemp-unique, no process.chdir, no .planning/ writes, no git spawns, and node --test gives per-file process isolation regardless. Fixed at the cause, not the symptom. A mustGit wrapper throws on a non-zero exit with the command, exit code and stderr, and all nine call sites route through it. The fixture now asserts its OWN preconditions before the assertion under test runs - the seed head is non-empty, and `git rev-list --count <seed>..HEAD` equals the requested commitsAhead - so a fixture that did not build what it claims fails loudly as a FIXTURE ERROR naming got-versus-asked, instead of quietly handing a weaker input to the assertion. Proven: dropping one commit now raises FIXTURE ERROR: requested commitsAhead=19 but git rev-list --count reports 18 where it previously produced a silent ["W006"] pass-for-the-wrong-reason. 64/64 tests in that block pass unperturbed. Deliberately NOT done: no threshold change, no retry, no loosened assertion, no skip. The assertion was correct; the input was silently wrong. Worth naming, because it is the same shape from the other side: this PR ships eslint-rules/no-swallowed-precondition.cjs, whose entire subject is a swallowed precondition failure being laundered into a plausible downstream outcome. This fixture is that defect in test code - the swallowed git failure was laundered into a legitimate-looking "W024 did not fire". The rule does not cover test fixtures, so the connection is noted at the fix site rather than enforced. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): two tests wrote to committed files; the shard packing decided when that mattered CI red on windows-latest shard 1/3 only: "gen-exit-code-registry: CLI" > "a --write run redirected to a tmpdir leaves every committed artifact untouched" AssertionError: hooks artifact must be untouched The Linux runner passed the same sha at 40425/40425. It is Linux-only, so a Windows-scheduling defect is structurally invisible to it. Root cause, established by measurement rather than inference. tests/cli-exit.test.cjs appended a corruption marker to the REAL COMMITTED hooks/lib/exit-code-registry.js, held it corrupted across a full subprocess, and restored it in a finally. tests/exit-code-registry.test.cjs reads that same real file before and after its own subprocess and asserts byte equality. If it samples while the other test holds the file corrupted, it fails. The landmine is pre-existing, from |
||
|
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> |
||
|
Tom Boucher
|
12f9d1d9a0 |
enhance(#3913): docs, and the guards come down (#3994)
ADR-3889 terminal phase. Generated docs/reference/exit-codes.md from the exit-code declaration with a --check drift arm; deleted the inert soft-error-exit-zero oracle; promoted untyped-success from SMELL to VIOLATION so it can fail a build; pruned all 5 smell-baseline entries. Fixed inline: two mis-scoped oracles (routing-validity, value-hygiene), a second source behind the band table, unescaped declaration strings reaching Markdown, and a pre-existing Windows 8.3 short-name path-comparison defect. Guard ledger corrected from a claimed net -4 to a measured net -1. Closes #3913 |
||
|
Tom Boucher
|
d24e22b156 |
enhance(#3912): gsd-tools declares outcomes, pinned at v1 (#3983)
* enhance(#3912): gsd-tools declares outcomes, pinned at v1 ADR-3889 §4. Phase 6 already moved error()'s terminator onto the seam, so what remained was the declaration — and the pin that makes it invisible today. The census corrected two documented figures before any code changed. ERROR_REASON has exactly 25 members (the ADR and epic were right; an earlier note of mine claiming 23 was wrong and is corrected). And output({error}) is **64 sites across 9 files, not the 60 ADR-2980 ratified** — the module shape holds but the total drifted +4: frontmatter 7 not 6, phase 4 not 2, roadmap 3 not 2. That matters because this phase's criterion demands the pin be asserted over the enumerated population rather than sampled; asserting over a stale 60 would leave four sites unpinned while claiming full coverage, which is the shape of failure this epic exists to remove. The issue does not state the fact that shapes the design: output() never touches the exit code. Confirmed by reading it — it writes fd 1 and returns. So a declared outcome for those 64 sites had nowhere to be READ. The mapping was never the work; wiring somewhere for the declaration to land was. The seam already existed twice over. cli-exit.cts holds two globalThis-Symbol cells, each because the module is emitted to three locations and a module-level `let` would let instances disagree, and runMain already maps a code returned by main(). A third cell inherits that solution. output() records DEGRADED for any {error} payload — key-order agnostic, which is exactly why the "42 sites" figure undercounts — and runMain projects the cell only when main() returns nothing, so an explicit return still wins. error() maps its reason through a table over the closed 25-member enum, leaving all 278 call sites untouched; 226 of them pass no reason at all. The version gate lives in error(), NOT in projectOutcome: registered names are version-invariant there, so mapping a reason straight through would make USAGE project to 64 under v1 and break the pin on its first line. projectOutcome is left exactly as Phase 2 shipped it, DEGRADED's 0/80 asymmetry included. Proven rather than asserted. v1 is byte-identical across three real CLI paths — config-get plain, config-get --json-errors, and an output({error}) path — matching exit code and exact bytes against the pre-change build. Under GSD_EXIT_CONTRACT=v2 the same commands now exit 66 (CONFIG_KEY_NOT_FOUND -> NO_INPUT) and 80 (DEGRADED), both looked up through the registry. An anti-vacuity test pins that v1 and v2 genuinely differ for at least one reason, because without it a mapping where everything projects to 1 under both versions would satisfy every other assertion and the declaration would be theatre. A1 iterates all 25 enum members and A3 asserts over the measured 64-site population, so a 26th reason or a 65th site fails until it is given a mapping — the drift guard this phase needs, given ADR-2980's own count had drifted +4 unnoticed. Verification runs on the remote runner. Refs #3912 * fix(#3912): the outcome cell must never lower an exit code The remote run caught a fail-open that this phase introduced, in the phase whose entire purpose is removing fail-opens. `state validate --strict` on a missing STATE.md exited **0** where it must exit 1. Mechanism: `runMain` projected the pending outcome whenever `main()` returned void, and under v1 DEGRADED projects to 0 — so a `process.exitCode` already set non-zero by the command was clobbered down to success. Confirmed live against a fixture, before and after. This refutes a review conclusion recorded earlier in this phase, that the cell was "fail-closed and can never mask a failure as success". It could, and did. Recording that plainly so the assumption is not repeated: the cell's danger was never only that it might add a failure — it was that projecting it unconditionally overwrites whatever decision came before. Projection is now guarded: it may set a code only when none is set, and an already-non-zero exit code always wins. The full precedence — explicit `main()` return, then an existing non-zero exitCode, then the declared outcome — is written at the projection site. A regression test drives a void return with a pre-set non-zero code and a pending DEGRADED, and fails against the pre-fix build. The second failure was my test encoding the wrong contract, not a code defect. It asserted `output({found:false, error: undefined})` records DEGRADED because the KEY is present. `JSON.stringify` drops undefined, so the payload the user receives is `{"found":false}` — carrying no error at all, and calling that degraded would hand back exit 80 under v2 for output that reads as clean. The discriminator is a serializable error VALUE, not key presence. The test now pins `{error: undefined}` as explicitly NOT degraded, and the design doc's wording is tightened to match. Verification runs on the remote runner. Refs #3912 * docs(#3912): the versioned exit contract, and a flag defect the docs found Diataxis pass for Phase 8, plus a real fix that only surfaced because writing the how-to meant running its own examples. The docs. ADR-2980's "Revisit if" clause asked for exactly the versioned projection this phase provides, so it gets an amendment naming #3912 / ADR-3889 section 4 as that boundary: v1 stays 0 byte-for-byte, v2 projects DEGRADED to 80. The amendment also records the count drift rather than restating a stale figure — the ADR ratified 60 output({error}) sites in 9 modules; the AST-measured population is 64 across the same 9 (frontmatter 7 not 6, phase 4 not 2, roadmap 3 not 2). The pin is asserted over the enumerated 64. json-errors.md gains the outcome-declaration reference, including the precedence order a review pass got wrong and the suite refuted: an explicit main() return, then an already-set non-zero process.exitCode, then the declared outcome. Projection may only ever set a code, never lower one. A how-to is owed here and is written, not skipped. Under v1 nothing changes, so the audience is an operator opting into v2 and needing to know what the codes mean for a CI gate — a migration, which is how-to shaped. It covers turning v2 on, the code table, why 80 is "ran and reported a condition" rather than a crash, and how to split a gate that treats any non-zero as fatal. No tutorial: there is no new entry point to learn, and under the default contract a reader would be walked through observing nothing. The defect. Running the how-to's own Step 1 example returned $ gsd-tools --exit-contract=v2 state validate --strict Error: Unknown command: --exit-contract=v2 (exit 64) while the same flag trailing the subcommand worked and exited 80. The flag half-worked, by argv position. resolveContractVersion scans argv non-destructively, so the token survived into the dispatcher, which treats argv[2] as the command name. --json-errors had already solved precisely this at gsd-tools.cjs:4455, under a comment naming the hazard verbatim: "The argv splice must happen here too, otherwise the dispatcher below sees --json-errors as an unknown command." The later flag never got the same treatment. Fixed rather than documented around: the version is resolved first — which memoizes the cell and makes an invalid value throw early — and then every --exit-contract= token is spliced out of the dispatcher's argv copy. --exit-contract is now listed in TOP_LEVEL_USAGE, where it never was. The regression test pins leading position, trailing position, agreement between the two, and a loud failure on v3 rather than a silent fall back to v1. Neither review engine would have caught this: the defect is invisible in the diff, because the diff does not touch argv handling. It surfaced only from running the documentation's own example. Writing a how-to is an execution pass. Verification runs on the remote runner. Refs #3912 * fix(#3912): the flag splice has to run before the run-with-timeout return An isolated review of the previous commit found that the fix did not deliver what it claimed, and that two of its own tests were weak. All three findings reproduced by execution before any change was made. The fix was placed below a return. main() intercepts `run-with-timeout` at gsd-tools.cjs:4436 and returns from there — above both the --json-errors block and the --exit-contract splice added in the previous commit. So the flag still died in leading position for that one command: $ gsd-tools --exit-contract=v2 run-with-timeout 5 -- node -e "..." Error: Unknown command: run-with-timeout (exit 64, child never ran) The previous commit message and the test's describe-block both claimed position-independence unconditionally. That was an overclaim, not a gap left open, and it is the part worth naming: the fix was verified by hand on the commands I happened to think of, and `run-with-timeout` returns before the code I was verifying. Both global-flag blocks now run above the interception, with a comment naming it so a later edit cannot slide them back down. Moving --json-errors up fixes the identical pre-existing bug for that flag, verified failing beforehand (exit 1, sdk_unknown_command). Fixing the sibling is deliberate: same defect, same block, and a known-broken twin next to a fixed one is not a resting state. Two tests were not pulling their weight. The invalid-value test was vacuous — it passed against the pre-fix build, because `--exit-contract=v3` already exited 1 there and already printed the resolve error lazily through error() -> getContractVersion. Both its assertions held before the fix, so it pinned nothing. The real discriminator is that the pre-fix build emits BOTH "Unknown command: --exit-contract=v3" and the resolve error, while the fixed build emits only the latter; the test now asserts that absence. The leading-position and leading==trailing tests asserted proxies — "not 64", "no Unknown command", "the two agree" — none of which pin a value, and all of which would survive both positions being identically broken. With a .planning directory and no STATE.md, state-snapshot exits exactly 80 under v2 and 0 under v1 in both positions. Those numbers are pinned now. The multi-token case the descending splice loop exists for is covered too, and run-with-timeout has regression tests for both flags. The lesson is narrower than "test more". Hand-verifying the production behavior does not verify that the test would have caught its absence. The pre-fix binary has to be run against the test's own assertions. Investigated and deliberately not changed: splicing before --cwd parsing degrades one diagnostic from "Missing value for --cwd" to "Invalid --cwd: <path>", but that is pre-existing — verified on the pre-fix build via --json-errors, which already did it. This change joins the pattern rather than creating it, and both forms exit 64 on malformed input either way. Verification runs on the remote runner. Refs #3912 * chore(#3912): backfill changeset pr numbers to 3983 * test(#3912): pin the reason-table invariant as set equality, not a count A graph-backed review flagged the unchecked lookup in expectedErrorCode3912. Investigated by execution: the drift guard DOES hold — for an unmapped reason under v2 the production error() yields 1 while the table yields undefined, so the assertion fails. Not a correctness defect, and deliberately NOT made tolerant, since a tolerant lookup would destroy the guard. Two real problems remained. The guard asserted the wrong invariant: it counted the TABLE's keys at 25 rather than checking they match the ENUM's values, so a renamed member keeps the count at 25 and slips past, and a 26th member leaves the table at 25 and slips past too. Both were then caught only indirectly, by an undefined mismatch producing 'must exit undefined'. It is now a sorted set equality, so the failure names the specific missing or extra reason. And the comment above it described a '?? FAIL' fallback that does not exist anywhere in the function. It now states what the code actually does, verified by running it rather than by reading it. Refs #3912 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
355c943b08 |
enhance(#3626): make CONTEXT.md seam claims checkable via a SEAM.*.enforced-by gate (#3975)
* feat(#3626): make CONTEXT.md seam claims checkable via SEAM.*.enforced-by gate Adds SEAM.<id>.owns / SEAM.<id>.enforced-by=lint-rule:<name>|test:<path> predicates to CONTEXT.md, generalizing the existing WORKTREE.SEAM.* shape, plus scripts/lint-seam-enforcement.cjs (wired into lint:ci) which fails when a declared single-owner seam names no existing, registered enforcement mechanism. Backs all six current module-level single-seam/ single-canonical-owner claims found in CONTEXT.md, including the Shell Command Projection Module's Windows-binary-resolution claim via #3619's local/no-private-binary-resolution rule. Scope is resolves-only per maintainer decision: the gate proves an enforcement pointer exists and is registered, not that its surface covers every file the seam claims. See docs/adr/3626-context-md-seam-claim-gate.md. Closes #3626 * fix(#3626): back the Package Identity Module's seam claim too Isolated adversarial review caught a miss in the "no grandfather list" sweep: the Package Identity Module also declares itself "Single seam owning GSD's published-package coordinates" and already names its real enforcement (scripts/lint-package-identity-drift.cjs). Backs it with SEAM.package-identity.owns/enforced-by=test:tests/package-identity.test.cjs, bringing the total to 7 backed seams. Also makes explicit, in the design doc and ADR, that function-level "single owner" sentences inside already-covered modules (STATE.md Document Module, etc.) are deliberately out of scope — a seam claim is about a module's boundary, not every function inside it. * docs(#3626): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2ea5efc151 |
enhance(#3911): hooks declare their crash policy (#3960)
* enhance(#3911): give hooks an exit seam that needs no build ADR-3889 Phase 7 foundation. The 19 shipped enforcement hooks hold 91 of the epic's 128 terminators and cannot reach `terminateNow` today. The obvious route — requiring `gsd-core/bin/lib/cli-exit.cjs`, as gsd-agent-isolation-guard.js already does for two other modules — is rejected. That precedent carries its own warning (#3582): those files are tsc output, gitignored and absent on a raw plugin-marketplace or git-clone install, so the hook must first call ensureRuntimeBuild() to self-heal. Making the module a hook needs IN ORDER TO TERMINATE depend on a build inverts the dependency, and its failure mode is precisely the fail-open this phase exists to remove: a guard that cannot terminate cannot deny. `lint-hooks-runtime-build-seam` already encodes that concern, and Design B would have had to add an ensureRuntimeBuild() call to all 19 hooks to satisfy it. So `hooks/lib/` becomes a third emit location for cli-exit and a fifth for the registry, preserving the invariant `src/cli-exit.cts`'s own header states: it imports nothing but node:fs and its sibling registry, and the generator dual-emits that sibling alongside each copy so a relative require resolves next to whichever copy loaded it. Shipping needed no change — build-hooks.js already declares HOOKS_SUBDIRS_TO_COPY = ['lib']. Proven, not asserted: the two files are copied into an otherwise-empty tmpdir and a child process requires them and terminates — PASS exits 0, HOOK_DENY exits 2 with the payload on both stdout and stderr. That test fails the moment the hooks copy gains a require reaching outside hooks/lib/. Also fixed inline: the registry's fifth target let any `--write` test overwrite the real committed hooks/lib/exit-code-registry.js, because the test helper derived only three of the other output paths. It now redirects all five, and a regression test asserts every committed artifact is byte-identical after a redirected write. Install-tree goldens pick up the two new shipped paths across 11 runtimes — insertions only, no removals. lint:ci was green while they were stale, so this was found by regenerating rather than by a gate. Verification runs on the remote runner. Refs #3911 * enhance(#3911): declare a crash policy, and migrate the write guard Adds `hooks/lib/hook-exit.js` — the hook-facing vocabulary over `terminateNow`, hand-written because the cli-exit copy beside it is generated: allow(payload) exit 0 deny(payload, stderr?) exit 2 crash(onCrash, payload) whichever the hook DECLARED `crash()` takes the policy as a required argument with no default, which is the whole mechanism: fail-open by accident stops being expressible. A hook must name ALLOW or DENY at the call site, and an unrecognized value terminates INTERNAL rather than guessing. Fail-open stays legal; fail-open by omission does not. `gsd-write-guard.js` is the first hook migrated, all 12 sites, and it exposed a gap in the seam. `terminateNow`'s doc comment justified its fd-2 write by citing this hook's `emitBlock` — but modeled it as sending the same bytes to both streams, when `emitBlock` actually sends full JSON to stdout and only the bare `reason` string to stderr, because Kimi's hook bus feeds stderr verbatim back to the model. Migrating as written would have turned a readable sentence into a JSON blob for Kimi-backed agents. #3911 requires both "all 19 hooks terminate through terminateNow" and "no hook's effective default changes". Those are jointly satisfiable only by teaching the seam to carry a distinct stderr payload, so `terminateNow` gains an optional third argument: omitted, behavior is byte-for-byte what it was; a string is written raw, which is exactly the Kimi case. The doc comment's inaccurate claim about emitBlock is corrected in place. Proven rather than asserted: the pre-migration file is reconstructed from HEAD and driven with the same catastrophic-shrink payload as the migrated one — exit code, stdout and stderr all byte-identical. Verification runs on the remote runner. Refs #3911 * enhance(#3911): all 19 hooks terminate through the seam Migrates the remaining 18 enforcement hooks onto allow/deny/crash. An AST walk now reports zero `process.exit(` call sites across every `hooks/*.js` — down from the 91 the census measured. Each hook with an outer catch declares its policy once, at module top, with the reason that policy is right for that specific guard: a read guard that cannot scan must not block the read; a statusline that renders every prompt must degrade rather than crash; an injection scanner must not retroactively block a result already returned. Those sentences are the deliverable — they are what turns fail-open-by-accident into fail-open-on-purpose. No hook's effective default changed. Wiring exposed two defects, both fixed here rather than noted. A SECOND stdout/stderr-splitting site turned up in `gsd-workflow-guard.js`'s `emitForceAddBlock`, matching the pattern already known from the write guard — full JSON to stdout, bare reason to stderr for the Kimi bus. It uses the `stderrPayload` argument added in the previous commit, which is now carrying its second real caller rather than one special case. More seriously, `terminateNow` emitted both streams inside ONE try, so a payload that failed to serialize aborted before the stderr write ever ran. The two windsurf guards write nothing to stdout on a block and only a reason string to stderr, so `deny(undefined, reason)` exited 2 with EMPTY stderr — a deny that silently loses its reason, which is the exact "fails with success" class this epic exists to close. The streams are now emitted independently, each with its own guard, and `undefined` means "nothing to write for this stream" rather than an error. Regression tests inject a throwing write on one fd and assert the other still receives its payload; they fail against the single-try version. Byte-identity was proven per hook, not assumed: each pre-change file is reconstructed from HEAD and driven side by side with the migrated one across its normal path, its deny path, malformed stdin and empty stdin — exit code, stdout and stderr compared. Verification runs on the remote runner. Refs #3911 * enhance(#3911): harden the three shell hooks, and pin every hook's policy `gsd-phase-boundary.sh`, `gsd-session-state.sh` and `gsd-validate-commit.sh` gain `set -euo pipefail`. The expected hazard did not materialize, and that is worth recording: every intentionally-non-zero command in all three is already the condition of an `if`/`elif`, which `set -e` never fires on, and none of them reads a possibly-unset variable or pipes through a grep that may legitimately match nothing. No `|| true` guards were needed. Each hook was still checked command-by-command before the flags went in rather than after. Twenty-one before/after cases across the three hooks — disabled and enabled, planning and non-planning, missing STATE.md, malformed JSON, the Kimi payload shape, quoted and unquoted `-m`, valid and over-long Conventional Commits — all match on exit code, stdout and stderr. The hardening is shown to actually fire, not merely added: with a stubbed `node` that fails at the JSON-emit step, phase-boundary and session-state go from silently exiting 0 with empty stdout to failing visibly with the error surfaced. No such case could be constructed for `gsd-validate-commit.sh`, whose every statement already sits inside an if-condition — recorded as unproven rather than claimed. `tests/hooks-crash-policy.test.cjs` adds the per-hook coverage the issue asks for, table-driven over all 19 hooks rather than 76 hand-written cases: normal allow, deny where a deny path exists, crash-honors-the-declared-policy, and an unclosed-stdin case — the one `process.exitCode` structurally cannot serve. The deny assertions encode each hook's ACTUAL stream split rather than a uniform shape, since four of the six deliberately differ. A drift guard enumerates `hooks/*.js` and fails if a terminating hook is ever added without a row. Writing those tests surfaced two hooks that emit a block decision in their JSON body and exit 0. Both were checked rather than assumed, and neither is a fails-with-success: `gsd-read-injection-scanner.js` is PostToolUse, where the tool has already run and exit 2 has no meaning, and `gsd-cursor-subagent-start.js` follows Cursor's JSON-body protocol. They are deliberately left alone — a mechanical sweep to `deny()` would have broken exactly these two. Verification runs on the remote runner. Refs #3911 * fix(#3838): the commit validator says when it could not validate #3911 claims to subsume #3838. Measurement said otherwise, so this closes it for real rather than by assertion. `set -euo pipefail`, added earlier on this branch, does NOT fix #3838: bash exempts a command used as an `if` condition from `set -e`, and all three of the hook's swallow-and-pass sites are exactly that shape. Verified against the hardened hook with a node shim that fails only the classifier call — a non-conforming commit still exited 0 with empty stdout AND empty stderr, indistinguishable from "your commit conforms". That is the defect verbatim. All three sites named in #3838 now capture the real exit status instead of consuming it as a condition, and each distinguishes its genuine negative from "could not run": - the classifier: 0 = is a git commit, 1 = genuinely not one, anything else = could not classify. Its `node -e` now wraps the require and the call in try/catch and exits 3 on a throw, so a broken require chain can never be mistaken for `isGitSubcommand` legitimately returning false — which is the arm that matters, since `token-scanner.cjs` is a gitignored build artifact and a fresh checkout lands there. - the opt-in config read and the JSON command extraction get the same treatment. On "could not run" the hook emits a diagnostic to stderr naming which check failed and why, then exits 0. The issue confirms this is safe — it is a PreToolUse hook, so stderr does not disturb the JSON protocol — and ranks it the smallest sufficient fix. The gate still fails open, but it can no longer do so silently, which is the whole complaint: a validator that disables itself quietly costs more than one that is absent, because it is trusted. Both controls are unchanged and pinned by tests: a conforming commit still passes silently, a non-conforming one still exits 2 with its existing block payload. The defect test asserts stderr is non-empty and names the failure; it fails against the pre-fix hook. Verification runs on the remote runner. Refs #3911, #3838 * docs(#3911): document the hook crash-policy contract Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it), INVENTORY rows for the three new hooks/lib files, and an ARCHITECTURE note on the hooks section. How-To: docs/how-to/declare-a-hook-crash-policy.md, indexed from docs/README.md — a hook author now has to choose and declare a crash policy, which is more than one step and crosses into which harness protocol their hook speaks. It covers allow/deny/crash, writing an ON_CRASH reason that is actually useful, when a deny needs a distinct stderr payload, the two hooks whose harness reads a JSON-body decision and must NOT use deny(), and what to do when a check cannot run at all — with #3838 as the worked example. Refs #3911 * test(#3911): prove the seam actually ships, and stop hand-rolling temp cleanup Two review findings. The acceptance criterion 'hooks/dist/** stays in parity via the build seam (lint:hooks-runtime-build-seam)' was misstated and unmet: that lint checks something else — that a hook requiring a compiled gsd-core/bin/lib module also calls ensureRuntimeBuild(). Nothing exercised that the three new hooks/lib files reach hooks/dist/lib at all. That gap is not theoretical: #770 is a recorded ship-blocking bug where a new hook never shipped because a copy list missed it. The suite now builds dist through the repo's own ensureBuiltHooks(), byte-compares each shipped copy against its source, and spawns a child that requires the SHIPPED dist copy and denies — which is what catches a copy that exists but cannot resolve its sibling registry. gsd-validate-commit.sh hand-duplicated mktemp/run/rm three times; one idempotent trap on EXIT replaces them, guarded so cleanup cannot alter the exit status. Behavior-neutral across five cases, with temp-file counts taken before and after each run. Refs #3911 * fix(#3911): stage transitive hook lib requires, not just one level The remote run returned 7 failures across 3 real causes. The important one is a PRODUCTION bug this phase exposed rather than caused. `writeCursorHooksJson` scanned each hook script for `./lib/X` requires exactly one level deep and never re-scanned the lib files it staged for their own sibling requires. Nothing had a transitive lib dependency before, so the gap was invisible. Adding hook-exit.js -> cli-exit.js -> exit-code-registry.js made real Cursor installs ship a bundle that dies at require time with MODULE_NOT_FOUND. It now walks to a fixed point, and a real installed Cursor hook runs to completion. The staging harness in shared-hooks-dir-resolution hand-copied its fixture, so the injection scanner crashed at require time and its exit-1 was being read as a policy decision. Migrated to copyScriptWithDeps, which walks the require graph — the repo's recorded rule for this class, since adding another copyFileSync keeps it alive for the next person. The missing-lib-source test in cursor-hook-workspace-roots hardcoded which lib file it expected to be named in the abort message; the same throw now fires for a different file first. Its assertion is unchanged in substance — staging still must abort rather than ship a broken hook — only the name is no longer pinned. The last one was my own test asserting an uppercase reason code. Measured against origin/next: the pre-change hook emits the same lowercase 'config_unreadable', so the test was wrong, not the migration. Corrected to the real value rather than making the code match the test. Verification runs on the remote runner. Refs #3911 * chore(#3911): regenerate the cursor install-tree golden The staging fix means a Cursor install now correctly carries the two transitive lib files it was silently missing. Additive only — no path was removed. The golden diff is the evidence the packaging defect was real. Refs #3911 * chore(#3911): backfill the changeset PR number Refs #3911 * fix(#3911): a git probe that timed out is not a negative A macOS CI lane failed three deny cases at 2084ms, 2112ms and 2177ms — just past the 2000ms budget these hooks give their git probes. The three that passed took 72ms, 595ms and 651ms. Under shard contention `git rev-parse` overruns, the hook reads the non-zero result as "not a git repo", and allows with exit 0 and empty stdout AND empty stderr. Under load, the guards silently stop guarding. That is ADR-3889's thesis exactly, sitting inside the security hooks this phase is about. The repo had already recognized the class in one place — gsd-cursor-subagent-start.js fail-closed-denies on `git_timed_out` (#3045) — but nowhere else. `hooks/lib/git-probe.js` classifies a probe's outcome, distinguishing a real non-zero exit from ETIMEDOUT, a signal kill, and a spawn failure, rather than folding all four into `status !== 0`. Three guards route their eight git probes through it. The resolution is the same shape #3838 took, and the same one that issue endorsed as smallest-sufficient: fail open, but loudly. **No exit code changes on any path** — a developer on a loaded machine is still not blocked, which keeps #3911's declaration-pass contract intact for exit codes. What changes is that the hook now says on stderr which probe could not answer, instead of presenting silence as a clean verdict. Scope was checked across every hooks/*.js, not just the three that failed: gsd-agent-isolation-guard spawns no git; gsd-statusline's two probes gate only a cosmetic display segment, not an allow/deny decision, and are left alone. The C2 deny assertion was a real-race test — it demanded exit 2 while a slow git legitimately yields 0. It now requires the hook to either deny, or allow with a diagnostic naming the probe that could not run; a silent allow still fails, so the assertion is not vacuous. A deterministic regression stubs git on PATH to sleep past the budget rather than waiting for load to reproduce it. Verification runs on the remote runner. Refs #3911 * test(#3911): a PATH shim cannot intercept the hooks' git spawn on Windows The deterministic timeout regression stubbed git on PATH and asserted the guard reports rather than silently allows. It passes on Linux and macOS and failed on Windows in 83ms and 176ms — the stub was never invoked at all. Mechanism: the hooks call spawnSync('git', args) with no shell:true, so on Windows CreateProcess resolves git.exe only and never a PATH .cmd shim. The git.cmd branch could not have worked and is removed rather than left implying a Windows path that does. Adding shell:true to the hooks to serve a test would change product behavior and widen an injection surface, so the case is skipped on win32 only, with the mechanism written into the skip reason so a future reader does not 'fix' it that way. Linux and macOS keep the coverage, and macOS is where the underlying fail-open was actually caught. Refs #3911 --------- Co-authored-by: sim <sim@local> |
||
|
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> |
||
|
Tom Boucher
|
fa41bfec5c |
enhance(#3942): the emitted-drift ack is PR-lifetime data — move it to a commit trailer (#3954)
* test(#3942): failing-first suite for the emitted-drift ack commit trailer Binds 37 input classes from the phase test matrix to the behavior ADR-3942 specifies, before any of it exists. Stubs return benign empty values rather than throwing, deliberately: several rows assert that something DOES throw (cap overflow, uncomputable commit range), and a throwing stub would turn those green for the wrong reason and destroy the red. The two rows that carry the design's load: - merge-base semantics. The range is $(git merge-base base HEAD)..HEAD, not base..HEAD, because changedPaths comes from `git diff base...HEAD` (three dot). Two-dot would let the ack set and the change set disagree about which commits are this PR's. The fixture forks a topic branch, puts a trailer on each side, and asserts only the topic-side trailer is in range. - fail-closed on an uncomputable range. With fragments a depth-1 checkout passes VACUOUSLY, every fragment reading as brand-new. With trailers the range cannot be computed at all, and returning an empty set would silently disarm the gate, so it must throw. The fixture builds a genuine shallow clone rather than simulating one. Also covers the self-inflicted case: this change's own documentation quotes the trailer syntax, so an example landing at the end of a commit message would arm a live acknowledgment keyed on the literal placeholder text. Keys carrying angle brackets or whitespace are rejected. Authored per the phase artifacts 40-design.md and 50-test-matrix.md. Not yet run on the remote runner — this commit exists to be tested. Refs #3942 * chore(#3942): move the emitted-drift ack to a commit trailer Implements ADR-3942, superseding ADR-2719 section 3 and its #2789 amendment. Sections 1, 2 and 4-7 are retained: the conservation law is unchanged, only the storage of its escape hatch moved off the working tree. An acknowledgment explains one PR's ripple, and the moment that PR merges the ripple is in the base, so it can never clear anything again. It was stored in permanent shared state anyway, and every consequence of that mismatch had to be built and then maintained. The chain is #2789 -> #2914 -> #3078 -> #3842 -> #3823 -> #3875, each fix generating the next defect, ending in a scheduled sweeper whose own first PR could not merge itself. Added parseAckTrailers + renderAckTrailer (pure) and readAckTrailers (IO shell), reading Emitted-Drift-Ack-Hash: / Emitted-Drift-Ack-Growth: trailers over the merge-base range. tests/emitted-ack-trailer.test.cjs, 37 cases, written failing-first and confirmed red before any of this existed. Changed diffEmitted takes two structurally distinct key-space maps instead of one shared paths map. That closes a latent defect: the spaces were separated by convention only, so a growth key satisfied a hash lookup by naming coincidence. staleAcks now reports which space a key was declared in. REMEDIATION teaches the trailer, per space, with its example rendered through renderAckTrailer so the taught grammar cannot drift from what the parser accepts. Removed the sweep workflow, the guard-no-ack-on-next job, the standalone linter and its lint:ci entry, the fragment directory and its three spent fragments, the legacy single-file union, and the baseAck/spentAcks mechanism -- spentness is now structural, not computed. Two range properties carry the design and are pinned by tests rather than asserted: the range is merge-base scoped, matching git diff base...HEAD, so an already-merged trailer is out of range by construction; and an uncomputable range throws instead of reading as zero acknowledgments, which is the inverse of the fragment guard's vacuous pass. Three deliberate observable changes, each disclosed in the changeset: the unread runtime field is gone, the legacy file is no longer read, and cross-space excusal no longer works. Ten open PRs carry fragments and will meet a modify/delete conflict. Measured before landing and accepted deliberately; the one-line migration is in the PR body. Verified: lint:ci exit 0. Remote runner to follow on this exact sha. Refs #3942 * fix(#3942): silent trailer collapse, lost coverage, and an unbounded cap Six findings from the orthogonal review round, all fixed in place. BLOCKER -- two trailers of the same name on one commit collapsed silently. readAckTrailers built `separator=1d` where git needs `separator=%x1d`: the `separator=` value inside a %(trailers:...) placeholder is itself a pretty-format string, so the bare hex was emitted as two literal characters and the split on \x1d never matched. Two same-name trailers therefore joined into one value with errors empty -- the first reason absorbing the second entry's key. Silent truncation, the exact class MAX_ACK_TRAILERS throws to prevent. Confirmed with od -c against real git output before and after. The failing-first matrix did not catch it because its "both spaces coexist" row uses Hash plus Growth -- different trailer NAMES -- so the value separator was never exercised. Two regression tests now cover same-name trailers directly. Coverage recovered: normalizeAckReason and INVISIBLE stayed on the live path via parseAckTrailers but lost every test when the old suite was pruned. Back under test against the current surface -- all six invisible codepoints individually, whitespace collapse, trim, CRLF, and two seeded fast-check properties. Dropping any single codepoint now fails. MAX_ACK_TRAILERS counted raw trailers before de-duplication, so one trailer carried forward across rebased commits counted once per commit and could throw on a legitimate branch. Now counts distinct entries; 100 identical repeats dedupe to one. diffEmitted validated baseline, current and changedPaths but not the new ackHash/ackGrowth, so a bad shape raised an unhandled TypeError instead of an error verdict -- the same defect shape this file documents for #2778. Docs: CONTRIBUTING and TESTING-SUITES were rewritten only in their first sections; the later passages still taught fragments, git rm and the deleted guard, contradicting the new text directly above them. Finished. Also extends lint-removed-but-needed to exempt docs/adr and docs/research. That gate fails on any docs mention of a file deleted in the same diff, which makes it impossible to document a deletion in the PR performing it -- an ADR's whole job is naming what it retired. Exemption is narrow and comes with a test proving the gate still fires for a live consumer elsewhere under docs/. A guard that cannot fail is worse than no guard. Maintainer-approved. CONTEXT.md names the retired machinery by role rather than by filename: its generated projection lands in docs/, which that gate does scan. Adds docs/how-to/acknowledge-emitted-drift.md. The required docs set is Reference and Explanation, so the task quadrant can be empty with every gate green -- and this change has a real multi-step journey, including the fragment migration ten open PRs now need. lint:ci exit 0. Refs #3942 * docs(#3942): correct the duplicate-trailer rule in CONTRIBUTING Both axes of the code review independently flagged the same passage, without seeing each other's output. It claimed two declarations of the same key are always "a hard, loudly-reported error, not a silent last-wins". That is only half true, and the missing half is the one contributors hit: identical declarations -- same key, same reason -- dedupe silently, because a trailer legitimately survives a rebase and reappears on every rebased commit. Failing there would red a branch for doing nothing wrong, which is exactly why the dedup exists. Only a same-key/different-reason pair errors, and that one is a genuine ambiguity about which explanation holds. As written, the paragraph told a contributor that a rebase-carried trailer breaks the gate -- the opposite of the behavior. CONTEXT.md's parallel entry already stated it correctly; this brings CONTRIBUTING into line. Doc-only, root-level markdown. Refs #3942 * chore(#3942): backfill changeset PR number to 3954 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1e67ec9737 |
enhance(#3908): the scanners distinguish an empty diff from one they could not compute (#3937)
* feat(#3908): the scanners distinguish an empty diff from one they could not compute collect_files ended 2>/dev/null || true, which destroyed the evidence three ways: the redirect discarded git's diagnostic, the pipe replaced git's status with grep's, and || true forced success regardless. Four distinct conditions - an established-empty diff, a bad ref, no repository, and a repository with no commits - all reported clean, and a secret scanner reporting clean because git failed is indistinguishable from an all-clear to any gate consuming it. git now runs separately from the filter so its status and diagnostic both survive. An established-empty diff exits NO_INPUT; a scope that could not be established exits UNAVAILABLE; the usage sites move off 2 to USAGE. || true is retained on the filter alone, where it is correct: a diff of only images is empty, not failed. Codes are sourced from a generated shell fragment rather than written into three scripts, so a re-allocation cannot desync them, and a missing fragment fails loudly instead of falling back to literals. The security workflow is updated in the same change: without it, a docs-only PR would newly fail the job. * fix(#3908): keep scanner stderr out of the file list, and drop try/finally from test bodies Capturing git and find output with 2>&1 was right for the failure path but wrong for the success path: a warning emitted alongside a successful diff flowed into the file list and was treated as a filename. stderr is now captured separately, forwarded as a warning on success and as the diagnostic on failure, and never folded into the list. Also converts the control tests' try/finally blocks to t.after(), which CONTRIBUTING bans inside a test body because it masks failures. * chore(#3908): backfill changeset pr number * docs(#3908): record the scanners' four-outcome exit contract SECURITY.md is root-level, so the docs gate correctly held: a Changed fragment owes a file under docs/. The contract also belongs where the feature is described, as REQ-SCAN-INJ-05. docs/FEATURES.md is GENERATED from per-feature fragments (#3840) - the first edit went into the generated file and gen-features --check caught it, which is the same edit-the-output drift this epic exists to close. The fragment is the source; FEATURES.md is regenerated. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c5f2b94b27 |
enhance(#3907): gates report no-input instead of a verdict they never reached (#3932)
* feat(#3907): gates report no-input instead of asserting a verdict they never reached The three stdin-reading gates bound 2 to a stdin read error only, with no arm for stdin closed at zero bytes - so empty input flowed into the detector, found nothing, and exited 1, which each module's own comment defines as a negative verdict. An unset PHASE_SECTION made the UI gate assert the phase has no UI. Empty and whitespace-only input now exit NO_INPUT, and a read error exits UNAVAILABLE rather than a locally-invented 2, both resolved through the registry and delivered by terminateNow. The exit code was only half of it: under --json the same input emitted {detected:false}, byte-identical to the fabricated payload #3909 exists to fix, and the blocking coverage gate reads that payload. Empty input now emits the in-tree {skipped:true,reason} form with no detected key at all. teams-status is excluded: it never reads stdin and has no invented 2, so the four-module framing in the issue and ADR is wrong. The dead root bin/lib/ui-safety-gate.cjs is deleted - no installer reference, no workflow invocation, and the live fallback chains are for other modules. Its removal restores the unit tests to the module that actually ships; they had been asserting the stale copy's two-field shape, which is why it drifted unnoticed. * fix(#3907): drive gate tests through the process seam, and make removed-but-needed basename-precise CONTRIBUTING requires every subprocess go through tests/helpers/process-seam.cjs; two of the three gate suites hand-rolled spawnSync while the third, added in the same change, used runNode correctly for the identical injection case. Converted the blocks this change added, leaving pre-existing ones alone. Deleting one of two files sharing a basename made lint-removed-but-needed report 14 references that were all to the surviving canonical module - the false-positive class its own docstring names. It now matches on the deleted file's full path when a surviving file shares its basename, which is more precise rather than weaker: a genuine full-path reference still fails, and behaviour is unchanged when no basename collides. It immediately caught a docstring on this branch that spelled the deleted path. * test(#3907): update the one existing assertion that pinned the old empty-stdin verdict A pre-existing test asserted exit 1 on empty stdin - the defect this phase removes - and was missed because the change added new blocks without auditing existing ones pinning the old contract. Audited the rest: the other three status-1 assertions in that file all feed real input and are the genuine-negative controls that must keep returning 1, so exactly one was stale. The retired 2 is gone from the describe's contract comment too. * chore(#3907): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
941b62249e |
enhance(#3906): two terminators over one registry, with a versioned exit projection (#3924)
* feat(#3906): two terminators over one registry, with a versioned projection Adds terminateNow (write-then-terminate, for callers that cannot wait for the event loop) beside runMain (drain-then-exit), both projecting through one shared function so they cannot disagree - the parity the ADR makes mandatory. A failed write does not change the exit code: letting it propagate would fail a hook open, which is what the fail-closed branches exist to prevent. The projection is versioned. v1 reproduces today's integers, including keeping a payload-carried degraded result at exit 0 - ADR-2980 ratified that across 60 sites and declined normalizing it on measured blast radius. v2 applies the registry. --exit-contract=v2 or GSD_EXIT_CONTRACT=v2 selects it; an unrecognized version throws rather than silently defaulting. The registry is now emitted beside both copies of the exit module, so it resolves as a sibling in the built tree and in the committed scripts/ copy that must load on an unbuilt clone. * fix(#3906): actually restrict code 2 to terminateNow, and generate the registry's type The claim that terminateNow is the only place 2 can be produced was false: runMain's outcome arm applied no guard, so runMain(()=>'HOOK_DENY') set exitCode 2 through the drain path - and the parity matrix demonstrated it while calling it parity. runMain now refuses any outcome projecting to the hook-protocol code, gated on the code rather than the name so an alias cannot slip past, and the matrix asserts the restriction instead of contradicting it. The ambient type for the generated registry was hand-written with no gate against the generator's actual output - the declared-surface-diverges-from-runtime defect class this epic exists to close, reintroduced inside it. It is now a third generated artifact covered by the same --check. Also converts every test-body try/finally to t.after(). * test(#3906): derive the glossary fixture's dependencies instead of hand-listing them Adding a require to scripts/lib/cli-exit.cjs broke 31 tests in one suite that built its fixture from a hand-written dependency list, so the new sibling was absent and the copied script could not load. copyScriptWithDeps walks the require graph and exists for exactly this class - #3412 paid the same bill when one new require broke 82 tests across two suites. Migrating rather than adding another copyFileSync line keeps the class closed. The other nine suites referencing that path were triaged; none copies-and-spawns, so none needed migrating. * fix(#3906): enumerate the new shipped file, drop a vendor name from shipped data, and fix three test defects install: scripts/lib/exit-code-registry.cjs was missing from GSD_SCRIPTS_LIB_FILES, so it shipped to every install and orphaned on uninstall. The registry gave HOOK_DENY a meaning naming one harness, and that string ships into every runtime's tree - a guard correctly caught it leaking into the hermes and qwen installs. The registry is runtime-neutral infrastructure; the vendor name belongs in the ADR, not in shipped data. Two more fixture harnesses built their trees from hand-listed dependencies and broke on the new require; both migrated to the derived helper, and all 23 copy-and-spawn candidates were enumerated so the class is closed rather than patched. One generator test used a fixture code that collided with a real allocation, so the generator correctly reported a duplicate where the test expected drift. The large-payload test embedded a 256KB literal in the child's argv, exceeding Linux's 128KiB MAX_ARG_STRLEN so the child never started - it now builds the payload inside the child. * chore(#3906): backfill changeset pr number * docs(#3906): document the exit-code contract selector P2 is the first phase of this epic with a user-invocable surface, so the flag and env var owe a reference entry. Records what actually differs between v1 and v2 today (one outcome), that an unrecognized value is rejected rather than silently defaulted, and the fail-safe property that makes switching safe. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
39673ae9ff |
fix(#3738): antigravity global skills/agents install to ~/.gemini/config (#3921)
* test(#3738): antigravity global skills/agents must resolve under ~/.gemini/config Regression tests (RED first): --skills-root and gsd-tools query surfaces, install-plan dest dirs, and converter skills-path rewrite. * fix(#3738): antigravity global skills/agents install to ~/.gemini/config Antigravity's machine-local discovery scans ~/.gemini/config/{skills,agents}; the configHome (~/.gemini/antigravity) is deprecated for artifacts. Declare the ADR-1239 skills/agents 'home' override on the antigravity global layout — the same mechanism codex uses (.agents) — and divert ~/.claude/skills/ references in converted global content to ~/.gemini/config/skills/. configHome, settings, probe/migration semantics, and the local .agents layout are unchanged. * fix(#3738): retire deprecated configHome artifacts via installer migration 010 Next install converges an existing antigravity install: manifest-managed skills/gsd-*/ and agents/gsd-*.md under the configHome (a location AGY does not scan) are removed — modified files backed up first, unmanifested and non-gsd entries preserved — and now-empty containers retired. Global scope only; the local .agents surface is live. Docs + inventory updated. * fix(#3738): converter sync in bin/install.js, harness emit-root coverage, migration baseline - bin/install.js converter gains the same ~/.claude/skills → ~/.gemini/config/ rewrite as src (ADR-1508 dual copy must stay in sync). - Parity-manifest walk covers home-override emit roots (extraEmitRootsFor) so antigravity's emitted skills/agents stay differential-visible at their new install root; install-tree fixture regen confirms an unchanged key set. - skills-from-commands rule declares the antigravity converter as a runtime-scoped transform; one ack fragment covers the identity-classed workflow whose antigravity copy embeds the old skills path. - Migration 010 checksum baseline + home-override set doc updated; existing tests updated to the #3738 contract (global dest, golden parity via layout dest, integration expectations). * fix(#3738): tolerate an absent extra emit root on baseline-side measurement The base tree's installer predates the home override, so <HOME>/.gemini/config does not exist there; walk() threw ENOENT and the in-job baseline build failed. An absent extra root is the legitimate pre-override shape — skip it. * fix(#3738): review findings — manifest agents root, bare skills-path rewrite, guard comment - writeManifest resolves the agents-kind home override (_kindDestDirSafe), so the manifest records agents at their actual install root and drift detection keeps working (isolated review finding 1, major). - Converter bare forms ~/.claude/skills and $HOME/.claude/skills (no trailing slash) divert to ~/.gemini/config/skills instead of falling through to the retired configHome path (finding 2). - real-home-guard comment updated: antigravity's global agents kind is the first agents-kind home override (finding 3, doc-only). - Regression tests for both behavioral findings. * chore(#3738): changeset fragment (pr number backfilled after PR creation) * chore(#3738): backfill changeset PR number (3921) * fix(#3738): sandbox HOME in tests that install antigravity global artifacts antigravity is the first home-override runtime in the golden-parity and skills-wrapper suites (codex is not in their runtime lists), so those tests never needed HOME sandboxing — the real-home guard now (correctly) refuses their un-sandboxed global installs on CI, where HOME is the passwd home. * fix(#3738): stop the K3 sequential-sandbox env leak; sandbox L2's home-override plans K3's two back-to-back sandboxHome calls leave HOME pointing at the first sandbox once the after-hooks restore (each call saves the env as it found it, so the second saves the first's sandbox as 'original'). On the windows matrix that leaked gsd-k3-qwen-* home into the L2 property, whose antigravity/global run then (correctly) refused via the #3712 real-home guard — antigravity is the runtime that made L2's plan escape into os.homedir(). K3 now manages the env with a single restore; L2 sandboxes HOME per run, mirroring L1. * fix(#3738): L2 property's HOME sandbox must exist on disk The #3712 guard's sandbox exemption fails closed when identify(effectiveHome) is 'absent' — L2 never created its configDir, so on the windows matrix (tmpdir under the real home) the antigravity/global run refused even with HOME sandboxed. Create the per-run sandbox dir and clean it up. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e20744eacb |
enhance(#3884): failure is a value — strict argv, and --pick that signals absence (#3922)
* test(#3884): failing-first coverage for strict argv and absence-signalling --pick ADR-3473 §8.4 says failure is a value. Three families currently encode failure as success, and this commit pins each one RED before the fix lands. Measured on this tree, 2026-08-26: gsd-tools generate-slug "test" --pick nonexistent -> empty stdout, exit 0 (#3365) gsd-tools audit-open --pick nonexistent_field -> dumps the entire human-readable audit report, exit 0 gsd-tools generate-slug "Hello World" --raw --pick bogus -> prints "hello-world", another field's value, exit 0 gsd-tools query state.planned-phase 3 (positional, no --phase) -> exit 0; STATE.md's "Phase: 2 of 5 (Widget Support)" is overwritten to "Phase: null - READY TO EXECUTE" and the frontmatter gains a corrupted current_phase_name (#3358) tests/pick-flag.test.cjs:27 previously asserted the #3365 defect as the contract ("returns empty string for missing field", success === true). That assertion is replaced by the required behavior rather than deleted. The new parseNamedArgs block calls the spec-object signature that does not exist yet, so it fails today by construction. The 11 existing behavior-lock tests are left untouched here; they are corrected in the implementation commit. C1/C4 assert at the consumer's output - STATE.md's bytes - per ADR-3180 Decision 4(b). A unit assertion on the parser would have passed throughout this defect's life. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3884): failure is a value — strict argv, and --pick that signals absence Implements ADR-3473 §8.4. Absence, emptiness and failure stop being interchangeable ways to say "I could not answer". parseNamedArgs (src/command-arg-projection.cts) Takes a spec object with a REQUIRED `positionals: number | 'rest'` and returns the hub's Result shape instead of a bare Record. Declaring the positional arity is what makes #3358's call site unrepresentable rather than merely detectable: an unrecognized flag or a token past the declared boundary is now InvalidArgs, naming the offending token and listing the accepted flags. The legacy positional-array call shape throws a TypeError — an internal invariant violation per ADR-3473 Decision 2, so a stale hand-written .cjs call site fails loudly instead of destructuring undefined off a Result. parseNamedArgsOrExit projects a failure onto the caller's error(); it is a projection over the one parser, not a second parser. Measured before, against a STATE.md with a populated phase-2 block: query state.planned-phase 3 (positional, no --phase) -> exit 0; "Phase: 2 of 5 (Widget Support)" overwritten to "Phase: null - READY TO EXECUTE", frontmatter gains a corrupted current_phase_name After: exit 1, `unexpected positional argument "3"`, STATE.md byte-identical. The flag form is unchanged and still updates STATE.md. --pick <field> (gsd-core/bin/gsd-tools.cjs) extractField returns {found,value}, and the pick block no longer shares one catch between "output was not JSON" and "field was absent". An absent field exits 1 with pick_field_absent, naming the field and the keys that do exist; non-JSON output exits 1 with pick_output_not_json instead of dumping the command's entire output. A field that is PRESENT with value null, '', 0 or false still prints at exit 0 — that is an answer, not a failure, and it is what keeps `--pick count` printing 0 on a fresh project. Measured before: `audit-open --pick nonexistent_field` printed the whole human-readable audit report at exit 0, and `generate-slug X --raw --pick bogus` printed "hello-world" — a different field's value, confidently, at exit 0. ADR-3409 Decision 7 explicitly deferred this contract fix to #3473; this is it. The sub-issue's "returns 0 when the count is zero OR absent" wording is superseded by the ADR rule it implements: zero prints 0, absence exits non-zero. Defaulting absence to 0 would demote "could not answer" to "the answer is zero" — the hazard docs/how-to/resolve-unreachable-guard-findings.md already warns against. Guard ledger (ADR-3473 Decision 6) scripts/lint-unreachable-guard-drift.cjs Detector A is RETIRED. Its premise — that a `--pick ... || echo` arm can never fire — is now false, so the shape it forbade is the correct idiom and keeping it would forbid the fix. Detector B (glob-consuming cat/ls, a nullglob mechanism this change does not touch) is retained in full, as are the shared scanner, the escape-marker parser and the baseline. Net: -1 detector, 0 added. The file is not deleted. Call-site audit 45 prompt-layer --pick invocations, every one a plain X=$(...) assignment — none in an if test, && chain, or a pipeline whose status is consumed, and no shell block in workflows/commands/agents/references sets -e. Of the 13 (command, field) pairs the prompt layer reads, 10 are always present; the 3 sometimes-absent ones each sit behind a prior found/existence check. No ADR-3409-class "field the command never produces" remains. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): escape untrusted tokens in diagnostics, and cover five unpinned rows Two review findings, both fixed here rather than recorded as limits. 1. A newline in an untrusted token forged a second stderr line. Before, plain-text mode: $ gsd-tools query state.planned-phase $'foo\nError: forged second line' Error: unexpected positional argument "foo Error: forged second line" After: Error: unexpected positional argument "foo\nError: forged second line" --json-errors mode was never affected — io.error runs that payload through JSON.stringify. Plain-text mode writes 'Error: ' + message verbatim, and the three new InvalidArgs reasons plus the two new --pick diagnostics all interpolate a token that comes straight from argv. Fixed with ONE shared helper, formatDiagnosticToken (src/io.cts), applied at every interpolation site — not a copy per site. It is deliberately NOT applied inside error() itself: several callers in this tree emit intentional multi-line diagnostics, and escaping newlines there would mangle them. The available-top-level-keys list needed the same treatment for a reason the review did not anticipate: `frontmatter get <file>` reads an ARBITRARY user document and echoes that document's own keys into the diagnostic. Verified reachable — a frontmatter key containing a newline reaches the key list — so formatKeyForDiagnosticList is guarding a live path, not a hypothetical one. Ordinary keys still render plain and unquoted; a fix that merely dropped the key would also have passed a "one line" assertion, so the test pins the escaped key's presence too. 2. Five behavior-table rows were implemented but nothing pinned them: B7 a dotted path that dies partway B9 bracket syntax on a non-array B10 a negative array index, in and out of range B14 a JSON root that is not an object B17 an @file: payload over 50KB B17 is the load-bearing one. output() writes @file:<path> instead of inline JSON past 50000 characters, and --pick resolves that BEFORE parsing; with no test, a future reordering of those two steps turns every large result into a false pick_output_not_json. The fixture seeds 1200 phase directories and measures the payload at 62474 characters, asserting the spill actually happened rather than assuming it. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): correct the strict-argv surface against a full verification run The first full run came back with 90 failures across 12 files, none in the new tests. They were the argv surface telling me what it actually is. Ten root causes; each classified before anything was changed. I over-implemented, and that is reverted. ADR-3473 §8.4 says parseNamedArgs rejects "unrecognized and positional tokens". It says nothing about a value flag whose value is missing. Making that an error was my design decision, not the rule, and it broke a deliberately recorded contract: `--prd` with no value resolving to null (tests/init.test.cjs emptyPrdValueIsFalsyAndTreatedAsAbsent, row B5; tests/section-manifest-init-facts.test.cjs "flag-shaped value"). The "requires a value" branch is deleted outright rather than kept behind an option — an unused strictness mode is speculative generality. Unknown-flag and unexpected-positional rejection, which is what §8.4 actually mandates, is unchanged. --wave needed a third flag kind the original design did not anticipate. `--wave N` is documented (commands/gsd/execute-phase.md:4,48) and the shipped workflow reconstructs and passes it (execute-phase.md:84), while #2932 records token-PRESENCE semantics: the CLI cares only that the flag appeared, and the value belongs to the workflow layer. That is neither a boolean flag nor a value flag, so `optionalValueFlags` now exists — presence-only in `data`, and the validation cursor consumes a following non-flag token so it is not reported as a stray positional. Every other declared boolean flag was checked against every argument-hint and prose usage in commands/, workflows/, agents/ and docs/; `--wave` is the only one of this shape. Five tests were pinning forms that never worked. tests/adr857-core-without-capabilities.test.cjs passed `init plan-phase --phase 01-stub`, but the documented form is positional (docs/CLI-TOOLS.md:776) and the handler reads args[2] — which for that form is the literal string "--phase". Measured on the pre-fix build against a real .planning/phases/01-stub/ directory: init plan-phase 01-stub -> phase_found=true init plan-phase --phase 01-stub -> phase_found=false The test asserted only exit 0 and key presence, so it had been green while proving nothing about phase resolution. Corrected to the documented form and strengthened to assert phase_found === true. Same class in state.test.cjs (`--plan-count`, a flag that does not exist; the real one is `--plans`), milestone-archive.test.cjs (`init new-milestone --json`, silently ignored), and concurrency-safety.test.cjs (a bare positional field name whose OR-assertion passed because a whole-document dump happens to contain the substring it looked for). Six handlers had no argv validation at all — the same #3358 shape this phase exists to close, found while fixing the rest: init verify-work / phase-op / review / todos / remove-workspace read args[2] with nothing checking the rest, and validate health read --repair/--backfill through a bare args.includes() scan that bypassed the parser entirely. All now go through the seam, so the flag has one owner. tests/init-debug.test.cjs rows C4/C5 asserted that an unrecognized flag must NOT fail. That is the behavior §8.4 removes, and Decision 8 says a caller's local expectation does not override §8, so they are inverted and renamed — a test still called "ignores an unrecognized flag" while asserting rejection would be its own defect. Row C6's point is its PWNED canary; that assertion is kept verbatim and only its exit-status expectation changed, because the hostile token is now rejected rather than absorbed. The blast-radius estimate in 40-design.md is corrected rather than quietly left wrong. get_impact reported MEDIUM / 8 symbols upstream, and that was accurate for what the graph can see — parseNamedArgs's callers. It cannot see that those callers' handlers accept argv shapes wider than the code reading args[2] suggests, which is where the real surface was. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): withdraw the validate-health tightening, finish the A2/A3 revert Second full run: 46 failures, down from 90. Four causes, two of them mine. Reverted `validate health` entirely — it was scope creep, and it broke a real flag. ~30 of the 46 read `unknown flag "--json"; accepted: --repair, --backfill`. The previous commit routed `validate health` through the parser on the reasoning that a flag should have one owner. That was wrong twice over: §8.4 names parseNamedArgs and count queries, and `validate health` was never a parseNamedArgs call site — it read its flags, just not through the parser, so it had no silent-drop defect to fix. Tightening it omitted `--json`, which the health-diagnostic suites use heavily. The handler is now byte-for-behaviour back to its pre-branch form. `validate context` stays converted: it genuinely was a call site, and its `--json` is now declared rather than read by a second `args.includes` scan. The five handlers that had NO validation at all — init verify-work / phase-op / review / todos / remove-workspace — stay fixed. Those read args[2] with nothing checking the rest, which is the #3358 shape this phase owns. Finished the A2/A3 revert. Three tests still encoded the deleted "a value flag with a missing value is an error" rule, including one added by the previous commit for that rule. All three now assert the reverted null contract, and the ones whose titles said "rejected" are renamed — a test named for a contract it no longer asserts is its own defect. `--wave=` and `--wave --weird` are correctly rejected. Neither is documented in commands/gsd/execute-phase.md, gsd-core/workflows/execute-phase.md or docs/, and neither is emitted by the shipped prompt layer, so both are unrecognized tokens that §8.4 mandates rejecting. `doesNotConsumeFollowingFlagAsWaveValue` keeps the property it exists for — asserted directly now, at the parser, that `--wave` does not swallow a following flag as its value — and only its exit-status expectation changed. A contradiction inside this branch, surfaced by the audit and resolved the safe way. Two pre-existing #3573 tests call `state begin-phase '2'` and `state planned-phase '2'` with a bare positional, relying on the old permissive parser to ignore it. This branch's own #3358 regression test requires that exact argv to be REJECTED. The two are mutually exclusive. Widening the router to accept a bare positional — mirroring complete-phase — would have silently re-opened #3358, and was verified to do exactly that: with the widened router, `query state.planned-phase 3` returned exit 0 and wrote current_phase_name again. It is reverted. docs/CLI-TOOLS.md:116 and docs/COMMANDS.md:2192 document only the `--phase N` form for both verbs, so the two #3573 tests move to it. Their assertions were never about the call shape — only that total_phases survives the resync — and both still pass. complete-phase is untouched: its bare positional IS documented, and it keeps the dynamic boundary and the negative-space note that record why. The audit that produced this is in the PR body: for every handler whose declaration changed, the flags it reads anywhere in its body, the flags the shipped surface documents, and the shapes the suite passes, compared. The `--json` miss was a pattern, not an accident — declaring a handler's flags from its parseNamedArgs call alone misses whatever it reads elsewhere. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3884): backfill the changeset PR number Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
878f25025c |
enhance(#3905): the exit-code registry — one number, one meaning, enforced at build (#3920)
* feat(#3905): the exit-code registry — one number, one meaning, enforced at build A generated registry replaces locally-invented exit codes. Every entry records code, name, meaning, owning module and the decision that authorized it. The generator refuses to build a table where two entries claim one code, two claim one name, a code falls in a range Node or the shell reserves, 2 is claimed by anything but the hook adapter, or an allocation carries no justification. exitCodeFor is pure and total: it throws rather than returning undefined, including for prototype-chain names. Inert by design — nothing emits a registered code until #3906. Every registered code is non-zero, asserted over the whole table, so a caller testing for failure behaves identically for pass and trips for everything else. * feat(#3905): make the registry generator's failures machine-readable Adds a --json mode carrying {ok, reason, context, detail}, where context is a typed payload naming the specifics the prose embedded - which code collided and under which names, which band rejected a code, which field was missing. The tests now assert on that structure instead of regex-matching the generator's stderr, which CONTRIBUTING prohibits, and the CONTEXT.md glossary gains the entry the issue's scope requires. * test(#3905): refresh the install-tree fixtures for the new declaration The registry declaration ships in the install tree, so all 19 golden fixtures needed regenerating. Caught by the remote matrix, not by lint:ci - the install-tree goldens are verified by a test rather than a lint, so a newly shipped file clears every local gate and fails only under the suite. * chore(#3905): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7e9d33c378 |
enhance(#3915): stryker on the official tap runner, 29m to 12m on the critical path (#3919)
* enhance(#3915): stryker on the official tap runner, per-test coverage The 'command' runner is the one runner Stryker excludes from coverage analysis, which forced coverageAnalysis:'off' and made mutation cost strictly linear in (mutants x whole-shard test time). The frontmatter shard measured 1751s against 212s for the next slowest. Swap to @stryker-mutator/tap-runner (official plugin, peer-pinned to the @stryker-mutator/core@9.6.1 already installed) and turn coverage analysis on, so Stryker re-runs only the test files that cover each mutated line. Per-shard injection moves from a MUTATION_TEST_CMD command string to MUTATION_TEST_FILES, read by a new fail-closed resolveMutationTestFiles() that mirrors resolveMutationBreak. It stays derived from scripts/mutation-matrix.cjs and now has a single owner: the union moves behind allCoveredTests() and stryker.config.mjs no longer imports COVERED. The resolver existence-checks every entry, because the tap runner resolves tap.testFiles with glob() and a non-matching pattern yields an empty list silently - a fast, confident, meaningless run. tap.forceBail is off by measurement, not preference: a structural AST audit found 3 of 26 shard test files spawn subprocesses, and bail fires on every killed mutant, so leaving it on would kill processes mid-spawnSync and orphan their children. The matrix 'isolation' field is removed - per-file process isolation is inherent to the tap runner, so the field had no consumer left. Score arithmetic is unchanged and now pinned: mutationScore counts NoCoverage in the same denominator as Survived, which both Stryker's thresholds.break and check-mutation-score-ratchet.cjs read. A new non-vacuity test proves it diverges from mutationScoreBasedOnCoveredCode, so the gate cannot be quietly swapped to the field that would make every floor trivially satisfiable. Refs #3915 * fix(#3915): enforce the resolver's documented containment contract Review findings, all fixed in place. resolveMutationTestFiles claimed to verify each entry exists 'relative to the repo root' but used a bare fs.existsSync(path.join(...)), which accepts an existing DIRECTORY and lets ../ segments escape the root (path.join('/repo/root','../../etc/passwd') resolves to /etc/passwd). Not reachable from PR content - the value only ever comes from the static COVERED registry via CI env - but a fail-closed contract that overstates its own guarantee is a defect in the contract. Each entry is now resolved, rejected if path.relative puts it outside the root, and required to be a regular file. Three hostile-input tests added; the original missing-file wording is preserved so the existing assertion still binds. Also: removed a stale buildResult comment still naming the isolation field this branch deleted; hoisted one top-level path require in place of three inline ones; de-duplicated the derived-test-list expression in the tests to a single const, deliberately still re-derived from COVERED rather than calling allCoveredTests() so the assertion cannot become a tautology; tightened the workflow-parity assertion to exact equality; changed the tap-runner range from an exact 9.6.1 to ^9.6.1 so it tracks the caret-ranged core its peerDependency pins exactly. Regenerated examples/dynamic-context-management/CONTEXT-INDEX.json, which the earlier CONTEXT.md edit left stale - lint:ci was red on lint-example-parser-parity until it was refreshed. Refs #3915 * test(#3915): kill model-catalog survivors, set frontmatter budget from measurement From mutation run 33026833181 (all 13 shards, dispatched on this branch before any PR). WALL TIME: frontmatter measured 713s (11m53s) vs 1751s (29m11s) on the command runner, a 59% cut. timeoutMinutes 60 -> 20 (1.68x measured). Not deleted outright: the shared 15-minute default would leave only 21% headroom, and this module's mutant count grew 1.8x in one change. MODEL-CATALOG: came back 57.91 against its floor of 58. Diagnosed from the report JSON, not assumed - all 24 of its new RuntimeError mutants are in the load-time catalog bootstrap, so mutating them makes the module throw at require. Under node --test that is a failed test file (Killed); under the tap runner the process dies before emitting TAP, which Stryker classifies RuntimeError and excludes from the denominator. Add them back as killed and the shard is 248/416 = 59.62, the pre-change number exactly. Detection did not regress, classification changed. The floor is NOT lowered to absorb that. 11 new behavioural tests target ~42 genuinely surviving mutants: exact Set equality on the EFFORT_RENDERING/EFFORT_ARGV supported sets, an exact-string table render that makes the column-width arithmetic observable (padEnd never truncates, so the old substring assertion could not see a too-small width), clampEffortForHost's full null matrix plus a spoofed-toString host, and a prototype-pollution guard driven through Object.fromEntries. Mutants judged equivalent were skipped rather than papered over; reasoning is in the phase artifacts. All 15 new assertions verified against unmutated code. lint:ci exit 0. Refs #3915 * test(#3915): ratchet model-catalog's floor to 74 on measured 75.26% CI run 33029755081 measured model-catalog at 75.26% (295 killed / 49 survived / 48 no-coverage / 24 runtime-error, totalValid 392), so check-mutation-score-ratchet.cjs correctly failed the shard for unclaimed headroom: 17.26 points above the declared floor of 58, well past the 5-point slack. Floor raised 58 -> 74 (floor(75.26)-1, this file's documented convention), with RATCHET_BASELINE updated in the same diff as the equality assertion requires. Worth recording why the number moved this far. The shard first came back at 57.91 under the tap runner and the temptation was to lower the floor to match. The drop was not a regression: all 24 of its RuntimeError mutants sit in the load-time catalog bootstrap, and adding them back as killed reproduces 248/416 = 59.62, the pre-swap figure exactly. Rather than absorb a reporting artifact by weakening the gate, 11 behavioural tests went after the genuinely surviving mutants and killed 68 of them - carrying the module from 59.62 past its old ceiling to 75.26, within reach of the ADR-456 target of 80. The #3007 measurement is kept as clearly-labelled prior context rather than deleted, so the entry does not read as carrying two current numbers. Refs #3915 --------- Co-authored-by: sim <sim@local> |