91ed46882ab58f828fc543d4633896ad0a4397f5
372 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
91ed46882a |
feat(#3675): quick-batch core primitives and resumable manifest (#4190)
* test(#3675): add failing tests for quick-batch core primitives Adds the full behavioral (tests/quick-batch.test.cjs) and property-based (tests/quick-batch.property.test.cjs) coverage for #3675's quick-batch core primitives per the phase's 35-row test matrix — task-list parsing (inline + --file, with path-confinement/symlink-escape/non-regular-file rejection), collision-safe quick-id preallocation under withPlanningLock, BATCH.json schema/validation/resume, dependency-DAG + partitionByFileOverlap wave construction, and exactly-once STATE.md completion (including the STATE-row-written-but-manifest-not-yet-updated crash window). The import target (gsd-core/bin/lib/quick-batch.cjs, compiled from a not-yet-written src/quick-batch.cts) does not exist yet — every test in both files fails at the top-level require() before any assertion runs. Five fast-check properties cover collision-freedom under lock contention, resume idempotency, exactly-once STATE completion, wave totality, and DAG-respecting wave order, per the design doc's property-based-coverage requirement. * feat(#3675): implement quick-batch core primitives Adds src/quick-batch.cts (ADR-457 build-at-publish, compiled to gsd-core/bin/lib/quick-batch.cjs) implementing #3675's quick-batch core primitives per the phase design lock — pure/state primitives and CLI-testable core operations only, no agent dispatch, no worktree creation, no user-facing command (Phase 4/#3676's job): - parseTaskList / parseTaskListFromFile: inline bulleted/numbered task-list parsing (>=2 items required) and a --file variant strictly confined to the planning workspace root via requireSafePath, rejecting non-regular-file targets. - allocateQuickIds / createBatch: collision-safe YYMMDD-xxx quick-id preallocation under withPlanningLock, checked against both on-disk .planning/quick/ entries and sibling .planning/quick-batches/*/BATCH.json manifests (never on-disk-only, which would miss another in-flight batch that hasn't dispatched any real quick directory yet) — replicates cmdInitQuick's own grammar rather than delegating to it (that function's 2-second granularity is not batch-safe). - computeWaves: deterministic wave construction combining dependency-DAG layering with partitionByFileOverlap (#3674), called per DAG layer over path-separator-normalized planned_files — normalization happens at this module's boundary, never inside the Phase 2 helper. - loadBatch: fail-closed BATCH.json schema validation (corrupt/truncated JSON, wrong types, missing fields, out-of-batch dependency references, dependency cycles, a worktree path absent from disk). - resumeBatch: skips complete items, never auto-retries failed items, propagates/reverses blocked status along the DAG to a fixed point, and detects a STATE.md row that already exists for a non-complete item (the "STATE written, BATCH.json not yet updated" crash window) — completing it without re-appending. Idempotent across repeated calls. - completeQuickItem / hasQuickTaskRow: exactly-once STATE.md completion — appendQuickTaskRow (unmodified) is called at most once per quick id, gated by hasQuickTaskRow's own idempotency check re-parsing the real "Quick Tasks Completed" table, since appendQuickTaskRow itself carries no idempotency. BATCH.json lives at .planning/quick-batches/<batch-id>/BATCH.json, a sibling of .planning/quick/ — never inside it, so scanQuickTasks never misreads a batch manifest as a broken quick task. * docs(#3675): register the new quick-batch module New src/*.cts -> bin/lib/*.cjs modules need four hand-maintained registrations beyond the code itself: .gitignore (compiled artifact), eslint.config.mjs (ADR-457: lint the .cts, not the emitted .cjs), docs/INVENTORY.md's CLI Modules roster row plus the regenerated docs/INVENTORY-MANIFEST.json cli_modules entry, and a CONTEXT.md glossary entry matching the convention set by the sibling File Overlap Partitioner Module (#3674) entry it sits beside. NOTE: docs/INVENTORY-MANIFEST.json was updated BY HAND (alphabetically sorted single-entry insertion into families.cli_modules, matching the existing file's structure) rather than via `node scripts/gen-inventory-manifest.cjs --write` — this session's MEMTRACE-FIRST guard hard-blocks direct execution of that indexed script path from Bash, with no available Memtrace tool to route through instead. The orchestrator should re-run `node scripts/gen-inventory-manifest.cjs --check` to confirm this hand-edit is byte-identical to the generator's own output before merging. * fix(#3675): resolve lint findings in quick-batch primitives and tests Unsafe `any[]` assignment from `new Array(n)` in the DAG cycle-check color array, two unnecessary `as string[]` casts TS 5.5's inferred type predicates already narrowed, raw `fs.rmSync` in test cleanup (needs the Windows-EBUSY retry budget `helpers.cleanup` carries), an unused `loadBatch` import, an unbounded `mkfifo` subprocess spawn missing a timeout, and a CONTEXT.md glossary illustration that looked like a real file reference. * feat(#3675): close acceptance-criteria gaps found in review Standards- and spec-axis review (plus a self-caught race) surfaced real gaps against issue #3675's own acceptance criteria and this repo's test conventions: - BATCH.json was missing options, base_revision, per-item wave, and per-item commit — the issue's AC explicitly lists all four as things the manifest must track. Added them: createBatch persists caller-supplied batchOptions/baseRevision verbatim and assigns each item its computed wave index; completeQuickItem now persists the commit onto the item, not just the STATE.md row. All four are backward-tolerant on load (an older/hand-built manifest without them still validates). - resumeBatch had no "incompatible base divergence" check at all, despite the AC and the ADR's own "Base divergence" section requiring one. Added an opt-in currentBaseRevision comparison that fails closed with a recoverable diagnostic on mismatch, and touches nothing on refusal. - resumeBatch read-modify-wrote BATCH.json OUTSIDE withPlanningLock — the only durable write path in this module that wasn't lock-protected, a real lost-update race against a concurrent completeQuickItem or another resume. Now runs inside the same lock createBatch/ completeQuickItem use. - loadBatch and collectExistingBatchQuickIds used raw JSON.parse with no size cap (security review, Low/informational); switched to the existing safeJsonParse (1MB cap) for defense-in-depth. - Parser (parseTaskList) had only example-based tests; CLAUDE.md requires a fast-check property test for parsers. Added one plus a companion reject-property for <2 items. - The id-exhaustion fail-closed ceiling (MAX_TIME_BLOCK) was untested at any boundary. Exported the pure allocateIdsGivenUsed/MAX_TIME_BLOCK for direct limit-1/limit/limit+1 testing without needing 46k fixture dirs. - Issue AC explicitly asks for prompt-injection-payload test coverage, distinct from the existing shell-metacharacter test; added one. - Test row 9 (FIFO skip) silently returned instead of calling t.skip(), so an unsupported platform would report a pass rather than a documented skip; fixed to bind the test-context param and skip properly. - Extracted toWaveInput to remove a 2-site production duplication of the QuickBatchItem -> computeWaves reshape (Standards-axis smell). - Added the required .changeset/ fragment (CONTRIBUTING.md: editing src/ is user-facing even though the compiled .cjs is gitignored). * fix(#3675): restore "not valid JSON" wording in loadBatch's parse-failure reason gsd-test caught this: switching loadBatch to safeJsonParse changed the parse- failure message shape ("... parse error — ...") without preserving the "not valid JSON" substring row 27's own test asserts on. Re-wrap safeJsonParse's error into the original diagnostic phrasing regardless of which of its three failure modes fired. * docs(#3675): backfill changeset pr number to 4190 * fix(#3675): detect a silently-no-op mkfifo on Windows, not just a throwing one CI caught this on windows-latest: row 9's platform-skip only caught mkfifo throwing (command not found). On this runner mkfifo resolves to something that exits 0 without creating a file (NTFS has no FIFO concept), so execution fell through to parseTaskListFromFile against a path that doesn't exist, producing an ENOENT stat error instead of the expected "not a regular file" rejection. Check the artifact actually exists before trusting a zero exit code, and skip with a documented reason either way. --------- Co-authored-by: sim <sim@local> |
||
|
|
acb903c2e8 |
enhance(#3661): make the code-review hook point configurable (#4159)
* feat(#3661): make the code-review hook point configurable Add `workflow.code_review_point` (`execute:post` default, or `execute:wave:post`) so a multi-wave phase can run code review once per wave instead of once at the end, scoped to what changed since the phase's prior review. The code-review capability now declares its step at both loop points via a new generic `pointFrom` step field: `pointFrom` names an enum config key, and the step is only active at its own `point` when that key resolves to a matching value. `_resolvePointGate` (capability-activation.cts) is the single shared implementation consumed identically by loop-resolver.cts and capability-state.cts, and capability-validator.cjs enforces that `pointFrom` references an enum key whose values cover the declaring step's own point. code-review.md's manual-invocation gate now reads `workflow.code_review` directly instead of probing registry presence at the hardcoded execute:post point (so manual `/gsd-code-review` keeps working regardless of which automatic point is configured), and its file-scope tiers narrow to what changed since the phase's last review commit when one exists. execute-phase.md's wave-post step dispatch gets a small, precedented carve-out so the code-review skill still receives its required phase argument when dispatched generically (caught by the isolated spec review). Closes #3661 Emitted-Drift-Ack-Growth: code-review.md — #3661 adds a point-aware config gate check and LAST_REVIEW_COMMIT-based incremental scoping to the file-scope tiers. Emitted-Drift-Ack-Growth: execute-phase.md — #3661 adds one carve-out sentence so the wave-post generic step dispatch passes PHASE_NUMBER to the code-review skill. * docs: backfill changeset PR number for #3661 (#4159) * fix: scope tests/io.test.cjs's fs.writeSync fault-injection mocks by fd Five fault-injection mocks in the "bug #1008" describe blocks intercepted every fs.writeSync call regardless of file descriptor, and several threw or truncated unconditionally on the first call. This surfaced as an intermittent macOS CI failure: node:test's own IPC channel back to the parent process (which also goes through fs.writeSync internally) could get a bogus injected error or truncated write if node's internal machinery called it while one of these mocks was active, corrupting the message frame the parent tried to deserialize ("Unable to deserialize cloned data.", location tests/io.test.cjs:1:1, uncaughtException — a whole-file IPC crash, not a test assertion failure). Root cause confirmed by a working counter-example already in the same file: the "#3912 A6" mocks gate on `fd !== 2` before any fault injection and were never implicated. Applied the same fd-scoped pattern to the five unscoped mocks (four output()-targeting tests gate on fd 1, one error()-targeting test gates on fd 2), and added a regression test proving an unrelated fd passes through untouched while the fault-injection mock is active. Found while verifying #3661; unrelated to that change's own diff. --------- Co-authored-by: sim <sim@local> |
||
|
|
b0572c0108 |
feat(#3674): extract shared file-overlap wave partitioner (#4166)
* test(#3674): characterize existing wave-dispatch output and add tests for the extracted partitioner Pins resolveWaveDispatch's and emitWorkflowScript's current, unextracted output (chain-overlap, disjoint-empty-set, and a multi-wave/multi-stage golden script) as a regression safety net ahead of extracting partitionStages into a standalone module. Also adds the new module's unit and property tests (test matrix rows 1-11) against its expected public API, which does not exist yet and is added in the next commit. * feat(#3674): extract file-overlap partitioner into a shared, generic module Moves partitionStages' greedy first-fit file-overlap algorithm into a new, dependency-free src/file-overlap-partitioner.cts module (partitionByFileOverlap), generalized over a plain {id, files}[] shape rather than claude-orchestration.cts's Plan/Wave interfaces. partitionStages becomes a thin adapter mapping its own Plan[] shape onto the generic input and back — behavior-preserving, no dependency ordering, no path normalization, no filesystem access moved or added. Enables a future consumer (quick-batch, #3675 / ADR-1239) to reuse the same primitive without pulling in orchestration internals. * docs(#3674): register the file-overlap-partitioner module bookkeeping New src/*.cts -> bin/lib/*.cjs modules need four hand-maintained registrations beyond the code itself: .gitignore (compiled artifact), eslint.config.mjs (ADR-457: lint the .cts, not the emitted .cjs), docs/INVENTORY.md's CLI Modules roster row (regenerated via gen-inventory-manifest.cjs --write), and a CONTEXT.md glossary entry matching the convention set by similarly-scoped leaf modules (text-lines.cts, plan-dependency-graph.cts, spec-section.cts). * fix(#3674): alphabetize INVENTORY.md row, manifest regen no-op, fast-check import already correct - docs/INVENTORY.md: move file-overlap-partitioner.cjs row to alphabetical position - docs/INVENTORY-MANIFEST.json: regenerated via gen-inventory-manifest.cjs --write, produced no diff (manifest is keyed by content, not row order) - tests/claude-orchestration.test.cjs's direct require('fast-check') is correct as-is: tests/helpers/fast-check-setup.cjs's own docstring scopes the shared-seed wrapper to "every *.property.test.cjs file"; claude-orchestration.test.cjs is not a .property.test.cjs file, and every .property.test.cjs file sampled uses the wrapper consistently. No outlier. * fix(#3674): constrain the no-overlap property test to unique ids, fixing an ambiguous duplicate-id reconstruction The `no two plans in the same stage share a modified file` property reconstructs which physical item produced each output id via `remaining.findIndex(r => r.id === id)`. Under duplicate ids (an explicitly-supported input shape for `partitionByFileOverlap`) that reconstruction can pick the wrong physical occurrence, producing a false-positive overlap failure (observed counterexample: p0(f1), p208(f1), p208([]) — correctly staged as [[p0,p208#2],[p208#1]], but misread by id-order as [[p0,p208#1],...], which do overlap). Properties (a) determinism and (b) totality already exercise duplicate ids correctly and are left unchanged; only this property's generated items are now constrained to unique ids via `fc.uniqueArray`, where the reconstruction is unambiguous. --------- 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> |
||
|
|
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 @
|
||
|
|
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 ( |
||
|
|
8487f0ed42 |
enhance(#3552): warn on additional protected branches beyond the resolved base branch (#3648)
* test(01-01): add failing protected-branch warning coverage - pin configured, absent, and malformed branch-list behavior - require opposite CLI and execute warning outcomes * feat(01-01): warn on configured protected branches - resolve the base branch union configured protected branch names - expose exact boolean CLI comparison output for workflow callers - keep execute-phase warning advisory and within its byte budget * test(01-01): add failing protected branch config coverage - cover valid list persistence and null unset - reject hostile shapes while preserving the prior value * feat(01-01): validate protected branch configuration - register git.protected_branches as a canonical config key - require a non-empty array of non-blank branch names * test(01-02): add failing ship protected-branch controls - Execute both workflow warning blocks with exact predicate arguments - Require true and false results to produce opposite warning outcomes - Preserve the none-strategy feature-branch offer contract * feat(01-02): warn at ship on protected branches - Reuse the typed protected-branch predicate in ship preflight - Keep raw base resolution for PR targeting and advisory branch creation - Prove execute and ship warning blocks with opposite-result controls * test(01-02): add failing protected-branch docs parity - Require the canonical schema key in both English config references - Pin the non-empty string-array type and absent default - Require synchronized multi-branch examples and advisory semantics * feat(01-02): publish protected branch configuration contract - Document the optional non-empty string-array field in both references - Explain resolved-base union and absent-field compatibility - Keep execute and ship warnings advisory under branching_strategy none * fix(01): CR-01 honor active workstream branch policy * fix(01): WR-01 assert protected config path selection * docs: add changeset fragment for #3648 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CteVPJt4BkPmroMPGajYx * fix(#3648): resolve base_branch precedence inversion and round-1 findings Blocker 1/2: production config resolution was flat-first, so a project that migrated to git.base_branch but still carried a stale flat base_branch got the old value back. Add base_branch to normalizeLegacyKeys (mirrors the existing branching_strategy/sub_repos pattern: canonical nested wins) and route readEffectiveGitConfig's test seam through the same normalization so it can't silently diverge from production again. Adds a regression test with both keys set that fails without the fix. Blocker 3/4/5: restore the handle_branching case-selector prose and "none" contract sentence that #3389's tests anchor on, and revert the unrelated prose/comment compaction in the same step — both were drive-by edits outside #3552's scope. Also addresses review majors/minors: delete readConfigBaseBranch and readConfigProtectedBranches (dead in production, only self-tested); --is-protected now fails closed (reports protected) instead of silently answering false when the base branch can't be verified; trim configured protected-branch names; fix HOME-without-USERPROFILE vacuous isolation on Windows; correct the drift-ack's byte accounting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S44stkuQbhD3jTCtKzte5N * test(#3648): add failing legacy-key hoist safety coverage Round-2 review found normalizeLegacyKeys block 5 records a normalization carrying the DISCARDED flat value on the canonical-wins branch. Probing that turned up a second, unreported defect in the same helper shape: blocks 1, 2 and 5 all spread result['git'] / result['planning'] with no object guard, so a config whose section key holds a string is spread into index keys — {"git":"main","base_branch":"release"} -> {"git":{"0":"m","1":"a","2":"i","3":"n","base_branch":"release"}} The resolved value is accidentally still correct, so nothing fails and no diagnostic fires. But normalizations.length > 0 sets configDirty, and config-loader then serializes that shape back into the user's config.json — a read that silently corrupts config. The deleted #3057 W3 suite covered {"git":"main","base_branch":"release"} explicitly; this is the input it would have caught. Covers both defects across blocks 1 and 5, with object/array/null negative controls that must stay green in both phases, and a fast-check property over arbitrary `git` values. * test(#3648): pin fail-closed handling of malformed protected_branches Replaces the test that pinned the fail-OPEN behaviour. The old assertion — ['develop', 42] yields isProtected === false for 'develop' — locked in the exact failure #3552 exists to close: config-set validation is bypassable by a direct edit of .planning/config.json, so a user who believes 'develop' is protected got a silent false and no warning. It was also inconsistent with the fail-CLOSED direction twelve lines away, where an unverified base reports protected and writes a diagnostic. A protection predicate must not have two opposite failure directions depending on which input is bad (#3648 review Blocker 3). New coverage: a bad element drops only itself, a non-array contributes no names, an empty list is well-formed rather than malformed, and --is-protected surfaces the rejection. Both negative controls — a clean list reports nothing rejected and writes no diagnostic — must stay green in either phase, so the reject channel cannot fire unconditionally. * fix(#3648): drop only invalid protected_branches and report them Partition git.protected_branches instead of discarding the whole list on one bad element, and carry the rejections out through ProtectedBranchStatus so --is-protected can name them on stderr. Valid names keep protecting; the user finds out the rest were ignored. A non-array value still contributes no names — a bare string is not a list of branch names — but is now reported rather than swallowed. An empty array stays silent: declaring no extra protected branches is a valid choice, not a misconfiguration. writeDiagnostic is hoisted out of the unverified-base branch since both arms now use it. * test(#3648): prove the predicate diagnostic survives both call sites The workflow bash stub now emits a stderr diagnostic the way the real command does, which is what makes a swallowed `2>/dev/null` visible to a test — previously the stub was silent on stderr, so discarding it changed no observable behaviour and the call sites could drop the explanation undetected. Adds the Minor 2 binding check as well: ship must expose the predicate result as IS_PROTECTED rather than only echoing a warning, asserted by running the extracted bash and reading the bound value, not by grepping the workflow source. Both tests carry opposite-outcome controls — an empty diagnostic must leave the text absent, and a false predicate must bind false. * fix(#3648): surface the predicate diagnostic and bind ship's result Drop `2>/dev/null` from the --is-protected call at both call sites. The fail-closed explanation and the new rejected-entry warning both go to stderr, so discarding it left the user with a bare "protected branch" warning on a branch that is not protected and no way to tell a real match from a degraded-git guess. `git branch --show-current` keeps its own redirect — that one is genuine noise. ship.md binds IS_PROTECTED and its prose now branches on the variable, so the following steps have evaluable state instead of having to infer it from warning text in tool output. execute-phase.md byte accounting refreshed: 92326 -> 92645, net growth 319 bytes (was 331 before the redirect came out). Baseline re-verified against the current rebase base by blob id; the ceiling check passes with 755 bytes of margin. * test(#3648): restore negative space for the readFile config seam The #3057 W3 suite was deleted with readConfigBaseBranch, but every arm it pinned survives verbatim in readEffectiveGitConfig's readFile branch — the JSON.parse catch, the non-object guard, the git-section object guard, .trim() and blank-string rejection — and the four surviving readFile injections were positive-path only. protected_branches was never driven through this seam at all. Restores nine cases against the seam, including protected_branches partitioning, plus a control proving loadConfig still wins when both seams are supplied. Records honestly what the suite pins. Mutating the built lib shows .trim() is KILLED, while the non-object guard and the blank-string rejection SURVIVE — both are unreachable through this entry point for the same reasons the deleted suite documented against its own equivalents: a JSON-parsed non-object carries no relevant own-property either way, and a blank value is rejected a second time downstream by the resolver's truthiness check. They stay as defence-in-depth and are labelled known-unkillable rather than left looking like coverage this suite does not provide. * test(#3648): distinguish detached HEAD from a missing branch argument `args[1] ?? ''` collapsed two different situations into one: a detached HEAD, where `git branch --show-current` legitimately prints nothing, and the flag being called with no argument at all. Both answered false, so the right outcome arrived by an unintentional path and a caller bug was indistinguishable from normal operation. Asserts the detached case stays silent and the missing-argument case reports, with a control that the two diagnostics differ. * fix(#3648): report a missing --is-protected branch argument Answer false either way, but say so when the flag arrives with no argument. A detached HEAD passes an explicit empty string and stays silent, since that is a normal state rather than a misconfiguration. * docs(#3648): state exact-name matching and per-entry rejection isProtected is exact string equality, so a git-flow project must enumerate every release/* and hotfix/* by name. #3552 only asked for an integration-branch field, so the implementation satisfies the letter of the issue while leaving its git-flow motivation partly unserved — say so where users will meet it rather than leaving them to discover it. Also documents the Blocker 3 behaviour change: an invalid entry is ignored with a warning naming it and the remaining names still apply. Both statements land in docs/CONFIGURATION.md and gsd-core/references/planning-config.md, and the config-field-docs parity test asserts each in both so the two cannot drift. * refactor(#3648): extract isValidProtectedBranches for cross-surface pinning The `git.protected_branches` check inside `cmdConfigSet` and the resolver's per-entry filter in `git-base-branch.cts` are deliberately different shapes — all-or-nothing on write, per-entry on read, so a hand-edited config.json cannot fail the guard open. Nothing structural keeps their two definitions of "usable branch name" in step. Lifting the write-side check into a named, exported predicate lets a property test ask both surfaces about the same value and assert they agree, which is the fast-check gap the round-2 review flagged. No behaviour change: the predicate is the same expression, called from the same place. * fix(#3648): stop --is-protected rewriting the config it is asking about `gsd_run query git.base-branch --is-protected` runs on every execute-phase and every ship. It resolved config through `loadConfig`, whose normalize-then-write path rewrites `.planning/config.json` whenever any legacy key normalizes — so a boolean question was silently editing the user's checked-in config. This PR had widened the trigger by adding a fifth normalization block (top-level `base_branch` -> `git.base_branch`), making it fire for exactly the projects the feature targets. `loadConfigResolved` gains `options.persist` (opt-OUT, default true): resolution is unchanged, only the two write-back side effects are suppressed. The predicate passes `persist: false`; the ~30 other callers are untouched, so a legacy config is still migrated by ordinary use. Asserted on BYTES rather than parsed shape, because the rewrite reorders keys and reflows whitespace even when the values are equivalent. Three tests, each with its own control: the end-to-end CLI leaves the file byte-identical while still answering `true` from the legacy key (proving the config WAS read); an ordinary persisting load of the same fixture DOES change the bytes (proving the fixture is live rather than inert); and `persist:false` vs default over one directory returns deep-equal config while differing on the write. Reverting the one-line `persist: false` fails the first of those and only that one. Also from the review: - `readEffectiveGitConfig`'s comment claimed the readFile branch routed "through the same precedence authority production uses". It does not, and cannot — it reproduces two of production's steps over a single file. The comment now names what the seam covers and what it does NOT (root/workstream deep merge, builtin and global defaults, federated merge), and the seam now applies production's flat-then-nested lookup so it stops disagreeing about a surviving flat key. - The missing-argument diagnostic promised "answering false", which the fail-closed guard on the same call can contradict by printing `true`. It now states what it did with the argument and leaves the answer to stdout. * test(#3648): re-pin block 5 on #3760's refusal contract #3767 landed on next while this PR was in review and fixed the non-object config-section defect properly: a present-but-non-object section now BLOCKS its own migration — value preserved, no Normalization pushed, refusal reported via `skipped[]` — rather than being rebuilt from a plain-object view. That supersedes this branch's round-2 `hoistLegacyKey`, which prevented the character-key spread but still dropped the section value silently, and which the round-3 review correctly called out as destruction in place of corruption. The rebase drops that commit and routes block 5 through the upstream helper. This file's tests asserted the superseded design, so they are rewritten to pin block 5 — `base_branch` -> `git.base_branch`, which did not exist when #3760's suite was written — against the contract that now governs it: ordinary hoist into an absent/null/object section, canonical-nested-wins, and refusal for each of string/number/boolean/array sections with the exact `skipped` entry. Two controls keep it from passing vacuously: the refusal must be scoped to block 5 (an unrelated block still normalizes in the same call), and a property over arbitrary `git` values asserts hoist and refusal are exhaustive AND mutually exclusive per key, that a refusal leaves both the section and the legacy key untouched, and that a hoist manufactures no index key the input did not carry. * docs(#3648): correct the Git Query and Config Loader module contracts CONTEXT.md's Git Query Module still described base-branch tier 1 as a direct `.planning/config.json` read. Since this PR it is the EFFECTIVE configuration resolved by the Config Loader — a materially different authority, carrying the root/workstream deep merge, flat-then-nested lookup and builtin/federated defaults. The `--is-protected` predicate, `git.protected_branches`, and the two invariants that distinguish the predicate from the plain query (fails closed on an unverified base; must not write) were undocumented entirely. The Config Loader entry now states that loading is not side-effect-free by default and documents `options.persist`. docs/INVENTORY.md's `git-base-branch.cjs` row carried the same stale ladder and no mention of the predicate. `node scripts/gen-inventory-manifest.cjs --write` was run and produced no diff: the manifest indexes roster NAMES, not row prose, so a description edit cannot move it. Also closes the global-defaults minor: `git.protected_branches` is inert in `~/.gsd/defaults.json`, but so is every other `git.*` key — no branch-policy key appears in `_globalBaseCfg` or `GLOBAL_DEFAULTS_RESOLUTION_KEYS`. That is section-wide and predates this PR, so the fix is to state the scope where users meet it rather than to quietly extend the resolution set for two new keys. * fix(#3648): close four defects found by the round-4 external review Two external reviewers (codex, antigravity/Gemini 3.1 Pro) were run adversarially against this branch. Four findings reproduced against source; each is fixed with a failing-first test and a control, and each fix was verified by reverting it and watching exactly the intended test fail. 1. `persist:false` was DROPPED by the workstream fallback (codex). Blocker 1 was only half closed. `loadConfigResolved` re-enters itself with a bare `{ workstream: null }` when a workstream has no config.json of its own, and that literal discarded every other option — so the recursive pass ran at the DEFAULT persistence and rewrote the ROOT config. Reproduced: with GSD_WORKSTREAM=alpha and a legacy flat `base_branch`, `--is-protected` rewrote `.planning/config.json` despite `persist:false`. Both recursions now forward `options` and override only `workstream`; the explicit override still wins the hasOwnProperty check, so spreading cannot let `workstreamContext` reintroduce a workstream. 2. Both workflow call sites failed OPEN, and aborted under `set -e` (both reviewers, independently). `IS_PROTECTED=$(gsd_run ...)` yields an empty string when the query fails, so `[ "$X" = true ]` was simply false: no warning, no trace — a silent hole in the guard whose only job is to warn. The bare assignment also aborted the step under `set -e`. Both sites now degrade VISIBLY: `|| IS_PROTECTED=""`, then an explicit empty-string arm that says the check did not run. Deliberately not fail-closed — claiming "protected" on no evidence would warn on every branch whenever gsd-tools is unavailable. 3. `isValidProtectedBranches` and the resolver disagreed on a sparse array (antigravity). `.every()` skips holes; the resolver's `for...of` yields `undefined` for them, so `["main", , "develop"]` was accepted by config-set and rejected by the resolver. The cross-surface property passed only because `fc.array` cannot generate a hole. The predicate now indexes, and the generator punches holes so that axis is actually falsifiable. JSON cannot express a hole, so this is unreachable in production — but two definitions of one predicate must not contradict each other. 4. A top-level `protected_branches` silently outranked `git.protected_branches` (antigravity). Routing the key through `get(key, {section, field})` gave it flat-then-nested precedence, which is back-compat for keys `normalizeLegacyKeys` migrates. `protected_branches` is new in #3552 and has no legacy form, so that invented an undocumented alias. It now resolves nested-only through a new `getNested`, in production and in the test seam. `base_branch` keeps flat-then-nested — it HAS a legacy spelling that #3760's refusal path can leave behind — and a control pins that distinction. Also narrows a CONTEXT.md claim this round introduced. The predicate fails closed only when a git query TIMED OUT or could not be spawned (#3057 B4's `verified`); a git command that runs and exits non-zero counts as a clean negative, so a cwd that is not a repository answers `false`, not `true`. Verified pre-existing on next @ |
||
|
|
dd4f179672 |
feat(#3970): per-task external-tracker content-resolution seam (#4000)
* feat(#3970): per-task external-tracker content-resolution seam Implements ADR-3646 (Phase 1, #3970): a `<task tracker-id="...">` attribute plus a new optional `taskContentResolver` capability-manifest field let a capability resolve a task's action/verify/acceptance-criteria/read_first/done content from an external issue tracker instead of PLAN.md's inline body. - src/plan-document.cts: parses the `tracker-id` attribute into `PlanTask.trackerId` - src/task-content-resolution.cts: new leaf module — split/find/build/resolve, with a hard-halt (throw) contract on ambiguous/failed/timeout/malformed resolution, never a silent fallback to possibly-stale inline text - src/task-command-router.cts: new `task resolve-content --plan --task-id --raw` CLI verb wiring the module into a real process exit code - gsd-core/bin/lib/capability-validator.cjs: validates the new `taskContentResolver` manifest field (feature-role only, cross-capability trackerPrefix uniqueness) - gsd-core/workflows/execute-plan.md, gsd-core/references/loop-hook-dispatch.md, docs/reference/capability-manifest.md: wire the seam into the per-task loop and document it as a new `execute:task` point outside the existing contribution/step/gate vocabulary (unconditional in autonomous mode) Closes #3970 * fix(#3970): gate checkpoint tasks out of content resolution, close trackerPrefix grammar parity gap, cover path-traversal guard Standards/Spec code-review pass on the task-content-resolution seam (ADR-3646 Phase 1) found three defects: 1. execute-plan.md's task-content-resolution bullet fired on any tracker-id-bearing task with no check that it wasn't type="checkpoint:*", contradicting ADR-3646 Decision 1 (a checkpoint task must never enter resolve-content). plan-document.cts already parses trackerId: null unconditionally for checkpoint tasks; only the workflow prose needed the fix, so the bullet now explicitly excludes checkpoint tasks. 2. task-content-resolution.cts's parseResolverDeclaration accepted any non-empty trackerPrefix with no grammar check, while capability- validator.cjs's KEBAB_RE enforces kebab-case at install time — a Generative Fix Divergence gap. Added the same grammar (as a literal regex, documented as intentionally not shared across the .cts/.cjs build boundary) plus a parity test asserting the two surfaces agree across a valid/invalid trackerPrefix table. 3. task-command-router.cts's routeResolveContent path-traversal guard on --plan had zero test coverage. Added a test exercising a ../../../etc/passwit-shaped path and asserting the USAGE rejection names the offending path. * fix(#3970): sanitize resolver diagnostics and cap resolver timeoutMs Two findings caught by an isolated security-review pass on the task content resolution seam: - ResolverFailedError/ResolverMalformedOutputError embedded raw, unsanitized subprocess stderr/stdout (attacker/model-influenced via the tracker-id argv token) into .message. A hostile or buggy resolver could smuggle a newline plus a forged "Error: " line, or terminal escape sequences, into a diagnostic io.cjs's error() writes verbatim to stderr. Fixed at the constructor (task-content-resolution.cts) via io.cjs's existing formatDiagnosticToken(), so every caller of resolveTaskContent gets a safe .message by construction. - capability-validator.cjs's validateTaskContentResolverFields had no upper bound on taskContentResolver.invoke.timeoutMs, letting a manifest declare an effectively unbounded value and defeat the "bounded subprocess" design intent. Added a 120000ms ceiling specific to this field, without touching the shared isPositiveIntegerMs() helper (still used unbounded by the reviewer lane's timeoutFloorMs and probe timeoutMs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3970): fix gsd-test failures — stale prose allowlist line and stderr-vs-message assertion gsd-test (remote dockerized matrix) came back red with 5 failures on this PR; all five are real defects, fixed here. - tests/no-bare-gsd-tools-command-position.test.cjs: PROSE_ALLOWLIST's execute-plan.md entry pointed at line 415, which ffc190df4's checkpoint-exclusion caveat (added near line 221) shifted down by one line. The actual "validated downstream by gsd-tools uat classify-coverage" descriptive mention now sits at line 416. Updated the allowlist entry's line number to match. - tests/task-command-router-resolve-content.test.cjs: the path-traversal test asserted the outside-project-scope diagnostic against the thrown ExitError's own .message. io.cts's error() (ADR-3889) writes its human-readable message to fd 2 via writeAllSync and then throws a bare `new ExitError(1)` with no message argument — by design, so the exception carries no duplicate text and the thrown ExitError's message defaults to "process exit 1" (cli-exit.cts's ExitError constructor). Root cause was the test, not the source: task-command-router.cjs's outside-project-scope rejection already calls error() correctly and the diagnostic text is genuinely emitted, just on fd 2, not on the exception. Fixed the test to capture fd-2 writes (mirroring tests/estimate-calibrate.test.cjs's runCalibrateExpectError and this same file's own captureStdout for fd 1) and assert against the captured stderr text instead of err.message. This was masked locally because a manual `node -e` sanity check that only inspects the caught exception's .message cannot see what the real node:test run actually failed on. Emitted-Drift-Ack-Growth: execute-plan.md — adds the ADR-3646 task-content-resolution bullet and checkpoint-exclusion caveat to the per-task execute loop; a real behavioral prose addition, not incidental bloat. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3970): backfill changeset PR number (pr:0 -> pr:4000) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
|
3a6c0412a9 |
enhance(#3624): local/no-exact-case-env-access — ratchet ADR-1703 onto production env reads (epic #3411 Phase 4) (#3976)
* enhance(#3624): local/no-exact-case-env-access — ratchet ADR-1703 onto production env reads (epic #3411 Phase 4) Extends ADR-1703's portability rule catalog with a second production-runtime rule: it flags an exact-case read of a Windows case-varying environment variable (PATH, PATHEXT, ComSpec, USERPROFILE, TEMP, TMP, APPDATA) off any receiver that is not process.env itself, matched via an env-shaped-receiver check to avoid colliding with ordinary `.path`-named properties elsewhere in the tree. Exports the seam's private `_envGet` as `envGet` so the rule's remediation message names a real helper, and fixes the one pre-existing violation the tightened rule found (`src/runtime-hooks-surface.cts`'s `env.APPDATA` read). Closes #3624 * fix(#3624): extractStaticName recognizes non-computed Literal destructuring keys; add missing accessor-call test case Review findings from the code-review + isolated-adversarial passes: - extractStaticName only matched non-computed Identifier keys, so a destructuring like `const { 'PATH': v } = opts.env;` (the issue's own I8 acceptance case) silently evaded the rule. Widened to accept a Literal key regardless of computed, which is safe for MemberExpression too (its non-computed property is always an Identifier by grammar). - Added the missing RuleTester valid case for "a case-insensitive accessor call" (envGet(env, 'PATH')) from the issue's Done-when checklist. * docs: backfill changeset PR number for #3624 (PR #3976) --------- Co-authored-by: sim <sim@local> |
||
|
|
355c943b08 |
enhance(#3626): make CONTEXT.md seam claims checkable via a SEAM.*.enforced-by gate (#3975)
* feat(#3626): make CONTEXT.md seam claims checkable via SEAM.*.enforced-by gate Adds SEAM.<id>.owns / SEAM.<id>.enforced-by=lint-rule:<name>|test:<path> predicates to CONTEXT.md, generalizing the existing WORKTREE.SEAM.* shape, plus scripts/lint-seam-enforcement.cjs (wired into lint:ci) which fails when a declared single-owner seam names no existing, registered enforcement mechanism. Backs all six current module-level single-seam/ single-canonical-owner claims found in CONTEXT.md, including the Shell Command Projection Module's Windows-binary-resolution claim via #3619's local/no-private-binary-resolution rule. Scope is resolves-only per maintainer decision: the gate proves an enforcement pointer exists and is registered, not that its surface covers every file the seam claims. See docs/adr/3626-context-md-seam-claim-gate.md. Closes #3626 * fix(#3626): back the Package Identity Module's seam claim too Isolated adversarial review caught a miss in the "no grandfather list" sweep: the Package Identity Module also declares itself "Single seam owning GSD's published-package coordinates" and already names its real enforcement (scripts/lint-package-identity-drift.cjs). Backs it with SEAM.package-identity.owns/enforced-by=test:tests/package-identity.test.cjs, bringing the total to 7 backed seams. Also makes explicit, in the design doc and ADR, that function-level "single owner" sentences inside already-covered modules (STATE.md Document Module, etc.) are deliberately out of scope — a seam claim is about a module's boundary, not every function inside it. * docs(#3626): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
fa41bfec5c |
enhance(#3942): the emitted-drift ack is PR-lifetime data — move it to a commit trailer (#3954)
* test(#3942): failing-first suite for the emitted-drift ack commit trailer Binds 37 input classes from the phase test matrix to the behavior ADR-3942 specifies, before any of it exists. Stubs return benign empty values rather than throwing, deliberately: several rows assert that something DOES throw (cap overflow, uncomputable commit range), and a throwing stub would turn those green for the wrong reason and destroy the red. The two rows that carry the design's load: - merge-base semantics. The range is $(git merge-base base HEAD)..HEAD, not base..HEAD, because changedPaths comes from `git diff base...HEAD` (three dot). Two-dot would let the ack set and the change set disagree about which commits are this PR's. The fixture forks a topic branch, puts a trailer on each side, and asserts only the topic-side trailer is in range. - fail-closed on an uncomputable range. With fragments a depth-1 checkout passes VACUOUSLY, every fragment reading as brand-new. With trailers the range cannot be computed at all, and returning an empty set would silently disarm the gate, so it must throw. The fixture builds a genuine shallow clone rather than simulating one. Also covers the self-inflicted case: this change's own documentation quotes the trailer syntax, so an example landing at the end of a commit message would arm a live acknowledgment keyed on the literal placeholder text. Keys carrying angle brackets or whitespace are rejected. Authored per the phase artifacts 40-design.md and 50-test-matrix.md. Not yet run on the remote runner — this commit exists to be tested. Refs #3942 * chore(#3942): move the emitted-drift ack to a commit trailer Implements ADR-3942, superseding ADR-2719 section 3 and its #2789 amendment. Sections 1, 2 and 4-7 are retained: the conservation law is unchanged, only the storage of its escape hatch moved off the working tree. An acknowledgment explains one PR's ripple, and the moment that PR merges the ripple is in the base, so it can never clear anything again. It was stored in permanent shared state anyway, and every consequence of that mismatch had to be built and then maintained. The chain is #2789 -> #2914 -> #3078 -> #3842 -> #3823 -> #3875, each fix generating the next defect, ending in a scheduled sweeper whose own first PR could not merge itself. Added parseAckTrailers + renderAckTrailer (pure) and readAckTrailers (IO shell), reading Emitted-Drift-Ack-Hash: / Emitted-Drift-Ack-Growth: trailers over the merge-base range. tests/emitted-ack-trailer.test.cjs, 37 cases, written failing-first and confirmed red before any of this existed. Changed diffEmitted takes two structurally distinct key-space maps instead of one shared paths map. That closes a latent defect: the spaces were separated by convention only, so a growth key satisfied a hash lookup by naming coincidence. staleAcks now reports which space a key was declared in. REMEDIATION teaches the trailer, per space, with its example rendered through renderAckTrailer so the taught grammar cannot drift from what the parser accepts. Removed the sweep workflow, the guard-no-ack-on-next job, the standalone linter and its lint:ci entry, the fragment directory and its three spent fragments, the legacy single-file union, and the baseAck/spentAcks mechanism -- spentness is now structural, not computed. Two range properties carry the design and are pinned by tests rather than asserted: the range is merge-base scoped, matching git diff base...HEAD, so an already-merged trailer is out of range by construction; and an uncomputable range throws instead of reading as zero acknowledgments, which is the inverse of the fragment guard's vacuous pass. Three deliberate observable changes, each disclosed in the changeset: the unread runtime field is gone, the legacy file is no longer read, and cross-space excusal no longer works. Ten open PRs carry fragments and will meet a modify/delete conflict. Measured before landing and accepted deliberately; the one-line migration is in the PR body. Verified: lint:ci exit 0. Remote runner to follow on this exact sha. Refs #3942 * fix(#3942): silent trailer collapse, lost coverage, and an unbounded cap Six findings from the orthogonal review round, all fixed in place. BLOCKER -- two trailers of the same name on one commit collapsed silently. readAckTrailers built `separator=1d` where git needs `separator=%x1d`: the `separator=` value inside a %(trailers:...) placeholder is itself a pretty-format string, so the bare hex was emitted as two literal characters and the split on \x1d never matched. Two same-name trailers therefore joined into one value with errors empty -- the first reason absorbing the second entry's key. Silent truncation, the exact class MAX_ACK_TRAILERS throws to prevent. Confirmed with od -c against real git output before and after. The failing-first matrix did not catch it because its "both spaces coexist" row uses Hash plus Growth -- different trailer NAMES -- so the value separator was never exercised. Two regression tests now cover same-name trailers directly. Coverage recovered: normalizeAckReason and INVISIBLE stayed on the live path via parseAckTrailers but lost every test when the old suite was pruned. Back under test against the current surface -- all six invisible codepoints individually, whitespace collapse, trim, CRLF, and two seeded fast-check properties. Dropping any single codepoint now fails. MAX_ACK_TRAILERS counted raw trailers before de-duplication, so one trailer carried forward across rebased commits counted once per commit and could throw on a legitimate branch. Now counts distinct entries; 100 identical repeats dedupe to one. diffEmitted validated baseline, current and changedPaths but not the new ackHash/ackGrowth, so a bad shape raised an unhandled TypeError instead of an error verdict -- the same defect shape this file documents for #2778. Docs: CONTRIBUTING and TESTING-SUITES were rewritten only in their first sections; the later passages still taught fragments, git rm and the deleted guard, contradicting the new text directly above them. Finished. Also extends lint-removed-but-needed to exempt docs/adr and docs/research. That gate fails on any docs mention of a file deleted in the same diff, which makes it impossible to document a deletion in the PR performing it -- an ADR's whole job is naming what it retired. Exemption is narrow and comes with a test proving the gate still fires for a live consumer elsewhere under docs/. A guard that cannot fail is worse than no guard. Maintainer-approved. CONTEXT.md names the retired machinery by role rather than by filename: its generated projection lands in docs/, which that gate does scan. Adds docs/how-to/acknowledge-emitted-drift.md. The required docs set is Reference and Explanation, so the task quadrant can be empty with every gate green -- and this change has a real multi-step journey, including the fragment migration ten open PRs now need. lint:ci exit 0. Refs #3942 * docs(#3942): correct the duplicate-trailer rule in CONTRIBUTING Both axes of the code review independently flagged the same passage, without seeing each other's output. It claimed two declarations of the same key are always "a hard, loudly-reported error, not a silent last-wins". That is only half true, and the missing half is the one contributors hit: identical declarations -- same key, same reason -- dedupe silently, because a trailer legitimately survives a rebase and reappears on every rebased commit. Failing there would red a branch for doing nothing wrong, which is exactly why the dedup exists. Only a same-key/different-reason pair errors, and that one is a genuine ambiguity about which explanation holds. As written, the paragraph told a contributor that a rebase-carried trailer breaks the gate -- the opposite of the behavior. CONTEXT.md's parallel entry already stated it correctly; this brings CONTRIBUTING into line. Doc-only, root-level markdown. Refs #3942 * chore(#3942): backfill changeset PR number to 3954 --------- Co-authored-by: sim <sim@local> |
||
|
|
39673ae9ff |
fix(#3738): antigravity global skills/agents install to ~/.gemini/config (#3921)
* test(#3738): antigravity global skills/agents must resolve under ~/.gemini/config Regression tests (RED first): --skills-root and gsd-tools query surfaces, install-plan dest dirs, and converter skills-path rewrite. * fix(#3738): antigravity global skills/agents install to ~/.gemini/config Antigravity's machine-local discovery scans ~/.gemini/config/{skills,agents}; the configHome (~/.gemini/antigravity) is deprecated for artifacts. Declare the ADR-1239 skills/agents 'home' override on the antigravity global layout — the same mechanism codex uses (.agents) — and divert ~/.claude/skills/ references in converted global content to ~/.gemini/config/skills/. configHome, settings, probe/migration semantics, and the local .agents layout are unchanged. * fix(#3738): retire deprecated configHome artifacts via installer migration 010 Next install converges an existing antigravity install: manifest-managed skills/gsd-*/ and agents/gsd-*.md under the configHome (a location AGY does not scan) are removed — modified files backed up first, unmanifested and non-gsd entries preserved — and now-empty containers retired. Global scope only; the local .agents surface is live. Docs + inventory updated. * fix(#3738): converter sync in bin/install.js, harness emit-root coverage, migration baseline - bin/install.js converter gains the same ~/.claude/skills → ~/.gemini/config/ rewrite as src (ADR-1508 dual copy must stay in sync). - Parity-manifest walk covers home-override emit roots (extraEmitRootsFor) so antigravity's emitted skills/agents stay differential-visible at their new install root; install-tree fixture regen confirms an unchanged key set. - skills-from-commands rule declares the antigravity converter as a runtime-scoped transform; one ack fragment covers the identity-classed workflow whose antigravity copy embeds the old skills path. - Migration 010 checksum baseline + home-override set doc updated; existing tests updated to the #3738 contract (global dest, golden parity via layout dest, integration expectations). * fix(#3738): tolerate an absent extra emit root on baseline-side measurement The base tree's installer predates the home override, so <HOME>/.gemini/config does not exist there; walk() threw ENOENT and the in-job baseline build failed. An absent extra root is the legitimate pre-override shape — skip it. * fix(#3738): review findings — manifest agents root, bare skills-path rewrite, guard comment - writeManifest resolves the agents-kind home override (_kindDestDirSafe), so the manifest records agents at their actual install root and drift detection keeps working (isolated review finding 1, major). - Converter bare forms ~/.claude/skills and $HOME/.claude/skills (no trailing slash) divert to ~/.gemini/config/skills instead of falling through to the retired configHome path (finding 2). - real-home-guard comment updated: antigravity's global agents kind is the first agents-kind home override (finding 3, doc-only). - Regression tests for both behavioral findings. * chore(#3738): changeset fragment (pr number backfilled after PR creation) * chore(#3738): backfill changeset PR number (3921) * fix(#3738): sandbox HOME in tests that install antigravity global artifacts antigravity is the first home-override runtime in the golden-parity and skills-wrapper suites (codex is not in their runtime lists), so those tests never needed HOME sandboxing — the real-home guard now (correctly) refuses their un-sandboxed global installs on CI, where HOME is the passwd home. * fix(#3738): stop the K3 sequential-sandbox env leak; sandbox L2's home-override plans K3's two back-to-back sandboxHome calls leave HOME pointing at the first sandbox once the after-hooks restore (each call saves the env as it found it, so the second saves the first's sandbox as 'original'). On the windows matrix that leaked gsd-k3-qwen-* home into the L2 property, whose antigravity/global run then (correctly) refused via the #3712 real-home guard — antigravity is the runtime that made L2's plan escape into os.homedir(). K3 now manages the env with a single restore; L2 sandboxes HOME per run, mirroring L1. * fix(#3738): L2 property's HOME sandbox must exist on disk The #3712 guard's sandbox exemption fails closed when identify(effectiveHome) is 'absent' — L2 never created its configDir, so on the windows matrix (tmpdir under the real home) the antigravity/global run refused even with HOME sandboxed. Create the per-run sandbox dir and clean it up. --------- Co-authored-by: sim <sim@local> |
||
|
|
e20744eacb |
enhance(#3884): failure is a value — strict argv, and --pick that signals absence (#3922)
* test(#3884): failing-first coverage for strict argv and absence-signalling --pick ADR-3473 §8.4 says failure is a value. Three families currently encode failure as success, and this commit pins each one RED before the fix lands. Measured on this tree, 2026-08-26: gsd-tools generate-slug "test" --pick nonexistent -> empty stdout, exit 0 (#3365) gsd-tools audit-open --pick nonexistent_field -> dumps the entire human-readable audit report, exit 0 gsd-tools generate-slug "Hello World" --raw --pick bogus -> prints "hello-world", another field's value, exit 0 gsd-tools query state.planned-phase 3 (positional, no --phase) -> exit 0; STATE.md's "Phase: 2 of 5 (Widget Support)" is overwritten to "Phase: null - READY TO EXECUTE" and the frontmatter gains a corrupted current_phase_name (#3358) tests/pick-flag.test.cjs:27 previously asserted the #3365 defect as the contract ("returns empty string for missing field", success === true). That assertion is replaced by the required behavior rather than deleted. The new parseNamedArgs block calls the spec-object signature that does not exist yet, so it fails today by construction. The 11 existing behavior-lock tests are left untouched here; they are corrected in the implementation commit. C1/C4 assert at the consumer's output - STATE.md's bytes - per ADR-3180 Decision 4(b). A unit assertion on the parser would have passed throughout this defect's life. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3884): failure is a value — strict argv, and --pick that signals absence Implements ADR-3473 §8.4. Absence, emptiness and failure stop being interchangeable ways to say "I could not answer". parseNamedArgs (src/command-arg-projection.cts) Takes a spec object with a REQUIRED `positionals: number | 'rest'` and returns the hub's Result shape instead of a bare Record. Declaring the positional arity is what makes #3358's call site unrepresentable rather than merely detectable: an unrecognized flag or a token past the declared boundary is now InvalidArgs, naming the offending token and listing the accepted flags. The legacy positional-array call shape throws a TypeError — an internal invariant violation per ADR-3473 Decision 2, so a stale hand-written .cjs call site fails loudly instead of destructuring undefined off a Result. parseNamedArgsOrExit projects a failure onto the caller's error(); it is a projection over the one parser, not a second parser. Measured before, against a STATE.md with a populated phase-2 block: query state.planned-phase 3 (positional, no --phase) -> exit 0; "Phase: 2 of 5 (Widget Support)" overwritten to "Phase: null - READY TO EXECUTE", frontmatter gains a corrupted current_phase_name After: exit 1, `unexpected positional argument "3"`, STATE.md byte-identical. The flag form is unchanged and still updates STATE.md. --pick <field> (gsd-core/bin/gsd-tools.cjs) extractField returns {found,value}, and the pick block no longer shares one catch between "output was not JSON" and "field was absent". An absent field exits 1 with pick_field_absent, naming the field and the keys that do exist; non-JSON output exits 1 with pick_output_not_json instead of dumping the command's entire output. A field that is PRESENT with value null, '', 0 or false still prints at exit 0 — that is an answer, not a failure, and it is what keeps `--pick count` printing 0 on a fresh project. Measured before: `audit-open --pick nonexistent_field` printed the whole human-readable audit report at exit 0, and `generate-slug X --raw --pick bogus` printed "hello-world" — a different field's value, confidently, at exit 0. ADR-3409 Decision 7 explicitly deferred this contract fix to #3473; this is it. The sub-issue's "returns 0 when the count is zero OR absent" wording is superseded by the ADR rule it implements: zero prints 0, absence exits non-zero. Defaulting absence to 0 would demote "could not answer" to "the answer is zero" — the hazard docs/how-to/resolve-unreachable-guard-findings.md already warns against. Guard ledger (ADR-3473 Decision 6) scripts/lint-unreachable-guard-drift.cjs Detector A is RETIRED. Its premise — that a `--pick ... || echo` arm can never fire — is now false, so the shape it forbade is the correct idiom and keeping it would forbid the fix. Detector B (glob-consuming cat/ls, a nullglob mechanism this change does not touch) is retained in full, as are the shared scanner, the escape-marker parser and the baseline. Net: -1 detector, 0 added. The file is not deleted. Call-site audit 45 prompt-layer --pick invocations, every one a plain X=$(...) assignment — none in an if test, && chain, or a pipeline whose status is consumed, and no shell block in workflows/commands/agents/references sets -e. Of the 13 (command, field) pairs the prompt layer reads, 10 are always present; the 3 sometimes-absent ones each sit behind a prior found/existence check. No ADR-3409-class "field the command never produces" remains. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): escape untrusted tokens in diagnostics, and cover five unpinned rows Two review findings, both fixed here rather than recorded as limits. 1. A newline in an untrusted token forged a second stderr line. Before, plain-text mode: $ gsd-tools query state.planned-phase $'foo\nError: forged second line' Error: unexpected positional argument "foo Error: forged second line" After: Error: unexpected positional argument "foo\nError: forged second line" --json-errors mode was never affected — io.error runs that payload through JSON.stringify. Plain-text mode writes 'Error: ' + message verbatim, and the three new InvalidArgs reasons plus the two new --pick diagnostics all interpolate a token that comes straight from argv. Fixed with ONE shared helper, formatDiagnosticToken (src/io.cts), applied at every interpolation site — not a copy per site. It is deliberately NOT applied inside error() itself: several callers in this tree emit intentional multi-line diagnostics, and escaping newlines there would mangle them. The available-top-level-keys list needed the same treatment for a reason the review did not anticipate: `frontmatter get <file>` reads an ARBITRARY user document and echoes that document's own keys into the diagnostic. Verified reachable — a frontmatter key containing a newline reaches the key list — so formatKeyForDiagnosticList is guarding a live path, not a hypothetical one. Ordinary keys still render plain and unquoted; a fix that merely dropped the key would also have passed a "one line" assertion, so the test pins the escaped key's presence too. 2. Five behavior-table rows were implemented but nothing pinned them: B7 a dotted path that dies partway B9 bracket syntax on a non-array B10 a negative array index, in and out of range B14 a JSON root that is not an object B17 an @file: payload over 50KB B17 is the load-bearing one. output() writes @file:<path> instead of inline JSON past 50000 characters, and --pick resolves that BEFORE parsing; with no test, a future reordering of those two steps turns every large result into a false pick_output_not_json. The fixture seeds 1200 phase directories and measures the payload at 62474 characters, asserting the spill actually happened rather than assuming it. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): correct the strict-argv surface against a full verification run The first full run came back with 90 failures across 12 files, none in the new tests. They were the argv surface telling me what it actually is. Ten root causes; each classified before anything was changed. I over-implemented, and that is reverted. ADR-3473 §8.4 says parseNamedArgs rejects "unrecognized and positional tokens". It says nothing about a value flag whose value is missing. Making that an error was my design decision, not the rule, and it broke a deliberately recorded contract: `--prd` with no value resolving to null (tests/init.test.cjs emptyPrdValueIsFalsyAndTreatedAsAbsent, row B5; tests/section-manifest-init-facts.test.cjs "flag-shaped value"). The "requires a value" branch is deleted outright rather than kept behind an option — an unused strictness mode is speculative generality. Unknown-flag and unexpected-positional rejection, which is what §8.4 actually mandates, is unchanged. --wave needed a third flag kind the original design did not anticipate. `--wave N` is documented (commands/gsd/execute-phase.md:4,48) and the shipped workflow reconstructs and passes it (execute-phase.md:84), while #2932 records token-PRESENCE semantics: the CLI cares only that the flag appeared, and the value belongs to the workflow layer. That is neither a boolean flag nor a value flag, so `optionalValueFlags` now exists — presence-only in `data`, and the validation cursor consumes a following non-flag token so it is not reported as a stray positional. Every other declared boolean flag was checked against every argument-hint and prose usage in commands/, workflows/, agents/ and docs/; `--wave` is the only one of this shape. Five tests were pinning forms that never worked. tests/adr857-core-without-capabilities.test.cjs passed `init plan-phase --phase 01-stub`, but the documented form is positional (docs/CLI-TOOLS.md:776) and the handler reads args[2] — which for that form is the literal string "--phase". Measured on the pre-fix build against a real .planning/phases/01-stub/ directory: init plan-phase 01-stub -> phase_found=true init plan-phase --phase 01-stub -> phase_found=false The test asserted only exit 0 and key presence, so it had been green while proving nothing about phase resolution. Corrected to the documented form and strengthened to assert phase_found === true. Same class in state.test.cjs (`--plan-count`, a flag that does not exist; the real one is `--plans`), milestone-archive.test.cjs (`init new-milestone --json`, silently ignored), and concurrency-safety.test.cjs (a bare positional field name whose OR-assertion passed because a whole-document dump happens to contain the substring it looked for). Six handlers had no argv validation at all — the same #3358 shape this phase exists to close, found while fixing the rest: init verify-work / phase-op / review / todos / remove-workspace read args[2] with nothing checking the rest, and validate health read --repair/--backfill through a bare args.includes() scan that bypassed the parser entirely. All now go through the seam, so the flag has one owner. tests/init-debug.test.cjs rows C4/C5 asserted that an unrecognized flag must NOT fail. That is the behavior §8.4 removes, and Decision 8 says a caller's local expectation does not override §8, so they are inverted and renamed — a test still called "ignores an unrecognized flag" while asserting rejection would be its own defect. Row C6's point is its PWNED canary; that assertion is kept verbatim and only its exit-status expectation changed, because the hostile token is now rejected rather than absorbed. The blast-radius estimate in 40-design.md is corrected rather than quietly left wrong. get_impact reported MEDIUM / 8 symbols upstream, and that was accurate for what the graph can see — parseNamedArgs's callers. It cannot see that those callers' handlers accept argv shapes wider than the code reading args[2] suggests, which is where the real surface was. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): withdraw the validate-health tightening, finish the A2/A3 revert Second full run: 46 failures, down from 90. Four causes, two of them mine. Reverted `validate health` entirely — it was scope creep, and it broke a real flag. ~30 of the 46 read `unknown flag "--json"; accepted: --repair, --backfill`. The previous commit routed `validate health` through the parser on the reasoning that a flag should have one owner. That was wrong twice over: §8.4 names parseNamedArgs and count queries, and `validate health` was never a parseNamedArgs call site — it read its flags, just not through the parser, so it had no silent-drop defect to fix. Tightening it omitted `--json`, which the health-diagnostic suites use heavily. The handler is now byte-for-behaviour back to its pre-branch form. `validate context` stays converted: it genuinely was a call site, and its `--json` is now declared rather than read by a second `args.includes` scan. The five handlers that had NO validation at all — init verify-work / phase-op / review / todos / remove-workspace — stay fixed. Those read args[2] with nothing checking the rest, which is the #3358 shape this phase owns. Finished the A2/A3 revert. Three tests still encoded the deleted "a value flag with a missing value is an error" rule, including one added by the previous commit for that rule. All three now assert the reverted null contract, and the ones whose titles said "rejected" are renamed — a test named for a contract it no longer asserts is its own defect. `--wave=` and `--wave --weird` are correctly rejected. Neither is documented in commands/gsd/execute-phase.md, gsd-core/workflows/execute-phase.md or docs/, and neither is emitted by the shipped prompt layer, so both are unrecognized tokens that §8.4 mandates rejecting. `doesNotConsumeFollowingFlagAsWaveValue` keeps the property it exists for — asserted directly now, at the parser, that `--wave` does not swallow a following flag as its value — and only its exit-status expectation changed. A contradiction inside this branch, surfaced by the audit and resolved the safe way. Two pre-existing #3573 tests call `state begin-phase '2'` and `state planned-phase '2'` with a bare positional, relying on the old permissive parser to ignore it. This branch's own #3358 regression test requires that exact argv to be REJECTED. The two are mutually exclusive. Widening the router to accept a bare positional — mirroring complete-phase — would have silently re-opened #3358, and was verified to do exactly that: with the widened router, `query state.planned-phase 3` returned exit 0 and wrote current_phase_name again. It is reverted. docs/CLI-TOOLS.md:116 and docs/COMMANDS.md:2192 document only the `--phase N` form for both verbs, so the two #3573 tests move to it. Their assertions were never about the call shape — only that total_phases survives the resync — and both still pass. complete-phase is untouched: its bare positional IS documented, and it keeps the dynamic boundary and the negative-space note that record why. The audit that produced this is in the PR body: for every handler whose declaration changed, the flags it reads anywhere in its body, the flags the shipped surface documents, and the shapes the suite passes, compared. The `--json` miss was a pattern, not an accident — declaring a handler's flags from its parseNamedArgs call alone misses whatever it reads elsewhere. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3884): backfill the changeset PR number Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
878f25025c |
enhance(#3905): the exit-code registry — one number, one meaning, enforced at build (#3920)
* feat(#3905): the exit-code registry — one number, one meaning, enforced at build A generated registry replaces locally-invented exit codes. Every entry records code, name, meaning, owning module and the decision that authorized it. The generator refuses to build a table where two entries claim one code, two claim one name, a code falls in a range Node or the shell reserves, 2 is claimed by anything but the hook adapter, or an allocation carries no justification. exitCodeFor is pure and total: it throws rather than returning undefined, including for prototype-chain names. Inert by design — nothing emits a registered code until #3906. Every registered code is non-zero, asserted over the whole table, so a caller testing for failure behaves identically for pass and trips for everything else. * feat(#3905): make the registry generator's failures machine-readable Adds a --json mode carrying {ok, reason, context, detail}, where context is a typed payload naming the specifics the prose embedded - which code collided and under which names, which band rejected a code, which field was missing. The tests now assert on that structure instead of regex-matching the generator's stderr, which CONTRIBUTING prohibits, and the CONTEXT.md glossary gains the entry the issue's scope requires. * test(#3905): refresh the install-tree fixtures for the new declaration The registry declaration ships in the install tree, so all 19 golden fixtures needed regenerating. Caught by the remote matrix, not by lint:ci - the install-tree goldens are verified by a test rather than a lint, so a newly shipped file clears every local gate and fails only under the suite. * chore(#3905): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
7e9d33c378 |
enhance(#3915): stryker on the official tap runner, 29m to 12m on the critical path (#3919)
* enhance(#3915): stryker on the official tap runner, per-test coverage The 'command' runner is the one runner Stryker excludes from coverage analysis, which forced coverageAnalysis:'off' and made mutation cost strictly linear in (mutants x whole-shard test time). The frontmatter shard measured 1751s against 212s for the next slowest. Swap to @stryker-mutator/tap-runner (official plugin, peer-pinned to the @stryker-mutator/core@9.6.1 already installed) and turn coverage analysis on, so Stryker re-runs only the test files that cover each mutated line. Per-shard injection moves from a MUTATION_TEST_CMD command string to MUTATION_TEST_FILES, read by a new fail-closed resolveMutationTestFiles() that mirrors resolveMutationBreak. It stays derived from scripts/mutation-matrix.cjs and now has a single owner: the union moves behind allCoveredTests() and stryker.config.mjs no longer imports COVERED. The resolver existence-checks every entry, because the tap runner resolves tap.testFiles with glob() and a non-matching pattern yields an empty list silently - a fast, confident, meaningless run. tap.forceBail is off by measurement, not preference: a structural AST audit found 3 of 26 shard test files spawn subprocesses, and bail fires on every killed mutant, so leaving it on would kill processes mid-spawnSync and orphan their children. The matrix 'isolation' field is removed - per-file process isolation is inherent to the tap runner, so the field had no consumer left. Score arithmetic is unchanged and now pinned: mutationScore counts NoCoverage in the same denominator as Survived, which both Stryker's thresholds.break and check-mutation-score-ratchet.cjs read. A new non-vacuity test proves it diverges from mutationScoreBasedOnCoveredCode, so the gate cannot be quietly swapped to the field that would make every floor trivially satisfiable. Refs #3915 * fix(#3915): enforce the resolver's documented containment contract Review findings, all fixed in place. resolveMutationTestFiles claimed to verify each entry exists 'relative to the repo root' but used a bare fs.existsSync(path.join(...)), which accepts an existing DIRECTORY and lets ../ segments escape the root (path.join('/repo/root','../../etc/passwd') resolves to /etc/passwd). Not reachable from PR content - the value only ever comes from the static COVERED registry via CI env - but a fail-closed contract that overstates its own guarantee is a defect in the contract. Each entry is now resolved, rejected if path.relative puts it outside the root, and required to be a regular file. Three hostile-input tests added; the original missing-file wording is preserved so the existing assertion still binds. Also: removed a stale buildResult comment still naming the isolation field this branch deleted; hoisted one top-level path require in place of three inline ones; de-duplicated the derived-test-list expression in the tests to a single const, deliberately still re-derived from COVERED rather than calling allCoveredTests() so the assertion cannot become a tautology; tightened the workflow-parity assertion to exact equality; changed the tap-runner range from an exact 9.6.1 to ^9.6.1 so it tracks the caret-ranged core its peerDependency pins exactly. Regenerated examples/dynamic-context-management/CONTEXT-INDEX.json, which the earlier CONTEXT.md edit left stale - lint:ci was red on lint-example-parser-parity until it was refreshed. Refs #3915 * test(#3915): kill model-catalog survivors, set frontmatter budget from measurement From mutation run 33026833181 (all 13 shards, dispatched on this branch before any PR). WALL TIME: frontmatter measured 713s (11m53s) vs 1751s (29m11s) on the command runner, a 59% cut. timeoutMinutes 60 -> 20 (1.68x measured). Not deleted outright: the shared 15-minute default would leave only 21% headroom, and this module's mutant count grew 1.8x in one change. MODEL-CATALOG: came back 57.91 against its floor of 58. Diagnosed from the report JSON, not assumed - all 24 of its new RuntimeError mutants are in the load-time catalog bootstrap, so mutating them makes the module throw at require. Under node --test that is a failed test file (Killed); under the tap runner the process dies before emitting TAP, which Stryker classifies RuntimeError and excludes from the denominator. Add them back as killed and the shard is 248/416 = 59.62, the pre-change number exactly. Detection did not regress, classification changed. The floor is NOT lowered to absorb that. 11 new behavioural tests target ~42 genuinely surviving mutants: exact Set equality on the EFFORT_RENDERING/EFFORT_ARGV supported sets, an exact-string table render that makes the column-width arithmetic observable (padEnd never truncates, so the old substring assertion could not see a too-small width), clampEffortForHost's full null matrix plus a spoofed-toString host, and a prototype-pollution guard driven through Object.fromEntries. Mutants judged equivalent were skipped rather than papered over; reasoning is in the phase artifacts. All 15 new assertions verified against unmutated code. lint:ci exit 0. Refs #3915 * test(#3915): ratchet model-catalog's floor to 74 on measured 75.26% CI run 33029755081 measured model-catalog at 75.26% (295 killed / 49 survived / 48 no-coverage / 24 runtime-error, totalValid 392), so check-mutation-score-ratchet.cjs correctly failed the shard for unclaimed headroom: 17.26 points above the declared floor of 58, well past the 5-point slack. Floor raised 58 -> 74 (floor(75.26)-1, this file's documented convention), with RATCHET_BASELINE updated in the same diff as the equality assertion requires. Worth recording why the number moved this far. The shard first came back at 57.91 under the tap runner and the temptation was to lower the floor to match. The drop was not a regression: all 24 of its RuntimeError mutants sit in the load-time catalog bootstrap, and adding them back as killed reproduces 248/416 = 59.62, the pre-swap figure exactly. Rather than absorb a reporting artifact by weakening the gate, 11 behavioural tests went after the genuinely surviving mutants and killed 68 of them - carrying the module from 59.62 past its old ceiling to 75.26, within reach of the ADR-456 target of 80. The #3007 measurement is kept as clearly-labelled prior context rather than deleted, so the entry does not read as carrying two current numbers. Refs #3915 --------- Co-authored-by: sim <sim@local> |
||
|
|
6b7df61938 |
enhance(#3881): one YAML parser — vendored js-yaml replaces the hand-rolled dialect (#3888)
* docs(#3881): answer §8.1's open question and correct three wrong premises ADR-3473 §8.1 carries a blocking open question with a forcing function: it must be answered before any implementation PR for the rule opens. Answered here as (a), a string-coercing adapter, with the measurement that settles it. The sequencing note bet that §8.8's schema would make (b) tractable. Measured against merged reality it does not: only 33 of extractFrontmatter's 78 non-test call sites read STATE.md, and two of the five compensating mechanisms §8.1 lists survive real types, leaving ~31 lines across 3 call sites as the actual prize. Also corrects three claims verified false while answering it. §8.1's justifying sentence names #3349 and #3360 as defects a real parser would fix; both are already fixed on next, confirmed by executing the compiled parser rather than reading it. The guard roster calls lint-frontmatter-scalar-broad-grep.cjs an expected casualty of this rule, but it guards shell grep idioms in workflow bash fences and never touches our parser. The same roster calls lint-vendored-deps.cjs reusable as-is; it is hardcoded to re2js throughout. The last two were caught by applying the rule this amendment records -- a factual claim in this ADR is a hypothesis until the implementing phase executes it -- on its first use. Refs #3881 * docs(#3881): record that §8.1's fork is ill-posed and (a) is not implementable An adversarial pass on the Phase 4 design established by execution that extractFrontmatter is not a YAML parser but a line-oriented scanner whose output is a function of raw source text. Four spellings of the same value collapse to one js-yaml tree but produce four distinct legacy strings, one of them mangled. No adapter over a tree can choose among outputs the tree does not distinguish, so fork (a) -- keep a string-coercing adapter so the existing contract holds -- cannot be built. For any document with a non-scalar value, (a) collapses into (b); about 26 percent of frontmatter-carrying documents have one. Also records three design defects and one new attack surface, all confirmed by execution: catching a parse failure and returning {} would delete the frontmatter block on the next write at eight call sites that conflate empty with unparseable; an empty value yields null where legacy yields {}, and reconstructFrontmatter omits null-valued keys, so the shipped state template's empty progress key would vanish; the #1882 truncation probe is parseYamlRegion itself rather than a pre-parse heuristic, so it cannot both stay unchanged and survive that deletion; and FAILSAFE_SCHEMA still resolves aliases, expanding seven lines to 22.8 MB. The rule is not deferred. The measurement is the deliverable and the re-scoping is recorded as an open question with a forcing function, per section 8's own rule. Refs #3881 * test(#3881): failing-first rows for block scalars, unicode keys and the missing #3594 matrix Creates tests/feat-3594-parser-adversarial-frontmatter.test.cjs, the file the fixture README instructs contributors to register fixtures in but which never existed. Section C: table-driven ownership check over tests/fixtures/adversarial/frontmatter/ so a fixture with no matrix entry fails loudly; six existing fixtures (duplicate-keys, crlf-mixed, unclosed-block, unicode-keys-and-values, null-byte-value, huge-bounded) each get the invariant its README states. B1 blockScalarValueIsNotTheBlockIndicator: parsing commands/gsd/add-tests.md must give argument-instructions the instruction text, not the literal '|'. RED today. B2 blockScalarDoesNotInventATopLevelKey: same parse must not produce a top-level Example key scraped from inside the block body. RED today. B3 unicodeKeyRoundTripsAsIs: the 相 key in unicode-keys-and-values.md must survive parsing; today it is silently dropped. RED today. Refs #3881 * chore(#3881): vendor js-yaml and generalize the vendored-deps guard to a manifest Packaging step for ADR-3473 §8.1: makes js-yaml available to gsd-core/bin/** without promoting it out of devDependencies (promoting broke every installed tree, #3496). gsd-core/bin/lib/vendor/js-yaml.cjs is a verbatim copy of node_modules/js-yaml/dist/js-yaml.js (the self-contained UMD dist bundle, not index.js), exposing load/dump/FAILSAFE_SCHEMA/YAMLException with zero require() calls of its own. src/vendor/js-yaml.d.cts is hand-authored, not copied, because js-yaml ships no upstream .d.ts and @types/js-yaml is not installed. It is deliberately narrow, declaring only the four symbols in use, so anchors/aliases/custom types/loadAll are unreachable from typed code -- a compile-time enforcement of ADR-3473 §8.1's refusal to expand alias resolution for security reasons. Because it has no upstream counterpart it is excluded from the byte-compare. scripts/lint-vendored-deps.cjs is refactored from a script hardcoded to re2js into a table-driven VENDORED manifest (one row per package: upstream/vendored .cjs paths, optional .d.cts paths, twin kind upstream-verbatim vs hand-authored) so a second vendored package does not require a second hardcoded check block, per ADR-3473 §8.3 'one implementation per rule'. The four existing re2js checks (vendored .cjs vs node_modules, vendored .d.cts vs node_modules, src/vendor twin vs bin-side twin, devDependency version pin vs installed version) are preserved unchanged; verified pass/fail identical before and after the refactor, and the guard's ability to fail was re-proven with a deliberate one-byte append to both re2js.cjs and js-yaml.cjs, then restored. docs/INVENTORY.md and docs/INVENTORY-MANIFEST.json (via gen-inventory-manifest.cjs --write, run after build:lib) register vendor/js-yaml.cjs. gsd-core/bin/lib/vendor/README.md documents both vendored packages and the two twin kinds. Refs #3881 * feat(#3881): parse .planning frontmatter with the vendored js-yaml ADR-3473 §8.1: extractFrontmatter's read path is no longer a hand-rolled line scanner. parseYamlRegion, escapeDoubleQuoted, unescapeDoubleQuoted and parseQuotedScalar are deleted (not patched); parsing now goes through the vendored js-yaml (./vendor/js-yaml.cjs) under { schema: FAILSAFE_SCHEMA, json: true }. Everything js-yaml does not do is layered on top, in one place, carrying the seven design-doc consequences: 1. Empty value: a null js-yaml value is coerced to {} (matching legacy's own empty-value contract) so reconstructFrontmatter — which omits null-valued keys — still round-trips a bare `key:` line instead of deleting it. Verified live: progress: with no value survives parse -> reconstruct -> re-parse. 2. Unparseable no longer collapses to a bare {}: a new FRONTMATTER_UNPARSEABLE Symbol (exported), keyed exactly like the existing #3257 FULL_LINE_COMMENTS channel, is carried on the {} returned for malformed/refused YAML. Invisible to Object.keys/entries/JSON.stringify/for-in, so the 70 call sites that never inspect it are unaffected; wiring the 8 hasFrontmatter sites to consult it is a separate change, not done here. 3. Non-scalar object-list items (the four spellings of `- test: a b` that js-yaml collapses into one tree shape) are rendered as a canonical `key: value[, key2: value2]` string per item, keeping the existing array-of-strings value SHAPE. A full corpus differential over all 1702 tracked markdown files found 11 residual divergences from the legacy parser (enumerated in the PR/report), most of them the parser now being MORE correct (a dropped quoted top-level key, the block-scalar/phantom-key defect, a dropped Unicode key). 4. The #1882 truncation probe still runs the one real parser, but derives its key count from js-yaml's own thrown error and mark.line when the whole region doesn't parse cleanly (the dominant real truncation shape: fence opened, well-formed keys, no closing fence). Verified against both the clean-parse and the exception-fallback path. 5. The #3257 comment channel now attributes each pending column-0 comment against js-yaml's own parsed top-level key list (matched by literal key text, in document order) instead of the legacy ASCII-only key regex, so a comment above a Unicode key attaches correctly. 6. Anchors, aliases and merge keys are refused outright (a raw-text pre-scan, since FAILSAFE_SCHEMA still resolves them) — corpus occurrences today: zero. A 7-line billion-laughs fixture is verified refused rather than expanded. 7. A literal U+0000 is swapped for a private-use sentinel before the parse and restored in every resulting string afterward, since js-yaml rejects NUL unconditionally under every schema. escapeDoubleQuoted is deleted and reimplemented via js-yaml's dump() (forced double-quoted style), with control-char hex escapes lowercased to keep serialized output byte-stable (#1779 emitted lowercase); it keeps its exported name and signature for its two other call sites (commands.cts, runtime-artifact-conversion.cts), which need no change. frontmatterDeepEqual, the comment channel, sliceTopLevelFrontmatterSegments, regenerateFrontmatterKey's guard, noOpObjectListSetError and parseMustHavesBlock are all unchanged — retiring them is fork (b) and is not this phase. Refs #3881 * fix(#3881): quote template placeholders and preserve unparseable frontmatter SECURITY.md/UI-SPEC.md/VALIDATION.md wrote frontmatter placeholders as bare {N}/{phase-slug}/{date}, which is valid YAML flow-mapping syntax under the vendored js-yaml parser, not the literal placeholder text intended. Quote them so they parse as strings. Wire the FRONTMATTER_UNPARSEABLE Symbol (exported but unused) at the 8 call sites in state.cts/state-transition.cts that compute hasFrontmatter via Object.keys(extractFrontmatter(...)).length > 0 and reassemble the document without a frontmatter block when false. That check conflated 'no frontmatter' with 'unparseable frontmatter' (both parse to {}), so a document with a merge-conflict marker or refused alias in its frontmatter had that block silently dropped on write. Each site now preserves the exact raw bytes stripFrontmatter removed when the marker is set, leaving the genuinely-empty case unchanged. Refs #3881 * test(#3881): consequence and boundary coverage for the js-yaml migration Rows: A1 emptyValuedKeySurvivesAWrite, A2 unparseableDocumentKeepsItsFrontmatterBlock, A3 unparseableIsDistinguishableFromEmpty, A4 nonScalarValuesCanonicalize, A5 truncationProbeStillFiresOnAnOpenFence, A6 commentsStayOnTheirOwnKey, A7 anchorsAndAliasesAreRefused, A8 aliasExpansionCannotExhaustMemory, F1 UNTERMINATED_KEY_THRESHOLD boundary, F2 alias/nesting refusal bound, F3 frontmatter size boundary (huge-bounded.md + larger). Adds tests/fixtures/adversarial/frontmatter/anchor-alias-bomb.md and its entry in the feat-3594 fixture matrix. Refs #3881 * docs(#3881): document the vendored parser, correct a stale rationale, add a vendoring how-to Refs #3881 * docs(#3881): correct the frontmatter glossary entry Two errors in the entry as first written: it named parseYamlRegion as part of the read path when that function is deleted, and it recorded the eight hasFrontmatter call sites as unwired follow-on work when they were wired in e35ac2a2c. Also records the scope caveat that the CLI write path rebuilds the frontmatter block independently, so the marker binds at the transform layer. Refs #3881 * docs(#3881): record the semantic-migration decision and the counted guard ledger The maintainer chose the full semantic migration over splitting the rule into its own epic or patching the scanner, so section 8.1 is answered as "the fork was ill-posed and the migration is semantic" rather than as (a) or (b). Also replaces the pre-implementation guess that this phase would shrink the guard surface with the counted result: excluding vendored third-party lines the hand-maintained surface is net +307, and frontmatter.cts grew by 68 lines despite four functions being deleted, because the compatibility layer over js-yaml is larger than the scanner it replaced. Section 8.1's stated benefit is therefore not delivered as written; what improved is the kind of code maintained, not the amount. Decision 6 requires recording that rather than netting it away. Refs #3881 * chore(#3881): changeset for the vendored YAML parser migration Refs #3881 * test(#3881): golden parity, round-trip property and packaging coverage Refs #3881 * fix(#3881): refuse anchors structurally and fold in review findings ADR-3473 §8.1 review findings, addressed inline: Finding 1 (BLOCKER): refuseAnchorsAndAliases was a raw-line regex that matched only the bare-key spelling (key: &x). A quoted key ("a": &x), a flow mapping ({b: &x}) and a flow sequence ([&x, *x]) all define/use the SAME anchor mechanics while never matching that line shape, so the exact expansion the guard exists to stop went straight through unrefused (a 303-byte quoted-key bomb expanded to ~35.8MB). Replaced with js-yaml's own `load` `listener` callback, which reports `state.anchor` for every event belonging to an anchored node in every spelling, and throws from inside the callback to abort before any expansion (~1-2ms vs full expand-then-discard). A merge key with an alias is still refused (merge always requires a previously anchored node, so the alias itself trips the listener); a bare merge key with NO alias is no longer separately refused, documented as intentional: FAILSAFE_SCHEMA never resolves `!!merge`, so it carries no expansion risk. Table-driven tests added for all four bypass spellings + merge key, plus a quoted-key-spelled billion-laughs fixture registered in the adversarial matrix and README. Finding 2: src/vendor/js-yaml.d.cts's docblock falsely claimed anchors/ aliases were "simply UNREACHABLE from typed code" through the twin. Corrected to state the truth: anchor/alias resolution is document-level `load` mechanics reachable through exactly the declared surface, and refusal is enforced at RUNTIME (Finding 1's listener), not by the type surface. Finding 3 (MAJOR): the null-byte sentinel (U+E000) round-trip was non-injective — restoreNullBytesDeep rewrote every U+E000 in the parsed tree back to NUL, including one the document author legitimately wrote, silently corrupting it. Now refuses outright whenever the raw region already contains U+E000 (consistent with the existing anchor/merge-key refusal path), making the substitution provably injective. Tests added for a real NUL alone (preserved), a pre-existing U+E000 alone (refused, not corrupted), and both together (refused, not merged into one byte). Finding 4 (MAJOR): scripts/lint-vendored-deps.cjs's `srcTwin` field was dead for a hand-authored row (only read inside the upstream-verbatim branch) — exactly how Finding 2's stale docblock drifted unnoticed. Added checkHandAuthoredTwin: every value-level export the twin DECLARES must be an actual own property of the vendored runtime module at require-time. Tests added, including a sensor that a declared-but-nonexistent export IS caught. Finding 5: the existingFm/hasFrontmatter/stripFrontmatter/fmPrefix/ unparseableFm/reassemble preamble, copy-pasted at 7 sites in state-transition.cts plus a sixth hand-inlined copy in state.cts's cmdStateCompletePhase, is now one exported helper (beginFrontmatterReassembly) every site routes through, including the hand-inlined one. Three call sites (beginPhaseCore, patchCore, updateCore) keep a literal `body = stripFrontmatter(content)` assignment alongside the helper call so scripts/lint-state-write-path-drift.cjs's single-hop backward scan (which does not chase aliases) still sees the strip; stripFrontmatter is pure/idempotent so the extra call changes nothing observable. Finding 6: corrected the frontmatter.cts docblock's stale "wiring is a separate change" claim (the 8 call sites are wired on this branch) and the changeset's backlink from (#3473) to (#3881). Finding 7: fixed the lint:ci failures blocking the gate — an @typescript-eslint/only-throw-error violation from throwing a bare Symbol as the anchor-detected signal (now a real Error subclass), unused-var warnings left over from the Finding 5 refactor, a lint-test-file-count cap exceeded by two migration-specific test files (allowlisted with justification), and the lint-state-write-path-drift false positive from Finding 5's helper (fixed above). tests/frontmatter-golden-parity.test.cjs:117's execFileSync already carried an explicit timeout; no change was needed there. Golden fixture: added a golden entry for the new anchor-alias-bomb-quoted.md fixture ({} — matches what the legacy line scanner would also produce, since it independently dropped every quoted top-level key). No other corpus document diverges: real .planning/ documents carry zero anchors/aliases/merge keys/U+E000 today. Refs #3881 * fix(#3881): fold in second-round review findings Finding 1 (BLOCKER): tests/frontmatter.test.cjs pinned the pre-migration ASCII-only key regex for the Unicode fixture; updated to require the 相 key's value now that js-yaml has no such restriction. Audited the rest of the file for other pre-migration pins (block scalars, quoted keys, flattened values, empty values, duplicate keys, unclosed blocks, null bytes) by execution against real fixtures; found none regressed. Finding 2: parseYamlRegion and escapeDoubleQuoted renamed to parseGuardedYamlRegion and escapeDoubleQuotedScalar in src/frontmatter.cts so no function still answers to the deleted hand-rolled scanner's name (ADR-3473 §8.1 "deleted, not patched"). escapeDoubleQuotedScalar's three external call sites (src/commands.cts, src/runtime-artifact-conversion.cts) updated in the same change — a mechanical rename, not an ADR-amendment matter. Finding 3 (BLOCKER): fixed a real crash and a silent data-loss bug found by execution. A top-level key named constructor/__proto__/toString/ valueOf/hasOwnProperty crashed reconstructFrontmatter (bracket read resolving an inherited Object.prototype member); a key literally named __proto__ was silently DROPPED entirely (bracket assignment on an ordinary {} invoked the inherited __proto__ setter instead of creating a data property). Fixed by building every parsed Frontmatter object with Object.create(null), and replacing an `in` check with hasOwnProperty.call in propagateCommentChannel. Added round-trip tests for all five hostile keys, each with its own leading comment. Finding 4 (MAJOR): escapeDoubleQuotedScalar's docstring falsely claimed full byte-stability across the migration. Verified by execution: BEL/NUL/ NEL/NBSP/LS/PS/BOM now emit YAML-named escapes instead of the old hex/raw- literal forms. Proved round-trip equivalence (each escape re-parses to the exact source codepoint) and corrected the docstring. Found and fixed a related real defect while verifying: a lone UTF-16 surrogate was emitted BARE (scalarNeedsDoubleQuoting didn't trigger), producing genuinely unparseable YAML that silently collapsed to {} on re-read — extended scalarNeedsDoubleQuoting to route surrogates through the quoted+escaped path. Finding 5 (MAJOR): countKeysBeforeTruncation went silent on 4 real truncation shapes (unquoted colon, open flow collection, mis-indented sibling key, refused anchor). Root cause: the mark-based prefix recovery excluded the very line whose key needed counting, and a mark-less refusal never entered the recovery branch at all. Fixed by taking the max of two lower bounds: the longest parser-verified line-prefix, and a raw-text count of key-shaped lines (reusing the same key-shape pattern this file already uses for isFrontmatterShaped). Extended test-matrix row A5 table-driven over all 4 regressed shapes. Finding 6: the design doc's claim that no test owned the #3594 adversarial fixture corpus was false — consolidation epic #1969 had already folded it into tests/frontmatter.test.cjs. An earlier commit on this branch re-created a standalone duplicate under that false premise; folded its genuinely-new coverage (fixture-ownership check, anchor-bomb fixtures, block-scalar B1/B2 rows) into frontmatter.test.cjs and deleted the duplicate file. Corrected the false claims in 40-design.md §3.3.1 and the ADR's §8.1 note, including the roadmap-sibling claim (no such file exists). Finding 7: the golden serializer sorted object keys, making it structurally blind to the key-order-parity invariant ADR-3473 §8.1 actually claims. Made it order-preserving and regenerated the golden fixture from a standalone compile of the legacy (pre-#3881) parser at ddde001af; the current parser matches it with zero undocumented divergences, confirming key-order parity genuinely holds. Extended row A2 table-driven across 6 of the remaining 7 transitionCore kinds (all pass) plus documented, by execution, a newly-discovered 8th-site regression: state.cts's cmdStateCompletePhase calls the same preservation helper but its result is clobbered by a later unconditional resync — filed as a distinct finding rather than fixed here (touches syncAndPreserveStateMd, outside this change's verified scope). Refs #3881 * fix(#3881): preserve unparseable frontmatter through the CLI write path Characterization (executed, before/after shown): case (b), not (a). The frontmatter FENCE survives — `state complete-phase` on a conflict-marked STATE.md returns success and a well-formed, freshly-derived frontmatter block, not a document with no frontmatter at all. But the block's actual content (the merge-conflict markers, and with them any signal to a human that the document was in conflict) is silently discarded and replaced. Root cause was two clobber sites, not one: 1. syncStateFrontmatter (src/state.cts) re-parses the already-preserved `transformedContent` from readModifyWriteStateMd, finds {} + the FRONTMATTER_UNPARSEABLE marker, and unconditionally rebuilt a fresh frontmatter block from the body anyway. 2. Even after (1) is fixed, applyPostSyncPreservation's own postFm/applyStatePreservation/authoritativeFm-reassertion machinery re-extracts frontmatter from syncedContent, restores curated fields from the pre-write snapshot, and reconstructs a NEW block again — confirmed live via `state begin-phase`, which still lost the markers after fixing (1) alone. Both are now guarded by the same predicate (isUnparseableFrontmatter, checking FRONTMATTER_UNPARSEABLE): when the ORIGINAL frontmatter did not parse and the caller is not on ADR-3408 §8.3's closed "body wins" list, both functions return their input content unchanged rather than re-deriving over it. The closed list (cmdStateSync #905, /gsd-health --repair's REGENERATE_STATE, both routed only through writeStateMd, which never reaches applyPostSyncPreservation and passes sanctionedPermanentEmptyFallback=true to syncStateFrontmatter) is untouched — neither widened nor narrowed; verified by execution that `state sync` still overwrites the conflict-marked block exactly as before. Other verbs sharing the same readModifyWriteStateMd path were checked and were equally affected before this fix: state update, query state.patch, and state begin-phase all lost the conflict markers (RED, shown by execution), and all three now preserve them (GREEN). Covered table-driven in tests/feat-3881-yaml-parser-consequences.test.cjs's new A2b describe block, which drives the real CLI verbs via runGsdTools — not just the pure transitionCore layer the earlier A2 rows exercised — plus a control asserting state sync's body-wins contract is unchanged. Refs #3881 * fix(#3881): restore the parse surface's prototype and fix remote-runner failures Root cause of the bulk of the 88 remote-runner failures: extractFrontmatter/parseGuardedYamlRegion handed back Object.create(null) trees for prototype-pollution safety, but assert.deepStrictEqual compares prototypes, so every assertion against a plain object literal failed (57 frontmatter.unit.test.cjs + 5 frontmatter.test.cjs + others). Fixed by keeping the internal construction null-prototype (unchanged) and converting to a plain-prototype tree via Object.defineProperty (never bracket assignment, so __proto__/constructor/toString keys stay safe) at the parseGuardedYamlRegion/unparseableResult return boundary only; the internal FULL_LINE_COMMENTS Symbol channel is copied by reference, not recursed, so its own __proto__-safety is untouched. Per-class fixes: (1) bomAcrossArtifactTypes was the same prototype bug, no separate code change needed. (2) frontmatter-cli #1660: added objectListFieldWouldLoseData, a broader lossy-field detector alongside the existing byte-identical noOpObjectListSetError -- js-yaml's flattenObjectListItem now correctly includes every sub-key of an object-list item (a real bug fix over the legacy scanner, which silently dropped every field but the first), so a set that drops that now-included data is no longer byte-identical to the original and needs its own guard. (3) uat.test.cjs: updated the pinned expectation for the human_verification quote-stripping artifact -- js-yaml resolves quoting correctly where the legacy regex left an unbalanced quote; documented as an intentional, non-lossy behavior change. (4) smart-entry: added a fallback-only loadWithAmbiguousColonRepair so a column-0 key: value line whose value itself contains an unquoted colon (the #2571 hand-edited-STATE.md shape) round-trips instead of failing the whole frontmatter block closed. (5) frontmatter.unit.test.cjs bracket-array leniency: added a second fallback, repairMalformedInlineArrays, restoring the legacy scanner's tolerant inline-array handling (consecutive/blank commas, unclosed bracket) -- both repairs run ONLY after the primary parse already threw, so well-formed documents are unaffected. (6) prompt-injection-scan: src/frontmatter.cts had a literal U+FEFF BOM embedded in a comment illustrating the #2977 fix; replaced with the U+FEFF text escape. (7) eslint-glob-coverage: allowlisted the new src/vendor/js-yaml.d.cts vendored type declaration, same precedent as the existing re2js.d.cts entry. (8) frontmatter-golden-parity: git ls-files *.md now runs with -c safe.directory=* (process-scoped) so it survives the remote runner's dubious-ownership check without a persistent git config write. Refs #3881 * chore(#3881): backfill changeset PR number Refs #3881 * test(#3881): make golden parity resistant to unrelated tree churn A corpus-wide snapshot keyed to every tracked *.md file was coupled to mutable-by-design files: .changeset/*.md's pr:0 -> real-PR-number backfill is a required workflow step, not a parser change, yet it turned this suite red. Training people to 'just regenerate the golden' on that kind of failure defeats the point of the snapshot. Exclude .changeset/** from the golden corpus entirely, tolerate tracked *.md files with no golden entry (they postdate the capture) instead of failing on them, keep hard failures for a golden entry whose file has vanished from the tree and for any real parity divergence, and add a coverage floor so the enumeration cannot quietly degrade to comparing a handful of files. Golden regenerated by recompiling the legacy pre-migration parser (git show ddde001af:src/frontmatter.cts) standalone, independent of the current parser, over the same non-changeset corpus. Refs #3881 * test(#3881): make the parser golden hermetic instead of tree-keyed This repo merges ~21 commits/day; a 14-day sample measured 937 touches of the exact files (commands/gsd/*.md, gsd-core/workflows/*.md, agents/*.md, docs/*.md) the prior golden pinned by tracked path. Any PR editing one of those files' frontmatter for reasons unrelated to the parser (an argument-hint addition, an allowed-tools tweak) turned the suite red, and the reflex fix -- "regenerate the golden" -- overwrote the very snapshot meant to catch a real regression. Excluding .changeset/** was not enough; the design itself was wrong: a regression fixture must not be keyed to mutable repo paths, and a single 376-entry JSON every such PR touches is also a guaranteed merge-conflict surface. Rebuilt the fixture to carry its own documents: each of 51 entries stores a stable id, literal documentText (shrunk from a real ddde001af-era corpus document), and an expectedParse captured independently from the pre-migration legacy parser (git show ddde001af:src/frontmatter.cts, compiled standalone against its byte-identical sibling modules). The test reads no tracked path, shells out to no git command, and enumerates no tree -- a PR editing commands/gsd/help.md cannot affect it. Every entry's reconstruction was verified at capture time to reproduce both the current and legacy parser's output on the original document; 0 of 51 candidates were dropped by that check (1, the deliberately-unterminated unclosed-block.md adversarial fixture, has no closing fence to truncate at and is stored unshrunk). Kept the 5 documented DIVERGENCES rows (now diverges:true entries) and the D2 order-preserving structural serializer that keeps the comparison from passing vacuously; dropped the tree-enumeration helpers, the coverage floor, the post-capture-skip logic, and the vanished-file check -- all artifacts of the path-keyed design. Refs #3881 * fix(#3881): resolve vendored-deps paths independently of cwd shape Five rows in tests/lint-vendored-deps-manifest.test.cjs failed on windows-latest CI: the test passed absolute scratch-file paths into compareFiles()/checkRow(), whose helpers joined every input onto ROOT via path.join(ROOT, rel), producing garbage when the input was already absolute. It surfaced on windows-latest specifically because GitHub's Windows runners checkout the repo on a different drive than TEMP, so path.relative(REPO_ROOT, tmpFile) returned the absolute path unchanged (no relative traversal is representable across drives) rather than the relative form the test assumed. The remote gsd-test runner this repo gates pushes on is Linux-only and could never have caught this; GitHub CI's windows-latest job is the only signal that does, and it did. Fixed the helper itself (scripts/lint-vendored-deps.cjs's new resolvePath()) to treat an already-absolute input as absolute-in, absolute-out instead of silently mis-joining it, and updated the test to pass the scratch file's absolute path directly rather than relying on a relative conversion that is not always representable. Kept every mutation-sensor assertion intact and added coverage proving resolvePath is a no-op for relative inputs and correctly passes absolute ones through unchanged. Refs #3881 * fix(#3881): warn when state sync regenerates over unparseable frontmatter state sync (ADR-3408 §8.3's sanctioned regenerate path) correctly overwrites an unparseable frontmatter block per its 'body wins' contract — that overwrite behavior is unchanged here. The defect was the silence: synced:true/exit 0 gave no signal that the existing block (including git merge-conflict markers) could not be parsed and was destroyed, per ADR-3473 §8.5 ('a derived conclusion may not be reported as authoritative when the derivation dropped input it could not resolve') and §8.4 ('failure is a value'). Adds a gsd: warning — ... (#3881) line on stderr, matching the existing #3573 precedent, and surfaces the same disclosure in the JSON result's existing changes[] array so a machine consumer sees it too. Exit code and synced:true are left unchanged — sync did what its contract says. REGENERATE_STATE (/gsd-health --repair's sibling on the same sanctioned-regenerate list) is DESTRUCTIVE-risk and unconditionally refused by applyRepairs's dispatcher before runRepairAction ever runs (src/health-diagnostic.cts), so it is not a live path today and is not in scope for this fix. Refs #3881 * fix(#3881): exit non-zero when a state command returns an error Refs #3881 * chore(#3881): changeset for the state exit-code fix Refs #3881 * fix(#3881): honor the documented --project-dir flag Refs #3881 * revert(#3881): restore exit-0 result envelopes for state errors Reverts 9638f2936 and its changeset. The change was wrong and the revert is the correction. This repo distinguishes two error mechanisms deliberately. error() in src/io.cts writes to stderr and calls process.exit(1) -- the hard-failure path. output({error: ...}) writes a JSON result envelope to stdout and returns normally with exit 0. The reverted commit converted 23 result-envelope sites into hard failures, which is a different contract, not a bug fix. tests/state-contract.test.cjs's errorPathDoesNotPublish asserts the envelope contract directly -- a failing command exits 0 with a JSON error envelope and must not publish state.json -- and the remote matrix run caught it along with four cases in the QA scenario walk. Thirteen tests in tests/state.test.cjs that the original commit rewrote were encoding that real contract, not the bug it claimed; they are restored. Whether an error envelope on stdout with exit 0 is the right CLI design is a genuine question, and it is section 8.4's rule ('failure is a value') with its own phase. It is not something to flip inside this PR. Refs #3881 * chore(#3881): backfill changeset PR number for the project-dir fix Refs #3881 * test(#3881): keep the frontmatter mutation shard inside its time budget The Stryker (frontmatter) shard hit the documented 15-minute (900s) shard cap. Root cause is NOT row-level spawn overhead (contrast the #2790/ core-utils precedent): the three shard test files' own logic runs in ~413ms total (356+30+27ms) with all 392 assertions passing. Instead, src/frontmatter.cts grew from ~825 to 1496 lines (+671/-187) migrating to the vendored YAML parser, proportionally growing the mutant count Stryker generates for gsd-core/bin/lib/frontmatter.cjs. Stryker's command runner bills the full 'node --test <3 files>' invocation once per mutant, and node:test's default per-file process isolation forks a child process for each of the three files on every one of those invocations — pure fork overhead multiplied by a much larger mutant population. Fix: scripts/mutation-matrix.cjs COVERED.frontmatter now declares isolation: 'none', and .github/workflows/mutation.yml passes --test-isolation=${{ matrix.isolation }} (defaulting to 'process' — i.e. unchanged behavior — for the other 8 shards, which were not individually audited for cross-file state leakage under shared-process execution). Measured locally via node:test's run() API on the exact 3-file set: isolation:'process' took ~593ms vs isolation:'none' ~478ms for the same 392 passing assertions. The true CI-shard number can only be confirmed on the GitHub Actions run (Stryker cannot run locally, and 'node --test' is hard-blocked in this environment). Refs #3881 * test(#3881): register the vendored-parser tests in the frontmatter mutation shard stryker.config.mjs's own rule ("Keep this list in sync with the tests arrays in scripts/mutation-matrix.cjs COVERED") was violated: #3881 grew src/frontmatter.cts from ~825 to 1496 lines but its new tests (tests/feat-3881-yaml-parser-consequences.test.cjs, tests/frontmatter-golden-parity.test.cjs, tests/frontmatter-roundtrip.property.test.cjs, and +167 lines in tests/frontmatter.test.cjs) were never added to the frontmatter shard's tests array, so Stryker's mutants in the new vendored-js-yaml adapter had nothing constraining them. PR #3888 measured 55.8% against the 65 floor (748 killed / 593 survived / 17 timeout) and the shard was separately cancelled at 15m04s against the 15-minute per-shard cap. Registers all four files (each earns its slot on evidence of a unique constraining assertion, documented inline), gives the shard a measured/projected 180-minute budget via a new per-module timeoutMinutes field threaded through mutation.yml's job-level timeout-minutes the same way isolation is threaded, and removes the prior isolation:'none' override (re-measured at this file-set size, its savings are within run-to-run noise, not worth the unaudited cross-file-state-leakage risk). Refs #3881 * feat(#3881): derive the mutation test list and ratchet the score floor Refs #3881 * test(#3881): ratchet five stale mutation floors and close the frontmatter gap Raised five module minScore floors per CI run 33012034388 (floor(achieved)-1): config-schema 75.51%->74, prompt-budget 88.95%->87, context-composer 79.92%->78, context-utilization 92.31%->91, active-workstream-store 87.42%->86. Updated both scripts/mutation-matrix.cjs COVERED entries and tests/mutation-matrix-ratchet.test.cjs RATCHET_BASELINE in the same diff per the ratchet's own contract. Closed the frontmatter shard's 63.03%-vs-65 gap with new behavioral tests in tests/feat-3881-yaml-parser-consequences.test.cjs, each paired with a documented near-miss: frontmatterDeepEqual's array-order/length/type-mismatch/key-order semantics (via spliceFrontmatter's no-op guard), scalarNeedsDoubleQuoting's leading/trailing-whitespace and dash/surrogate triggers (via reconstructFrontmatter), repairAmbiguousColonValues' already-quoted vs ambiguous-colon repair paths (via extractFrontmatter), and the null-byte sentinel round-trip surviving at region offset 1. Did not lower minScore. Refs #3881 * test(#3881): decouple the ratchet test from real module floors The CLI end-to-end rows in tests/mutation-score-ratchet.test.cjs hardcoded config-schema's real floor (52), which commit 973321541 legitimately ratcheted to 74 -- breaking a test pinned to the exact value the mechanism under test exists to change. Add an injectable --matrix seam to scripts/check-mutation-score-ratchet.cjs and point the CLI rows at a synthetic module + synthetic floor built via a temp fixture, so the rows are indifferent to any real module's floor moving while still exercising the same fail/pass behaviour. Refs #3881 * refactor(#3881): parse must_haves with the vendored parser and drop re-implemented leniency Refs #3881 * fix(#3881): restore the ambiguous-colon repair its hand-edited-STATE.md contract needs A tracked-document sweep of 910 *.md files cannot see this dependent: repairAmbiguousColonValues's one real caller is user hand-edited STATE.md content that never lives in this repo's tree, only on end users' machines, and is pinned by tests/smart-entry.unit.test.cjs. Restores the function plus its post-throw fallback path (loadWithAmbiguousColonRepair) only; repairMalformedInlineArrays and splitLegacyInlineArrayItems stay deleted, reverified against the full frontmatter test shard. Adds a frontmatter-level regression row in tests/feat-3881-yaml-parser-consequences.test.cjs so the dependency is visible where the function lives. Closes #2571 Refs #3881 --------- Co-authored-by: sim <sim@local> |
||
|
|
a638ca4332 |
enhance(#3882): stop sentinel phases skewing estimation calibration (#3893)
* test(#3882): failing-first rows for sentinel phases skewing calibration Adds A1a/A1b/A2/A3 to tests/estimate-calibrate.test.cjs, the module's existing test file, rather than a new bug-NNNN file. collectCalibrationSamples (src/estimate-cli.cts:206) does a raw readdirSync over .planning/phases and never applies isSentinelPhaseId, so a sentinel phase (milestone 0 or 999) carrying a PLAN estimate / SUMMARY actuals pair contributes a phantom calibration sample. computeCalibration is median-based, so a single 50x outlier among three samples leaves the factor unmoved — asserting "the factor is unchanged" against one sentinel would pass on the broken code for the wrong reason. Each row instead asserts the WHOLE computed CalibrationResult object (factor, applied, confidence, sampleCount, clamped) for a sentinel-free project against its sentinel-injected twin: - A1a: one sentinel flips applied false->true and confidence low->med on phantom evidence (calibration switches on with zero real signal). - A1b: two sentinels corrupt the factor itself (1 -> 3, clamped false->true). - A2: the sentinel's own sample is verified absent from the returned list. - A3: the two genuine phases still contribute their own unchanged samples (regression pin — stops A1/A2 passing by filtering everything). Verified RED on today's code (node tests/estimate-calibrate.test.cjs): A1a/A1b/A2 fail with the exact differing objects; A3 and all pre-existing rows in the file remain green (no collateral). Refs #3882 * feat(#3882): route phase enumeration through its owner and name the sentinel axis Task 1: collectCalibrationSamples (src/estimate-cli.cts) hand-rolled a raw readdirSync over .planning/phases, treating every directory (including sentinel phases, milestone 0/999) as a completed phase and feeding phantom PLAN/SUMMARY samples into the estimation calibration factor. Routed through the existing owner, listMilestonePhaseDirs(phasesRoot) with no cwd -- already 'all milestones, sentinels excluded', exactly the combination this caller needs; no new API was required for this half. It now also surfaces the scope discriminator: an unreadable phases directory throws PhasesUnreadableError instead of silently returning zero samples, and cmdEstimateCalibrate reports it via a new ERROR_REASON.ESTIMATE_PHASES_UNREADABLE instead of persisting a phantom empty calibration document. Task 2: added listAllPhaseDirs(phasesDir, { includeSentinels }) to src/phase-locator.cts -- the one genuinely missing axis: 'physical set, sentinels INCLUDED'. includeSentinels has no default and is required, so a call site cannot obtain sentinel-inclusion by omission (compile-time refusal, not just documentation). Mirrors listMilestonePhaseDirs's absent/unreadable scope handling. Task 3: migrated the two exemptions whose written reason maps cleanly onto 'physical set, sentinels included' -- cmdRoadmapAnalyze's _phaseDirNames (src/roadmap.cts) and cmdInitMilestoneOp's diskPhaseDirs (src/init.cts), both heading->directory lookup indexes. Left the rest: archivePhaseDirectories's own body has no readdirSync to migrate (its callers already resolve dirs before calling it, and both current callers deliberately EXCLUDE sentinels -- migrating it would be an unauthorized behavior change, not an API swap); cmdValidateHealth's exemption is vestigial (its actual physical-set sweep already lives in planning-snapshot.cts's buildAllPhaseDirNamesField, a pre-existing near-duplicate of the new axis, flagged as a finding, not restructured); cmdPhasesClear/cmdMilestoneComplete/cmdVerifySchemaDrift/detectHasPriorPhases/detectUiPhaseActive want a different combination (sentinels excluded, or a single-phase lookup) and are unaffected. Task 4: detector 2 (sentinel literal) is untouched and retained. Removed exemption entries only for the two migrated call sites; every other function-scoped exemption is preserved. Guard exits 0. Refs #3882 * refactor(#3882): delegate the snapshot phase-dir scan to its owner buildAllPhaseDirNamesField duplicated listAllPhaseDirs's own readdirSync + directory-filter + absent/unreadable handling — the 'one implementation per rule' defect ADR-3473 SS8.3 names, introduced by this branch's own #3882 work. Delegate to listAllPhaseDirs and re-apply the field's existing lexicographic sort on top, since W007's observable order must not change. Refs #3882 * docs(#3882): document the sentinel axis and the enumeration consolidation Records listAllPhaseDirs in the Phase Locator glossary entry, and the fact that the owner already answers the all-milestones sentinel-free question when called without a cwd -- the call collectCalibrationSamples was missing. Also notes that buildAllPhaseDirNamesField now delegates rather than carrying a second readdir, and that exactly one readdirSync over the phases directory remains across the two modules. Refs #3882 * test(#3882): close review findings — real order proof, unreadable coverage, collision fixtures Refs #3882 * chore(#3882): backfill changeset PR number Refs #3882 --------- Co-authored-by: sim <sim@local> |
||
|
|
ddde001af6 |
enhance(#3873): the STATE.md schema — one owner, generated artifacts (#3880)
* test(#3873): failing-first locale parity, plus tripwires for what must not move Pins ADR-3473 §8.8 at the artifact a reader actually sees. The English STATE.md reference carries a Status lifecycle section that is missing from all four translations — the section documenting the status enum whose clobbering is #3853. The test derives the heading set rather than hard-coding the missing one, and names the locale and the heading when it fails. Two tripwires that must pass today and after. The field-drift guard still catches a re-derived fallback ladder: §8.8 instructs deleting that script, and that instruction rests on a wrong premise about what it guards, so the test stops a future reader from deleting it on the ADR's word. And last_activity's label resolution is pinned to what ships today, because it is declared in one of the two tables this phase consolidates and not the other — the consolidation must not silently pick a side. The locale test buckets under docs rather than state, which is what it tests; that bucket is allowlisted with justification rather than folded into an unrelated docs suite. It reads only markdown, so it carries no allow-test-rule marker — a marker there would suppress nothing and would grow the unverified pool against its ceiling. Refs #3873 * feat(#3873): one schema owns the STATE.md key set, three tables become projections ADR-3473 §8.8. The key set was declared in four places that had to agree by hand and already did not: FIELD_CLASSIFICATION, FRONTMATTER_BODY_SOURCE, FRONTMATTER_KEY_TO_BODY_LABEL and buildStateFrontmatter's emit behavior. One frozen null-prototype schema now declares each key's type, enum, cardinality, source, preservation, body source, body label, accepted parse shapes and whether it is emitted unconditionally; the three tables are derived from it at module load. The projections are byte-identical to the literals they replace, key order included, and the parity tests compare against verbatim copies of today's tables rather than re-deriving both sides from the schema — a parity test fed from one source proves nothing, which is how a consolidation ships a changed policy under a green test. last_activity was the live disagreement: present in one table, absent from the other. The schema declares what ships today rather than the tidier answer, and a test pins it. The schema is a leaf module and owns the four field-policy types, re-exported from state-transition so existing importers are untouched — the same split health-diagnostic-types made to break a CJS require cycle. Refs #3873 * feat(#3873): generate the schema-derived regions, parity-check the prose tables ADR-3473 §8.8's generator half. gen-state-md-docs.cjs owns marked regions in the shipped template and all five reference docs, follows gen-features.cjs's fail-closed contract, and is wired into regen:derived and lint:generated-sync. The Status lifecycle section was missing from all four translations — the section documenting the status enum behind #3853 — and is now generated into every locale. Field cardinality is a new generated table: pure schema data, no prose, so nothing to lose. The Field-reference and Status-values tables are parity-CHECKED rather than generated. Their Purpose, When-populated and Matched-text columns are genuinely hand-translated per locale, and §8.8 itself says prose stays hand-translated; generating them from an English registry would overwrite four locales' translations on every write. The row set is checked against the schema instead, so a key added to one and not the other fails, which is what field drift actually means. Building that check found last_activity_desc undocumented in all five tables. Three keys the docs describe are absent from the schema — active_phase, next_action, next_phases. They are grandfathered by name, not by wildcard, so a fourth fails: a declared gap with a forcing function rather than a silent one. Refs #3873 * fix(#3873): declare what the parsers do, and close the shape-parity gap Two declarations in the new schema described intended behavior rather than actual — the defect class this epic exists to end, committed inside the epic. Both were caught by executing the parsers instead of reading their docstrings. current_plan.acceptedShapes claimed ['N', 'N of M']. Standalone, the hybrid shape errors; the path that looks like support is parseInt truncating '2 of 5' to 2 and discarding the rest. Narrowed to ['N']. The parser is deliberately NOT fixed here: that is #3784 and PR #3791 is already doing it. When #3791 lands this row must widen, and the shape test will go red until it does — the schema and the parser cannot drift apart quietly, which is what §8.8's checked-not- generated rule is for. STATUS_LIFECYCLE_ENUM claimed to be the closed set status can hold. normalizeStateStatus passes unrecognized prose through unchanged, so it is not closed at runtime. The seven members are the canonical values it maps onto; the docstring now says that and the test asserts the real lenient contract. Closes the acceptance item that a test asserts the parsers accept exactly the declared shapes: the check is table-driven over every row carrying acceptedShapes, guarded against passing vacuously on an empty set, and fails loudly if a future row has no registered driver. Adds the unwired-label throw and the fast-check property that every projection agrees with its schema row. Refs #3873 * fix(#3873): keep the shipped template's frontmatter first, and make row 27 able to fail The remote matrix caught 12 failures with one cause. Making the template's frontmatter a generated region wrapped it in its own yaml fence ahead of the markdown fence, so extractFileTemplate and readShippedStateTemplateBody — which both match the single markdown block — found the heading first, not the frontmatter. That breaks the contract every new project's STATE.md is created from: bug #21 and epic #1969 B8 pin that the File Template block starts with frontmatter and carries gsd_state_version. The markers now sit inside the single markdown fence, so the fence opens before the frontmatter and the region still ends ahead of the heading. Same layout as before this phase, with markers embedded rather than a second fence. Row 27 existed to catch exactly this and did not, because it was writer-seeded: it asserted against the generator's own output shape, so it passed on the broken template. It now parses the fence the way production does and was verified to fail against the broken shape before being trusted against the fixed one. A test that would not have caught the bug it exists to prevent is worse than no test. The emitted-attribution failure was separate and the fragment was the wrong remedy: gsd-core/templates/state.md self-attributes under a verbatim-copy identity rule, so a diff touching it needs no acknowledgment. Fragment deleted rather than left explaining nothing. Refs #3873 * docs(#3873): how to change the STATE.md schema The phase gate was right and my docs artifact was wrong. I listed lint:generated-sync as the second enablement step, which is a verification command dressed as one, and then claimed a one-step sequence owed no how-to. The real sequence is build:lib then regen:derived, and the ordering is a trap: the generator reads the COMPILED schema, so regenerating before building regenerates against the previous schema and commits artifacts that look plausible while disagreeing with the code just written. A reference table cannot carry an ordering dependency; that is what the how-to test is for. The page covers adding, changing and removing a key, every reason code the check emits and what to do about each, what is generated versus hand-translated and why the two prose-bearing tables are parity-checked instead of generated, adding a language, and the three grandfathered keys. Indexed from docs/README.md. Refs #3873 * chore(#3873): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
3b18eff388 |
enhance(#3872): what a command reports it wrote — the transaction diff (#3878)
* test(#3872): failing-first regressions for what a command reports it wrote Pins ADR-3473 §8.7 at the consumer's output. state planned-phase advances current_phase on disk and never reports it, and reports progress.total_plans which reconcileReportedFields silently drops because it cannot resolve a dotted key against nested frontmatter. Both directions of #3818's own before/after diff, reproduced against the real CLI. Also pins the two properties the change must not break: a fully-failed patch still reports an empty updated array, which is what state.cts:607's success boolean depends on; and two content-identical writes differ in last_updated alone. That second one measured state_head NOT to be ambient — it is recomputed every write but only changes when git HEAD moved — so the provenance exclusion is a one-element set, with a companion test pinning that state_head does change when HEAD moves. Refs #3872 * feat(#3872): derive what a command reports from the transaction diff ADR-3473 §8.7. reconcileReportedFields compared the transform's own output against persisted bytes and then filtered what preservation had restored by its FIELD_CLASSIFICATION policy. Both are replaced by one comparison of persisted against the pre-write state the transaction already holds, surfaced to the command through the same caller-allocates out-param idiom divergedFields established. Both of the old directions fall out of that single comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out, and a field nobody reported but the write moved is different and appears. The classification filter is deleted, not relocated — no policy test remains anywhere in the reporting path. Reporting is at dotted-leaf granularity, enumerated from the progress.* rows FIELD_CLASSIFICATION already declares rather than by walking user data to arbitrary depth. That closes a live defect: plannedPhaseCore already pushed progress.total_plans and reconcileReportedFields silently dropped it, because a flat hasOwnProperty cannot resolve a dotted key against nested frontmatter. Current Position was lost the same way and is fixed in the same place. The exclusion is one field, last_updated, and it is by provenance rather than by classification: it is the only field measured to change on every write regardless of content. state_head was measured NOT to qualify — it is recomputed every write but only changes when git HEAD moved. Without that exclusion state.patch's success boolean, which is updated.length > 0, would be permanently true and a fully-failed patch would report success. Refs #3872 * fix(#3872): cover the matrix, and close a prototype-chain read the coverage found Review found 20 of 29 test-matrix rows uncovered. Covering them found two real defects rather than merely documenting the intended behavior. bodyLabelFor read FRONTMATTER_KEY_TO_BODY_LABEL with a bare bracket index on a plain object literal, so a field named __proto__, constructor or toString resolved to the inherited prototype member and leaked a non-string value into the updated array. Fixed with an own-property check, mirroring the discipline resolveFrontmatterPath already had. The security-relevant matrix row proved it before the fix. applyPostSyncPreservation still carried its own inline copy of the value comparison alongside the new stateFieldValuesDiffer, which is two live copies of one rule introduced by the epic that exists to remove them. Routed through the single owner. Adds the fast-check property that a field appears iff its persisted value differs from the snapshot, the string-versus-number representation boundary, dotted paths into missing parents and into scalars, deleted and added keys, and the preserve-if-placeholder pair that proves no classification test survives in the reporting path. Refs #3872 * docs(#3872): document the transaction diff on the write path The updated array's contract belongs where the write path is described. States the iff rule, leaf granularity, the single provenance exclusion and why state_head is deliberately not one, and closes with the consequence a reader actually needs: these arrays are longer than they used to be, because they used to under-report. Refs #3872 * fix(#3872): a derived leaf materializing is not a change the caller made The remote matrix caught 17 failures with two causes. The substantive one is that progress is source: disk, and the disk cannot change during a STATE.md write — the write only touches STATE.md. So a progress block appearing where the snapshot had none is the scanner populating a document that had never been synced. The bytes moved; nothing the caller did moved them. That is the same shape as last_updated one level up, so the provenance rule is generalized rather than special-cased: a field appears iff its persisted value changed for a reason attributable to this write's action, and two cases are not attributable — a field stamped unconditionally on every save, and a declared derived leaf materializing from a source that did not change. Crucially this does not consult the preservation policy, so the filter §8.7 deleted stays deleted; it uses the declared leaf set to know which keys are derived. This had a second production consumer the earlier review concluded did not exist: cmdStatePlannedPhase gates publishStateContract on updated.length, and its own inline comment predicts exactly this failure. A no-op call was publishing state.json. advancePlanNoOpDoesNotPublish genuinely encoded pre-§8.7 behavior and moves. E2 and E6 had carved out total_plans as reportable-on-materialization, an error introduced earlier on this branch rather than a pre-existing pin, and are corrected with it. Refs #3872 * chore(#3872): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
8f674281fd |
chore(#3875): sweep the spent ack fragments and automate the sweep (#3877)
* chore(#3875): sweep the spent ack fragments and automate the sweep
next has been red on every push since
|
||
|
|
1863f5569c |
enhance(#3871): the state transaction — mandatory snapshot, open()/rebuild() (#3874)
* test(#3871): failing-first regressions for the dropped curated progress block Pins ADR-3473 §8.6 / #3756 at the consumer's output: state record-session and state add-decision on an archived-milestone project drop the curated progress frontmatter entirely, exit 0, and report nothing. Reproduced against the real CLI before writing the tests, not inferred from the issue text. Also adds the unit-level probe that applyStatePreservation's preserve-always row is inert on a resyncing write, and an over-preservation guard that an empty project is never inflated. Refs #3871 * feat(#3871): make the STATE.md pre-write snapshot mandatory via open()/rebuild() ADR-3473 §8.6. StatePreservationInput's nullable preFm and the always-present preFmSnapshot were the same extractFrontmatter call, one of them nulled on resync — a policy flag baked into a snapshot. Both collapse into a single StateTransaction whose snapshot cannot be absent: openStateTransaction() applies preservation, rebuildStateTransaction() does not, and both carry the snapshot because the reporting phase needs it either way. An absent snapshot is now a construction failure; an empty one stays legal, because that is what a document with no parseable frontmatter honestly has. writeStateMd requires a rebuild transaction, which types ADR-3408 §8.3's closed exception list at both call sites (state sync, health --repair) instead of matching them as strings in a ratcheted baseline. Fixes the dropped curated progress block: an all-zero or absent derived total set is an unmeasured scan, not a measurement, so the curated block stands. Also fixes two defects surfaced while building — preserve-always reported a mutation even when it restored an identical value, and it re-entered the curated object by reference, which would alias the snapshot the next phase diffs against. Refs #3871 * fix(#3871): close the three remaining subsumed defects and restore the arm the type does not replace Review of the first two commits found four things. The guard shrink deleted the seam-bypass axis whole, but only its writeStateMd( arm became redundant. Its other arm catches a call site re-assembling syncStateFrontmatter + applyPostSyncPreservation instead of the owned composition, which the transaction type does not make unrepresentable and which #3469 found live. Restored as findCompositionBypasses, terminal rather than ratcheted. Three of the four issues this phase claims were untouched. All three are the epic's own shape and are fixed at the seam: current_phase_name is reasserted from the curated value when the caller names none, and cmdStateJson stops carrying a hand-maintained list parallel to FIELD_CLASSIFICATION and projects it instead. The construction failure that is the point of this phase had no test. Every enumerated matrix row now has one, including the measured-versus-unmeasured coercion boundary and a seeded property that no curated key is ever dropped. ADR-3473 §8.6 said the guard 'keeps only its raw-write check'. Verified against next: there was no raw-write check, and four other checks it does not name. Amended in place with the evidence. ARCHITECTURE.md separately advertised a preservation policy the code had deleted. Refs #3871 * fix(#3871): do not let the unmeasured-scan rule block an explicitly-requested resync The remote matrix caught over-preservation, the failure this phase's own negative space says must not happen. state update Progress re-derives the block from the body the caller just rewrote; on a project with no phase dirs the derivation yields zero totals, the unmeasured rule read that as 'the scan measured nothing', and the stale curated percent was restored over the resync the user asked for. preserve-always already said what the missing condition was: never overwrite unless the caller explicitly names this field. explicitProgressField carries it and is derived from shouldResyncStateProgress, not set by hand at a call site, so it cannot drift from what the caller asked for. Two defects found in the same mechanism and fixed with it. readModifyWriteStateMd enumerates its option keys, so a new option was silently dropped rather than rejected. And the raw-write axis captured its first argument up to the first comma, which lands inside a nested path.join, so a write to a STATE.md literal was invisible to it — the prove-it-can-fail test caught that one immediately. No test assertion was weakened; all three frontmatter rows encode #3242, #1969 B3 and #1972 and stand unchanged. Refs #3871 * docs(#3871): record why the raw-write check is kept, not why it was named The amendment justified findRawStateWrites as 'written because §8.6 requires it to exist', which is cargo-culting the contract and would have been the wrong reason to keep anything. The real reason is that writeStateMd acquires the STATE.md lockfile and a raw fs.writeFileSync acquires nothing, so this is a lock bypass and lost-update is the #500/#905/#1230 family — and after this phase it is the one reachable path into the file that nothing else covers. Also records why ADR-3408 §8.6's deletion of the 'clear' policy is not the precedent it looks like: 'clear' was dead vocabulary in a closed enum, this is coverage of a reachable path. Refs #3871 * chore(#3871): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
fb2d122d7f |
feat(#3841): assert gsd-tools identity on every state-mutating verb (#3848)
* feat(#3841): assert gsd-tools identity before any state-mutating verb only this package publishes. The path-based branches — a project-local install, a runtime config directory — had no such guarantee; they trusted their configured location. This closes them. Mechanism: once resolution finishes, and before any verb runs, the preamble probes the tool it picked with `runtime-identity --raw` and matches the answer with a shell `case` pattern ANCHORED to the start of the compact payload (`{"packageName":"@opengsd/gsd-core"`). An unanchored substring match accepts the decoy `{"packageName":"get-shit-done-cc","note":"@opengsd/gsd-core"}`, which any colliding package could publish. The outcome is exported as the two-valued `GSD_IDENTITY_STATUS` (`ok`/`unverified`), so the gate is asserted on a VALUE rather than on warning prose. Rollout is warn-then-fail per the #3146 ruling: `unverified` prints one line naming BOTH causes and continues, because `no_identity_verb` cannot tell a foreign package from an `@opengsd/gsd-core` older than the verb, and at rollout the old-version case is the common one. The blocker was byte budget, not design. The preamble is inlined into 112 shipped files and several sat within single-digit bytes of frozen ceilings (`gsd-verifier.md` 16 bytes, `gsd-executor.md` 33, `execute-phase.md` 234); a first attempt broke five of them. What made room was collapsing the resolver's twenty near-identical `elif [ -f … ]` arms into one candidate-list helper (`_gsd_at`), which buys far more than the assertion costs. The preamble is now 2,624 bytes against 4,500 — a net 1,876 bytes SMALLER per inlined file, so every capped file moved away from its ceiling rather than toward it. No cap raised, no size-budget exception added, no override token emitted. Resolution order, every runtime-home probe, the `unset -f gsd_run` re-source fix, the fail-closed `exit 1`, and the `CLAUDE_ENV_FILE` persistence are all preserved byte-for-byte in substring terms; the snippet still begins with `_GSD_SHIM_NAME=` and still ends with `fi`, which the parity extractors anchor on. `gsd-core/references/gsd-run-resolver.md` is re-synced byte-equal. Also fixes two stale claims found in passing: CONTEXT.md and FEATURES.md both described an `[ -x ]` guard as the load-bearing re-source defense. That guard was tried and REMOVED in #3831 — it rejected the bare function name, fell through every branch, and hit `exit 1`, which kills a sourced caller's shell. `unset -f gsd_run` is the actual mechanism. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3841): pair the anchor's brace by requiring a closed identity payload The matrix went red on `tests/new-project-mvp-prompt.test.cjs` — "new-project.md has unbalanced braces: net depth 2" — plus a knock-on report from its parent `bug #1516` describe, which is the same failure counted once at the child and once at the block. Root cause: that guard (:182-189, mirroring #3784 bd53925f) walks characters and increments on `{`, decrements on `}`, with no awareness of shell quoting. It scans `new-project.md` PLUS every `new-project/steps/*.md`, and both `new-project.md` and `steps/auto-mode-config.md` carry one inlined preamble copy — hence net 2 from a snippet that was off by exactly one. The unpaired brace was the `{` inside the single-quoted `case` pattern of the identity anchor, which is correct shell and invisible to a text scanner. Fix in the snippet, not the guard. The pattern now anchors at BOTH ends: `'{"packageName":"@opengsd/gsd-core"'*'}'`. That balances 51/51 with a brace that does real work rather than a cosmetic pair — a truncated payload whose prefix matches now fails too, where before it verified. Safe for any future additive field: a JSON object's own closing brace is always the last character, whatever type the last value has, which is pinned by two negative-space tests (a nested object and an array-valued last key must both still verify). Cost: +3 bytes, against the 1,873 the resolver fold already gave back. The alternative considered and rejected was dropping the literal `{` for a `?` glob. It balances too, but weakens the anchor from "must be an opening brace" to "must be any one character", and the anchor is the entire point. Two guards added so this cannot recur silently: - runtime-launcher-parity (F0) pins brace balance at the SNIPPET, so the next edit to that pattern fails on the file it broke instead of surfacing three files downstream in a test whose name mentions neither the launcher nor this issue. It also asserts depth never goes negative, since a `}` preceding its `{` nets to zero while being unbalanced at every prefix. - runtime-identity gains behavioral truncated-payload and trailing-garbage fixtures, so the added `}` is proven load-bearing rather than merely present. Verified: snippet 51/51 braces; new-project combined net depth 0; the seven other preamble-bearing files with nonzero depth are unchanged from merged next (their own prose, not the preamble, and not in any guard's scan set); all 112 inlined copies and the resolver reference re-synced byte-equal; sync:launcher idempotent on the second run. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3841): backfill changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
394bf384be |
fix(#3696): report the last_activity invariant and make the verdict gateable with --strict (#3844)
* test(#3696): failing-first coverage for the last_activity invariant and --strict exit status * fix(#3696): report the last_activity invariant and make the verdict gateable with --strict * fix(#3696): agree with the real reader on last_activity, and stop reporting structure as truncation * chore(#3696): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
63abcface9 |
feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools (#3831)
* feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools The predecessor package get-shit-done-cc publishes a colliding gsd-tools bin whose phases.clear DELETES where this package's ARCHIVES, and both print success-shaped output against a gitignored .planning/ -- which is how #3129 cost a user 43 phase directories with no error and nothing recoverable from git. The launcher's PATH branch now resolves gsd_run, published only by this package and self-locating via its own symlink chain to the sibling shim, instead of the colliding gsd-tools. A foreign handler becomes unreachable from PATH, and when no gsd_run is reachable the resolver fails closed rather than falling back -- that fallback was the vulnerability. This is smaller than the branch it replaces, which matters: the preamble is inlined into 113 shipped files and agents/gsd-verifier.md sits 2 bytes under a red-line size cap. unset -f gsd_run leads the preamble so a re-source is idempotent. Without it, command -v finds the shell function, returns a bare name, and the resolver falls through to an exit 1 that kills a sourced caller's shell. Adds gsd-tools runtime-identity, a manual diagnostic reporting this runtime's package coordinates over the baked package-identity (#498) and readHostVersion, with a strict total classifier: only a JSON object with an exact packageName verifies, since JSON.parse admits 0/"str"/[]/null/true. An inlined identity assertion was built and reviewed first, then withdrawn -- it breaks five frozen size ceilings and no assertion fits in 2 bytes. Closes #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3146): stop sync:launcher relocating a deliberate preamble placement Pre-existing defect, surfaced by this PR because sync is a no-op unless the snippet content actually changes. transformFile inserts the preamble into the first block that CALLS gsd_run, but gsd-core/workflows/explore.md deliberately places it in a bootstrap-only block that DEFINES gsd_run without calling it -- its own comment explains why: declining the research offer must not leave Step 5's commit call unbootstrapped. Stripping empties that block of calls, so the preamble migrated forward and broke the define-before-use invariant tests/explore-command.test.cjs pins. Reproduced on a pristine origin/next checkout with the base snippet and base file, so this was not introduced here. The insertion target now honours a block that already carried the preamble, falling back to the first calling block for files that have none yet. Adds a behavioral regression test over a two-block fixture. Also updates three runtime-launcher-parity tests that pinned the removed PATH fallback to gsd-tools. Their intent is preserved -- the PATH stub is renamed gsd_run so it is reachable by the new resolver, and the RUNTIME_DIR-wins test still asserts the stub is never invoked. Fixture shebangs move to an absolute /bin/sh, because the fixture PATH is deliberately restricted and #!/usr/bin/env sh could not resolve. Refs #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3146): backfill changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3146): document the FEATURES.md section-numbering practice The monotonically increasing section number in docs/FEATURES.md is the most frequent merge-conflict source in this repo, and it has TWO conflict cells, not one: the ### N. heading and the hand-maintained table of contents. Two PRs adding differently numbered features still collide on the TOC, so renumbering alone does not make a branch safe. This branch alone was renumbered 165 -> 166 -> 167 -> 168 across successive rebases. Adds a CONTRIBUTING section stating the practice: allocate the number last, never pre-emptively renumber, take max+1 after a rebase and update the TOC in the same commit, and never renumber someone else's section. Fork contributors are told explicitly they may leave the number to a maintainer at merge rather than chasing the counter. Agents are told to lease the allocation and to include the file in their published touched set. Records the durable fix as planned rather than pretending it exists: FEATURES.md should be generated from per-feature fragments the way CHANGELOG.md is generated from .changeset/, and the way tests/emitted-drift-acks/ works (#2914). Also renumbers this branch's own section to 168, leaving 167 to the PR already in flight. Refs #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
c933184b97 |
enhance(#3172): require a stated failing direction for every automated acceptance command (#3825)
* test(#3172): failing-first suite for the stated failing-direction probe Pins the <fails_when> pairing walk, placeholder denylist, MISSING sentinel exemption, degraded-read contract, CLI arm and the plan-authoring contract text. RED by construction: the module exports it requires do not exist yet. Executed on the remote runner. * feat(#3172): require a stated failing direction for every automated acceptance command Every runnable <automated> command now carries a <fails_when> sibling naming what output constitutes failure. A command with no expressible failure mode is not an acceptance test: it reads as rigour and is not falsifiable. - verify-command-grounding gains a failing-direction probe sharing the existing <automated> grammar, MISSING sentinel and walk guard rather than copying them - gsd-tools check verify-failure-directions <N> backs it; plan-phase dispatches it and hands the JSON to gsd-plan-checker check 8f - Dimension 8 detail extracted to references to stay under the agent size cap Verified on the remote runner. * fix(#3172): close four review findings in the failing-direction probe - MISSING_SENTINEL_RE matched an env-var assignment prefix (MISSING=1 cmd), so a real command was exempted from the new blocking gate. Tightened the SHARED constant rather than adding a second copy. - Both token regexes scanned to EOF on unclosed openers (O(n^2), 1562ms at 40k). Bodies are now non-crossing; 1ms, byte-identical on well-formed input. The pre-existing AUTOMATED_BLOCK_RE carried the same defect and is fixed here too. - probePhaseFailingDirections reported status 'ok' when one plan was unreadable, conflating 'could not look' with 'nothing to report'. - Extracted the phase-resolution block both check arms had copied verbatim. Also corrects a docs/AGENTS.md dimension list stale since #2401. Verified on the remote runner. * fix(#3172): project the planner rule onto the spawn contract, settle emitted bookkeeping The remote runner refuted the planner-side edit. agents/gsd-planner.md is frozen under a 49152-LF-char cap asserted by four suites and sat at 49,146 — six chars of headroom — so the +537 of authoring rule blew it. #3297/#3645 already settled where such a rule goes: the planner spawn contract in plan-phase.md, beside <tracked_source_paths>. The agent file is reverted to origin/next verbatim. - plan-phase.md gains <failing_direction_contract>; tests row 30 now asserts the contract there and row 30b guards the freeze in both directions - plan-phase.md growth acknowledged by APPENDING to the 3409 fragment, per the precedent that two ack sources may never name the same path - install-tree fixtures regenerated for the three new reference files Verified on the remote runner. * chore(#3172): backfill PR number into the changeset fragment pr:0 -> pr:3825 now that the PR exists. --------- Co-authored-by: sim <sim@local> |
||
|
|
596540f864 |
feat(#3227): publish machine-readable state contract at step boundaries (#3824)
* feat(#3227): publish machine-readable state contract at step boundaries Adds src/state-contract.cts, a best-effort publisher that writes .planning/state.json (contract 1.0.0) at 11 step-boundary commands, so external tools read a versioned contract instead of parsing STATE.md and ROADMAP.md heuristically. Composes existing owners rather than re-deriving: phase rows come from a new locateProgressTable extracted from deriveProgressFromRoadmap (so the snapshot can never disagree with GSD's own progress counters), milestone identity from getMilestoneInfo, and next from classifyProject. Owners are required lazily to avoid the state -> state-contract -> smart-entry -> state require cycle. Also fixes a pre-existing defect in scripts/lint-test-file-count.cjs (maintainer-approved as a second concern): testEffectivePrefix never stripped the suite qualifier, so 65 dotted test files counted against no module and 9 mis-bucketed into a shorter one. Allowlist re-baselined for the 74 files the gate can now see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3227): backfill PR number into the changeset fragment pr:0 -> pr:3824 now that the PR exists. Doc-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3227): shape hostile-name fixtures away from the scan corpus The two hostile-input fixtures used a literal phrase from scripts/prompt-injection-scan.sh's corpus, so CI's Security Scan redded on this file. These tests assert that an arbitrary phase name round-trips into state.json as inert data -- the property holds for any string, so the injection flavor is illustrative, not load-bearing. Reshaped to a hyphenated fake instruction tag, which stays hostile-looking while matching none of the scanner's patterns. Allowlisting the file was rejected: that mechanism is for suites whose subject IS injection defense, and it would blind the scanner to this whole file permanently. See DEFECT.PROMPT-INJECTION-SCAN-COLLISION. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3227): ratchet the state-contract mutation floor to its measured score The module was registered at minScore 50, the ratchet's minimum permitted floor for a newly-registered module whose score had not been measured. This PR's own Stryker shard measured 66.25% (run 32769289750, job 97565813640), so the floor moves to floor(measured) - 1 = 65, per the rule the registry documents. 66.25 is below TARGET_MUTATION_SCORE (80), so this stays a ratchet candidate: raise as the tests improve, never lower. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
fb9823e1e1 |
fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON (#3828)
* test(#3689): failing-first coverage for the ledger table/JSON agreement guard `.planning/WINDOWS.md` renders its markdown table from the fenced JSON that is its source of truth, but nothing checks the two still agree before a write overwrites the table. `windows append` / `waive` / `fixed` therefore discard a drifted cell silently, and erase a table-only row entirely, both at exit 0. Adds to tests/broken-windows.test.cjs: - five refusal cases that fail today, covering all three write commands, a drifted cell, a table-only row, and drift on a non-first row; each asserts the typed reason via GSD_JSON_ERRORS and that the file is byte-identical after the refusal, so a guard that refuses only after writing cannot pass - six anti-tightening pins that must stay green: an agreeing ledger, the first-write ENOENT path, #2893 trailing-prose preservation, #3657 3-backtick fence tolerance, escaped pipes and backslashes in a description, and the zero-entry placeholder table - a fast-check property pinning the round trip the guard depends on — extractTableRegion(renderLedger(l)) === renderTable(l.entries) — because a false refusal on a clean ledger would be worse than the bug Fixtures are built by running the real CLI and then perturbing only the table, so frontmatter and JSON stay consistent and the pre-existing counts cross-check still passes; a hand-written ledger would pass these for the wrong reason. Refs #3689 * fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON `.planning/WINDOWS.md` renders its markdown table from the fenced JSON that is its source of truth, and `writeLedgerAtomic` regenerated that table on every `windows append` / `waive` / `fixed` without ever checking the two still agreed. A hand-edited cell was silently reverted; a row that existed only in the table vanished entirely. Both at exit 0, with nothing on stdout to say so. The write seam now compares the on-disk table against `renderTable(<entries parsed from the on-disk JSON>)` before regenerating anything, and refuses with a typed `windows_ledger_table_drift` error naming the drifted row ids and the remedy. Because the check sits at the single write seam, all three commands inherit it, and the file is left byte-identical on refusal. Deliberately not enforced in `parseLedger`: hardening the read would break `windows status` and the ship gate on exactly the ledgers an operator needs to inspect to diagnose the drift. Two hazards handled explicitly, both discovered in review of the first draft: - The pre-image read now distinguishes ENOENT from every other errno, per the #1950-H2 fail-closed-on-unreadable invariant `readLedgerOrNull` already honors. A bare catch would have let an unreadable pre-image skip the guard and write anyway. - Both the entries baseline and the table extraction pass the pre-image's own frontmatter `total_count` to `locateJsonBlock`. Without that hint the no-expectation fallback binds to the LATEST fenced JSON array in the file, which is the operator's prose block whenever that prose contains one — the exact case #2893 exists for — refusing every write on a ledger that never drifted. A regression test covers it. Also extends the CONTEXT.md Broken Windows Ledger glossary entry: the table is a third projection of the same source, cross-checked at the write seam, and the frozen REASON enum gains WINDOWS_LEDGER_TABLE_DRIFT. Fixes #3689 * fix(#3689): bind prose preservation to the pre-image's own ledger block Found while reviewing the table drift guard: the #2893 trailing-prose preservation in `writeLedgerAtomic` passed `ledger.total_count` — the POST-mutation count — as the disambiguation hint for a lookup over the PRE-image. On an append the pre-image holds N entries while the hint says N+1, so the hint can never match and `locateJsonBlock` falls through to its last-array-shaped-span fallback. When the operator's trailing prose itself contains a fenced JSON array — the ordinary case #2893 was written to protect — that prose block wins the fallback. The preserved region is then computed from the prose fence rather than the ledger fence, and everything between them, including the operator's own text above the array, is silently dropped on the next write. Reproduced against the real CLI: a prose block reading "Operator notes above the array, IMPORTANT DO NOT LOSE THIS TEXT." plus a fenced 3-element array came back empty after one `windows append`. Both the prose lookup and the drift guard now share one pre-image-derived `preImageExpectedTotal`, taken from the pre-image's own frontmatter, so they bind to the same and correct block. The existing trailing-prose regression test is strengthened to assert the prose survives byte-for-byte rather than merely that the command exited 0 — asserting only the exit code is why this was invisible. Refs #3689 * fix(#3689): anchor table extraction on the header row, not a line-prefix scan Independent review found the drift guard could brick a ledger nobody had hand-edited. `validateDescription` accepts a description containing a raw newline, and `renderTable`'s cell escaping covers backslash and pipe but not newlines — so such a description renders a row that physically spans two file lines, the second of which does not begin with `|`. `extractTableRegion` bounded the table by walking backward over the contiguous run of `|`-prefixed lines, so it stopped at that split. In the common case where the row's tail is the last line before the fence it returned null, and every subsequent append/waive/fixed was refused with "table region could not be located" — permanently, with no CLI recovery path, on a ledger that never drifted. A false refusal is worse than the bug this guard exists to fix. The region is now anchored on the header row `renderTable` always emits, running from its last line-start occurrence to the end of the pre-fence text. The boundary is the fence rather than a line prefix, so a multi-line row is captured whole, re-renders byte-identically, and compares equal. The header literal is hoisted to one constant both `renderTable` branches and the extractor share, so the two surfaces cannot drift apart. Deliberately unchanged: `cell()` and `validateDescription`. The cosmetic corruption a newline causes in the rendered table is pre-existing, and either escaping it or rejecting the input would change what existing ledgers render to or what input is accepted. Also closes a coverage gap the standards review raised: the non-ENOENT pre-image read branch — the one that stops an unreadable file from bypassing the guard — now has a behavioral test that injects EACCES by monkeypatching `fs.readFileSync` for that one path and restoring it in a `finally`, never by `chmod 0o000` (root ignores mode bits, so that would pass with zero coverage). The #3689 property generator no longer strips newlines out of descriptions, which is why this was invisible to it. Refs #3689 * chore(changeset): backfill PR number for #3689 fragment * chore(changeset): backfill PR number for #3689 fragment * fix(#3689): terminate the header scan when the match sits at index 0 `extractTableRegion`'s backward search for the table header could loop forever. On a rejected match at index 0 it set `searchFrom = idx - 1`, i.e. `-1`; `String.prototype.lastIndexOf` clamps its position argument into `[0, length]`, so the next iteration searched from 0, found the same match, rejected it identically, and set `-1` again. The loop made no progress. Reachable only through the exported `extractTableRegion` — `writeLedgerAtomic` reaches it after `parseFrontmatterStrict` has already succeeded, so the candidate region begins with the `---` frontmatter fence and a match at index 0 is impossible. Latent rather than live, but an exported `for(;;)` that can fail to advance is not something to ship. Confirmed by running the pre-fix compiled function on `TABLE_HEADER_LINE + 'X\n' + <a valid json fence>` as a backgrounded child: it was still alive after five seconds having printed nothing, and had to be killed. Post-fix the same input returns `null` promptly — correct, since the sole header occurrence fails the end-of-line test and no valid header exists. A regression here would stall the suite rather than fail it, so the new test also asserts the returned value rather than relying on termination alone. No wall-clock assertion is involved. Refs #3689 * test(#3034): publish the lane trace before the done-file that releases dependents `preservesSelectionOrderParallelDespiteCompletionOrder` forces a reverse completion order with a dependency chain rather than sleeps: each stub lane waits on `done-<dep>` before finishing. It then ended with touch "$RUN_DIR/done-$slug" echo "end:$slug" >> "$TRACE" Those are two unsynchronized operations in separate shell processes. A dependent's `wait_for_file` unblocks the instant the upstream's `touch` lands, but the upstream's own `echo` has not necessarily run — so if the upstream is descheduled between the two, the dependent can run its whole body and append its `end:` line first. The done-file was published before the state it signals. Observed on the remote runner as `[end:claude, end:codex, end:gemini]` where selection order demands `[end:claude, end:gemini, end:codex]`. The failure was in the fixture's own self-check, before it reached the assertion #3034 exists to make. Not a flake and not a wall-clock margin: this branch passed the full suite twice at 14f494644 and 90c5d7a03, and the only delta in the failing run was one added test in tests/broken-windows.test.cjs — an unrelated module. Adding load elsewhere in the suite was enough to invert it, which is what a real race does. Swapping the pair establishes a genuine happens-before: anything a dependent can observe is written before the file that releases it. A comment records why, so the order is not tidied back. The production path is unaffected and was independently confirmed correct — `invoke_reviewers` joins every lane with `wait`, then aggregates by iterating DISPATCH_SLUGS in selection order, reading per-slug result files. It consumes no completion-order signal at all. Refs #3034 --------- Co-authored-by: sim <sim@local> |
||
|
|
a84f756303 |
fix(#3078): sweep all-spent ack fragments on next, name the collision remedy (#3823)
* fix(#3078): sweep all-spent ack fragments on next, name the collision remedy
`guard-no-ack-on-next` only ever watched the legacy tests/emitted-drift-ack.json.
#2914 exempted the fragment directory on the premise that a persisting fragment
"cannot conflict with any other PR". Fragments do not share a FILE, but they do
share a PATH KEY SPACE, and a path claimed by two sources is a hard failure in
the same script -- so a fully-spent fragment on next owns keys it can no longer
gate, and the next PR to grow one of those paths can declare it neither there
(spent) nor in its own fragment (duplicate). Measured at the sweep: 45 fragments
owning 403 paths, up from 13/272 at triage 19 days earlier.
- `assertNoAllSpentFragments` fails a fragment only when EVERY surviving entry is
spent against the copy at HEAD^, so a partially spent fragment -- and the
re-arm-by-appending route #2639/#2993 ship on -- keeps working.
- `ackProse` duplicates the gate's zero-width/whitespace stripping across the
scripts-ship/tests-do-not line, bounded by a prose-parity test.
- The guard job's checkout takes fetch-depth: 2; at depth 1 HEAD^ is absent and
every fragment reads as brand-new, i.e. the guard passes vacuously.
- The duplicate-ack error now names both resolutions, since the guard is
post-merge by design and cannot stop the colliding PR.
- All 45 spent fragments deleted, 0000-legacy-migration.json included, and the
three tests that pinned its permanence corrected.
Verification is the remote runner (gsd-test), not a local suite.
Closes #3078
* fix(#3078): make the prose-parity test two-sided, cover the git seam, base on the pre-push tip
Three review findings, all fixed:
- The parity test was a tautology: it checked ACK_INVISIBLE against a
hardcoded list matching its own definition, never against the gate. The
gate's INVISIBLE and its reason normalizer (hoisted out of diffEmitted as
normalizeAckReason) are now exported for that sole purpose, and the test
sweeps 0x00-0xFFFF against both surfaces. Mutation-checked: adding a
codepoint to one side and not the other now fails.
- resolveBaseRef, readFragmentAtRef and assertUsableBaseRef had zero direct
coverage -- the tests reimplemented the git reads in a local helper, so the
ls-tree-vs-show discrimination, the root-commit fallback and the
option-injection guard were never executed. All are exported and tested
against real temp repositories now, plus an end-to-end --base-ref subprocess.
- HEAD^ is not 'the state of next before this push'. The default branch allows
REBASE merges, so one push can carry N commits, and a 2-commit rebase-merge
whose first commit adds a fragment would be told to git rm it on the very
push that introduced it. CI now passes github.event.before via --base-ref and
fetches it explicitly; HEAD^ remains only the local fallback.
Also adds the safe.directory guard every other git call in this repo carries
(#2767), and stops naming the deleted migration fragment by filename in
CONTEXT.md, which tripped lint-removed-but-needed.
Refs #3078
* fix(#3078): keep the fragment directory alive after the sweep empties it
Sweeping every fragment leaves the directory untracked, and check-glossary-refs
then fails: CONTEXT.md references tests/emitted-drift-acks, which no longer
exists. The empty directory IS the intended steady state, so it has to survive
its own remedy.
Adds tests/emitted-drift-acks/README.md documenting the create/use/delete
lifecycle where a contributor actually meets it, matching the existing
tests/qa/smell-acks/README.md precedent. Every reader filters on .json, so the
README is invisible to the gate.
Also sweeps #3809's ack fragment, which the rebase onto origin/next brought in
and the new guard immediately reported as all-spent -- its own remedy applied.
Refs #3078
* fix(#3078): guard the added tests' git calls, drop a second fragment-existence pin
Both defects surfaced by the remote runner (linux-node24, 4/37445 failed).
- The new --base-ref E2E test ran `git rev-parse HEAD` against the checkout
without the #2767 safe.directory guard. The runner mounts the repo at a path
owned by another uid, so git refused every operation there with 'detected
dubious ownership'. Every git call the new tests make now names its own
specific directory as safe, via one local helper, mirroring safeDirArgs in
helpers/emitted-runtime.cjs.
- tests/agent-tracked-source-rule.test.cjs pinned the existence and contents of
the 3645 and 3409 ack fragments. That is a merged PR's paperwork, not live
behavior: once the growth is in next's baseline the acks are spent and this
PR's guard sweeps them. The third assertion pinned the hand-appended
workaround for the exact collision #3078 removes. Deleted; #3645's real
protection is the two behavioral tests above it, untouched.
Also restores #3809's ack fragment, which merged one commit before this branch.
Deleting an ack in the same window as its introducing PR races any consumer
whose baseline predates it -- the runner's container proved it, resolving
origin/next to
|
||
|
|
31fcb833ec |
fix(#3679): gate pr-branch verify on planning-tree deletions (#3803)
* test(#3679): failing-first rows pinning planning preservation and the verify deletion gate * fix(#3679): gate pr-branch verify on planning-tree deletions * test(#3679): extract hashes via rev-parse and de-vacuate the pure-code pin * fix(#3679): close review findings — merged ack, pinned prose gate * fix(#3679): close two-axis review findings — no-renames gate, structural pin * chore(#3679): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
314ea20fa4 |
fix(#3663): fold path casing only on win32 in the w027 active-worktree check (#3793)
* test(#3663): failing-first rows for w027 path-casing normalization * fix(#3663): fold path casing only on win32 in the w027 active-worktree check * fix(#3663): close review findings — seam-owned compare key, deterministic case pin * chore(#3663): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
4af59f8dd3 |
fix(#3662): resolve managed hook node runners at hook-fire time (#3790)
* test(#3662): failing-first suite for runtime-resolving hook runners * fix(#3662): resolve managed hook node runners at hook-fire time * fix(#3662): close review findings and document the resolver * fix(#3662): close adversarial and security review findings * chore(#3662): backfill changeset pr number * test(#3662): honor win32 skip return and platform-aware sh runner pin * test(#3662): pin the bare win32-claude sh-hook shape omitting the bash runner --------- Co-authored-by: sim <sim@local> |
||
|
|
107eb8c1d9 |
feat(#3753): run docs guards on the PR that changes the docs they read (#3787)
A PR whose diff is entirely under docs/ runs zero tests, so a guard whose INPUT
is shipped prose cannot protect the PR lane of the diffs it exists to check. Its
only firing opportunity is after merge, on the shared branch -- which is how next
went red on
|
||
|
|
622f43353c |
fix(#3299): tracer feedback gate honors workflow.human_verify_mode (#3390)
* fix(#3299): tracer feedback gate honors workflow.human_verify_mode
The tracer feedback gate (#2294) predates `workflow.human_verify_mode`
(#3309, whose scope was the planner and verifier only), and branched on
auto-mode alone. Under the documented `end-of-phase` default an
interactive run therefore halted after EVERY `type="tracer"` task,
synthesizing a `checkpoint:human-verify` no planner ever emitted and
asking the user to retype a verdict the executor had just computed —
at the cost of a full executor cold-start each time.
Planner-side suppression cannot reach this halt because the executor
synthesizes it at runtime, which is why #3309 did not close it.
The gate now branches on HUMAN_VERIFY_MODE in the interactive path:
under `end-of-phase` an automated-only tracer `<verify>` is re-run and,
on success, expansion continues with no checkpoint. HALT-on-failure is
unchanged. `mid-flight`, `gate="blocking-human"`, and tracers carrying
genuine `<human-check>` evidence all still stop; the autonomous branch
is untouched.
`--default end-of-phase` on the config read is load-bearing, not
decorative: `workflow.human_verify_mode` is absent from SCHEMA_DEFAULTS,
so a bare `config-get` exits non-zero with `Key not found` on any
project whose config.json predates #3309 — which is the reporter's
exact config and every pre-existing project.
Both copies of the rule (workflows/execute-plan.md and
agents/gsd-executor.md) are updated together; the reference doc records
the seam and the human-check-still-halts rationale so it cannot recur.
Fixes #3299
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): add changeset
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): reconcile the canonical schema table and the stale acceptance test
Review round 1 (trek-e) — three items, all in the drift class this PR is
about, two of them landed inside this PR's own diff.
1. docs/reference/plan-md.md:233 — CONTEXT.md names this file the canonical
schema reference for the tracer task-type contract, and its Task-types row
still claimed interactive runs unconditionally present a
checkpoint:human-verify. CONTEXT.md and docs/AGENTS.md were updated in the
first round; this one was missed, so the authoritative reference was the
wrong answer. The row now carries the human_verify_mode-conditional
behavior and points at the canonical precedence chain.
2. tests/tracer-bullet.test.cjs — the docs assertion only checked that a
tracer ROW EXISTS, never its content, which is why CI could not see the
drift. It now asserts the row's actual claims and rejects the pre-#3299
wording. Separately, the #1945 acceptance test named 'interactive run emits
checkpoint:human-verify after the tracer' kept passing only because its
substrings still occur in the fallback clause, while its name asserted the
opposite of shipped behavior. Renamed and narrowed to what #1945 still
guarantees, plus a new interactiveIsConditional pin so the unconditional
prose cannot be restored under a passing substring check.
3. plan-md.md's <verify> row now documents that the legacy bare-text form
(valid, and still shown at :179) does not reach the #3299 auto-continue —
only a <verify> carrying <automated> does — so the benefit is silently
unreachable for tracers using that format.
Mutation-verified: reverting the plan-md row fails 1 test; reverting the
executor's interactive branch fails 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): make the tracer gate reachable from the planner template, and bind the assertions
Peer review round 3 found two Majors, both verified by reproducing the
mutation before fixing.
MAJOR 1 — the fix was largely inert on its own default path.
agents/gsd-planner.md's Nyquist Rule (:191) says every <verify> includes
<automated>, but the tracer-specific template twelve lines later emitted the
legacy bare-text form. The gate auto-continues only on a <verify> carrying
only <automated>, so every tracer produced from the canonical template fell
to the STOP fallback and #3299's benefit was unreachable for exactly the task
type it targets. Template now wraps in <automated>; a contract assertion pins
it so the two cannot drift apart again.
MAJOR 2 — the new assertions did not bind condition to action.
Appending 'Nevertheless, interactive runs always present a
checkpoint:human-verify' to the canonical row, and 'then immediately STOP and
return a checkpoint:human-verify' to the auto-continue clause in BOTH
operative copies, restored unconditional interactive checkpointing and left
the suite 35/35 green. Every required keyword still matched. Fixed by:
- clause 2 must now contain no STOP outcome and emit no checkpoint at all —
'never a checkpoint' has to be true OF the clause, not merely stated in it;
- interactiveIsConditional replaced with the ordered-clause parse plus the
same no-STOP property, instead of proving only that HUMAN_VERIFY_MODE
appears somewhere on the line;
- the plan-md.md Autonomy cell is now pinned EXACTLY rather than by keyword
presence. Deliberately brittle: CONTEXT.md names that table the canonical
schema reference, so a wording change must be a conscious edit in both
places.
Mutation-verified after the fix: the combined semantic regression now fails 3
tests; reverting the planner template fails 1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): exact-pin the safety clauses instead of blacklisting outcome verbs
Peer review round 4. Blacklisting did not hold, twice over:
- Round 3 banned literal STOP and the 'return a'/'present a' checkpoint
forms in the auto-continue clause. Round 4 defeated that by appending
'then pause and invoke checkpoint_protocol with a checkpoint:human-verify
before expansion' — none of the banned tokens, same restored interruption
after every successful tracer. 36/36 passed.
- The planner guard looked for <automated> anywhere inside <verify>, so
'<verify>[...]<!--<automated>--></verify>' satisfied it while leaving the
legacy bare form operative. 107/107 passed across tracer, planner and the
three size-cap suites.
Synonyms are unbounded; the clauses are not. Both are now pinned exactly on
normalized whitespace, the same approach already proven on the plan-md.md
Autonomy cell, with defence-in-depth checks behind them: no checkpoint-emitting
or blocking outcome in any wording inside clause 2, and the planner's <verify>
body must be exactly one non-empty <automated> child with no commented markup.
These pins are deliberately brittle. Each is a safety contract, so changing the
behavior must be a conscious edit in both the prose and the expectation.
Mutation-verified: the synonym-checkpoint mutation fails 1; the commented-out
wrapper fails 1; the round-3 literal-STOP + contradictory-doc-row regression
fails 3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): strip comments, require uniqueness, pin whole regions
Peer review round 5. Exact-pinning one clause was still bypassable two ways,
both reproduced before fixing (each left the suite fully green):
- COMMENTED DECOYS. Put the correct text in an HTML comment followed by a live
wrong copy: every extractor selected the commented decoy. Worked against the
planner template, the canonical plan-md.md row, and both executor branches.
- SURROUNDING OVERRIDE. Insert 'after every tracer, pause and invoke
checkpoint_protocol before expansion, regardless of the mode-specific rules
below' immediately ABOVE the pinned clause, or 'ignore row 3; always wait for
approval' below the canonical table. The pinned text was untouched, so
equality held while the shipped meaning inverted.
The shape that holds, applied to every operative surface:
1. strip HTML comments BEFORE selecting, so a decoy cannot be chosen;
2. require the structural anchor to occur EXACTLY ONCE, so a live second copy
cannot hide behind a correct first one;
3. pin the ENTIRE decision region, not one clause, so no unparsed prefix or
suffix can override what the pin proves.
Applied to: the executor's whole tracer branch, execute-plan.md's whole
dispatch line, checkpoints.md's whole precedence section, and plan-md.md's
Autonomy cell.
Also addresses the round-5 Minor: the planner template is now asserted
STRUCTURALLY (exactly one <verify> in the fenced block, body exactly one
non-empty <automated> child) rather than pinning the descriptive placeholder
verbatim, so behavior-preserving wording changes no longer false-fail. The
clause and section pins keep their exact form — those have a safety rationale
the placeholder copy does not.
Mutation-verified, all six rounds: override-above-clause 1; commented decoy row
1; commented decoy branch 1; ignore-row-3 override 1; synonym checkpoint 1;
commented-out wrapper 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): drop the superseded exact-placeholder planner assertion
Peer review round 6, Minor. The round-5 brittleness fix ADDED a structural
planner assertion but left the old exact-placeholder one in place, so the
over-brittleness it was meant to remove was still live: rewording the
descriptive placeholder while preserving exactly one non-empty direct
<automated> child failed the old test and passed the new one.
Removed the old test. The structural assertion is the real contract — the gate
auto-continues on the SHAPE of the verify, not on the wording of a placeholder.
Verified both directions: a behavior-preserving reword now passes; reverting the
template to bare <verify> still fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): select operative prose via parsePredicates, not a hand-rolled scanner
Peer review round 7. I had judged the round-6 selector bypass adversarial-only
and out of scope, intending to disclose it. Both premises were wrong, and the
review said so:
- 'Needs new src API' — false. parsePredicates is ALREADY a public export and
internally uses the repo's interleaved fence/comment scanner. Instrumenting
candidate lines as throwaway predicate declarations borrows that scanner with
no src change at all.
- 'Adversarial-only' — false, and this is the part that mattered. Two ORDINARY
edits silently turned the guards into decoy checks:
* a forgotten '-->' comments the live rule through to EOF, and the
balanced-only stripper still saw and accepted the commented rule;
* a normal fenced documentation example of the rule, plus a whitespace-only
reformat of the live list item, made the selector choose the example.
Neither needs intent. A dangling comment is a typo; a fenced example is good
documentation. Together they reproduce exactly the accidental drift #3299 came
from — with CI green.
The selection layer now defers to parsePredicates for operativeness, uses
whitespace-tolerant anchors so a reformat cannot decouple the live line from its
pin, extracts regions by operative line index rather than string search, and
carries a self-guard test proving fenced / balanced-commented /
after-unclosed-comment copies are all excluded. The helper also ignores indexes
it did not inject, so a pre-existing GSDTEST.CANDIDATE line cannot pollute it.
Verified both ordinary-edit scenarios now fail the suite (each was green before).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): close the operative-selection gaps the maintainer blocked on
trek-e's Blocker: the operative-line selection layer had three gaps, all
reachable by ordinary future doc edits rather than sabotage. He independently
found a fourth I had not disclosed. All are fixed.
1. INDENTATION PROMOTION (his find, not in my disclosure). The instrumentation
replaced a matched candidate with an UNINDENTED marker regardless of the
original line's indentation. A 4-space-indented CommonMark code block is not
skipped by parsePredicates (it accepts indented declarations by design), so
stripping the indent PROMOTED an indented decoy to operative — the exact
inversion of the guard's purpose. The marker now preserves the original
indent, and a candidate that is itself indented 4+ spaces is never injected.
2. NO SET MEMBERSHIP. The filter accepted any in-range integer, so a
pre-existing literal GSDTEST.CANDIDATE=<valid index> in source text could
pollute the count. Now filters on a Set of the indexes actually injected on
this call.
3. RAW FENCE SELECTION (planner). The template test matched the first raw
```xml fence after the marker with no fence/comment awareness — the one
selection in the suite that was not operative-aware — so a commented-out
decoy template between the marker and the real one would be selected while
the live template regressed. The opener must now be operative AND the first
non-blank line after the marker.
4. RAW END ANCHOR (regionFrom). The end anchor was tested against raw lines, so
a fenced example containing a ### / <type line truncated the pinned region
early — a false FAILURE on a legitimate doc edit. End anchors now go through
the same operative filter as start anchors.
Mutation-verified: the indented-decoy + whitespace-varied-anchor combination
and the commented-out fence decoy each now fail the suite (both passed clean
before). Truncation is confirmed fixed by extraction — the region spans the
full section and retains the content following a fenced example, where it
previously stopped at it.
Note on the remaining brittleness: adding a fenced example INSIDE a pinned
region still fails the whole-region exact pin. That is the intended tradeoff
for a safety contract, not the truncation defect, and is called out as such.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): allow-list operative indentation; pin marker provenance
Review round 9.
BLOCKER — the round-8 indentation guard was written as a DENY-list,
/^(?: {4,}|\t)/, and CommonMark has more indented-code forms than that
enumerates: " \t", " \t" and " \t" all open an indented code block and all
slipped through, so an indented decoy was still promoted to operative while the
live rule regressed (34/34 green). Inverted to an allow-list — only 0-3 literal
spaces is ordinary block indentation; anything else is code. Enumerating the
bad shapes was the error, not the specific regex.
MINOR — the injected-index Set validated the marker's VALUE but not its SOURCE.
A pre-existing literal `GSDTEST.CANDIDATE=<n>` could name an index that some
other (skipped) candidate had contributed to the set, and be accepted. Now also
requires p.line - 1 === Number(p.value): the predicate must have been parsed
from the line it names.
MINOR (false negative) — ```xml title=x is a valid CommonMark info string, and
requiring exactly ```xml failed the suite (33/34) on a behavior-preserving edit.
Both the opener assertion and the extraction now accept an info string.
Mutation-verified: the mixed " \t" decoy and the forged-provenance marker each
now fail; the info-string fence no longer false-fails.
KNOWN LIMITATION, disclosed on the PR rather than papered over: parsePredicates
is a predicate parser, not a general CommonMark operativeness oracle. Two
standards-valid constructs still read as operative — a lazy blockquote
continuation line (state opens only on a line that literally starts with ">"),
and a comment opened mid-line ("prose <!--", where state opens only when the
trimmed line STARTS with "<!--"). Closing those means either teaching the shared
src/context-predicates.cts about container/lazy-continuation state — a change to
a module every health rule consumes, well outside a tracer-gate fix — or
hand-rolling a CommonMark parser inside a test, which is how this suite got into
trouble in the first place. Left for the maintainer to scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): re-arm the execute-plan.md emitted-drift ack after the base merge
The #3299 ack rode on tests/emitted-drift-acks/2652-quick-diagnose-dispatch-isolation.json,
which upstream retired in
|
||
|
|
a44d513566 |
fix(#3712): confine in-process installs to a sandboxed HOME (#3725)
* fix(#3712): confine in-process installs to a sandboxed HOME
A runtime kind may declare a global `home` override resolved from os.homedir()
rather than from the caller's configDir — codex's skills kind (`home: ".agents"`,
ADR-1239 / #2088) is the only live case. Sandboxing configDir/targetDir does not
contain it, and assertDestWithinConfigHome cannot see the class: that gate
confines a destSubpath to whatever root it is handed, and here the root IS the
escaped home. So an in-process caller that forgot to sandbox HOME wrote to, and
pruned gsd-* entries from, the developer's REAL ~/.agents/skills.
tests/agent-descriptor-parity.install.test.cjs's K1 loop did exactly that: it
iterates every agents-kind runtime (codex included) with a sandboxed targetDir
and an un-sandboxed HOME. Reproduced against a canary home on next @
|
||
|
|
004e9dd741 |
fix(#3007): resolve Codex reasoning effort per model and make every clamp visible (#3765)
* test(#3007): failing-first suite for per-model Codex effort capability RED by construction. Binds to behavior renderEffortForRuntime does not yet have: an optional third `model` argument, a per-model advertised-level table, `max` passing through instead of clamping to `xhigh`, `minimal` clamping to `low`, `ultra` rejected outright, and clamp visibility (`requested`/`clamped`/ `reason`) so a downgrade is legible from resolver output rather than silent. Two of these pin defects that exist on next today: - `max` is discarded. Both Codex models whose catalog entries are retrievable (sol, luna) advertise `max`; GSD clamps it to `xhigh` and reports nothing. - `minimal` is emitted to a model that refuses it. providerPresets.openai. haiku.low pairs gpt-5.6-luna with reasoning_effort "minimal", and luna's advertised floor is `low`. GSD is sending a value into a document Codex itself validates. The parity test is what pins that fixed, and it names the offending path/model/effort when it trips. Also corrects tests/model-resolver.test.cjs:351, which asserted renderEffortForRuntime('codex','max').value === 'xhigh' -- the defect pinned as though it were a contract. ADR-443 recorded "Codex has no max" as fact and it was true when written; Codex has since added both `max` and `ultra`. That is a stale premise, so the assertion is corrected here rather than worked around. The property test asserts the invariant the whole change exists for: a rendered effort is always a level the target model actually advertises, or an explicit rejection. There is no third outcome. * fix(#3007): resolve Codex effort per model, and make every clamp visible Codex declares supported_reasoning_levels per MODEL and validates against it, so a single per-runtime capability set cannot be right for all of them. GSD's was wrong in both directions at once. `max` reaches Codex now. ADR-443 recorded "Codex has no max" as fact and clamped max -> xhigh on that basis; it was accurate when written, and Codex has since added both `max` and `ultra`. Every Codex model whose catalog entry is retrievable advertises `max`, so the clamp was discarding a level the provider supports, silently, on the most-used path. `minimal` stops reaching Codex. No Codex model advertises it -- both retrievable entries floor at `low` -- yet providerPresets.openai.haiku.low paired gpt-5.6-luna with reasoning_effort "minimal". GSD was writing a value the receiver validates and refuses into a file the receiver reads. Being unconservative in what you send is the half of Postel's rule with no defensible reading, so that preset is corrected and a parity test pins it. `ultra` is refused rather than laddered. Codex's own catalog calls it "Maximum reasoning with automatic task delegation": at ultra, effective_multi_agent_mode returns Proactive and Codex spawns sub-agents on its own initiative, underneath GSD's orchestration rather than inside it (#2167). It is a mode switch, not a reasoning depth, so it is not added to the universal ladder -- which stays provider-agnostic by ADR-443's design -- and it is rejected even for gpt-5.6-sol, which does advertise it. Clamping it down to `max` was considered and rejected: that silently discards what the user actually asked for. Clamping is now visible. RenderedEffort carries requested/clamped/reason and resolve-execution surfaces them. The previous table clamped correctly but invisibly, so a user asking for `max` on Codex had no way to find out they were getting `xhigh` -- exactly the failure mode the robustness principle's modern critique warns about, and why "be liberal" has to mean "liberal and loud". Also closes a latent trap found while reviewing the implementation: the clamp-up loop walks the ladder upward, and for a future model advertising `ultra` but not `max` it would have selected `ultra` as the clamp target -- re-entering by the back door the mode the rejection above exists to keep out. A clamp may never produce a value that a direct request for that value would refuse. Unreachable with today's catalog, which is why no test caught it; a test now asserts the invariant directly. Signature stability is preserved: the third `model` argument is optional and the two-argument form still resolves, against the family baseline. That form's BEHAVIOR does change for `max` and `minimal`, and it must -- keeping the old answer would have fixed the defect only where a model happened to be threaded through and left it live everywhere else. tests/model-resolver.test.cjs:351 asserted the defect as if it were a contract and is corrected here rather than worked around. * fix(#3007): close every review finding on the Codex effort alignment Two isolated reviewers, correctness and security. Both found the same two blockers, and the per-model work was inert on every surface that matters until this commit. BLOCKER — resolve-execution never passed the model and discarded the clamp. cmdResolveExecution called the two-argument form and emitted only effort_rendered/effort_param/effort_propagation, so the per-model table was unreachable from production code (tests were its only caller) and requested/ clamped/reason were computed and thrown away. Requested outcome 3 names "the effective rendered effort in resolver output" specifically, so the feature was unmet on the exact surface the issue asks for. Now passes the resolved model and emits effort_requested / effort_clamped / effort_clamp_reason, flat, matching the existing key convention rather than introducing a nested object. BLOCKER — the docs described output that did not exist. CONFIGURATION.md showed a nested {"effort": ...} sample; the real result is flat and those keys were absent entirely. A reference doc asserting a JSON path a reader can copy is worse than no doc. Corrected against the actual emitted key set. MAJOR — the argv channel still shipped both original defects. EFFORT_ARGV.codex kept minimal in its supported set and still clamped max down to xhigh, so the invocation-time and install-time channels disagreed about the same runtime's capability: --host codex with max emitted xhigh while the generated TOML said max. This is the repo's documented generative-fix-divergence class, so both tables now cross-reference each other and a parity test fails if they ever diverge again. MAJOR — malformed catalog data failed OPEN and could crash the CLI. A null _baseline became an EMPTY Set that is nonetheless truthy, so the nullish fallback never fired and every effort rendered as null. And a non-array value made the Set constructor throw at module load — model-catalog.cjs is required across the whole CLI, so one bad JSON value killed every command, not just codex effort. Guarded on size and filtered to array values; both degrade to the hardcoded baseline. MAJOR — value widened to a nullable string with two consumers left behind. runtime-artifact-conversion passed it straight into injectEffortFrontmatter (a null effort key in generated frontmatter); install-effort-resolver still declared a non-nullable return, a structural lie that silently defeated null checking. Both corrected, both omitting the key on null — the same posture as 'inherit', where omission means "follow the host default". MAJOR — the per-model table is inert today, and the docs now say so. All three shipped models advertise the same usable range and ultra (sol's only differentiator) is rejected for every model, so no observable output differs by model. The table stays because Codex declares capability per model and the sets are free to diverge — a single per-runtime assumption is precisely what went stale and produced this issue — but overselling it as a visible per-model feature would have been the same class of error as the doc blocker above. Tests: three passed under a full revert and are strengthened rather than deleted, since each guards a real contract (#3533's inherit rule, the undeclared-host rule, off-ladder handling) — they now also assert the clamp-visibility fields, which only exist after this change. The fast-check property is kept for its shrinking, and a deterministic nested loop over the full cross-product now sits beside it so coverage is exhaustive rather than sampled. Also folded in earlier: bin/install.js generated the Codex TOML with the two-arg form and would have written a literal null reasoning effort on the ultra path; CONTEXT.md's Model Catalog Module glossary entry now records CODEX_MODEL_EFFORT. The installer defect was found by the co-change gate, not by a reviewer — install.js is a historical co-change partner of model-catalog.cts that this diff had not touched. * test(#3007): correct assertions that pinned Codex's stale effort premise Thirteen pre-existing tests encoded "Codex has no max" as fact and failed on the shipped commit. Every one is a stale pin, not a defect: each was probed against the built module before its expectation was changed, and none failed for a reason other than this premise correction. Kept as its own commit per CONTRIBUTING — a test-fixture correction made stale by a production change must not ride inside another commit, because the release-sdk hotfix cherry-pick filter routes by subject prefix and a correction buried under the wrong prefix ships a half-state (v1.42.3, #3621). The most valuable one was tests/model-resolver.test.cjs's cross-provider validity invariant, which hardcoded the Codex enum as `minimal|low|medium|high|xhigh` and failed with "real API would 400". That message is now false in both directions: Codex accepts `max`, and rejects `minimal`, which no model advertises. The enum is corrected to `low|medium|high|xhigh|max` and the guard is kept intact — it is exactly the "would the real API refuse this" check worth having, and it was right to fail here. It simply carried the stale fact in its own fixture. Test NAMES were corrected alongside their assertions wherever the name asserted the old behavior — "max is Anthropic-only", "max clamps to xhigh", "minimal passthrough". A renamed test that still claims the old thing is worse than a failing one, and a green test whose name states a falsehood is how the next reader inherits the wrong premise. Both channels are covered: install-time (renderEffortForRuntime, and the generated .toml in install-runtime-artifacts) and invocation-time argv (effort-surface-axis). They were deliberately brought into agreement in this change, so their assertions had to move together. Each site carries a #3007 comment recording that Codex gained max/ultra and that capability is declared per model, so a future reader can tell this was a deliberate premise correction rather than a test bent to fit an implementation. * test(#3007): separate the effort-precedence case from the clamp case The previous stale-assertion pass over-corrected one test. It saw `effort: { default: 'max' }` on codex expecting `effort_rendered: 'xhigh'`, assumed the xhigh came from the max→xhigh clamp #3007 removes, renamed it to "max passes through" and changed the expectation to `max`. The remote runner disagreed. Reproduced against the real CLI: with that config and `gsd-planner`, the resolver emits `effort: "xhigh"`, `effort_requested: "xhigh"`, `effort_clamped: false`. The xhigh is produced by effort-resolution PRECEDENCE — gsd-planner is heavy/opus tier and its routing-tier default outranks `effort.default` — so `max` never reaches the renderer at all. The test says nothing about clamping and never did; it only looked like a clamp pin because both mechanisms happened to yield the same string. Restored to `xhigh` and renamed to say what it actually tests. It now also asserts `effort_clamped === false` and `effort_requested === 'xhigh'`, which is what makes it impossible to mistake for a clamp pin again: those two fields prove the value is what the resolver produced rather than something the renderer downgraded. Before #3007 there was no way to tell the two apart from the output — which is precisely why the previous pass could not tell them apart either. Added the test that was actually missing: `effort.agent_overrides`, which outranks the tier default, so the requested level genuinely reaches the renderer and `max` survives to `effort_rendered` end-to-end through the real CLI. Verified by probe before asserting. One test now pins the precedence rule and the other pins the #3007 behavior, and neither can be read as the other. That the clamp-visibility fields are what resolved this is a small argument for having added them. * chore(#3007): backfill changeset pr number to 3765 * test(#3007): put model-catalog under the mutation gate The Stryker shard showed as `skipping` on this PR despite the diff rewriting model-catalog's effort logic. That was legitimate, not a detection bug: `model-catalog` was never in scripts/mutation-matrix.cjs's COVERED map, so the whole module — including everything #3007 touches — sat entirely outside mutation scoring with has_work "false". Registered, with a dedicated spawn-free surface. tests/model-catalog.unit.test.cjs is new: 44 in-process tests, no runGsdTools, no child process, no filesystem, no temp dirs. That shape is not stylistic — it is the #2790 precedent this file already documents. Stryker's command runner treats a whole `node --test <file>` invocation as ONE test costing whatever its slowest case costs, and re-runs it per mutant, so pointing a shard at tests/model-resolver.test.cjs (which uses runGsdTools throughout) would reproduce exactly the 15-minute shard-cap cancellation #2790 hit. The integration file is unaffected and keeps running in full in the normal test job. Coverage spans the module rather than only the diff, because the score is measured over the whole file: effort rendering across every model and ladder level in both channels, the prototype-chain host guard, the exported enums and maps, isAnthropicFlavoredModel's provider namespacings, the profile projections, nextTier, and mergeEffortTierDefaults. The last two were nearly left out and are worth naming — every uncovered exported function is score given away, and mergeEffortTierDefaults turned out to have a genuinely interesting contract (#3531: a partial override merges over the built-ins rather than replacing them, and isValid gates the VALUE, not the tier name, so an unknown tier key is still merged in). Every expectation was probed against the built module before being asserted. minScore is 1 and that is a PLACEHOLDER, flagged as such in the registry comment. Floors in this repo are measured, not chosen — the existing entries sit at 94, 75 and 56 — and they can only be measured in CI, because mutation shards run `node --test`, which is hard-blocked locally. The first CI run on this branch reports the real number and the floor gets ratcheted to it before merge. A placeholder of 1 reaching `next` would make the gate decorative: it would pass whether or not a single mutant is ever killed. Note the target is "never regress from measured", not a fixed 80 — planning-inspect sits at 56 and is documented as an accepted ratchet candidate. * test(#3007): bootstrap model-catalog's mutation floor legally The placeholder floor was structurally illegal and the remote run said so. tests/mutation-matrix-ratchet.test.cjs guards the guard: every COVERED module must carry a matching RATCHET_BASELINE entry in the same diff, minScore must EQUAL that baseline, and it must be at least 50. `minScore: 1` failed all three. That is the ratchet working exactly as intended — a floor nobody can satisfy accidentally is the point of it. Bootstrapped at 50 in both places. Fifty is not a measured score and the comment says so plainly: it is the minimum the guard permits, and it coincides with Stryker's own configured `break` threshold, so it is the lowest legal starting point for a module that has never been measured. It still must be ratcheted to floor(measured) - 1 before this PR merges. Also corrected a real defect in the file's own instructions. "HOW TO UPDATE" step 1 read "Run the per-module Stryker shard locally" — which cannot be done here, and which the same file contradicts eighty lines further down, where the #2790 scores are recorded as "not a local run; mutation shards run `node --test`, hard-blocked in this repo's local environment". stryker.config.mjs confirms the command runner invokes `node --test` once per mutant, and .claude/hooks/block-local-node-test.sh denies exactly that. So the documented first step sends the next contributor at a wall. Rewritten to describe the path that works — push, read the measured score off the CI shard, then set the floor and its baseline together in one diff — and to say why local measurement is not available, so nobody rediscovers it the slow way. GOODHART SAFETY is untouched. The two-step is inherent to the environment rather than a shortcut: a floor cannot be measured before the first CI run exists, and the guard rightly refuses to accept an unmeasured one below its minimum. * test(#3007): ratchet model-catalog's mutation floor to its measured score The shard ran in CI and reported 59.62% — 248 mutants killed, 168 survived, no timeouts, no errors (run 32605073352, job 97108869486). Floor set to 58 per this file's own rule, minScore = floor(measured) - 1, which is the same arithmetic every sibling entry used: 57.03 to 56, 76.58 to 75, 95.65 to 94. Both halves moved together, because the ratchet guard asserts minScore equals its RATCHET_BASELINE entry and would reject them drifting apart. The spawn-free unit surface is vindicated by the clock: 57 seconds, against a 15-minute shard cap and a 9m46s frontmatter shard in the same run. That was the whole reason for creating tests/model-catalog.unit.test.cjs rather than pointing the shard at tests/model-resolver.test.cjs — #2790 recorded shards being CANCELLED at that cap when they targeted a runGsdTools-heavy integration file. The registry comment is rewritten rather than deleted. It previously warned that the floor was provisional and must not ship that way; leaving that text next to a measured floor would make the file lie in the other direction. It now records the measurement the way the sibling entries do, including that 59.62 sits below TARGET (80) and is therefore a ratchet candidate like planning-inspect at 56 — comfortably clear of its own floor with real room to grow. Raise it as the tests improve; never lower it. Worth stating plainly: 168 surviving mutants is not a clean bill of health. It is an honest floor for a module that had NO mutation coverage at all an hour ago, and it is now pinned so it cannot silently regress. --------- Co-authored-by: sim <sim@local> |
||
|
|
3fd03bec4c |
fix(#3760): refuse a legacy-key migration into a non-object config section (#3767)
* test(#3760): failing-first regression for non-object config section Locks the contract from the issue's Expected section before any fix exists: a legacy-key section holding a string, number, boolean or array must be preserved verbatim, reported, and never persisted in an expanded form. Covers both blocks the issue names (branching_strategy -> git.*, sub_repos -> planning.*), the migrateOnDisk multiRepo branch that shares the shape, and the loader write paths that are what actually reach the user's config.json. Includes the negative-space cases that must keep hoisting ({} , null, absent section, canonical-nested-wins) and two fast-check properties. Refs #3760 * fix(#3760): refuse a legacy-key migration into a non-object config section normalizeLegacyKeys hoisted a legacy top-level key into its canonical nested section by spreading `result[section] ?? {}`. `??` guards only null and undefined, so a section holding a string was enumerated by index — `{...'main'}` is `{0:'m',1:'a',2:'i',3:'n'}` — while a number or boolean spread to `{}` and the value vanished. Because a fired block always pushed a Normalization, and every caller treats a non-empty normalizations array as 'config is dirty', that shape was written back to .planning/config.json and the original value became unrecoverable. Both blocks the issue names are fixed via one shared hoistLegacyKey helper, plus the two further sites that share the shape and are reachable from the same input: migrateOnDisk's multiRepo branch, and the loader's two `if (!planning) planning = {}` guards, where a non-empty string is truthy and the following assignment threw a strict-mode TypeError that the enclosing catch swallowed — discarding the user's entire config. A present non-object section now blocks its own migration. The section, the legacy key, and the file are left byte-identical; no Normalization is pushed, so nothing marks the config dirty; the refusal is reported in-band as `skipped[]` and out-of-band through the ADR-1411 warnUnusableInput seam (new frozen reason config_section_not_object). null and undefined keep their long-standing 'absent' meaning and still create the section. This is the nested-section analog of the ADR-227 shape check _readConfigFile already performs on the top-level document: valid JSON is not a config object. isConfigSection is exported and shared by both modules rather than copied. Fixes #3760 * fix(#3760): keep the multiRepo marker when planning cannot receive it Follow-up from the isolated adversarial review, and the same defect class as the two blocks the issue names — in the block it did not name. normalizeLegacyKeys block 3 deleted `multiRepo` and pushed a Normalization before anything consulted the planning section, deferring 'can this section receive sub_repos?' to the caller that runs filesystem detection. By then the marker was already gone and the config was already dirty, so with {"multiRepo":true,"planning":"docs"} the loader wrote the file back with multiRepo removed, the sub_repos injection silently no-opped against the string, and no diagnostic was emitted at all. migrateOnDisk warned for the same input; the ~30-caller loadConfig path did not. Section validity is knowable from the parsed config alone — detection is only needed for the VALUE, not for whether the destination can hold it. The refusal moves into block 3: the marker is kept, no Normalization is pushed, and a skipped entry is recorded, so all three callers inherit the preservation and the diagnostic together. The caller-side guards drop to pure narrowing. Also from review: skipped[] now reports sectionType ('string' | 'number' | 'boolean' | 'array') instead of sectionValue. migrateOnDisk's report is printed verbatim by `migrate-config`, and this module already masks config values on the set/unset output path; the type is the whole diagnostic and the value is still in the file. And `migrate-config --raw` no longer answers a refused migration with 'No legacy keys found — config is already canonical.' Legacy keys WERE found and declined, and the decline is the one thing only the user can fix by hand. Refs #3760 * fix(#3760): keep configuration.cjs dependency-free; emit from its callers The remote matrix caught a regression my own change introduced: adding `require('./unusable-input.cjs')` to configuration.cts broke the #3571 install-layout contract. `configuration.cjs` must load from a layout holding only itself plus bin/shared/*.manifest.json — the installer does not co-locate arbitrary siblings — so the new require failed at load time: Cannot find module './unusable-input.cjs' Require stack: - /tmp/gsd-3571-.../.codex/gsd-core/bin/lib/configuration.cjs pinned by 'co-located bin/shared manifests let configuration.cjs load without sdk/shared' in tests/install.test.cjs (3 failures). The contract is deliberate and the test is right, so the module goes back to zero sibling requires and the out-of-band diagnostic moves to the callers that already carry a dependency budget and hold the resolved path: cmdMigrateConfig (config.cts) and loadConfigResolved (config-loader.cts). normalizeLegacyKeys keeps reporting refusals in-band via skipped[], which is what lets it be pure and dependency-free at the same time. The emission-count and dedup assertions move to tests/config-loader.test.cjs, where the diagnostic now originates. A new assertion pins the inverse for the module itself — migrateOnDisk must emit ZERO diagnostics while still reporting skipped[] — so regrowing a sibling require fails a unit test instead of only the install suite. CONTEXT.md records why the emitter is the caller. Refs #3760 * chore(#3760): backfill changeset pr number to 3767 --------- Co-authored-by: sim <sim@local> |
||
|
|
b6977d9d11 |
fix(#3762): enforce docs/INVENTORY.md roster rows, backfill 32 gaps (#3766)
* test(#3762): failing-first roster gate for docs/INVENTORY.md rows
Anchors the human half of the inventory-drift rule: every entry in
docs/INVENTORY-MANIFEST.json must have a hand-written row in
docs/INVENTORY.md. Expected RED on this commit -- next carries 32
unrostered surfaces, which is the defect the gate exists to catch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3762): enforce docs/INVENTORY.md roster rows, backfill 32 gaps
docs/INVENTORY.md calls itself the authoritative roster of every shipped
GSD surface, and CLAUDE.md's inventory-drift rule requires both a roster
row and a manifest regen. Only the manifest half was anchored, so a PR
could ship a surface, regenerate the manifest, omit the row, and stay
green -- as PR #3758 did with gsd-core/references/planner-coupling.md.
Adds the roster half to tests/inventory-manifest-sync.test.cjs, backed by
a pure matcher in tests/helpers/inventory-roster.cjs, and backfills the 32
surfaces already missing rows on next.
Fixes #3762
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(#3762): harden roster matcher against fenced blocks and trim exports
Skips fenced code regions when splitting level-2 sections so a documented
'## ' example inside a fence cannot truncate a family section (false red)
or contribute a phantom row (false pass); makes the heading pattern linear
rather than a backtracking lazy match; narrows the module surface to the
three names the gate consumes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3762): close two false-RED gaps found in orthogonal review
Indented headings: CommonMark permits an ATX heading to carry 1-3 leading
spaces, and a ^##-anchored pattern read such a document as having no family
sections at all -- reporting all six missing, a structural red for zero real
drift. Reproduced, then fixed and pinned.
Link-wrapped cells: a row written as [`x.md`](../x.md) was not recognized,
though docs/INVENTORY.md already uses that form elsewhere. Unwrapping now
peels a whole-cell link and a whole-cell code span, and only layers that
wrap the cell entirely -- a file mentioned mid-prose is still not a row, so
the false red is not traded for a false pass.
Adds a second fast-check property over the commands source-link rule, the
one family whose matching rule differs from the other five.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(#3762): boundary trio for the fence-marker length limit
RULESET.TESTS.boundary-coverage — the matcher's only numeric limit is the
fence marker's {3,}. Exercises 2 (inline markup, swallows nothing), 3, and
4 characters, for both backtick and tilde delimiters.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* chore: remove stray pwned_cmdsub injection-test canary from the repo root
A zero-byte file committed by
|
||
|
|
95f7c14413 |
fix(#3642): stop the single-section total_phases leak into an absent milestone (#3727)
* test(#3642): failing-first single-section leak rows * fix(#3642): gate the unbounded total on any-milestone-section, not >=2 * test(#3642): rewrite the 3185 wrapper row to the withhold contract * docs(#3642): glossary amendment for the >=1 sibling; changeset * chore(#3642): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
9a69a86f42 |
enhance(#2971): strict planning filter mode for /gsd-pr-branch (#3720)
* test(#2971): failing-first suite for the pr-branch planning-path filter Binds the not-yet-built planning.pr_strict mode and the corrected filter recipe for /gsd-pr-branch across six layers: pure classification and forbidden-path predicates, real-git fixtures that run the cherry-pick filter loop end to end, config-key registration through the real CLI and both manifests, the executed worktree-materialization claim the issue's triage asked to establish, fast-check properties over arbitrary path sets, and a drift guard over the shipped workflow. Two live defects in today's shipped recipe are pinned as regressions, both reproduced empirically first: `git rm -r --cached` stages a deletion of any .planning/ path the target branch already tracks, so the generated PR removes the base branch's planning files; and the same command leaves the cherry-picked file untracked on disk, so a second commit touching that path aborts the pick with "untracked working tree files would be overwritten" and every remaining commit is silently dropped. The test helper parses the canonical path lists out of gsd-core/workflows/pr-branch.md rather than restating them, so the workflow stays the single source of truth and the suite cannot drift from what ships. Refs #2971 * feat(#2971): strict planning filter mode for /gsd-pr-branch Adds planning.pr_strict — a boolean, default false, that selects what /gsd-pr-branch means by "filtered". Default mode is unchanged: structural planning state survives into the PR branch and the nine transient subdirectories do not. Strict mode drops every .planning/ path, structural files included, and carries a commit over only when it touches at least one file outside .planning/. Strict mode is what makes planning.commit_docs: true safe for a project that versions its planning tree locally but publishes none of it. The alternative posture, commit_docs: false, silently costs parallel executor isolation — a worktree is checked out from a commit, so an untracked or ignored .planning/ is simply absent inside it and the executor has no PLAN.md to read. That claim is now established by an executed fixture rather than inherited. The two path lists are declared once and both projections derived from them, so create_pr_branch and verify can no longer disagree about what the filter promised. verify previously counted every .planning/ path against a documented success criterion of zero while create_pr_branch was specified to preserve five structural files, so a correct run reported itself as failed on every phase that touched STATE.md — which is every phase. It now asserts against the active mode, and names the .planning/ paths default mode deliberately keeps rather than trading a wrong signal for silence. Two verified defects in the same recipe are fixed alongside, because strict mode would have amplified both. `git rm -r --cached` staged a deletion for any .planning/ path the target branch already tracked, so the generated PR removed the base branch's planning files — under strict mode that would have been the entire tree. The same command left the picked file untracked on disk, so a second commit touching that path aborted the cherry-pick with "untracked working tree files would be overwritten" and every remaining commit was silently dropped. Both were reproduced against real git before being fixed. The filter now forces excluded paths back to what the PR branch's HEAD carries, in the index and the working tree; a conflict outside the filter halts instead of being improvised past; a commit left empty by filtering is skipped rather than failing. A clean-working-tree precondition makes the worktree half safe. Closes #2971 * fix(#2971): unwind the checkout on a conflict halt, and test the real recipe Two review findings, both fixed in place. The isolated adversarial pass found that the conflict-outside-the-filter branch exited while leaving the user checked out on the half-built PR branch with cherry-pick state still live — this loop runs in the user's own working directory, so stranding them there is a real cost even though it is not a vulnerability. The branch now aborts the pick, returns to the original branch, removes the partial PR branch, and says so before exiting. The standards pass found the L2 fixtures executed a hand-written mirror of the cherry-pick filter recipe rather than the recipe itself, so a reordering in the workflow would not have been caught — and the order is load-bearing, since restoring a path from HEAD before removing it inverts the filter. The helper now extracts the canonical loop from the shipped workflow and the fixtures execute that verbatim, which also gives the conflict-halt unwind above real coverage. The drift guard additionally pins the two commands' relative order and asserts the workflow carries exactly one canonical loop. Also records the publication gate in the CONTEXT.md glossary next to the commit gate it is distinct from. Refs #2971 * fix(#2971): make the conflict-halt unwind actually unwind, and use the colon slash form The remote matrix caught two defects in the previous commit. The halt path claimed to restore the original branch but did not. `git cherry-pick --abort` does not apply to a single `--no-commit` pick with no sequencer file, and the fallback left the unmerged index in place, which makes `git checkout` refuse — a failure the `2>/dev/null || true` then swallowed, so the user was told they had been restored while still sitting on the half-built PR branch. The unwind now drops sequencer state, hard-resets the disposable PR branch to clear the unmerged index, and only claims a restore when the checkout actually succeeded; when it does not, it says where the user is and gives them the two commands to finish it by hand. Verified against real git: exit 1, the conflict named, HEAD back on the original branch, the partial branch gone, a clean tree and no CHERRY_PICK_HEAD. Two runtime-loaded source artifacts used the retired `/gsd-<cmd>` hyphen form, which names a command no runtime registers. The canonical authoring token for workflows and references is `/gsd:<cmd>`; docs keep the hyphen form, so the documentation added in this branch is unaffected. The comment in src/config.cts moves to the colon form too, since it propagates into the generated lib. Refs #2971 * docs(#2971): backfill PR number into the changeset fragments (#3720) --------- Co-authored-by: sim <sim@local> |
||
|
|
14679b866b |
enhance(#2856): add default-off live-DOM UAT capability (#3716)
* test(#2856): add failing-first suite for the live-dom-uat capability Binds the approved triage shape before any of it exists: - containment — the execute:wave:post hook must not render unless workflow.live_dom_uat is true AND the capability resolves active (fail-closed on a missing state entry, and on a non-boolean value) - criterion 4 — agents/gsd-executor.md carries no browser MCP family; asserted as an absence, which is the only way it is observable - Hyrum guard — the pre-existing mcp__playwright__* branch must stay outside the key-gated block, or upgrading silently removes working automated UI verification for every current Playwright-MCP user - parity — the browser glob list now lives in two surfaces (agent frontmatter + workflow detection block); the assertion fails if either gains or loses a family without the other Red by construction: the capability, agent and workflow block do not exist yet. Verified on the remote runner. Refs #2856 * enhance(#2856): add default-off live-DOM UAT capability A phase whose acceptance criteria needed a live DOM could not be finished by the agent that executed it: gsd-executor carries no browser tools, so it correctly returned checkpoint:human-action even though the work was not human-only, just tool-less. Every such phase degraded to "executed, then finished by hand in the orchestrator", and autonomous: false could not distinguish "a human must judge this" from "the executor lacks the tool". Implements the shape approved at triage, not the one reported. The executor's tools: line is NOT widened, in any configuration: for a first-party agent the static list is the only control that exists (ADR-1244 D2, ADR-857 D4, no per-dispatch override). Instead one default-off capability owns the key, the agent, and the step: - capabilities/live-dom-uat/ — activationKey workflow.live_dom_uat (boolean, default false), one additive step at execute:wave:post (onError: skip, gates: []), so it can never halt a wave - agents/gsd-dom-verifier.md — the only GSD agent carrying browser MCP globs, in its own tools: line, with no Bash - verify-work automated_ui_verification — a gsd:live-dom-families block naming both new families AND the key; presence alone never activates Two independent fail-closed gates: isCapabilityActive renders a hook only on state.active === true, plus the step's own `when`. The pre-existing mcp__playwright__* branch keeps the gating it already had and stays outside the new block. Pulling it behind a default-off key would have silently removed working automated UI verification from every current Playwright-MCP user on upgrade. Also closes a host gap this surfaced: execute:wave:post dispatched only contribution + gate, so ANY registered step was declared and silently never run — exactly the single-kind hand-roll loop-hook-dispatch.md names. Step 5.75 now dispatches every kind == "step". The browser-profile lock is tolerated, not coordinated: --isolated is a flag on the operator's own MCP-server registration that GSD neither launches nor parameterizes, so the verifier reports could_not_look / profile_locked, names the flag, and stops. DOM-VERIFY.md keeps could_not_look and nothing_to_report distinct behind a closed reason enum — collapsing them is the ambiguous-run-notes defect reported. Verified on the remote runner. Closes #2856 * fix(#2856): apply review findings from the orthogonal passes Correctness pass (blocker): - delete detectionBlockIsCrlfSafe. It was pass-always: it read the file, replaced LF with CRLF, then indexOf'd marker strings that contain no newline, so the replacement could not change the result and the assertion could never fail for the reason it stated. There is no real CRLF risk on this surface either — the gsd:live-dom-families block has no parser, only human and agent readers. Deleted rather than replaced, per the repo's pass-always-test rule. Isolated security pass (two minors, both real): - execute-phase.md step 5.75: this change is what first activates kind == "step" dispatch at execute:wave:post, which newly opens the ref.command shell path at that loop point. Our own step uses ref.agent and never touches it, but the door is now open, so the step-dispatch line carries the same in-context validate-before-shell warning the sibling gate-dispatch line directly below it already carries. - gsd-dom-verifier: quoted page text in DOM-VERIFY.md is attacker influenced. Require it wrapped in inline code or a fence, kept short, and never left reading as a directive to the next reader. Verified on the remote runner. Refs #2856 * fix(#2856): settle the new-agent roster ripple Checkpoint 2 returned 28 failures, none in the new suite — all of them the guards that exist to make adding an agent a deliberate act. Each is a real boundary that had to move: - docs/AGENTS.md: Tools row must copy the frontmatter verbatim (#2526), so the browser globs lose their backticks; primary-agent counts 21->22, roster 33/34->34/35, Verifiers category 1->2 - docs/INVENTORY.md: roster completeness requires every agents/gsd-*.md to be classified exactly once - gsd-dom-verifier: add the anti-heredoc instruction and the commented hooks: frontmatter pattern both agent gates require - gsd-core/bin/shared/model-catalog.json: every shipped agent needs a profile entry (#3229) - copilot-install / kilo-upgrades / qwen-upgrades: expected agent list and the 34->35 roster boundary - execute-wave-post-gate-pipeline-e2e: execute:wave:post legitimately carries one step now. Asserted as an exact shape — one step, capId live-dom-uat, ref.agent gsd-dom-verifier, onError skip — so it stays a real guard against accidental change rather than being relaxed Two findings worth naming: mcp-tool-inheritance (#2526) rejected the agent for documenting mcp__playwright__* while its tools: line withholds it — a dead instruction that invites the agent to claim a path it cannot take. The prose now names the Playwright MCP family without the dispatchable token, in both the agent and the capability fragment. runtime-launcher-parity rejected the new gsd_run call: each fenced block is its own shell, so a workflow step file invoking gsd_run needs its own canonical preamble. Propagated with scripts/sync-runtime-launcher.cjs. That script also normalizes explore.md, which is unrelated pre-existing drift the parity check tolerates, so it is reverted to keep this diff scoped. The emitted-drift ack supersedes the spent #3370 entry for execute-phase.md — it is merged into next, so its ripple is absorbed at the base and it can no longer clear anything. That is the same supersede the #3370 entry itself performed on the spent #3324 fragment. Its unrelated execute-plan.md entry is untouched. Verified on the remote runner. Refs #2856 * fix(#2856): drop the stale emitted-drift ack entry The automated-ui-verification.md entry was written speculatively rather than from a reported growth, and the check names that precisely: an ack "written or reworded in THIS diff, but nothing here needed it, so it explains nothing". The growth tier keys on the bare filename as it appears under gsd-core/workflows/ or agents/. automated-ui-verification.md is nested under verify-work/steps/, so it was never in the tracked set — only execute-phase.md was ever reported, both before and after the launcher preamble landed. Only ack what the check actually reports. Verified on the remote runner. Refs #2856 * chore(#2856): backfill changeset pr number pr:0 -> 3716. The placeholder fails both changeset-lint (fail_invalid_fragment) and docs-lint (fail_malformed_fragment) by design and can only be resolved once the PR number exists. Both now report ok against GITHUB_BASE_REF=next. Refs #2856 --------- Co-authored-by: sim <sim@local> |
||
|
|
8da2dd3ad2 |
feat(#2790): add read-only planning.inspect schema-v1 snapshot query (#3708)
* feat(#2790): add read-only planning.inspect schema-v1 snapshot query Adds a read-only query emitting a schema-versioned JSON projection of .planning/ so downstream harness UIs can consume planning state without parsing GSD's Markdown a second time. Composed strictly from the ADR-3180 section 7 owners plus parsePlanDocument, parseRequirements and parseUatItems; markdown structure is read through the Markdown Sectionizer and Markdown Table Model seams. It declares its own flat external schema rather than serializing PlanningSnapshot, which is the diagnostic-rule subject and still growing. Extracts plan-document parsing out of cmdPhasePlanIndex into a shared leaf module so phase.plan-index and planning.inspect cannot drift, including the plan-id derivation both surfaces report. Also fixes parseRequirements dropping the separator delimiter used by the shipped requirements template, surfaced while wiring the requirement rows. * fix(#2790): close spec gaps and a raw-text test assertion found in review Review findings from the standards, spec and security passes: - phases[] rows carry goal and dependencies, the two per-phase elements the issue Summary names that had no corresponding field. Goal is bounded to the section's leading prose so the Depends-on line, the Plans checklist and the wave annotations are not duplicated into it. - requirement rows carry their own diagnostic codes, so a consumer no longer has to string-parse the global diagnostics subject to correlate. - roadmap_acceptance.checkbox is looked up through the phase-id key owners. It was compared raw against the on-disk directory name, so it read null for every real-world slugged phase directory and the evidence channel was inert. - the hostile-input test asserts the structured payload instead of matching the raw stdout string. The absence proof over raw stdout is kept deliberately. * fix(#2790): register planning in the runtime usage list and repair fixtures Remote runner reported 9 failures on 9b3f9aa. Two root causes, both fixed: - gsd-tools.cjs registered the planning family in HOST_COMMAND_ROUTERS but never added it to TOP_LEVEL_USAGE's Commands list. Those are two surfaces a parity test guards, and the top-of-file block comment is not the runtime help string. A real wiring gap that every local gate and three review passes missed. - the new suite's fixtures could not produce a resolvable phase set. STATE.md frontmatter omitted the milestone field, which ADR-3180 7.2 rule 1 makes the primary milestone selector, so the phase set scoped unscoped and every percentage was correctly withheld. Separately declarePhase returned a path without creating the directory, so a phase declared but never written to left phases empty. Both reproduced against the built module before fixing. No assertion was weakened. The withholding path is still exercised and still returns null when the roadmap is absent. * chore(#2790): backfill changeset pr number * test(#2790): cover every enumerated matrix row and contain a symlink escape Reverses a silent deferral. An earlier revision left 23 of the 78 enumerated matrix rows unimplemented and 7 more as one-off manual checks, with a paragraph in the artifact and the PR body describing the gap. CLAUDE.md is explicit that such a note is not a fix and is not surfacing. The rows are implemented instead and the manual-evidence bucket is gone: 49 test cases become 88, covering all 78. Writing the symlink row proved a real leak: a *-PLAN.md symlinked outside .planning/ had its content emitted into the payload, confirmed via a direct call and the spawned CLI. readDocument now resolves target and planning root with realpathSync and rejects an escape, returning the ordinary unreadable-document shape. Tested both ways, because a containment check that over-rejects is its own defect: an escaping symlink leaks nothing and degrades that plan alone, while a legitimately relocated .planning/ symlink stays fully readable. The three new modules are registered in the mutation COVERED registry, which had been reporting has_work false and skipping the Stryker gate entirely. Provisional non-binding floors so the shards run and report; raised to the measured value before merge, since the registry forbids calibrating from a local run. * fix(#2790): satisfy the mutation ratchet contract and scope the 1MB test Remote runner reported 16 failures on 8c451ed. Two causes. The COVERED registry has a paired contract the earlier commit violated: every module needs a matching RATCHET_BASELINE entry, and minScore must be between 50 and 100 with minScore === baseline. The provisional floor of 1 was illegal on both counts. All three modules now sit at 50 — the registry's own enforced minimum — with matching baselines. The score cannot be measured locally: the shard runs node --test, which this repo hard-blocks, so CI is the only source. Floors are raised to the measured value once this PR's shards report; a shard below 50 means the tests need strengthening, since the floor cannot go lower. The 1MB test was measuring the test harness rather than the product. The command handles the oversized payload correctly by spilling to a tmpfile and resolving it back, but the resolved stdout then exceeds runGsdTools' maxBuffer and the helper reports ENOBUFS. It now uses --pick so stdout stays one byte while the full 1MB document is still read and parsed end to end. * fix(#2790): wire containment across every document read this command drives An isolated security review of the containment control found the boundary logic sound but not comprehensively wired: two content reads reached the filesystem without it. An escaped phase DIRECTORY could enumerate external filenames into the file fields and diagnostic subjects. Both enumeration sites now containment-check the directory before reading. Worth recording that the leak was already prevented one layer earlier than the review claimed: Dirent#isDirectory() reports false for a directory symlink, so such a directory never becomes a phase row at all. The guard is defense-in-depth for a direct caller and for platforms where a reparse point reports as a directory. A *-VERIFICATION.md symlinked outside the root leaked one frontmatter value verbatim, because readVerificationStatus does its own read and copies an unrecognized status into the payload's next_action. Closed from the consumer side through that function's existing fs injection seam, so src/verification.cts keeps its signature and its other callers are untouched. The reviewer additionally rated a forged status: passed as an integrity bypass. It is not: anyone able to plant the symlink can plant a real VERIFICATION.md saying the same thing. The incremental risk is confidentiality, which is what these fixes close. src/plan-scan.cts is deliberately unchanged: isPlanSuperseded reads symlink-followed content but yields only a derived boolean, no document text. * test(#2790): give the mutation shards an in-process surface Two Stryker shards were CANCELLED at the 15-minute cap, not failed on score. CI log: 640 mutants instrumented, and the dry run reported 'Ran 1 tests in 20 seconds' because the shards pointed at the integration suite, where nearly every case spawns a gsd-tools subprocess and Stryker's command runner treats the whole test-runner invocation as a single test. 640 x 20s cannot finish in 15 minutes; at the kill it was 27/640 with an ETA over an hour. Every other COVERED module points at a property or unit file, and the workflow's own paths filter lists exactly those two patterns. In-process is the intended mutation surface; the shards were pointed at the wrong shape of test. Adds tests/planning-inspect.unit.test.cjs — 39 cases in 10 describes that spawn nothing and call the built modules directly. plan-document and the router need no filesystem at all, one being a pure content-to-object parser and the other taking an injected mock. The three shards now point here. The 91-case integration suite is untouched and still runs in the normal test job. * chore(#2790): ratchet mutation floors to the measured CI scores CI run 32392791843 measured all three shards, which is the only source the registry accepts — local runs count timeouts as kills and inflate badly. planning-command-router 95.65 -> floor 94 plan-document 76.58 -> floor 75 planning-inspect 57.03 -> floor 56 Applied the registry's own rule, floor(score) - 1, and updated RATCHET_BASELINE to match, since the ratchet test enforces equality. planning-inspect sits well below the file's target of 80 and is the obvious ratchet candidate as its tests improve. planning-command-router already exceeds the target. The placeholder comment about floors pending measurement is removed rather than left standing as a false statement. --------- Co-authored-by: sim <sim@local> |
||
|
|
77fa08f1e8 |
fix(#2773): feed the spec-phase edge probe English-translated requirement text (#3713)
* test(#2773): failing-first contract and premise tests for translated edge-probe input Locks the Step 5.5 contract that a response_language project must feed the edge probe an English translation of each requirement's text, and binds that advice to measured engine behavior: the same requirement classifies to zero shapes in Portuguese and to collection/adjacency/empty/ordering in English. Also pins the honest limit — the issue's own repro sentence classifies to [] in English too, so translation is necessary but not sufficient and the authored shapes override is the documented fallback. Red before the doc change; the assertions are all false today. Refs #2773 * fix(#2773): feed the spec-phase edge probe English-translated requirement text The shape cues in src/edge-probe.cts are English word-boundary regexes, so a project running with response_language set wrote its SPEC requirements into the Step 5.5 $REQS_JSON heredoc in that language, matched no cue, classified to zero shapes, and landed every row in the unclassified sentinel (#1110). The taxonomy contributed nothing and --auto left it all unresolved — the probe was a silent no-op for exactly the spec type it exists to harden. Step 5.5 now states that the $REQS_JSON payload is engine input rather than user-facing output, so the response_language rule does not govern it: each requirement's text carries a faithful English translation, the SPEC keeps its original language, and requirement ids are never translated or renumbered. The instruction sits before the heredoc on purpose — the downstream APPLICABLE=0 warning fires only when every requirement is unclassified, so a partly-classified non-English spec would otherwise slip through with no signal at all. Measured against the compiled engine: the same requirement returns [] in Portuguese and collection -> adjacency/empty/ordering in English. Also measured: the issue's own repro sentence returns [] in English too, so translation is necessary but not sufficient — the instruction therefore points at the authored shapes override for prose carrying no cue in any language rather than promising that translation restores classification. Doc scope only, per the triage disposition on the issue. The compiled engine is untouched; the lang-hint / per-language cue-set fix is a separate follow-up. Closes #2773 * fix(#2773): clean up the edge-probe temp file on the placeholder-guard exit path Surfaced by the isolated security review of this branch. Between the mktemp and the unconditional cleanup, Step 5.5 has two sibling guards that disagreed about their own invariant: the engine-failure guard runs rm -f "$REQS_JSON" before exiting, while the empty/placeholder guard directly above it exited without one. A spec run that tripped the placeholder check therefore stranded a temp file holding the SPEC's requirement text in TMPDIR, once per failed run. The added contract test walks the region between the mktemp and the unconditional cleanup and asserts no exit path leaves the file behind, so the two guards can no longer drift apart. Proven to bind: run against the pre-fix file the walker reports the leaking exit; against the fixed file it reports none. Refs #2773 * docs(#2773): record the edge probe's English-cue input constraint in the predicate store The co-change gate flagged CONTEXT.md (13 co-changes with spec-phase.md) and docs/CONFIGURATION.md (11) as candidate-missing-updates, and both were real gaps rather than incidental coupling. CONTEXT.md's EdgeCompletenessProbeModule entry documents the input contract for classifyShape but did not record that SHAPE_CUES are English word-boundary patterns — so the predicate store implied text was language-agnostic, which is what a future agent reads before touching this seam. docs/CONFIGURATION.md's response_language row is what a non-English project reads when it turns the setting on; it now names the one deliberate exception and links to the FEATURES.md explanation, so the interaction is discoverable from the config key rather than only from the workflow. CONTEXT-INDEX.json regenerated via gen-context-index.cjs --write. The drift-ack fragment is updated for the final byte range and now also records the placeholder-guard cleanup fix folded into the same block. Refs #2773 * fix(#2773): append the growth rationale to the existing spec-phase.md ack entry The remote runner caught this: emitted-attribution.test.cjs pins the 0000-legacy-migration.json spec-phase.md entry permanently (the #2914 migration regression test asserts the exact '31987 -> 31997' delta text survives), so removing it to avoid a duplicate-key collision with a new fragment broke that test instead of satisfying the ratchet. The entry is an accreting log, not a single-use slot — #2733, #3132 and #3102 were each appended to the same reason string by later PRs, which is how a shared growth key coexists with the rule that two ack sources may never name the same path. This appends the #2773 rationale the same way and drops the separate fragment, whose spec-phase.md key was the collision. Verified locally by reproducing both affected tests against the real fragment before re-dispatching: the pinned delta survives, grown[0].acked is true, staleAcks is empty, and all 35 entries still read as spent. Refs #2773 * docs(#2773): add a how-to for probing edges in a non-English project The phase gate's enablementSequence check caught a wrong call of mine. I had recorded that no how-to was owed because the user takes zero extra steps — the workflow translates the probe input itself. Written out, though, the sequence from off to value is two steps and step 1 depends on response_language, a setting owned by a different capability than the edge probe, which is exactly the condition the how-to test names. There is also real task content a reference table cannot carry: the three-way split between a few unclassified rows (the classifier's recall gap), every row unclassified (the probe could not read the spec at all), and the silent partly-classified case where the APPLICABLE=0 warning never fires. That last one is what a user would otherwise misread as a clean bill of health. Shaped after the resolve-edge-coverage-findings / resolve-unreachable-guard siblings and indexed from docs/README.md next to its closest relative. Refs #2773 * chore(#2773): backfill the changeset PR number pr:0 placeholder replaced with the real PR number now that #3713 exists. Refs #2773 --------- Co-authored-by: sim <sim@local> |
||
|
|
adb46cdd85 |
feat(#2734): surface STATE.md commit-age on the statusline (#3700)
* test(#2734): failing-first suite for the statusline STATE.md freshness marker Binds the contract before any hook change exists: a `state ~N commits back` segment gated on the state_head stamp landed by #2622, firing at the same advisory threshold /gsd-health's W024 uses rather than at > 0. Covers all five acceptance criteria — threshold parity (19/20/21 boundaries), both renderers including formatGsdStateCompact, an exact spawn-count assertion, repo-pinning and sub_repos degradation, and behavioral parity against readStateHeadFreshness rather than a source-grep of the two fence copies. 52 example-based tests plus 5 seeded fast-check properties. Red now by design. * feat(#2734): surface STATE.md commit-age on the statusline Adds an opt-in `state ~N commits back` marker to the GSD-state segment, consuming the `state_head` stamp and freshness contract landed by #2622. A solo developer returning to a project reads "Phase 4, executing" in STATE.md and acts on it, without noticing the codebase moved 40 commits since that line was written. /gsd-health reports it as W024, but only if you think to run it; the statusline is the surface you see without asking. Fires at STATE_HEAD_ADVISORY_COMMITS (20), the same threshold W024 uses, not at > 0: with commit_docs:true the commit carrying a STATE.md sync advances HEAD by one, so > 0 would alarm permanently on a fresh project. Costs exactly one bounded git subprocess per render and none when disabled. `rev-list --left-right --count` answers ancestry and distance together, and repo pinning is a filesystem check mirroring projectOwnsItsRepo rather than a --show-toplevel compare, which is unreliable on macOS /private/var and Windows 8.3 paths. Every unresolvable input degrades to the tri-state unknown -- the marker is absent, never a "fresh" claim the project cannot substantiate: a malformed stamp, a root that does not own its .git, a sub_repos workspace, history rewound past the stamp, or git being unavailable. Also collapses statusline config resolution onto one resolveStatuslineOptions() seam. runStatusline() and renderStatusline() duplicated it byte-for-byte; one copy is what keeps a newly-added key from reaching only one of them. * test(#2734): route the e2e spawn through the process seam and fix fixture leaks Review findings from the two orthogonal passes: - `bothEntryPointsResolveOptionsIdentically` spawned a child and substring-matched its stdout to test a pure function. It now calls resolveStatuslineOptions() directly — no subprocess, no text matching. - `skipsFreshnessWorkWhenTodoTaskActive` genuinely needs a child (the !task gate lives in runStatusline, which reads stdin), so it now spawns through tests/helpers/process-seam.cjs and proves the negative with a filesystem fact: the git shim appends to a marker file on every invocation, and the assertion is that the marker never appears. Stronger than asserting text is missing, and it drops the last stdout substring match in the block. - Every fixture-creating test now registers `t.after(() => cleanup(dir))` instead of a trailing cleanup(dir), which leaked the temp repo on assertion failure. derivationAgreesWithStateModule reassigns `dir` across five fixtures, so it binds each directory at scheduling time rather than cleaning only the last. Also corrects markerCoexistsWithMilestoneComplete, which asserted the wrong expectation rather than finding a code defect: `percent` drives the progress bar too, so the milestone segment reads "v1.9 [##########] 100%". The marker appends after it, which is what the test exists to prove. CONTEXT.md's opt-in statusline key list was missing statusline.show_git as well as the new key; both are now enumerated. * docs(#2734): backfill changeset PR number (#3700) --------- Co-authored-by: sim <sim@local> |
||
|
|
2fca0e17e4 |
enhance(#2554): resolve code review depth from path-scoped override rules (#3695)
* test(#2554): failing-first suite for path-scoped code review depth overrides Binds the not-yet-built code-review-depth module: segment-aware path-prefix matching of a changed-file set against ordered {paths,depth} rules, resolution order flag > strongest matching rule > global > standard, typed validation errors, and the large-scope downgrade boundary. Also proves behaviorally that workflow.code_review_depth_overrides is not yet a registered config key. Refs #2554 * feat(#2554): resolve code review depth from path-scoped override rules Adds workflow.code_review_depth_overrides — an ordered array of {paths, depth} rules matched against a review's changed-file set by segment-aware path-prefix comparison. Resolution order is --depth= flag, then the strongest matching rule, then workflow.code_review_depth, then standard; a matching rule replaces the global rather than being max'd with it, so quick and standard rules stay meaningful. Glob metacharacters are a hard configuration error rather than sugar for a prefix, and malformed rules halt the review instead of degrading to standard. The resolver is pure and reports its own provenance, so the workflow can print the resolved depth and the rule that matched. The pre-existing >50-file deep-to-standard downgrade moves into the module and now names the rule it overrode. The key is registered centrally rather than as a capability config slice: the federated slice channel admits only boolean/string/number/enum, so an array slice would be dropped as malformed. Closes #2554 * test(#2554): correct depth-provenance assertions and pin out-of-repo paths Two corrections to the failing-first suite. The source assertion for a non-matching rule with no global configured expected 'config'; with no global set the depth comes from the default, and a companion assertion tolerated either value, so both passed against an implementation that derived provenance from whether any rules existed rather than from where the depth came from. The out-of-repo absolute-path case used a home-directory path that matched neither implementation, so it never exercised the defect it named. It now pins the discriminating cases: an absolute path outside the repo root must not match a repo-relative rule, and one under the root must. * docs(#2554): document path-scoped code review depth overrides Reference rows for workflow.code_review_depth_overrides in the configuration, features and commands references plus the locale copies that carry those tables, and in the planning-config reference. Explanation of why escalation is whole-review rather than per-file and why v1 is prefix-only. New how-to for scoping review depth by path, carrying the configuration-error reason table and the distinction between nothing to report and could not look. CONTEXT.md glossary entry and the INVENTORY row for the new CLI module. ja-JP and ko-KR CONFIGURATION.md carry no code_review keys at all, and ko-KR and pt-BR FEATURES.md carry no code-review config table, so those files are deliberately untouched. * fix(#2554): make the depth-misconfiguration halt executable and reject control chars Three review findings, all in this change. The misconfiguration halt was prose rather than shell: the error-printing fence was followed by an unconditional extraction fence, so an ok:false result threw and left the depth empty instead of stopping the review. Prose is not a guard — the two fences are now one block with a real conditional, and anything that is not the literal string true fails closed. An interior control character in a rule path survived validation and reached the provenance string and the summary box; rule paths now reject control characters via a new PATH_CONTROL_CHAR reason, after the glob check so precedence is unchanged. That in turn makes the field record safe to delimit, so the seven node invocations that each re-parsed the same result to read one field collapse to one. Also corrects the glossary entry's illustrative paths, which the glossary-ref check read as real repository references. * fix(#2554): use the fast-check v4 string API and acknowledge workflow growth Two failures from the remote matrix on d3111f45, both this branch's. The property block built its segment arbitrary with fc.stringOf, removed in fast-check v4. Because the arbitrary is constructed in the describe body, the throw took out all four property tests rather than one — they had never executed. Rewritten to fc.string({unit, ...}), the form this repo already uses in emitted-attribution.test.cjs. Every other fast-check helper in the file was audited against the installed module. The emitted-attribution growth arm needed an acknowledgment for code-review.md, which grew 5376 bytes. The pre-existing 3503 fragment keying the same file is spent — its ripple was absorbed when #3503 merged, and the base file is exactly the 34435-byte baseline this growth is measured against — so it cannot clear anything, while the ack lint hard-fails on a duplicate key across two sources. Removed it in favor of the new fragment, which is exactly how #3503 itself replaced the spent 3191 fragment. * docs(#2554): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
|
79781e68eb |
enhance(#2401): ground verify-command paths and inherit prior-phase commands (#3678)
* feat(#2401): ground <automated> verify-command paths and inherit prior-phase commands Adds a deterministic resolvability probe over each PLAN.md <automated> verify command and surfaces the nearest prior phase's proven commands to the planner at every context window. - src/verify-command-grounding.cts: recognizer (not a shell interpreter) that grounds a leading cd <literal> chain and npm --prefix <literal>, and reports unresolvable rather than guessing. Never executes command text. - gsd-tools check verify-command-paths <N>: per-phase probe, wired into plan-phase.md before the plan-check pass. - init.plan-phase gains prior_verify_commands, ungated by context_window. - gsd-plan-checker: new Verify Command Path Resolvability dimension that reports the failing target and never prescribes a replacement. Also fixes first-match-wins prefix bucketing in scripts/lint-test-file-count.cjs (readdir order is not stable across platforms, so a module whose name extends another's with a hyphen bucketed differently on Linux than on macOS). Closes #2401 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2401): ground the canonical --prefix form, quoted paths, and absolute cd resets Independent review found three defects in the recognizer: - npm --prefix DIR run SCRIPT never reached the script-existence check, because the pattern required npm and run to be adjacent. That is the form the docs tell planners to prefer, so script_missing never fired for it. The prefix flag and its value are now stripped before matching. - --prefix captured with \S+, so a quoted path containing a space was truncated to a stray opening quote and reported as a missing directory - a false blocker, worse than the bug this feature fixes. The capture is now quote-aware. - A chained cd whose later segment was absolute concatenated instead of resetting, producing a nonsense path and another false blocker. The fold now resets on an absolute segment. Also replaces the bespoke phase-directory regex with the canonical phase-id helpers. Real phase directories are NN-slug, not phase-N-slug, so the prior-command harvest matched nothing outside its own fixtures and the planner-inheritance half of this feature was dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(#2401): source task blocks from the canonical sectionizer The module carried its own copy of the <task>-block grammar - a fourth hand-rolled mirror of the one markdown-sectionizer owns. verify.cts keeps its copy only because it needs the type= attribute the canonical helper discards; this module never reads that attribute, so it can share the owner outright instead of adding a test around a copy. extractAutomatedCommands now takes task bodies from extractTaggedBlocks and the out-of-task remainder from stripTaggedBlocks. A task-grammar parity test pins the attributed task-name set against the canonical helper across six awkward task shapes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2401): extract agent-file overflow to references and repair the property arbitrary The remote matrix run came back red with 19 failures, four root causes: - agents/gsd-plan-checker.md and agents/gsd-planner.md both blew the 49152 agent cap. Their bodies move to gsd-core/references/, leaving @-reference stubs, per the documented overflow pattern. - The new checker dimension invoked gsd_run before the canonical preamble that defines it. The call is deleted outright: plan-phase.md already runs the probe and hands the result in as {VERIFY_PATHS}, so the dimension consumes that rather than re-running anything. - fc.fullUnicodeString does not exist in fast-check 4.8.0. Replaced with fc.string({ unit: 'binary' }), which covers the same 0000-10FFFF range. - Three runtime-loaded files grew; acknowledged in the existing ack fragments that already own those bare filenames, since two ack sources may never name the same path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2401): regenerate golden install-tree fixtures for the new references Adding two files under gsd-core/references/ changes what the installer emits into every runtime's tree, so all 19 golden install-parity fixtures went stale. Regenerated with npm run gen:install-tree; the delta is exactly the two new reference paths per runtime, no removals. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2401): backfill changeset pr number to 3678 * fix(#2401): treat ~ as a home expansion only at the start of a path Windows CI caught this on both shards; the Linux-only remote matrix cannot see it. The dynamic-path refusal rejected ~ anywhere, and a GitHub Windows runner's tmpdir is an 8.3 short name - C:\Users\RUNNER~1\AppData\Local\Temp - so a valid absolute Windows path came back unresolvable/dynamic_path. This was a production bug, not a test artifact: any Windows user whose project path carries an 8.3 short name, or any literal ~, silently lost the probe entirely - every command degrading to unresolvable with no explanation. ~ is a home expansion only at the start of a path; elsewhere it is an ordinary literal. The check is now split: $, backtick, *, ? and newline stay refused anywhere (substitution and globs, and the glob characters are illegal in Windows path components regardless), while ~ is refused only leading, tolerating one leading quote since the check runs before quote stripping. The prior tests only caught this on Windows because only Windows puts a ~ in tmpdir. Four new tests pin it on every platform via a fixture directory literally named RUNNER~1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
4e60dba717 |
fix(#3604): make glossary ref visibility independent of backtick parity (#3680)
* test(#3604): pin parity-dependent ref visibility in the glossary gate * fix(#3604): make glossary ref visibility independent of backtick parity * chore(#3604): regenerate CONTEXT-INDEX for corrected predicates * chore(#3604): regenerate examples CONTEXT-INDEX for corrected predicates * fix(#3604): complete retired-family exemptions and pin the guard rails * chore(#3604): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
|
1bf73d957b |
enhance(#2295): record the resolved model per reviewer in REVIEWS.md frontmatter (#3649)
* test(#2295): failing-first coverage for per-lane resolved-model recording * feat(#2295): record the resolved model per reviewer lane * docs(#2295): document the recorded reviewer model and its provenance * fix(#2295): refuse control characters in a recorded model value * test(#2295): correct watermark assertions for the widened mark shape * fix(#2295): anchor the role-manipulation injection pattern at a word boundary * feat(#2295): record the applied reasoning effort in the model value * chore(#2295): backfill changeset pr number * chore(#2295): restore em-dash in changeset body --------- Co-authored-by: sim <sim@local> |
||
|
|
1adf6d2245 |
fix(#3620): point the docs at files that actually exist (#3658)
* fix(3620): point the docs at files that actually exist docproof found 34 stale references; the reporter hand-read all 34 and reported the 8 that are real, explaining why the other 26 are deliberate (files the documents themselves label legacy or "superseded by", and one pre-Diataxis link label whose target still resolves). Those 26 are left alone — re-touching them would contradict the issue's own analysis. Every claim was re-verified against git ls-files at HEAD before editing. docs/INVENTORY.md said its roster is anchored by six drift-control tests. Five are gone (commands-doc-parity, agents-doc-parity, cli-modules-doc-parity, hooks-doc-parity in 5d8a8c4d; command-count-sync in |
||
|
|
9e4f0e99ad |
fix(#3631): exclude only __pycache__-resident bytecode from the consent digest (#3650)
* test(3631): failing-first coverage for bytecode-cache in the consent hash
bundleContentHash digests a walk with no exclusion, so a routine 'python3 -m unittest'
inside a Python-backed capability bundle writes __pycache__ under the bundle, the
recomputed hash stops matching the consent record, and the capability silently goes
inactive — no error, no warning, and loop render-hooks then omits its step and gate.
Two distinct triggers, and the second is the sharper one: collectBundleEntries pushes a
{kind:'dir'} entry for EVERY directory and the digest emits a TAG_DIR marker for it, so an
EMPTY __pycache__/ flips the hash before a single .pyc is written. A fix filtering only
*.pyc would leave that live. Verified by execution against the built lib: 5 of 7 probe
rows diverge from intent today, including the empty-directory row.
The anti-regression rows are the point of the shape: editing a real scripts/m.py and
adding node_modules/pkg/index.js must BOTH still change the hash. node_modules is
deliberately not excludable — its contents are required at runtime, so dropping it from
the digest would stop consent binding executable content. The symlink row pins ordering:
exclusion must apply after the lstat fail-closed rejection, never before.
Refs #3631
* fix(3631): exclude derived bytecode caches from the consent digest
RED proven at e5ba8f1fe on the remote runner: 8 failures, exactly the rows predicted to
fail, with the four anti-regression rows already green.
collectBundleEntries now skips a hardcoded, gitignore-independent set from the DIGEST:
basenames __pycache__, .pytest_cache, .DS_Store, and any .pyc/.pyo file. Matching is
byte-exact on the raw Buffer name (the walk never utf8-decodes) and case-sensitive, so the
digest does not vary with how a name happens to be spelled on a case-insensitive volume.
Three properties were preserved deliberately, each pinned by a test:
- The filter runs AFTER the lstat symlink/non-regular fail-closed rejection. Filtering
first would have turned the exclusion into a way to smuggle a symlink past the check;
a symlink named x.pyc still throws.
- Excluded entries still count toward BUNDLE_MAX_FILES and BUNDLE_MAX_TOTAL_BYTES. The
caps guard the WALK; the digest answers a different question, and exclusion must not
become an unbounded-bytes hole.
- An excluded DIRECTORY is neither emitted as a TAG_DIR marker nor recursed into. The
directory marker was the sharper half of this bug: an empty __pycache__ flipped the
hash before any .pyc existed, so a *.pyc-only filter would have left it live.
The issue proposed either a gitignore-aware walk or a list including node_modules. Both
are rejected. A consent binding must not delegate its scope to a .gitignore the bundle
author does not control — one line there would drop arbitrary executable content out of
the hash. And node_modules holds code that is required at runtime; excluding it would stop
consent binding executable content, turning a usability bug into a supply-chain hole. What
makes __pycache__ different is that CPython validates each .pyc against its sibling
source, which remains hashed, so a real code change still invalidates consent.
Docs: CONTEXT.md's 'EVERY regular file AND directory' claim is corrected in place.
ADR-2363's residual-gap section said the walk had 'no exclusions' — per
docs/adr/README.md ('ADRs are append-only') that is corrected by a dated amendment rather
than an in-place edit. Its D4 argument is unaffected: skill bodies are .md and stay bound.
Fixes #3631
* fix(3631): narrow the digest exclusion after two isolated security reviews
The first cut of this fix passed the full suite and was still wrong. Both orthogonal
reviews rejected it, and the second one found a hole that has nothing to do with Python.
HIGH — an excluded DIRECTORY was 'continue'd before recursion, so its whole subtree was
permanently outside the digest. Declared hook script paths allow '_', '.' and '/' with no
directory or extension rule, so hooks:[{script:'__pycache__/run.js'}] installed, executed
via node, and its bytes could be rewritten forever without moving the hash. Ship benign
v1, collect consent, then own the machine. No Python involved.
FALSE RATIONALE — the justification I wrote into the code, CONTEXT.md, the ADR amendment
and the changeset claimed CPython validates a cached .pyc against its sibling source, so
the source staying hashed kept consent honest. That is not true, and I proved it by
execution rather than argument: default timestamp invalidation compares only the source's
mtime and size, both settable by anyone who can write the bundle. A forged pyc ran while
the .py was byte-identical.
Also wrong: '*.pyc' matched anywhere, but a legacy sourceless scripts/x.pyc IS importable,
so excluding it was a live vector.
Narrowed to what is actually defensible:
- a DIRECTORY named __pycache__/.pytest_cache has only its TAG_DIR marker suppressed;
the walk still recurses and hashes every non-excluded child.
- .pyc/.pyo are excluded ONLY when the parent basename is exactly __pycache__.
- a regular FILE named __pycache__, and a DIRECTORY named x.pyc, stay bound.
- declared hook paths containing a __pycache__/.pytest_cache segment or a .pyc/.pyo
basename are now rejected in both validator copies — a file named .pyc can contain
perfectly valid JavaScript, so the exclusion must not be reachable from a declared
surface.
Accepted residual risk, stated plainly in ADR-2363 and CONTEXT.md instead of explained
away: a forged __pycache__/mod.pyc matching an unmodified, still-hashed mod.py executes
without moving the digest. Before this change that write was detected. It is accepted to
stop routine bytecode caching from silently deactivating capabilities, and it is bounded —
the attacker needs post-consent write access, everything outside __pycache__/*.pyc stays
hashed, and no declared surface can point into the excluded space.
Known limitation, not papered over: .pytest_cache CONTENTS still move the digest. Only the
directory marker is suppressed. Excluding that subtree would reopen the HIGH finding.
Refs #3631
* fix(3631): drop the .DS_Store exclusion and pin what the caps actually bind
Second round of isolated review findings. The hardening closed the two original holes —
both re-reviews confirmed that by execution — but it introduced a new one of the same
shape, and left three claims unbacked.
HIGH, self-inflicted: .DS_Store was excluded from the digest at any depth, but the hook
path validator was hardened only for __pycache__/.pytest_cache/.pyc/.pyo. So
script:'hooks/.DS_Store' was ACCEPTED, runnableHookCommand emits the bare quoted path for
a non-.js name (the branch .sh hooks already use), and capability-source copies it with
its mode bit intact. Ship it +x with a benign shebang, take consent, then rewrite it
forever — the digest never moves. Fixed by DELETING the .DS_Store exclusion rather than
teaching the validator about it: .DS_Store has nothing to do with this issue's Python
bytecode symptom, and an excluded filename is a permanently unhashed name. The narrower
the exclusion, the smaller the hole.
The residual-risk bound in ADR-2363 and CONTEXT.md claimed declared surfaces cannot reach
excluded space. That is false and is now stated correctly: node resolves an unregistered
extension through the default .js handler, so a hashed, consent-covered hooks/run.js that
requires '../__pycache__/mod.pyc' reaches it in one hop. The validator guard raises the
bar for DECLARED surfaces; it does not contain the risk. The two bounds that are real —
post-consent write access required, everything outside __pycache__/*.pyc still hashed —
are kept.
The BUNDLE_MAX_FILES boundary test had gone vacuous: it padded with root-level *.pyc,
which the hardening made non-excluded, so it no longer proved anything about excluded
entries while the ADR claimed the caps were test-pinned. It now pads __pycache__/f{i}.pyc,
with the arithmetic re-derived by execution (capability.json + the still-counted
__pycache__ dir + N). BUNDLE_MAX_TOTAL_BYTES had zero coverage at all and is now pinned by
a sparse 32 MiB __pycache__/big.pyc that must still trip the size cap — the test that
proves exclusion did not become an unbounded-bytes hole.
Added the parity assertion CLAUDE.md's Generative Fix Divergence rule requires for the two
isSafeHookScriptPath copies, and proved it can fail: mutating one BUILT copy to drop .pyo
made the parity check report the divergence. Also pinned semantics that were correct but
untested and would have survived mutation — __pycache__/sub/x.pyc stays hashed (the parent
resets to sub, which is the recursion threading itself), .pytest_cache/y.pyc stays hashed,
and .pyo in both directions, which was a free surviving mutant.
Changeset rewritten: it still described the rejected wholesale-exclusion semantics.
Refs #3631
* chore(3631): backfill changeset PR number (#3650)
---------
Co-authored-by: sim <sim@local>
|
||
|
|
2972da4c9d |
enhance(#3619): ratchet the platform seam with local/no-private-binary-resolution (epic #3411 Phase 3) (#3636)
* chore(#3619): ratchet the platform seam with local/no-private-binary-resolution
Epic #3411 Phase 3, the ratchet. Scope revised with maintainer approval and
recorded on the issue: the epic's literal ask was a rule rejecting a bare-name
spawn outside the seam. Surveyed at
|