1e67ec97375520320fc34cda152aef5f458e793d
96 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
e20744eacb |
enhance(#3884): failure is a value — strict argv, and --pick that signals absence (#3922)
* test(#3884): failing-first coverage for strict argv and absence-signalling --pick ADR-3473 §8.4 says failure is a value. Three families currently encode failure as success, and this commit pins each one RED before the fix lands. Measured on this tree, 2026-08-26: gsd-tools generate-slug "test" --pick nonexistent -> empty stdout, exit 0 (#3365) gsd-tools audit-open --pick nonexistent_field -> dumps the entire human-readable audit report, exit 0 gsd-tools generate-slug "Hello World" --raw --pick bogus -> prints "hello-world", another field's value, exit 0 gsd-tools query state.planned-phase 3 (positional, no --phase) -> exit 0; STATE.md's "Phase: 2 of 5 (Widget Support)" is overwritten to "Phase: null - READY TO EXECUTE" and the frontmatter gains a corrupted current_phase_name (#3358) tests/pick-flag.test.cjs:27 previously asserted the #3365 defect as the contract ("returns empty string for missing field", success === true). That assertion is replaced by the required behavior rather than deleted. The new parseNamedArgs block calls the spec-object signature that does not exist yet, so it fails today by construction. The 11 existing behavior-lock tests are left untouched here; they are corrected in the implementation commit. C1/C4 assert at the consumer's output - STATE.md's bytes - per ADR-3180 Decision 4(b). A unit assertion on the parser would have passed throughout this defect's life. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3884): failure is a value — strict argv, and --pick that signals absence Implements ADR-3473 §8.4. Absence, emptiness and failure stop being interchangeable ways to say "I could not answer". parseNamedArgs (src/command-arg-projection.cts) Takes a spec object with a REQUIRED `positionals: number | 'rest'` and returns the hub's Result shape instead of a bare Record. Declaring the positional arity is what makes #3358's call site unrepresentable rather than merely detectable: an unrecognized flag or a token past the declared boundary is now InvalidArgs, naming the offending token and listing the accepted flags. The legacy positional-array call shape throws a TypeError — an internal invariant violation per ADR-3473 Decision 2, so a stale hand-written .cjs call site fails loudly instead of destructuring undefined off a Result. parseNamedArgsOrExit projects a failure onto the caller's error(); it is a projection over the one parser, not a second parser. Measured before, against a STATE.md with a populated phase-2 block: query state.planned-phase 3 (positional, no --phase) -> exit 0; "Phase: 2 of 5 (Widget Support)" overwritten to "Phase: null - READY TO EXECUTE", frontmatter gains a corrupted current_phase_name After: exit 1, `unexpected positional argument "3"`, STATE.md byte-identical. The flag form is unchanged and still updates STATE.md. --pick <field> (gsd-core/bin/gsd-tools.cjs) extractField returns {found,value}, and the pick block no longer shares one catch between "output was not JSON" and "field was absent". An absent field exits 1 with pick_field_absent, naming the field and the keys that do exist; non-JSON output exits 1 with pick_output_not_json instead of dumping the command's entire output. A field that is PRESENT with value null, '', 0 or false still prints at exit 0 — that is an answer, not a failure, and it is what keeps `--pick count` printing 0 on a fresh project. Measured before: `audit-open --pick nonexistent_field` printed the whole human-readable audit report at exit 0, and `generate-slug X --raw --pick bogus` printed "hello-world" — a different field's value, confidently, at exit 0. ADR-3409 Decision 7 explicitly deferred this contract fix to #3473; this is it. The sub-issue's "returns 0 when the count is zero OR absent" wording is superseded by the ADR rule it implements: zero prints 0, absence exits non-zero. Defaulting absence to 0 would demote "could not answer" to "the answer is zero" — the hazard docs/how-to/resolve-unreachable-guard-findings.md already warns against. Guard ledger (ADR-3473 Decision 6) scripts/lint-unreachable-guard-drift.cjs Detector A is RETIRED. Its premise — that a `--pick ... || echo` arm can never fire — is now false, so the shape it forbade is the correct idiom and keeping it would forbid the fix. Detector B (glob-consuming cat/ls, a nullglob mechanism this change does not touch) is retained in full, as are the shared scanner, the escape-marker parser and the baseline. Net: -1 detector, 0 added. The file is not deleted. Call-site audit 45 prompt-layer --pick invocations, every one a plain X=$(...) assignment — none in an if test, && chain, or a pipeline whose status is consumed, and no shell block in workflows/commands/agents/references sets -e. Of the 13 (command, field) pairs the prompt layer reads, 10 are always present; the 3 sometimes-absent ones each sit behind a prior found/existence check. No ADR-3409-class "field the command never produces" remains. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): escape untrusted tokens in diagnostics, and cover five unpinned rows Two review findings, both fixed here rather than recorded as limits. 1. A newline in an untrusted token forged a second stderr line. Before, plain-text mode: $ gsd-tools query state.planned-phase $'foo\nError: forged second line' Error: unexpected positional argument "foo Error: forged second line" After: Error: unexpected positional argument "foo\nError: forged second line" --json-errors mode was never affected — io.error runs that payload through JSON.stringify. Plain-text mode writes 'Error: ' + message verbatim, and the three new InvalidArgs reasons plus the two new --pick diagnostics all interpolate a token that comes straight from argv. Fixed with ONE shared helper, formatDiagnosticToken (src/io.cts), applied at every interpolation site — not a copy per site. It is deliberately NOT applied inside error() itself: several callers in this tree emit intentional multi-line diagnostics, and escaping newlines there would mangle them. The available-top-level-keys list needed the same treatment for a reason the review did not anticipate: `frontmatter get <file>` reads an ARBITRARY user document and echoes that document's own keys into the diagnostic. Verified reachable — a frontmatter key containing a newline reaches the key list — so formatKeyForDiagnosticList is guarding a live path, not a hypothetical one. Ordinary keys still render plain and unquoted; a fix that merely dropped the key would also have passed a "one line" assertion, so the test pins the escaped key's presence too. 2. Five behavior-table rows were implemented but nothing pinned them: B7 a dotted path that dies partway B9 bracket syntax on a non-array B10 a negative array index, in and out of range B14 a JSON root that is not an object B17 an @file: payload over 50KB B17 is the load-bearing one. output() writes @file:<path> instead of inline JSON past 50000 characters, and --pick resolves that BEFORE parsing; with no test, a future reordering of those two steps turns every large result into a false pick_output_not_json. The fixture seeds 1200 phase directories and measures the payload at 62474 characters, asserting the spill actually happened rather than assuming it. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): correct the strict-argv surface against a full verification run The first full run came back with 90 failures across 12 files, none in the new tests. They were the argv surface telling me what it actually is. Ten root causes; each classified before anything was changed. I over-implemented, and that is reverted. ADR-3473 §8.4 says parseNamedArgs rejects "unrecognized and positional tokens". It says nothing about a value flag whose value is missing. Making that an error was my design decision, not the rule, and it broke a deliberately recorded contract: `--prd` with no value resolving to null (tests/init.test.cjs emptyPrdValueIsFalsyAndTreatedAsAbsent, row B5; tests/section-manifest-init-facts.test.cjs "flag-shaped value"). The "requires a value" branch is deleted outright rather than kept behind an option — an unused strictness mode is speculative generality. Unknown-flag and unexpected-positional rejection, which is what §8.4 actually mandates, is unchanged. --wave needed a third flag kind the original design did not anticipate. `--wave N` is documented (commands/gsd/execute-phase.md:4,48) and the shipped workflow reconstructs and passes it (execute-phase.md:84), while #2932 records token-PRESENCE semantics: the CLI cares only that the flag appeared, and the value belongs to the workflow layer. That is neither a boolean flag nor a value flag, so `optionalValueFlags` now exists — presence-only in `data`, and the validation cursor consumes a following non-flag token so it is not reported as a stray positional. Every other declared boolean flag was checked against every argument-hint and prose usage in commands/, workflows/, agents/ and docs/; `--wave` is the only one of this shape. Five tests were pinning forms that never worked. tests/adr857-core-without-capabilities.test.cjs passed `init plan-phase --phase 01-stub`, but the documented form is positional (docs/CLI-TOOLS.md:776) and the handler reads args[2] — which for that form is the literal string "--phase". Measured on the pre-fix build against a real .planning/phases/01-stub/ directory: init plan-phase 01-stub -> phase_found=true init plan-phase --phase 01-stub -> phase_found=false The test asserted only exit 0 and key presence, so it had been green while proving nothing about phase resolution. Corrected to the documented form and strengthened to assert phase_found === true. Same class in state.test.cjs (`--plan-count`, a flag that does not exist; the real one is `--plans`), milestone-archive.test.cjs (`init new-milestone --json`, silently ignored), and concurrency-safety.test.cjs (a bare positional field name whose OR-assertion passed because a whole-document dump happens to contain the substring it looked for). Six handlers had no argv validation at all — the same #3358 shape this phase exists to close, found while fixing the rest: init verify-work / phase-op / review / todos / remove-workspace read args[2] with nothing checking the rest, and validate health read --repair/--backfill through a bare args.includes() scan that bypassed the parser entirely. All now go through the seam, so the flag has one owner. tests/init-debug.test.cjs rows C4/C5 asserted that an unrecognized flag must NOT fail. That is the behavior §8.4 removes, and Decision 8 says a caller's local expectation does not override §8, so they are inverted and renamed — a test still called "ignores an unrecognized flag" while asserting rejection would be its own defect. Row C6's point is its PWNED canary; that assertion is kept verbatim and only its exit-status expectation changed, because the hostile token is now rejected rather than absorbed. The blast-radius estimate in 40-design.md is corrected rather than quietly left wrong. get_impact reported MEDIUM / 8 symbols upstream, and that was accurate for what the graph can see — parseNamedArgs's callers. It cannot see that those callers' handlers accept argv shapes wider than the code reading args[2] suggests, which is where the real surface was. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): withdraw the validate-health tightening, finish the A2/A3 revert Second full run: 46 failures, down from 90. Four causes, two of them mine. Reverted `validate health` entirely — it was scope creep, and it broke a real flag. ~30 of the 46 read `unknown flag "--json"; accepted: --repair, --backfill`. The previous commit routed `validate health` through the parser on the reasoning that a flag should have one owner. That was wrong twice over: §8.4 names parseNamedArgs and count queries, and `validate health` was never a parseNamedArgs call site — it read its flags, just not through the parser, so it had no silent-drop defect to fix. Tightening it omitted `--json`, which the health-diagnostic suites use heavily. The handler is now byte-for-behaviour back to its pre-branch form. `validate context` stays converted: it genuinely was a call site, and its `--json` is now declared rather than read by a second `args.includes` scan. The five handlers that had NO validation at all — init verify-work / phase-op / review / todos / remove-workspace — stay fixed. Those read args[2] with nothing checking the rest, which is the #3358 shape this phase owns. Finished the A2/A3 revert. Three tests still encoded the deleted "a value flag with a missing value is an error" rule, including one added by the previous commit for that rule. All three now assert the reverted null contract, and the ones whose titles said "rejected" are renamed — a test named for a contract it no longer asserts is its own defect. `--wave=` and `--wave --weird` are correctly rejected. Neither is documented in commands/gsd/execute-phase.md, gsd-core/workflows/execute-phase.md or docs/, and neither is emitted by the shipped prompt layer, so both are unrecognized tokens that §8.4 mandates rejecting. `doesNotConsumeFollowingFlagAsWaveValue` keeps the property it exists for — asserted directly now, at the parser, that `--wave` does not swallow a following flag as its value — and only its exit-status expectation changed. A contradiction inside this branch, surfaced by the audit and resolved the safe way. Two pre-existing #3573 tests call `state begin-phase '2'` and `state planned-phase '2'` with a bare positional, relying on the old permissive parser to ignore it. This branch's own #3358 regression test requires that exact argv to be REJECTED. The two are mutually exclusive. Widening the router to accept a bare positional — mirroring complete-phase — would have silently re-opened #3358, and was verified to do exactly that: with the widened router, `query state.planned-phase 3` returned exit 0 and wrote current_phase_name again. It is reverted. docs/CLI-TOOLS.md:116 and docs/COMMANDS.md:2192 document only the `--phase N` form for both verbs, so the two #3573 tests move to it. Their assertions were never about the call shape — only that total_phases survives the resync — and both still pass. complete-phase is untouched: its bare positional IS documented, and it keeps the dynamic boundary and the negative-space note that record why. The audit that produced this is in the PR body: for every handler whose declaration changed, the flags it reads anywhere in its body, the flags the shipped surface documents, and the shapes the suite passes, compared. The `--json` miss was a pattern, not an accident — declaring a handler's flags from its parseNamedArgs call alone misses whatever it reads elsewhere. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3884): backfill the changeset PR number Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
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> |
||
|
Tom Boucher
|
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> |
||
|
Tom Boucher
|
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> |
||
|
Tom Boucher
|
de95c03f72 |
fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source (#3846)
* test(#3699): failing-first coverage for derived-key reporting and the case-D fallback * fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source * fix(#3699): scope session-field writes to ## Session so an archived line cannot absorb the update * fix(#3699): resolve the session writer from body labels only, so a frontmatter key never writes the body * chore(#3699): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
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> |
||
|
Tom Boucher
|
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
|
||
|
Tom Boucher
|
8d1f770dfe |
test(#3395): pin the clock and scope the stale-prose scan that reddened windows shard 2/3 (#3669)
* test(#3395): failing-first coverage for the silently-ignored clock pin Lands the regression matrix BEFORE the fix so the failure is proven rather than asserted. Three assertions fail deterministically on this commit: 1. PINNED_ENV does not actually pin. `_pinnedNowMs()` (src/clock.cts) returns null unless GSD_TEST_MODE is set, so GSD_NOW_MS alone is discarded and last_updated is stamped from the live wall clock. An instant ending ...:35.149Z contains the substring 35.1, which is what reddens the windows-latest shard 2/3 lane roughly 1 run in 600. 2. The colliding-instant regression cannot reach its instant, for the same reason. 3. The #3052 same-date test never lands on 2020-09-10, so it has been exercising the different-date path and passing for the wrong reason. Also adds currentPositionBlock() plus boundary (ms 099/100/199/200, second 34/35/36, LF and CRLF) and two-arm fast-check coverage for the scoped read the fix will switch to. Refs #3395 * test(#3395): pin the clock and scope the stale-prose scan to the body Drives the failing-first coverage from a5a919ffb green. Two changes, both needed: 1. PINNED_ENV now sets GSD_TEST_MODE alongside GSD_NOW_MS. _pinnedNowMs() (src/clock.cts:44) returns null without it, so the pin was silently discarded and last_updated carried a live wall-clock instant. src/clock.cts is deliberately NOT changed: requiring both keys is what stops an ambient GSD_NOW_MS from freezing a production clock, so the caller was the side that was wrong. 2. The stale-prose assertion now reads currentPositionBlock(stateContent) instead of the whole document. Frontmatter is not phase prose, and an instant ending ...:35.149Z contains the substring 35.1 — which is exactly how a document with no stale prose in it produced 'the stale 35.1 phase prose must be refreshed away'. Confirmed hypothesis: the two defects compose. The inert pin supplies a live timestamp; the whole-document scan turns it into a failure. Either alone is latent, which is why this sat unnoticed for five days and then reddened a lane the release never touched. Also corrects two things the failing-first run exposed. The property test used fc.date() without noInvalidDate, so ~1 sample in 300 was an Invalid Date whose toISOString() threw (counterexample: new Date(NaN)); re-soaked at 5000 runs. And a precondition assertion added to the #3052 block was measured to pass with or without the pin, so it was removed rather than shipped as vacuous truth — last_activity there is body-derived, not clock-derived. Refs #3395 * test(#3395): apply review findings — pin #3052, one fixture builder, CRLF coverage Spec-axis review caught a real slip: the #3052 block carried a comment saying its pin was being added as hygiene, but the RED-state revert had removed GSD_TEST_MODE and the fix commit never restored it. A comment describing an action that was not taken is worse than either doing it or leaving it alone — the pin is now actually there. Standards-axis review flagged the same frontmatter+heading fixture shape being rebuilt in three tests. Extracted one stateDoc({iso, lines, eol}) builder; eol is a parameter rather than a constant because the helper's CRLF behavior is a claim under test. Self-review finding: CRLF was only exercised on a single-heading document, and the following-heading case only under LF — so the exact claim the helper's comment rests on (`\n## ` matches inside `\r\n## ` because the CR precedes the newline) was never actually run. The control test now loops both line endings WITH a following heading. Also drops a comment that restated the PINNED_INSTANT rationale verbatim. Refs #3395 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
46f14c621e |
fix(#3583): one percent per write — route update-progress through the shared computation (#3634)
* test(3583): failing-first coverage for one percent per write state update-progress computes plan throughput (summaries/plans) for stdout and the body Progress bar, while the same write re-derives frontmatter progress.percent as min(planFraction, phaseFraction). Neither consults the other, so mid-phase the file contradicts itself and state json disagrees with the verb that just wrote it. These tests fail on that: equality across stdout, body bar, frontmatter and state json on fixtures where the two fractions differ, plus a derivation-parity test that fails if completedPhases is ever derived by summary parity instead of verification-passed status. Also updates three pre-existing tests that pinned stdout to the plan-throughput value (50->0, 50->0, 100->0). Those fixtures have summarized-but-unverified phases, so the old expectations encoded the bug; changing them IS the fix, as the issue states explicitly. * fix(3583): one percent per write — route the verb through the shared computation RED proven at 7dbbb2d2: 9 failures — the new cross-surface equality tests, the withhold test, and the pre-existing tests whose expectations encoded the bug. state update-progress computed plan throughput (summaries/plans) for stdout and the body Progress bar, while the SAME write re-derived frontmatter progress.percent as min(planFraction, phaseFraction) through a separate path. Neither consulted the other, so on any project where plan throughput ran ahead of phase completion — the normal mid-phase state — the file contradicted itself and state json disagreed with the verb that had just written it. Exit 0, no signal. This is not a dispute about which metric is right. The min cap is deliberate (#3242 Bug B) and is untouched; the fix aligns the printed and body values WITH it. Verified by diff: computeProgressPercent's definition and cmdStateSync are both unmodified. The verb now takes its percent from buildStateFrontmatter — the single owner of the isPhaseComplete-based completedPhases count and the ROADMAP-union totalPhases logic that the frontmatter sync later uses inside the same read-modify-write. Both calls hit the same disk-scan cache against the same on-disk state, so they cannot disagree. Reusing that owner, rather than re-deriving completedPhases locally, is the point: a second almost-identical derivation is the very defect class being fixed, and a parity test now fails if anyone swaps it for summary parity. The first cut fell back to plan throughput when the shared computation withheld. That reintroduced the defect in a rarer case — stdout would print a number the frontmatter deliberately did not contain — so it is gone. The verb now withholds in the same shape as its existing #3217 and #3233 guards. That path is reachable, not theoretical: a bare vX.Y token in ROADMAP prose with no versioned heading leaves the milestone unbounded while both existing guards see a COMPLETE scope. Covered by a test that also asserts state json omits the percent, proving it is the same withhold rather than a divergent local computation. Three pre-existing tests pinned stdout to plan throughput (50->0, 50->0, 100->0); their fixtures have summarized-but-unverified phases, so those expectations encoded the bug. Updating them is the fix, as the issue states. Fixes #3583 * fix(3583): source the reported counts from the same milestone window as the percent The adversarial pass found the first cut left the SAME defect one field over. cmdStateUpdateProgress still reported completed/total from the top-of-function scan, which calls listMilestonePhaseDirs with NO versionOverride — the auto-derived current milestone — while percent now came from buildStateFrontmatter, whose scan scopes by versionOverride: storedMilestone. getMilestonePhaseFilter shows those can select different milestone windows, and #3017's own comment warns about exactly that mis-bind. So a single JSON object could report a percent inconsistent with its own counts: the self-contradiction this issue was filed to close, relocated rather than removed. Counts now come from the same buildStateFrontmatter result as the percent. Proven on a real divergent-milestone fixture where a preamble phase leaks into the auto-derived scan but is excluded from the stored-milestone-scoped one: with the fix stashed the verb emits {percent:0, completed:1, total:2}; with it applied, {percent:0, completed:1, total:1}. The guard scan remains, gating only the #3217/#3233 withholds. Also corrected a comment that overstated caching. Only the phase/plan disk scan is shared between the two buildStateFrontmatter calls; getMilestoneInfo re-reads and re-parses ROADMAP.md and readGitHeadSha spawns a bounded git rev-parse, and both now run twice per invocation. Threading a precomputed frontmatter through the write seam to avoid it was rejected: that seam is the shared ADR-3408 §8.3 composition with three other callers and heavily-documented invariants, and this is not the change to renegotiate it. The comment now says what is and is not cached instead of implying the second call is free. Standards: six new assertions matched raw STATE.md body text the code under test had just produced — the pattern CONTRIBUTING bans by name. They now extract the body Progress field with the repo's own field extractor and assert the parsed percent, so the check survives rewording of the rendered bar. The acceptance criterion still verifies the bar; only what it asserts on moved. Also trimmed ~50 lines of narration around a ~15-line change into a named helper, and fixed a stale test comment that still claimed 100% next to assertions expecting 0%. * chore(3583): add changeset fragment * chore(3583): backfill changeset PR number (#3634) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bcefffc132 |
fix(#3578): derive milestone status from phase counters, not phase-completion prose (#3614)
* test(3578): failing-first coverage for milestone status on partial completion Completing phase 2 of a 4-phase milestone sets frontmatter status: completed while the same call correctly writes completed_phases: 2 / total_phases: 4. These tests fail on that conflation and pin the boundary either side of it (3-of-4 must not complete, 4-of-4 must), plus milestone_name byte-identity and the 1-of-1 case that legitimately does complete. * fix(3578): derive milestone status from phase counters, not phase-completion prose RED proven at 253843b4 (tests-only): the 2-of-4 and 3-of-4 cases failed while the 4-of-4, milestone_name and 1-of-1 controls passed — the conflation, and nothing else. state complete-phase writes body prose `Phase N complete`. normalizeStateStatus matches 'complete' as a case-insensitive SUBSTRING, so phase-level prose collapsed into milestone-level frontmatter status: completed — even while the same call correctly derived completed_phases: 2 / total_phases: 4 / percent: 50. Check ORDER is why the sibling surface stays correct: completePhaseCore writes 'Ready to plan' for non-final phases, hitting the 'planning' arm before 'complete'. The two phase-completion surfaces disagreed and this was the conflated one — a violation of ADR-2207, which gives milestone termination solely to milestoneCompleteCore. buildStateFrontmatter now honors a 'completed' normalization from phase-completion prose only when the counters it already derived agree. Scoped deliberately: - anchored to bare `Phase <token> complete`, so 'All phases complete' and '<version> milestone complete' are untouched (both out of scope). Verified by executing the guard's own regex from source against both forms. - gated on counter trustworthiness (COMPLETE disk scope, finite counts, positive denominator) so an unknown scope withholds rather than guessing 'not complete', which would be the mirror-image bug - normalizeStateStatus itself is NOT modified — it feeds every state.* write and the read path A 1-of-1 milestone still yields 'completed' by the rule, not by exemption, so the #1255 pinning test stays green on its merits. Fixes #3578 * fix(3578): gate the guard on milestone boundedness and close the review gaps Review findings from two orthogonal passes, all fixed inline. GUARD (correctness, from the standards pass): the guard omitted `milestoneUnbounded`, which is the established trust authority for these very counters in this same function — it nulls progressPercent at :2286 and gates the prose fallback at :2294. An unbounded milestone yields a conflated/understated total, so `completedPhases < totalPhases` could be an artifact of a bad denominator and demote a genuinely-complete milestone. Now gated. TESTS: - Prose/guard parity assertion. The guard regex-matches prose emitted from a DIFFERENT file; if that prose drifts the guard silently stops firing and the bug returns undetected. Per the repo's generative-fix-divergence rule, a test now asserts the emitted body Status still matches the guard's pattern — asserting the emitted value against the pattern rather than duplicating the string. - limit+1: completedPhases > totalPhases must NOT fire; inconsistent counters fall through rather than guessing. - Untrustworthy counters (no phases dir → totalPhases null) must NOT fire. - AC4: MCP invoke-command dispatch parity via handleMessage, the criterion both reviewers independently flagged as asserted-but-untested. - Hand-rolled STATE.md writes routed through the existing writeState fixture helper. The adversarial pass independently verified, by reading rather than trusting the diff's own comments, that: paused/stopped short-circuit before 'completed' so a paused milestone can never be clobbered; only cmdStateCompletePhase emits the targeted prose, so no sibling caller over-fires; the counters come from a fresh disk scan independent of this write, so there is no pre/post off-by-one; and the #1255 pinning fixture creates no phases dir, leaving completedPhases null and the guard inert — so that test is provably unaffected rather than assumed to be. * chore(3578): add changeset fragment * chore(3578): backfill changeset PR number (#3614) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
59e7a677fe | fix(#3511): scope every phase-directory scan to the phase it belongs to (#3535) | ||
|
Tom Boucher
|
d922469613 |
refactor(#3408): close the two known limits instead of recording them (#3524)
* refactor(#3408): close the two known limits instead of recording them
Both of these were flagged in review and written down as 'known limits' in a
PR body and an issue comment. CLAUDE.md is explicit that a note is not a fix
and is not surfacing — it is a silent defer. Recording them while closing the
epic was the pattern this epic exists to remove, performed on the epic itself.
syncAndPreserveStateMd and applyPostSyncPreservation each took eight
positional arguments, the last three optional, one of them an out-param. The
review's own wording was that 'a third consumer should trigger an
options-object refactor' — a deferral with a trigger condition nobody would
notice firing. Content and path stay positional; resync, authoritativeFm,
deriveProgressKeys and divergedFields move into a named
StatePreservationOptions. Every call site updated, with tsc as the proof none
was missed.
cmdStateCompletePhase's updated array carried both field labels and a section
name, worked around by a SECTION_ENTRIES Set that re-derived the distinction
by string matching. The kinds are now typed where they are produced and
flattened once at output.
Output contract unchanged: updated is still a flat string array with the same
entries in the same order.
Behavior-preservation was proven rather than asserted — the compiled lib was
built at
|
||
|
Tom Boucher
|
1b027298dc |
fix(#3481): resolve add-roadmap-evolution's phase from STATE.md, not a literal ? (#3522)
* fix(#3481): resolve add-roadmap-evolution's phase from STATE.md, not a literal `?` `state add-roadmap-evolution` built its entry from the raw `--phase` flag alone, so omitting the flag persisted `- Phase ?` even when STATE.md's own frontmatter carried `current_phase` above the insertion point — the #3231 defect at a second call site. Roadmap-evolution entries are the permanent trail explaining why the roadmap changed shape; `Phase ?` makes that trail unattributable, and the command is mostly invoked from agents that do not know to pass `--phase`. The #3481 triage confirmed the #3231 sibling site (`add-decision`) was also still unfixed on next — both PRs that attempted it (#3232, #3347) were closed unmerged. This applies the #3347 treatment to both call sites: - Extracts the write-path phase-resolution ladder `cmdStatePrune` already ran — frontmatter `current_phase` → body `Current Phase` field → prose `Phase: X of Y` scoped to `## Current Position` — into a shared `resolveCurrentPhaseId`, and routes `cmdStateAddRoadmapEvolution`, `cmdStateAddDecision`, and `cmdStatePrune` through it. - Deliberately NOT routed through `resolveStatePhase` (#3208): its `matchCurrentPositionSection(body) ?? body` fallback widens the prose rung to the whole document when no `## Current Position` section exists, where the pipe-table fallback matches any historical `| Phase | N |` row (#1776). Read-path callers (snapshot/validate) report to a human; write-path callers persist durably, so they take the strict rung and render `?` instead of guessing. - The resolved id is returned as written, never parsed to a number (`11-01` and `04.1` are real ids). Prune still parses its own integer cutoff, so its behavior is byte-identical. - Explicit `--phase` still wins and its path is untouched — STATE.md is not even read. When no rung resolves, `?` is still written. Tests: per-call-site coverage for both commands — omitted `--phase` resolves (including a non-integer prose id), explicit `--phase` wins, and two counter-tests pinning the degraded verdict (nothing resolvable → `?`, and a historical `| Phase | 7 |` table row must NOT be adopted). Plus a static guard sweeping src/*.cts for the raw `phase || '?'` placeholder shape so a future call site cannot reintroduce the class. Fixes #3481 * chore(#3481): add changeset fragment for PR #3522 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
411196bc3a |
refactor(#3471): one enforcement point for the empty case, and reports that match the disk (#3519)
* refactor(#3471): one enforcement point for the empty case, and reports that match the disk Implements ADR-3408 section 8.5 and section 8.4's residue (folded in when Phase 3 closed as subsumed). Four items, and two findings the design did not predict. FINDING 1 — the guards could not simply be deleted, as the design instructed. state sync and REGENERATE_STATE never run applyStatePreservation at all, so those six conditions were their ONLY empty-field fallback. A baseline probe on the unedited tree confirmed unconditional deletion drops current_phase, current_phase_name, current_plan, stopped_at and paused_at from a blank-body STATE.md on state sync — breaking the byte-identical requirement section 8.3 grants those two sanctioned-permanent exceptions. They are now GATED, not deleted: on for the exceptions, off for the write seam, where an empty derived value finally reaches the executor unmolested. FINDING 2, the more serious one — there was a FOURTH encoding of this policy. The pre-existing #2202 unknown-key carry-forward loop independently restored the same six fields whenever derivedFm lacked the key, completely neutralizing the fix. It is named nowhere in the ADR, the design, or three prior phases. It was found only because a probe that should have passed did not: the first attempt reported divergedFields: [] and silently restored both fields, reproducing the exact bug this phase exists to close. That is worth stating plainly. This epic's thesis is 'policy declared in one table, enforcement hand-rolled per call site.' The final phase found one more call site than anyone had counted — which is the fourth consecutive time a copy count in this epic proved to be a lower bound. Also: divergedFields could only observe fields the executor actively RESTORED, by diffing postFm. A discard-to-empty is absent both before and after, so it was invisible. A second pass now reports it, which is what makes section 8.5's 'preservation is visible' true for the delete-the-body-line case rather than aspirational. cmdPhaseComplete now reports what it preserved — #3374 was filed against that command and its complaint was warnings: [], silence. cmdStateJson's private third copy of the guards is routed onto the executor's preserve-when-unchanged rule. A read is definitionally not a write, so the #1230 delta is 'unchanged' and curated wins over a stale annotation. shouldPreserveExistingProgress is a different rule and is untouched. Report reconciliation is ONE shared helper across seven commands, not five copies of fix(#3351)'s block. Five copies of a reconciliation is precisely the shape this epic removes, and introducing it in the final phase would have been a poor joke. Both untraced commands were traced rather than assumed: cmdStatePlannedPhase matched cmdStateBeginPhase exactly; cmdStateCompletePhase turned out to be a different legacy hand-rolled path reporting a mix of field names AND a section name, where the naive helper would have dropped 'Current Position' as a false negative every time. * test(#3471): characterization coverage for one enforcement point and reconciled reports Matrix sections A-E, asserted at the consumer's output per ADR-3180 Decision 4(b)/(c) — this phase owes Decision 5's outcome metric, the one the drift guard's zero may never be reported without. Three walls matter more than the new coverage: A2 is SIX separately named tests, one per gated guard, not one parameterised assertion over a list. A list is trivially shortened later; six named tests are not, and six guards is exactly where a field gets silently dropped. A6 pins what Phases 1-3 already fixed — non-empty stale body, delta unchanged, losing to fresher curated frontmatter, with the divergence reported. If A6 reddens, this phase broke the thing the epic was for. D1/D2 pin state sync byte-identical. The implementation had to GATE the six guards rather than delete them precisely because state sync has no executor, and a baseline probe showed unconditional deletion drops five fields. Nothing else in the suite would notice that regression. E6 covers #3345's direction — a field preservation restored that the intent never named IS reported. Nothing has ever tested that direction. Assertions were empirically verified against the compiled lib and the real CLI before being written, since the suite cannot be executed locally. That caught two type bugs in the draft: fm.current_phase after a quoted-YAML round-trip is the string '5', not the number 5. E5 is recorded as structurally unreachable rather than weakened or faked. Those four commands report body Title-Case labels, which cannot string-collide with a frontmatter snake_case key the way cmdStatePatch's arbitrary field names can — which is why fix(#3351) targeted only cmdStatePatch. Testing it directly would need reconcileReportedFields exported from private scope; the helper is exercised through E6 and all seven commands instead. * docs(#3471): amend ADR-3408 section 8.5 — a fourth enforcement point, and guards that could not be deleted Amendment 3. The contract held; two of section 8.5's own statements did not. It said the six empty-only guards are DELETED. They cannot be. writeStateMd is the sole path for both section 8.3 sanctioned-permanent exceptions and never runs applyStatePreservation, so those guards were their only empty-field fallback. A baseline probe on the unedited tree confirmed unconditional deletion drops five fields from a blank-body STATE.md on state sync, breaking the byte-identical guarantee section 8.3 grants it. They are gated instead. It also mis-located cmdStateJson's guards, describing them as living in syncStateFrontmatter. They were a separate private copy on the read path with no delta check at all, so a stale body annotation always beat fresher curated frontmatter in state.json — #3395's shape entirely outside the write seam. THE FINDING: a fourth enforcement point nobody had counted. The pre-existing #2202 unknown-key carry-forward loop independently restored the same six fields, silently neutralizing the fix. It is named nowhere in this ADR, in the phase design, or in three prior phases, and was found only because a probe that should have passed did not. Fourth consecutive time a copy count in this epic proved a lower bound: 2 write-seam bypasses became 4, three preservation encodings became four, and the estimate was wrong every time. ADR-3180's standing rule has earned itself in every phase — read the code, not the write-up. Records the Row 2 decision (a discard-to-empty wins per the delta rule and is reported, not silent — the sharpest Hyrum exposure in the epic), section 8.4's residue landing as ONE shared reconcileReportedFields across seven commands rather than five copies, and the parity assertion added because FRONTMATTER_KEY_TO_BODY_LABEL was itself a second table that failed silently — this epic's shape in miniature, in its final phase. * fix(#3471): repair four regressions the checkpoint caught Checkpoint returned 16 failures of 34389: six real regressions in pre-existing tests, plus seven of my own test bugs. My hypothesis was wrong and is recorded as such. I predicted the #2202 carry-forward skip was the cause, reasoning it had removed a load-bearing fallback the way the six guards nearly were. It was not implicated in any of the six. Three unrelated causes: #2111 — current_phase came back undefined from milestone complete, which is the epic's own defect class reintroduced by its final phase. Root cause is Row 2 working exactly as designed: milestoneCompleteCore rewrites the body Phase: line to a closure message, so current_phase's #1230 delta reads CHANGED and the new rule correctly discards the curated value. The transition never declared any intent to touch that field. Fixed by re-asserting current_phase and current_phase_name through authoritativeFm — the existing #2736 mechanism beginPhaseCore and completePhaseCore already use — rather than by weakening Row 2, which A5 pins. That interaction is worth naming: a rule that keys on 'did this write change the body source' will fire on a transition that moves the body line for an entirely unrelated reason. The design did not anticipate it. #1264 / #3242 / the state.patch progress report — reconcileReportedFields folded EVERY divergedFields entry into updated, including preserve-always progress restores no caller asked about. Now scoped to preserve-when-unchanged rows only. #1162 / case-insensitive table fields — valueOf checked frontmatter before body, so a lowercase table field name exact-matched the lowercase frontmatter key sync always derives, comparing stale pre-sync body text against a post-sync frontmatter enum. Flipped to body-first. That last one is the SAME lesson as Phase 2's patchCore, recurring in a different function two phases later: in this model the body is authoritative and frontmatter is the projection, so a name that could mean either resolves body-first. Twice now. Test bugs: a stray unused parameter shifted every argument at six call sites, so body arrived undefined; and A4 compared nested progress scalars against numbers when extractFrontmatter returns raw YAML strings. The string-vs-number YAML round-trip has now been caught three times in this phase alone. * test(#3471): one helper for the progress coercion that bit four times A2f failed on the string-vs-number YAML round-trip: extractFrontmatter returns nested progress scalars as raw YAML strings, so a comparison against numeric literals can never pass. This is the FOURTH time this exact class has been caught in this phase — twice during test authoring, once as A4 in the previous checkpoint, now as A2f. Patching it a fourth time by hand would guarantee a fifth. Added numericProgress() with a comment saying why it exists, and routed every progress-reading assertion in the #3471 block through it. Swept the block: C3 needed no change, because cmdStateJson's output already runs through normalizeProgressNumbers. Deliberately NOT shared with frontmatter.test.cjs's readPersistedProgress: that one is path-based and re-reads from disk, while these assert on an in-memory string that is never written. Sharing would have meant either a disk round-trip these tests do not do, or duplicating half the helper — so the coercion pattern is mirrored locally and the reason recorded, rather than manufacturing a dependency to satisfy the letter of consolidation. * chore(#3471): backfill pr number in changeset fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e2f4c16d9e |
refactor(#3469): one composition for the STATE.md write seam (#3501)
* docs(#3469): amend ADR-3408 section 8.3 — the pipeline has sanctioned exceptions Section 8.3 read 'Every STATE.md write applies the pipeline.' That is false by design for two commands, and acting on it would have inverted a shipped feature. Preservation makes curated frontmatter win over a re-derived body value. state sync exists to do the opposite — #905's 'body annotation beats existing frontmatter when both are present'; it re-derives frontmatter FROM the body. REGENERATE_STATE is a factory reset that rebuilds STATE.md from scratch. Applying the pipeline to either would re-lock exactly what the command was invoked to replace. This issue's own scope line, inherited from the epic, said to route the direct writeStateMd callers through the pipeline. For cmdStateSync that would have shipped silently, with every gate green, because no test asserts that sync LETS the body win. Caught by reading the helper's docstring and then verifying the claim against the code — a stale comment had already misdirected this epic once. Both commands are now named in a closed exception list and are permanent ratchet entries. Consequence recorded rather than left to bite Phase 4: the 'drive the ratchet to 0 and delete the file' target in this ADR and in #3471 is wrong. Two entries are permanent, so the correct end state is 2, and the honest report is '0 removable bypasses, 2 sanctioned'. A guard reaching 0 here would only do so by having stopped looking at two real writers. * refactor(#3469): one composition for the write seam, not one per caller Implements ADR-3408 section 8.3 as amended. syncAndPreserveStateMd is now the single composition of syncStateFrontmatter and applyPostSyncPreservation. readModifyWriteStateMd and cmdPhaseComplete both CALL it instead of each assembling the two steps themselves. cmdPhaseComplete keeps its own writePlanningFileSet envelope — the composition returns content, it does not take over the write, so STATE.md still commits atomically with ROADMAP and REQUIREMENTS. Assembling the stages at a call site is a re-derivation even when every step calls an owner. Upstream's fix(#3374) routed cmdPhaseComplete through applyPostSyncPreservation but left it calling syncStateFrontmatter directly first, so the composition was duplicated and free to diverge with both guards green. That is ADR-3180 Amendment 2's finding repeating on the write side. cmdMilestoneComplete gains preservation. It wrote through writeStateMd, so it got sync and no preservation — the identical shape #3374 reported for phase.complete, and flagged upstream as a follow-up in the helper's own docstring. This is that follow-up. Divergence is now visible: preservation_warnings names each field restored over a disagreeing derived value. Deliberately NOT named warnings — cmdPhaseComplete already exposes warnings as a prose string array, and two sibling commands carrying that name with different element types is Generative Fix Divergence, the class this epic exists to remove. patchCore stops running stateReplaceField over the whole document. One observable consequence, intended per design row 9: a frontmatter-shaped patch key with no body counterpart now reports failed instead of silently succeeding, because the old whole-document match was literally hitting the YAML line case-insensitively. The guard closes Phase 1's DECLARED KNOWN GAP as promised rather than re-deferring it: section 8.3(b) detection is tractable now the composition exists. Scoped by two factors to avoid Phase 1's measured 29-to-1 false positive rate — a variable field-name argument AND a content argument whose nearest preceding assignment is not stripFrontmatter. Verified 0 findings and 0 false positives across all 33 call sites, plus 5 synthetic shapes. It also detects the re-assembly shape above. Ratchet: 4 entries to 2, both sanctioned-permanent. cmdStateSync's owner changes from #3471 to sanctioned-permanent per Amendment 2 — routing it through preservation would invert the #905 contract. Also fixed inline rather than deferred: cmdMilestoneComplete's STATE.md read now happens inside withStateLock. It previously read outside any lock before writeStateMd took its own, leaving a TOCTOU window under concurrent writers. * test(#3469): characterization coverage for the single write seam Matrix sections A-E. Criterion 6 was amended by maintainer decision — all five instances closed by point fixes while Phase 1 was in flight — so these are characterization tests at the consumer's output per ADR-3180 Decision 4(b)/(c), paired with the drift guard's count, never either alone. Section C is the one that earns its keep. cmdStateSync is a sanctioned permanent exception: state sync exists to re-derive frontmatter FROM the body, so preservation there re-locks exactly what the command was invoked to replace. C1 pins that the body wins; C4 pins that this phase left the command byte-identical. Nothing else in the suite would notice if a future change made sync start preserving, and the natural reading of 'one write seam' is to make precisely that change. Section E pins the guard's false-positive scoping. E4 (updateCore's strip-then-replace) and E5 (sectionBody-scoped calls) must NOT be reported — the naive detector measured 29 false positives to 1 true positive in Phase 1. E7 is the inverse: a sanctioned-permanent entry disappearing must FAIL, because a guard reaching zero here would only do so by having stopped looking at two real writers. Also corrects a stale test that asserted patchCore's old whole-document behavior, which this phase deliberately changes. One honest limitation, flagged rather than papered over: A1's 'byte-identical to pre-refactor' cannot be diffed against real pre-refactor bytes from inside the suite. It is implemented as the seeded fast-check property that cmdPhaseComplete's composed output equals readModifyWriteStateMd's for the same inputs — the strongest available proxy, not the literal claim. * docs(#3469): refresh the seam glossary entry and add the changeset Two spec-review gaps, both real. CONTEXT.md's STATE.md Transition Module entry named three direct writeStateMd callers including cmdMilestoneComplete. This phase routed that one through the composition, so the line was false the moment the refactor landed. Worth recording plainly: I wrote that sentence in Phase 0, correcting an older stale pointer in it, and my own Phase 2 change invalidated it again within the same epic. That is the exact drift this epic exists to remove, demonstrated on the epic's own documentation — and it is why the entry now ends by saying the whole-repo drift guard, not this line, is the authoritative count. The entry now records the composition (syncAndPreserveStateMd) and states that exactly two direct callers remain, both SANCTIONED PERMANENT rather than debt. Changeset: type Changed, because milestone complete's observable output moves. Tier-2 per ADR-3180 Decision 3 — a stale body line no longer wins over fresher frontmatter, and the command gains preservation_warnings. Docs requirement is met by the ADR amendment already in this diff. * test(#3469): register property-test temp-dir cleanup at creation time Standards review, minor but real: the new fast-check property cleaned up its temp dirs in a loop AFTER fc.assert returned. A genuine property failure throws, so that line never ran and every dir from the failing run — including all of fast-check's shrinking iterations — leaked. The failure path is exactly when a littered machine hurts most, and a failing property test is the case the test exists for. Cleanup is now registered with t.after() at dir-creation time, so teardown happens however the test exits. Not try/finally — CONTRIBUTING.md:356 bans it inside test bodies, which is why the after-the-assertion shape existed in the first place. Swept the rest of the branch's test diff for the same shape; phase.test.cjs already uses registered teardown and nothing else matched. * fix(#3469): patchCore routes frontmatter writes instead of dropping them Checkpoint returned 10 failures of 33880. One implementation defect, three test defects, one stale test — all fixed, and the implementation defect is the one that matters. patchCore stripped frontmatter and then reconstructed it VERBATIM, applying no patches to it. An arbitrary custom frontmatter key with no body counterpart and no FIELD_CLASSIFICATION row — risk_level in the upstream fix(#3351) test — therefore always reported failed and silently never wrote. It worked before, via the old whole-document match on the raw YAML line. That is a regression against this phase's own design row 9, which requires frontmatter changes to ROUTE THROUGH the seam — still work, policy-governed — not to stop working. Removing a capability is not routing it. An upstream test caught it, which is the argument for running the checkpoint before believing the refactor. patchCore now partitions by frontmatter shape, decided structurally from the parsed frontmatter's own keys rather than a naming heuristic: - classified keys still report failed — policy owns them and a raw patch may not bypass it; - unclassified keys apply to the frontmatter object and report updated — Phase 1's behavior-table row 19, a field with no row is not this contract's business; - body-shaped keys are unchanged. The property 'failure' was my own test breaking the repo's Clock Seams rule. The two paths agree byte-for-byte; the only difference was last_updated, stamped from the wall clock on two invocations milliseconds apart, so it could never pass. Time is now frozen with mock.timers across both — not by excluding last_updated from the comparison, which would have silently stopped comparing a field the composition writes. B4's fixture could not discriminate: normalizeStateStatus maps any text containing 'complete' to 'completed', and milestone complete's own new body value derives to exactly that — which was also the fixture's stale value. The stale value is now 'executing' so the assertion can tell 'body correctly won' from 'stale survived'. B5's fixture tripped a pre-existing unstarted-phase guard before reaching any write-seam code; it now has the matching phase directory. D9 asserted the old exempt set. readModifyWriteStateMd now calls one symbol rather than assembling two, so it needs no exemption; syncAndPreserveStateMd is the sole legitimate composition site. * fix(#3469): patchCore resolves body-first, so the body wins a name collision Re-verification returned 2 failures of 33880, both D4 — the hostile row for a key that exists as BOTH a frontmatter key and a body field. The partition checked frontmatter first, so 'status' — classified in FIELD_CLASSIFICATION and also present as a body 'Status:' line — routed to the frontmatter branch, was rejected as classified, and reported failed. Wrong order. Patching 'status' means the body field, and upstream fix(#3351) says so in its own comment: 'the legitimate working case for state.patch is display-cased BODY fields — Status, Current Plan, Phase.' The body is authoritative in this model; frontmatter is the projection. D4 asserted exactly that and was right. Resolution order is now body, then frontmatter: 1. resolves to a body field -> apply to body, updated 2. else an own key of the frontmatter: classified -> failed (policy owns it) unclassified -> apply to frontmatter, updated 3. else -> failed Verified by probe against the compiled lib for all four cases rather than asserted: risk_level (frontmatter-only, unclassified) still lands; current_phase still fails; display-cased Status unchanged; D4's lower-cased status now lands via the body with the frontmatter untouched. The current_phase case was the one that could have regressed silently, so its fixture was read rather than assumed — D1's body carries 'Phase: 3 (alpha)' and no 'Current Phase:' line, so body-first cannot reach it. * chore(#3469): backfill pr number in changeset fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
1218d76d62 |
refactor(#3468): dispatch state preservation on the declared policy, not the field (#3495)
* test(#3468): add write-path drift guard, ratcheted at its measured baseline Guard-first, per ADR-3180 Amendment 3's standing rule that a phase builds and runs its guard BEFORE its scope is fixed, and states its copy count as 'N found by the guard', never 'N per the epic'. Measured, not assumed: Axis 1 (policy dispatch, ADR-3408 section 8.1) — 7 violations, RED by design. 5 field-name-keyed getFieldClassification('literal') branches plus 2 declared FieldPreservation members with no executor at all (derive, clear). This is the fail-first evidence for the refactor. Axis 2 (write seam, section 8.3) — 4 bypasses, ratcheted. Epic #3408 scoped this at two writers; the whole-repo scan found four, and one the epic named (patchCore) is not among them because it bypasses via stateReplaceField rather than the seam calls. Fourth consecutive time an epic's copy count proved a lower bound. Two detectors were written and removed again before this commit, both recorded in the file header rather than silently dropped: - A prompt-layer detector that reported 5 backticked prose mentions as drift. That is ADR-3180 Amendment 3's recorded false-positive class, and CONTRIBUTING.md already settles it: a backticked command reference is a mention. Now gated on inline-code spans. - A stateReplaceField co-occurrence detector for section 8.3(b). Measured at 29 false positives to 1 true positive — it matched the function's own definition and ~20 calls on frontmatter-free body slices. Banking 29 non-defects to catch one is the 'ratchet as a parking lot' gaming route Decision 5 names, so it is a DECLARED KNOWN GAP owned by Phase 2 (#3469), which both fixes it and makes its detection tractable. * test(#3468): failing-first coverage for policy dispatch and the loud failure Matrix sections A, B and C from 50-test-matrix.md. Expected RED against this tree, confirmed by static trace rather than assumed: B1, B2, B3 — an unwired declared preserve-when-unchanged row must throw with code STATE_PRESERVATION_UNWIRED_ROW and a structured .field. Today src/state-transition.cts:314 silently continues. A4 — a whitespace-only snapshot is restored today, because the guard is .length > 0. Required behavior is skip. Everything else is characterization, locking in behavior the refactor must preserve. C1 is table-driven over every FIELD_CLASSIFICATION key; C2 pins current_phase_name's exact outputs as literals, because its row is being reclassified preserve-always to preserve-when-unchanged as a behavior-preserving change and nothing else would catch a drift. C3 is a seeded fast-check property (seed 3468, 200 runs, replay data on failure). A22 is deliberately NOT a behavioral test. Whether 'derive' has an explicit executor is not observable through applyStatePreservation's public API — it is a structural property, and the drift guard's unimplemented_policy axis is what enforces it. That split is ADR-3408 Decision 5's own pairing: the lint is the structural metric, the test is the outcome metric, and neither is reported alone. * refactor(#3468): dispatch preservation on the declared policy, not the field Implements ADR-3408 sections 8.1, 8.2 and 8.6. applyStatePreservation is now one loop over FIELD_CLASSIFICATION dispatching on the row's preservation value, with four small executors — one per FieldPreservation member. No branch is selected by field name. Zero literal-argument getFieldClassification calls remain. Behavior-preserving for 16 of 20 input classes. The four that change: - An unwired declared preserve-when-unchanged row now THROWS (code STATE_PRESERVATION_UNWIRED_ROW, structured .field) instead of silently continuing. This fires only on an internal invariant violation with both ends in our own source; a drifted, malformed or unparseable user STATE.md must never reach it, which is section 8.2's bright line and what test B8 proves through the real CLI. - derive gained an explicit no-op executor. That is what makes the throw decidable: 'policy says do nothing' is now distinguishable from 'nobody wired this'. - current_phase_name's row is corrected from preserve-always to preserve-when-unchanged. The row was wrong, not the code — it has always been delta-gated on the body Phase line, so preserve-always had two divergent implementations. Behavior is unchanged and test C2 pins it. - A whitespace-only snapshot is no longer restored; the check is trimmed. clear is deleted from the FieldPreservation union — no row used it and no executor existed. Speculative Generality: a policy invented for a need that never arrived. Verified zero dependents. The caller folds six dedicated pre/post parameters into one bodyDeltas map keyed by field, so all seven preserve-when-unchanged rows travel one channel instead of two. Two shapes for one kind of data is why the executor needed per-field branches at all. Also fixed, found while reviewing the refactor rather than deferred: - applyPreserveIfPlaceholder opened with a field-name literal test, which section 8.1 forbids outright. The executor is idempotent, so the test bought nothing. The drift guard could not see it, so Axis 1 is widened to catch field-variable comparisons against literals — the guard reported zero while a violation sat in the file it polices, which is Goodhart's gaming-by-indirection. - loadBaseline conflated an unreadable baseline with an absent one. A guard whose own diagnostic collapses two states into one identical result reproduces the exact failure shape this epic exists to remove. * docs(#3468): record Phase 1 validation as ADR-3408 Amendment 1 Amendment 1 records what Phase 1 found, per ADR-3408 section 8's rule that a behavior it does not state is not decided: - preserve-always had TWO divergent implementations; current_phase_name's row was wrong and is reclassified, behavior unchanged. - section 8.6 resolved: clear is deleted, zero dependents. - the closed guard vocabulary is real and has exactly one true member, because stopped_at's scoping turned out to be caller-side extraction. - copy count found by the guard: 4 write-seam bypasses where the epic scoped 2, and patchCore — one of the two it named — is not among them. - two detectors built and removed again, with their measured false-positive rates, so nobody re-attempts them. - a DECLARED KNOWN GAP for section 8.3(b), owned by Phase 2. - Decision 5's anti-gaming list earned itself twice in one phase. Also adds the changeset fragment. * test(#3468): fix review findings — try/finally, stale clear allowlist, ratchet owners Standards axis, both hard violations: - tests/state-write-path-drift-guard.test.cjs wrapped stdout/argv/exitCode restoration in try/finally inside the test body. CONTRIBUTING.md:356 forbids it outright, and the correct t.after() pattern was already in use two lines up in the same test. - tests/state-transition.test.cjs still listed 'clear' as an allowed FieldPreservation value in the row-enumeration test AND the getFieldClassification property test, after this PR deleted it. A stale allowlist weakens the property's negative space — it would accept a resurrected clear row as valid. Contract tension, resolved rather than left: ADR-3408 section 8.3 requires each ratchet entry carry the issue owning its removal. All four shipped with owner: null. The guard was right not to INVENT one, but the owners are known from the phase plan, so recording them is not inventing: phase.cts -> #3469, state.cts and milestone.cts -> #3471, health-diagnostic.cts -> sanctioned-permanent. Rather than a JSDoc caveat, --baseline now MERGES prior owner values on the (file, source) key, so a mechanical regeneration can no longer silently discard curated provenance. Verified by regenerating twice. * fix(#3468): sanitize attacker-controlled fields on every guard output path Isolated security review, MEDIUM, confidence 8/10. findSeamBypasses and findPromptSeamUses built findings with an UNSANITIZED `file`, while the co-located `source` on the same object was correctly wrapped in sanitizeForReport. On a fork PR a filename is exactly as attacker-controlled as a source fragment — a repo can legally track a filename carrying C1 control bytes or bidi overrides. The raw value reached two paths: --json stdout, and the COMMITTED baseline JSON via buildBaselineEntries. JSON.stringify neutralizes C0 controls but does NOT escape C1 (0x7f-0x9f) nor the bidi/zero-width range sanitizeForReport exists to strip — which is the precise threat the guard's own header names. Only the human formatter was safe. Sanitization now happens at CONSTRUCTION, so every consumer inherits it rather than each output path having to remember. The same defect was present on `field` and `policy` and is fixed alongside. Double-sanitization in the formatter is left in place, verified idempotent: escaped output is ASCII and cannot re-match the control/bidi classes. Also: the guard was not referenced anywhere in package.json, so nothing ran it. A drift guard nobody runs is not a guard, and ADR-3408 Decision 5 assumes it runs. Wired into lint:ci beside its sibling drift guards; it was already green on this tree, so the chain stays green. * chore(#3468): re-curate ratchet after an upstream rewording of a tracked bypass The rebase onto origin/next turned the guard red on its first real day, which is the ratchet working rather than a defect. |
||
|
Tom Boucher
|
be9329b10b |
fix(#3374): phase.complete stops harvesting stale body stopped_at (#3491)
* fix(#3374): phase.complete stops harvesting stale body stopped_at Variant A: cmdPhaseComplete's adapter calls syncStateFrontmatter directly (deliberately - STATE.md commits atomically with ROADMAP/REQUIREMENTS), which also bypassed the #948/#1230 preservation pass every RMW write gets. A stale body 'Stopped at:' line then silently clobbered a fresher frontmatter stopped_at on every phase completion, with warnings: []. Three layers close it without reversing #3517's refresh expectation: - completePhaseCore now refreshes the body continuity line it implies ('Phase N complete, ready to plan Phase N+1'; ADR-2207 phrasing on the last phase), session-scoped via the new stateReplaceFieldInSession seam so a decoy bold Stopped-at line in an unrelated section cannot absorb the refresh. Replace-only - a layout with no session line keeps its shape and its frontmatter value survives via the preservation delta. - the RMW post-sync preservation chunk (snapshots + table-driven applyStatePreservation + #2736 re-assert, full bodyDeltas wired) is extracted into the shared applyPostSyncPreservation helper; the phase.complete adapter and writeStateMd (milestone complete / state sync - the gap the closed PR #3442 review flagged) now run it too. - cmdStateRecordSession pushed 'Stopped At' onto updated[] on any label MATCH, including a value already on disk - reporting a write that never changed a byte. It now reports only on real change, and the match is tracked separately so an identical value does not arm the #944 DWIM section rewrite (which would reset an executor-authored resume file to None). * docs(#3374): backfill changeset pr field to 3491 * fix(#3374): drop the writeStateMd preservation pass - state sync's #905 contract is body-wins CI on this PR caught what the closed PR #3442 review's MAJOR remediation option (a) would have broken: state sync's #905 contract ('body annotation beats existing frontmatter when both are present') is the opposite by design - sync exists to re-derive frontmatter from the body. A blanket applyStatePreservation pass on writeStateMd re-locked stale frontmatter (current_phase 3 over the body's 5) on every sync. Take the review's sanctioned option (b) instead: the scope claim is accurate (phase.complete only) and the milestone complete / state sync exposure is tracked as follow-up issue #3492. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8bead8b0ff |
fix(#3395): own the phase line in planned-phase and persist --name (#3490)
* fix(#3395): own the phase line in planned-phase and persist --name * fix(#3395): backfill changeset pr 3490 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
58e3437a48 |
fix(#3351): reconcile state.patch report with persisted state.md (#3487)
* fix(#3351): reconcile state.patch report with persisted state.md * chore(#3351): add changeset fragment * chore(#3351): backfill pr number in changeset fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
69e7afd0c7 |
chore(#3212): bounded quantifiers over document content — prohibition with teeth — Phase 4 (#3441)
* feat(#3415): ship local/no-unbounded-quantifier, burn down ReDoS class Phase 4 of epic #3212 (ADR-3212 §5/§7, the final phase). New rule flags an unbounded */+/{n,} quantifier over a broad character class ([\s\S], dotAll ., or a 1-2-unit negated class like [^\n]/[^)\n] — the exact #2128-fixed shape) applied to a regex whose match target is data-flow-traced to readFileSync content. eslint-rules/lib/readfilesync-trace.cjs extracts the data-flow tracer shared with no-crlf-fragile-split (Phase 2) rather than a second copy — no-crlf-fragile-split refactored onto it with zero behavior change, parity-tested. Real triage, not 798 mechanical edits: the ADR's census (2026-08-08) screened every unbounded quantifier in the tree unscoped. Correctly scoped to readFileSync-derived content (matching Phase 2's own G2/G3 scoping), the rule found 162 real hits across two detection waves — the second wave (93) surfaced only after a genuine off-by-one bug in this rule's own first draft was caught while writing its RuleTester tests and fixed (the bug silently missed every directly-quantified [\s\S]* with no gap before the quantifier — exactly the class this rule exists to catch). 3 hits landed in production src/ (commands.cts, milestone.cts, roadmap.cts) and were each empirically timed against adversarial input (matching #2128's own measured-not-assumed precedent) — all confirmed linear-time/benign, left unbounded with a measured-evidence comment rather than mechanically bounded. The remaining 159 are test-file fixture parsing (test-author-controlled, fixed-size content, not adversarial input) — each suppressed with a specific, non-generic reason. Zero functional behavior changed anywhere in this diff. tests/no-pending-3212-markers.test.cjs locks the epic's own closing invariant (ADR §7: "assert zero pending #3212 markers remain") — ground truth confirmed trivially true today (no phase left any such marker behind), now regression-locked going forward. Design: .gsd/phase/chore-3415-prohibition-with-teeth/40-design.md Test matrix: .gsd/phase/chore-3415-prohibition-with-teeth/50-test-matrix.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): correct rule category mislabel, add CI test-scope entry An orthogonal Standards-axis review found eslint-rules/no-unbounded-quantifier.cjs mistakenly carried meta.docs.category: 'Portability', copied from a sibling rule without realizing what that implied: docs/contributing/cross-platform- portability-rules.md governs an ADR-1703 rule family under a hard "zero escape hatches" contract (tests/portability-rule-disable-ban.test.cjs's PROTECTED_RULES bans eslint-disable for those rules entirely). This rule is not part of that family — it's ADR-3212 (ReDoS/CWE-1333), a different epic — and its eslint-disable-next-line suppressions (159 of them, added earlier this same phase after empirical benign-verification) are an intentional, correct design, not a bypass. Corrected to category: 'Best Practices', matching the actual precedent (no-adhoc-regex-escape.cjs, Phase 1 of the same epic, which is also correctly outside PROTECTED_RULES), and the rule's own docstring now states this explicitly so a future reader doesn't have to re-derive it. Also registers a new scripts/ci-test-scope.cjs bucket so editing this rule or the shared eslint-rules/lib/readfilesync-trace.cjs helper re-runs their own test suites under targeted CI selection — was previously unregistered and invisible to that fast-path (this PR's own gsd-test checkpoint runs the full suite regardless, so this only affects future narrowly-scoped PRs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): bound no-unbounded-quantifier's own scanner (CWE-1333, ironic) Security review found the rule meant to catch algorithmic-complexity bugs had one of its own: hasUnboundedBroadQuantifier's negated-class inner scan walked from each `[^` occurrence to the next `]` (or EOF) with no bound, while the outer loop only ever advanced by one character — O(n²) total work on a pattern with many unclosed `[^` runs. Runs unconditionally inside checkPattern on any `new RegExp('literal string')` argument in any linted file, before the (cheap) readFileSync data-flow gate — so a single crafted string literal, no valid regex syntax required, could make `npm run lint` / CI hang. Empirically confirmed both the bug and the fix: pre-fix, n=4000/8000/ 16000/32000 chars took 30.8/115.6/463.8/1874.3ms (~4x work per 2x n, quadratic); extrapolated, the 300000-char repro from the finding would run ~165s. Post-fix (bail the inner scan once units exceeds the rule's own 1-2-unit scope, rather than continuing to hunt for a closing `]`), the same 300000-char input runs in 8.7ms via the real rule module, independently reconfirmed at 18ms via a fresh Linter.verify() call. New regression row in tests/no-unbounded-quantifier.rule.test.cjs asserts the RuleTester run on a 50000-char adversarial pattern completes and returns a defined result — no wall-clock assertion (CLAUDE.md Clock Seams / local/no-elapsed-assertion). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): triage 3 new sites, re-raise ceiling after upstream batch next merged 12 more PRs during this PR's review. Two consequences: - tests/edit-phase.test.cjs (fix #3262, unrelated) added 3 new content.match(/<tag>([\s\S]*?)<\/tag>/) reads of this repo's own workflow .md content — the same Class A pattern as the ~159 sites already triaged elsewhere in this PR. Suppressed with the same established reason. - lint-allow-test-rule-refs' ratchet ceiling needed re-raising again (301 -> 303) for the same reason as the two prior bumps: organic growth from unrelated, already-reviewed PRs landing concurrently, not a defect in this branch's own diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
5fff839713 |
fix(#3258): honor all field-classification preservation rows (#3447)
* fix(#3258): honor all field-classification preservation rows * chore(#3258): set changeset pr to 3447 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
6dbc124018 | enhance(#3180): the sibling validators share one envelope and one owner — Phase 12 (#3407) | ||
|
Tom Boucher
|
2b20b7e2cd |
fix(#3257): preserve full-line frontmatter comments through the parse→reconstruct pair + syncStateFrontmatter (#3387)
* test(#3257: full-line frontmatter comments survive the parse→reconstruct pair AND a mutating state verb parseYamlRegion dropped column-0 # comments and reconstructFrontmatter rebuilt from Object.entries alone, so full-line comments were silently destroyed on every mutating STATE verb. Add failing-first regressions: 3 unit tests for the public pair (comment between keys, leading+trailing, consecutive) and an e2e test running a state verb (state update) on a commented STATE.md — the e2e exercises syncStateFrontmatter's fresh-derivedFm rebuild path, which is the actual loss site the issue is filed against. RED — fails on next; fix follows. * fix(#3257: preserve full-line frontmatter comments through parse→reconstruct AND syncStateFrontmatter Carry column-0 # comments through the frontmatter pair via a Symbol-keyed channel (FULL_LINE_COMMENTS): parseYamlRegion captures ^# lines and attaches them to the next top-level key (leading) or a trailing slot; reconstructFrontmatter re-emits them in place. The Symbol is invisible to Object.entries/keys/JSON, so every existing reader is unchanged; the channel is created only when a comment is seen, so comment-less frontmatter is byte-identical. CRITICAL (isolated review): syncStateFrontmatter rebuilds its target via buildStateFrontmatter (fresh object) + an Object.keys carry-forward, both of which skip the Symbol — so the pair-preserving channel was lost on the very STATE verbs the issue names. Export propagateCommentChannel(source, target) from frontmatter.cts and call it in syncStateFrontmatter before reconstruct, copying the channel onto derivedFm (leading filtered to keys still present so a deleted key's annotation drops with it, trailing preserved). Decision A. * chore(#3257: add changeset fragment * chore(#3257: backfill changeset PR number (#3387) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
23e6d49929 |
fix(#3233): no-op state update-progress when the milestone scan finds zero plans (#3375)
* test(#3233): zero plans (0/0) is a no-op; plans-but-none-done still writes 0% cmdStateUpdateProgress mapped 0/0 through clampPercent to 0% and rewrote the shipped Progress record after milestone close. Replace the stale 'handles zero plans gracefully' test (which asserted the buggy percent:0) with a #3233 no-op regression (100% record preserved, updated:false), and add a negative-space guard: plans exist but none done must still write a legitimate 0%. RED — fails on next; fix follows. * fix(#3233): no-op state update-progress when the milestone scan finds zero plans cmdStateUpdateProgress mapped 0/0 through clampPercent to 0% and unconditionally rewrote the body Progress line, so after /gsd-complete-milestone archived the phases (.planning/phases/ empty, scope COMPLETE) a routine update-progress run destroyed the shipped record ([██████████] 100% → [░░░░░░░░░░] 0%). Add an early-return no-op when totalPlans === 0 — mirroring the established scope-withholding no-op (stderr WARNING + {updated:false, reason}) and computeProgressPercent's null-for-empty contract ('nothing to measure' ≠ '0% done'). The legitimate 0% case (plans exist, none summarized) is unaffected: totalPlans > 0 reaches clampPercent(0, N>0) = 0 and writes 0% as before. * test(#3233): unshadow 'Progress field missing' — clear the zero-plans guard The new totalPlans===0 no-op guard fires before the 'Progress field not found' branch, so the existing 'returns error when Progress field missing' test (no phase dirs → 0 plans) was passing for the wrong reason and that branch lost coverage. Give that test a phase dir + PLAN so totalPlans > 0 clears the guard and it reaches the branch it is named for. (Isolated review finding.) * chore(#3233): add changeset fragment * chore(#3233): backfill changeset PR number (#3375) --------- Co-authored-by: sim <sim@local> |
||
|
Dennis Kim
|
5e951540af |
fix(#3162): resolve active state phase before drift scan (#3208)
* test(02-01): reproduce template state validation drift - derive command fixtures from the shipped STATE template - pair passed-verification drift with a clean opposite-result control * test(02-01): cover state phase resolution boundaries - exercise precedence conflicts fallbacks and fail-closed directory handling - prove canonical equality and reject outside-root verification evidence * docs: add changeset for PR #3208 * Address review feedback * fix(#3162): preserve phase validation after state refactor * test(#3162): align validation scope cases |
||
|
Rezolv
|
e87fb409ee |
enhance(#2573): stamp STATE.md with its commit and surface a freshness hint (#2622)
* enhance(#2573): stamp STATE.md with its commit and surface a commit-age freshness hint Adds a `state_head` stamp to STATE.md and derives a tri-state commit-age freshness proxy (state_commits_behind / state_commit_stale) through state.cjs's readStateHeadFreshness, surfaced on smart-entry signals and as health W024. The proxy is advisory: classify() deliberately does NOT consume it (ADR-1787 locks the classification/routing boundary — a signal, not a route). Composes with #3099 and #1882 (both merged to next after this branch): the commit-age proxy reads `state_head` while the LAST_ACTIVITY_UNPARSEABLE diagnostic reads `last_activity` — two different fields, not "two staleness signals on one field." A new regression test asserts a STATE.md carrying both an unparseable last_activity AND a valid state_head resolves each independently (diagnostic fires once; freshness reads state_head, commits_behind 0). Rebased onto next (flattened): resolved the add/add conflicts in src/smart-entry.cts (kept both the #2573 freshness import/derivation and the #3099 diagnostic import/call) and tests/smart-entry.unit.test.cjs (kept both describe blocks). Drift-ack for health.md's W024 row is unchanged (12348 B). Tests: smart-entry 62, state/state-transition/health/verify 639, all pass. * chore(#2573): allowlist health-validation test in the prompt-injection scan The scanner's `exec('` code-execution pattern matches the benign `re.exec('<phase-id>')` RegExp method calls in the phase-ID grammar tests (pre-existing: 16 such calls on next, this PR adds none). The file entered the diff-mode scan's changed-file set only because #2573's W024 state_head assertions touch it. Allowlist it alongside the other test files that carry pattern-matching content as data (same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class). Scanner self-test 38/0; diff scan 14 files, 0 findings. |
||
|
Tom Boucher
|
aceea3ce4a |
refactor(#3217): withhold a percentage when its scope is not complete (#3318)
* wip(#3217): rule-4 scope withholding — parked, two open findings Implemented but NOT shippable. An isolated review found buildStateFrontmatter still hardcodes SCOPE.COMPLETE, so state json reports percent 0 where roadmap analyze, stats and query progress all correctly report null on the same disk state - rule 4 reintroduced at a site this phase claims to close. Also: roadmap analyze emits scope complete beside progress_percent null with nothing explaining it. Parked to build Phase 4 (#3186) first, which is unblocked. Findings recorded in .gsd/phase/refactor-3217-completion-ratio-scoping/60-review.json. * fix(#3217): withhold the sync percentage on a non-complete scope The parked blocker is fixed - buildStateFrontmatter no longer hardcodes SCOPE.COMPLETE, and the prose Progress fallback is gated too, which was a second leak found while tracing the first. roadmap analyze exposes progress_scope so a consumer can tell WHY a percentage is absent from the JSON alone. Then a residual gap was reproduced rather than assumed. cmdStateSync carried the same hardcode behind a written reason claiming it did not reproduce. It did: on a TRUNCATED window and on UNSCOPED row 4, state sync wrote Progress 0 percent to 100 percent while state json, roadmap analyze, stats and query progress all withheld - and it persisted a self-contradictory file, body claiming 100 percent while its own frontmatter correctly omitted percent. The excuse was also wrong. syncRoadmapRaw is already parsed in that function and is exactly what produces a real scope, so there was a scope to pass. Threaded through listMilestonePhaseDirs; a non-complete scope now skips the write with a reason in changes. milestoneBounded stays as the orthogonal 1761 guard for row 5. Second time this epic a does-not-reproduce claim was too generous. Recorded in ADR Amendment 8 as a correction rather than a quiet rewrite. Verified on the remote runner. * test(#3217): give the withholding fixtures a resolvable scope 40 matrix failures, all fixture drift - no code regression. My own hypothesis that this was over-withholding was wrong and is recorded as such: the worry case, a plain ROADMAP with Phase entries and no version heading, resolves to complete exactly as ADR 7.1 says it should. The real causes were two fixture shapes. Most had no ROADMAP.md at all, which is unreadable via a pre-existing graceful path, and asserted a numeric percent. The five vscode, pi-extension, mcp-server and shell-projection failures were that shape - bare temp dirs using progress json as a reachability proxy while asserting typeof percent is number, which under rule 4 is now null. The rest had a version token in a title or heading with no STATE.md milestone pointer to resolve it, which is classification row 4, versioned but unresolved, so withholding is correct per the contract. Verified on the remote runner. * test(#3217): make the LM-tools reachability tests dispatch against their fixture The gsd_progress reachability test was never testing its fixture. invoke() resolves cwd from vscode.workspace.workspaceFolders by design (the real LanguageModelToolInvocationOptions has no cwd field, per the 2103 fix in extension.js), the mock had no workspace at all, and the test passed a cwd option nothing reads - so it dispatched against the repo working directory. Writing a ROADMAP into the temp dir had no effect. Rule 4 only made it visible. Fixed by mocking workspaceFolders. The two siblings in the same file carried the identical dead cwd and were dispatching against the repo too; they were not failing only because their assertions did not touch scope-dependent output. Both now use their own fixture with assertions unchanged - the no-planning fallback paths already satisfy them honestly. Re-scanned the other five reachability files: no further instances. They thread cwd into parameters that genuinely read it, not through an options shape that ignores it. Verified on the remote runner. * chore(#3217): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3318 exists. * ci(#3217): give the coverage merge enough heap for the merged shards The coverage gate OOMed at exit 134. c8 report merges three shard artifacts, roughly 358MB of V8 dumps in coverage/tmp, and died holding their per-file position maps at the ~4GB default heap. Verified as this branch's delta rather than pre-existing: the same job succeeded on next at 14:18, after phases 4 and 5 merged. Both coverage-gate steps get the bump because both re-slice the same merged data. 8192 doubles what failed and leaves headroom on a 16GB ubuntu runner, matching the idiom the shard step already uses at 6144. This is a memory bound, not a change to what is measured. No threshold was touched. The test file was checked for gratuitous subprocess spawning and is already reasonable at 43 spawns, each a distinct fixture-by-surface pairing. Verified on the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e201cde73c |
refactor(#3186): one shared phase-completion predicate, disk-strict (#3306)
* docs(#3186): record the disk-strict completion decision in ADR-3180 7.4 The maintainer decided #2957 on 2026-08-08: disk state is authoritative and a ROADMAP checkbox is a human annotation with no machine authority. Section 7.4 still carried the OPEN QUESTION and was marked blocked, so the contract said one thing and the tracker another. Recorded per section 7's own rule - a behavior not stated there is not decided, and amending a rule is an ADR amendment rather than a code change with a comment. The decision comment names Phase 4's PR as the carrier of this edit and makes it an acceptance criterion that the text be in the tree before implementation begins, so this lands first, alone, ahead of any code. Also clears the stale blocked-on-2957 row in the guard roster. * refactor(#3186): one shared phase-completion predicate, disk-strict isPhaseComplete in verification.cts becomes the single owner. It calls readVerificationStatus UNCONDITIONALLY - plan count is not a precondition - so a zero-plan phase with a passing VERIFICATION.md is complete. That is #3168: init gated the read on a plan count and synthesized a not_required sentinel, so phase.complete succeeded while init.manager reported incomplete for the same phase. The guard, built and run before scope was fixed per Amendment 3, found 9 re-derivations where the ADR named 3. Four were unnamed, including one in the prompt layer: mvp-phase.md ORed a ticked checkbox with disk status, which under disk-strict is the divergence itself. Per the #2957 decision, a ticked ROADMAP checkbox is a human annotation with no machine authority. The overrides in roadmap analyze and init manager are deleted rather than generalized; the user's checkbox stays in ROADMAP.md, only its authority goes. scanPhasePlans.completed and buildWorkstreamInventory are deliberately NOT folded - they answer 'are all plans summarized', which is a different question, and folding them would either over-report completion or invert the dependency direction between Phase 1's owner and this one. Verified on the remote runner. * fix(#3186): close seven review findings and record the missing-verdict rule The isolated review reproduced a write-path regression I introduced: migrating cmdRoadmapUpdatePlanProgress dropped its summaryCount>=planCount gate, so a phase with a fresh passing verification plus a newly-added unsummarized plan reported complete AND wrote a checkbox into ROADMAP.md while phase complete refused. The owner stays right per 7.4 - plan count is not a completion precondition - so the gate is restored at the write site as an explicit composition, mirroring the separate 2648 unexecuted-plan gate cmdPhaseComplete already carries. The spec axis was right that my 0.x-split reasoning was too permissive. The 2957 decision names buildStateFrontmatter as one of the three that must converge, and buildWorkstreamInventory combined a summaries-met local with verification data to decide the same verdict - Decision 4(c)'s named bypass, and it reproduced 3168 in a third surface. Both now route through the owner. The raw scanPhasePlans helper stays: it answers are-plans-summarized, which genuinely is a different question. Maintainer decision recorded in 7.4: a missing verdict is not a passing one, so an absent VERIFICATION.md means not complete everywhere. That retires 2645's verifier-disabled tolerance and inverts its Goodhart incentive - deleting the evidence now lowers completion instead of raising it. Guard hardened: block-form count gates and algebraic restatements are caught, and the header now discloses its remaining limits instead of overclaiming. Verified on the remote runner. * fix(#3186): route state sync through the owner and catch bare completed reads The matrix found 52 failures. 51 were fixtures asserting the old semantics: a phase with plans and summaries but no VERIFICATION.md used to count complete and correctly no longer does. Each fixture now carries a passing verification where that is what the test was actually about, rather than having its assertion weakened. The 52nd was a real 10th re-derivation the guard could not see. cmdStateSync destructured scanPhasePlans().completed directly - a bare field read, not a comparison - and used it as a completion verdict, so state sync and state json disagreed on completed_phases for identical disk state. Routed through the owner. Guard gains shape (d): any read of .completed off a scanPhasePlans() result outside plan-scan.cts, in chained, destructured and indirect forms, function scoped with no line window. It cannot tell a summaries-met read from a completion read - that is data flow - so it flags every one and requires a written-reason exemption, which is the same discipline shapes a-c already use. The blind spot is disclosed in the header rather than overclaimed. The emitted-attribution failure was also mine, not pre-existing: the mvp-phase.md checkbox-OR removal moves emitted bytes, acknowledged in tests/emitted-drift-acks. Verified on the remote runner. * test(#3186): give the nested-plans sync fixture a passing verification Last 3 matrix failures were one failure echoing up two describe levels. Phase 01-alpha had plans and summaries but no VERIFICATION.md, so under disk-strict completed stayed 0 and no Progress change was emitted - correct new behavior, not a regression. Added the passing verification rather than dropping the Progress expectation, so the test still covers what #3257 is about: that a nested plans/ layout is counted and not undercounted. Probe against the built lib confirms Progress: 0% -> 50% alongside Total Plans in Phase: 0 -> 3. * chore(#3186): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3306 exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
693f12ad56 |
refactor(#3187): give state field extraction one canonical owner (#3283)
* refactor(#3187): give state field extraction one canonical owner stateFieldValue in state-document.cts becomes the single owner of the #1760 frontmatter-then-body fallback chain. The new whole-repo guard found 14 independent re-derivations where the epic scoped 5, all now routed through it: cmdStateSnapshot (11), cmdStatePrune (2) and smart-entry fmScalar (1). state validate was a gate that could not fail. Every warning it could emit sat behind a phase resolved without the frontmatter tier, so a STATE.md whose phase lives only in frontmatter skipped the drift scan entirely and returned valid:true. It also read unstripped content, letting a frontmatter status: key shadow the body field (#1255 class). Both fixed; output gains a scope field so could-not-look stops being output-identical to looked-and-clean. Verified on the remote runner. * docs(#3187): document the state validate scope field and its reason codes Adds docs/how-to/interpret-state-validate-results.md so a reader can tell nothing-to-report from could-not-look, updates the COMMANDS.md and USER-GUIDE.md entries, corrects the CONTEXT.md glossary overstatement about Current Position sole ownership, and drops the changeset fragment. * fix(#3187): close three drift-guard evasion shapes and test the refuse path The isolated adversarial review found the ladder detector was evadable by ordinary reformatting, not just deliberately: a member or computed operand (fm.key / fm[key]) missed the bare-identifier backreference, a swapped tier order missed a hardcoded number-then-boolean sequence, and a ladder wrapped across lines missed single-line detection. All three now caught, each with its own test plus a proven boundary control. The frontmatter-parse refuse path on the destructive complete-phase route was unreachable and therefore untested. It is now driven by an injected parse failure and asserts STATE.md is byte-identical after the refusal, rather than shipping untested defensive code on a path that rewrites user state. Verified on the remote runner. * fix(#3187): widen the drift guard to the prompt layer and disclose tier-2 changes The code-review spec axis found the guard's scan surface was src/ only, which is Decision 4(d)'s forbidden allowlist one directory wide - and it had a live miss: gsd-core/workflows/smart-entry.md tells an agent to read status from frontmatter or the body, a prose expression of this same chain. The surface now covers the prompt layer. That one site carries a permanent written exemption rather than a ratchet: it is the gsd-tools-is-down fallback, so it cannot call the owner by construction, and a ratchet would imply removable debt that does not exist. Two tier-2 output changes shipped undisclosed and are now named in the changeset and docs: complete-phase's idempotency guard consulting frontmatter, and the workstream inventory resolving frontmatter-only fields. docs/COMMANDS.md gains a state complete-phase entry, which it never had. Also records Amendment 5 on ADR-3180, extracts the duplicated frontmatter-parse block the epic's own thesis forbids, and re-points two assertions from free-form warning prose onto the structured drift object. Verified on the remote runner. * chore(#3187): backfill changeset PR number pr:0 placeholder replaced with the real PR number now that #3283 exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9faacc0c15 |
test(#3148): bound the long tail and delete the unbounded-spawn allowlist (#3192)
* test(#3148): bound the long tail and delete the allowlist Migrates the final 170 unbounded sync spawn sites across 49 files, then removes the allowlist entirely. local/no-unbounded-spawn now runs with no exemption surface across tests/**: there is no file to add a name to. drift-detection's throw-native git() helper routes to gitOrThrow -- bare runGit would have taken 16 call sites quiet on failure. commands.test.cjs has two independently-scoped runGsdTools/runCli helpers, one already bounded and one not; they are kept distinct rather than unified, the same trap as the two same-named git() helpers in Wave 1. runNpm's bound was erasable. Its options spread callerOptions after the defaults, so an explicit timeout:undefined silently dropped the 180000ms bound -- the rule flagged it and was right; it was not a false positive. Fixed by destructuring with a default, with a test that fails when the default is removed. Two sites stay on a raw spawn with an explicit timeout because the seam cannot express them: one needs shell:true for npm.cmd on Windows, one redirects stdout to a real fd. Both are the rule's own documented second option, not an escape from it. Closure verified rather than asserted: the derivation scan reports 0 unbounded spawn helpers and 0 unbounded direct git call sites, and a temporary file carrying an unbounded spawn still errors with the allowlist gone. Closes #3064. * test(#3148): close a hole in the guard's own eslint-disable ban The ban listed only the top level of tests/, so it was blind to 37 .cjs files under tests/helpers, qa, observability, fixtures and dispatch. With the allowlist deleted this test is the sole remaining way to detect someone silencing the rule inline, so the gap was load-bearing: a nested file could carry an unbounded spawn plus an eslint-disable and pass everything. Proven before and after. A probe planted under tests/helpers with both was invisible to the guard and clean under eslint; after making the listing recursive the guard fails on it. The scanned set goes from 771 files to 808. Pre-existing since the guard shipped, but this wave is what promoted it to sole defense, so it is fixed here rather than filed. Also converts the last hand-rolled throw check to throwIfFailed and the last re-derived legacy shape to compose toLegacyResult, which makes the epic's none-remain claim true rather than nearly true. toLegacyResult itself is not widened -- eight callers depend on its shape and one consumer does not justify changing a shared contract. * fix(#3148): correct seam incoherence at the bound and a slow review-lane error path Two real failures from the remote runner, both fixed at the cause. The seam could return outcome TIMED_OUT together with exitCode 0. At the exact bound spawnSync reports ETIMEDOUT while the child has already exited with a real status, and toSeamResult classified on the error code while passing status straight through -- an incoherent pair its own boundary test was written to catch, and did. A status that is not null is direct evidence the child exited on its own, so it now decides the outcome before the error-code branches run. process-seam.cjs was deliberately untouched by every earlier wave; this is a defect in the module itself, kept surgical, with a unit test that fails against the old logic. review-lane with an unknown subcommand fell through to its usage error only after loading the capability registry and building a per-lane plan, which spawns one child process per lane -- up to twelve. The error path took ~1288ms instead of ~119ms, and under bench load it outran a caller's spawn timeout and was killed before writing anything, which is the empty stdout and stderr CI saw. It now fails fast before any of that work begins. This is the epic's first production change. It is user-facing, so it carries a changeset rather than a no-changelog label. * test(#3148): replace a real-race timeout test with a deterministic one E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a warm container git finishes first, spawnSync returns status 0 with no error at all, the seam correctly classifies EXITED, and gitOrThrow correctly does not throw -- so the test failed on both lanes. A probe confirms a genuine timeout always carries status null, so this was never the seam misbehaving. Raising the bound would only lengthen the odds, which is the same defect with better luck. The test now drives gitOrThrow against a stubbed runGit that returns a synthetic TIMED_OUT result, so it asserts exactly what it always meant to -- that a timeout propagates as a throw -- with no timing dependence. Five consecutive runs are identical where the old one varied. I wrote this test in Wave 0; it is a real-race test by construction and CLAUDE.md says to replace those rather than re-run them. * chore(#3148): backfill changeset PR number 3192 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
80ec0791eb |
fix(#3052): preserve frontmatter last_activity_desc on same-date body prose conflict (#3140)
* fix(#3052): preserve frontmatter last_activity_desc on same-date body prose conflict preferNewerLastActivity only preserved last_activity_desc when the derived date was OLDER than the existing frontmatter date. When the dates matched (same-date), the derived body prose (potentially stale) overwrote the authoritative frontmatter desc. Fix: when derDate === exDate, also preserve the frontmatter desc. * chore(#3052): backfill changeset PR number 3140 --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
a4560c3669 |
test(#3090): read the body Status, not the frontmatter key that shadows it
stateExtractField tries bold **Field:**, then a plain ^Field: line, first match wins. STATE.md's frontmatter carries a `status:` key that appears before the body's plain `Status:` line, so extracting "Status" from unstripped content returns the frontmatter value and never the body prose the test was written against. Scoping the lookup to stripFrontmatter() fixes it. The tempting fix was to assert the frontmatter enum instead, on the reasoning that a typed value beats matching prose. That would have been wrong and would have weakened the test: normalizeStateStatus maps both "Phase complete — ready for verification" and "Verifying Phase N" onto the same 'verifying' enum, so the enum cannot tell phase-complete from mid-verification, which is precisely the distinction this case exists to prove. Typed is not automatically stronger when the type conflates the cases under test. Case 4 carried the same collision and was passing only because normalizeStateStatus falls through to the raw text when no known pattern matches, so frontmatter and body happened to agree. Fixed alongside it rather than left for the next person to trip over. All fourteen conversions on this branch were audited against the same failure mode. The collision can only arise where the frontmatter key and the body field name are identical case-insensitively — Status is the only such field, since every other frontmatter key is snake_case against a Title Case body label. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
7dd9e59f6b |
test(#3090): stop exempting violations under categories that do not fit
An allow-test-rule annotation citing a category that does not apply is worse than no annotation, because it reads as reviewed. Eight were confirmed by reading the assertions each one covered, and auditing the rest found five more plus one refutation — a converter test whose wording described the wrong mechanism while the covered assertion genuinely was deployed-text. The instructive one used the CANONICAL string for the same mistake: STATE.md command output labelled as a deployed artifact. A canonical string is not evidence the category fits, which is why normalising strings alone would have laundered the problem rather than fixed it. Every mapping the audit had inferred rather than code-verified was spot-checked before rewriting, and the ones that turned out not to fit were re-annotated rather than relabelled. Fourteen STATE.md assertions had a typed extractor available all along and now use it; their annotations came out because nothing needs exempting. Eight assertions genuinely need a production change first — CLI stdout and stderr with no structured mode — and are tagged pending-migration-to-typed-ir citing #3090, which is what that category is for. It had zero real uses before this, while one file carried a real citation to migration issue #2974 under a non-canonical tag. Six annotations covered assertions that do no text matching at all. An exemption for a violation that does not exist is noise that makes the real ones harder to audit; those are removed. atomic-write-coverage gains the annotation it always warranted — its own docstring describes a structural-regression-guard while the file carried none. Fifty-nine non-canonical strings across roughly thirty files are normalised, and the allow-test-rule allowlist is regenerated to match. 472 annotations became 463: every one now uses a canonical category, and the two remaining non-canonical strings are ESLint RuleTester fixtures, not annotations. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
73418c516f |
fix(#2956): scope Phase extraction to ## Current Position (3rd gen of #2444/#2567) (#2961)
* test(#2956): fail-first regressions for Phase scoped to ## Current Position Third generation of #2444 / #2567. Stopped At / Paused At were scoped to ## Session; Phase (canonically in ## Current Position per templates/state.md) was left unscoped, so a historical Phase: / **Phase:** line in an archive section overwrites current_phase on every write. Since current_phase is routing input for gsd-progress / --next, the rewind routes work to the wrong phase. Six failing-first regressions + one round-trip: - shape B: bold **Phase:** 19 archive BELOW the section - shape C: plain archive Phase: 19 ABOVE the section - bootstrap h3 ### Current Position variant - CRLF variant - Phase token in decisions prose (over-broad-fix guard) - Paused At read-path parity with the write seam (## Session) - write-then-read round trip stays at 22 (read/write agreement) Folded into tests/state.test.cjs (lint:regression-test-names bans a new tests/bug-NNNN-*.test.cjs file). * fix(#2956): scope Phase extraction to ## Current Position at both seams Third generation of #2444 / #2567. Stopped At / Paused At were scoped to ## Session by those fixes; Phase (canonically in ## Current Position per templates/state.md) was left unscoped, so a historical Phase: / **Phase:** line in an archive section silently overwrote current_phase on every write. Because current_phase is routing input for gsd-progress / --next, the rewind routes work to the wrong phase. Fix mirrors the proven #2444 seam exactly: - new matchCurrentPositionSection helper (collectSection-based, CRLF-tolerant, level-flexible for the bootstrap ### Current Position h3 variant), sited next to matchSessionSection. - read path (cmdStateSnapshot): extract Phase from matchCurrentPositionSection ?? body. Also scope Paused At to matchSessionSection ?? body so the read seam agrees with the write seam (which already scoped Paused At to ## Session). - write path (buildStateFrontmatter): extract Phase from matchCurrentPositionSection ?? bodyContent. stateExtractField itself is untouched (its bold/plain precedence is load-bearing for other fields — the #3265 test depends on it), and preferNewerLastActivity is untouched (Last Activity has no canonical section; its date-direction guard is deliberate). Fall back to full-body when no ## Current Position section exists so files without the heading keep current behaviour. * chore(#2956): add changeset fragment (pr:0 placeholder, backfill after PR) * test(#2956): make round-trip test actually trigger the write-path resync The write-then-read round-trip test used 'state update Status "Executing"' on a fixture with no Status field, so the update was a no-op (updated:false) and no frontmatter resync ran through buildStateFrontmatter — the assertion on the written frontmatter then failed not because the fix is wrong, but because no write happened. Add a **Status:** field so the update performs a real field update (updated:true) and forces the resync. Verified locally: pre-fix this writes current_phase:19 (the archive value); post-fix it writes 22. The code fix is correct (5 of 7 RED tests passed; the 2 failures were this defective test). This is the 'fix the bad test' half of the TDD-loop rule. * chore(#2956): backfill changeset PR number 2961 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ec681e3c21 |
fix(#2567): scope Paused At to ## Session + guard Last Activity date regression (#2660)
* test(#2567): add failing regression for stale state field overwrites buildStateFrontmatter extracts Last Activity and Paused At from the full STATE.md body via stateExtractField, which matches the first 'Field:' line anywhere. Historical archive sections containing stale field-shaped lines silently overwrite the correct frontmatter value on every sync, and because the poisoning line stays in the body it regresses on the next write. Same divergence class as Bug #2444 (which scoped Stopped At to ## Session but did not propagate). Failing-first: all three tests reproduce the bug on unmodified next (verified via the dedicated red run on the test-only commit). * fix(#2567): scope Paused At to ## Session + guard Last Activity date Two complementary fixes for the stale-archive-overwrites-frontmatter bug class, chosen per field semantics: - Paused At is a session field: scope extraction to ## Session (via the existing matchSessionSection helper), exactly mirroring the #2444 fix for Stopped At. A stale 'Paused At:' line in an archive section can no longer win over the current value. Falls back to full body when no ## Session. - Last Activity has no single canonical section (it appears in the preamble, ## Configuration, and ## Current Position across STATE.md layouts), so a section scope cannot reliably exclude archive copies. Instead guard the information-losing direction: when the body-derived date is OLDER than the existing frontmatter date, keep the existing value and description (preferNewerLastActivity). Applied at both the write seam (syncStateFrontmatter) and the read seam (cmdStateJson) so they agree. Non-date values pass through unchanged. A first attempt scoped ALL current-state fields to the body preamble, but that broke STATE.md variants where the fields legitimately live inside ## Configuration / ## Current Position (regressed 4 frontmatter.test.cjs suites). This minimal fix targets only the two fields the issue names. * docs(#2567): add changeset fragment for stale state field overwrite fix * docs(#2567): backfill PR number in changeset fragment |
||
|
Tom Boucher
|
04a0eb8d63 |
fix(#2450): CRLF-tolerant session-section rewrite + no-op-detection guard (#2482)
* fix(#2444): re-resolve body-parser to 2.3.0 in lockfile (GHSA-v422-hmwv-36x6) GHSA-v422-hmwv-36x6 (body-parser DoS via invalid limit value, low severity, published 2026-07-20T23:23:26Z) made tests/npm-integrity-gate.test.cjs (#3588: root workspace production tree has no advisories) fail any subsequent npm audit --omit=dev. The advisory affects body-parser >=2.0.0 <2.3.0 pulled transitively via @anthropic-ai/claude-agent-sdk -> @modelcontextprotocol/sdk -> express -> body-parser@2.2.2. express@5.2.1 already declares body-parser as ^2.2.1, so 2.3.0 is a valid re-resolution within express's own compatibility range — no override needed. Regenerated the lockfile via 'npm audit fix --omit=dev' which re-resolves transitive deps within their declared ranges; package.json is unchanged. Verified: npm audit --omit=dev reports 0/0/0/0/0 advisories; body-parser now reads as 2.3.0 in 'npm ls body-parser --omit=dev'. * test(#2450): failing-first CRLF regression for record-session insert path cmdStateRecordSession's section-rewrite regexes (src/state.cts:1166, :1195) used literal \\n which cannot match CRLF STATE.md delimiters. The detector regex (CRLF-tolerant via $ under /m) entered the rewrite branch, the writer regex silently no-op'd, but updated.push(...)/ sessionCreated=true ran unconditionally. Result: caller reported recorded:true with 'Resume File' in updated, but the field was never written to disk. With core.autocrlf=input, the CRLF working-tree file produces no git diff, so the bug was invisible. Adds three regression tests covering all three rewrite paths: - CRLF STATE.md with ## Session and Resume file absent - CRLF STATE.md with ## Session and Stopped at absent - CRLF STATE.md with ## Session Continuity (bootstrap shape) Each asserts the field IS on disk (the bug discriminator: the pre-fix command's JSON output looked identical to a successful write). * fix(#2450): CRLF-tolerant session-section rewrite + no-op-detection guard Two regexes in cmdStateRecordSession used literal \\n which cannot match CRLF STATE.md, silently no-op'ing the section rewrite while the CRLF- tolerant detector above entered the branch. The reporter's exact repro: on a CRLF STATE.md with one canonical session field absent, the command returned recorded:true + updated:['Resume File'] but the field was never written to disk. Three changes: 1. src/state.cts:1166 (canonical ## Session rewrite regex): \\n -> \\r?\\n 2. src/state.cts:1195 (## Session Continuity insert regex): \\n -> \\r?\\n 3. Defensive invariant (#2450 class fix per reporter's suggestion): track whether the chosen branch's replace actually matched via callback flag. Only set sessionCreated=true and push to updated when rewriteMatched. Unreachable post-fix, but fail-loud is the right posture for a silent- success gate. If a future drift between the detector and writer regexes reintroduces the asymmetry, the caller will not see false updated entries. Same canonical CRLF-tolerant form already in use at check-command-router.cts :205 (extractPlanDesignatedSections). Same bug class previously fixed in #1658, #1668, #2206, #2449. * fix(#2450): address review followups + add changeset Code-review + security-review both flagged the unreachable else at the Session Continuity branch (defaulted rewriteMatched=true in dead code, re-arming the bug class for future drift). Removed the else; the remaining code path leaves rewriteMatched=false if linesToInsert is empty, preserving the fail-loud posture. Added scope-limitation doc to the rewriteMatched gate: it covers the INSERT path only, not the earlier in-place stateReplaceField successes (which DID land on disk and correctly push to updated unconditionally). Tests: - Normalized STATE_CRLF_SESSION_MISSING_RESUME fixture to match the canonical 6-key frontmatter of STATE_WITH_SESSION (code-review I2). - Added mixed-ending test (LF frontmatter + CRLF body) to close CONTRIBUTING.md:490 'Mixed CRLF/LF newlines' requirement (I1). Added Fixed changeset (code-review H1). * docs(changeset): backfill PR number to 2482 |
||
|
Tom Boucher
|
a54feb4216 |
fix(#2440): per-counter progress ratchet — total_plans always takes derived value (#2468)
* fix(#2440): per-counter progress ratchet — total_plans always takes derived value Two sites fixed (targeted — existing body-only write tests preserved): Site A — read path: shouldPreserveExistingProgress (state-document.cts:167) removed total_plans from the all-or-nothing ratchet check. It now joins total_phases as an always-derived counter. Only completed_phases and completed_plans keep ratchet behaviour (they are monotonic). This fixes gsd-tools query state.json reporting stale total_plans when a curated completed_plans triggers the ratchet. Site B — write path: applyStatePreservation (state-transition.cts:162) gained a deriveProgressKeys opt-in flag. When true (passed by cmdStatePlannedPhase only), total_plans and total_phases take the derived (post-sync) value instead of the wholesale curated restore. When false (the default — state.update, state.patch), the existing #3242 wholesale protection stays fully in force. This fixes the state planned-phase verb writing a stale total_plans. The opt-in approach preserves all 8 existing #3242/#1264/#500 body-only write tests that assert wholesale progress preservation during non- progress updates. Tests: - tests/state.test.cjs: 4 unit tests for shouldPreserveExistingProgress (total_plans upward/downward/equality + completed_plans ratchet active). - tests/state-transition.test.cjs: 2 #2440 regression tests for deriveProgressKeys=true (total_plans takes derived; boundary at equality). The existing !resync wholesale-restore test stays unchanged (default behavior preserved). References: #2440; #1446 (total_phases read-path fix — same principle); #3242 Bug A (body-only preservation — protection preserved via the opt-in gate); ADR-1769 (applyStatePreservation table-driven preservation). * chore(#2440): backfill pr:2468 in .changeset/mellow-eagles-chatter.md |
||
|
Tom Boucher
|
1a46bc068a |
fix(#2376): emit absolute subagent-facing paths from init/state, convert workflow literals (#2428)
* fix(#2376): emit absolute subagent-facing init/state paths Make init.* and state.* path fields absolute rather than cwd-relative so subagent prompts resolve correctly regardless of working directory. Adds intel_dir/conflicts_path/requirements_path/roadmap_path/state_path to cmdInitIngestDocs, an absolute debug_dir to cmdStateLoad, and replaces bare .planning/... literals in 12 workflow Agent() prompt blocks with the absolute init-JSON path fields. Includes decoy-cwd regression tests and realpath'd tmpdir fixtures for macOS. Squashed rebase of the #2376 commit series onto a fresh origin/next (previous merge ee25543a1 was against a now-stale next). * chore(#2376): add changeset * chore(#2376): regenerate golden fixtures + workflow size baseline Regenerated after rebasing the absolute-path fix onto current next (picks up #2351's run-with-timeout content in execute-phase.md too). * fix(#2376): trim execute-phase.md redundancy to stay under the size margin * chore(#2376): regenerate golden/size baseline after rebase onto next |
||
|
Tom Boucher
|
8b70db343b |
fix(#2204): phase-completion writes 'All phases complete' per ADR-2207 (#2259)
* fix(#2204): phase-completion writes 'All phases complete' per ADR-2207 completePhaseCore was writing the overloaded bare 'Milestone complete' on the last phase — the same string space the milestone-close verb owns for terminal state. Per ADR-2207, phase-completion now writes the existing intermediate value 'All phases complete' (already used in gsd2-import.cts). Milestone termination ('<version> milestone complete' / 'Awaiting next milestone') remains solely with milestoneCompleteCore. Status lifecycle: Ready to plan → All phases complete → <version> milestone complete → Awaiting next milestone. Changes: - src/state-transition.cts: completePhaseCore status value - src/phase.cts: #2028 guard comment - tests/state-transition.test.cjs: assertion + test name - tests/phase.test.cjs: 8 assertion updates (positive + negative) - tests/state.test.cjs: normalizeStateStatus test case + reset regex - tests/workstream.test.cjs: fixture status to terminal value - gsd-core/workflows/progress.md: Route D label - gsd-core/workflows/transition.md: Route B label - CONTEXT.md: Status lifecycle glossary entry (ADR-2207) - .changeset/brave-geese-jump.md * test(#2204): regenerate golden-install-parity fixtures + workflow-size baseline Workflow file edits (progress.md, transition.md) changed install payload hashes and pushed past the committed workflow-size baseline. Regenerated all 17 golden-install-parity fixtures + claude-local via the standalone gen script (which now also covers the local-scope claude layout). Updated workflow-size-baseline.json and agent-size-baseline.json via size:baseline. * fix(#2204): correct claude-local golden hashes + document gen-script limitation The gen-script's claude-local generation produces macOS-specific hashes incompatible with Linux CI (local-scope install embeds platform-varying node-runner paths). Reverted to manual update using Linux FAILURES.md +actual hashes for the 2 changed workflow files. Added explanatory comment in the gen script. * test(#2204): add isCompletedInventory coverage + clarify CONTEXT.md glossary Addresses orthogonal code-review findings (Medium #1 + #2): - Add isCompletedInventory test cases for ADR-2207 status lifecycle (terminal 'milestone complete' → true; intermediate 'All phases complete' → false; archived → true; active statuses → false) - Clarify CONTEXT.md glossary: note that isCompletedInventory intentionally excludes the intermediate value * docs: backfill changeset PR number (#2259) * docs(#2204): add Status lifecycle table to state-md reference (ADR-2207) |
||
|
Tom Boucher
|
c1885df9e5 |
chore(#2143): prohibition-with-teeth + migrate remaining ad-hoc table sites — Phase 4 (final) (#2253)
* chore(#2143): prohibition-with-teeth + migrate remaining table sites — Phase 4 Phase 4 of epic #2143 (ADR-2143 §7). Completes the markdown table/mutation consolidation by (a) giving the ad-hoc-parsing prohibition teeth and (b) migrating the last ad-hoc table sites onto the shared seam. - src/markdown-table.cts: new formatting-preserving `updateTableCell` primitive (self-contained, ragged-row-tolerant header/delimiter/cell-range scan; splices only the target cell's raw span, preserving all other bytes incl. padding/CRLF; no-op-preserves-padding when a transformer returns the current value). Exports splitTableRow/isDelimiterRow/findTableStartOffset for tolerant reuse. - eslint-rules/no-adhoc-markdown-parsing.cjs: TABLE-REGEX detector extended to `new RegExp(<literal|static-template>)`; new `.replace()`-mutation detector for roadmap/state/content receivers with a table/section-shaped pattern. - scripts/lint-table-schema-drift.cjs (wired into lint:ci): fails if a TABLE_SCHEMA header drifts from its authored table; tests import its logic (single source). - Migrated onto the seam (behaviour-preserving vs pre-Phase-4 HEAD, verified byte-diff old-vs-new): roadmap.cts cmdRoadmapUpdatePlanProgress, phase.cts cmdPhaseComplete + traceability, milestone.cts cmdRequirementsMarkComplete, uat.cts read path, state.cts metrics/decisions/By-Phase. - Incidental correctness gains from the migration: a decoy table can no longer swallow a phase-progress update (## Progress scoping); a ragged neighbouring row no longer silently aborts an edit; completing integer phase N no longer touches a decimal sub-phase N.x row; record-metric no longer drops trailing section content or duplicates the ## Performance Metrics section. - Kept justified allow-adhoc-markdown markers only where genuinely not a table (security.cts <|role|> token) or a loose non-GFM section (uat human-verify). Two orthogonal isolated reviews (correctness/adversarial + security) passed; correctness found 4 behaviour regressions in the first migration pass, all fixed and re-verified byte-identical-or-better vs OLD. Surfaced for maintainer (pre-existing, ambiguous domain logic, NOT changed here): templates/state.md places a By-Phase table under ## Performance Metrics while cmdStateRecordMetric assumes a Plan|Duration|Tasks|Files table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): match traceability row by first-cell value, not Requirement header Phase 4's migration matched the REQUIREMENTS.md traceability row by a column literally named `Requirement` (`row['Requirement']`), but real tables head that column `REQ-ID`. The by-name lookup found nothing, so `phase complete` and `requirements mark-complete` left the Status cell `Pending` (regressed #2769 / #2203, caught by gsd-test — 8 failures, both node 22/24). - src/phase.cts, src/milestone.cts: match the row by its FIRST cell's value (the requirement-ID column) regardless of that column's HEADER name, via `Object.values(row)[0]` (updateTableCell builds the record in header order). This mirrors OLD's first-cell `\|\s*<id>\s*\|` anchor, restoring header-name independence while keeping the seam. - src/milestone.cts hasTable: broadened from `Requirement`-only to also recognize `Requirement ID` / `REQ-ID` / `REQ ID` headers, kept in sync with the now-positional rowMatch/hasRow so a REQ-ID-headed table participates in the ADR-2143 §6 write-set and the #2140 table_unmatched drift check (it was silently omitted before — a checkbox-only partial reconcile against a REQ-ID table could report as fully reconciled). The `Requirement`-headed path is byte-identical to OLD. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2143): replace stale structural milestone guards with behavioural suite The `milestone.cjs regex global state fix` block was a source-structure guard (allow-test-rule: structural-regression-guard) — it readFileSync'd the compiled milestone.cjs and asserted removed regex idioms (`tablePattern.test`, `afterTable !== reqContent`, `doneTable = new RegExp(...)`). Phase 4's migration deleted those regexes (table update is now updateTableCell), making the assertions obsolete. Per the Test Cleanup rule, replace them in-PR with a behavioural suite driving the compiled CLI: - multi-ID mark-complete flips all IDs (guards the lastIndex/global-state class), - Pending->Complete flip under both `REQ-ID` and `Requirement` headers (#2769), - idempotent already_complete detection with no corruption, - REQ-ID-headed table participates in write_set (traceability entry, applied), - REQ-ID-headed table trips #2140 table_unmatched drift on a missing row. Pruned the now-nonexistent structural-regression-guard entry from the lint-allow-test-rule-refs allowlist (the source-text-is-the-product entry for the same file remains valid). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(changeset): backfill PR number 2253 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): record-metric targets its own metrics table, not By-Phase velocity `state record-metric` appended its per-plan row (`| Phase 1 P1 | 5min | 3 tasks | 4 files |`) into the FIRST table under `## Performance Metrics` — which on a real template-derived STATE.md is the By-Phase velocity table `| Phase | Plans | Total | Avg/Plan |`, polluting it on EVERY plan completion (execute-plan.md:414 is a per-plan call). The command's own metrics table is `| Plan | Duration | Tasks | Files |`, which the template does not ship, so the row never reached it; the scaffold branch also emitted a wrong `| Phase | Plan | Duration | Notes |` header matching neither the row nor the canonical table. Pre-existing (predates Phase 4); surfaced while migrating this site and fixed here per no-defer, on the user's explicit go-ahead. - src/state.cts cmdStateRecordMetric: locate the metrics table by its own header shape (`Plan|Duration|Tasks|Files`, via splitTableRow/isDelimiterRow) rather than "first table in the section". When the section exists but has no metrics table (only the By-Phase table), self-heal by appending a fresh **Per-Plan Metrics:** table to the END of the section body — By-Phase table, Recent Trend and footer preserved verbatim, no duplicate `## Performance Metrics` heading, created stays false. Absent-section scaffold header corrected to the canonical `| Plan | Duration | Tasks | Files |`. Ragged-tolerance + None-yet preserved. - Not touching templates/state.md (golden-install-parity hashed) — record-metric self-creates the table on first use instead. Failing-first regression test (tests/state.test.cjs) demonstrates the By-Phase pollution on the pre-fix build, then green after. Verified: no pollution, self- heal idempotency, both-tables isolation, content/heading preservation, flags, None-yet, corrected scaffold header (23-check adversarial harness + all existing record-metric scenarios). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#2143): deleteSection seam primitive (level-bounded whole-section removal) ADR-2143 §4 shipped withSection/collectSection (replace a section BODY) but no way to DELETE a section (heading + body). Phase 4 suppressed the phase-remove section delete instead of building it. deleteSection(content, predicate, opts) locates the section via the collectSection machinery and splices out from the heading's start offset to the next same-or-higher-level heading — so a level-3 `### Phase N` delete stops at a following level-2 `## Progress`, never past it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): phase remove no longer deletes ## Progress on last-phase removal updateRoadmapAfterPhaseRemoval deleted a `### Phase N` detail section with a greedy raw regex whose lazy scan, on the LAST phase, ran to EOF and destroyed the following `## Progress` heading and its entire tracking table — silent data loss, uncovered by tests (removal tests only exercised a middle phase). Migrated onto the new deleteSection seam (level-bounded, stops at `## Progress`); dropped the allow-adhoc-markdown SECTION-DELETION suppression. Failing-first regression (tests/phase.test.cjs) removes the LAST phase and asserts the ## Progress heading + table survive; middle-phase removal is byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#2143): deleteTableRow seam primitive (row removal, ragged-tolerant) Sibling of updateTableCell: locates the first GFM table, matches a DATA row by predicate (ragged-tolerant record build, header order), and splices out that row's whole line preserving every other byte. Returns {ok:false,reason} on no table / no match. Enables migrating the phase-remove Progress-table row delete off its ad-hoc regex (ADR-2143 §7 — the "future row-delete seam" Phase 4 punted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): phase remove deletes the Progress row via deleteTableRow The Progress-table row delete used a whole-document regex with two defects: (a) `\.?\s` required whitespace after the phase number, so a COMPACT row `|2|Beta|` was never deleted (stale row left behind); (b) unscoped — it could strike a row in a different table (e.g. an earlier `| Phase | Requirements |` table). Migrated onto deleteTableRow, scoped to the `## Progress` section (mirrors deriveProgressFromRoadmap), matching the row by first-cell phase number (integer zero-pad-insensitive; decimal exact; removing `2` never touches `2.5`). Both allow-adhoc-markdown suppressions removed. New behavioural tests: compact unpadded row deleted; padded byte-parity on the surviving rows (their ordinal correctly renumbers via the pre-existing renumber block). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): deleteTableRow leaves no dangling newline on last EOL-less row Deleting the final row of a table with no trailing EOL sliced from the row's start to end-of-string, stranding the newline that terminated the previous line. Back rowStart over the preceding \r?\n in that branch so the table ends cleanly. (Caught by the primitive's own unit test on gsd-test; local scenario checks missed the no-trailing-EOL edge.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): migrate read-only section-collects onto collectSection Six hand-rolled `## Section` read-extract regexes replaced by the collectSection seam (behaviour-preserving; extracted bodies feed the same downstream parsers): state.cts matchSessionSection (## Session / ## Session Continuity) + ## Blockers, smart-entry.cts ## Blockers, audit.cts ## Current Focus + ## Open Questions. Removes 6 allow-adhoc-markdown "pending #1372" suppressions. Incidental fix: the old Session regex `## Session[ \t]*\n` silently failed on a CRLF `## Session\r\n` heading (Windows STATE.md), nulling all session fields; collectSection is CRLF-safe, so session state now resolves on Windows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): fence-safe state-transition section writes + dedup stripFrontmatter - milestoneCompleteCore's `## Current Position` and `## Operator Next Steps` section resets used fence-blind raw regexes that a fenced `##` inside the body could truncate/mis-target (#2130/#2067/#2080 class). Migrated onto a fence-aware tokenizeHeadings-based helper (resetSectionVerbatim) that is byte-identical to the old output on the canonical path (9/9 fixtures) and correctly ignores a fenced fake heading (proven robustness gain). - mutateCurrentPositionFirstTime: hand-rolled locate+splice → collectSection + replaceSection (byte-parity). - stripFrontmatter was inlined byte-identically in state.cts AND state-transition.cts; hoisted the single canonical copy into frontmatter.cts (both call sites now import it) + unit tests — eliminates the divergence risk per CLAUDE.md "Generative Fix Divergence". Removes 3 allow-adhoc-markdown / #1372 markers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): name-address By-Phase sum + uat parse, eslint recall hole, catches - state.cts By-Phase "Total plans completed" sum: positional 2nd-cell regex → name-addressed splitTableRow read (correct on a reordered header, where the old code silently summed the wrong column). Marker removed. - uat.cts parseVerificationItems: loose pipe regex → splitTableRow within the existing table/numbered/bullet union scan (item list byte-identical; does NOT reintroduce the reverted strict-parseMarkdownTable item-drop). Marker removed. - eslint no-adhoc-markdown-parsing: close the `new RegExp(identifier)` recall hole — resolve a const-declared table-shaped regex identifier (mirrors the .replace() detector) + RuleTester cases; param/call args stay out (boundary). - commands.cts: delete a lying comment that claimed the scaffold date "stays on raw UTC / deferred" — #2136 already moved it to realClock.localToday(). - Empty catches (classified, not blind-swept): removed 4 dead try/catch; fixed 3 error-hiding (phase-insert decimal-dir I/O collision now fails loud; phase-remove rename partial-failure surfaced; milestone-archive true count via finally); left best-effort swallows with justification comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): extractFencedBlock seam + migrate api-coverage named fence parseCoverageMatrix extracted its ```coverage fenced block with an ad-hoc regex (the last real allow-adhoc-markdown suppression). Added extractFencedBlock to the markdown-sectionizer seam (reuses stripFencedCode's CommonMark fence engine — info-string match, ~~~/backtick, nesting, indent) and migrated onto it; byte- parity on the parsed CoverageMatrix across 8 fixtures. Only security.cts:367 (a genuine `<|role|>` protocol-token false-positive, not a GFM table) remains marked in src/ — the "prohibition with teeth" goal (nothing grandfathered but a true FP) is met. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): By-Phase row insert is name-addressed (insertTableRow seam) updatePerformanceMetricsSection's INSERT-new-row branch located the By-Phase table with a canonical-column-order-only regex + a hardcoded positional row literal, so on a reordered header it silently inserted nothing — inconsistent with the now name-addressed UPDATE and SUM halves of the same function. Added insertTableRow (markdown-table seam sibling of updateTableCell/deleteTableRow: name-addressed, header-order-agnostic, EOL-preserving) and migrated the branch onto it, mapping By-Phase values by column NAME. Canonical-order output is byte-identical; a reordered header now inserts a correctly-mapped row; a pre-existing CRLF mixed-EOL splice glitch is incidentally fixed. Retired the now-dead byPhaseTablePattern const. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): phase-list checkbox flip via updateBullet seam Added updateBullet (markdown-sectionizer): a fence-aware, offset-tracked single-bullet write primitive (GFM 1–4-space marker tolerance) — the write counterpart to read-only iterateBullets. Migrated mutateMilestonePhase's phase-list checkbox flip (`- [ ] Phase N …` → `- [x] … (completed <date>)`) off its whole-slice regex onto it, same milestone-slice scope + clock seam. Byte-identical across simple / idempotent / metachar-title / double-space / CRLF scenarios. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): scope the Progress-ordinal renumber to ## Progress via seam phase remove's integer-renumber decremented Progress-table phase ordinals with a whole-document `content.replace(/(\|\s*)(\d+)(\.\s)/g, …)` — unscoped, so it also rewrote any `| N. …` cell in an unrelated/decoy table (same class as the batch-2 row-delete scoping bug). Migrated onto updateTableCell, scoped to the ## Progress section, decrementing each affected row's leading phase ordinal by column name. Byte-identical on canonical Progress tables + multi-row + decimal-sibling cases; a decoy `| 3. … |` row before ## Progress is now correctly left untouched. The sibling heading / checkbox-bullet / PLAN.md-filename / Depends-on-prose renumbers are not GFM-table mutations (outside ADR-2143's table/section mandate) — left as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): review fixes — scope traceability write, restore Current Position H3-stop Adversarial review of the remediation (BLOCK verdict) — all 9 findings fixed: - F1 (BLOCKER): requirements mark-complete / phase complete flipped the checkbox but NOT the traceability row on the shipped template, because updateTableCell bound to the FIRST table (## Out of Scope, no Status column) instead of the ## Traceability table — the #2140 silent-divergence class, re-introduced by the seam migration and missed by tests (fixtures had Traceability first). Scoped the write + hasRow probe to the ## Traceability section slice (updateTraceability Cell helper) in milestone.cts + phase.cts. Failing-first tests on the Out-of-Scope-before-Traceability layout; the #2769 first-cell match preserved. - F2 (MAJOR): mutateCurrentPositionFirstTime restored to locateCurrentPosition (STOP_H2_PLUS) — collectSection's default H2-stop swallowed a level-3 subsection and the field regexes clobbered it (#2130 class). - F3/F8: Progress-ordinal renumber re-escapes via escapeCell + keys padding recovery by row index (was de-escaping `\|` and losing padding on dup values). - F4: insertTableRow escapes cell values internally. - F5: updateBullet accepts a tab after the marker (`[ \t]{1,4}`). - F7: resetSectionVerbatim consumes CRLF blank lines (byte-parity on CRLF). - F6/F9: corrected two misleading comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(changeset): data-loss + CRLF-session user-facing fixes (#2253) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2143): de-flake the G10 windsurf ReDoS-guard wall-clock assertion The G10 test asserted `elapsedMs < 1000` for a 200k-char payload — a wall-clock assertion (CLAUDE.md: never assert on wall-clock time) that flaked on a loaded node24 bench at ~1.1s. It was redundant: runHook's spawnSync `timeout: 10000` already SIGKILLs a catastrophic-backtracking hook, so the exit-0 assertion is the real ReDoS guard. Removed the timing assertion; kept exit-0 + documented the subprocess-timeout mechanism. Surfaced (not caused) by this branch's gsd-test runs loading the bench; unrelated to the markdown-parsing changes but fixed in place per the no-flaky-tests rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
d49ac81306 |
chore(#2143): markdown table model + schema registry + fail-loud pilot — Phase 1 (#2248)
* chore(#2143): markdown table model + schema registry + fail-loud pilot — Phase 1 Phase 1 of epic #2143 (ADR-2143): consolidate markdown table parsing onto a canonical seam and migrate the pilot reader. - Add src/markdown-table.cts: parseMarkdownTable (GFM tables -> typed {columns, rows} addressed by column NAME; ragged rows are typed parse errors, not silent), a single-source TABLE_SCHEMAS registry (RoadmapProgress / RequirementsTraceability / QuickTasks / Security, with variants under one id), matchTableSchema, and findTableBySchema. Result<T> is scoped to this seam (distinct from the dispatch Result). - Migrate deriveProgressFromRoadmap (src/phase-lifecycle.cts) off the position-anchored regex to name-based resolution via the seam — fixes #2137 (the 5-column milestone-grouped Progress table previously returned all-null). - Add a schema-backed `gsd-tools quick-tasks-append` subcommand and route fast.md's log_to_state through it, retiring the inline `awk NF-2` column arithmetic — fixes #2133 (addresses #2012, #2119). Cell values are escaped (| and newlines) and the STATE.md read-modify-write is atomic under readModifyWriteStateMd (lost-update race, cf. #500/#905/#1230). - Writer/reader/template parity test guards TABLE_SCHEMAS against drift (ADR-2143 §3 Generative-Fix-Divergence). Registration: .gitignore, eslint.config.mjs, docs/INVENTORY.md + INVENTORY-MANIFEST.json, CONTEXT.md glossary, docs/CLI-TOOLS.md. Behaviour-preserving for the canonical 4-column Progress table; the named bugs are driven fail-first. Extend-never-mutate (ADR-2143 §2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#2242): backfill changeset PR number (#2248) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2242): escape backslash before pipe in markdown-table cell escaping CodeQL js/incomplete-sanitization (high): escapeCell escaped | -> \| but not the backslash itself. Now escapes \ -> \\ before | -> \|, and splitTableRow unescapes both \\ -> \ and \| -> | symmetrically so cell values (incl. literal backslashes) round-trip exactly. Added backslash round-trip tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2242): read ROADMAP Progress table by column name — supersede #2168 ad-hoc scan Rebase reconciliation with #2168 (the tactical #2137 fix that marked itself "pending #2143"). deriveProgressFromRoadmap now resolves the Progress table via a new seam helper findTableWithColumns (first table whose header is a superset of Phase/Plans Complete/Status/Completed, any order, extra columns ignored) and reads cells by NAME — order/injection-invariant per ADR-2143 §3 — instead of the exact TABLE_SCHEMAS match. This satisfies #2168's column-invariance property test while staying seam-based and preserving its `## Progress` scoping (#2012/#1445). Ragged Progress tables now resolve to null (ADR-2143 fail-loud); updated the stale state.test.cjs assertion that predated the Phase-1 migration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
f89740d53f | Merge branch 'next' into fix/2137-derive-progress-header-driven | ||
|
Tom Boucher
|
7ccf57200d |
fix(#2202): preserve unknown frontmatter keys in syncStateFrontmatter (#2233)
* fix(#2202): preserve unknown frontmatter keys in syncStateFrontmatter syncStateFrontmatter rebuilds frontmatter from a fixed schema, dropping any custom/unknown key on every mutating verb. Before reconstruction, merge any existing frontmatter key the schema does not own. Schema keys still win. Closes #2202 * docs(#2202): add changeset fragment * docs(#2202): backfill PR number * fix(#2202): add regression test + remove redundant type assertion - tests/state.test.cjs: behavioral regression test asserting custom/unknown STATE.md frontmatter keys survive a mutating verb (they were silently dropped before the syncStateFrontmatter carry-forward). - src/state.cts: drop the unnecessary `as Record<string, unknown>` assertion that tripped @typescript-eslint/no-unnecessary-type-assertion (the lint-tests gate failure). Refs #2202 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Behruz Nassre Esfahani
|
7a61294196 |
test(#2137): column-invariance property + boundary tests; mark parser pending #2143
Address @trek-e's review (Option 2, interim tactical fix) on PR #2168: - Add a fast-check property test (tests/derive-progress.property.test.cjs) asserting deriveProgressFromRoadmap's derived counts are invariant to header column ORDER and COUNT — shuffle the header cells and inject unrelated columns, assert counts match the data. This pins the parser's core new capability (header-name lookup vs. the old position-locked regex). Named derive-progress (not phase-lifecycle) so it isn't greedily attributed to the shorter `phase` prod prefix under lint-test-file-count; like the other *.property.test.cjs files it maps to no module and is cap-exempt. - Add boundary regression tests in state.test.cjs: header+separator only (0 rows -> all-null), exactly 1 row, and ragged rows (extra/short cells, handled without throwing). - Mark the adhoc Progress-table scan as pending the ADR-2143 parseMarkdownTable/TABLE_SCHEMAS seam (re-point pending #1372 -> #2143). - Fix the changeset lead-in to the required bold form. Per the maintainer, reader/writer parity and CONTEXT.md updates are explicitly out of scope for this stopgap — Phase 1 (ADR-2143) owns them. Validated: property test 200 runs green; state.test.cjs #2137 block 8/8; eslint clean; lint-test-file-count green; full `npm test` exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Behruz Nassre Esfahani
|
78f4c4a9c3 | Merge branch 'next' into fix/2137-derive-progress-header-driven | ||
|
Tom Boucher
|
605984cd79 |
test(#2177): use lint-clean .includes() for frontmatter-survival assertions
CI lint (local/no-crlf-fragile-split + no-regex-spaces) flagged the regex assertions: a bare \n on readFileSync content is CRLF-fragile on Windows git-autocrlf, and literal double-spaces are hard to count. Switch to .includes() strings — same validation intent, lint-clean on all platforms. |
||
|
Tom Boucher
|
dbec81d5ad |
test(#2177): cover frontmatter progress: shadow + suffix preservation
Adds the frontmatter-bearing STATE.md fixture the suite lacked. Asserts the body Progress: line (not the YAML progress: key) is the update target, the descriptive suffix after [bar] NN% is preserved, the frontmatter block is not mangled, and a body with no Progress: line reports updated:false even when the frontmatter has a progress: key (no false success). |
||
|
Behruz Nassre Esfahani
|
3f6be47db9 |
fix(#2137): parse the ROADMAP Progress table by header, not fixed column count
deriveProgressFromRoadmap read the `## Progress` table with two
4-column-only regexes, so every project past its v1.0 milestone — which
uses the 5-column milestone-grouped shape the same template ships
(| Phase | Milestone | Plans Complete | Status | Completed |) — parsed to
{completedPhases: null, totalPhases: null, totalPlans: null}. Because the
body is wrapped in a bare try/catch the failure was silent, and
phase.complete flipped the ROADMAP row to Complete but never advanced
STATE's progress percent / completed-phase count.
Parse the table by header instead: locate the `## Progress` section
(line-anchored h2, mirroring the writer's #2012 scoping so an earlier
Phase/Status/Completed-shaped table can't hijack it), read the header row
to find the Status, Completed, and plans columns by name, then index each
data row positionally. One code path now handles both the 4-column
greenfield and 5-column milestone-grouped shapes — the same understanding
the writer (cmdPhaseComplete, with its `cells.length === 5` branch)
already had. Preserves the 999.x backlog exclusion, M/N plan summing, and
the prior contract that a count of 0 is reported as null.
Regression tests fold into tests/state.test.cjs: the template's own
5-column Progress block parses non-null, both shapes derive identically,
999.x stays excluded in the 5-column shape, and decoy tables (an earlier
## Retrospective table and an ### Progress h3 subheading) do not hijack
the scope.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Tom Boucher
|
bd5fcfceeb |
fix(#2125): route complete-phase resolver through canonical parser (review)
Orthogonal review surfaced that resolvePhaseIdForCompletePhase (state.cts) and cmdStateCompletePhase's idempotency check still used an unanchored /(\d+[A-Z]?(?:\.\d+)*)/i — even more permissive than the parseProsePhaseField regex this phase fixes. Reachable corruption: after `milestone complete v0.5`, `state complete-phase` (no --phase) mined "0.5" from the body line "Phase: Milestone v0.5 complete" and rewrote STATE.md as "Phase 0.5 complete". Both sites now delegate to phase-id.cts:parsePhaseFromProse (the same anchored parser), so a milestone-closure line yields no token and the existing "unable to resolve" guard fires instead of corrupting. Canonical tokens (3, 03, 3A, 3.3, "3 of 5", "1 — Setup") are preserved unchanged. Regression (tests/state.test.cjs, complete-phase suite): `state complete-phase` on a "Milestone v0.5 complete" STATE.md now rejects and does not mine "0.5". Demonstrated fail-first. Refs #2125, #2121 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
85ed50cc4f |
test(#1972): consolidate 94 command/module regression tests into subject suites
Fold 94 issue-named command/module regression files into the canonical test file that owns each subject-under-test, across 52 existing suites (state, config, frontmatter, roadmap-parser, capability-registry, shell-command-projection-dispatch, plan-phase-drift-guard, health-validation, runtime-converters, commands, etc.). Verbatim block-scoped describe wrappers; 881 subtests conserved 1:1. No new test files. Host-env pre-check (per B2): the only GSD_WORKSTREAM/GSD_PROJECT-touching destinations (intel, planning-workspace) clear those vars hermetically, so folded CLI tests are safe. Regenerates regression-name allowlist (222->162), ratchets file-count allowlist across 8 buckets (validate entry removed after dropping <=2), makes 34 relocated allow-test-rule exemptions issue-ref-compliant (ADR-456; prunes 34 stale ids). Repoints CONTEXT.md + ADR-0002/443/1235/3524 test-file references. lint:ci green. Part of epic #1969. Closes #1972. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |