* fix(#3544): restore tilde form for at-refs in the global spec tree
A global claude install emitted @$HOME/.claude/gsd-core/references/*.md
in its workflows and references. $HOME does not expand in a Claude Code
@-import - only relative, absolute and ~ are documented, and a controlled
/context test confirmed a $HOME import loads nothing - so 54 includes
across 22 files silently resolved to nothing on a live install.
This is a divergence, not a new bug. #3133 already applies exactly this
correction to skill and command bodies through _applyRuntimeRewrites's
claude case; copyWithPathReplacement, the spec-tree emit path, never had
it. Both now call one exported helper, so the two surfaces cannot drift
apart again.
Deliberately narrower than changing computePathPrefix's return value:
shipped markdown also carries double-quoted "$HOME/.claude/..." shell
invocations, and ~ does not expand inside double quotes, so rewriting the
prefix wholesale would regress #1284. Only @-prefixed references move.
Refs #3544
* fix(#3544): derive the tilde restore from the resolved prefix
Three review findings, one batch.
The restore was hardcoded to the literal .claude directory, so a global
install with --config-dir pointing anywhere else silently no-opped and
reproduced the very defect this fixes. It now derives the tilde form from
the resolved prefix, which also closes the same latent gap in #3133's
original path since both call sites share the helper.
The @-anchor is quote-aware, so a double-quoted shell path is never
rewritten into a form the shell does not expand. Deliberately a lookbehind
rather than a line-start anchor: @-references are documented to work
mid-line, and anchoring would have traded a theoretical bug for a real one.
Found while testing the above: the bare-form rewrites re-matched their own
output whenever a config dir name extends .claude, emitting
.claude-work-work. Guarded with the same negative-lookahead convention
this file already uses to preserve .claude-plugin.
The tests prove the emitted form, never that the host resolves it - no CI
test can - and both the helper and the suite now say so, because an
undocumented verification boundary is how this defect stayed green for its
whole life.
Refs #3544
* test(#3544): acknowledge the tilde-restore emitted drift
The converter change moves 94 emitted paths that no source-file diff can
explain, which is exactly the case the per-PR ack fragment exists for.
Verified before acknowledging rather than after: both trees were built
from real installs and every one of the 211 changed lines across all 94
paths is @$HOME becoming @~, with nothing outside that single kind.
Nine spent entries were pruned from the #3151 and #2658 fragments. Those
paths moved again here, and two ack sources naming one path is a hard
duplicate error rather than last-wins, so the inert entries had to go
before this one could land. Both fragments retain their remaining
entries.
Refs #3544
* chore(#3544): backfill changeset pr number
---------
Co-authored-by: sim <sim@local>
The invisible-Unicode scan flagged the compiled sanitizer: BIDI_RE in
install-shadow-report.cts carried literal U+202A-U+202E where its two
neighbouring regexes already used \u{...} escapes, so the module that
strips bidi controls was itself a carrier for them.
The prompt-injection-scan failure alongside it was the same defect rolling
up through the parent describe, not a second cause - verified by running
the scanner across every category it checks.
Test fixtures and property generators now name their codepoints (RLO, LRE,
PDI) instead of embedding invisible bytes, so a reviewer can see which
character is under test.
Refs #2873
Isolated security review found resolveSpecRootReference's fence tracker
toggled on any delimiter, so a backtick fence could be closed by a tilde
one and an include in the gap was rewritten inside a code block. Fixed by
reusing scanFencedBlocks - the canonical engine already behind
stripFencedCode and extractFencedBlock - rather than carrying a fourth
copy of fence detection, which also closes the duplication the standards
review flagged.
sanitizeForRender now strips combining marks and zero-width characters
alongside the ANSI, control and bidi classes it already handled.
Adds the C, E and F matrix rows the spec review found missing, including
installer-level coverage that spawns the real install rather than calling
the report builder. Ships the how-to, the reference and command docs in
five locales, the changeset, the inventory and glossary entries, and
regenerates health.md for the new W028 rule.
Refs #2873
4a - the detection floor. A shadowed install now reports which triggers are
shadowed and which scope wins, at install time and through a new W028
/gsd-health diagnostic. Exit codes are untouched: a shadowed install is a
warning, not a failure. Only triggers whose stem exists at BOTH scopes are
reported, so a global full profile beside a local core profile no longer
names local artifacts the user does not have.
4b - spec-root reachability, claude runtime and global scope only. The
winning global skill stops carrying a static workflow @-include and instead
resolves its spec at runtime: prefer the project-local copy, fall back to
the global one, stop if neither exists. Every other @-include stays static,
and the local emission is byte-identical. It runs after the staged-skills
rewrite pass, whose claude branch would otherwise mangle the literal tilde
path into an undocumented $HOME form.
Also fixed inline: readInstallManifest classified a top-level JSON array as
an installed v1 manifest, because typeof [] is object.
Refs #2873
New read-only leaf src/install-shadow-report.cts turns Phase 3's
resolveInstalledSurfaces output into a typed shadow-report IR plus a
line-array renderer, with declaredRuntime sanitized at the render seam
(ANSI, C0/C1, newlines, bidi overrides; idempotent, no second truncation
over the reader's 64-char cap).
Also closes the symlink asymmetry the resolver carried: the local scope
resolves against process.cwd(), and this phase is what makes that path
reachable from an arbitrary cloned repository, so the manifest read is now
lstat-guarded rather than following. Matches the getAgentsDir precedent and
degrades to the same installed:false shape the EACCES path already returned.
Refs #2873
* 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 411196bc3 and post-fix, and the same fixtures run through each. Both
byte-identical, modulo the clock-driven last_updated.
* fix(#3408): update every non-typed call site, and make a wrong options call loud
The previous commit claimed 'tsc is the proof a site was not missed'. That was
wrong and I asserted it. tsc type-checks src/ only; the test call sites are
plain .cjs and are not checked at all. Fourteen tests failed with
divergedFields: [] because a positional resync boolean landed in the options
slot and every option came through undefined.
Nine stale call sites converted. Also caught: the drift guard suite's E2
fixture embedded the old call shape 'verbatim from src/milestone.cts' — a
fixture that mirrors production and had silently drifted from it.
The deeper defect is that the refactor itself introduced the failure shape
this epic exists to remove. An options-object parameter is silently
mis-consumable by any non-TypeScript caller: pass the wrong thing and the
function proceeds with every option undefined, returning a well-formed,
plausible, empty result. That is precisely ADR-3408's Context section,
reintroduced by the change meant to tidy the code up.
Both functions now assert their options argument is a non-null object and
throw STATE_PRESERVATION_OPTIONS_INVALID carrying the offending type, mirroring
throwUnwiredRow. A test pins that the guard fires on the exact mistake that
produced these fourteen failures.
Verified by probe rather than asserted: a correct options call returns the
expected divergedFields; a legacy positional call throws with the structured
code instead of silently returning empty.
* chore(#3408): drop the changeset — this PR has no user-facing impact
CONTRIBUTING.md: 'PRs with no user-facing impact (test refactors, lint config
changes, CI tweaks, formatting-only changes) can add the no-changelog label.'
Typing this Changed and then exempting it from the docs requirement would have
been wrong twice: it publishes a CHANGELOG entry under Changed when no command
output moves, and it uses a per-fragment exemption to paper over a type that
was wrong to begin with.
Both refactors are behavior-preserving, verified byte-identical against the
pre-change compiled lib. The one new throw guards a module-private seam in
src/state.cts that no external caller can reach.
* refactor(#3408): derive StatePreservationOptions, narrow the guard message
Two findings from the orthogonal reviews on the close-known-limits PR.
StatePreservationOptions repeated all four ReadModifyWriteOptions fields, differing only in resync being required, and divergedFields carried a second independently-worded docstring. It is now derived via Omit so the shared fields have one definition and cannot drift out of hand-sync.
assertStatePreservationOptions echoed JSON.stringify(options) into the thrown message text. The contract for that guard is a structured code plus the offending type, not the value; echoing the value would become a disclosure path if a caller ever passed user-derived data. Removed from the message; err.code and err.receivedType are unchanged, and the test asserts on those.
---------
Co-authored-by: sim <sim@local>
* fix(#3518): pin uat_path to the phase's own UAT artifact via the shared phase-pinned resolver
Both uat_path projectors in src/init.cts picked the phase's UAT file with a
bare .find() over unsorted readdir order — no phase-membership check, no
ordering — so a stray cross-phase 04-UAT.md in phase 03's directory could
become phase 03's uat_path, filesystem-dependently (creation order on APFS,
hash order on ext4/XFS): two machines on the same commit could emit different
uat_path values for the same phase.
Route both sites through a new resolveUatFile in src/verification.cts, the
UAT counterpart of #3357/#3492's resolveVerificationFile, sharing the exact
selection rule via one extracted core (resolvePhaseArtifactFile): the phase's
own <token>-UAT.md always wins; otherwise the alphabetically-first dashed
candidate (deterministic everywhere); a bare UAT.md only via allowBare when
no dashed candidate exists. resolveVerificationFile now delegates to the same
core — behavior byte-identical.
Guarded by: two end-to-end repro tests in tests/init.test.cjs (plan-phase and
phase-op, red on the pre-fix readdir pick), resolveUatFile contract anchors
and a src/-wide call-site guard in tests/verification-status.test.cjs.
* chore(#3518): add changeset fragment for PR #3525
---------
Co-authored-by: sim <sim@local>
* 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>
* 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>
A phase directory can hold more than one `*-VERIFICATION.md` — an ad-hoc `03-CORRECTION-VERIFICATION.md` worksheet beside the real `03-VERIFICATION.md`. Discovery took the alphabetically-first match, so the worksheet won and the phase could report `missing` while a passing report sat next to it.
The issue named two copies. There were seven, in four grammars: two `.sort()[0]` sites in the verification module, three `.find()` over UNSORTED readdir order (phase status, and `verification_path` twice — filesystem-dependent, so two machines on one commit could disagree), and two in shell. All seven now route through one exported `resolveVerificationFile`; the shell copies via a new `verification resolve-file` verb rather than hand-rolling the rule an eighth time.
Fixing five of seven would have been worse than fixing none: the verify-work workflow is a WRITER that stamps `status: passed` onto the file it picks, so canonical-aware readers plus an alphabetical writer means the human_needed→passed canonicalization silently no-ops forever while the worksheet gets stamped. That divergence did not exist on next.
The first resolver was itself a regression — it preferred ANY canonically-shaped name over the phase's own report, so a stray cross-phase or sentinel-numbered file outranked it. The global-canonical preference was removed rather than narrowed; the rule is pinned to the phase token via `PHASE_NUMBER_TOKEN_SOURCE`, its existing owner.
Fixed in passing: the transition workflow's awk guarded on `NR==1` instead of `FNR==1`, so across a multi-file glob it armed only on the first file — a leading worksheet with no frontmatter blocked transition even when the canonical report passed. Also removed a U+00AD soft hyphen introduced earlier on this branch.
Five broader-grammar AGGREGATE scans are deliberately out of scope — a different defect class (phase-unscoped scanning), tracked as #3511.
Closes#3357
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cmdVerifyKeyLinks` resolved `from:` and `to:` with `path.join(cwd, <value>)` where the value comes verbatim from plan YAML. `path.join` normalizes `../` rather than rejecting it, so a plan travelling with a repository could name any file the process can read, and the command reports whether the link's `pattern` matched it — an arbitrary-file-read oracle reachable from `verify-phase`.
Both reads now go through `validatePath` in src/security.cts, the existing realpath-based confinement seam already used at 11 call sites. Not a missing capability — a bypassed one.
Two defects in that seam, found by adversarial review and fixed here because they affect all 11 callers:
1. A dangling in-project symlink escaped confinement. A link to an EXISTING outside path was refused (realpath lands outside) while a link to a MISSING outside path took the parent-resolution fallback and was accepted — an existence oracle for arbitrary absolute paths. lstat succeeds on a dangling link and throws ENOENT on a truly absent path; an unresolvable link is now refused. A symlink resolving inside the project is still accepted.
2. A canonicalized base was compared against an uncanonicalized path when a file and its parent were both missing, wrongly refusing legitimate in-project paths on any non-canonical cwd (every macOS temp dir). This was a live regression in this PR: the wave-pending classification (#1202) depends on the not-yet-created case. Resolution now walks up to the nearest existing ancestor.
Two adjacent aborts fixed: the from: read sat outside the per-link try, so a non-ENOENT errno killed the whole command; and an empty from: read the cwd directory, throwing EISDIR. Both now fail per-link.
The issue was filed as a fourth ADR-0174 consolidation loss. It is not one — validatePath/requireSafePath never went away, only the SDK's name for the concept did. This is an instance of epic #3473's F2 family. The ADR-0174 loss count is three.
Closes#3493
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* 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>
`cmdVerifyKeyLinks` compiled `must_haves.key_links[].pattern` from plan frontmatter with `new RegExp()` and tested it against whole file contents, so a nested-quantifier pattern such as `(a+)+$` hung `verify-phase` indefinitely (CWE-1333). JavaScript has no regex-execution timeout.
Untrusted patterns now run on RE2 (re2js), whose match time is linear in input length — the class is closed by the engine, not by a heuristic screen. The screen lost in the ADR-0174 consolidation was deliberately NOT restored: it never worked, since `(a|a)*$`, `((a+))+$`, `(a+){2,}$` and `(a{1,3})+$` all evade it. A refused pattern's matcher returns false for every input, so it cannot report a match no matter what the caller does.
The engine is vendored at gsd-core/bin/lib/vendor/re2js.cjs because gsd-core/bin/** is copied into installed trees with no node_modules; runtime dependencies are unchanged. New ESLint rule local/no-external-require-in-bin enforces that invariant, which had been documented in a comment since the #3024/#2071 bug class and enforced nowhere.
Backreferences and look-around are unsupported by RE2 by construction — disclosed in a Changed changeset.
Closes#3477
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RegExp.escape is ES2026 (first shipped in Node 24); pattern.cts called it
unconditionally, and the build consumes the module
(scripts/gen-loop-host-contract.cjs), so npm run build failed on Node 22 and
the gsd-test linux-node22 lane could not reach run_tests.
Fix: prefer the built-in when present, else an in-file metachar escape —
still the sole owner of escaping (#3212 invariant; lint scope unchanged).
Builtin captured at module load so runtime mutation cannot flip the path.
Regression: tests/pattern.test.cjs section 4 — child-process probes neuter
RegExp.escape before/after require and assert match-behavior equivalence.
Co-authored-by: sim <sim@local>
* 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.
c90ae479f fix(#3350) reworded cmdPhaseComplete's syncStateFrontmatter call
onto one line and changed its third argument. Because entries are keyed on
(file, trimmed source text) rather than a line number, that single upstream
edit registered as BOTH a stale acknowledgment and an unrecorded site — the
two-sided signal the design intends, forcing a human to look rather than
letting a tracked bypass drift out of view.
The owner-preserving merge behaved exactly as designed: three owners survived
because their keys were unchanged, and phase.cts's dropped to null because its
source text is genuinely a different key. Re-curated to #3469, the phase that
owns its removal.
Note for Phase 2: c90ae479f is #3350's fix landing independently on next —
one of the two instances Phase 2 was scoped to drive fail-first. Surfaced to
the epic rather than absorbed silently.
* test(#3468): derive B1's fixture from the table so it cannot go stale
Checkpoint 2 came back with 2 failures of 33803, both B1:
actual 'current_phase_name'
expected 'current_plan'
The implementation was right and the test was stale. B1 hand-built a
bodyDeltas literal intending current_plan to be the ONLY unwired row, but it
also omitted status, stopped_at and current_phase_name — all three of which
became preserve-when-unchanged rows in THIS PR. Table order puts
current_phase_name first, so the throw correctly named it.
B1 now builds from neutralBodyDeltas() and deletes exactly one key, which is
what its own comment always claimed it did. A future table change can no
longer silently make it assert the wrong field.
Audited every other bodyDeltas literal in the file: four exist, all correct —
two enumerate all seven rows explicitly, two pass {} where the emptiness is
the point of the test. Roughly thirty other sites already derive from the
helper.
Also renames the local unchchangedChanged to lastActivityDescChangedDeltas.
A typo'd identifier that happens to work is still a Mysterious Name; noted
during research and fixed now that this change touches the file.
* chore(#3468): re-curate ratchet and fold the seam channel into the shared helper
The rebase onto be9329b10 fix(#3374) was a true semantic conflict, resolved
rather than handed back, because the resolution was determinable:
That PR extracted the post-sync preservation pass into a shared
applyPostSyncPreservation helper — which is ADR-3408 section 8.3, i.e. a
piece of Phase 2's own deliverable, landing upstream. Its structure is kept
wholesale; this branch's contribution is applied INSIDE it.
That combination had to be checked rather than assumed. Upstream's helper
wires only FOUR bodyDeltas keys and still passes status / stopped_at /
current_phase_name through six dedicated parameters. This branch reclassifies
current_phase_name to preserve-when-unchanged, deletes those six parameters
from StatePreservationInput, and makes an unwired declared row THROW. Taking
upstream's file as-is would therefore have thrown on EVERY STATE.md write.
The helper now wires all seven rows through the single channel. Verified
7-to-7 against FIELD_CLASSIFICATION, with a clean tsc — which is the real
proof the dedicated parameters are gone, since they no longer exist on the
input type.
The ratchet also caught the same phase.cts call being reworded a second time,
reporting it as both a stale acknowledgment and an unrecorded site. Re-curated
to #3469. Recording the tradeoff plainly: keying on (file, source text) means
an upstream reword of a tracked line needs re-curation, where keying on line
numbers would churn on every unrelated edit. ADR-3180 Decision 4(e) chose
source text deliberately, and the owner-preserving merge added earlier covers
the common case where the text is unchanged.
* chore(#3468): backfill pr number in changeset fragment
---------
Co-authored-by: sim <sim@local>
* 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>
* fix(#3395): own the phase line in planned-phase and persist --name
* fix(#3395): backfill changeset pr 3490
---------
Co-authored-by: sim <sim@local>
* 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>
* fix(#3355): pick phase-dir dedup survivor from content, not mtime
* chore(#3355): add changeset fragment
* chore(#3355): backfill PR number in changeset fragment
---------
Co-authored-by: sim <sim@local>
* fix(#3384): strip mcp__* tool grants from zcode-installed subagents
ZCode's dispatcher treats every mcp__<server>__* entry in an agent's
tools: frontmatter as a required MCP server and hard-fails the subagent
spawn (CONFIGURATION_ERROR) when it is not connected, whereas Claude Code
treats the same grants as an optional allowlist. ZCode shared Claude's
verbatim agents copy (converter: null), so all 8 MCP-granted agents
failed to spawn out of the box with zero MCP servers configured.
Add convertClaudeAgentToZcodeAgent — a line-surgical converter that
filters mcp__* entries out of the frontmatter tools: grant list (both
inline comma and YAML block-list shapes) and preserves every other byte.
Declare it on both of zcode's capability.json agents entries and cut
zcode over to the descriptor-driven agents path
(_DESCRIPTOR_AGENTS_RUNTIMES) so the legacy inline loop stops
deleting+re-copying the converted agents raw. Claude Code, Kimi, and
Gemini install behavior is unchanged.
* chore(#3384): link changeset fragment to pr 3483
---------
Co-authored-by: sim <sim@local>
* fix(#3354): preserve stored total_phases when milestone is unbounded
* chore(#3354): backfill PR number in changeset fragment
---------
Co-authored-by: sim <sim@local>
* test(#1884): reproduce phantom lock timeout from swallowed mkdir failure
withPlanningLock swallows a platformEnsureDir failure at src/planning-workspace.cts:210,
so an EACCES/ENOSPC/EROFS creating .planning/ is misreported as a 10s "held by a live
process" lock-contention timeout instead of the real filesystem error. These tests pin
the corrected contract and currently fail against the unfixed source (RED). Also proves
the pre-existing Docker overlay-fs lock-write retry race is unaffected by this change.
* fix(#1884): surface mkdir failures instead of a phantom lock timeout
withPlanningLock swallowed platformEnsureDir failures (EACCES/ENOSPC/EROFS/EMFILE)
creating .planning/, so the subsequent lock write failed with ENOENT (parent
missing), which is retryable (PLANNING_LOCK_RETRY_ERRNOS, added for a Docker
overlay-fs race). The loop then spun the full 10s budget and threw a phantom
"held by a live process" contention error pointing at a nonexistent holder.
The mkdir failure now propagates immediately with its real errno and message.
The Docker overlay-fs ENOENT lock-write race (directory present) is unaffected,
as is every path where .planning/ already exists or is creatable.
Per docs/adr/1411-resolution-provenance.md's 2026-07-26 amendment, this is the
one site in epic #1879 that legitimately throws (ADR-227's genuinely-fatal
carve-out) -- its defect was throwing the wrong error after swallowing the
real one, not that it threw at all.
* chore(#1884): backfill changeset PR number
---------
Co-authored-by: sim <sim@local>
* fix(#3329): reconcile stale managed .sh hook commands on install/update
applySettingsJsonHooks registers the four .sh managed hooks only-if-absent,
so entries registered before the #580/#3393 shellHookOmitsBashRunner fix kept
their bash-runner-prefixed commands forever — /gsd-update re-invokes the
installer but never re-derived existing entries. Add
reconcileManagedShellHookCommands (wired into applySettingsJsonHooks): on
win32+claude it rewrites existing managed .sh entries to the command this
install would generate today, scoped to exact managed basenames so
user-authored hooks are untouched, and inert wherever the bash runner is
still the correct shape.
Also bumps the allow-test-rule ceiling 301→302: PR #3455 added
tests/milestone-lock.test.cjs (the 302nd marked file) without the ratchet
bump, leaving lint-tests red on next.
* chore(#3329): add changeset fragment
* chore(#3329): backfill changeset pr number 3460
---------
Co-authored-by: sim <sim@local>
* 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>
* fix(#3311): milestone lock makes parallel-phase state conflicts visible
Two sessions running different phases in one working tree silently
clobbered STATE.md's single un-scoped ## Current Position slot: byte-level
serialization already existed (STATE.md.lock since #464), but nothing ever
surfaced that two sessions claimed two different phases, and
state.advance-plan — which takes no phase argument — kept advancing whatever
plan the just-clobbered position named.
Adds the maintainer-chosen milestone lock (issue #3311 comment): an advisory
.planning/milestone.lock claim keyed by phase + session id (session identity
via getWorkstreamSessionKey: env-first, then controlling TTY). begin-phase
claims (inside the STATE.md lock), advance-plan detects a claim/position
mismatch and heartbeats a matching claim, phase.complete warns via warnings[]
and releases the claim when the claimed phase completes. Conflicts warn
(stderr + typed milestone_conflict JSON field) instead of blocking, per the
decision's blocking/warning latitude; TTL 4h with heartbeat liveness expires
abandoned claims. milestone.lock is registered in the canonical artifact
registry so validate.health W019 recognizes it.
* chore(#3311): point changeset fragment at pr 3455
---------
Co-authored-by: sim <sim@local>
* test(#1762): assert reassuring verification-routing wording (RED)
Regression test for #1762 — asserts readVerificationStatus's missing/
unknown next_action text reassures the user that execute-phase resumes
at the verification gates without redoing work, instead of reading as
a blind re-run instruction. Fails against current wording; the fix
lands in the next commit.
* fix(#1762): reassure verification-missing/unknown routing text is safe to run
readVerificationStatus's 'missing' and 'unknown' next_action text read as
"redo the implementation", when execute-phase's own discover_and_group_plans
step (#2868) already resumes at the verification gates and skips
execute_waves/checkpoint_handling entirely when every plan already has a
SUMMARY.md. Reword both to say so explicitly, and soften the 'unknown'
message to acknowledge a non-standard status may be an intentional marker
rather than presuming re-verification is always the fix.
Also updates progress.md's Route V.missing / V.unknown to consume the
dynamic $VERIFICATION_NEXT_ACTION (matching V.gaps/V.human) instead of a
hand-duplicated string, so the reassurance lands there too without a second
copy to keep in sync.
No change to next_command routing, status values, or the isPhaseComplete
completeness predicate (still gated on status === 'passed' per the #2957
DISK-STRICT decision) — advisory text only.
* fix(#1762): drop duplicated reassurance clause in progress.md routing text
Review finding: the SUMMARY.md reassurance was stated once via the
interpolated $VERIFICATION_NEXT_ACTION and again as a parenthetical on
the command line, in both Route V.missing and Route V.unknown. Trimmed
the parenthetical so $VERIFICATION_NEXT_ACTION stays the single source
of truth.
* docs(#1762): add changeset fragment
Type Changed with a docs-exempt marker — advisory routing text only,
no docs page documents this specific message today.
* test(#1762): acknowledge progress.md emitted-content growth
Route V.missing/V.unknown now render a small block referencing
${VERIFICATION_NEXT_ACTION} instead of a bare command line, growing
progress.md by 376 bytes. Deliberate per the differential attribution
check (ADR-2719).
* chore: drop spent progress.md ack from #3218's emitted-drift fragment
The #3218 growth it described is already on next (inert, per the
lint's own note that spent acks 'can no longer clear anything').
Its presence collided with the new #1762 fragment naming the same
bare filename, which lint-emitted-drift-ack rejects outright ('two
ack sources may never name the same path'). #3218's other two
acks (plan-phase.md, plan-review-convergence.md) are untouched.
* docs(#1762): backfill changeset PR number
---------
Co-authored-by: sim <sim@local>
* fix(#3189): drop non-id prose from phase_req_ids before gap check
* chore(#3189): pin changeset fragment to pr 3438
* fix(#3189): use hyphenated IDs in e2e fixtures (parseRequirements requires hyphen)
* fix(#3189): assert e2e item set via table, not rows (check query has no rows)
* fix(#3189): check prose fragments against table rows, not the table heading
---------
Co-authored-by: sim <sim@local>
