ff0361071dfdc25ce35e2c04058fcc639a4e360f
1176 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
ff0361071d |
feat(#3653): add review.models.cursor — wire modelArg/modelConfigKey for the cursor reviewer lane (#4160)
* feat(#3653): add review.models.cursor — wire modelArg/modelConfigKey for the cursor reviewer lane cursor-agent exposes --model (204 selectable models) but the cursor lane declared modelArg: null / modelConfigKey: null, so review.models.cursor was rejected as an unknown config key and the #1517 reviewer-instances escape hatch silently discarded a configured model at modelExpansion. Wire the lane the same way codex already is: inject {{model}} into args right after -p, set modelArg to --model, and declare modelConfigKey as review.models.cursor plus its config schema entry. An unconfigured lane still invokes byte-identically to today. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3653): add changeset for review.models.cursor Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3653): update co-change surfaces that assumed cursor has no model key gsd-test surfaced three surfaces still hardcoding "cursor declares no modelConfigKey", broken by wiring review.models.cursor: - tests/reviewer-config-federation.test.cjs: the #3691-narrows-#2797 invariant test listed cursor among lanes that must own no model key. - tests/settings-integrations.test.cjs: the #3651 keyless-lane test listed cursor as keyless, including a live config-set assertion that now correctly succeeds instead of failing (swapped to qwen). - gsd-core/workflows/settings-integrations.md: the settable-keys enumeration and two prose call-outs still named cursor as keyless. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3653): acknowledge deliberate growth of settings-integrations.md settings-integrations.md grew 4 bytes because it now enumerates review.models.cursor as a settable key alongside the other reviewer lanes, matching the modelConfigKey wired for cursor in this PR. Emitted-Drift-Ack-Growth: settings-integrations.md — adds review.models.cursor to the settable-keys enumeration and removes cursor from the two keyless-lane call-outs, matching #3653's modelConfigKey change Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3653): fix malformed Emitted-Drift-Ack-Growth trailer The previous commit's trailer was separated from Co-Authored-By by a blank line, splitting it into an earlier, non-trailer paragraph — git's trailer parser only recognizes the last contiguous block. Restating it here immediately adjacent to Co-Authored-By so both parse as trailers. Emitted-Drift-Ack-Growth: settings-integrations.md — adds review.models.cursor to the settable-keys enumeration and removes cursor from the two keyless-lane call-outs, matching #3653's modelConfigKey change Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3653): backfill changeset pr number pr:0 -> pr:4160 now that https://github.com/open-gsd/gsd-core/pull/4160 exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
|
ddb877fa0a |
enhance(#3957): a no-op reports the real condition and the values it already computed (#4157)
* test(#3957): add failing-first coverage for no-op decline reporting (epic #3473 B9) * fix(#3957): a no-op reports the real condition and the values it already computed (epic #3473 B9) * test(#3957): correct stale assertions and a withheld-arm fixture after rebase (epic #3473 B9) * docs(#3957): add Fixed changeset fragment for no-op decline reporting (epic #3473 B9) * docs(#3957): backfill changeset PR number to #4157 --------- Co-authored-by: sim <sim@local> |
||
|
|
bf4485ada2 |
enhance(#3717): make the edge probe's shape cues language-aware via an optional text_en field (#4156)
* test(#3717): add failing-first coverage for text_en language-aware classification Adds unit tests for the not-yet-implemented text_en field on Requirement (fallback selection, empty/whitespace/non-string rejection, shapes-override precedence), a SHAPE_CUES/VALID_SHAPES parity guard (RULESET.GENERATIVE-FIX), and workflow-prose contract tests asserting spec-phase.md Step 5.5 documents populating text_en for response_language projects. All new tests are RED until src/edge-probe.cts and the workflow docs are updated. * feat(#3717): make edge-probe shape classification read an optional text_en field Requirement gains an optional text_en; classifyShape's own signature stays untouched (a locked, directly-tested export), and the text_en ?? text selection is pushed to proposeEdges' single call site instead. text_en is validated fail-closed: an empty or whitespace-only value throws rather than silently winning the ?? fallback and degrading classification to zero shapes. This makes the #2773 doc-only translation convention an explicit, validatable field instead of an invisible instruction, per the approved Form-1 scope on #3717. * docs(#3717): document the text_en field across spec-phase, reference and how-to docs Updates Step 5.5's response_language instructions, the edge-probe reference Inputs contract, the FEATURES.md fragment, and the non-English how-to guide to describe the new text_en field: text keeps the requirement's own wording in all cases, text_en (when populated) is the engine-only English rendering the classifier prefers. * docs(#3717): record the text_en locked-surface change in CONTEXT.md and ADR-550 Updates the Edge Probe Module glossary entry to describe the text_en field and its fail-closed validation, and appends an ADR-550 amendment recording why this is additive and does not re-open the #652 LLM-classifier rejection (text_en is a plain field read by the existing deterministic regex classifier, not a new model-dependent surface). * docs(#3717): add changeset fragment and regenerate FEATURES.md pr:0 placeholder — backfilled with the real PR number after the PR opens. * docs(#3717): attribute the text_en machine check to engine-level validation, not prose tests Code-review (Spec axis) finding: the workflow-prose contract tests and the ADR-550 amendment overclaimed themselves as "the machine check the #2773 doc-only stopgap lacked." That check is actually engine-level (validateRequirement/classifyShape, covered in tests/edge-probe.test.cjs) — the prose tests are the same style of assertion #2773 already used. Reworded both to attribute the claim correctly. * fix(#3717): rewrap spec-phase.md so the id-unchanged sentence stays on one line The #3717 rewrite of Step 5.5's response_language paragraph moved a line break so "requirement `id`s" ended one physical line and "are never translated" started the next. The pre-existing #2773 regression test (tests/edge-probe-spec-phase-contract.test.cjs) asserts id + "never translated" on the SAME line (no \n in between, matching git's own line-oriented prose), so the reflow silently broke it. Rewrapped so the sentence lands on one line again, verified against every #2773/#3717 regex assertion in that test file. Emitted-Drift-Ack-Growth: spec-phase.md — #3717 adds text_en documentation to Step 5.5 (response_language paragraph + REQS_JSON heredoc comment); this growth is this PR's own diff, not incidental drift. * chore(#3717): backfill changeset PR number pr:0 -> pr:4156 now that the PR exists. --------- Co-authored-by: sim <sim@local> |
||
|
|
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> |
||
|
|
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> |
||
|
|
4c6643a8d7 |
fix(#3945): require a non-empty denominator for milestone-complete (#4164)
* test(#3945): 0-of-0 milestone counters must not render complete Regression coverage for the string-truthiness vacuous equality in both statusline renderers: '0' counters, boundary 1/1 vs 0/1 vs 1/0, compact parity. * fix(#3945): require a non-empty denominator for milestone-complete Both statusline renderers compared the regex-captured string counters directly, so '0' && '0' && '0' === '0' fired Scene 3 on the empty set. Numeric coercion plus total > 0 restores the guard's intent; all non-zero shapes and percent=100 render byte-identically. * chore(#3945): add changeset fragment * chore(#3945): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
c9bff8d2b9 |
fix(#3899): resolve REVIEWS.md by quoted assignment, not an unquoted ls (#3928)
* fix(#3899): resolve REVIEWS.md by quoted assignment, not an unquoted ls The convergence loop resolved the phase REVIEWS.md through `$(ls ${phase_dir}/... 2>/dev/null)`. The unquoted ${phase_dir} word-splits, so a project path containing a space handed `ls` arguments that do not exist, the discarded stderr hid the error, and the empty result was reported as "review agent did not produce REVIEWS.md" — blaming the agent for a quoting defect. A glob metacharacter fails worse: the pattern expands to whatever sibling directory matches, so the loop silently reads a different phase's REVIEWS.md and `ls` still exits 0. Assign the path directly, quoted, and fail closed with an error that names the expected location. The regression block extracts the bash fence from the shipped workflow and RUNS it against space-, glob- and missing-file fixtures, because every text assertion in that suite passed against the broken line. * chore(#3899): add the changelog fragment * fix(#3899): reject a non-file reviews path and anchor the gate harness Adversarial review (codex, gemini) on the first cut, three findings taken: - `[ -r ]` alone is true for a readable DIRECTORY, so a directory standing where the reviews file belongs passed the gate and reached the consumers that read it. Require a regular file as well. - the harness picked its fence by scanning the WHOLE document for a bash block assigning REVIEWS_FILE. If the real fence ever stopped assigning it and an unrelated one started, the harness would execute the wrong block and report green. Anchor the span to the verification step, between its opening sentence and the next heading. - the no-subshell assertion matched only `$(`; a backtick rewrite would reintroduce identical word-splitting unseen. The discarded-stderr assertion is scoped to lines naming REVIEWS_FILE rather than banning the redirect across the whole fence. Four findings declined: `return 1 || exit 1` (the document's own idiom is a bare `exit 1`, including the guard immediately below), `${phase_dir:-}` for `set -u` (unchanged exposure from the old line, and an unset phase_dir is a bug worth surfacing), an explicit empty-phase_dir branch (the guard already fails closed and prints the truncated path), and a downstream `[ -n "$REVIEWS_FILE" ]` hazard (no such consumer exists — the guard exits before any of them run). * chore(#3899): backfill the changeset PR number * fix(#3899): address required review feedback Use the fragment-aware workflow reader, remove the inert exemption marker, document the unreadable-arm coverage limit, align errors with the workflow convention, and name an empty phase_dir directly. Migrate the obsolete emitted-drift fragment to the current commit-trailer contract. Emitted-Drift-Ack-Growth: plan-review-convergence.md — #3899: replace unquoted $(ls ... 2>/dev/null) resolution with a quoted direct path, fail-closed file/readability checks, and an explicit phase_dir diagnostic; surrounding prose records why quoting is load-bearing. --------- Co-authored-by: davdittrich <davdittrich@gmail.com> Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
|
b6dd4e2e74 |
fix(#3936): dispatch quick research with researcher persona and model (#4158)
* test(#3936): quick research dispatch must use researcher persona/model Regression tests for the planner-persona leak in quick's Step 4.75 research dispatch: init quick emits researcher_model, the workflow resolves AGENT_SKILLS_RESEARCHER and parses researcher_model, and the researcher dispatch uses the researcher persona/tier while planner and executor dispatches keep their own. * fix(#3936): dispatch quick research with researcher persona and model cmdInitQuick now emits researcher_model (parity with the plan-phase init), quick.md resolves AGENT_SKILLS_RESEARCHER and parses the new field, and Step 4.75's dispatch swaps ${AGENT_SKILLS_PLANNER}/ {planner_model} for the researcher's own persona and tier. The #2517 model-omission list gains researcher_model. Emitted-Drift-Ack-Growth: quick.md — researcher persona resolution line + researcher_model parse-list entry (#3936) * chore(#3936): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
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> |
||
|
|
bdfc62889b |
fix(#3784): read the hybrid Current Plan: N of M shape, keep zero-padding, and name the accepted shapes on failure (#3791)
* fix(state): read the hybrid "Current Plan: N of M" shape advancePlanCore derived the value FORMAT from the field NAME, so it handled the legacy pair (`Current Plan` + `Total Plans in Phase`) and the compound `Plan: N of M`, but not the hybrid of the two: the legacy field name carrying a compound value with no Total Plans sibling. `legacyTotal` is null so the legacy branch fell through, and the compound branch reads the `Plan` field through a `^Plan:`-anchored pattern that never matches `Current Plan:`. Both produced NaN against a file whose plan numbers are plainly readable. The shape is not exotic. An agent wrote it unprompted into a project's STATE.md, believing it was the parseable form, and every subsequent run in that project inherited the failure and worked around it by hand. Track the field name and the value shape separately (`planSourceField`, `planRawValue`) so write-back targets whichever field the value came from. The legacy pair still takes precedence when both fields exist, so a stray "of N" inside Current Plan cannot override an explicit Total Plans — covered by a new test. Also replace the caller's catch-all error. It reported "Cannot parse Current Plan or Total Plans" for ANY transition failure, and named no accepted shape, so a reader learned neither what failed nor what to write. It now distinguishes "no result" from "unreadable plan position" and lists all three shapes. The existing test asserted the literal "cannot parse"; it now asserts the message names the shapes, which is the property that makes it actionable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8 * fix(state): keep zero-padding when advancing a compound plan value The compound write-back rewrote only the leading half of "N of M", so a padded value drifted lopsided: "04 of 06" advanced to "5 of 06". Cosmetic on its own, but a plan line that looks wrong is one the next writer tidies by hand, and hand-tidying this particular line is what produced the hybrid shape the previous commit had to teach the parser to read. Pad the incremented number to the width it was written with. padStart never truncates, so a value that outgrows its padding widens correctly: 09 of 12 advances to 10 of 12. Unpadded values are untouched — 2 of 6 still advances to 3 of 6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8 * fix(state): pass a literal field name to the compound write-back The previous commit passed `planSourceField` — a variable — as the field-name argument to `stateReplaceField`, which trips the state-write-path drift guard's `unstripped_content_write` axis (ADR-3408 §8.3(b)). The guard is right to care: a Title-Case literal cannot collide with a lowercase or snake_case frontmatter key, so it is safe whatever the content argument is, while a variable could hold anything and therefore requires its content to be demonstrably frontmatter-stripped first. The content argument here IS stripped — `body` is `stripFrontmatter(content)` — but the guard does a narrow backward scan rather than dataflow tracking, by design, and the nearest preceding assignment to `body` is another `stateReplaceField` result. Rather than baseline a bypass or ask a future reader to re-derive that the invariant holds, dispatch on the discriminator and pass the literal. Guard goes from 1 finding to 0; its own 32 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8 * chore(3784): add changeset fragment for #3785 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8 * test(#3784): cover the maintainer's AC1 write-back and AC6 reader-anchoring Triage published six acceptance criteria; two were only half-covered. AC1 asks that the hybrid write back to the SAME field with padding preserved. The existing hybrid test used an unpadded value and asserted only `result.data`, so it proved the parse but never the write. Now asserts the written content is `05 of 06` on the original field, and that no separate `Plan:` field appears as a side effect. AC6 asks that the shared field reader not be loosened. Reading the hybrid is the transition's job; `stateExtractField('Plan')` is line-anchored and has 13+ callers, so teaching it to match a name merely ENDING in "Plan" would be the wrong fix and would silently change what those callers read. This holds by construction here — the reader is untouched — but nothing locked it in. The new test fails if anyone later reaches for that shortcut. Also drops the changeset fragment written against the auto-closed PR number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8 * chore(#3784): add changeset fragment for #3791 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8 * fix(#3784): write the advanced plan back to the field it was read from Review findings 2-6 on #3791 were one defect seen from several angles: the read path learned the hybrid `Current Plan: N of M` shape, the write path did not follow it. - `bumpLeadingNumber` now owns the increment for all three parse branches. Only the leading digits belong to this transition; the padding width and everything after it (` of M`, and the `\r` of a CRLF file) are the author's text and are preserved. The legacy branch wrote `String(newPlan)`, which turned `2 of 99` into `3` and `04` into `5`. - `mutateCurrentPositionForAdvance` takes the plan field NAME. Its plan arm only ever looked for `Plan:`, so on a hybrid file the `## Current Position` section was never reached; combined with the body-level write being single-shot and bold-preferring, a file carrying the field at both sites advanced the header and left the section a plan behind. The parameter defaults to `Plan`, so the two callers that pass no plan are unchanged. - Tests: both-sites-advance (fails without the section arm), legacy write-back content assertions (the previous test read only `data` and so could not see the lossy write), hybrid boundary at limit-1 and limit+1, a CRLF fixture, and an fc property pinning the padding-width contract. Two characterization tests pinned `**Current Plan:** 02` advancing to `3`. That dropped padding is the defect #3784 reports, so the expectation is corrected to `03` rather than the fix being narrowed around it. * fix(#3784): drop the unreachable advance-plan error branch, sync the doc Findings 1 and 8 on #3791. The `!resultData` arm could not fire: the transform callback assigns `resultData` unconditionally, only runs once STATE.md is known to exist (the missing-file case returns "STATE.md not found" upstream), and every `advancePlanCore` return path sets `data`. It was a speculative second failure mode with a message no caller could receive, and the comment beside it claimed to distinguish two things that were never two. `!resultData` stays in the condition as a type guard, which is all it ever was. `docs/json-errors.md:142` quoted the old error literal verbatim and was the sole occurrence in the tree; it now quotes the emitted one. * chore(#3784): describe the write-back fix in the changeset * fix(#3784): anchor the plan grammar and widen the schema row to match Review round 3 on #3791: B1, B2, M1, M2, M3, M4 and the planSourceField nit. B1 — `STATE_FIELD_SCHEMA.current_plan.acceptedShapes` widens to `['N', 'N of M']`, which is what `src/state-md-schema.cts`'s own comment instructed this PR to do on merge. `'N/M'` stays undeclared so row 23 keeps a non-vacuous undeclared candidate to probe. Verified `gen-state-md-docs --check` exits 0 and `--write` rewrites 0 of 6: the generated artifacts do not surface this row, so there is nothing stale to regenerate. B2/M4 — the discriminator was `/of\s+(\d+)/`, unanchored, so a total could be read out of prose. `Current Plan: 4 — blocked on review of 2 PRs` parsed as `4 of 2`, took the `currentPlan >= totalPlans` branch and WROTE `Status: Phase complete — ready for verification` into the user's file. Both shapes are now anchored at the start and every number comes from a capture group via `planNumberFrom`, which rejects anything past `Number.MAX_SAFE_INTEGER` rather than letting `data` and the persisted string disagree. Nothing on this path calls `parseInt` on a raw field value any more. The grammar keeps a trailing remainder after the total, because `Plan: 2 of 5 in current phase` is a real tested shape. The refusal comes from requiring `of <total>` to follow the leading number immediately, not from forbidding a suffix. M1 — `bumpLeadingNumber` is total. It returned its input unchanged when there were no leading digits, so `+2` reported `advanced: true` while writing the file untouched. M2 — both section arms use replacer functions. File-derived text was being spliced into a `String.replace` replacement string, where `$&` / `` $` `` / `$'` expand: a value of `04 of 06 $&` spliced part of the document into itself. `stateReplaceField` already used a function; these now agree with it. M3 — the section arm targets the name the SECTION carries, and the body write now writes both spellings, each with its own rendering. Keying off the header's name left the other name stale in both directions: a legacy header beside a `Current Plan:` section line, and a `**Plan:**` header beside one. * fix(#3784): derive the shape error from the schema, widen the test coverage Review round 3 on #3791: B3, m1, m2, m5 and the two test nits. B3 — the accepted-shape set had two owners: the parser branches and an English list hand-written beside them in `state.cts`. Nothing coupled them, so adding a branch left the message stale and removing one left it advertising a shape that errors, with no test able to see either. The message is now built from `STATE_FIELD_SCHEMA.current_plan.acceptedShapes`, and the CLI test walks the schema instead of restating the list. `Plan: N of M` is still spelled out explicitly because no schema row owns the body-only `Plan` field — `buildStateFrontmatter` never reads it into frontmatter, so it has no key to hang a row on. m1 — the property drove only the pre-existing `**Plan:**` branch, i.e. not the branch under review. It now drives both compound spellings and ranges past 99 so the width transition is covered by the property rather than one example. A second property covers the legacy pair's own preservation contract. Both were mutation-checked: dropping the padStart turns 9 tests red. m2 — degenerate boundary fixtures around the threshold (`0 of 0` is phase-complete, not an error; `0 of 3` advances) plus the shapes the anchored grammar must refuse, including Arabic-Indic digits. m5 — `docs/json-errors.md` described rather than quoted the message, since it is now schema-derived and a verbatim quote would be a third owner. Nits — the CRLF assertion could not see a `\n` at index 0; the `!/^Plan:/m` presence proxy is now an identity assertion on the whole `## Current Position` body. * fix(#3784): give the section plan write its own flag, and stop narrowing what parses Review round 4 on #3791: Blockers 1-4, Majors 1-2, Minors 1-2. B1 — the section fallback was guarded by `!mutated`, and `mutated` is FUNCTION-wide, already set by the phase/status/lastActivity arms that `advancePlanCore` always populates. A section spelling the field bold or as a pipe-table row therefore skipped its fallback because an UNRELATED field had been refreshed, and stayed a plan behind the header — the split-brain document this arm exists to prevent. The arm now tracks its own `planWritten`. Worth recording: the reviewer's fixture does not reproduce. The body-level status write lands on the section's own `Status:` when the document has no header `Status:`, so `mutated` is still false by the time the plan arm runs and the fallback fires. The discriminating shape needs a header `Status:` to absorb that write AND a bold section plan line. The mechanism was right; the example was not, and the regression test uses the shape that actually fails. B2 — `fallbackName` chose one name by ternary. In the legacy shape both values are populated, so it always chose `Current Plan` and a `**Plan:**` section line — which base did write — got nothing. Each name is now attempted independently with its own fallback. B3 — `PLAN_SHAPE_N` was anchored harder than `PLAN_SHAPE_N_OF_M`, so values base parsed via `parseInt` began to hard-error: `Total Plans in Phase: 5 phases`, `Current Plan: 3 (blocked)`. #3784's brief puts normalizing plan numbers beyond this transition's read/write out of scope, so that narrowing was not licensed. Both grammars now carry the same trailing tolerance. The prose defect stays closed by the START anchor, not by forbidding suffixes. Major 1 — the whole-body `Plan` write is scoped to documents that declare a `Plan` field, instead of firing unconditionally where `stateReplaceField`'s first match could be prose outside `## Current Position`. Major 2 — the error message names both `Plan` spellings the parser accepts; it previously omitted the sibling-paired form, which is the same message-disagrees-with-parser drift the derivation exists to close. B4 — the changeset claimed a guarantee B1 broke; it now describes what ships. Minors — safe-integer boundary coverage at limit-1/limit/limit+1, and the CRLF comment states the real mechanism (`stateExtractField`'s `(.+)` stops before the CR; the trailing group is belt-and-braces, not the primary defence). All three blocker regression tests verified red against the pre-fix source. * test(#3784): pin the hybrid shape against #3807's ambiguity refusal #4028 landed `advance-plan`'s multi-`Phase:` refusal on `next` after this branch's last run, on the same function. The guard sits above the parse, so a refused document is never parsed and the shape #3784 adds cannot reach the mutation — but that is a property of source ordering, so assert it as behaviour instead. Fail-first proven, not assumed: with `phaseCandidates.length > 1` disabled, the ambiguous hybrid document advances its FIRST entry's `Current Plan: 04 of 06` to `05 of 06` and writes it — #3807's exact defect, reached through #3784's shape. Both tests go red; both go green with the guard restored. The control pins the other direction: an unambiguous hybrid section still advances, and its zero-padding still survives. * fix(#3784): advance every spelling from its own text, refuse when they disagree Round 6 review. B1 and M1 are one defect, so they are one fix. `advancePlanCore` picked one field to parse from, computed `newPlan`, then wrote BOTH spellings from that field's numbers. Two symptoms: B1 With `Plan` as the parse source, `Current Plan` was re-stamped with the number just derived from `Plan`. `Current Plan: 7` beside `Plan: 2 of 5` silently became `Current Plan: 3` — a value nothing derived for that field, no error, no diagnostic. M1 With the legacy pair winning, the `Plan:` line was re-rendered from a bare `${newPlan} of ${totalPlans}` built out of the sibling field. `Plan: 2 of 9` became `3 of 5`; `Plan: 03 of 05` became `4 of 5`. The changeset's claim that padding and everything after it survive was true only for whichever field happened to be the parse source. Now: every spelling is advanced from its own raw text via `bumpLeadingNumber`, so each keeps its own padding, its own total and its own trailing annotation. Differing TOTALS are preserved, not reconciled — `Plan: 2 of 9` beside a `Total Plans in Phase: 5` advances to `3 of 9`. Differing CURRENT numbers are refused, with `reason: "ambiguous_plan_position"` and both candidates named. Same posture as #3807's multi-`Phase:` guard one field over: name the conflict, let the caller resolve it, never pick. The guard sits immediately after the parse, BEFORE the phase-complete branch — guarding only the normal advance would let `Current Plan: 7` beside `Plan: 5 of 5` write a terminal "Phase complete" into a document whose two spellings never agreed. A field present but unreadable (`Plan: TBD`) is left exactly as authored. Refusing the whole document because an unrelated line cannot be read would be a narrowing #3784 does not license; writing a derived number over it is the fabrication B1 was filed for. The `planSourceField`/`planRawValue`/`useCompoundFormat` tracking is gone. It existed only so the write path could ask which field the value came from, and the write path no longer asks. M2. The bare `Plan: N` + `Total Plans in Phase: M` shape is dropped. A revision of this PR added it; base refused it. It cannot be given the schema-row + forcing-test coupling the other shapes have, because `Plan` is body-only and `buildStateFrontmatter` never reads it into frontmatter, so there is no `current_*` key to hang a row on. Parser, the spelling in `advancePlanShapeError`, and the lockstep test move together — the invariant is the lockstep, not the length of the list. N1. The whitespace narrowing (`5phases` no longer parses where `parseInt` read 5) is documented in the changeset beside the other deliberate narrowings, rather than loosened. Loosening restores the half-parse this change exists to remove. Tests: eight new cases plus a property that crosses the two spellings with agreeing and disagreeing numbers — the review noted the existing properties never did. Fail-first proven: restoring the old write path reddens seven of the eight, both new property arms, and two pre-existing padding tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H3eK225hgcnEDZsnmtaP1U * fix(#3784): report Current Plan as updated only when it was written The write became conditional in the previous commit — a `Current Plan:` that is present but unreadable is left as authored — but the `updated` push stayed unconditional, so `transitionCore` reported a field it had not touched. `reconcileReportedFields` would have caught it against the persisted bytes at the `state.cts` caller, but `transitionCore`'s own `updated` is consumed directly (milestone-lock, the transition tests) and has to be true on its own. Covers the mirror of the unreadable-spelling case: `Current Plan: TBD` beside a readable `Plan: 2 of 5`, where `Plan` is the parse source and the legacy field is the one that cannot advance. Fail-first proven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H3eK225hgcnEDZsnmtaP1U * test(#3784): account for the new refusal in the output({error}) census `tests/io.test.cjs`' A3 census asserts the exact population of `output({error})` call sites in `src/`, per module. The `ambiguous_plan_position` refusal added a 27th to `state.cts`, so the census went red at 26/65. Updated the way #3807 updated it when it added the ambiguous-POSITION error one line above: bump the count and name the addition inline, so the next person reads why the number is what it is. The alarm did its job — it is the only gate that noticed a new user-visible error path had been introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H3eK225hgcnEDZsnmtaP1U --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
|
7c116b1c17 |
fix(#3697): warn when the phase-complete Requirements-line tokenizer under-selects REQ-IDs (#3744)
* fix(#3697): warn when the Requirements line under-selects REQ-IDs
`cmdPhaseComplete` tokenizes ROADMAP's `**Requirements**:` line by splitting
on `[,\s]+` and keeping tokens matching the anchored REQ-ID shape. That is
correct for the canonical comma list the template ships, and silently wrong
for every other form:
`RANGE-01 … RANGE-05` -> the two ENDPOINTS only; the interior IDs are
never considered, yet `requirements_updated`
reports true with zero warnings
`RANGE-01…05` -> ZERO IDs; the whole line is inert
The silence is structural: the only cross-check, `ghostReqIds`, is itself
`citedReqIds.filter(...)`, so an ID the tokenizer dropped is invisible to it
by construction — and to `traceabilityWriteMisses` and `requirements_updated`
with it.
Warn on both paths. This does not add range support: the selected set is
unchanged, so no existing ledger write changes. The trigger is ID-SHAPED
EVIDENCE only — an ID-shaped substring the tokenizer did not select, or a
range operator joining two IDs — with parenthetical citations and HTML
comments stripped before the scan, so the #2334/#2339 over-warning on
`None`, on the shipped `<!-- brackets optional -->` template comment, and on
annotated lines cannot return.
Regression tests extend the #2316/#2334 fixture family in tests/phase.test.cjs
(10 cases: 4 defect, 2 canonical controls, 4 negative-space controls).
Fixes #3697
* fix(#3697): rework under-selection detection onto tokens, not a free-text scan
Round 2, driven by the P4.6 cross-AI review (codex, gpt-5.6-sol) of b3ce71cb.
That review refuted 5 of 9 claims; three were false-positive classes in exactly
the category #2334/#2339 had to REMOVE:
`RANGE-01, RANGE-02 - 3 points` the bare-hyphen alternative read
`RANGE-02 - 3` as a range
`REQ-01, REQ-02 — locked per ADR-7.` the trailing period kept `ADR-7.` out
of the anchored filter, so the
unanchored substring scan reported it
as unparsed
`REQ-01, REQ-02 (see (ADR-7), then ADR-8)`
nested parens left `ADR-8)` behind
Replaces the free-text substring scan + loose range regex with three narrow,
token-based rules (R1 range-shaped token, R2 pure range operator flanked by two
selected IDs, R3 zero-selection with ID-shaped text). Also fixes the review's
CLAIM 9: the warning said IDs were "marked complete" when a ghost range marks
nothing — it now says "selected".
Side effect: the two false NEGATIVES the same review found are now covered —
`RANGE-01 through RANGE-05` and a parenthesised `(plus RANGE-02..RANGE-05)`.
NOT YET DONE (see the handoff prompt): regression tests for the four false
positives, the two new true positives, and the #3697-4 tightening the review's
CLAIM 8 asked for (it currently filters on the warning's phrasing rather than
asserting silence). Verified so far: tsc clean, the 10 existing #3697 tests
green, and a 20-case standalone harness covering every case above.
* test(#3697): pin the v2 token-detector boundary end-to-end
Six new cases + two hardenings for the review findings against v1:
- #3697-1 gains the worded spaced range (`RANGE-01 through RANGE-05`) —
the operator set's `to|thru|through` arm was previously untested.
- #3697-5 (new): a tight range hidden inside balanced parentheses
(`RANGE-01 (plus RANGE-02..RANGE-05)`) warns, names the range token,
and ticks exactly RANGE-01 — the paren shave must not hide it.
- #3697-4 gains the four false-positive classes a free-text detector
produced: numeric estimate (`- 3 points`), date annotation, em-dash
citation with trailing period (`— locked per ADR-7.`), and nested
parenthetical citations.
- #3697-3 and #3697-4 now assert the ENTIRE warnings channel is empty,
not that one phrase is absent — a re-worded over-warning cannot pass.
Negative control: against the merge-base with its lib rebuilt, all 6
defect tests fail and all 10 controls pass.
* fix(#3697): close round-2 review findings — annotation false positives
Round 2 of the adversarial review (against 822a72a04) refuted five
claims; this closes the false-positive class and the cheap misses:
- R1's bare-hyphen arm now demands a full ID on BOTH sides
(`REQ-01-REQ-05`): `LETTERS-\d+-\d+` is also a date-like annotation
(`FY-2026-08`) and a sub-numbered ID, and warning on those is the
expensive class. Tight hyphen shorthand with a live selection is the
disclosed false negative; at zero selection R3 still catches it.
- R2 requires the endpoint pair to imply an INTERIOR (same prefix,
gap > 1): `REQ-02 - REQ-03` selects both endpoints and can drop
nothing, so an annotation hyphen between adjacent IDs stays silent.
- R3 skips placeholder-led lines: `None (per ADR-7)` is a declared-empty
line citing its rationale, not unparsed residue.
- Token shave: quotes/backticks now shaved from alphanumeric tokens
(`` `RANGE-02..RANGE-05` `` warns); punctuation-only tokens get a
bracket-only shave so `(..)` surfaces its operator.
- 256-char token cap bounds the quadratic unanchored substring test.
- Warning text mentions range expansion only when a range rule fired.
Tests: 6 new cases (22 total). Negative control against the merge-base:
8 defect tests fail, 14 controls pass.
* fix(#3697): close round-3 review findings — half-spaced ranges, cross-prefix annotations, markdown wrappers
Round 3 of the adversarial review (against 2eb92dd0e) refuted four
claims; this closes them:
- Half-spaced ranges (`REQ-01 -REQ-05`, `REQ-01- REQ-05`) split at the
tokenizer before R1's `\s*` can see them and under-selected silently.
A glued-fragment rule warns when an operator is glued to a full ID
with an ID-shaped neighbour on the open side and the endpoint pair
implies an interior.
- Cross-prefix pairs around a separator no longer read as ranges:
`REQ-02 - (ADR-7)` and `REQ-02 (...) (ADR-7)` are annotations, and
real ranges are same-prefix by nature. `impliesInterior` now returns
false on prefix mismatch and computes the gap with BigInt (parseInt
lost precision past 2^53).
- The token shave now removes markdown emphasis markers and curly
quotes, so `**None** (per ADR-7)` reaches the placeholder gate and
`**RANGE-02..RANGE-05**` reaches R1.
- The unanchored-substring cap rises to 2048 (a markdown-link range
with a long URL cleared 256); the anchored range regexes scan
linearly and drop their cap.
Tests: 6 new cases (28 total; 377/377 file-wide). Negative control
against the merge-base: 11 defect tests fail, 17 controls pass.
* fix(#3697): round-4 review finding — word operators excluded from glued-fragment rule
`TOREQ-05` is a valid prefix-agnostic REQ-ID, and the glued-fragment
rule read it as `to` + `REQ-05`, warning on the canonical two-ID list
`REQ-01, TOREQ-05`. Glued fragments are now SYMBOL-operator-only
(`..`+, ellipsis, dashes): a word operator glued to an ID is an ID,
not a range spelling.
Tests: word-operator-prefixed ID control (misparse channel silent; the
fixture's ghost-ID warning legitimately fires, so the whole-channel
assertion stays with the registered controls) and an underscore-wrapped
tight-range defect case. 30 targeted cases; 379/379 file-wide; negative
control: 12 defect tests fail on the merge-base, 18 controls pass.
* fix(#3697): round-5 review findings — trailing word-op glue, dot shave, honest wording
- The glued-fragment TRAILING arm takes the word operators back: an ID
must end in digits, so `REQ-01through` can never be an ID — the
round-4 TOREQ collision was leading-arm-only, and symbol-only on both
arms lost the `REQ-01through REQ-05` typo class.
- A trailing run of 2+ dots survives the punctuation shave: `REQ-01..`
is a glued range operator, not sentence punctuation, and the shave
was silently eating the `REQ-01.. REQ-05` form.
- The warning now says the line "could not be parsed as" a
comma-separated REQ-ID list: `**REQ-01**, **REQ-05**` IS such a list
— the selector just cannot parse decorated tokens — and a warning
that misstates the input teaches readers to distrust it.
Tests: two new trailing-glue defect cases (32 targeted; 381/381
file-wide). Negative control: 14 defect tests fail on the merge-base,
18 controls pass.
* test(#3697): use t.after for cleanup per CONTRIBUTING test ruleset
CONTRIBUTING bans try/finally inside test bodies (it masks failures);
the approved shape is `t.after(() => cleanup(tmpDir))`. All seven
converted tests are this PR's own additions; the file's pre-existing
instances are untouched.
* chore(#3697): add changeset fragment for the Requirements-line under-selection warning
changeset-lint fails on this PR (fail_missing_fragment): src/phase.cts is a
user-facing surface and the branch carried no .changeset/*.md. Adds the Fixed
fragment via `npm run changeset -- --type Fixed --pr 3744`, symptom-led per
the house format, with the (#3697) backlink.
* refactor(#3697): extract the Requirements-line detector to a testable surface
Round-3 review Blocker 1 requires a fast-check property test over this
detector (`RULESET.TESTS.property-based-testing`: modules implementing
parsing contracts must include at least one), and Blocker 2 requires
limit-1/limit/limit+1 fixtures on its 2048-char token cap
(`RULESET.TESTS.boundary-coverage.fixtures`). Neither is expressible while
the logic is a closure inside `cmdPhaseComplete`: every existing #3697 test
reaches it by spawning the CLI, and a property test cannot pay a subprocess
per generated case.
So the selector and the three detection rules move to module scope as
`analyzeRequirementsLine` (pure, exported) plus
`formatRequirementsLineWarning`, and `cmdPhaseComplete` calls them. This
commit changes NO behaviour: `tests/phase.test.cjs` is untouched here, and
the pre-round suite passes against it unmodified (403/403).
Two things the move makes explicit rather than incidental. The selector and
the detector tokenize the SAME line DIFFERENTLY — the selector strips only
`[` and `]`, the detector also shaves quotes, emphasis and trailing sentence
punctuation — and that gap is deliberate: it is why `ADR-7)` is not selected
while `ADR-7` is still nameable in a warning. They now sit adjacent with the
reason written down, so they cannot drift apart silently.
And the stale citations in the moved comment are corrected. It pointed at
src/phase.cts:833,920,1078 for the `**Requirements**: TBD` seeds, which had
drifted to 1132/1237/1413, and at `templates/roadmap.md:32`, which is
`gsd-core/templates/roadmap.md:32`. Both are now anchored by content.
* fix(#3697): stop the warning claiming a misparse that did not happen
Round-3 review Major 3 and Minor 4. Both are the same defect: the warning
asserted more than the evidence supported.
MAJOR 3 — a correct comma list such as `RANGE-01, RANGE-02 — RANGE-05
deferred` warned "could not be parsed ... Range forms are not expanded;
rewrite the line". Reproduced: it selects RANGE-01, RANGE-02 AND RANGE-05,
i.e. every ID written on the line. Nothing was dropped, and the pinned
control only stayed silent because its pair was ADJACENT (gap == 1), so the
control was passing by accident of the fixture rather than by the rule.
The obvious fix — go silent — is not available. `RANGE-02 — RANGE-05` as a
range and as an annotation separator are textually identical, and no
token-level rule separates them; staying quiet re-opens the exact silent
under-selection #3697 is about. Deciding the ambiguity by assertion in
either direction is wrong. So it is DISCLOSED: the warning now has two
channels, chosen by whether any ID-shaped token was actually left unselected
(`droppedIdShaped`).
* something was dropped (tight range, glued fragment, inert residue)
-> "could not be parsed as a comma-separated REQ-ID list", as before.
* nothing was dropped (only the spaced-operator rule fired)
-> "contains what reads as a range between two cited REQ-IDs", stating
both readings and saying explicitly that an annotation separator means
the line is already correct.
This retires the "could not be parsed" wording for the four #3697-1 spaced
cases too, and that is a deliberate expectation change rather than a fix
counted twice: those lines never failed to parse either. They still warn,
still name the selected IDs, and still assert the endpoint-only marking is
unchanged; #3697-1 now also asserts the misparse channel stays SILENT.
MINOR 4 — `Deferred (see ADR-7)` reported `Unparsed text: ADR-7`, naming a
citation as requirement content it had failed to read. The trigger is
correct and stays: #3697's acceptance criterion asks for a warning "when it
selects zero IDs from a line that is non-empty and is not the `TBD`
placeholder", and inferring placeholder-ness from arbitrary prose is the
free-text heuristic this detector exists to avoid. What was wrong is the
wording, so the non-range arm now says "ID-shaped text that was not
selected" and names the escape the author actually has (`TBD` / `None`).
Tests: #3697-9 (three spaced forms — must warn, must NOT claim a misparse,
must offer both readings) and #3697-10 (`Deferred (see ADR-7)`, `N/A
(tracked in ADR-12)` — must warn, must not say "Unparsed text", must not
diagnose a range, must name the placeholder escape).
Reversion control: reverting the ambiguous channel fails #3697-9 (3 named
tests); reverting the R3 wording fails #3697-10 (2 named tests).
* fix(#3697): cap every token predicate, complete the dash set, cover the boundary
Round-3 review Blocker 2 and Nit 6, plus one self-found finding. All three
are about the detector's own predicates, so they land together.
BLOCKER 2 — the 2048-char budget had no boundary coverage.
`RULESET.TESTS.boundary-coverage.fixtures` requires limit-1 / limit /
limit+1 for any budget parameter. #3697-B1 and #3697-B2 now exercise 2047 /
2048 / 2049 against BOTH predicate families the cap guards, and each asserts
its fixture's exact length before asserting behaviour, so a mis-built
fixture fails loudly rather than passing at the wrong size. Clause (d) of
that rule — an input pushed within reserve-distance of the limit — has no
referent here: this is a hard cap with no reserve constant beside it, and
the test comment says so rather than leaving the omission to be re-derived.
NIT 6 — the cap guarded only the unanchored ID-substring regex. The
anchored range regexes were left uncapped, justified by a comment asserting
they scan linearly. The finding is right that this is informational (they
are anchored; the input is a local ROADMAP.md), but an asserted property is
cheaper to enforce than to defend, so all three predicates now share one
`short()` guard. #3697-B2 is what pins it: at 2049 the anchored scan must
now decline to classify.
SELF-FOUND (RV4 guard-shape census) — the range-operator set is a list this
code fixes at author time over a domain that grows without it, so the round
owes a census of what the enumeration reaches.
reached: `..`+, U+2026, U+2013, U+2014, ASCII `-`, to/thru/through
NOT reached: U+2010 hyphen, U+2011 non-breaking hyphen, U+2012 figure
dash, U+2015 horizontal bar, U+2212 minus sign
consequence: a range spelled with any of those is SILENTLY under-selected
— #3697's own defect, in the code that exists to fix it
Those five close. They are the same operator at a different codepoint and
carry none of the ASCII hyphen's collision risk, because they are not the
REQ-ID separator: `FY-2026-08` is date-shaped only with ASCII hyphens, so a
U+2010 never reaches the ID shape. They therefore join the NOHYPHEN arm
beside `—` and `–`; the strict full-ID-both-sides shape the bare hyphen is
held to is untouched, and #3697-12 pins that.
Still NOT reached, declined with reason rather than left unstated: `→`, `~`,
`..=`, `..<`, `until`, and `up to` (two tokens, so never one operator
token). Each is a symbol or word with an independent non-range use between
two REQ-IDs — the over-warning class #2334 cost three rounds.
Reversion control: reverting the uniform cap fails #3697-B2 (limit+1);
reverting the dash set fails #3697-11 (5 named tests).
* test(#3697): add the fast-check property coverage the parser rule requires
Round-3 review Blocker 1. `RULESET.TESTS.property-based-testing` (CONTEXT.md)
requires modules implementing parsing contracts to carry at least one
fast-check property test asserting a domain invariant, and the round-2 diff
had zero occurrences of `fc.` across its +311 test lines. Five properties,
1,900 generated cases:
P1 soundness of silence (boundary containment) — for ANY canonical comma
list of well-formed REQ-IDs, the selected set EQUALS the written set
and nothing warns. This is the #2334 over-warning invariant and the
#3697 under-warning invariant asserted as one statement, over
generated IDs rather than hand-picked ones. It generalises #3697-4b:
a prefix beginning with a word operator (`TORANGE-05`) is an ID, and
P1 covers that class rather than the single example.
P2 completeness — a same-prefix pair with an interior between them,
separated by any of the nine spaced operators, ALWAYS warns.
P3 the #2334 invariant — an ADJACENT pair around a separator can drop
nothing, so it stays silent however it is annotated.
P4 totality + idempotency — total over arbitrary strings, deterministic,
and the formatter agrees with the analysis on whether there is
anything to say (a warn with no text, or text with no warn, is a
channel that can go silent or noisy on its own).
P5 containment — every selected ID is ID-shaped and appears verbatim in
the input.
Honest scoping, since a property test is easy to overclaim: P1, P3, P4 and
P5 hold against the round-2 code as well as this one — they are regression
guards, not bug-finders, and their value is that the invariants are now
stated and generatively checked rather than implied by examples. P2 is the
one that would have failed before the dash enumeration was completed.
fast-check v4 removed `fc.stringOf`, so the ID-prefix tail is built from
`fc.array(...).map(join)` with the alphabet pinned to the selector's own
`[A-Z0-9]` class.
These live in tests/phase.test.cjs rather than a new
`phase.property.test.cjs`: `lint-test-file-count` caps a production module
at 2 test files and phase.cts is already at its allowlisted entry, so a new
file would trade one gate for another.
* docs(#3697): document the ROADMAP Requirements-line grammar
Round-3 review Minor 5 — the change adds net-new user-visible warning output
for a grammar constraint documented nowhere under docs/. `type: Fixed` is
docs-exempt so this does not block, but a warning about a rule the reader
cannot look up is not actionable, and that is worth fixing whether or not a
gate demands it.
Added as a subsection of `phase complete` in docs/CLI-TOOLS.md, beside the
existing SUMMARY artifact-check advisory it is a sibling of: the supported
comma-list form, why ranges are deliberately not expanded, that `TBD` and
`None` are the entire placeholder vocabulary, and what each of the two
warning voices means — including that the range/annotation one may be
reporting a line that is already correct.
Existing file rather than a new one, deliberately: docs/ carries generated
indexes and zh-CN / ja-JP trees, and a new top-level page invites a parity
or index gate this change has no reason to touch.
* fix(#3697): rule-scope the warning-channel discriminator
Self-found at the round's pre-push review, against the Major 3 fix two
commits back. That fix chose the channel from a LINE-GLOBAL question — "was
any ID-shaped token left unselected?" — while the rules that produce the
warning are not line-global. The two disagree as soon as the line carries an
ID-shaped token no rule fired on:
`RANGE-01, RANGE-02 — RANGE-05 deferred per (ADR-7)`
`(ADR-7)` survives the selector's bracket strip, so the global test called it
a drop and sent the line to the assertive channel — putting the false "could
not be parsed ... rewrite the line" claim back on a correct line. That is
review finding Major 3 returning through a side door, and it directly
contradicts #3697-4, which pins a parenthetical citation as NOT unparsed
residue.
The discriminator is now rule-scoped: R2 is the only ambiguous rule, so the
ambiguous channel requires that R2 fired, that no other rule did, and that
every endpoint R2 fired on was actually selected. The last conjunct is not
redundant — the detector shaves brackets and the selector does not, so R2 can
fire on a `(RANGE-02)` that was never selected, and that IS a drop:
`RANGE-01 (RANGE-02) — RANGE-05` -> assertive, correctly
`droppedIdShaped` is replaced by `spacedRangePairs` (R2's hits, so the
channel can ask about the endpoints the rule fired on) and the
`rangeReadingOnly` verdict.
Reversion control: against the line-global rule, #3697-9b fails. #3697-9c
passes under both rules — there the dropped token IS the R2 endpoint, so the
two agree; it is a regression guard, not a bug-finder, and is recorded as
such rather than counted as a second control.
* fix(#3697): hold every dash to the strict range shape, not just ASCII
Self-found at the round's pre-push review, and it CORRECTS a claim made two
commits back. That commit widened the range-operator set by five Unicode
dashes and asserted they "carry none of the ASCII hyphen's collision risk,
because they are not the REQ-ID separator". That reasoning was wrong. The
collision is a property of the SHAPE — `PREFIX-\d+ <dash> \d+` is also a date
(`FY-2026-08`) and a sub-numbered ID (`API-2-01`) — and the shape does not
care which dash sits in the operator slot, because the ID's own separator is
still ASCII either side of it. Measured:
RANGE-01 (target FY-2026-08) silent <- pinned by #3697-4
RANGE-01 (target FY-2026‐08) WARNED <- same line, U+2010
So the widening reintroduced the #2334 over-warning class on a date
annotation. It also exposed that the inconsistency PREDATES this PR: U+2013
and U+2014 were already in the loose arm at ce71dd399, so the en- and em-dash
forms of that same date annotation warned before round 3 ever ran.
One rule for every dash: a tight range spelled with any of the eight must
carry a FULL ID on both sides, exactly as the bare hyphen already had to.
`..`, `…` and the word operators stay loose — no date or sub-number reading
exists between two numbers, so the strict shape would cost them coverage for
nothing.
The cost is a false negative, and it is one the design already accepts:
`RANGE-01, RANGE-02-05` is silent today, deliberately, and now
`RANGE-01, RANGE-02–05` is too. That removes an inconsistency rather than
opening a gap, and a bare `RANGE-02–05` still warns — it selects nothing, so
R3 catches it.
Tests: #3697-13 (date annotation AND sub-numbered ID silent for all eight
dashes), #3697-13b (full-ID tight range still warns for all eight),
#3697-13c (loose operators keep their numeric endpoint), #3697-13d (the
accepted false negative is symmetric, and the bare zero-selection line still
warns).
* fix(#3697): close the round's own pre-push review findings
An adversarial cross-AI review of this round refuted 4 of its 10 claims. All
four were real. Every fix below is to code THIS round introduced.
1. THE SOFT VOICE CLAIMED TOO MUCH (refuted CLAIM 1).
`REQ-01, (REQ-02), REQ-03 — REQ-05` took the range-reading voice and told
the author "the line is already correct and nothing needs to change" — while
`(REQ-02)` had been dropped by the selector, which does not strip
parentheses.
The channel choice is still right, and deliberately so: `(ADR-7)` and
`(REQ-02)` are the SAME shape, so routing on "was anything unselected?" puts
the false "could not be parsed" claim back on a line carrying a citation —
the misroute fixed two commits ago. No rule can adjudicate this; the author
can. So the voice stops asserting the line is correct (it now speaks about
the SEPARATOR, which is all it has evidence about), and BOTH voices gained a
factual clause naming ID-shaped text the selector skipped, with the reason
(brackets are not stripped) and no verdict attached.
2. THE CAP SILENCED A LINE THAT USED TO WARN (refuted CLAIM 2).
A 2049-char range token warned before this round and went silent after it:
the "uniform cap" commit bounded the predicate and, with it, the warning.
That is #3697's own defect, introduced by the fix for a nit.
The cap bounds the WORK, not the warning. An over-cap token carrying `-` is
now recorded as unclassified (a linear `includes`, never the unanchored
regex the cap exists to keep off it) and gets its own voice: "could not be
checked ... the REQ-ID selection on this line is unverified". Unclassified
is reported, never treated as clean.
3. THE CAP WAS NOT UNIFORM (review MISSED finding).
R2 capped the operator token but not its neighbours, so
`<2049-char ID> .. <2049-char ID>` still ran REQ_ID_SHAPE_RE and BigInt over
both endpoints unbounded. The glued rule had the same hole. Every
participant is capped now.
4. PROPERTY P5 WAS VACUOUS (refuted CLAIM 5).
It drew from a bare `fc.string()`, which over 500 samples produced max
length 10 and ZERO inputs containing a REQ-ID — the loop body never executed
an assertion. A containment property that never contains anything is a green
test measuring nothing. The generator now interleaves real IDs with noise
and the property ASSERTS it saw them (>50/500), so it can never silently go
vacuous again. The free-form coverage it was actually providing survives,
honestly labelled, as #3697-P6.
The same finding refuted this round's claim that P2 distinguishes pre-round
behaviour: every operator P2 uses was already in the pre-round operator set.
P2 is a regression guard, and its comment now says so.
Also: docs/CLI-TOOLS.md repeated the broken channel claim verbatim (review
MISSED finding) and is corrected with the code.
Tests: #3697-9d (soft voice names the skipped ID, never claims the line is
correct), #3697-9e (over-cap token reported as unclassified, still warns),
#3697-9f (R2 and the glued rule cap their neighbours). #3697-B1/B2 now key the
boundary on the PREDICATE's verdict with `warn` asserted true at every length —
asserting `warn === false` at limit+1 was itself finding 2.
* docs(#3697): describe the third voice and the dash rule
Follow-on to the review-findings commit: that commit corrected the docs' claim
about the soft voice but left two things the code now does undescribed.
- There are THREE voices, not two. The over-cap voice ("could not be checked
... unverified") arrived with the fix for the review's CLAIM 2 and had no
entry.
- Dash spellings require a full ID on both sides, and `..` / `…` / the word
operators do not. That asymmetry is deliberate and load-bearing —
`PREFIX-<digits><dash><digits>` is date- and sub-number-shaped — so a reader
hitting `REQ-01-05` and getting silence has no way to find out why. The
accepted cost (`REQ-01, REQ-02-05` unreported, bare `REQ-02-05` still
reported) is stated rather than left to be discovered.
Documentation only; no behaviour change.
* fix(#3697): close the continuation review's findings
A continuation of the same adversarial reviewer, run against the reworked
round, refuted 6 of 7 claims. Four were real defects in this round's own work
and are fixed here; the other two are answered rather than changed, below.
1. THE SKIPPED-TEXT CLAUSE WAS ON ONE VOICE, NOT BOTH (refuted CLAIM A).
The previous commit's message said both voices gained it. Only the soft
return appended it. The assertive voice now carries it too — and, because
that voice already names range tokens and inert residue under its own
clauses, the note is filtered to what those did not already name. A warning
that says the same token twice is one readers learn to skim.
2. THE CLAUSE'S WORDING WAS FALSE (also CLAIM A).
It read "brackets and parentheses are not stripped". Square brackets ARE
stripped by the selector — `[REQ-01, REQ-02]` is the documented form — so
only parentheses qualify. Corrected in the message and in docs/CLI-TOOLS.md,
which had inherited the same error.
3. THE OVER-CAP RULE STILL SILENCED A LINE (refuted CLAIM B).
`oversizedTokens` filtered on `includes('-')`, which misses an over-cap
OPERATOR: `REQ-01 <2049 dots> REQ-05` warned before this round, R2 declined
to classify it once capped, and nothing reported it. That is the exact
regression the field was added to close, one input over. Any token past the
cap now counts — what it contains is irrelevant when we could not read it.
4. AND THEN OVER-REPORTED ONE (review MISSED finding).
With (3) in place, a 2049-character CANONICAL REQ-ID was selected by the
uncapped, fully-anchored selector AND flagged "REQ-ID selection on this line
is unverified" — a contradiction inside one warning. A token the selector
took was examined end to end, so it is excluded.
Two findings are answered, not changed:
CLAIM C — the selector's own `REQ_ID_SHAPE_RE.test` is uncapped. True, and
deliberate: this round does not touch what gets MARKED, and the pattern is
anchored at both ends with no nested quantifier, so it is linear. The claim
that "all predicate paths are capped" was too broad; the DETECTOR's are.
CLAIM E — `REQ-01, REQ-02<dash>05` is silent for every dash. That is the
documented, deliberate cost of holding dashes to the strict shape, and it is
symmetric with ASCII, which behaved that way before this PR. The reviewer is
right that "without losing a range spelling that should be detected" was too
strong; a bare `REQ-02<dash>05` still warns.
Tests: #3697-9g (clause on the assertive voice, no repetition, bracket claim
true), #3697-9h (over-cap operator does not silence the line), #3697-9i (a
selected over-cap ID is never called unverified). #3697-9f is rebuilt — its
first version used the SAME id twice, so R2 could not have fired even uncapped
and it proved nothing; it now uses endpoints with a gap and fails when the
neighbour cap is removed.
Reversion control: all four fixes fail a named test when reverted in isolation
(#3697-9h, #3697-9i, #3697-9g, #3697-9f).
* fix(#3697): scope the over-cap exemption to what could actually pair
A second continuation of the same reviewer, against the reworked round,
confirmed the two claims that matter most and refuted three. This closes the
one real defect; the other two are answered below.
CLAIM J / CLAIM K (one defect, found from both directions). The previous
commit exempted EVERY selector-accepted token from `oversizedTokens`, on the
reasoning that the selector is uncapped and anchored so it examined the whole
token. True of that token's SELECTION — and not the same as "no rule was
suppressed by it". Two over-cap valid IDs either side of `..` are both
selected, so both were exempted, and R2 is capped: a line that warned before
this round went silent.
That is the third appearance of one class in this round — the cap suppresses a
check, and the suppression is not reported. Each fix for it over-corrected in
the opposite direction, which is why the rule is now stated in terms of what
was actually suppressed rather than in terms of the token: an over-cap token is
exempt only when it was selected AND nothing beside it could have paired with
it into a range (no range operator, no glued fragment, no second over-cap
token). Everything else is unexaminable and says so.
Two findings are answered, not changed:
CLAIM M — the reviewer demonstrated, with driven evidence, a contextual rule
that catches `REQ-01, REQ-02-05` while leaving `FY-2026-08` and `API-2-01`
silent: recognise `PREFIX-a<dash>b` only when another SELECTED id on the line
shares that prefix. That refutes this round's claim that the strict-dash
trade was FORCED, and the claim is withdrawn — it is a design choice. The
choice stands for this PR: the conservative rule is what ASCII already did
before #3697, adopting a new contextual heuristic unreviewed at the end of a
round is how the last three defects in this round were made, and #3697 asks
for a warning rather than better range inference. Named here so the
alternative is on the record rather than lost.
Docs MISSED — CLI-TOOLS said every token over 2,048 characters "is not
classified at all" and warns. Selection is not bounded; only range detection
is. Corrected.
Confirmed by the same pass, and worth recording because they are the PR's
load-bearing promises: a 20,000-input comparison of the pre-extraction selector
against HEAD found `mismatches=0` (nothing about which REQ-IDs are MARKED has
changed), and the uncapped selector regex was measured linear from 100k to 800k
characters.
Tests: #3697-9f now asserts the range case is reported rather than silent, and
#3697-9j pins the exemption's scope in both directions. Reversion control:
restoring the blanket exemption fails both.
* fix(#3697): warn on zero selection, as the acceptance criterion asks
`Deferred`, `N/A`, `Pending`, `TBA` and `-` selected no REQ-IDs and stayed
SILENT, while three shipped artifacts said they warned: `docs/CLI-TOOLS.md`,
the `placeholderLed` census comment, and the advice string the command emits
to the user. The asymmetry was the tell — `Deferred (see ADR-7)` warned,
because the citation supplied the ID-shaped residue R3 required, while bare
`Deferred` did not. The claim was written into three places and never
executed once.
This is also #3697's AC-1b/AC-4 verbatim: "warn when `citedReqIds.length ===
0` while the raw capture is non-empty and not `TBD`".
R3b keys on the SELECTION being empty, never on what the prose means, so it
adds no free-text heuristic. It is deliberately not gated on ID-shaped
residue the way R3 is, and the negative space is what settles that: all
fifteen #2334/#2339 fixtures are held silent by non-zero selection or by
`placeholderLed`, and not one of them by the ID-shape gate — measured, not
argued. The gate was buying no negative space while costing the acceptance
criterion.
`tokens.length > 0` keeps an empty line and a comment-only line silent: the
tokenizer strips `<!-- ... -->` before splitting, so the shipped template's
own comment cannot reach the rule.
Selection behavior is unchanged. This warns; it never invents an ID.
Also extracts `warn` to a named const (round 3 review Minor 3) — this commit
adds a disjunct to exactly that predicate, and in the return literal a later
reordering would be a TDZ ReferenceError rather than a reader-visible error.
Tests: #3697-14 (six zero-selection lines warn and tick nothing, and the
warning names the TBD/None escape), #3697-14b (five placeholder spellings
stay whole-channel silent), #3697-14c (comment-only line stays silent).
Fail-first controls: all six #3697-14 cases fail against the pre-fix tree;
-14b and -14c pass at both ends, which is correct — they pin silence the
widening must preserve.
* fix(#3697): name the REQ-ID a glued delimiter dropped
`RANGE-01; RANGE-02` selects only RANGE-02 and marks only RANGE-02, with
`requirements_updated: true` — #3697's own half-success failure mode, reached
by one wrong delimiter, and silent before this rule. It is the issue's AC-1a
("a warning whenever the line contains ID-shaped content that the tokenizer
did NOT select") at the shape most likely to be typed by accident.
Round 4 review rated this Major rather than Blocker on the ground that the
case is indistinguishable from a parenthesised citation, since `(ADR-7)` also
shaves down to a bare ID. At the RAW token level it is distinguishable, and
that is what makes the rule shippable: `REQ-01;` is shaved of a trailing
DELIMITER, `ADR-7)` of a citation wrapper. R4 keys on that shave class and
requires the token to sit outside any parenthetical.
Measured before implementing: 0 false positives and 0 false negatives across
21 probes, including all fifteen #2334/#2339 negative-space fixtures. A first
cut without the parenthetical test scored 3 false positives — every one of
them a colon inside a citation (`(see ADR-7: section 3)`) — which is why that
test is the rule's boundary rather than an optimisation.
Adds the delimiter census the module did not have. The range-operator domain
was already censused; the comma-substitute domain was not. Swept 26
spellings: exactly two produce a silent under-selection, `; ` and `: `. Every
other spelling either selects both IDs or selects none and already warns. The
review hand-listed the semicolon; the colon is the sibling that sweep found,
and it fails identically.
`rangeReadingOnly` now excludes an R4 hit — the ambiguous voice claims nothing
was dropped, and must not speak for a line where something demonstrably was.
Tests: #3697-15 (four delimiter shapes warn, name EVERY dropped ID, and tick
exactly the unchanged selection), #3697-15b (three citation forms stay
whole-channel silent). Fail-first control: all four #3697-15 cases fail
against the previous commit's tree; -15b passes at both ends, pinning the
boundary the widening must not cross.
* fix(#3697): give the Requirements-line warning a stable machine kind
The warning's kind existed only in the prose of its message, so every consumer
and every test had to regex an English sentence — and rewording a message
silently un-asserted the tests that pinned it. Round 4 review Major 3.
The repo already had the settled seam for exactly these semantics.
`CONTEXT.md` records `diffLiveConfig` emitting `kind:'unverified'` for a
truncated scan, which is precisely this module's third voice; and
`WAVE_CLEANUP_WARNING` in `src/worktree-safety.cts` carries codes for the same
reason. ADR-3473 Decision 3 ("failure is a value") points the same way.
`formatRequirementsLineWarning` now returns `{ code, message }` instead of a
bare string, which also settles round 4 Nit 3 — `null` still means CLEAN, a
legitimate value, but the success arm is no longer a naked string one field
away from the shape the ADR standardises on.
The kind is carried ALONGSIDE the prose, never instead of it. `warnings[]` is
a documented `string[]` in `phase complete`'s JSON output, rendered by
execute-phase.md's "If has_warnings is true" step, so re-typing its elements
would be a breaking output-contract change for a shipped command. The code is
emitted as its own additive `requirements_line_warning` field, absent
entirely when the line is clean.
Vocabulary, exported so tests key on it rather than on string literals:
`req-line-misparse`, `req-line-range-reading`, `req-line-unverified`.
Tests: channel ROUTING in #3697-9/-9b/-9c/-9d/-9e/-9g/-10 now asserts the code;
message-content assertions stay where the user-visible wording is itself under
test. #3697-16 pins the code end-to-end through the CLI's JSON for four line
shapes and asserts warnings[] is still a string[]; #3697-16b pins that a clean
line emits no kind at all, because a field present on every run carries no
information. #3697-P4 holds kind-and-message-appear-together and
kind-is-in-the-declared-vocabulary over arbitrary input, so a channel added
later cannot ship without one.
* test(#3697): pin the divergence against the second parser of the same line
CLAUDE.md, KNOWN DEFECTS & ANTI-PATTERNS: "Generative Fix Divergence: when
sharing constants/arrays/parsers between parallel surfaces, add a parity
assertion test that fails if they diverge." Round 4 review Major 2.
`normalizePhaseReqIds` (src/gap-checker.cts) parses the SAME ROADMAP
`**Requirements:**` value — its own docblock says callers "may pass the
roadmap value through verbatim" — and diverges on four axes. Measured, not
inferred:
line phase complete gap-checker
RANGE-01..RANGE-05 [] 5 IDs
None (per ADR-7) [] ["ADR-7"]
(REQ-02) [] ["REQ-02"]
REQ-01a [] ["REQ-01a"]
REQ-01, REQ-02 both both
This pins the divergence rather than removing it, which is the review's
second option and the correct one here: unifying the two would change what
`phase complete` MARKS, and "the ledger-writing set is byte-identical to base"
is the one invariant this PR holds fixed. Every axis is now asserted in BOTH
directions, so drift on either side fails here instead of widening silently.
The range axis is a DELIBERATE disagreement and is labelled as such — #3697
declines range expansion in terms ("I am not asking for range syntax to be
supported") while gap analysis adopted it under #1269.
The placeholder axis is the one worth reading twice: `None (per ADR-7)` is a
declared-empty line to `phase complete`, which reads the lead token, and a
one-requirement line to gap-checker, which strips parentheses first so the
citation survives its ID-shape filter. That is a citation being reported as a
requirement.
#3697-17b states the cost concretely: one line, five requirements in scope to
gap analysis and zero to phase complete. This PR is what makes that
contradiction visible, by finally giving the silent side a voice.
* fix(#3697): stop the skipped-text rider reporting a date, and close the 4b channel gap
Two round 4 review minors, both about a warning saying something it cannot
support.
MINOR 2 — false rider content. `REQ_ID_SUBSTRING_RE` is unanchored, so
`FY-2026-08` matches as `FY-2026` and lands in `unselectedIdShaped`.
`REQ_RANGE_TOKEN_RE`'s entire strict-dash arm exists to keep that shape
silent, and #3697-4 pins `RANGE-01 (target FY-2026-08)` as producing no
warning at all — but whenever some OTHER rule fired on a line that also
carried a date annotation, the rider told the author to "check whether any of
it is a requirement" about a date. Not a false warning, since the line was
warning anyway; false CONTENT, in the #2334 voice, through the side door.
Filtered at the MESSAGE rather than in the analysis: `unselectedIdShaped`
stays a faithful record of what the selector skipped — it is documented as a
fact that never routes — while the user-facing clause declines to assert
requirement-ness about a shape the design already ruled unadjudicable.
#3697-18b is the other half, so the filter cannot become a silencer: a
genuinely dropped REQ-ID is still named.
MINOR 1 — `#3697-4b` asserted only that the ASSERTIVE channel stayed silent,
so a regression routing `RANGE-01, TORANGE-05` into the AMBIGUOUS channel
would have passed. Whole-channel silence is not available on that fixture (the
pre-existing ghost-ID warning legitimately fires on the unregistered
`TORANGE-05`), so the precise assertion is that no Requirements-line warning
of ANY kind was emitted. The machine code added earlier in this round is what
makes that statable; before it, "both channels" could only have meant a second
prose regex.
* docs(#3697): record the Requirements-line seam in CONTEXT.md
CLAUDE.md names the CONTEXT.md glossary as a PR gate, and
`get_cochange_context(src/phase.cts, 45d)` ranks CONTEXT.md 4th at 25
co-changes — above src/init.cts and src/roadmap.cts. This PR introduced a
named seam, three warning kinds, a bound, a rule taxonomy and a deliberate
cross-parser divergence, and recorded none of it. Round 4 review Major 4.
The precedent is explicit rather than inferred: the directly analogous seam
is already there as `LIVE-CONFIG.GUARD.SEAM.truncation`, including its bound
and its boundary obligation — and that entry is the one this module's third
voice was modelled on.
Eight predicates, in the machine-oriented section beside it:
.module the two exported functions and the code vocabulary
.selector-identity citedReqIds is byte-identical to base and is the
only thing reaching the ledger — a change to what
phase.complete MARKS is outside this contract
.rules R1 / R2 / R2' / R3 / R3b / R4 / over-cap
.kinds the three codes, and why they ride beside
warnings[] rather than inside it
.cap 2048, neighbours included, and the boundary rule
.placeholder the gate that actually holds the negative space
.census-domains both open domains with their NOT-reached members
.gap-checker-divergence the four axes, pinned not unified
The changeset type is `Fixed`, which exempts this PR from the docs/
co-change requirement — but the glossary gate is separate from that
exemption, and the 2048 cap in particular is a machine-canon-shaped fact
that until now existed only inside a source comment.
`docs/CONTEXT-INDEX.json` regenerated (269 predicates); lint:generated-sync
confirms all six targets in sync.
* docs(#3697): document what the command now does, in one changeset sentence
DOCS. The grammar section predated this round's two new rules, so it
under-described the behaviour it exists to make lookup-able:
- The placeholder paragraph enumerated three words; the rule is a DEFAULT.
Any wording that selects no REQ-IDs warns, and the placeholders are matched
as the LEAD token, so `None (per ADR-7)` and `**None**` are declared-empty
too. The comment-only line is called out, because "any other wording" would
otherwise read as covering the shipped template's own `<!-- ... -->`.
- The comma rule was implicit. `REQ-01; REQ-02` marks only REQ-02, and it is
the quietest way to lose a requirement on this line — `requirements_updated`
reads `true` either way — so it gets its own paragraph, with the
parenthetical exemption stated beside it.
- The machine kind is documented where a consumer would look for it, with the
instruction to key on the kind rather than the wording.
- The skipped-text note no longer implies it reports date shapes; it
deliberately does not, and silently omitting that left the doc promising the
behaviour this round removed.
CHANGESET (round 4 review Minor 4). CONTRIBUTING.md's format is
`**<Bold user-visible change>** — <symptom-led explanation>.` and both
canonical examples are one sentence; this fragment ran three. Now one, and
covering what the round actually delivers rather than only the range shape it
started from.
* test(#3697): keep phase.test.cjs off the docs-guard exemption fingerprint
A comment added earlier in this round named `docs/CLI-TOOLS.md` by path. The
docs-guard exemption ratchet (#3753 FIX 3) fingerprints literal `docs/`
references in exempt test files and fails when a new one appears, so that
comment turned four green gates red — `ci-docs-guard-registry` and the
registration lint — for a file that reads no documentation at all.
Caught by diffing the full suite's failing-name set against the same suite run
at `upstream/next` in a probe worktree: 33 of 37 failures reproduce at base
(install / config-home / shadowing tests under the sandbox HOME), and exactly
these 4 did not.
Rephrased rather than baselined. Adding the path to
DOCS_GUARD_EXEMPT_DOCS_PATHS is the sanctioned response when a test genuinely
starts READING a new docs path — the violation text asks the author to
re-confirm the exemption still holds. Nothing here reads documentation; the
guard matched prose. Baselining would have recorded a coupling that does not
exist and made the next reader wonder what phase.test.cjs does with
CLI-TOOLS.md. The comment still names where the contract is written, just
without planting a path string.
* fix(#3697): close four defects this round's own pre-push review drove
An adversarial cross-AI review of this round, run before the push, returned 6
CONFIRMED and 4 REFUTED. Every refutation was driven against the built tree,
and every one was a shape the author had not probed — the rules were correct
across the probe set and wrong just outside it.
(1) R4 FALSE POSITIVE, and it is the #2334 over-warning class arriving through
the rule added to close a different hole. `REQ-01, see ADR-7: section 3` fired:
`ADR-7:` is the same shave class as `REQ-01;`, and the parenthetical test does
not reach a BARE citation. The FP probe that scored this rule 0/0 only ever
tested the parenthesised form.
Fixed by requiring the dropped id's prefix to agree with a SELECTED id — the
module's own idiom, not a new heuristic: `reqEndpointsImplyInterior` already
demands an agreeing prefix for the same reason. Cost, stated in the census: a
dropped id whose prefix is on no selected id (`REQ-01, FOO-02: x`) stays
silent. Same trade the strict-dash rule takes — under-report a rare shape
rather than over-report a common one. Pinned as a declared blind spot by
(2) R4 FALSE NEGATIVE, on the DOCUMENTED form. `[REQ-01; REQ-02]` dropped
REQ-01 silently: the selector strips square brackets and R4's raw scanner did
not. The bracket spelling the shipped template recommends was the one shape the
rule could not see.
(3) The rider filter suppressed a REGISTERED requirement. `API-2-01` is a legal
requirement id — gap-checker's `parseRequirements` accepts it from
REQUIREMENTS.md — so a `\d+-\d+` filter hid a genuinely dropped requirement
behind a rule meant only to hide dates. Narrowed to a four-digit year segment.
The earlier #3697-18 case asserting `API-2-01` should be suppressed is REMOVED,
and the removal is recorded in place: its premise was refuted, it was not
inconvenient.
(4) An INVISIBLE line warned. A lone U+200B carried a token to the parser while
reading as empty to the author, so R3b fired with nothing on screen to explain
it. Zero-width and format characters are now stripped — stripped rather than
treated as delimiters, because splitting on one would fabricate two fragments
out of one ID.
Also corrects the documentation the same review found overstated: the line is
split on commas AND whitespace, and the ID shape is matched case-insensitively,
so `REQ-01 REQ-02` and `req-01, req-02` both select and neither warns. That was
pre-existing selector behaviour; this round is the one that asserted the docs
were true of it.
Tests: #3697-19 (four invisible-only shapes), -19b (embedded zero-width is
stripped, not split on), -19c (three citation forms), -19d (both bracket
spellings), -19e (both halves of the rider boundary), -19f (the declared blind
spot), -19g (the two documented tolerances). 500 tests in phase.test.cjs, 0
failures; lint:ci clean.
* fix(#3697): generalise the drop rule, and stop the invisible fix hiding a drop
The pre-push review's continuation refuted six of seven follow-up claims. The
first one is the one that mattered: the invisible-character fix committed in
c7dce173a INTRODUCED #3697's own defect. Stripping zero-width characters from
the detector wholesale made `REQ-01<ZWSP>, REQ-02` go SILENT — the selector
really does drop REQ-01, and the strip removed the only evidence of it. The
test written alongside asserted the tokens and the empty R4 result and never
asserted `warn`, so it DOCUMENTED the bug rather than catching it; that
omission was the reviewer's own MISSED finding.
An invisible is two different questions about one character, and the fix is to
stop conflating them: absence-of-content for the empty test, DECORATION on a
token for the drop rule. Neither is a reason to delete it from the line.
R4 is generalised accordingly, because the continuation drove four more shapes
a trailing-delimiter-only regex could not see — `REQ-01 ;REQ-02`,
`REQ-01 :REQ-02`, `**REQ-01;** REQ-02`, the backticked form — plus
`**REQ-01**, REQ-02`, where emphasis alone defeats the selector. These are one
class: decoration on a token the selector then cannot take. One rule, not four
patches; patching them individually is how a list stays short and wrong.
PARENTHESES ARE NOT DECORATION, and the suite caught me learning that: shaving
them made `REQ-01, (REQ-02), REQ-03 — REQ-05` report a glued delimiter that was
never there and broke #3697-9d's channel routing with it. A parenthesis is this
rule's citation marker.
The rider stops adjudicating an undecidable shape. `API-2-01` is a legal
requirement id and `API-2026-08` is too, while `FY-26-08` and `FY-2026-08-15`
are dates — no regex separates them, and both filters this round tried scored a
miss in each direction. It now NAMES the token and states the ambiguity, which
is the same thing the two warning voices already do about a range separator.
Filtering hides a real dropped requirement; reporting it bare asks the author
whether a date is a requirement; saying "this may equally be a date" does
neither.
The census and the docs are corrected to what the code does, including the part
that is NOT complete: the prefix gate does not stop a citation that SHARES a
selected prefix (`ADR-01, see ADR-7: sec 3` fires), and nothing at token level
separates that from a real drop. A prose heuristic on "see" is the free-text
detector this module exists to avoid, so the honest move is to say so.
CLAIM 17 — the invariant that actually matters — came back CONFIRMED on a
20,000-run fast-check property over arbitrary Unicode: `citedReqIds` is
identical to upstream/next's for every input, and marking is untouched.
513 tests in phase.test.cjs, 0 failures; lint:ci clean.
* fix(#3697): gate the drop rule on evidence, and stop an unmatched paren swallowing the line
Third pass of the round's own pre-push review, scoped to regression-hunting
rather than further polish. Three findings, all driven, all mine.
R4 OVER-WARNED on markdown styling. `REQ-01, see **REQ-7** for context` claimed
a dropped requirement: the previous cut treated any shaved decoration as
evidence, and emphasis is not evidence. Nothing separates that line from
`**REQ-01**, REQ-02` meaning to list one, so the rule now requires a positive
signal — a glued `;`/`:` (a list separator was INTENDED) or an invisible (the
token is CORRUPTED; nobody types one on purpose). Emphasis alone falls back to
the skipped-text rider, which names the id without asserting a drop, exactly as
`(REQ-02)` is handled. That is the #2334 class caught one cut before shipping.
R4 UNDER-WARNED on `**REQ-01**; REQ-02` — one shave pass cannot reach a wrapper
sitting behind a delimiter. Shaves to a stable point now.
The range OPERATOR lost its invisibles handling. `REQ-01 <ZWSP>..<ZWSP> REQ-05`
went silent, because the previous commit removed the invisible strip from BOTH
the tokenizer and R4 when only R4's was wrong. An invisible is two questions
about one character: for the classification rules it is noise and is stripped
from the token; for the drop rule it is the evidence and must survive on the
raw line. Stripping in both places hid a dropped id; stripping in neither hid a
range. The reviewer's MISSED finding named the missing control — regression
tests covered invisibles inside ids and not beside operators — and #3697-19i is
that control.
UNBALANCED PARENTHESES swallowed the line. `REQ-01, (note REQ-02; REQ-03`
reported nothing: a running-depth counter left the unclosed `(` open through
end-of-line, so every genuine drop after it inherited citation immunity. A
parenthesis confers that immunity only as part of a MATCHED span now — an
unmatched one is a typo, not a citation.
CLAIM 22 re-confirmed on a fresh 20,000-run property over arbitrary Unicode:
`citedReqIds` identical to upstream/next, marking untouched, warnings appended.
522 tests in phase.test.cjs, 0 failures; lint:ci rc=0; full suite carries zero
head-only failures against a probe worktree at upstream/next.
* fix(#3697): make delimiter ADJACENCY the rule, and delete matched citations outright
Fourth and final pass of the round's own pre-push review. Three findings, and
they shared one root cause, so this is a narrower rule rather than a longer list
of shapes.
TOKEN-WIDE PAREN IMMUNITY LEAKED. `REQ-01, REQ-02;(note) REQ-03` is a single
whitespace token, so a matched parenthetical inside it conferred immunity on the
`REQ-02;` sitting OUTSIDE the parens, and the drop went silent. Matched spans
are now deleted from the line outright — which states what is actually meant,
that for this rule a citation is not on the line — and an UNMATCHED paren is a
typo that confers nothing. That also retires the running-depth counter whose
previous bug was the mirror image: an unclosed `(` swallowing the rest of the
line.
DECORATION WAS TESTED TOKEN-WIDE, so `REQ-01, see **REQ-7**; next topic` was
reported as a dropped requirement. It is a citation with sentence punctuation.
The rule is now ADJACENCY: styling is stripped, then the `;`/`:` must be
touching the id. `REQ-01;`, `;REQ-02` and `**REQ-01;**` qualify;
`**REQ-01**;` does not, because outside the styling that character is
punctuation. An invisible needs no adjacency test — nobody types one on
purpose, so anywhere in the token it is corruption rather than intent.
`**REQ-01**; REQ-02` therefore goes silent, and the test row asserting
otherwise is inverted rather than deleted quietly: it was added one commit ago
on the reasoning this pass refuted, and nothing distinguishes it from
`see **REQ-7**; next topic`.
Worth recording plainly: three successive cuts of this rule fired on a
citation, and each fix was a narrower definition of EVIDENCE, never a longer
list of shapes. The list-lengthening instinct is what produced the bug each
time.
The review's last MISSED finding named the missing control — the paren tests
all surrounded matched spans with whitespace, so none covered a span sharing a
token with an id outside it. #3697-19j carries both directions now.
526 tests in phase.test.cjs, 0 failures; lint:ci rc=0; full suite zero
head-only failures against a probe at upstream/next; the invariant that
`citedReqIds` is identical to upstream re-confirmed on 20,000 arbitrary
Unicode inputs.
* fix(#3697): state R4's real boundary, and stop the over-cap voice masking a drop
Two defects, both found by this round's own pre-publication body claim-audit.
1. A DEMONSTRATED drop was discarded by the unverified voice. On
`REQ-01, REQ-02: <2049 chars>` the analyzer names REQ-02 in
delimiterDroppedIds and the formatter then reported `req-line-unverified`,
whose message never mentions it — the one actionable finding masked by the
token beside it. The over-cap channel now excludes a line carrying an R4
hit, exactly as rangeReadingOnly already did and for the same reason: that
voice's whole claim is that nothing could be checked, and R4 has already
checked something. The assertive channel still carries the over-cap rider,
so nothing about the cap is traded away. Pinned by #3697-19l, which fails
against the pre-fix build and nothing else does.
2. Three shipped artifacts asserted behaviour the code does not have — the
same class as this PR's round-4 blocker, re-committed. CONTEXT.md's rules
predicate, the CLI tools reference, and the warning's own advice string all
listed markdown emphasis as an R4 trigger. It is not: styling is shaved
BEFORE the test and tolerated around an id, never a trigger on its own, so
`**REQ-01**, REQ-02` and `**REQ-01**; REQ-02` are both silent. The trigger
is exactly a glued `;`/`:` or an embedded invisible.
The census predicate was wrong in a second way. Its 26-spelling separator
sweep found only `;` and `:` because the sweep was SYMMETRIC-ONLY and
therefore biased: one-sided attachment drops silently for every punctuation
outside the set — `/ | & + . > \` and the full-width and non-ASCII forms
`; , ؛` all measured silent. The domain is wide open and R4 covers two
characters of it. Said plainly in all three places rather than widened
here: every previous widening of this rule first fired on a citation, so it
is not done blind at the end of a round.
Both blind spots are now PINNED as tests (#3697-19m styling-only, #3697-19n
one-sided separators) so the documents and the code cannot drift apart again —
which is what the round-4 blocker asked for.
tests/phase.test.cjs: 542 tests, 542 pass, 0 fail, 0 skip. lint:ci rc=0.
Both CONTEXT-INDEX consumers regenerated.
* fix(#3697): re-sweep the separator census properly, and say what it really found
The round-4 census in src/phase.cts concluded "exactly two — `; ` and `: `"
from a 26-spelling sweep. That conclusion was forced by how the sweep was
built, not by the code: it swept the ONE-SIDED form (`REQ-01; REQ-02`) for the
semicolon and colon, and only the BARE and SYMMETRIC forms (`|`, ` | `) for
every other separator. Different members of the domain were tested in
different shapes, so no other answer was reachable. Caught by this round's
pre-publication claim-audit of the response comment, reading the census
comment against its own swept list.
Re-swept fully crossed and driven through the built artifact: 21 separators x
{bare, trailing-space, leading-space, both-spaces} = 84 combinations. 26 select
both ids, 24 under-select and already warn, and 34 UNDER-SELECT SILENTLY. All
34 are one shape — a separator glued to exactly one of the two ids, e.g.
`REQ-01/ REQ-02` or `REQ-01 /REQ-02` — for every punctuation except `,` and
the `;`/`:` that R4 covers.
So R4 covers TWO CHARACTERS of a wide-open domain. That is now what the census
comment, the CONTEXT.md census-domains predicate and the CLI tools reference
all say. The set is deliberately not widened here: three successive cuts of
this rule fired on a citation, and a fourth at the end of a round with no
adversarial pass is how each of those got in.
Second false passage in the same block: styling-only decoration was described
as "left to the skipped-text rider, which names the id". A rider only exists
inside a message, and a message only exists once some rule sets `warn` — so on
a line where nothing else fires, `REQ-01, **REQ-02**` is wholly silent.
Describing it as handled reads as coverage. #3697-19m already pins the silence.
so the test matches the documented claim.
tests/phase.test.cjs: 552 tests, 552 pass, 0 fail, 0 skip. lint:ci rc=0.
Both CONTEXT-INDEX consumers regenerated.
* chore(#3697): regenerate both CONTEXT-INDEX.json after rebasing onto next
Rebased onto next @
|
||
|
|
900504f985 |
fix(#3776): decide nothing-to-commit from the staged diff, not from staging success (#3859)
* fix(#3776): decide nothing-to-commit from the staged diff, not from staging success
`cmdCommit`'s empty-diff guard tested `stagedPaths.length === 0`, but
`stagedPaths` records paths whose `git add` exited 0 — "did staging
succeed", not "is there anything to commit". Staging an already-committed,
unmodified file succeeds while contributing no diff, so the guard was
reachable only when every named path was missing from disk.
The ordinary empty-diff case therefore fell through to `git commit`, where
the only thing converting the failure back to `nothing_to_commit` was a
string match on git's output. Git runs the pre-commit hook before it
decides there is nothing to commit, so a rejecting hook pre-empted that
match and the caller was handed `commit_failed` carrying a gate message
about a commit that had nothing to gate.
Ask git whether the staged paths actually differ instead. Two conjuncts
are load-bearing: the `length === 0` short-circuit keeps the
all-missing-paths case exact (a pathspec-less `diff --cached` would test
the whole index, so unrelated staged work would suppress the guard), and
`!isMergeInProgress` keeps a merge from being abandoned — during a merge
git refuses a partial commit, so the pathspec describes nothing about what
would land.
Nine regression cases in tests/commands.test.cjs cover the brief's six
acceptance criteria plus the merge interaction. Against the pre-fix build
exactly one fails; the other eight pin behaviour that was already correct.
Residual sibling of #2608/#2693, which covered `git add` failing; this
covers `git add` succeeding and contributing nothing.
* fix(#3776): exempt a cherry-pick too, but never a revert
The empty-diff guard must not decide from a pathspec git will not honour.
That test was merge-only; git refuses a partial commit during a cherry-pick
for the same reason, so the guard would have fired there and reported a
silent `nothing_to_commit` where the pre-fix code surfaced git's refusal.
The three sequencer states do not agree, so this is driven rather than
reasoned by analogy (git 2.54):
MERGE_HEAD fatal: cannot do a partial commit during a merge.
CHERRY_PICK_HEAD fatal: cannot do a partial commit during a cherry-pick.
REVERT_HEAD permitted; behaves like an ordinary commit.
REVERT_HEAD is therefore deliberately excluded: enumerating it alongside the
other two — the obvious move — would suppress this fix during a revert and
reintroduce the very misreport it removes. Both new states are pinned by a
test, and the revert arm fails against the pre-fix build exactly as AC1 does.
`canScope` keeps its narrower merge-only test on purpose; widening it would
change pre-existing cherry-pick behaviour, which is outside this fix.
* fix(#3776): probe the working tree, not the index
`git commit -- <paths>` is a PARTIAL commit: it records the working-tree
content of those paths and ignores what is staged. The guard was probing
`git diff --cached` — the index — which answers a different question than
the commit asks.
Driven on the same path, in this order: `git add` an unmodified file, then
write to it, then probe.
git diff --cached --quiet -- p rc 0 ("nothing staged")
git diff --quiet HEAD -- p rc 1 ("the tree differs")
git commit -m m -- p committed the new content
So a working-tree write landing between the `git add` above and the probe —
another process in a shared checkout, which this project explicitly supports
— would let the guard report `nothing_to_commit` for a call that would have
recorded that content. Probing `HEAD` asks the question the commit answers.
Not reachable through this function single-threaded, because the staging
loop re-adds every named path immediately beforehand, so index and working
tree agree at the probe. The change is correctness by construction rather
than a fix for an observed miscommit.
An unborn HEAD makes `diff HEAD` fatal; that falls through to the commit as
any other probe error does, and is now pinned by a test — the first commit
in a repo must not be swallowed by an empty-diff guard.
Both added probes are now gated on the guard being able to fire at all, so
an unscoped commit and an `--amend` pay for neither.
Found by adversarial pre-filing review; the index/worktree distinction was
not something my own path-shape probes could have surfaced.
* test(#3776): pin the assume-unchanged boundary; correct a stale comment
`git update-index --assume-unchanged` makes `git add` stage nothing and makes
BOTH diff forms — `--cached` and `HEAD` — report no difference, so no
diff-based guard can see a change to such a path. `git commit -- <path>` is
the odd one out: it reads the working tree directly and records it.
So a modified assume-unchanged path now reports `nothing_to_commit` where it
previously committed. That is the answer consistent with this function's own
staging step, which honoured the flag one loop earlier — but it is a
behaviour change, and it belongs on the record as a decision rather than
surfacing later as a surprise.
Also corrects an AC3 comment still describing the `diff --cached` whole-index
form that the previous commit replaced.
* chore(#3776): set changeset fragment pr to 3859
The fragment carries the PR's own number, which is unknowable before the PR
exists. Backfilled post-create; the repo's changeset lint rejects the `pr: 0`
placeholder.
* fix(#3776): do not read an unanswered sequencer probe as "no merge"
`execGit` surfaces a spawn timeout as `exitCode: 1` (`_spawnResult`:
`result.status ?? 1`) — the same code `rev-parse --verify` returns for a ref
that does not exist. So the MERGE_HEAD and CHERRY_PICK_HEAD probes could not
tell "not in that state" from "never answered", and the empty-diff guard read
both as "not in that state". That is the one path in #3776 that did not fail
toward the previous behaviour: a timeout during a real merge decided
`nothing_to_commit` from a pathspec git will not honour and left the merge
unconcluded, where before it was a loud `commit_failed`.
Treat an unanswered probe as "assume the partial commit would be refused" —
which falls through to `git commit` and lets git speak for itself.
Routed into `partialCommitRefused` only, deliberately never into
`isMergeInProgress`. That flag also feeds the pre-existing `canScope`, and
widening it there is worse than the misreport it fixes: with `canScope` false
the commit runs bare, and a bare commit during a merge is PERMITTED — git
concludes the merge with the whole index under a message naming one file.
Driven: the whole-flag form reports `committed` where this form reports
`commit_failed`, and it drops the pathspec on the ordinary timeout, re-opening
the #2112 scope leak.
* fix(#3776): pin the empty-diff probe against diff-only configuration
`git diff` is porcelain and honours settings `git commit -- <paths>` does not,
so an unpinned probe let a caller's configuration decide whether the guard
fires. Driven against git 2.54, each with the paired `git commit -- <path>`
confirmed to record the change the unpinned probe reported as absent:
diff.ignoreSubmodules=all a gitlink bump is invisible to the probe
.gitmodules ignore = all the same, and it needs NO local config at
all — it is checked in, so it arrives with
a clone
diff=<driver> + textconv two different blobs converge to one text,
so the probe sees no change; no submodule
involved
`--ignore-submodules=dirty` rather than `=none`, because `dirty` is what a
partial commit of a submodule path actually means: it records the gitlink,
which moves only when the submodule's HEAD does. Under `=none` a merely dirty
submodule work tree reports a difference the commit would not record, sending
an empty call back to `git commit` — the same misreport, re-entered from the
other side. `dirty` still overrides both `diff.ignoreSubmodules` and a
checked-in `.gitmodules` `ignore`, so the gitlink vectors stay closed.
`--no-ext-diff` is deliberately absent: `--quiet` short-circuits ahead of an
external diff driver, so `diff.<driver>.command` cannot invert the probe
(driven: rc 1 with and without the flag).
* docs(#3776): disclose the two outcome changes the changeset omitted
The body listed what stays unchanged and never named the arms whose
user-visible outcome moves, so neither would have reached the changelog:
- a modified path under `git update-index --assume-unchanged` now reports
`nothing_to_commit` where it was previously committed. `git add` already
honoured the flag one loop earlier; the guard reports what staging did.
Documented in a code comment and pinned by a test since the first round,
but absent from the fragment.
- naming a submodule whose work tree is dirty while its recorded commit has
not moved now reports `nothing_to_commit` rather than `commit_failed`,
because nothing would have landed. New in this round, from the
`--ignore-submodules=dirty` pin.
* test(#3776): use helpers.cleanup() for the submodule fixture teardown
`local/no-raw-rmsync-in-tests` rejects a bare `fs.rmSync` in a test: the
helper carries the Windows-EBUSY retry budget (`maxRetries`/`retryDelay`)
that a raw call does not, and a submodule work tree is exactly the shape
that holds handles open on Windows.
Caught by CI, not locally — the round ran the two affected suites but not
`npm run lint:ci`, so the repo's own rule never fired until the push. The
chain now exits 0 locally against this tree.
* fix(#3776): never drop a named assume-unchanged path
`--assume-unchanged` is the one state where `git diff` and
`git commit -- <paths>` genuinely disagree: `git add` stages nothing,
both diff forms report no difference, and `git commit -- <path>` still
reads the working tree and records it. The empty-diff guard therefore
reported `nothing_to_commit` about content the caller named in `--files`
and git would have written.
commit is made — so suppressing its misreport must not be paid for by a
silent drop. Same rule the timeout routing already follows: a fix for a
misreport may not cost content.
The guard now asks `git commit --dry-run --porcelain` whether the commit
would record anything, and stands aside on rc 0. That is the same
decision the real commit makes, so there is no second implementation of
it to drift. It does not run the `pre-commit` hook (driven: a rejecting
one neither fires nor writes a marker), which is what matters — a firing
`pre-commit` is the whole of #3776. It is NOT hook-free in general: git
2.54 fires `post-index-change` here, so a repo using that hook sees it
once for the probe and once for the commit. Stated rather than claimed
away.
Asking git rather than reconstructing its answer was reached by
measurement. Comparing `git hash-object` against `HEAD:<path>` was tried
and is wrong three ways, each a silent drop of named content: it misses a
mode-only change (`chmod +x` leaves the blob identical while the commit
records `100755`); it cannot hash a submodule path at all (`fatal: Unable
to hash sub`, while the commit advances the gitlink); and the path it
needs must be parsed out of `ls-files` output, which `core.quotePath`
renders as `"caf\303\251.md"` by default. Each has its own arm, and the
non-ASCII arm pins `core.quotePath` so it cannot go vacuous.
Falling through on the `ls-files` tag alone — without asking whether
anything would land — is also wrong: an UNMODIFIED assume-unchanged path
would reach `git commit`, which with any unrelated modified file present
prints `no changes added to commit`, a string the fallback does not
match, and returns `commit_failed`. That is #3776 re-entered from the
other side, the same shape `--ignore-submodules=none` would have
re-entered it. Pinned by its own arm.
The `ls-files` read is an optimisation, not a gate: it keeps the dry run
off the hot path when no assume-unchanged entry is present, and when it
cannot answer the dry run simply runs, because the dry run needs nothing
from it. Failing closed there would drop content and failing open would
re-enter #3776 — both are wrong answers to a question that can be asked
directly.
`--skip-worktree` is not a second instance. A present, modified one exits
1 from `git add` and fails closed as `staging_failed` above the guard; an
absent one is skipped before `git add` runs (#2014) and is answered by
the `stagedPaths.length === 0` arm, exactly as it was pre-fix. Both
shapes pinned, because the shorter claim ("never reaches the guard") is
too strong.
* test(#3776): register fixture teardown so a failed assertion cannot leak
`bumpedSubmodule()` creates its sub-repo as a SIBLING of `tmpDir`, and
the unborn-HEAD arm creates `fresh` outside it too, so the describe's
`afterEach(() => cleanup(tmpDir))` reaches neither. Both were cleaned by
a trailing statement in the test body, which any failing assertion above
it skips — leaking a git repo into the temp root.
`bumpedSubmodule()` now records the path and a describe-scoped
`afterEach` drains it, which covers all three of its callers at once;
the unborn-HEAD arm takes `t.after`, the form already used elsewhere in
this file.
Negative-controlled both ways with a deliberate assertion failure
injected into the dirty-submodule arm, under an overridden TMPDIR:
before, one `*-sub` repo survives the run; after, none.
* fix(#3776): never read an unanswered dry-run probe as "nothing to record"
The `git commit --dry-run --porcelain` probe that decides the assume-unchanged
boundary is the one probe in the guard whose rc 0 is the reassuring answer, so
it inverts the diff probe's safety: `execGit` collapses a spawn timeout (or any
spawn error) to `exitCode: 1`, byte-identical to git's own "nothing to record",
and the guard then reported `nothing_to_commit` about content named in
`--files` that git was never asked to write. Same conflation the sequencer
probes already defend against.
Only a CONFIRMED rc 1 with no spawn error closes the path now; a timeout, a
spawn error, or rc 128 falls toward the real commit, where git speaks for
itself. Five arms in tests/commit-files-pathspec.test.cjs pin it (posix +
windows timeout shapes, rc 128, the ls-files optimisation's own timeout, and a
negative control on an unmodified path); the injection helper gains an optional
`matchArg` so the dry run can be targeted without intercepting the real commit.
Also corrects the comment that claimed both sequencer probes are gated on
`guardApplies` — the MERGE_HEAD probe predates this fix and is unconditional.
* fix(#3776): probe with --no-verify so a hook-firing git cannot close the guard
Round 4, review finding 3 (Minor). The `git commit --dry-run --porcelain`
probe's safety rested on an empirical claim about one git version: that
`--dry-run` does not run `pre-commit`. git 2.54 satisfies it, but the failure a
differing version would produce is silent and lands in exactly #3776's own
configuration.
A `pre-commit` that fires and rejects exits 1 — the same code git returns for
"nothing to record" — so the closure would read it as a CONFIRMED empty answer,
drop the content the caller named in `--files`, and report `nothing_to_commit`.
That is #3776 re-entered through the probe the fix added.
`--no-verify` forecloses it structurally rather than documenting the version
dependency. Driven on git 2.54: rc-identical in both directions (rc 0
would-record, rc 1 nothing) with and without the flag, so it is behaviour-
neutral where the version already agrees.
Two claims deliberately NOT widened: `--no-verify` does not suppress
`post-index-change`, which still fires on this call with or without it (driven
both ways); and the real `git commit` is untouched — #3776 is a bug about a
hook's message reaching the caller wrongly, never a licence to skip hooks.
The new arm pins the FLAG rather than an outcome, because the outcome it
protects is unobservable on a git that already declines to run the hook. It is
a seam assertion over the argv the guard actually issued, not a source grep.
* test(#3776): pin all-missing --files during a merge or cherry-pick
Round 4, review finding 1 (Major) and finding 7 (Nit, its coverage half). The
review asks for the `stagedPaths.length === 0` disjunct to be gated on
`!partialCommitRefused`, or for the combination to be documented and tested.
Documented and tested — the gating is refused, with cause.
The premise is confirmed: the state is reachable exactly as described, and
during a merge the `nothing_to_commit` report does not tell the caller the merge
is still open. The prescription is not. With every named path missing,
`stagedPaths` is empty, so `canScope` is false and the fall-through reaches a
BARE `git commit`, which git PERMITS during a merge and which then CONCLUDES it.
Driven, git 2.54, through cmdCommit with the prescription applied:
cmdCommit(cwd, 'add the thing', ['.planning/never-produced.md'])
-> { "committed": true, "hash": "8e6bf45", "reason": "committed" }
MERGE_HEAD gone; HEAD is a 2-parent merge commit recording
.planning/shared.md with the caller's resolution content.
So the gating trades a report that writes nothing for one that silently writes
the whole index under a message naming a path that does not exist, and reports
success. That is the same trade the timeout routing already refuses one block
up, which is why the sequencer states gate the DIFF branch only.
The behaviour is also pre-existing and unchanged by this PR: at
|
||
|
|
41466e8e88 |
fix(#4023): preserve decimal phase ids in init progress ordering and smart-entry output (#4110)
* test(#4023): reproduce decimal phase-id coercions * fix(#4023): preserve decimal phase ids in progress signals * test(#4023): align phase token contract expectations * chore(#4023): point the changeset at PR #4110 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
|
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> |
||
|
|
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> |
||
|
|
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> |
||
|
|
8c9265d4e5 |
fix(#3724): warning-only Dimension 3b findings no longer force the revision loop (#3758)
* fix(#3724): stop advisory Dimension 3b findings from forcing the revision loop Dimension 3b (undeclared/temporal coupling, #1954) is spec'd "never a blocker" but tagged severity: warning — the tier plan-phase's revision loop counts as must-fix — and the planner is never taught the rule, so every multi-wave phase touching shared mutable state replans at least once, and intentionally coupled plans re-flag identically every iteration to the stall prompt. Three coordinated changes: - gsd-plan-checker: retag 3b to severity: info, the tier references/revision-loop.md already exempts by design; recognize a coupling_justified frontmatter declaration in the Do-NOT-flag list so deliberate pairs converge. Additions are offset by trimming 3b motivation prose — the checker sits 45 bytes under its LARGE hard cap. - plan-phase step 12: INFO-only accept — an issues block with zero BLOCKER/WARNING entries accepts the plan and surfaces the advisories instead of re-entering the revision loop. Real blockers and warnings still gate unconditionally. - gsd-planner: slim pointer in assign_waves to the new progressive-disclosure reference gsd-core/references/planner-coupling.md (the planner sits 19 chars under its own cap), which carries the shared-mutable-state rule and the coupling_justified escape hatch so first-pass plans avoid the finding when the coupling is unintentional. Documented the coupling_justified field in docs/reference/plan-md.md. Growth acks per #2914; inventory manifest and install-tree fixtures regenerated for the new reference file. Closes #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): pin Dimension 3b at severity: info The severity retag makes the old assertion (severity: warning) stale; lock the advisory tier from both directions — info must be present, warning must not — so a future edit cannot silently re-arm the revision-loop trigger. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * chore(#3724): changeset fragment for PR #3758 Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * docs(#3724): roster planner-coupling.md in docs/INVENTORY.md The new reference was enumerated in the manifest and all 19 install-tree fixtures but missing its row in the Modular Planner Decomposition table — the roster half the manifest-sync test cannot check. (Review Blocker.) Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): cover all four acceptance criteria (review round 1) - plan-checker-coupling: the 3b severity assertion is now a PARITY check deriving the exempt tier from revision-loop.md's flow instead of hardcoding info — editing either side alone reds the suite. New describe pins the other three criteria: plan-phase's INFO-only accept clause (proven failing-first), the BLOCKER + WARNING count staying intact, the coupling_justified Do-NOT-flag exemption + fix_hint, and the planner pointer + planner-coupling.md content. - ack fragment: $comment's plan-phase figure corrected to +79B; the 2775 pin note carried forward into the gsd-planner.md entry, updated for upstream's #3761/#3764 Rule-paragraph anchor (which this diff leaves verbatim). The parallel-dependent-plans re-anchor this commit originally carried was superseded by upstream #3764 during review; this branch no longer touches that file. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 2 — align the stance enumeration, complete the template contract MAJOR: <adversarial_stance>'s severity enumeration gains the INFO bullet so it agrees with Dimension 3b's 'ALWAYS INFO' mandate instead of contradicting it. Funded by extracting the inline <examples> block to the new progressive- disclosure reference gsd-core/references/plan-checker-examples.md (@-inlined from the same spot; #1949 precedent), which also restores the 3b motivation clause round 1 traded away (Nit 4) and nets the agent file SMALLER than base (49107 -> 48486) — the extraction the byte pressure was owed. MINOR: gsd-core/templates/phase-prompt.md now carries coupling_justified, and the field's shape becomes one 'plan-id: reason' string per coupled peer so a plan justified against two peers can express it; docs/reference/plan-md.md's Type column names the shape. NIT: the 3409 ack's plan-phase entry no longer calls the #1168 workflow ratchet an 'XL tier'. Acks and derived artifacts updated accordingly (checker entry removed — a shrink needs no ack; INVENTORY roster row + regen:derived for the new file). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): derive the 3b negative severity assertion (review round 2) Every severity token in the 3b span must BE the tier revision-loop.md exempts, replacing the hardcoded severity:warning negative — if the loop's exemption ever moves, the failure names the real conflict instead of blaming the agent file with a mutually-unsatisfiable pair. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): refit the planner coupling pointer under the char cap Upstream #3299 (PR #3390) grew agents/gsd-planner.md to 49146 chars at the base, leaving 5 chars of headroom where the +16-char pointer was measured against 13 more. The pointer prose shortens to 'Non-file coupling:' — 49150 chars, back under the strict 49152-char cap — and the ack figures follow. The @-path the tests pin is unchanged. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): re-home the plan-phase ack after the #3823 spent-fragment sweep Upstream #3078/#3823 deleted all fully-spent ack fragments, including 3409-unreachable-guard-arms.json, which carried this PR's plan-phase.md +79B append. Per the collision remedy that sweep added: take the deletion and home the still-live entry in this PR's own fragment. Figures re-measured at this merge base (90871 -> 90950 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): absorb the spent #3172 plan-phase fragment into this PR's ack Upstream #3825 shipped 3172-stated-failing-direction.json naming only plan-phase.md, now spent at the base — colliding with this PR's live plan-phase entry. Per the #3003 pattern the fully-spent single-path fragment is deleted and this fragment stays the path's one source; figures re-measured at this base (93073 -> 93152 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 3 — true up the ack figures, restore the wave comment The fragment's absolute sizes are re-measured and anchored to base |
||
|
|
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> |
||
|
|
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 ( |
||
|
|
ef9ce3e598 |
fix(#3702): count asterisk, plus and ordered markers as deferred-items entries (#3739)
* fix(#3702): deferred-items counts `*`, `+` and ordered markers as list items
`deferred-items.md` has no template and no mandated shape, but its parser
recognised only the `- ` hyphen marker. Asterisk bullets, plus bullets and
dot-terminated ordered lists — all lists in CommonMark and GFM — contributed
ZERO entries on both the headless and the heading-delimited path, and a mixed
file dropped its non-hyphen entries while keeping their hyphenated siblings,
under-reporting without ever looking empty.
The restriction was a regex literal inherited from the Gaps seam, where the
template genuinely mandates the hyphen YAML-lite form; nothing in the module's
stated rationale distinguishes `*` from `-`.
Widened on the deferred path only:
- `splitGapsEntriesCore`'s entry opener, `extractGapEntryFields`' line-0 strip
and `rawGapEntryText`'s line-0 strip take a `BulletMarkers` parameter that
DEFAULTS to the hyphen-only set, so `## Gaps` keeps its template-mandated
grammar byte-for-byte and the module still has exactly one grouping pass.
- `splitDeferredHeadingEntries`' body-bullet test, `stripLeadingBulletMarker`
and `acknowledgeDeferredItem`'s status-field regexes move in lockstep —
widening what OPENS an entry without widening what is STRIPPED before field
extraction would surface an entry that can never resolve.
Unchanged, and pinned by tests: prose-only and bare headings still contribute
nothing ("prose is not an item"); a table under a leaf heading still yields
exactly its rows, since table lines are skipped before the body-marker flag can
be set and a `|` row is not a list marker; the paren-terminated ordered form
`1)` is out of this fix's scope.
* docs(#3702): changeset fragment (pr: 0 placeholder pre-create)
* fix(#3702): widen the forensic-audit prose entry rule to match the parser
Sibling site of the same defect class, found by a defect-class sweep of the
deferred-items consumers. `/gsd-progress` check 7 does NOT go through
`gsd-tools query` — it globs `deferred-items.md` and has the model read entries
by a prose rule that mandated "one entry per top-level `- ` line". Left as-is,
the marker widening would hold on the CLI path while the one consumer that
bypasses the parser kept reporting "No unresolved deferred items" for a file
written with `*`, `+` or an ordered marker: the same false negative, surviving
in the only place the fix could not reach by code.
Also pass DEFERRED_BULLET_MARKERS explicitly where the heading path extracts
fields. It was already correct — stripLeadingBulletMarker pre-strips the widened
set from every line, so the default hyphen strip is a no-op there — but relying
on that leaves a detection site and a strip site nominally on different marker
sets, which is exactly the asymmetry the BulletMarkers doc comment warns about.
Explicit is local; inferred is a trap for whoever edits the strip next.
Out of scope, noted rather than fixed: forensic-audit.md globs only
`.planning/phases/*/` and so misses archived milestone phases that
`scanDeferredItems` covers. Pre-existing, a different defect, and not this
issue's ruling.
* docs(#3702): note the milestone-close halt for heading-shape non-hyphen files in the changeset
A heading-delimited deferred-items.md written with */+/ordered markers
previously parsed to zero and closed silently; it now yields entries whose
heading shape acknowledgeDeferredItem refuses, halting complete-milestone
until hand-edited. User-visible, so the fragment states it.
* chore(#3702): set changeset fragment pr to 3739
* fix(#3702): CR-normalise the heading path and the acknowledge writer (review B1, M4, m2)
B1 — `splitDeferredHeadingEntries` stored RAW lines; on a CRLF file every
body line but the last still carried its `\r`, the `$`-anchored marker
strip failed on it, the marker survived into field extraction and the
field was lost — a `**Status:** resolved` that was not the file's final
line resurfaced its entry as open. The heading path now stores CR-stripped
lines like the headless path already did, and the strip regex tolerates a
trailing CR on its own. Round 1's CRLF test put `**Status:**` on the last
line, the one position `collectSection`'s `.trimEnd()` had already
de-CR'd; the new tests put it first and mid-body.
M4 (pre-existing on `next`) — `acknowledgeDeferredItem` found the status
line on a CR-stripped copy but rewrote the raw line with a `$`-anchored
`.*`, which cannot consume `\r`; `replace` returned its input, and the
writer reported `ok` over byte-identical content. The rewrite now runs on
a CR-stripped line. The comment that claimed `.*$` consumed the `\r` is
corrected — it was the bug, stated as the design.
m2 — the indent probe for an inserted `status:` line ran on the raw line
and fell back to indent 0 on CRLF; it is CR-stripped too.
* fix(#3702): derive every deferred-items marker regex from one source (review M3, N1, N2)
M3 — round 1 carried the marker alternation in FOUR places: the
`BulletMarkers` pair and two inline literals inside
`acknowledgeDeferredItem`, under a doc comment saying the interface
existed so a detection site and its strip site could not drift. All four
now derive from `DEFERRED_MARKER_ALT`; drift is impossible rather than
discouraged. A parity test pins the vocabulary against
`markdown-sectionizer`'s `iterateBullets` on everything the two grammars
are meant to agree on, and names the two points they deliberately differ.
N1 — the ordered marker is `\d{1,9}\.` (CommonMark §5.2), not `\d+\.`.
N2 — the marker is followed by `[ \t]`, not `\s`, which also accepted
`\r`; the tab remains accepted (CommonMark-legal) and the divergence from
`iterateBullets`' literal space is pinned rather than papered over.
The four regexes are exported for the parity test only.
* fix(#3702): an ordered marker opens an entry only from `1.` or inside a run (review B2, m1)
B2 — `\d+\.` alone read ordinary prose as a list: "2026. was a bad year
for this module" and, under `### Notes`, "3. is the number of retries we
settled on." both opened an entry on round 1, the second straight through
the "prose is not an item" contract that round's AC4 claimed to preserve.
CommonMark §5.3 faces the same ambiguity when an ordered list would
interrupt a paragraph and resolves it by requiring the list to start with
1; `matchListOpener` applies that rule wherever an ordered marker is seen,
with the run carried per list (headless) or per leaf-heading body. Numbers
after the first are ignored, as CommonMark ignores them. Stated cost,
pinned: a hand-numbered list starting at 2 reads as prose — every ordered
record in the #3702 scan starts at 1.
Both reviewer cases are pinned as prose; the ruling's `1. alpha / 2. beta`
shape still counts.
m1 — the 9-digit boundary is pinned at both sides (`999999999.` opens,
ten digits is not a marker), and the 3-vs-4-space indentation cliff is
pinned as deliberately NOT applied: the parser is indent-lenient because
surfacing a questionable hand-written entry beats dropping a real one.
* fix(#3702): thematic breaks close the list and fenced code never opens an entry (review M1, M2)
M1 — `- - -` was a phantom `"- -"` entry on base; widening the marker set
added `* * *` and `+ + +` to the class, and `* * *` is the separator an
author writing in the `*` style is most likely to use. A CommonMark §4.1
thematic break (plus the `+ + +` gesture, which is the same garbage as an
entry name) now closes the open entry on the headless path and is dropped
from the body on the heading path — neither an item nor a continuation.
M2 — neither splitter was fence-aware, so `+ `-prefixed diff lines and
`1.`-numbered repro steps inside a code block counted as entries; #3702's
wild records carry exactly those blocks. Both splitters now classify lines
by the sectionizer's own `scanFencedBlocks` (so `~~~`, indented and
unterminated fences behave as `stripFencedCode` would): fence content
never opens an entry, is continuation inside an open one — keeping the
span invariant `acknowledgeDeferredItem` re-verifies — and is discarded
before the first.
* test(#3702): range the #2287 deferred-items property over marker × shape × line ending (review B3)
The `#2287` property hard-coded `- ` and filtered `\r\n` out of its
arbitraries, so the widened marker set — an enumerated domain, exactly
what a property is for — was never under it. It now ranges over
`{-, *, +, ordered}` × `{headless, heading}` × `{LF, CRLF}`, with the
heading shape placing `**Status:**` first or last: the review's
prescription (markers × line endings) would not have reached B1, which
lives on the heading path only, so the shape axis is the load-bearing
addition. Ordered entries are numbered from 1, so the B2 run rule is
under the property too.
A second property drives `acknowledgeDeferredItem` over every unresolved
headless entry across the same marker × line-ending grid — the one that
reaches M4 (a CRLF rewrite that reported `ok` and wrote nothing) and m2.
* test(#3702): pin the milestone-close halt on a heading-delimited `*`/`+`/`1.` file (review m3)
A heading-delimited `deferred-items.md` written with a non-hyphen marker
previously parsed to zero entries and let `complete-milestone` close
silently; it now yields entries whose heading shape `acknowledgeDeferredItem`
refuses, which the milestone loop turns into `record_ack_failure` → exit 1.
The loop is prose in a workflow, so the test drives the two CLI calls it
makes: `audit-open --json` must list the entry, and
`audit-open acknowledge --text <the audit's own text>` must refuse with the
heading-delimited message and write nothing.
* docs(#3702): changeset and forensic-audit prose carry the round-2 grammar
The changeset names the CRLF fixes, the ordered start-at-1 rule, thematic
breaks and fences. The `/gsd-progress` forensic-audit step is the one
prose parser of this file and must state the same grammar the code has.
* fix(#3702): round-review refinements — run ends at a paragraph, rejected ordinals unstripped, breaks at any indent, fenced fields, `## Gaps` scope
Findings from the pre-push adversarial review of round 2, each pinned:
- An ordered run ENDS at a paragraph that follows a blank line (CommonMark
§5.3); a non-indented line with no blank before it is lazy continuation
and keeps the run open. `1. a` / blank / `paragraph` / blank / `5. x` is
one entry, not two.
- The heading path strips the marker off every body line before field
extraction (#3457); a line whose ordinal `matchListOpener` REJECTED must
not be stripped, or "3. status: resolved" as prose loses its `3. ` and
reads as a resolved field. `splitDeferredHeadingEntriesDetailed` now
carries a per-line opener flag and only accepted openers are stripped —
in headless regions of a heading-shaped file too.
- A thematic break is recognised at any indent, matching the parser's
indent-lenient reading of items; ` * * *` was a phantom `* *`.
- Fenced lines carry no FIELDS either: a `status: resolved` quoted inside a
code block no longer resolves its entry on either path.
- Block structure (breaks, fences) is a property of the GRAMMAR, carried as
`BulletMarkers.blockStructure`: the deferred set opts in, the Gaps set
does not, so `## Gaps` is byte-for-byte on its `next` behaviour — the
round-2 M1/M2 change had reached it through the shared splitter.
* test(#3702): the property exercises the rejected-ordinal branch; the N2 control is independent
Round review: the widened #2287 property numbered every ordered run from 1
and so never generated an ordinal the start-at-1 rule rejects — it could
not tell round 1 from round 2 on B2. Each entry may now carry a decoy prose
line beginning with a non-1 ordinal, placed where it cannot end a run
(before the first headless entry; first in a heading body), followed by a
`status: resolved` that must never become a field; and a decoy-only
heading body must yield no entry.
The N2 assertion accepted a tab, which round 1's `\s` accepted too, so a
`[ \t]` → `\s` revert alone stayed green. NBSP, form-feed and vertical-tab
are now asserted refused — the assertion that fails on that revert on its
own, and the disclosure that `[ \t]` narrows what round 1 accepted.
* fix(#3702): the splitter records its own opener flags; an opener clears the blank-line memory
Round-review continuation, two state defects in the ordered-run logic:
- `blankSeen` survived the headless splitter's opener branch, so an opener
followed by a lazy continuation line read as "paragraph after a blank" and
ended the run — `1. a` / blank / `2. b` / lazy / `3. c` folded `c` into `b`.
The opener branch now clears it.
- The heading path re-derived per-line opener flags for headless regions
without the paragraph reset, re-accepting a rejected `3. status: resolved`
under a stale run and stripping it into a field. `GapsEntrySpan` now
carries the flags the splitter itself computed, and the heading path reads
them; the re-derivation is deleted.
* fix(#3702): ordered-run memory is per indent — nested runs resolve, nested ordinals never inherit the top-level run
Round-review continuation 2: nested openers consulted the TOP-LEVEL run
flag and never wrote their own, so a nested `1. / 2.` run under a hyphen
entry rejected its `2. status: resolved` (round 1 resolved it), while a
nested `3. status: resolved` under a nested `- ` bullet inherited an open
top-level run and was stripped into a false field.
`OrderedRuns` keys the memory by indent: a new opener at indent d resets
every deeper level, a paragraph after a blank at indent d ends the runs at
d and deeper, a thematic break or a heading clears all. Both splitters use
it; the top level still decides entry boundaries, nested levels decide
only which continuation lines are accepted openers for field stripping.
Pinned for LF and CRLF.
* fix(#3702): run levels — one top level at or above the base, CommonMark column indents, a fence ends its level's runs
Round-review continuation 3:
- A dedenting top-level list (` 1.` / ` 2.` / `3.`) lost its entry
boundaries: the exact-indent run lookup rejected the shallower ordinals
before the boundary check ran. Every indent at or shallower than the
list's base is now ONE level, in both splitters.
- `indentOf` counted characters, so a tab and a space aliased to one level
and `\t1. nested` / ` 2. status: resolved` resolved falsely. Indent is now
measured in CommonMark columns (§2.2: a tab advances to the next multiple
of 4), for the run level and the entry-boundary check alike.
- A nested run survived a fenced block. A fence is a non-list block: its
opening delimiter ends the runs at its level and deeper, exactly as a
paragraph after a blank does.
* fix(#3702): the indent measure is grammar-scoped — Gaps keeps next's character count
`blockStructure: false` promised the Gaps grammar byte-for-byte parity with
`next`, but the CommonMark-column indent measure added for the deferred
grammar was shared by the whole splitter core, so tab-indented Gaps input
changed entry boundaries in BOTH directions:
`\t- a` / ` - b` — next folded into one entry, HEAD split into two
` - a` / `\t- b` — next split into two, HEAD folded into one
`indentWidth` now keys the measure on the grammar: columns for the deferred
set, raw character count for Gaps. The opt-out covers indent semantics, not
only fences and thematic breaks.
Four cases pin both halves — the two flipped Gaps pairs, the two Gaps pairs
that never moved, and the same tab/space pairs on the deferred path returning
the opposite (column-measured) verdict by design.
* fix(#3702): the acknowledge path reads and writes through one classifier
Round 3, Blockers 1 and 3, and Minors 7 and 8 — one mechanism, so one commit.
Every consumer of an entry's lines now reads the splitter's own per-line
verdict instead of a re-derivation of it.
B1. Round 2 widened the WRITER's status-line finder to the deferred marker set
while `extractGapEntryFields` still de-bulleted line 0 only. A nested
` * status: pending` was therefore selectable by the writer and invisible to
the reader: acknowledge rewrote it in place, returned `ok`, and the item stayed
outstanding on every later audit. Measured against a `next` build, `*`, `+` and
`1.` each resolved on base and stopped resolving at round 2's head — a
regression, not a gap in new behaviour. The hyphen form of the same shape was
already broken on `next` and is fixed here too: one classifier cannot be right
for three markers and wrong for the fourth.
`parseGapEntryFieldLine` is now the single place a line is classified as a
field, and it reports the offset at which the VALUE begins. The rewrite happens
at that offset rather than through a second regex, so a line the classifier can
select is one whose rewrite it has already located — the selection and the
rewrite cannot disagree. Both `DEFERRED_STATUS_FIELD_RE` and
`DEFERRED_STATUS_REWRITE_RE` are deleted rather than widened. A read-back guard
returns `rewrite_not_readable` rather than `ok`; it is unreachable by
construction today and is the fail-loud floor under the next divergence.
B3. This is the end state the round-3 review prescribed on both #3739 and
#3773: #3773's shared classifier, parameterised by this PR's marker set, with
this PR's two status regexes deleted. #3773 lands first. Its hyphen-only strip
is consistent with `next`'s hyphen-only splitter today, so the writer/reader
divergence is created by THIS merge, which is why widening every consumer
belongs to the PR that widens the domain.
m7. The heading path marker-stripped its lines before calling the reader, so
the reader's fence scan ran over text the splitter never saw: `- ```sh` is an
ordinary bullet to the splitter but strips to a fence opener, and a
`**Status:** resolved` after it was suppressed as fence content — a resolved
entry resurfaced as open. Stripping now happens inside the reader, after the
fence scan.
m8. `rawGapEntryText` stripped a marker off line 0 unconditionally, but on the
heading shape line 0 is the heading TEXT: `### 1. Race in the writer` was
silently renamed to `Race in the writer`, and the name is the key acknowledge
matches on. Line 0 is stripped only when the splitter accepted it as an opener.
Also removed: `splitDeferredHeadingEntries`, whose sole caller only null-checked
it (round 3, M4 — the claim was zero callers, which was wrong; the wrapper's
`.map` was waste at the one call site), and `stripLeadingBulletMarker`, which
this change leaves with no callers at all. The export surface narrows to the two
splitter regexes the behavioural parity test reads (M6).
[PEER-ASK pr-order-12d5]
q: Reviewer blocked both on merge order. I'm declaring #3773 lands first and
building the end-state shape into #3739 now (both my status regexes
deleted). Does that match your plan?
reply: CONFIRMED - same order, derived independently. #3773 cannot carry the
fold: `DEFERRED_BULLET_MARKERS`/`BulletMarkers` have zero occurrences at
`next` (verified), so the prescribed end state is not executable inside
#3773 without absorbing this PR's work.
deadline: 03:55 UTC (answered before it)
fallback: declare #3773 first, adopt end-state shape in #3739, push+comment
decision: proceeded as stated; #3773 lands first, this PR carries the widening
of every consumer.
Refs #3740
* test(#3702): pin the detect/strip symmetry, and drop a white-box test that could not reach it
Round 3, Blocker 2 and Minors 6 and 9.
B2. The regression shipped green because no fixture put a marker on a nested
status line. Four markers x {nested status line}, each asserting the entry
READS BACK as acknowledged rather than that acknowledge merely reported `ok` —
reporting `ok` over a line the reader skips is the whole defect. Plus the bare
capitalised `Status:` case (the reader stores it case-sensitively, so the
writer must not select it), and an idempotence test, which is the failure the
defect actually produced: the item resurfaces, is acknowledged again, and never
settles.
Each of these was run against the pre-fix build first: all five fail there and
pass here. Two further assertions in the block are labelled CONTROL because
they held pre-fix — they guard the new offset-based rewrite and the opener-flag
threading against regressing, and calling them regression tests for a reported
defect would overclaim.
M6. The round-2 parity test asserted that four writer-side regexes embedded the
same source string. That is true of a detect/read asymmetry too, so it could
not have caught B1 — and two of the four regexes were widened into `export =`
purely to let it read them. Replaced with a behavioural test that drives the
real seam: every marker that opens an entry must also resolve it through
acknowledge. The structural assertion is kept for the two splitter regexes,
which really are two copies of one alternation.
m9. `expectedResolved` was computed and immediately voided; the loop beneath it
already asserts both polarities.
m7/m8 coverage lands here too: a bullet whose content is a fence opener must
not suppress the entry's fields, and a heading beginning with a list marker
must keep it in the entry name.
* docs(#3702): document the deferred-items entry shape where the file is written
Round 3, Major 5, and #3702's own item 2. The widened grammar was documented in
the reader (`forensic-audit.md`) but not at the write site, where
`executor-examples.md` still said only "log to deferred-items.md" — so the
question the issue actually raised, which shapes count, remained unanswered
anywhere a human writes the file.
States what opens an entry (`-`, `*`, `+`, and `1.` when the list starts at
`1.`), that `1)` is not a marker here, that a separator closes the list and
fenced content is never an entry or a field, and that an entry without an
explicit `status: resolved` stays open by design.
* chore(#3702): regenerate the changeset through the generator
Round 3, Minor 10. The fragment was hand-named against 64 generated names on
`next`, and its body ran ~250 words against CONTRIBUTING's one-sentence form.
Regenerated via `npm run changeset`, which is also what the random three-word
name is for: concurrent PRs never collide.
* fix(#3702): the fence gate lives on the seam both sides call, not just the reader
Found by the pre-push adversarial review of this round, and it is a regression
this round introduced rather than a pre-existing one.
`extractGapEntryFields` applied `fencedLineSet` before classifying; the
acknowledge writer's status-line search did not. So a `status:` line inside a
fenced block was SELECTED by the writer and SKIPPED by the reader — the write
produced a line nothing reads, the read-back guard refused it, and the entry
became impossible to acknowledge at all: `audit acknowledge` raised an internal
error and `complete-milestone` halted on it.
Measured, `- alpha` / fence / ` status: pending` / fence:
next ack=ok -> reads back "acknowledged"
round-2 head ack=ok -> reads back "" (the B1 defect)
before this ack=rewrite_not_readable -> refuses entirely (worse than next)
`entryFieldLines` is now the seam — per line of an entry, the field it declares
or `null`, fences included — and the reader and the writer both go through it.
That makes "the writer cannot select a line the reader will not read back"
structural rather than asserted, which is what the previous commit's message
claimed while a second read-side filter still lived outside the classifier.
Two comments corrected with it. The read-back guard is NOT "unreachable by
construction": this round shipped a reachable path to it, which is precisely
what an invariant asserted in a comment is worth. And the M6 replacement test
put its marker only on the entry opener, so it passed against the defective
build — the exact weakness it was introduced to fix in round 2's test. It now
marks the nested status line too, and fails pre-fix like the rest.
Round-3 tests against the pre-fix build: 10 of 12 fail there, and the 2 that
hold are labelled CONTROL because they guard this round's new code rather than
pin a reported defect.
* fix(#3702): one end-of-file CRLF algorithm, adopting #3773's with its B4 closed
Round-4 M1. Two open PRs shipped two different answers to "what line ending
does an entry that ENDS THE FILE get?", and the review's ruling was that the
disagreement needs one answer, not two. Neither shipped answer was that one.
Measured on builds of both heads:
case #3739 r3 #3773 here
undelimited single entry, CRLF preamble pass FAIL pass
LF-dominant list, one stray CRLF at EOF FAIL pass pass
(the other five) pass pass pass
This PR's content.endsWith('\r\n', matchIndexInContent) reads the terminator of
the PREVIOUS line, so it propagated an isolated CRLF into an LF-dominant list --
refuted by #3773's own LF-dominant fixture, ported here. Withdrawn.
#3773's crlfAtEof asks the right question -- does anything before the entry,
within scope, contradict CRLF -- and fails closed. But its scope goes EMPTY for
an undelimited single-entry list, because the entry-list region runs from the
first entry's start to the insertion point and those coincide; crlfAtEof('') is
false by its own before.length > 0 guard, so 'preamble\r\n\r\n- alpha' gained a
bare \n in a CRLF document. That is #3773's B4, verified by driving its head.
Adopted here with the scope widened to everything preceding the insertion point
where the preferred region is empty, rather than asserting LF from no evidence.
That only ever loosens a scope carrying zero information, and the predicate
stays fail-closed over the wider one. An entry at offset 0 of an undelimited
document has no evidence under either scope and stays LF.
Tests: 10 added. Negative control, driven -- 1 of the 10 fails against this
branch's own pre-fix head (the stray-CRLF fixture); B4 fails against #3773's
head; the remaining 8 are the scope counterexamples ported with the function,
which were regression pins in #3773 and are guards here. Each still kills a
simpler algorithm: drop any one and a refuted scope passes again.
Four deferred-items suites 450/450, 0 skipped. npm run lint:ci exit 0.
* fix(#3702): drop the unreachable rewrite_not_readable guard (B3)
Round-4 B3: the status had zero test coverage in either file. The review
offered two branches -- drive it from a test, or delete it and stop carrying an
untested terminal status. Taking the second, with the reason stated rather than
assumed.
Why it cannot be driven. Round 3 added the guard after a fenced `status:` line
proved the writer could select a line the reader would not read back. Round 3
then closed that divergence STRUCTURALLY, by routing the writer's line selection
and the reader's field extraction through one entryFieldLines seam. The guard
now detects a state construction prevents: 21 document shapes were driven
against it -- fence openers on the bullet line for every marker in the widened
set, duplicate and triplicate status lines, bolded and nested variants, fences
between duplicates -- and none reached it. The only seam that would is routing
the internal call through the module's exports so a test could stub it, which
reshapes production surface for a test.
Why leaving it undriven is not free. RULESET.TESTS.mutation-score runs Stryker
incrementally over changed files at an 80% threshold and says to treat a
surviving mutant as a failing test specification. An undriven `if` on a changed
file is exactly that, on both the condition and the .toLowerCase() comparison.
What this gives up, stated rather than hidden: if a future change re-splits the
writer's selection from the reader's extraction, acknowledgeDeferredItem returns
ok over an item that stays outstanding -- the original #3702 defect class. One
correction to the review's framing: match_verification_failed does NOT backfill
it. That check runs BEFORE the write and compares the matched span to the
target, so it cannot see a post-write read-back failure. The protection against
re-splitting is the shared seam and the round-3 tests that pin it, not a runtime
assertion. A comment at the removal site records all of this.
Removing it also drops the union member from both files, which resolves the PR
body's internal contradiction (it claimed no type-signature changes while adding
one) and the duplicate-status surface #3773 collides on.
No test changed behaviour: 450/450 across the four deferred-items suites, 149/149
across the audit suites, npm run lint:ci exit 0 -- the same figures as before the
removal, which is itself the evidence that nothing exercised the branch.
* fix(#3702): the deferred fence gate is indent-unbounded, like the rest of the grammar (M2)
Round-4 M2. scanFencedBlocks is CommonMark, which caps a fence delimiter's
indent at three spaces -- a fourth makes it an indented code block instead. This
grammar had already opted out of that cliff for entry openers ([ \t]*) and for
THEMATIC_BREAK_RE (^[ \t]*), but not for fences. So a fence at four spaces was
not a fence to the gate, and a `status: resolved` line inside it RESOLVED the
entry containing it.
That is not an exotic shape. A fenced block written under a NESTED bullet sits
at four spaces, so ordinary hand-written deferred-items.md files reach it.
Driven before the fix at indents 4, 5, 8 and a leading tab: all four silently
resolved. It is the #3702 silent-resolution defect class in a new place.
gsd-core/references/executor-examples.md, added by this PR, states flatly that
"nothing inside a fenced code block is an entry or a field". The review offered
fixing the parser or bounding that claim in three places. Fixing it -- the claim
is the one users will rely on, and the grammar had already chosen unbounded
indent everywhere else.
NO second fence dialect (the rule blankIndentedFenceDelimiters states). The
classification is still done by scanFencedBlocks, the one exported CommonMark
state machine, over a de-indented VIEW of the same lines. Run lengths, backtick
vs tilde, closer-must-match-and-not-trail, info-string rules and the
unterminated-at-EOF case remain that engine's answers. Indent is the only
dimension hidden from it, and it is exactly the dimension this grammar has
already declared it does not measure. Index alignment is 1:1 -- map preserves
length -- so every returned line index still addresses the original line.
Scope is the deferred grammar only. Both marker-parameterised call sites gate on
markers.blockStructure, which the Gaps set does not set, so Gaps reaches an empty
set. Verified, not asserted: the 47-fixture Gaps differential (marker x
line-ending x separator x fence x break x key-shape x list-shape) is
BYTE-IDENTICAL across this change, 8033 bytes both sides.
Tests: 14 added, of which 8 fail against the pre-fix source and pass here; the
other 6 are the deliberate controls -- indents 0 through 3, which must NOT move,
and the Gaps opt-out guard.
Four deferred-items suites green; the 58 suites touching uat/deferred/sectionizer
run 6045 tests with an IDENTICAL failing set before and after this change (17
pre-existing environment failures -- installs and an unpinned GSD_EMITTED_BASE;
emitted-attribution passes 259/259 in isolation with its base pinned). lint:ci
exit 0.
* fix(#3702): changeset, both prose parsers, and the minors (M3, M4, m1-m3, m5, n1-n2)
M3 -- the changeset omitted a user-BREAKING change. Measured against next: a
heading-delimited deferred-items.md written with `*`, `+` or `1.` went from
"0 entries, so complete-milestone has nothing to acknowledge and closes" to
"1 entry, the CLI writer refuses the heading shape, ACK_FAILURES accumulates,
exit 1". The `-` form already halted and is unchanged. That is release-note
material: a close that used to succeed now fails, and the correct response is to
fix the file, not revert. Also names the fence-indent fix below, and adds #3740
so #3773's issue is attributed here as it is absorbed.
M4 -- gsd-core/workflows/progress/steps/forensic-audit.md is a SECOND,
model-executed parser of the same grammar, and prose cannot carry a parity test.
Its widened text stated the start-at-1 rule, fences and separators but not the
`1)` exclusion nor the nine-digit ordinal cap, both enforced in code with pinned
tests. Both stated now, along with the round-4 fence-indent rule. (No ack
fragment: the size ratchet's currentSizes does a NON-recursive readdirSync of
gsd-core/workflows and agents, so a file under workflows/progress/steps/ is
outside its scope -- verified by reading the helper, not by the green.)
n1 -- executor-examples.md documented that the BOLDED status key is matched
case-insensitively and left the bare key's rule to inference. Driven: bare
`Status: resolved` is NOT read, so the entry stays open with no warning, while
`**Status:**` is. Stated explicitly, with the digit cap and the any-indent fence
rule (n2).
m1 -- boundary coverage was 2/3. limit (999999999.) and limit+1 (1234567890.)
were pinned; limit-1 (12345678.) added, per RULESET.TESTS.boundary-coverage.
m2 -- THEMATIC_BREAK_RE and the tab-expanding indent counter are hand-rolled
CommonMark rules with no in-repo peer to compare against, so the parity
assertion is against the SPEC: eight positive and five negative fixtures, plus
the two DELIBERATE divergences pinned as deliberate (`+` is a separator here but
not in CommonMark, because `+` is a list marker in this grammar and `+ + +`
would otherwise be a phantom entry; indent is unbounded). One fixture was
initially wrong -- `-- -` IS a CommonMark break, since the spec allows free
spacing between the three characters -- and the parser was right.
m3 -- the result union is hand-duplicated in audit.cts as part of a deliberate
structural view of uat.cjs, so the fix is not to delete a copy but to make drift
observable. Every REACHABLE status is now driven from a fixture; four of the six
(ambiguous, unsupported_heading_shape, already_resolved, match_verification_failed)
had no assertion anywhere in the suite before this. match_verification_failed is
still undriven and the test says so rather than omitting it.
m5 -- DECLINED, with the measurement. The review is right that `(\s*)` in the
opener and `/^[ \t]*/` in the reader disagree about \f, \v and NBSP, but its
prescribed narrowing was implemented, driven and REVERTED: as shipped, an entry
indented with any of those surfaces, parses its status field, acknowledges, and
reads back acknowledged -- a complete round-trip. Narrowing turns all three into
SILENTLY DROPPED entries, which is the #3702 defect class itself and the opposite
of this file's stated fail-safe rule. A latent inconsistency in the safe
direction is not worth a live regression in the unsafe one. Pinned by three
round-trip tests so the prescription cannot be re-applied silently; if it is ever
closed, the direction is to make the readers agree with the opener, not to make
the opener reject lines it accepts today.
Four deferred-items suites 475/475, 0 skipped. lint:ci and lint:changeset exit 0.
The 47-fixture Gaps differential is byte-identical at 8033 bytes.
* fix(#3702): the pinned `## Gaps` phantom now cites its issue (m4)
Round-4 m4. The second assertion in the Gaps byte-for-byte test pins a real
defect as expected output: a spaced hyphen thematic break in `## Gaps` is read
as an ITEM, so `- - -` surfaces a phantom open gap named `- -`. Reproduced on
pristine next at
|
||
|
|
996196fe02 |
fix(#4099): stop word-splitting an unquoted PLAN_IDS string (bash vs zsh diverge) (#4102)
* fix(review): stop word-splitting an unquoted PLAN_IDS string (bash vs zsh diverge) #3301's plan-coverage-manifest block built a space-separated PLAN_IDS accumulator and re-split it via unquoted `for x in $PLAN_IDS`. Bash word-splits an unquoted scalar on IFS by default; zsh does not, so every plan id collapsed onto one iteration under zsh whenever a review had 2+ plans, undercounting the manifest and merging bullets onto one line. This is exactly what was failing origin/next's CI on every push. Derive PLAN_COUNT and the bullet list directly from the same *-PLAN.md glob loop used for copying — glob iteration, unlike re-splitting a stored string, is identical under bash and zsh, so it never needs word-splitting. Closes #4099 Emitted-Drift-Ack-Growth: review.md — fix comment documents the bash/zsh word-splitting divergence (gsd-core#4099); net code is smaller, comment expansion grows the file * docs(#4099): add changeset for the plan-coverage-manifest zsh fix * docs(#4099): fix changeset body to match the bold-dash-explanation format Code review (standards axis) flagged the changeset body as not matching CONTRIBUTING.md's canonical `**<Bold>** — <explanation>.` format — the bold clause had its own trailing period instead of being followed directly by the em-dash separator. * chore(#4099): backfill changeset PR number (#4102) --------- Co-authored-by: sim <sim@local> |
||
|
|
6beaa66b25 |
enhance(#3304): gate re-verification blockers on deterministic evidence (#4085)
* test(#3304): add failing-first suite for the convergence evidence gate Content-assertion suite for the Step 7 re-verification evidence gate (agents/gsd-verifier.md / gsd-core/references/verifier-evidence-gate.md). Committed before the implementation to prove RED via gsd-test. * enhance(#3304): gate re-verification blockers on deterministic evidence Step 7's anti-pattern scan re-runs at full, unbounded scope on every re-verification pass, independent of the must-haves established in Step 2. A blocker it finds — other than the self-evidencing debt-marker check — previously reverted a completed gap-closure round and started another --gaps cycle on nothing more than the verifier's own new judgment call, with no bound on how many times that could repeat. A Step 7 blocker now blocks unconditionally in re-verification mode only if it is a carried-forward gap (present in the prior VERIFICATION.md's gaps: list) or the flagged file was git-modified since the prior pass (a regression; fails closed toward blocking when history is unresolvable). Otherwise it predates the gap-closure round unflagged and needs deterministic evidence — a named test run red, or another concrete reproducible artifact — to stay blocking. Unevidenced, it downgrades to a new advisory: frontmatter list and report section instead of setting status: gaps_found, and never reverts a completed must-have. Maintainer approval was narrowed to this evidence condition only, explicitly rejecting the broader "advisory whenever untraceable to a requirement/decision/prior-gap" proposal — implemented and pinned by tests/verifier-evidence-gate.test.cjs and documented as rejected in gsd-core/references/verifier-evidence-gate.md so it can't silently re-expand. Closes #3304 * fix(#3304): correct window-truncation and indentation bugs in evidence-gate tests gsd-test's GREEN checkpoint caught 3 real bugs in the test file itself (not the production prose): a {0,600} match window was shorter than the 724-char paragraph it was scanning (the "exclude from Step 9 Rule 1" phrase starts at offset 662), and two regexes assumed no indentation after a markdown list-continuation line break. All three phrases are confirmed unique across agents/gsd-verifier.md, so the windowed submatches are replaced with direct whole-string assertions instead of just widening the window. Also acknowledges the deliberate byte growth in agents/gsd-verifier.md that the differential-attribution check (ADR-2719) correctly flagged. Emitted-Drift-Ack-Growth: gsd-verifier.md — adds the #3304 re-verification evidence gate (Step 7 rule, Advisory bucket, advisory: frontmatter, report section); 1488 bytes, still within the LARGE-tier 48 KiB cap (48751/49152). * docs(#3304): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
62b0d939b6 |
feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey (#4083)
* feat(#3274): make reviewer-lane timeout configurable via timeoutConfigKey Add an optional `timeoutConfigKey` field to the reviewer lane descriptor, resolved in `resolveLanePlan` at invocation time and falling back to the frozen `timeoutFloorMs` when unset or invalid, in the same spirit as the existing `promptBudgetKey`/`modelConfigKey` fields. All 12 shipped lanes declare `review.timeouts.<slug>` on both surfaces (the descriptor and their capability.json manifest), validated by capability-validator.cjs. For the antigravity lane, the native `agy --print-timeout` flag — previously a second hardcoded literal (`540s`) independent of the outer cap — is now derived from the same resolved outer timeout in `antigravityArgv`, preserving the existing 60-second buffer relationship (ADR-2782 D6: the outer bound is declared data, the inner one is handler-owned). The antigravity default timeoutFloorMs stays at 600s per the maintainer's disposition; users raise it through the new config key instead. * docs(#3274): document review.timeouts.* and extract resolveTimeoutMs helper Address code-review findings on the timeoutConfigKey change: extract the inline timeout-resolution logic into a named, exported, directly-tested resolveTimeoutMs helper (matching the file's existing configString/ normalizeHost convention); document the new review.timeouts.* federated config keys in docs/CONFIGURATION.md, docs/reference/capability-manifest.md, and docs/how-to/ship-a-reviewer-lane.md; add the changeset fragment. * fix(#3274): resolve native antigravity timeout in resolveLanePlan, not the runner gsd-test caught two design mistakes in the prior commits: 1. SpawnPlan.argv is documented and tested as fully resolved by resolveLanePlan (model/effort/output/prompt already folded in) — leaving the antigravity '{{nativeTimeout}}' marker unresolved until the runner's antigravityArgv violated that contract and broke tests that read plan.argv directly (tests/antigravity-reviewer.test.cjs, tests/review-default-reviewers-workflow.test.cjs). Fix: '{{nativeTimeout}}' is now a fifth ARGV_PLACEHOLDER member, resolved by resolveLanePlan itself via the new nativeTimeoutToken() helper, exactly like the other four. antigravityArgv reverts to its pre-#3274 four-argument form. Also missed updating capabilities/antigravity/capability.json's invoke.args to match the descriptor, which broke the manifest/descriptor parity test. 2. tests/reviewer-config-federation.test.cjs enforces a deliberate, narrow invariant (#3691 narrows #2797): qwen, cursor, and coderabbit — the three lanes with neither a model flag nor a host — may own no config key beyond their own prompt-budget key. Adding review.timeouts.<slug> to all 12 lanes violated it. Fix: those three keep timeoutConfigKey: null and own no review.timeouts.* key, matching their existing modelConfigKey: null. The other 9 lanes are unaffected. * chore(#3274): backfill changeset PR number (pr:0 -> 4083) --------- Co-authored-by: sim <sim@local> |
||
|
|
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> |
||
|
|
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> |
||
|
|
e4df05126d | chore: promote CHANGELOG for v1.8.0 | ||
|
|
360b3ebc5a |
chore(#2496): clear five newly-disclosed production advisories (#2497)
* chore(#2496): clear five newly-disclosed production advisories The `#3588: npm audit --omit=dev reports zero advisories` gate began failing mid-release. The v1.8.0 finalize dry run was green at 17:27:27Z; GHSA-frvp-7c67-39w9 and GHSA-xgm2-5f3f-mvvc published at 18:17:25Z and 18:18:13Z, with fast-uri and two further hono advisories in the same window. Nothing in the tree changed — the advisory database did. All five arrive transitively through the one declared dependency @anthropic-ai/claude-agent-sdk -> @modelcontextprotocol/sdk. Two-part fix, both following existing repo precedent: 1. `npm audit fix --omit=dev` (no --force) re-resolves fast-uri and hono inside their already-declared ranges. package.json untouched — the same approach as .changeset/witty-badgers-hum.md (body-parser) and .changeset/archived/fix-3588-npm-audit-clean.md. Clears the only high. 2. overrides["@hono/node-server"] = ">=2.0.5" for the remaining chain, which cannot resolve in-range (^1.19.9 cannot reach 2.0.5) because @modelcontextprotocol/sdk@1.29.0 is already latest and still declares the vulnerable range. Extends the block that already pins qs and body-parser. Resolves to 2.0.11. Bumping @anthropic-ai/claude-agent-sdk to ^0.3.x was tested and REJECTED: 0.3.216 moves @modelcontextprotocol/sdk to peerDependencies, which npm auto-installs, so the chain survives and resolution pulls extra advisories — 5 vulnerabilities including a high, versus 4 moderate. Forced major sits under a dependency no tracked source imports (see src/mcp-server.cts:22 — the JSON-RPC loop is hand-rolled precisely to avoid the MCP SDK), so runtime risk is minimal. Revisit once upstream ships a release depending on patched @hono/node-server. npm audit --omit=dev: found 0 vulnerabilities. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#2496): add changeset fragment for the advisory clearance --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
3435218089 |
fix(#2452): stop shallow-fetching the base ref in three-dot-diff CI gates (#2485)
* fix(#2452): stop shallow-fetching the base ref in three-dot-diff CI gates `mutation.yml` re-fetched the base branch with `--depth=1` after checking out with `fetch-depth: 0`. The shallow re-fetch truncates the base ref's ancestry, so `git diff --name-only origin/<base>...HEAD` in scripts/mutation-matrix.cjs can no longer compute a merge base and aborts with `fatal: origin/next...HEAD: no merge base` (exit 2). The `detect` job then fails and the `mutate` shards never run — so the 80% mutation-score threshold went UNVERIFIED rather than enforced. The failure is branch-position dependent, which is why it went unnoticed: a branch already level with the base incidentally passes (its merge base IS the single fetched commit), while a branch that is BEHIND fails. Observed on PRs #2436 and #2005. `changeset-required.yml` and `docs-required.yml` shallow-fetched the base ref too (`--depth=50`), shrinking the same window further. All three now fetch the BASE REF unshallowed. Their shallow *checkout* depth is left at 50: that is a separate, deliberate cost control with fail-closed semantics, owned by tests/policy-lint-shallow-checkout.test.cjs. Only the base-ref fetch changes. The two `${{ }}` interpolations in mutation.yml's run: blocks now pass the base name through `env:`, matching the sibling workflows. Regression coverage in tests/mutation-workflow-base-ref.test.cjs: - a per-workflow contract guard asserting the base fetch carries no --depth (RED on origin/next for all three files, GREEN here). The YAML step parser handles block scalars and skips commented-out steps, so a future refactor to a multi-line `run:` cannot silently degrade the guard. - a real-git mechanism proof with boundary coverage at the shallow edge: with the base advanced 60 commits past the branch point, --depth=1 and --depth=60 both fail with `no merge base`, --depth=61 (merge base exactly at the boundary) succeeds, and an unbounded fetch succeeds. Each variant uses an independent clone, because a plain fetch does not un-shallow a repo that already carries a .git/shallow boundary. Also fixes a startup race in tests/run-with-timeout.test.cjs surfaced by this branch's gsd-test run (C1, linux-node24). The heartbeat file only appeared ~100ms after the grandchild's runtime was up, but the window is 1s spanning two cold node starts, so on a loaded runner the timeout fired before any heartbeat existed and the precondition failed for reasons unrelated to reaping. The child now writes its heartbeat once synchronously at startup; the frozen-vs-ticking comparison that actually proves reaping is unchanged. Closes #2452 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(changeset): backfill PR number to 2485 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
c5e0371775 |
feat(#1951): reversibility tagging — gate one-way-door decisions (#2471)
* test(#1951): add failing-first tests for reversibility tagging Red phase for issue #1951 (reversibility tagging: classify decisions by undo cost, gate one-way doors behind a checkpoint:decision). Tests assert, per the issue's acceptance criteria: - discuss-phase CONTEXT.md template records a **Reversibility:** field with a rationale on captured decisions, and states it is optional - gsd-planner @-references planner-reversibility.md and stays under the 49152-char agent cap (LARGE_CAP, tests/agent-size-budget.test.cjs) - a one-way rating inserts a checkpoint:decision before the dependent task; reversible inserts none; costly is flagged but never blocks - the taxonomy defaults to reversible when unsure (checkpoint-fatigue guard) and inserting a checkpoint implies autonomous: false - docs/reference/plan-md.md documents <reversibility> as optional with all three ratings - --no-reversibility-gates parses to REVERSIBILITY_GATES=false, is injected into the planner prompt, and is advertised in the command argument-hint and help full mode (argument-hint parity) - the override suppresses the gate but still persists the rating - cmdVerifyPlanStructure accepts every rating and the absent case (additive-validator guarantee, behavioral via runGsdTools) - parity: thinking-models-planning.md #4 adopts the canonical three-level taxonomy and the binary REVERSIBLE/IRREVERSIBLE vocabulary is gone - no content loss from the planner extraction made to fit under the cap Prose-contract assertions are Red until the implementation lands. The behavioral validator assertions pass immediately — regression guards proving the validator already accepts unknown optional tags. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#1951): reversibility tagging — gate one-way-door decisions Classify planning decisions by what undoing them would cost, and give a one-way door a human beat before the agent walks through it (issue #1951, The Pragmatic Programmer Topic 15 'Reversibility'; Bezos's one-way/two-way door framing). Acceptance criteria met: - discuss-phase records an optional reversibility rating with a rationale on <decisions> entries in the phase CONTEXT.md template. Unrated decisions are treated as reversible, so existing phases are unaffected. - a one-way rating makes gsd-planner insert a checkpoint:decision before the task that implements the decision, reusing the existing checkpoint mechanism -- no new checkpoint machinery. - reversible ratings trigger no checkpoint; costly ratings are flagged in the plan but never block. - the rating persists on the task as the optional <reversibility rating=> element. cmdVerifyPlanStructure accepts every rating and the absent case; the structural validator does not reject unknown optional tags. - --no-reversibility-gates (REVERSIBILITY_GATES=false) suppresses checkpoint insertion for intentionally-unattended runs while still recording ratings -- the override changes what stops the run, not what the plan remembers. Single taxonomy, not two: references/thinking-models-planning.md #4 already shipped a binary REVERSIBLE/IRREVERSIBLE classification and is loaded by both gsd-planner and gsd-plan-checker. It is rewritten onto the canonical three-level vocabulary and now points at planner-reversibility.md as the taxonomy owner, with a parity test that fails if the surfaces diverge (DEFECT.GENERATIVE-FIX-DIVERGENCE). agents/gsd-planner.md sat 47 chars under the 49152 LARGE_CAP, so the checkpoint DO/DON'T guidance was relocated verbatim into planner-antipatterns.md -- already @-referenced from the same section for the same topic, so the planner still loads it and nothing was dropped. A test guards the relocation against content loss. Files: gsd-core/references/planner-reversibility.md (NEW, canonical taxonomy + emission rules + anti-patterns), gsd-planner.md, plan-phase workflow/command/help (flag wiring + parity), plan-md.md schema, discuss-phase context template, CONTEXT.md glossary, INVENTORY + manifest, size baselines, install goldens, plugin skills regen, changeset. Closes #1951 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#1951): address orthogonal review findings Two isolated reviewers (correctness + security), neither of which authored the change. Every finding fixed: Security — the rationale is untrusted input (ADR-1577). It originates in conversation and flows CONTEXT.md -> planner -> PLAN.md -> executor, each hop an LLM reading the previous hop's output, with no validation on the path. planner-reversibility.md and the discuss-phase template now state it is data and never instructions, and name the </reversibility> early-termination hazard explicitly -- a rationale that closes its own element injects sibling structure the executor reads as real tasks. Four tests guard it. Correctness 1 — nothing machine-enforced the feature's own promise: a task rated one-way with no preceding checkpoint:decision validated as fully clean, so a planner error silently reopened the gap this feature exists to close. cmdVerifyPlanStructure now warns on an ungated one-way rating. A warning, not an error: <reversibility> stays additive and the plan stays valid. Four tests cover ungated (warns), gated (silent), still-valid, and reversible/costly never flagged. Correctness 2 — pass-always test. The --no-reversibility-gates parse test substring-matched the whole workflow file, and plan-phase.md prose mentions both tokens in one sentence, so it passed with the bash conditional deleted: it was testing the documentation, not the parser. Now scoped to the fenced bash blocks and matched as one physical line, with a negative control confirming prose alone cannot satisfy it. Correctness 3 — costly had no itemized emission rule, only one-way did, so two agents could diverge on whether to tag costly at all. Correctness 4 — template convention break: the example ratings were bare while every sibling field uses [...] to signal substitution, inviting an LLM to copy one-way/costly forward as boilerplate. Now bracketed. Correctness 5 — latent false-green: .includes('reversible') also matches inside irreversible/irreversibility, which appear in anti-pattern prose, so a surface that dropped the real taxonomy entry would still pass. Now word-boundary matched. ADR-857 phase-6 ceiling — the first gsd-test run caught plan-phase.md 1216 bytes over its frozen 94519 ceiling (it had 49 bytes of headroom on next). The ceiling may only rise for privileged host machinery, and reversibility gating is optional-feature logic, so the wiring was slimmed to its minimum and the explanatory prose moved to the reference files the planner already loads. plan-phase.md is now 94400 bytes -- 119 under the ceiling and 70 bytes SMALLER than on next, so the host loop shrank while gaining the feature, which is what phase 6 ratchets toward. The tracer contract (tests/tracer-bullet.test.cjs) is unchanged. Lint — fixed an unnecessary non-null assertion in verify.cts and a CRLF-fragile bare \n regex in the new test (DEFECT.WINDOWS-CRLF-TEST- PORTABILITY, the #1658/#1668/#2206/#2449/#2450 class). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1951): checkpoint fixture must carry the common task elements The gated-one-way fixture built a checkpoint:decision task from the abbreviated skeleton in gsd-planner.md, which shows only the checkpoint-specific elements (<decision>/<context>/<resume-signal>). cmdVerifyPlanStructure requires <name> and <action> on EVERY task regardless of type, so the fixture failed validation for reasons that had nothing to do with reversibility: errors: ["Task missing <name> element", "Task 'unnamed' missing <action>"] Caught by gsd-test on 14d14a39 (2 failures, both this fixture). The canonical shape is in tests/verify.test.cjs:266 — a checkpoint task carries <name>/<files>/<action>/<verify> like any other. Fixture corrected to match. Verified behaviorally against the real gsd-tools CLI across all four cases: gated one-way (valid, silent), ungated one-way (valid, warns), costly (valid, silent), absent (valid, silent). Not a product defect: the validator's every-task contract is intentional and pre-existing, and docs/reference/plan-md.md scopes its required-element list to type=auto/tracer only because those are the elements a planner must author, not because checkpoints are exempt from <name>. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1951): backfill changeset pr number to 2471 * fix(#1951): CodeQL incomplete-sanitization + prompt-injection scan collision Both CI failures were real defects in code this PR added, not false positives. CodeQL js/incomplete-sanitization (high), reversibility-tagging.test.cjs:46 — the namesRating helper built its regex with `rating.replace(/[-]/g, '\\-')`, which escapes the hyphen but not backslash, so the escape was incomplete. It was also unnecessary: `-` carries no special meaning outside a character class. Replaced with a complete metacharacter escape (backslash included). Word-boundary behavior verified unchanged across all three ratings — notably that "irreversible" prose still does not satisfy a "reversible" match, which is the false-green this helper exists to prevent. Prompt injection scan — the checkpoint fixture used the human-verification child element inside <verify>. That tag name is a fake-instruction-boundary pattern in scripts/prompt-injection-scan.sh, and the scan runs over changed files, so copying the shape from tests/verify.test.cjs (unflagged only because it is not in this diff) tripped the gate. Switched to the documented plain-prose <verify> form. The first attempt at that fix failed the same gate a second time: the comment explaining the collision quoted the offending tag literally. The comment now names it in prose instead — the scanner does not care whether a match is code or commentary, which is the whole point of the DEFECT.PROMPT-INJECTION-SCAN-COLLISION note in CLAUDE.md. Verified locally before push: scan reports 0 findings across 57 changed files, eslint clean, and both fixtures still validate as designed (gated one-way silent, ungated one-way warns, neither errors). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1951): record measured cost and halve gsd-tools spawns The Windows shard 1/3 job timeout was traced to the sharding layer, not to this PR's assertions — see #2472. Two contributing factors were this file's own, and are fixed here. 1. tests/test-timings.json had no entry for reversibility-tagging.test.cjs, so scripts/run-tests.cjs weighted it at the table's median fallback (~315ms) for LPT chunk packing. It actually measures 5595ms — an 18x under-weight. Recorded the measured value from the green gsd-test run (max across the node22/node24 lanes, per gen-test-timings.cjs's convention). Only this one entry: a full regen churns 634 entries of run-to-run drift, and the table is explicitly advisory and un-gated, so a 637-line diff does not belong in a feature PR. 2. Each verifyPlan() spawns gsd-tools, which dominates this file's cost. Spawns cut from 9 to 6 with no coverage lost: - the ungated-one-way warning and its stays-valid assertion now share one plan instead of building the same plan twice; - the reversible/costly never-flagged-as-ungated test was strictly subsumed by the additive suite, which already runs those two ratings ungated and asserts no /reversibilit/ warning at all — and the gate warning's text contains both "reversibility" and "one-way", so the broader assertion catches it. It only re-spawned gsd-tools twice to prove the same thing. Both are symptom fixes. The shard imbalance itself (19/11/10 minutes against a 20-minute cap, from a cost-blind round-robin partition that also reshuffles downstream files whenever one is inserted) is tracked in #2472. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1951): checkpoint fixture adopts the #2444 type-branched contract Surfaced by rebasing onto next, which gained #2444 (branch plan-structure validation on task type=checkpoint:*) while this PR was in review. cmdVerifyPlanStructure no longer applies one required-element set to every task. A checkpoint:decision now requires <name> + <resume-signal> + <decision> + <options>, and is exempt from the <action>/<verify>/<done>/ <files> set that auto and tracer tasks carry. The gated-one-way fixture predated that split and failed on the new requirement: errors: ["Task 'Task 0: Confirm the on-disk format' missing <options>"] Fixture rewritten to mirror the checkpoint:decision contract exactly — real <options> with two <option> children — rather than padding it with fields checkpoints no longer need. That also drops the plain-prose <verify> the earlier revision carried purely to dodge the prompt-injection scan; a checkpoint task has no <verify> requirement at all, so the workaround is moot. Verified against the real gsd-tools CLI across all four cases: gated one-way (valid, silent), ungated one-way (valid, warns), costly (valid, silent), absent (valid, silent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
04a0eb8d63 |
fix(#2450): CRLF-tolerant session-section rewrite + no-op-detection guard (#2482)
* fix(#2444): re-resolve body-parser to 2.3.0 in lockfile (GHSA-v422-hmwv-36x6) GHSA-v422-hmwv-36x6 (body-parser DoS via invalid limit value, low severity, published 2026-07-20T23:23:26Z) made tests/npm-integrity-gate.test.cjs (#3588: root workspace production tree has no advisories) fail any subsequent npm audit --omit=dev. The advisory affects body-parser >=2.0.0 <2.3.0 pulled transitively via @anthropic-ai/claude-agent-sdk -> @modelcontextprotocol/sdk -> express -> body-parser@2.2.2. express@5.2.1 already declares body-parser as ^2.2.1, so 2.3.0 is a valid re-resolution within express's own compatibility range — no override needed. Regenerated the lockfile via 'npm audit fix --omit=dev' which re-resolves transitive deps within their declared ranges; package.json is unchanged. Verified: npm audit --omit=dev reports 0/0/0/0/0 advisories; body-parser now reads as 2.3.0 in 'npm ls body-parser --omit=dev'. * test(#2450): failing-first CRLF regression for record-session insert path cmdStateRecordSession's section-rewrite regexes (src/state.cts:1166, :1195) used literal \\n which cannot match CRLF STATE.md delimiters. The detector regex (CRLF-tolerant via $ under /m) entered the rewrite branch, the writer regex silently no-op'd, but updated.push(...)/ sessionCreated=true ran unconditionally. Result: caller reported recorded:true with 'Resume File' in updated, but the field was never written to disk. With core.autocrlf=input, the CRLF working-tree file produces no git diff, so the bug was invisible. Adds three regression tests covering all three rewrite paths: - CRLF STATE.md with ## Session and Resume file absent - CRLF STATE.md with ## Session and Stopped at absent - CRLF STATE.md with ## Session Continuity (bootstrap shape) Each asserts the field IS on disk (the bug discriminator: the pre-fix command's JSON output looked identical to a successful write). * fix(#2450): CRLF-tolerant session-section rewrite + no-op-detection guard Two regexes in cmdStateRecordSession used literal \\n which cannot match CRLF STATE.md, silently no-op'ing the section rewrite while the CRLF- tolerant detector above entered the branch. The reporter's exact repro: on a CRLF STATE.md with one canonical session field absent, the command returned recorded:true + updated:['Resume File'] but the field was never written to disk. Three changes: 1. src/state.cts:1166 (canonical ## Session rewrite regex): \\n -> \\r?\\n 2. src/state.cts:1195 (## Session Continuity insert regex): \\n -> \\r?\\n 3. Defensive invariant (#2450 class fix per reporter's suggestion): track whether the chosen branch's replace actually matched via callback flag. Only set sessionCreated=true and push to updated when rewriteMatched. Unreachable post-fix, but fail-loud is the right posture for a silent- success gate. If a future drift between the detector and writer regexes reintroduces the asymmetry, the caller will not see false updated entries. Same canonical CRLF-tolerant form already in use at check-command-router.cts :205 (extractPlanDesignatedSections). Same bug class previously fixed in #1658, #1668, #2206, #2449. * fix(#2450): address review followups + add changeset Code-review + security-review both flagged the unreachable else at the Session Continuity branch (defaulted rewriteMatched=true in dead code, re-arming the bug class for future drift). Removed the else; the remaining code path leaves rewriteMatched=false if linesToInsert is empty, preserving the fail-loud posture. Added scope-limitation doc to the rewriteMatched gate: it covers the INSERT path only, not the earlier in-place stateReplaceField successes (which DID land on disk and correctly push to updated unconditionally). Tests: - Normalized STATE_CRLF_SESSION_MISSING_RESUME fixture to match the canonical 6-key frontmatter of STATE_WITH_SESSION (code-review I2). - Added mixed-ending test (LF frontmatter + CRLF body) to close CONTRIBUTING.md:490 'Mixed CRLF/LF newlines' requirement (I1). Added Fixed changeset (code-review H1). * docs(changeset): backfill PR number to 2482 |
||
|
|
909a3b180b |
fix(#2470): install pi's extension as gsd.js so pi actually discovers it (#2478)
* test(#2470): failing-first — pi extension must satisfy pi's auto-discovery filter pi auto-discovers extensions/ entries through isExtensionFile(), which accepts only .ts and .js. GSD installs its extension as gsd.cjs, so pi silently skips it: no /gsd command, no error, no log line. Encodes pi's discovery PREDICATE rather than a literal filename, so the contract keeps holding across future renames, and adds the migration-006 test matrix for retiring the stale gsd.cjs left in pre-fix installs. Red until the fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): install pi's extension as gsd.js so pi actually discovers it pi auto-discovers extensions/ entries via isExtensionFile(), which accepts only .ts and .js and skips everything else silently. capabilities/pi declared the dest as gsd.cjs, so the extension installed correctly and was then ignored forever: no /gsd command, no error, no log line. Install it as gsd.js. The in-repo source stays pi/gsd.cjs — tests require() it directly and .cjs is unambiguous CommonJS; only the installed name has to satisfy pi, and pi loads accepted files through jiti, which handles CJS and ESM alike. (The reporter's premise that ~/.pi/agent/package.json declares "type":"commonjs" does not hold — pi never writes that file.) Renaming an installed artifact requires a migration record, so add 006 to retire the stale gsd.cjs from pre-fix installs; without it the old path drops out of the manifest and uninstall can never remove it. The migration plans nothing for an unmanifested gsd.cjs: emitting remove-managed there would have the executor downgrade it to preserve-user and mark it blocked, failing the install for anyone who hand-placed their own file. Also pins body-parser >=2.3.0 (GHSA-v422-hmwv-36x6). The advisory reaches the production tree transitively via the Claude Agent SDK and fails the npm-integrity gate, blocking any PR; pinned via the existing overrides idiom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): address orthogonal review findings + register migration checksum Code review: - pi/gsd.cjs's install docstring still told readers to copy the file to extensions/gsd.cjs — the exact silently-broken state this PR fixes. Anyone following it recreated the bug. - Two stale extensions/gsd.cjs comments in install-minimal-hooks.test.cjs. Security review: - _installNativePluginIfDeclared confined nativePlugin.dir but joined nativePlugin.file onto the validated directory unchecked, so a descriptor whose file carried .., an absolute path, or a NUL byte would have written outside configHome. Not reachable in a shipped build (descriptors are first-party and compiled into the capability registry), but file is exactly the field this PR changes. Confine the full dest path instead; for a well-formed descriptor this resolves identically to the previous mkdir(dir) + join(dir, file). Covered by four new write-confinement tests. Also register migration 006 in the #670 EXPECTED_CHECKSUMS baseline — shipped migration bodies are locked to a committed checksum and a new migration fails CI until it is listed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): never dereference a symlinked managed path when snapshotting fs.copyFileSync follows symlinks, so a managed path replaced by a link had the REFERENT's bytes copied into the migration journal's rollback and backup trees — a gsd.cjs symlinked at a private key would land that key's contents under gsd-migration-journal/. Deletion was already safe (fs.rmSync unlinks the link, never the target); the copy was not. Nothing GSD installs is ever a symlink, so the faithful snapshot of a symlinked managed path is the link itself. copyPreservingSymlink recreates it, which keeps rollback fidelity (restore re-creates the same link) while never reading the referent. Scoped the pre-delete to the symlink branch only, so the regular-file path keeps copyFileSync's overwrite-in-place and a mid-restore failure cannot destroy the destination. The restore-side existence check moves to lstat, since existsSync follows a link whose target is gone and would silently skip the restore. This lives in the engine all six migrations share, so 000-005 are hardened too. Also regenerates the pi golden-parity hash: correcting pi/gsd.cjs's own install docstring changes the extension's content, which the golden suite caught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): symlink-preserve the in-apply failure-recovery restore too The previous commit routed three copy sites through copyPreservingSymlink but missed a fourth: the catch block inside applyInstallerMigrationPlan, which replays rollback snapshots taken earlier in the SAME apply attempt. Those snapshots are symlinks precisely because of that commit, so the raw copyFileSync there dereferenced them and wrote the referent's bytes to the LIVE install path — worse than the journal-tree leak it was meant to fix, since it is user-visible and at a predictable location. Verified by experiment rather than assertion: with the pre-fix line restored, the managed path comes back as a REGULAR FILE containing the referent's bytes; with the fix it comes back as a symlink and the bytes appear nowhere. The accompanying test injects the failure by letting the delete succeed and then throwing once, modelling a later step failing after the delete. That ordering is load-bearing — an earlier draft injected before the delete, which leaves the live path in place, so the pre-fix copyFileSync hit a same-file collision and threw instead of leaking. That draft passed against the bug it was written to catch; this one fails against it. Adds the missing rollback() coverage as well: a restored symlinked managed path must come back as a link pointing at its original target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2470): read the backup location from the journal, not the plan The new backup-content assertion read backupRelPath off result.plan.actions, where it is always null: the planner reserves the field and apply chooses the concrete location, recording it in the journal. The assertion therefore failed on "backup path must be recorded for the user" rather than on anything about the behavior it was written to check. Read it from the journal, which is the authoritative record. Verified by executing all four new test bodies in-process against the built engine — the backup file exists and holds the locally patched content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#2470): backfill changeset pr number to 2478 * chore(#2470): backfill changeset pr number to 2478 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
b6ebf9d4b4 |
fix(#2449): make tdd.review-checkpoint frontmatter regex CRLF-tolerant (#2477)
* test(#2449): failing-first CRLF regression for tdd.review-checkpoint cmdTddReviewCheckpoint's frontmatter regex /^---\\n...\\n---/ (line 751) cannot match CRLF PLAN.md, so a type:tdd plan with Windows line endings was silently classified as 'no type:tdd plans' — output indistinguishable from a phase that genuinely contains no TDD plans. The advisory gate then short-circuited to a confident pass with no violations table. Adds tddPlanCrlf() fixture helper (CRLF twin of tddPlan) and a [crlf] test case that asserts tddPlans===1 for a CRLF type:tdd plan (would be 0 before the fix), plus block:true/violations:1 since the fixture has no RED/GREEN commits. * fix(#2449): make tdd-review-checkpoint frontmatter regex CRLF-tolerant The frontmatter delimiter regex at src/check-command-router.cts:751 used literal \\n which cannot match a CRLF PLAN.md delimiter (---\\r\\n). frontmatterMatch was null, the plan was never classified type:tdd, tddPlanFiles stayed empty, and the advisory gate short-circuited to a confident pass with no violations table. The fix replaces /^---\\n([\\s\\S]*?)\\n---/ with /^---\\r?\\n([\\s\\S]*?)\\r?\\n---/ — the same CRLF-tolerant form already used elsewhere in the same file at line 205 (extractPlanDesignatedSections). Same canonical pattern; one-line change. Same bug class previously fixed in #1658, #1668, #2206; this instance is in a different file and is not a duplicate of any of them. * fix(#2444): re-resolve body-parser to 2.3.0 in lockfile (GHSA-v422-hmwv-36x6) GHSA-v422-hmwv-36x6 (body-parser DoS via invalid limit value, low severity, published 2026-07-20T23:23:26Z) made tests/npm-integrity-gate.test.cjs (#3588: root workspace production tree has no advisories) fail any subsequent npm audit --omit=dev. The advisory affects body-parser >=2.0.0 <2.3.0 pulled transitively via @anthropic-ai/claude-agent-sdk -> @modelcontextprotocol/sdk -> express -> body-parser@2.2.2. express@5.2.1 already declares body-parser as ^2.2.1, so 2.3.0 is a valid re-resolution within express's own compatibility range — no override needed. Regenerated the lockfile via 'npm audit fix --omit=dev' which re-resolves transitive deps within their declared ranges; package.json is unchanged. Verified: npm audit --omit=dev reports 0/0/0/0/0 advisories; body-parser now reads as 2.3.0 in 'npm ls body-parser --omit=dev'. * test(#2449): add mixed-endings (CRLF frontmatter + LF body) variant Per code-review Low finding: CONTRIBUTING.md:490 names 'Mixed CRLF/LF newlines' as a required adversarial fixture class. The pure-CRLF [crlf] test covers the reported bug (Windows editor + autocrlf=input). This [crlf-mixed] variant covers the more adversarial case where frontmatter delimiters are CRLF but the body is LF (editor that normalizes body text, or a toolchain concatenating CRLF + LF fragments). The classifier only reads frontmatter, so detection is unaffected — but the test locks the behavior. * docs(changeset): add Fixed fragment for #2449 PR * docs(changeset): backfill PR number to 2477 |
||
|
|
46ba9ed464 |
fix(#2444): branch plan-structure validation on task type=checkpoint:* (#2473)
* test(#2444): failing-first regression for checkpoint:* plan-structure validation Add acceptance-criteria tests covering the three canonical checkpoint task types (human-verify, decision, human-action) plus an unknown-subtype forward-compat case. Each canonical type must pass verify plan-structure when it carries its type-specific required fields (per gsd-core/references/checkpoints.md), and must be flagged when those fields are missing. Non-checkpoint tasks keep the existing <action>/<verify>/<done>/<files> requirements unchanged (AC3 regression guards). The existing 'errors when checkpoint task but autonomous is true' fixture is updated to use the canonical checkpoint:human-verify triple (<what-built>/<how-to-verify>/<resume-signal>) so it does not collide with the new per-type validator; the assertion (autonomous is not false) is unchanged. * fix(#2444): branch plan-structure validation on task type=checkpoint:* cmdVerifyPlanStructure unconditionally required <action>/<verify>/<done>/ <files> on every task, so every checkpoint:* task — which uses the checkpoint convention's type-specific fields instead — was reported as a structural error. Checkpoint-heavy phases produced walls of false findings. The fix introduces two pure helpers in verify.cts: - extractPlanTaskInfos(content): single ReDoS-safe pass over <task ...>...</task> blocks that captures BOTH the opening-tag attribute string (so the type= selector is not lost, as it is with extractTaggedBlocks) and the body, returning a typed PlanTaskInfo. - validatePlanTaskStructure(task): branches on the task's type. checkpoint:human-verify requires <what-built>/<how-to-verify>/ <resume-signal> (the canonical triple). checkpoint:decision requires <decision>/<options>/<resume-signal>. checkpoint:human-action requires <action>/<instructions>/ <verification>/<resume-signal>. Unknown checkpoint:* subtypes require only the universal <resume-signal> (forward-compat). All other types keep the historical <action>/<verify>/<done>/<files> requirements unchanged. Canonical reference: gsd-core/references/checkpoints.md. Per-type field sets validated against the documented templates in agents/gsd-planner.md and gsd-core/templates/phase-prompt.md. * fix(#2444): re-resolve body-parser to 2.3.0 in lockfile (GHSA-v422-hmwv-36x6) GHSA-v422-hmwv-36x6 (body-parser DoS via invalid limit value, low severity, published 2026-07-20T23:23:26Z) made tests/npm-integrity-gate.test.cjs (#3588: root workspace production tree has no advisories) fail any subsequent npm audit --omit=dev. The advisory affects body-parser >=2.0.0 <2.3.0 pulled transitively via @anthropic-ai/claude-agent-sdk -> @modelcontextprotocol/sdk -> express -> body-parser@2.2.2. express@5.2.1 already declares body-parser as ^2.2.1, so 2.3.0 is a valid re-resolution within express's own compatibility range — no override needed. Regenerated the lockfile via 'npm audit fix --omit=dev' which re-resolves transitive deps within their declared ranges; package.json is unchanged. Verified: npm audit --omit=dev reports 0/0/0/0/0 advisories; body-parser now reads as 2.3.0 in 'npm ls body-parser --omit=dev'. * test(#2444): close review gap-closure tests + harden type-attr charset Orthogonal review (code-review + security-review subagents) returned APPROVE on Standards and Spec. Per the playbook's zero-tolerance policy, address every Low finding: Spec gap-closures: - AC3 verbatim: add explicit <done> and <files> regression tests for non-checkpoint tasks (pre-existing tests only covered <action> and <verify>). - AC2: add checkpoint:decision missing <decision>, checkpoint:human-action missing <action>, checkpoint:human-action missing <verification> cases (the implementation enforces all of these; only one missing-field case per type was previously tested). - Remove the duplicate 'returns error for nonexistent file' test that leaked into the new describe block from the insertion edit. Security hardening (Low-sev, defense-in-depth): - Tighten the task type= attribute extractor in src/verify.cts from [^"'>\s]+ to [\w:-]+ so a hostile type= attribute cannot carry markup fragments (e.g. type=evil<fragment) into the verifier's typed JSON output. All legitimate type values (auto, tracer, manual, checkpoint:human-verify, checkpoint:decision, checkpoint:human-action, checkpoint:tdd-review) match the tighter charset. - Add adversarial regression test asserting type=evil<fragment surfaces as 'evil' (capture stops at '<'), with no markup chars (< > ( ) &) in the surfaced type field. * docs(changeset): add Fixed fragments for #2444 PR Two fragments: - sturdy-jays-tumble.md: the verify plan-structure checkpoint fix - witty-badgers-hum.md: the body-parser 2.3.0 re-resolution PR number backfilled to 0 placeholder per CLAUDE.md 'PR Number Handling'; will backfill to the real PR number immediately after gh pr create returns. * docs(changeset): backfill PR number to 2473 Per CLAUDE.md 'PR Number Handling': backfill the placeholder pr:0 with the real PR number returned by gh pr create. |
||
|
|
a54feb4216 |
fix(#2440): per-counter progress ratchet — total_plans always takes derived value (#2468)
* fix(#2440): per-counter progress ratchet — total_plans always takes derived value Two sites fixed (targeted — existing body-only write tests preserved): Site A — read path: shouldPreserveExistingProgress (state-document.cts:167) removed total_plans from the all-or-nothing ratchet check. It now joins total_phases as an always-derived counter. Only completed_phases and completed_plans keep ratchet behaviour (they are monotonic). This fixes gsd-tools query state.json reporting stale total_plans when a curated completed_plans triggers the ratchet. Site B — write path: applyStatePreservation (state-transition.cts:162) gained a deriveProgressKeys opt-in flag. When true (passed by cmdStatePlannedPhase only), total_plans and total_phases take the derived (post-sync) value instead of the wholesale curated restore. When false (the default — state.update, state.patch), the existing #3242 wholesale protection stays fully in force. This fixes the state planned-phase verb writing a stale total_plans. The opt-in approach preserves all 8 existing #3242/#1264/#500 body-only write tests that assert wholesale progress preservation during non- progress updates. Tests: - tests/state.test.cjs: 4 unit tests for shouldPreserveExistingProgress (total_plans upward/downward/equality + completed_plans ratchet active). - tests/state-transition.test.cjs: 2 #2440 regression tests for deriveProgressKeys=true (total_plans takes derived; boundary at equality). The existing !resync wholesale-restore test stays unchanged (default behavior preserved). References: #2440; #1446 (total_phases read-path fix — same principle); #3242 Bug A (body-only preservation — protection preserved via the opt-in gate); ADR-1769 (applyStatePreservation table-driven preservation). * chore(#2440): backfill pr:2468 in .changeset/mellow-eagles-chatter.md |
||
|
|
bb97ffb5aa |
fix(#2431): self-suppress TDD Audit section when all commits are missing (#2467)
* fix(#2431): self-suppress TDD Audit section when all commits are missing The TDD Audit section in ship.md step 8 was always emitted — but the execute pipeline only writes gate_status: git trailers when TDD mode is active. Without TDD mode (the default), every commit's trailer is absent and the section normalizes to 100% missing, producing a noise table with no way to disable it. Fix: add a self-suppress instruction at the point where gate_status values are normalized. When every commit in the scan normalizes to 'missing', skip both step 8 (TDD Audit section) and step 9 (aggregate gate_status trailer) entirely. Only emit when at least one commit carries a real value (skill, fallback, or exempt). This is data-driven, NOT config-gated. An earlier iteration used inline 'gsd_run query config-get workflow.tdd_mode' — but workflow.tdd_mode is owned by the tdd capability, and ADR-857 Phase 6 forbids host loop workflows from reading capability-owned keys via inline config-get. The self-suppress approach avoids any config-get entirely; it checks the actual trailer data and skips when there's nothing real to report. Matches the triage's suggested approach: 'have the audit gracefully degrade (skip the section)' when there is no real signal. Tests: tests/workflow-compat.test.cjs gains 3 #2431 assertions: - documents self-suppress when every commit is missing - step 9 (aggregate trailer) is also gated on real values existing - does NOT read workflow.tdd_mode inline (ADR-857 Phase 6 compliant) The existing feat-41 assertions still pass — the section content is preserved, only gated by the self-suppress instruction. References: #2431; PR #585 (consumer shipped, producer never wired); ADR-857 Phase 6 (capability-owned config keys must not be read inline by host loop workflows). * chore(#2431): backfill pr:2467 in .changeset/clever-moles-frolic.md |
||
|
|
6140627f5c |
fix(#2427): ground smart-entry completion in ROADMAP-derived counts + tighten status regex (#2466)
* fix(#2427): ground smart-entry completion in ROADMAP-derived counts + tighten status regex Two coupled defects in isComplete (src/smart-entry.cts): 1. Two-scale comparison: isComplete compared global current_phase (from STATE.md body 'Phase: N') against milestone-scoped total_phases (from STATE.md frontmatter progress.total_phases, written once at milestone- switch time and going stale as soon as new phases are appended to the roadmap). When current_phase >= stale total_phases (e.g. 7 >= 4), isComplete tripped true even though later phases were still unchecked in ROADMAP.md. 2. Over-broad status regex: /\bcomplete(d)?|done|shipped\b/i matched any 'shipped' or 'done' substring — including per-phase status like 'Phase X shipped — PR #N' — and falsely satisfied the status side of the completion check. Fix: - Added two new SmartEntrySignals fields: roadmap_total_phases and roadmap_completed_phases, populated by calling the existing deriveProgressFromRoadmap helper (from phase-lifecycle.cts:60) when ROADMAP.md exists. These are global, authoritative counts from the Progress table — never stale. - isComplete now prefers the roadmap-derived counts when available (completed >= total) and falls back to the legacy STATE.md comparison only when the roadmap has no parseable Progress table (backward compat for fresh or non-standard projects). - Tightened the status regex to /\b(milestone\s+complete|all\s+phases\s+complete|complete(d)?)\b/i. Drops 'done' and 'shipped' (per-phase language). Keeps milestone-level signals per ADR-2207 (milestone complete, all phases complete) plus the legacy short form 'complete'/'completed'. Tests (tests/smart-entry.unit.test.cjs gains a #2427 describe block): - Mid-milestone with stale total_phases=4, current_phase=7, per-phase 'shipped' status, and 3 unchecked roadmap phases → NOT complete (the core bug scenario). - All roadmap phases complete classifies as complete even with stale cached total_phases (roadmap wins). - Per-phase 'shipped' or 'done' status alone does NOT satisfy completion when roadmap phases are unchecked. - Legacy fallback: empty roadmap (no Progress table) still classifies via STATE.md comparison (backward compat). The makeProject test helper now accepts a string for the 'roadmap' parameter (written verbatim) in addition to the boolean shorthand, so tests can supply a real Progress table. References: #2427; ADR-2207 (milestone status lifecycle); ADR-2143 (column-name-driven Progress table parsing via deriveProgressFromRoadmap); triage note that this is a read-side fix only (the milestone-switch write path in state.cjs is out of scope). * chore(#2427): backfill pr:2466 in .changeset/curious-rams-run.md |
||
|
|
903182fed3 |
fix(#2414): mempalace-capture rooms example must be dicts with name key (#2464)
* fix(#2414): mempalace-capture rooms example must be dicts with name key
The skill's Step 3 'Add the drawer (verbatim)' example wrote a flat list
of bare strings under rooms: in the embedded mempalace.yaml. mempalace's
miner (detect_room + _mine_impl) indexes room["name"] — a bare-string
list crashes the first 'mempalace mine' invocation with
TypeError: string indices must be integers, not 'str'
Following the documented example verbatim and running the capture crashed
every time, before any file was routed. The bug shipped in #2220's fix
(commit
|
||
|
|
be5113abfe |
fix(#2408): fold colliding phase statuses + add W023 collision warning (#2461)
* fix(#2408): fold colliding phase statuses + add W023 collision warning Two coupled bugs from #2408: 1. cmdStats last-write-wins status (src/commands.cts:1610-1618): when two on-disk phase directories normalize to the same phase key (e.g. `05-real/` and `05-real-stray/`), the directory-scan merge overwrote `status` with whatever the *current* directory in scan order computed, discarding `existing?.status` entirely. fs.readdirSync order is not stable across platforms, so /gsd-stats could silently report `Not Started` for a phase that is actually `Complete`. Plan/summary counts were already additively merged; only `status` was wrong. Fix: introduced a `foldPhaseStatus(a, b)` helper that returns whichever status is further along the precedence ladder `Complete > Needs Review > Executed > In Progress > Planned > Not Started` (with `Pending` and unrecognized statuses ranked after). The merge site now calls `existing ? foldPhaseStatus(existing.status, status) : status`. The fold is commutative, so the result is identical regardless of read order. 2. cmdValidateHealth had no collision-detection pass (src/verify.cts): codes W001-W022 cover every condition except normalized-key collisions, so an operator got zero signal that anything was wrong. Fix: added W023 — groups phaseDirEntries by their normalized phase key (via the existing normalizePhaseName + extractPhaseToken helpers from phase-id.cjs — same normalization cmdStats uses) and emits a warning for any group with ≥2 dirs. The warning names the normalized key, both directory names (sorted by comparePhaseNum for stable output), and each directory's independently-computed status (via determinePhaseStatus imported from commands.cjs). Wording is deliberately neutral — never guesses which directory is the real one. The optional --repair path from the issue is intentionally NOT implemented in this PR (the issue marked it lower priority and acceptance criterion 4 is vacuously satisfied by omission). Triage correction applied: the issue proposed W022, but that code is already in use for config.json model-tier validation (src/verify.cts :1372-1391). The next free code is W023, used here. Tests: - tests/commands.test.cjs: integration test that 05-real/ (Complete) + 05-real-stray/ (empty/Not Started) collide and stats reports the merged phase as Complete regardless of read order; plus a direct unit test of foldPhaseStatus asserting commutativity + correct precedence for every status pair + correct handling of unrecognized statuses. - tests/health-validation.test.cjs: integration test that W023 fires on the collision naming both dirs + their statuses (and uses neutral wording), plus a negative test that no W023 fires when only one dir exists per key. References: #2408; reporter's three-layer triage + acceptance criteria; triage correction that W022 is already in use (model-tier validation). * chore(#2408): backfill pr:2461 in .changeset/graceful-koalas-forage.md |
||
|
|
455ad49ae3 |
feat(#2296): config-gated provider escalation on quota-exceeded (#2458)
* test(#2296): failing-first coverage for provider escalation on quota-exceeded
Covers the provider-escalation ladder layered onto EXEC.CLASSIFY: back-compat
(no escalation block without --failure-class), cap boundaries at
min(max_escalations, list length) at limit-1/limit/limit+1, opt-in gating,
malformed/hostile provider_escalation config, the --failure-class CLI negative
matrix, config-key registration, and a fast-check budget-limit property.
Red until the resolver, CLI flag, and manifest key land.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(#2296): config-gated provider escalation on quota-exceeded
The dynamic_routing tier ladder escalates within one provider, which does not
help when that provider is what ran out of quota. Add an opt-in provider ladder
layered on the existing EXEC.CLASSIFY seam.
- model-resolver: resolveProviderEscalation walks dynamic_routing.provider_escalation
capped at min(max_escalations, list length), reporting from/to/attempted/exhausted.
Invalid entries are dropped (ADR 227 shape validation). Stays a leaf module —
the quota-class policy decision is the caller's, per the CONTEXT.md contract.
- agent-command-router: export a frozen AGENT_FAILURE_CLASSES so the new CLI
validator cannot drift from the classifier that produces the values.
- resolve-execution: --failure-class flag; emits an escalation block ONLY when
passed, so the existing JSON contract is byte-identical for every caller.
- config-schema.manifest: register dynamic_routing.provider_escalation.
- execute-phase step 7.1: auto-escalate, honor Retry-After, fail loudly naming
every model tried once the ladder is spent.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(#2296): extract quota recovery to a reference fragment; regen goldens
The step 7.1a addition pushed gsd-core/workflows/execute-phase.md from 93390 to
95111 LF bytes, past the frozen ADR-857 Phase 6 ceiling (hard <93600, margin
<=93400) asserted by tests/fix-2285-claude-orchestration-wiring.test.cjs. The
base sat 10 bytes under the margin, so no inline wording would have fit.
That gate's own rationale is that optional-feature detail belongs in a fragment,
not the host loop. Moved BOTH the new provider-escalation branch and the
pre-existing manual recovery prompt into
gsd-core/references/execute-phase-quota-recovery.md, leaving step 7.1 as a
one-line pointer. execute-phase.md is now 92880 bytes — 510 SMALLER than base.
Also regenerates the fixtures that legitimately moved because three shipped
files changed (gsd-tools.cjs, config-schema.manifest.json, execute-phase.md):
golden-install-parity + install-tree for all 16 runtimes, INVENTORY.md +
INVENTORY-MANIFEST.json for the new reference, and the workflow size baseline.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(#2351): make the C1 orphan-reaping test load-independent
tests/run-with-timeout.test.cjs C1 asserted the child heartbeat file exists
after a 1s group-kill window, but the child only wrote it on the first 100ms
setInterval tick. Nothing synchronized the two: on a loaded container the group
is SIGKILLed before that tick lands, the file never appears, and the assertion
fails for a reason unrelated to reaping. Observed failing on both linux-node22
and linux-node24.
The behavior actually under test is the FREEZE assertion (heartbeat stops
advancing => descendant was reaped, not orphaned). That is unaffected by
sampling once more at t=0.
Child now writes its first heartbeat synchronously at startup before arming the
interval, and the kill window widens 1s -> 3s to cover child boot under load.
Both remove the timing dependency; neither weakens what the test proves.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore(#2296): backfill pr:2458 in .changeset/rapid-jays-bark.md
* chore(#2296): regenerate fixtures after rebase onto #2402
The rebase conflicted on the generated golden-install-parity fixtures and
workflow-size-baseline.json because #2402 (
|
||
|
|
b6e6a22fce |
fix(#2402): honor response_language across orchestrator output + UAT checkpoint renderer (#2457)
* fix(#2402): honor response_language across orchestrator output + UAT checkpoint renderer Replays the in-flight bot branch fix/2402-response-language-orchestrator-coverage (seven commits, never pushed) onto current origin/next as a single squashed commit. The original work was substantial and correct; this commit preserves its full scope, trimmed where rebase conflicts + workflow size budgets required it. Three independent layers where response_language was being dropped are closed: Layer 1 — orchestrator-facing directives across workflows. Adds the strong "All user-facing output in this workflow MUST be presented in {response_language}; technical terms, code, paths, and subagent prompts stay in English" directive to ~40 workflows that previously either lacked it entirely (verify-work, new-project, new-milestone, quick, manager, and ~35 more) or carried only the weak subagent-prompt-only form (plan-phase, execute-phase). The directive covers narration between tool calls and banner output, not just the AskUserQuestion prompts. Layer 2 — UAT checkpoint renderer (src/uat.cts). buildCheckpoint now accepts an optional responseLanguage parameter and renders the frame strings ("CHECKPOINT: Verification Required", "Type `pass` or describe what's wrong.") in any of 9 languages (English/Spanish/French/German/Portuguese/Japanese/ Chinese/Korean/Italian) with an alias table covering ~30 input variants (en, es, español, ja, 日本語, etc.). cmdRenderCheckpoint reads config.response_language via loadConfig(cwd) and passes it through, so the byte-for-byte block verify-work.md reprints verbatim is already localized when written — preserving the anti-injection hygiene rule at verify-work.md (the model is forbidden to translate after the fact). CJK display width is computed by East Asian Width property ranges (W/F) so the right ║ border of the banner stays aligned for full-width characters. English fallback is byte-identical to the pre-fix behavior when response_language is unset or unrecognized. Layer 3 — literal English report templates in execute-phase. The top-of- workflow directive covers all template sites (templates are a structural source, not literal output). Inline render-language notes that previously sat at each template site were removed during the squash because they pushed execute-phase.md over its frozen pre-phase-6 byte ceiling (93600 — ADR-857 Phase 6 capstone). The single top directive covers the same surface with fewer bytes. Also extends src/docs.cts and src/init.cts to propagate response_language into the init JSON bundle of the additional workflows so the directive can read it. Tests added: - tests/uat.test.cjs: buildCheckpoint with unset/unrecognized language falls back to English default; recognized language swaps only the two frame strings while structural lines stay untouched; CJK display-width regression (independent recomputation of East Asian Width W/F ranges). - tests/workspace.test.cjs, tests/docs-update.test.cjs: response_language wiring through docs.cts/init.cts. References: #2402; reporter's three-layer triage + Layer-4 follow-up; the byte-for-byte anti-injection hygiene rule at verify-work.md (the reason Layer 2 must be renderer-side, not model-translated). This is a squash of the in-flight bot branch — seven commits representing the original implementation plus its subsequent fix/CJK-padding/test/ changeset/regen cycles, none of which were ever pushed or PR'd. The squash captures the final coherent state. * chore(#2402): backfill pr:2457 in .changeset/2402-response-language-orchestrator-coverage.md * chore(#2402): regen golden + size baseline after rebase against #2315 (PR #2451) Rebase conflicts were entirely in generated artifacts (golden-install-parity fixtures + workflow-size-baseline.json). After taking theirs during rebase, regenerated cleanly against the merged source tree. |
||
|
|
352876ff0c |
fix(#2315): respect review.default_reviewers in bare convergence invocation (#2451)
* test(#2315): regression test for review.default_reviewers precedence A bare /gsd-plan-review-convergence invocation (no reviewer flags) is supposed to let users configure a persistent reviewer lineup via review.default_reviewers and just run the loop. Instead, the orchestrator's argument parser silently discards that configuration and forces --codex on every no-flag invocation — with no warning that the configured reviewers were ignored. This commit adds a regression test that fails against the pre-fix workflow (the buggy unconditional --codex fallback is still present at this commit) and passes after the fix lands: - Structural: the workflow must NOT contain an unconditional 'if [ -z "$REVIEWER_FLAGS" ]; then REVIEWER_FLAGS="--codex"; fi' line before the workflow.plan_review_convergence config gate. - Structural: the workflow must query review.default_reviewers AFTER the config gate and document that empty REVIEWER_FLAGS lets gsd-review apply the default. - Behavioral: matrix across {configured, unset, empty-array, explicit-flag} invoking the actual deployed parse + resolution blocks with a stubbed gsd_run. Also updates two existing tests whose assertions the fix makes stale: - #2293 behavioral: endMarker was the buggy unconditional fallback line; the bare invocation assertion was 'run("5") === "--codex"'. Both flip post-fix (endMarker is now the last --all grep line; bare invocation returns empty from the parse block, default applied later in step 1.5). - command-default-claim: the pre-fix command documented '--codex (default if no reviewer specified)' which was the user-facing mirror of the bug. The assertion now requires the command to document the review.default_reviewers precedence. * fix(#2315): respect review.default_reviewers in bare convergence invocation Root cause: plan-review-convergence.md step 1 (Parse and Normalize Arguments) contained an unconditional fallback that set REVIEWER_FLAGS=\"--codex\" whenever no explicit reviewer flag was supplied. This value was then interpolated verbatim into the gsd-review args, so gsd-review saw --codex as an explicit flag (precedence rule 1) and never reached rule 3 (review.default_reviewers). The same path silently dropped any configured review.reviewer_instances (instances participate ONLY via review.default_reviewers per ADR-1517). Fix: - Remove the unconditional --codex fallback from step 1. - Add a config-gated resolution in step 1.5 (after CONVERGENCE_ENABLED check) that queries review.default_reviewers and either leaves REVIEWER_FLAGS empty (letting gsd-review apply its own rule-3 default) or falls back to --codex when no default is configured — preserving the pre-fix default for unconfigured users (AC3). - Replace the banner {REVIEWER_FLAGS} token with {REVIEWER_DISPLAY} so the startup banner reflects what will actually run (AC4), not a hardcoded value. The fix upholds the documented precedence contract (ADR-0011, ADR-0015) that the bug was actively violating. Explicit-flag invocations (--gemini, --all, etc.) are unaffected (AC5). References: #2315; ADR-0011 (review.default_reviewers precedence); ADR-0015 (autonomous cross-AI convergence); ADR-1517 (reviewer instances). * chore(#2315): bump plan-review-convergence.md baseline + changeset - Bump plan-review-convergence.md size baseline 23713 → 25536 (the new step-1.5 default-resolution block). - Add .changeset/plucky-yaks-roar.md documenting the user-visible change. * fix(#2315): restore /gsd: colon syntax + skip behavioral test when jq missing Two follow-ups to the #2315 fix discovered by gsd-test: 1. The fix commit accidentally regressed the slash-command namespace in the disabled-feature exit message: /gsd:plan-review-convergence (correct, from PR #3452) became /gsd-plan-review-convergence (retired dash syntax). The slash-command-namespace invariant test caught this. Restored the colon form. 2. The behavioral test exercises the deployed reviewer-resolution block, which pipes through jq. jq is a documented production dependency (review.md:244 "install jq if missing") and is present in every production deployment, but is NOT on PATH in the gsd-test linux-node{22,24} containers (same constraint as tests/opencode-review-reconstruction. property.test.cjs). Without jq, the printf|jq pipeline fails silently, the ||echo 0 fallback yields DEFAULT_REVIEWERS_COUNT=0, and the resolution falls through to the --codex branch — producing a false negative. Added a jqAvailable guard at module load (matching the existing pattern) and skip the behavioral test when jq is absent. The structural tests (no bash execution) still run and validate the fix. * chore(#2315): regenerate golden-install-parity fixtures Source changes to plan-review-convergence.md (workflow + skill mirror + command doc) changed the install-tree hashes. Regenerated via 'npm run gen:golden' after rebuilding gsd-core/bin/lib/install-engine.cjs ('npm run build:lib') — the on-disk lib was stale relative to src/install-engine.cts (isSymlinkedDestOptIn) and blocked fixture gen. * test(#2315): address review findings — strengthen structural tests + property test Code-review + security-review (isolated subagent passes) surfaced Low/Nit findings; this commit addresses the test-side findings: - Structural test 1 ("unconditional --codex one-liner") now asserts the buggy line is absent EVERYWHERE, not just before the config gate. The earlier assertion allowed a maintainer to re-add the line after the gate (passing the structural test) while the bug would still bite at runtime before the gate runs. - Structural test 4 ("banner uses REVIEWER_DISPLAY") now asserts the LITERAL banner placeholder "Reviewers: {REVIEWER_DISPLAY}" and forbids "Reviewers: {REVIEWER_FLAGS}". The earlier workflow.includes( "REVIEWER_DISPLAY") was satisfied by a comment mention. - New structural test for command/skill content parity: both files must document the review.default_reviewers precedence on the --codex flag (catches a manual edit to one that the gen:plugin-skills mirror misses). - Behavioral test stub now passes default_reviewers via env var ($GSD_TEST_DEFAULT_REVIEWERS) instead of inline-interpolating into a bash single-quoted string. Removes the (currently-safe) fragility where a future test input containing a single quote would close the bash quote and execute as bash under execFileSync. - New property test (fast-check, numRuns=25) for the JSON-classification contract: non-empty arrays of slugs -> empty REVIEWER_FLAGS; empty array / scalar JSON / malformed JSON -> --codex fallback. Locks the parser contract per CLAUDE.md mandate. * fix(#2315): address review findings — defensive jq-missing warning + banner cleanup Code-review surfaced two Low-severity workflow-side findings: - Defensive jq-missing warning: if jq is not on PATH in production (it is a documented dependency per review.md:244, but the dependency can be absent in degraded environments), the printf|jq pipeline fails silently to "0" and a user with review.default_reviewers configured gets --codex with no indication their configured default was unreadable. Added a command -v jq guard at the top of the resolution that falls back to --codex AND emits a stderr warning explaining the reason. This makes the failure diagnosable instead of silently reproducing the #2315 override. - Banner leading-space cleanup: REVIEWER_FLAGS accumulates with a leading space ("$REVIEWER_FLAGS --gemini" from ""), so the explicit-flag branch of REVIEWER_DISPLAY="$REVIEWER_FLAGS" rendered "Reviewers: --gemini" (double space). Pre-existing but worth fixing alongside the AC4 banner work. Strip one leading space with the ${VAR# } parameter expansion in the explicit-flag branch only (the configured-default and --codex branches already produce clean strings). * chore(#2315): bump plan-review-convergence.md baseline + regen golden fixtures The defensive jq-missing warning grew plan-review-convergence.md (25536 -> 26285 bytes). Bumps the per-file baseline snapshot and regenerates the golden-install-parity fixtures for the resulting install-tree hash changes. * chore(#2315): backfill pr:2451 in .changeset/plucky-yaks-roar.md |
||
|
|
eb45fc0e8c |
fix(#2415): close_phase_todos stages the pending/ deletion alongside completed/ (#2447)
* fix(#2415): close_phase_todos stages the pending/ deletion alongside completed/ Bug: the workflow step moved resolved todos from .planning/todos/pending/ to .planning/todos/completed/ with a plain 'mv', then committed listing ONLY the destination directory in --files: mv "$TODO_FILE" "$COMPLETED_DIR/" gsd_run query commit '...' --files .planning/todos/completed/ .planning/STATE.md Git's index still tracked the moved file at its old pending/<name>.md path. The commit therefore only staged the new completed/<name>.md copy — the deletion at pending/ was never staged, never committed, and lingered as an unstaged deletion in git status indefinitely until some later broad 'git add -A' caught it. The phase genuinely closed the todo, but the working tree was never clean. Fix: add .planning/todos/pending/ to the --files list. 'git add' of that directory (since git 2.0) stages deletions of tracked files in the pathspec, so the moved-away file is staged as a deletion atomically with the new completed/ copy in the same commit. Chose plain mv + two-dir --files over 'git mv' because git mv FAILS on: - untracked todos (new todo file not yet committed) - non-git .planning dirs (worktree safety / pre-init projects) Plain mv has neither failure mode. Regression tests in tests/close-phase-todos-stage-deletion.test.cjs (source-text-is-the-product: workflow .md text IS what the runtime loads) cover: - the commit --files list includes BOTH completed/ AND pending/ - the move uses plain 'mv' (not 'git mv') so untracked + non-git cases work * chore(#2415): trim commit subject to keep execute-phase.md under byte ceiling The fix added '.planning/todos/pending/' (~26 bytes) to the commit --files list. To stay under the ADR-857 Phase 6 pre-phase-6 byte ceiling margin (93400 bytes, hard ceiling 93600), shortened the commit subject from 'auto-close N todo(s) resolved by this phase' to 'close N resolved todo(s)'. Net change vs origin/next: +6 bytes (93384 → 93390), well under the margin. Also regenerates the golden-install-parity fixtures (execute-phase.md content-hash update across all runtimes). * chore(#2415): bump execute-phase.md workflow-size baseline (93384 → 93390) The +pending/ fix added 6 net bytes (93384 → 93390), still well under the ADR-857 Phase 6 pre-phase-6 byte ceiling margin (93400). * chore(changeset): backfill pr:2447 in .changeset/sturdy-wasps-run.md * fix(#2415): add issue ref to allow-test-rule annotation (ADR-456) CI lint-allow-test-rule-refs failed on the prior commit — ADR-456 requires '// allow-test-rule: <category> see #NNNN' so every exemption is traceable to an issue. Added 'see #2415' to the source-text-is-the-product annotation. |
||
|
|
6c00cdad07 |
docs(#2343): list gsd-omp EoS integration (#2448)
* docs: list gsd-omp EoS integration * chore: add gsd-omp registry changeset * chore: update registry changeset PR reference --------- Co-authored-by: AI Assistant <ai@example.com> |
||
|
|
e227e81f59 |
fix(#2395): persist runtime identity into ~/.gsd/defaults.json for non-Claude installs (#2446)
* fix(#2395): persist runtime identity into ~/.gsd/defaults.json for non-Claude installs Bug: finishInstall for non-Claude runtimes never persisted a 'runtime' key into ~/.gsd/defaults.json. resolveRuntime() precedence is GSD_RUNTIME env > config.runtime > 'claude', so both inputs being empty on a Cursor (or any non-Claude) install caused agent_runtime and every runtime-branded slash hint to silently fall through to 'claude'. Cursor users saw 'agent_runtime: "claude"' and Claude-formatted /gsd-* hints with no env or config hand-set. Fix mirrors the existing resolve_model_ids: 'omit' write site at the same call site (bin/install.js finishInstall, gated on !_hostBehaviors(runtime) .nativeModelAliases && !GSD_TEST_MODE). Writes runtime: <runtime> into ~/.gsd/defaults.json when absent/null/empty. Claude is the resolveRuntime() fallback so it needs no write; an explicit pre-existing runtime value is always preserved across installs of any runtime. Pattern parity with #1156 (default-to-omit intent) and #1569 (preserve explicit user value) — the new write is the third sibling on the same install-time persistence block. Regression tests in tests/install.test.cjs cover: - absent / null / empty-string runtime → populated to <install runtime> - explicit pre-existing runtime preserved (no clobber across runtimes) - parameterized across 5 non-Claude runtimes (cursor, codex, opencode, antigravity, windsurf) Out of scope (per triage): the resolveRuntime() precedence order itself (env > config > default) is unchanged. A separate follow-up noted in the issue (subagent rendering when runtime correctly identifies as 'cursor') is unrelated to branding and not addressed here. * chore(#2395): regenerate golden-install-parity fixtures for new runtime persistence The golden install-tree fixtures capture the post-install state, including ~/.gsd/defaults.json. For non-Claude runtimes, defaults.json now includes runtime: <runtime> — content hash updated for each affected runtime (17 golden-install-parity/*.json files). Claude's defaults.json is unchanged (no runtime key written for Claude — it's the resolveRuntime() fallback). * chore(#2395): drop product names from changeset fragment (product-name-purity gate) * test(#2395): move describe out of fix-1521 fold + add same-runtime idempotence test Code review (correctness subagent) flagged 2 Low test-quality issues: 1. The new Bug #2395 describe was inserted inside the folded:fix-1521-real-install-stamping IIFE callback, muddying test reporting and tracing the regression to the wrong epic. Moved it outside the IIFE close to be a top-level sibling. 2. No explicit same-runtime idempotence test — the suite covered cross-runtime preservation (cursor seed → opencode install) but not 'install cursor twice → second is a no-op'. Added: seeds fresh defaults, installs cursor, captures mtime, installs cursor again, asserts runtime unchanged AND defaults.json mtime unchanged (idempotent, no rewrite churn). * chore(changeset): backfill pr:2446 in .changeset/fierce-ravens-dance.md |
||
|
|
12e4d93b19 |
fix(#2393): add GSD_ALLOW_SYMLINKED_DEST opt-in for intentional user-owned symlink layouts (#2445)
* fix(#2393): add GSD_ALLOW_SYMLINKED_DEST opt-in for intentional user-owned symlink layouts Bug: v1.7.0's destSubpath write-confinement (ADR-1239 Phase B) refused install/update whenever CLAUDE_CONFIG_DIR (or an artifact-kind child like skills/, hooks/) was a pre-existing symlink, with no opt-out. Three legitimate user-owned layouts were blocked: - (lars-hh) CLAUDE_CONFIG_DIR=~/.claude-personal with skills/hooks symlinked to a user-owned external dir - (Mamiki) ~/.claude/skills is a Windows Junction to a shared skills dir - (Azd325) ~/.claude itself is a symlink to a dotfiles repo (the early root-is-symlink return refused before the component loop ran) Fix: add GSD_ALLOW_SYMLINKED_DEST env var (accepts '1' or 'true'). When set, hasExistingSymlinkBetween follows symlinks instead of refusing them. Cross-platform: fs.lstatSync().isSymbolicLink() returns true for both POSIX symlinks and NTFS junctions (Node ≥ 16), so Mamiki's Junction case is handled by the same code path. Threat model preserved (these still refuse EVEN WITH opt-in): (a) path-traversal in the destSubpath string itself ('../../etc'-style) — ADR-1239 Phase B threat (a), untrusted destSubpath protection (b) a symlink whose resolved real path equals the install root itself — would let _removeGsdEntries wipe the root; #1704 threat (b) (c) broken symlinks (realpathSync throws) — fail-closed What opt-in RELAXES specifically: the 'pre-existing symlink pointing outside configHome' refusal — #1704 threat (c). The user has explicitly asserted they own and trust the symlink target. Error messages at all 4 call sites (installRuntimeArtifacts, _copyStaged, migrateLegacyDevPreferencesToSkill, installOpencodeFamilySkills) updated to (1) name the env var opt-in, (2) be accurate when the root itself is a symlink (Azd325's complaint that the old message accused destDir of 'containing' a symlink when the root was the actual symlink). Docs: docs/CONFIGURATION.md Environment Variables table updated. Regression tests in tests/install-write-confinement.test.cjs cover: - child-symlink layout (lars-hh / Mamiki): default refuses, opt-in allows - root-is-symlink layout (Azd325): default refuses, opt-in follows - path-traversal '../../etc' refused EVEN WITH opt-in (threat a preserved) - resolved-target-equals-install-root refused EVEN WITH opt-in (threat b) - broken symlink refused EVEN WITH opt-in (fail-closed) * test(#2393): import beforeEach/afterEach in install-write-confinement suite The original file imported only { describe, test } from node:test. The new #2393 opt-in describe block uses beforeEach/afterEach to manage the GSD_ALLOW_SYMLINKED_DEST env var lifecycle — add them to the import. * test(#2393): correct broken-symlink test — existsSync follows link → loop terminates early Initial test expected broken symlinks to be refused even with opt-in. That was wrong: fs.existsSync follows symlinks, so a broken symlink returns false from existsSync and the component loop terminates before the symlink check fires. Both default and opt-in paths share this behavior; the fix preserves it. Updates the test to pin the actual current behavior so a future refactor (e.g. switching to lstatSync for existence) is a deliberate behavior change. * fix(#2393): realpath the install root — guard against macOS /var ↔ /private/var Code review (security subagent) flagged a HIGH-severity hole in the threat-(b) preservation: realTarget (from fs.realpathSync) is fully symlink-resolved, resolvedRoot (from path.resolve) is lexical-only. On macOS /var is a symlink to /private/var, so resolvedRoot='/var/foo/.claude' but realConfigHome is '/private/var/foo/.claude'. A symlink whose realtarget matches the install root by real path would compare unequal to the lexical resolvedRoot — defeating the wipe-protection guard exactly in the reporter's case (Azd325, nix-darwin: ~/.claude is itself a symlink). Fix: compute realRoot once via fs.realpathSync(resolvedRoot) at function entry (with fail-closed fallback to lexical form on realpath failure — broken/missing root, permission denied, exotic FS). Threat (a) path-traversal check above still confines regardless. Compare against BOTH lexical and real forms in both the root-symlink and component-symlink branches. Also adds the reviewer's transitivity-trust clarification comment: once a symlink is followed under opt-in, the walk continues from the resolved real path WITHOUT re-checking further segments stay inside a confining boundary. This is documented opt-in semantics — one opt-in trusts the whole reachable tree — and the comment makes the design choice explicit so a future maintainer doesn't add a 'follow one symlink only' expectation. Regression test added for the macOS /var normalization case (spelled configHome via os.tmpdir() lexically while pointing the test symlink through its realpath). Test skips on non-darwin platforms and when os.tmpdir() has no symlink component. * fix(#2393): root-symlink branch — do not apply threat-(b) check to root itself Initial fix applied the wipe-threat-(b) check to the root-symlink branch unconditionally. That was wrong: when root itself is a symlink (Azd325's nix-darwin case), its realpath IS realRoot by construction — so the check always fires, defeating the opt-in for exactly the case it was meant to enable. The wipe threat (b) does NOT apply to root being a symlink: destDir is a CHILD of root, and resolving root just gives root's target. There is no circular back-reference to root from a path that descends from a resolved root. So the root-symlink branch should just follow the symlink under opt-in and continue the walk, no threat-(b) check. Threat (b) only fires in the COMPONENT loop, where a child symlink can resolve back to the install root. That branch keeps the (b) check using BOTH lexical and real forms of root (the macOS /var ↔ /private/var fix from the prior commit). * fix(#2393): apply opt-in at the 5 bin/install.js call sites + add env-var/transitive tests Code review (correctness subagent) flagged a Critical coverage gap: the initial fix updated only the 4 src/install-engine.cts call sites. Five more call sites in bin/install.js still used the 2-arg signature, so the opt-in env var was silently ignored on: - installCodexConfig (config.toml + agents/ dir + per-agent .toml paths) — Codex only - copyWithPathReplacement (the generic emit path: workflows, commands, staging) — ALL runtimes - resolveInstallRelativePath (path resolver used in various places) Result: a user setting GSD_ALLOW_SYMLINKED_DEST=1 would see SOME refusals disappear (engine path) and OTHERS remain (bin/install.js paths) — a partially-applied install and a confusing UX, directly contradicting the PR's headline claim. Fix: - Export isSymlinkedDestOptIn from src/install-engine.cts alongside hasExistingSymlinkBetween - Import it in bin/install.js - Update all 5 bin/install.js call sites to pass { allowOptInFollow } - Update all 3 bin/install.js error messages to name the env var, matching the engine's phrasing Also addresses reviewer's Medium test-adequacy findings: - isSymlinkedDestOptIn env-var parsing now tested directly (accepts only documented '1' / 'true'; rejects 'TRUE', 'yes', 'on', '0', 'false', empty, unset) - transitive symlink chain (configHome/outer → outside1 → outside2) test pins the documented 'transitive and unbounded' opt-in semantics so a future contributor can't accidentally narrow it * chore(changeset): backfill pr:2445 in .changeset/eager-wasps-swim.md |
||
|
|
517bae8d6d |
fix(#2372): widen decision-coverage-plan to all planner-canonical tags, drop misleading "(or body)" (#2443)
* fix(#2372): widen decision-coverage scan to planner-canonical tags, fix message Bug: check.decision-coverage-plan's remediation message told the user to cite decisions "(or body)" but extractPlanDesignatedSections only scanned <objective>/<tasks>/<task>/<action>. A decision cited in <read_first>, <behavior>, <verify>, <acceptance_criteria>, or <done> was invisible to the gate — false BLOCKING coverage gap, plus the message's own fix-hint sent the user to "the body" where re-citing still failed. Two-part fix (must change together — that drift was the bug): 1. Widen XML_DECISION_TAGS_RE in src/check-command-router.cts to also match <read_first>, <behavior>, <verify>, <acceptance_criteria>, <done>. These are all planner-canonical tags the planner is told to use (plan-phase.md:830-862, plan-phase.md:772). The body negative- lookahead mirrors the opening-tag set so each tag's body is captured independently. 2. Correct buildPlanMessage to name ONLY the surfaces the extractor actually scans (front-matter must_haves/truths/objective, designated markdown headings, and the nine planner-canonical tag bodies). The misleading "(or body)" clause is gone. Also updates the planner's documented contract (agents/gsd-planner.md:69) and user-facing docs (docs/CONFIGURATION.md, docs/USER-GUIDE.md) to reflect the wider scan. Regression tests in tests/decisions.test.cjs cover each newly-scanned tag body, a control (no citation still uncovered), and a message/extractor parity assertion that names every scanned surface — so the two cannot drift apart again. Out of scope (per triage): cmdDecisionCoverageVerify/buildVerifyMessage is a separate command (decision-coverage-verify) checking shipped artifacts, not plan citations — untouched. * chore(#2372): regenerate agent-size-baseline + golden-install-parity fixtures gsd-planner.md grew 49172 → 49294 (+122 chars) from the widened decision- coverage contract (5 new scanned tag names + heading clarification). Growth is justified: the contract surface is itself the fix — the prior text under-described what the gate scans, which was the bug. Updates: - tests/agent-size-baseline.json (gsd-planner.md: 49172 → 49294) - 17 tests/fixtures/golden-install-parity/*.json (one hash per runtime) - tests/fixtures/install-tree/*.json (regenerated by gen:golden) * fix(#2372): per-tag matching — outer-tag citations survive inner-tag nesting Code review (subagent) flagged a Medium edge-case regression from the single-alternation regex: when a newly-scanned tag nests inside another scanned tag, the alternation's negative lookahead halts the outer tag's body at the inner tag — losing any D-NN citation in the outer tag's prefix prose. Concretely: <action>per D-05 <verify>npm test</verify></action> → 3-tag alternation (old): captured 'per D-05 <verify>npm test</verify>' as <action> body → D-05 caught → 9-tag alternation (bug): captured 'npm test' only (from <verify>); D-05 in <action> prefix LOST Switches extractXmlTagBodies to per-tag matching: each tag gets its own regex whose negative-lookahead tempers only against the SAME tag's reopening. So <verify> inside <action> is absorbed into <action>'s body (D-05 caught) AND <verify> is matched separately on its own pass. Per-tag preserves both: - the reporter's case (sibling tags inside <read_first>) - nested-tag citations in outer-tag prefix prose - ReDoS safety (each per-tag regex keeps the #2128 body tempering) Also adds the reviewer's other requested edge-case tests: - non-scanned tag (<name>) bearing D-NN must NOT count - self-closing form <read_first /> safely ignored - attribute form <verify type="...">D-NN</verify> (canonical planner shape) - CRLF newlines in tag body do not break capture * chore(changeset): backfill pr:2443 in .changeset/noble-elks-chatter.md |
||
|
|
0bbbca2a46 |
fix(#2069): forward model_policy, model_profile_overrides, runtime from global defaults (#2442)
* test(#2069): add fail-first regression for global-defaults dropped keys Adds four failing-first regression cases to tests/defaults-json-fallback.test.cjs: - model_policy forwarded from ~/.gsd/defaults.json - model_profile_overrides forwarded from ~/.gsd/defaults.json - runtime forwarded from ~/.gsd/defaults.json - parity: model_policy survives identically whether it lives in the global defaults or in a project's .planning/config.json All four fail on unfixed code (Branch D of loadConfigResolved builds _globalBaseCfg from a whitelist that omits these three keys). The project-config path at config-loader.cts:602-604 already forwards them, so the global path should too. * fix(#2069): forward model_policy, model_profile_overrides, runtime from global defaults The _globalBaseCfg whitelist in Branch D of loadConfigResolved previously omitted three keys that the project-config path forwards parsed['…']: - runtime - model_profile_overrides - model_policy so ~/.gsd/defaults.json silently dropped them. A machine-wide model policy (or runtime / profile overrides) was honored inside a project (where .planning/config.json carries it) but ignored for out-of-project runs — resolve-model fell back to the profile default with no warning. Adds the three entries to _globalBaseCfg in the same (globalDefaults['…']) || null shape as the sibling keys and the project-config path, so global defaults honor them identically. Regression tests in the prior commit (#2069 fail-first) demonstrate the fix on the same suite that previously failed. * test(#2069): extend parity test to all three previously-dropped keys Code review (subagent) flagged that the parity test only asserted model_policy shape-parity between global-defaults and project-config paths. A future regression breaking just runtime or just model_profile_overrides shape (e.g. someone changing parsed['runtime'] to ?? null in the project path) would slip a single-key test. Extends the parity test to assert deepStrictEqual / strictEqual across all three keys: model_policy, model_profile_overrides, runtime. Same two-dir setup, three cheap assertions. * chore(changeset): backfill pr:2442 in .changeset/sturdy-seals-fly.md |
||
|
|
d16a66479a |
feat(#1950): broken-windows ledger — cross-phase defect register gating ship (#2441)
* feat(#1950): broken-windows ledger — cross-phase defect register gating ship Adds a new capability (#1950) that operationalizes GSD's no-defer discipline as a tracked, enforced artifact: accumulates stubs, TODOs, skipped tests, unrun verifies, and unmet truths across phases, and /gsd-ship blocks while any entry is open. Implementation: - src/broken-windows.cts → gsd-core/bin/lib/broken-windows.cjs: typed IR + I/O entry points (parseLedger/renderLedger/appendWindow/markWaived/markFixed + cmdWindowsStatus/Append/Waive/MarkFixed). Frozen REASON enum for typed error assertions. Windows-safe atomic rename with retry on transient EPERM/EBUSY/EACCES. - gsd-tools.cjs: new subcommand (status | append | waive | fixed), wired via routeWindows + HOST_COMMAND_ROUTERS.windows. - capabilities/broken-windows/capability.json: one ship:pre gate with artifact-frontmatter-equals predicate on WINDOWS.md open_count == 0. activationKey windows.enabled (default true) + sibling windows.enforce (default true, separate so tracking can precede enforcement). - gsd-core/workflows/ship.md: capId==broken-windows branch in preflight, sibling to security — reads gsd_run windows status --raw, fails closed on open_count > 0 or unreadable ledger. - agents/gsd-executor.md: extends the existing ## Known Stubs instruction to also append to WINDOWS.md via gsd_run windows append (best-effort, never blocks execution). - agents/gsd-verifier.md: new Step 8b — record unmet truths + human-verify items in WINDOWS.md. - gsd-core/workflows/progress.md: surfaces open + waived counts. - docs/COMMANDS.md + CONTEXT.md glossary entry + docs/INVENTORY.md: document the gate, waiver mechanism, and new module. - tests/broken-windows.test.cjs: pure + CLI behavioral coverage + fast-check roundtrip property; fail-closed on malformed ledger; security boundary on path traversal in --file. Backward-compatible: a project with no .planning/WINDOWS.md reports open_count: 0 and ships cleanly. Disable enforcement per-project with gsd config-set windows.enforce false (tracking continues, gate stays open). * chore(#1950): ratchet size baselines, defer verifier integration - Workflow size baseline: ship.md 25575→27928, progress.md 31789→32632 (broken-windows preflight branch + open-windows surface). - Agent size baseline: gsd-executor.md 46644→47951 (Known Stubs → also appends to WINDOWS.md). gsd-verifier.md unchanged. - LARGE_CAP (49152) preempted the planned verifier integration (gsd-verifier.md was at 49140 pre-PR — 12 bytes of headroom, not the documented 'real headroom'). Verifier integration deferred to a follow-up PR that extracts the VERIFICATION.md template (lines 739-859) to gsd-core/references/ — a pre-existing cap-tightness defect this PR exposed but does not expand scope to fix. Verifier integration is not in the issue's acceptance criteria (executor writes is; unmet-truths recording was an enhancement, not a gate). * fix(#1950): gate default-off, rename to workflow.windows_enforce, regen goldens Test-failure-driven fixes after first gsd-test run on db8733c8f failed 44 cases (pre-existing structural tests encoded 'ship:pre has 1 gate' / 'all caps off → empty hooks'): - capability manifest: rename windows.enabled+windows.enforce (default true) → single federated key workflow.windows_enforce (default FALSE, opt-in). Matches security's workflow.security_enforce convention and makes the adr857 all-caps-off test pass without modification (the test's buildAllFalseConfig handles workflow.* out of the box). Default-OFF keeps the gate out of the registry's default ship:pre resolution so existing loop-hooks-ship-pre-e2e structural assertions (exactly 1 gate, capId 'security') stay valid; users opt in via gsd config-set workflow.windows_enforce true. - drop activationKey (security doesn't have one either; workflow.* key doubles as the activation toggle). - regenerate docs/reference/capability-matrix.md to include broken-windows (capability-matrix-sync test). - regenerate tests/fixtures/golden-install-parity/*.json (18 runtimes) — installer now emits the new capability + lib file. - update CONTEXT.md, docs/COMMANDS.md, docs/FEATURES.md, ship.md, agents/gsd-executor.md to use the new key name and /gsd:colon slash syntax (slash-command-namespace test). - restore accidentally-regressed /gsd:capture in progress.md. Tracking-only by default; enforcement is opt-in. Acceptance criterion '/gsd-ship fails while any ledger entry is open' is met when workflow.windows_enforce=true (test fixture enables it). * test(#1950): update ship:pre structural invariants for 2-gate registry - loop-hooks-ship-pre-e2e: the registry now declares 2 gates at ship:pre (security + broken-windows), regardless of activation. Activation tests above still pin security-only or empty behavior via fixtures; these structural tests pin the REGISTRY shape, which has 2 gates as of #1950. - workflow-size-baseline: ship.md 27928→27945 (workflow.windows_enforce rename added 17 bytes). * fix(#1950): review H1+H2+M1+M2+M3 — fence-injection, EACCES fail-closed, cleanup, strict line, stryker Adversarial isolated review (Step 6.3) found 2 HIGH findings that block the PR and 3 mediums. All addressed: H1 (HIGH): description containing the markdown 3-backtick fence would terminate the ledger's JSON code block early inside JSON.stringify output (JSON doesn't escape backticks), corrupting the file and bricking the next parse. Fix: use a 4-backtick fence (json ... ) which JSON.stringify cannot produce on its own, AND validate that no entry text field contains a 4-backtick run (reject at append time with new WINDOWS_INVALID_TEXT reason code). Locked by a regression test. H2 (HIGH): readLedgerOrNull swallowed ALL fs errors as 'no ledger', silently returning open_count:0 on EACCES/EPERM/EIO. The ship gate would then pass on an unreadable ledger — the precise vector the workflow doc claims is impossible. Fix: only ENOENT returns null; every other fs error propagates as WINDOWS_LEDGER_MALFORMED so the gate blocks and the operator sees a real diagnostic. Locked by a regression test that chmod 000s a ledger with open_count=1 and asserts the result is never a false-green 0. M1: writeLedgerAtomic left an orphaned .tmp file on rename failure. Wrapped renameWithRetry in try/catch with best-effort unlink. M2: validateLine silently coerced 'abc' → NaN → null, hiding type drift. Removed the line === 0 special case (was undocumented) and made the error message match the strict check. Now any non-positive- integer line value throws, including strings. M3: tests/broken-windows.test.cjs (with its fast-check property test) was not in stryker.config.mjs DEFAULT_TEST_CMD — Stryker would mutate src/broken-windows.cts but no test would catch the mutations, producing false surviving-mutant scores. Added to the list. L1 (dead throw e after error()), L7 (line boundary tests, H1/H2 regression tests, 4-backtick CLI test) also addressed. * docs(#1950): inline concurrency + busy-wait notes (review L2+L3) * fix(#1950): regen goldens against latest gsd-tools; correct --line 0 boundary test gsd-test v4 caught two issues: - goldens I regenerated earlier (commit 526682084) predated the L1 routeWindows catch-block cleanup (commit dd844d565). Regenerated via 'npm run gen:golden' against current HEAD so the install parity hash for gsd-tools.cjs matches. - 'append --line boundary' test expected --line 0 to succeed with null entry.line, but the M2 fix correctly rejects 0 (lines are 1-indexed; 0 is not a valid source line). Updated the boundary test to assert --line 0 fails alongside -1 and 'abc'. * chore(#1950): regen goldens after rebase onto next * chore(#1950): quick.md baseline 50699→50993 (correct resolution from next rebase) * chore(changeset): backfill pr:2441 in .changeset/broken-windows-ledger.md * fix(#1950): renderTable escapes backslash before pipe (CodeQL incomplete-sanitization) CodeQL flagged the markdown-table cell escaper: String(s ?? '').replace(/\|/g, '\\|') — it escapes pipe but not backslash first. A description containing '\|' would render as '\\|' which markdown parses as 'literal backslash' + 'cell separator', splitting the column. Fix: escape backslash FIRST (each \ → \\), then pipe (each | → \|). Now a description with '\|' renders as '\\\\|' (literal '\\' + escaped pipe), which markdown renders as a single '\|' inside the cell. The JSON code block (the parse source-of-truth) was already correctly escaped via JSON.stringify; only the display-only table was affected. Locked by a regression test that: 1. Verifies the JSON block reparses with the description intact. 2. Walks the rendered table row counting unescaped pipes — must be exactly 11 (the row separators for 10 cells), proving no in-cell pipe added a split. |
||
|
|
1a46bc068a |
fix(#2376): emit absolute subagent-facing paths from init/state, convert workflow literals (#2428)
* fix(#2376): emit absolute subagent-facing init/state paths Make init.* and state.* path fields absolute rather than cwd-relative so subagent prompts resolve correctly regardless of working directory. Adds intel_dir/conflicts_path/requirements_path/roadmap_path/state_path to cmdInitIngestDocs, an absolute debug_dir to cmdStateLoad, and replaces bare .planning/... literals in 12 workflow Agent() prompt blocks with the absolute init-JSON path fields. Includes decoy-cwd regression tests and realpath'd tmpdir fixtures for macOS. Squashed rebase of the #2376 commit series onto a fresh origin/next (previous merge ee25543a1 was against a now-stale next). * chore(#2376): add changeset * chore(#2376): regenerate golden fixtures + workflow size baseline Regenerated after rebasing the absolute-path fix onto current next (picks up #2351's run-with-timeout content in execute-phase.md too). * fix(#2376): trim execute-phase.md redundancy to stay under the size margin * chore(#2376): regenerate golden/size baseline after rebase onto next |
||
|
|
d04e287fa9 |
fix(#2365): stop api-coverage detector false-positiving non-API phases (#2397)
* fix(#2365): stop the api-coverage detector false-positiving non-API phases detectApiIntegration fired on any integration verb co-occurring anywhere on a line with any API noun, treated / as a word boundary (so a first-party Next.js src/app/api/... route path matched the noun "api"), and read any capitalized word before API/SDK/REST/GraphQL as a service name behind a fixed stopword denylist (so threat-model prose like "Resolver-only API" fired). Because the verify:pre seal gate is BLOCKING, a phase touching no external API could not reach UAT without fabricating a coverage matrix. The compound rule now requires the verb and noun to share one clause (sentence punctuation and table-cell walls end a clause) within a bounded word gap. Non-prose spans are excluded before matching: fenced code (already), inline code spans (new stripInlineCode in the markdown-sectionizer seam), and path-shaped tokens. The <Service> API surface rule requires proper-noun position — a clause-initial capitalized word is ordinary English and needs dependency evidence (URL / package reference) on the same line — and rejects compound modifiers ("Resolver-only", lowercase after the hyphen). A phase that integrates no external API now has a first-class, reasoned way to say so: a COVERAGE.md containing "No external API integration: <reason>" satisfies the gate (declaration + rows is contradictory and blocks). The true-positive path is pinned by regression tests: every default-vocabulary positive still fires, including the widest word-gap pairing and the surface-rule-only shape. Fixes #2365 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2365): tighten api-coverage detector per Codex review (round 2) Applies the Codex review findings on the initial #2365 fix: - S-1: a COVERAGE.md "no external API integration" declaration is the human override for a fallible detector, so it must PASS even when detection still fires — but the contradiction is now SURFACED in the gate output (overridden signal count + terms) instead of passing silently. - S-2: verb/noun pairing is now a term-group nearest-pair merge walk over precomputed word ordinals (computeWordStarts / minWordGap), not a match×match cross product — a hostile line repeating one pair thousands of times stays linear instead of going quadratic. - FN-4: package-shaped inline-code spans (`stripe-sdk`, `@stripe/stripe-js`) are kept as noun/dependency evidence rather than being fully masked, so a genuine dependency reference inside code ticks still corroborates. - C-1: the <Service> API surface rule now scans every candidate in every clause; a rejected first candidate no longer shadows a later genuine service. - Cross-clause binding: a verb may bind a noun in the immediately following clause only when its own clause names a service object, within a tight gap — admits "Integrate Stripe, exposing its endpoints …" without re-admitting the unrelated-clauses false-positive class. - Internal-descriptor negative evidence ("internal Payments API", "the internal endpoint") never pairs; URL/scheme matching generalized beyond http(s). All 5 acceptance criteria still hold: the three reported false positives are clean and "integrate the Stripe API" still fires. Built .cjs committed alongside the .cts. tsc + eslint (incl. no-adhoc-markdown-parsing) + lint:regression-names clean; affected suites 256/256 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#2365): retune api-coverage detector fail-closed per Codex review (round 3) Codex's second-round review found the round-2 tightening had over-corrected into FAIL-OPEN false negatives — realistic external-API prose that the BLOCKING seal gate silently let through (the catastrophic class, since a missed API surface is worse than a dismissable false positive). Retuned the detector to be explicitly fail-closed: lean toward detecting, and let the one-line COVERAGE.md "no external API integration" declaration dismiss the residual false positives. Fail-open false negatives fixed (all now detect): - F1 clause-initial `<Service> API` with a plain follower ("Stripe API for payment processing") — dropped the follower-allowlist / corroboration gate on clause-initial surfaces; a service that is not a stopword, descriptor, or compound modifier is a real name from any clause position. - F2 scheme-less external host ("api.stripe.com/v1") — a dotted host with an alphabetic final label now contributes its API nouns; a first-party route path (no dotted host) still does not. - F3 vendor's first-party SDK ("Integrate Shopify's first-party SDK") — the compound path no longer filters nouns on "internal"/"first-party" (Codex: the qualifier can describe the vendor's own API, not the consuming project's). - F4 long single integration clause — removed the word-gap cap entirely: it could not separate a 21-word genuine clause from an 18-word internal one, so the clause boundary is now the whole relationship test. - F5 lowercase cross-clause service — cross-clause binding no longer requires a capitalized "service object". New false positives fixed (all now clean): - F6 a URL token that swallowed a trailing clause comma, merging two clauses — trailing clause punctuation is kept literal so the split survives. - F7 a capitalized internal component authorizing cross-clause binding — the new gate requires a dependent elaboration, not a new coordinate clause opened by a conjunction ("…, then document…"). - F8 a protocol name read as a service ("REST API", "GraphQL API") — protocol and locality descriptors are rejected in the `<Service>` position. - Finding 9: the inline-code-span scanner was O(n^2) on pathological backtick runs; rewritten to linear via a per-length run cursor (2 MB: 4.15 s -> ~6 ms), semantics preserved (148 sectionizer tests unchanged). Net simplification: the fail-closed model removed the round-2 minWordGap / groupByTerm / follower / corroboration machinery (350 insertions vs 445 deletions across the touched files). Under fail-closed, three round-2 negative tests now correctly detect (integration verb + "internal"-qualified noun, and the distant-same-clause case); none were trek-e acceptance FPs. Verified: 1491/1491 unit tests pass; tsc + eslint (incl. no-adhoc-markdown- parsing) + lint:regression-names clean; all 8 review findings reproduced as regression tests, both directions. Built .cjs committed alongside the .cts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#2365): resolve round-3 Codex review findings (fail-closed, round 4) Codex's round-3 adversarial review found the fail-closed retune had introduced new holes in both directions. Resolved: Fail-open false negatives (now detect): - External host addressing a PATH ("graph.microsoft.com/v1.0/me") is itself an integration surface and contributes an endpoint noun even when the host names no vocabulary word. A bare domain link with no path ("https://example.com") stays a non-signal, so "Integrate … from example.com, document …" is still clean. - Locality qualification ("internal", "private") no longer leaks across a sentence or clause boundary: only plain spaces may separate the descriptor from the service, so "The cache is private. Stripe API …" now detects. - Cross-clause binding: the fragile head-word cap (which could not tell a genuine "Connect … to Stripe payments, exposing its endpoints" from an unrelated "Integrate … from URL, document …" — both 4 words after the verb) is replaced by a participial-continuation rule: a verb binds a noun in the next clause only when that clause begins with an "-ing" elaboration. This fixes the 4-word-head false negative AND the false positive below at once. False positives (now clean): - Cross-clause no longer binds a finite continuation regardless of separator: "Wire the settings form. Document endpoint props." / "…; document …" / "…, document …" are separate actions, not elaborations. Perf (quadratic → linear): - The trailing-punctuation peel is a backward char scan instead of an unanchored `[…]+$` regex (16k chars: 156 ms → ~1 ms). - SERVICE_SURFACE_API_RE bounds the service-name length {1,40} so a hostile "A-A-…-x" run cannot drive O(n^2) backtracking (16k: 385 ms → ~3 ms). Consumer fail-open (blocking gate): - readPhaseScope now distinguishes "no plans" from a plan that EXISTS but is unreadable. On a read error the gate BLOCKS ("could not read the phase scope …") instead of silently certifying no-integration from partial scope — an unreadable plan could be the one describing the integration. Documented fail-closed tradeoffs, now pinned with tests so they are not "fixed" back into a fail-open: a clause-initial capitalized common word before "API" ("Payment API", "Search API") reads as a service name; a long clause pairs a verb with a distant noun; and a CommonMark inline code span that wraps a newline is matched within-line only. Codex judged these acceptable because the COVERAGE.md declaration is a cheap override. One documented limitation remains out of scope: "Integrate Stripe, and authenticate requests with its API" (a coordinate finite clause whose noun refers back by pronoun) needs coreference resolution, beyond a lexical detector. Verified: 379/379 affected + command-router tests pass (+14 new regression tests covering every round-3 finding, both directions); tsc + eslint (no-adhoc-markdown-parsing) + lint:regression-names clean. Built .cjs committed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#2365): simplify to robust core — remove whack-a-mole heuristics (round 5) Round-4 review confirmed the detector's two most complex features generate findings in both directions no matter how they are tuned, because they need a vendor dictionary + coreference the issue rules out in principle. Per the operator's "ship the robust core" decision, both are removed and their gaps are documented rather than chased further: - Cross-clause binding DELETED (allowsCrossClause / participle rule). It caused a fail-open on finite continuations ("Integrate Stripe; use its OAuth endpoints" — missed) and a false positive on "-ing"-SPELLED nouns ("…, billing endpoint terminology…" — wrongly fired). Detection is now same-clause only. - URL-path-as-evidence REVERTED. Treating every path-bearing URL as an endpoint fired on ordinary asset/link URLs ("…/theme.css", "…?next=/x", a docs/repo link) and recreated routine UI-phase false positives. An external URL is evidence only when it NAMES an API vocabulary word ("api.stripe.com/v1"). Two fail-open cases are now DOCUMENTED limitations, pinned by tests so a future maintainer does not re-add the heuristics that caused the false positives above: a service named only in a clause separate from its API noun, and a bare external host that names no vocabulary word. Both are cheaply covered by the COVERAGE.md declaration and rare in real phase prose ("integrate the X API"). Also fixed from the round-4 review: - Qualification now survives markdown emphasis ("The **internal** Payments API" stays clean) while still not crossing a sentence/clause boundary. - readPhaseScope fail-closes on a REAL read failure (EACCES/EIO) enumerating the phase directory or reading the roadmap fallback — not only per-plan-file failures; a missing directory/section remains a legitimate no-op. The declaration-override path surfaces scope_read_error so an incomplete-scope override stays visible. - SERVICE_SURFACE_API_RE length-bound comment no longer overclaims. Net: the detector is same-clause verb+noun + `<Service> API` surface, with path/code/inline masking and a fail-closed posture. All five acceptance criteria hold. 1573/1573 unit tests pass; tsc + eslint (no-adhoc-markdown-parsing) + lint:regression-names clean. Built .cjs committed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(#2365): close roadmap-fallback fail-open + stale JSDoc (round-5 review) The round-5 sanity review confirmed the detector simplification is sound (all acceptance positives fire, all required negatives clean) and flagged one real blocker plus a nit: - Blocker: readPhaseScope's roadmap fallback could still silently pass an UNREADABLE roadmap. getRoadmapPhaseWithFallback gated on fs.existsSync(), which returns false on EACCES/EIO too — so an unreadable ROADMAP.md read as "absent", no exception reached isRealReadFailure, and the blocking gate certified empty scope. Fixed at the source: read the roadmap directly and honor the function's OWN documented contract — null only on ENOENT (genuinely absent), otherwise throw. Both existing callers already wrap it in try/catch expecting that throw, and readPhaseScope now fail-closes (blocks) via its roadmap catch. Verified by a new e2e test (unreadable roadmap fallback → block). - Nit: the detectApiIntegration JSDoc still described the removed cross-clause participial binding and "every external hostname counts" — corrected to the actual same-clause-only behavior and the names-a-vocab-word URL rule. Verified: full unit suite green; tsc + eslint + lint:regression-names clean. Built .cjs committed (roadmap.cjs is gitignored/rebuilt, per repo convention). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(#2365): backfill changeset PR number (#2397) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(#2365): sync generated capability-registry + recapture install goldens CI surfaced two generated-artifact staleness issues (all failing test shards + lint-tests traced to these, not to a logic defect): - gsd-core/bin/lib/capability-registry.cjs was stale: the initial fix edited the ai-integration `api-coverage-plan-pre.md` fragment (added the "No external API integration" declaration section) but did not regenerate the registry, which embeds an inline copy of that fragment. Regenerated via `gen-capability-registry.cjs --write` — the diff is exactly the fragment text sync. Fixes `lint:generated-sync` and the "committed registry is in sync" + "registry integration" tests. - The 18 golden-install-parity fixtures were stale by exactly one hash line each — `gsd-core/references/api-coverage.md`, which this PR edits and which is a hashed installed artifact. Recaptured with `UPDATE_GOLDEN=1`; the diff is that single hash per runtime and nothing else. Fixes the `golden parity — *` tests. No source or behavior change — generated artifacts only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(#2365): flip representative-corpus manifest to assert the fixed behavior The #2371 representative corpus (merged into next after this branch was cut) is a known-bug tripwire: it asserts each fixture's currentBuggyOutput so the test fails loudly the moment #2365 is fixed, at which point — per its own contract in representative-corpus.test.cjs — the fixer removes currentBuggyOutput so the assertion checks expectedDetected instead. This is that moment. Removed currentBuggyOutput from the three detector fixtures (nextjs-route-path, unrelated-verb-noun, threat-model-prose); the corpus now asserts detected:false, which the fail-closed same-clause detector satisfies. Notes updated to describe the fix rather than the bug. The #2366 matrix corpus is left untouched — that tripwire belongs to its own PR (#2374). Corpus test: 7/7 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(#2365): skip chmod-000 fail-closed e2e tests on Windows The three fail-closed gate tests induce an unreadable plan / directory / roadmap with chmod 000, but Windows does not enforce POSIX mode bits — readFileSync still succeeds, so the gate never reaches the read-error path and the assertion fails on the windows-latest CI leg. The fail-closed LOGIC is platform- independent (readError → block) and is fully exercised on the macOS/Linux legs; only the method of inducing EACCES is POSIX-specific. Guard the three tests to skip on win32 as well as root, mirroring golden-install-parity's win32 skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(#2365): address trek-e review — glossary, clock-seam, IO injection, bounds Review response to PR #2397 (trek-e, CHANGES_REQUESTED). Fix logic unchanged; this closes the test/process-hygiene findings. Major: - CONTEXT.md "Markdown Sectionizer" glossary now lists the two exports this fix relies on, `stripInlineCode` and `scanInlineCodeSpans` (glossary is a PR gate). - Replaced the banned wall-clock assertion in the "hostile repeated-term line" test (Clock Seams rule — no elapsed-time asserts) with a deterministic signal-count assertion, which also directly verifies the term-dedup that keeps pairing linear (one signal for a 10k-pair line, not thousands). - Rewrote the three fail-closed read-failure tests: instead of chmod 0o000 (a no-op under root / on Windows, the pattern the repo's IO-failure convention avoids) they now exercise the newly-exported `readPhaseScope` in-process and inject the failure by monkeypatching fs.readFileSync/readdirSync to throw, restoring in finally. Deterministic and platform-independent (no skip needed), and they add the ENOENT-is-absence case that the chmod tests couldn't express. Minor: - Added limit / limit+1 boundary tests for SERVICE_SURFACE_API_RE's {1,40} service-name bound, QUALIFIER_LOOKBACK's 24-char window, and REASON_MAX_LEN (200) on the declaration reason. - Added a fast-check property that fuzzes the tokenizer / clause splitter / masking (scanLineTokens, splitClauses, collectTermMatches) with adversarial tokens (slashes, backticks, URLs, clause punctuation) and asserts the detector is total (never throws), shape-stable, holds detected <=> signals, and is deterministic. readPhaseScope is exported for the in-process tests. Verified: 125 detector + 19 gate tests pass; tsc + eslint + generated-sync (glossary/registry) + lint-regression-test-names + lint-test-file-count clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |