golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#3784): read the hybrid Current Plan: N of M shape, keep zero-padding, and name the accepted shapes on failure (#3791)

Browse Source 
* fix(state): read the hybrid "Current Plan: N of M" shape

advancePlanCore derived the value FORMAT from the field NAME, so it
handled the legacy pair (`Current Plan` + `Total Plans in Phase`) and the
compound `Plan: N of M`, but not the hybrid of the two: the legacy field
name carrying a compound value with no Total Plans sibling. `legacyTotal`
is null so the legacy branch fell through, and the compound branch reads
the `Plan` field through a `^Plan:`-anchored pattern that never matches
`Current Plan:`. Both produced NaN against a file whose plan numbers are
plainly readable.

The shape is not exotic. An agent wrote it unprompted into a project's
STATE.md, believing it was the parseable form, and every subsequent run
in that project inherited the failure and worked around it by hand.

Track the field name and the value shape separately (`planSourceField`,
`planRawValue`) so write-back targets whichever field the value came
from. The legacy pair still takes precedence when both fields exist, so
a stray "of N" inside Current Plan cannot override an explicit Total
Plans — covered by a new test.

Also replace the caller's catch-all error. It reported "Cannot parse
Current Plan or Total Plans" for ANY transition failure, and named no
accepted shape, so a reader learned neither what failed nor what to
write. It now distinguishes "no result" from "unreadable plan position"
and lists all three shapes. The existing test asserted the literal
"cannot parse"; it now asserts the message names the shapes, which is
the property that makes it actionable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8

* fix(state): keep zero-padding when advancing a compound plan value

The compound write-back rewrote only the leading half of "N of M", so a
padded value drifted lopsided: "04 of 06" advanced to "5 of 06". Cosmetic
on its own, but a plan line that looks wrong is one the next writer tidies
by hand, and hand-tidying this particular line is what produced the hybrid
shape the previous commit had to teach the parser to read.

Pad the incremented number to the width it was written with. padStart never
truncates, so a value that outgrows its padding widens correctly: 09 of 12
advances to 10 of 12. Unpadded values are untouched — 2 of 6 still advances
to 3 of 6.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8

* fix(state): pass a literal field name to the compound write-back

The previous commit passed `planSourceField` — a variable — as the field-name
argument to `stateReplaceField`, which trips the state-write-path drift guard's
`unstripped_content_write` axis (ADR-3408 §8.3(b)). The guard is right to care:
a Title-Case literal cannot collide with a lowercase or snake_case frontmatter
key, so it is safe whatever the content argument is, while a variable could
hold anything and therefore requires its content to be demonstrably
frontmatter-stripped first.

The content argument here IS stripped — `body` is `stripFrontmatter(content)` —
but the guard does a narrow backward scan rather than dataflow tracking, by
design, and the nearest preceding assignment to `body` is another
`stateReplaceField` result. Rather than baseline a bypass or ask a future
reader to re-derive that the invariant holds, dispatch on the discriminator and
pass the literal.

Guard goes from 1 finding to 0; its own 32 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8

* chore(3784): add changeset fragment for #3785

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8

* test(#3784): cover the maintainer's AC1 write-back and AC6 reader-anchoring

Triage published six acceptance criteria; two were only half-covered.

AC1 asks that the hybrid write back to the SAME field with padding preserved.
The existing hybrid test used an unpadded value and asserted only `result.data`,
so it proved the parse but never the write. Now asserts the written content is
`05 of 06` on the original field, and that no separate `Plan:` field appears as
a side effect.

AC6 asks that the shared field reader not be loosened. Reading the hybrid is the
transition's job; `stateExtractField('Plan')` is line-anchored and has 13+
callers, so teaching it to match a name merely ENDING in "Plan" would be the
wrong fix and would silently change what those callers read. This holds by
construction here — the reader is untouched — but nothing locked it in. The new
test fails if anyone later reaches for that shortcut.

Also drops the changeset fragment written against the auto-closed PR number.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8

* chore(#3784): add changeset fragment for #3791

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016QHxbMHTPYbEqAnKTpJPR8

* fix(#3784): write the advanced plan back to the field it was read from

Review findings 2-6 on #3791 were one defect seen from several angles: the
read path learned the hybrid `Current Plan: N of M` shape, the write path
did not follow it.

- `bumpLeadingNumber` now owns the increment for all three parse branches.
  Only the leading digits belong to this transition; the padding width and
  everything after it (` of M`, and the `\r` of a CRLF file) are the
  author's text and are preserved. The legacy branch wrote `String(newPlan)`,
  which turned `2 of 99` into `3` and `04` into `5`.
- `mutateCurrentPositionForAdvance` takes the plan field NAME. Its plan arm
  only ever looked for `Plan:`, so on a hybrid file the `## Current Position`
  section was never reached; combined with the body-level write being
  single-shot and bold-preferring, a file carrying the field at both sites
  advanced the header and left the section a plan behind. The parameter
  defaults to `Plan`, so the two callers that pass no plan are unchanged.
- Tests: both-sites-advance (fails without the section arm), legacy
  write-back content assertions (the previous test read only `data` and so
  could not see the lossy write), hybrid boundary at limit-1 and limit+1, a
  CRLF fixture, and an fc property pinning the padding-width contract.

Two characterization tests pinned `**Current Plan:** 02` advancing to `3`.
That dropped padding is the defect #3784 reports, so the expectation is
corrected to `03` rather than the fix being narrowed around it.

* fix(#3784): drop the unreachable advance-plan error branch, sync the doc

Findings 1 and 8 on #3791.

The `!resultData` arm could not fire: the transform callback assigns
`resultData` unconditionally, only runs once STATE.md is known to exist (the
missing-file case returns "STATE.md not found" upstream), and every
`advancePlanCore` return path sets `data`. It was a speculative second
failure mode with a message no caller could receive, and the comment beside
it claimed to distinguish two things that were never two. `!resultData`
stays in the condition as a type guard, which is all it ever was.

`docs/json-errors.md:142` quoted the old error literal verbatim and was the
sole occurrence in the tree; it now quotes the emitted one.

* chore(#3784): describe the write-back fix in the changeset

* fix(#3784): anchor the plan grammar and widen the schema row to match

Review round 3 on #3791: B1, B2, M1, M2, M3, M4 and the planSourceField nit.

B1 — `STATE_FIELD_SCHEMA.current_plan.acceptedShapes` widens to
`['N', 'N of M']`, which is what `src/state-md-schema.cts`'s own comment
instructed this PR to do on merge. `'N/M'` stays undeclared so row 23 keeps a
non-vacuous undeclared candidate to probe. Verified `gen-state-md-docs
--check` exits 0 and `--write` rewrites 0 of 6: the generated artifacts do not
surface this row, so there is nothing stale to regenerate.

B2/M4 — the discriminator was `/of\s+(\d+)/`, unanchored, so a total could be
read out of prose. `Current Plan: 4 — blocked on review of 2 PRs` parsed as
`4 of 2`, took the `currentPlan >= totalPlans` branch and WROTE
`Status: Phase complete — ready for verification` into the user's file. Both
shapes are now anchored at the start and every number comes from a capture
group via `planNumberFrom`, which rejects anything past `Number.MAX_SAFE_INTEGER`
rather than letting `data` and the persisted string disagree. Nothing on this
path calls `parseInt` on a raw field value any more.

The grammar keeps a trailing remainder after the total, because
`Plan: 2 of 5 in current phase` is a real tested shape. The refusal comes from
requiring `of <total>` to follow the leading number immediately, not from
forbidding a suffix.

M1 — `bumpLeadingNumber` is total. It returned its input unchanged when there
were no leading digits, so `+2` reported `advanced: true` while writing the
file untouched.

M2 — both section arms use replacer functions. File-derived text was being
spliced into a `String.replace` replacement string, where `$&` / `` $` `` /
`$'` expand: a value of `04 of 06 $&` spliced part of the document into itself.
`stateReplaceField` already used a function; these now agree with it.

M3 — the section arm targets the name the SECTION carries, and the body write
now writes both spellings, each with its own rendering. Keying off the header's
name left the other name stale in both directions: a legacy header beside a
`Current Plan:` section line, and a `**Plan:**` header beside one.

* fix(#3784): derive the shape error from the schema, widen the test coverage

Review round 3 on #3791: B3, m1, m2, m5 and the two test nits.

B3 — the accepted-shape set had two owners: the parser branches and an English
list hand-written beside them in `state.cts`. Nothing coupled them, so adding a
branch left the message stale and removing one left it advertising a shape that
errors, with no test able to see either. The message is now built from
`STATE_FIELD_SCHEMA.current_plan.acceptedShapes`, and the CLI test walks the
schema instead of restating the list. `Plan: N of M` is still spelled out
explicitly because no schema row owns the body-only `Plan` field —
`buildStateFrontmatter` never reads it into frontmatter, so it has no key to
hang a row on.

m1 — the property drove only the pre-existing `**Plan:**` branch, i.e. not the
branch under review. It now drives both compound spellings and ranges past 99
so the width transition is covered by the property rather than one example. A
second property covers the legacy pair's own preservation contract. Both were
mutation-checked: dropping the padStart turns 9 tests red.

m2 — degenerate boundary fixtures around the threshold (`0 of 0` is
phase-complete, not an error; `0 of 3` advances) plus the shapes the anchored
grammar must refuse, including Arabic-Indic digits.

m5 — `docs/json-errors.md` described rather than quoted the message, since it
is now schema-derived and a verbatim quote would be a third owner.

Nits — the CRLF assertion could not see a `\n` at index 0; the
`!/^Plan:/m` presence proxy is now an identity assertion on the whole
`## Current Position` body.

* fix(#3784): give the section plan write its own flag, and stop narrowing what parses

Review round 4 on #3791: Blockers 1-4, Majors 1-2, Minors 1-2.

B1 — the section fallback was guarded by `!mutated`, and `mutated` is
FUNCTION-wide, already set by the phase/status/lastActivity arms that
`advancePlanCore` always populates. A section spelling the field bold or as a
pipe-table row therefore skipped its fallback because an UNRELATED field had
been refreshed, and stayed a plan behind the header — the split-brain document
this arm exists to prevent. The arm now tracks its own `planWritten`.

Worth recording: the reviewer's fixture does not reproduce. The body-level
status write lands on the section's own `Status:` when the document has no
header `Status:`, so `mutated` is still false by the time the plan arm runs and
the fallback fires. The discriminating shape needs a header `Status:` to absorb
that write AND a bold section plan line. The mechanism was right; the example
was not, and the regression test uses the shape that actually fails.

B2 — `fallbackName` chose one name by ternary. In the legacy shape both values
are populated, so it always chose `Current Plan` and a `**Plan:**` section line
— which base did write — got nothing. Each name is now attempted independently
with its own fallback.

B3 — `PLAN_SHAPE_N` was anchored harder than `PLAN_SHAPE_N_OF_M`, so values base
parsed via `parseInt` began to hard-error: `Total Plans in Phase: 5 phases`,
`Current Plan: 3 (blocked)`. #3784's brief puts normalizing plan numbers beyond
this transition's read/write out of scope, so that narrowing was not licensed.
Both grammars now carry the same trailing tolerance. The prose defect stays
closed by the START anchor, not by forbidding suffixes.

Major 1 — the whole-body `Plan` write is scoped to documents that declare a
`Plan` field, instead of firing unconditionally where `stateReplaceField`'s
first match could be prose outside `## Current Position`.

Major 2 — the error message names both `Plan` spellings the parser accepts; it
previously omitted the sibling-paired form, which is the same
message-disagrees-with-parser drift the derivation exists to close.

B4 — the changeset claimed a guarantee B1 broke; it now describes what ships.

Minors — safe-integer boundary coverage at limit-1/limit/limit+1, and the CRLF
comment states the real mechanism (`stateExtractField`'s `(.+)` stops before the
CR; the trailing group is belt-and-braces, not the primary defence).

All three blocker regression tests verified red against the pre-fix source.

* test(#3784): pin the hybrid shape against #3807's ambiguity refusal

#4028 landed `advance-plan`'s multi-`Phase:` refusal on `next` after this
branch's last run, on the same function. The guard sits above the parse, so
a refused document is never parsed and the shape #3784 adds cannot reach the
mutation — but that is a property of source ordering, so assert it as
behaviour instead.

Fail-first proven, not assumed: with `phaseCandidates.length > 1` disabled,
the ambiguous hybrid document advances its FIRST entry's
`Current Plan: 04 of 06` to `05 of 06` and writes it — #3807's exact defect,
reached through #3784's shape. Both tests go red; both go green with the
guard restored.

The control pins the other direction: an unambiguous hybrid section still
advances, and its zero-padding still survives.

* fix(#3784): advance every spelling from its own text, refuse when they disagree

Round 6 review. B1 and M1 are one defect, so they are one fix.

`advancePlanCore` picked one field to parse from, computed `newPlan`, then
wrote BOTH spellings from that field's numbers. Two symptoms:

  B1  With `Plan` as the parse source, `Current Plan` was re-stamped with
      the number just derived from `Plan`. `Current Plan: 7` beside
      `Plan: 2 of 5` silently became `Current Plan: 3` — a value nothing
      derived for that field, no error, no diagnostic.
  M1  With the legacy pair winning, the `Plan:` line was re-rendered from
      a bare `${newPlan} of ${totalPlans}` built out of the sibling field.
      `Plan: 2 of 9` became `3 of 5`; `Plan: 03 of 05` became `4 of 5`.
      The changeset's claim that padding and everything after it survive
      was true only for whichever field happened to be the parse source.

Now: every spelling is advanced from its own raw text via
`bumpLeadingNumber`, so each keeps its own padding, its own total and its
own trailing annotation. Differing TOTALS are preserved, not reconciled —
`Plan: 2 of 9` beside a `Total Plans in Phase: 5` advances to `3 of 9`.

Differing CURRENT numbers are refused, with `reason:
"ambiguous_plan_position"` and both candidates named. Same posture as
#3807's multi-`Phase:` guard one field over: name the conflict, let the
caller resolve it, never pick. The guard sits immediately after the parse,
BEFORE the phase-complete branch — guarding only the normal advance would
let `Current Plan: 7` beside `Plan: 5 of 5` write a terminal "Phase
complete" into a document whose two spellings never agreed.

A field present but unreadable (`Plan: TBD`) is left exactly as authored.
Refusing the whole document because an unrelated line cannot be read would
be a narrowing #3784 does not license; writing a derived number over it is
the fabrication B1 was filed for.

The `planSourceField`/`planRawValue`/`useCompoundFormat` tracking is gone.
It existed only so the write path could ask which field the value came
from, and the write path no longer asks.

M2. The bare `Plan: N` + `Total Plans in Phase: M` shape is dropped. A
revision of this PR added it; base refused it. It cannot be given the
schema-row + forcing-test coupling the other shapes have, because `Plan`
is body-only and `buildStateFrontmatter` never reads it into frontmatter,
so there is no `current_*` key to hang a row on. Parser, the spelling in
`advancePlanShapeError`, and the lockstep test move together — the
invariant is the lockstep, not the length of the list.

N1. The whitespace narrowing (`5phases` no longer parses where `parseInt`
read 5) is documented in the changeset beside the other deliberate
narrowings, rather than loosened. Loosening restores the half-parse this
change exists to remove.

Tests: eight new cases plus a property that crosses the two spellings with
agreeing and disagreeing numbers — the review noted the existing
properties never did. Fail-first proven: restoring the old write path
reddens seven of the eight, both new property arms, and two pre-existing
padding tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H3eK225hgcnEDZsnmtaP1U

* fix(#3784): report Current Plan as updated only when it was written

The write became conditional in the previous commit — a `Current Plan:`
that is present but unreadable is left as authored — but the `updated`
push stayed unconditional, so `transitionCore` reported a field it had not
touched. `reconcileReportedFields` would have caught it against the
persisted bytes at the `state.cts` caller, but `transitionCore`'s own
`updated` is consumed directly (milestone-lock, the transition tests) and
has to be true on its own.

Covers the mirror of the unreadable-spelling case: `Current Plan: TBD`
beside a readable `Plan: 2 of 5`, where `Plan` is the parse source and the
legacy field is the one that cannot advance. Fail-first proven.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H3eK225hgcnEDZsnmtaP1U

* test(#3784): account for the new refusal in the output({error}) census

`tests/io.test.cjs`' A3 census asserts the exact population of
`output({error})` call sites in `src/`, per module. The
`ambiguous_plan_position` refusal added a 27th to `state.cts`, so the
census went red at 26/65.

Updated the way #3807 updated it when it added the ambiguous-POSITION
error one line above: bump the count and name the addition inline, so the
next person reads why the number is what it is. The alarm did its job —
it is the only gate that noticed a new user-visible error path had been
introduced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H3eK225hgcnEDZsnmtaP1U

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Adnan
2026-09-01 20:03:20 +01:00
committed by GitHub
parent 7c116b1c17
commit bdfc62889b
9 changed files with 1241 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
src/state-md-schema.cts
View File

@@ -186,19 +186,29 @@ export const STATE_FIELD_SCHEMA: Readonly<Record<string, StateFieldSchema>> = Ob
// (verified: it calls `stateExtractField(bodyContent, 'Current Plan')`
// only), so that shape is out of scope for this row regardless.
//
// #3784 is the open issue for teaching `Current Plan` to read the
// hybrid shape; **PR #3791** ("fix(#3784): read the hybrid
// `Current Plan: N of M` shape, keep zero-padding, and name the
// accepted shapes on failure") is the in-flight fix. Do NOT widen
// this row speculatively — that would assert a shape the shipped
// parser does not accept, which is the exact defect class §8.8
// exists to make impossible. When #3791 merges, `acceptedShapes`
// MUST widen to `['N', 'N of M']` — until then, the row 23/24/25
// parser-shape tests (`tests/state-transition.test.cjs`) will go RED
// the moment the parser changes underneath it. That failure is the
// forcing function working as designed, not a broken test: it is
// what stops the schema and the parser from drifting apart silently.
acceptedShapes: Object.freeze(['N']),
// #3784/#3791 WIDENED THIS ROW. The paragraph above describes the
// pre-#3791 parser and is kept as the record of what the shape was
// before, because row 25 exists to stop exactly that reading from
// being re-asserted by accident.
//
// `advancePlanCore` now accepts the hybrid shape, so the declared set
// is `['N', 'N of M']`. Two properties of the widening matter to a
// future reader:
//
// - It is ANCHORED. The parser matches `/^(\d+)\s+of\s+(\d+)\s*$/`
// against the whole value, so `4 — blocked on review of 2 PRs`
// is REJECTED rather than yielding a total of 2 out of prose.
// Declaring `'N of M'` is a claim about that grammar, not about
// "contains the word of".
// - `'N/M'` stays UNDECLARED and must keep failing. Row 23 probes
// the undeclared remainder of `SHAPE_EXAMPLES`, so it needs at
// least one member outside the declared set to stay non-vacuous.
//
// Widening this row without widening the parser (or the reverse) goes
// RED on rows 23/24/25. That coupling is the forcing function, and it
// is the reason this row is data rather than a predicate: §8.8's
// "parsers are checked, not generated".
acceptedShapes: Object.freeze(['N', 'N of M']),
emitted: 'when-present',
} as StateFieldSchema,

372
src/state-transition.cts
View File

@@ -1288,6 +1288,93 @@ function mutateCurrentPositionResume(
return body.slice(0, span.start) + sectionBody + body.slice(span.end);
}
/**
* The two value grammars `Current Plan` / `Plan` accept, ANCHORED to the whole
* value. These are the executable half of `STATE_FIELD_SCHEMA.current_plan`'s
* declared `acceptedShapes` (`['N', 'N of M']`); ADR-3473 §8.8 keeps the parser
* hand-written and has rows 23/24/25 assert the two agree, so widening one
* without the other goes red rather than drifting.
*
* Anchoring is the whole point. An unanchored `/of\s+(\d+)/` reads a total out
* of prose — `Current Plan: 4 — blocked on review of 2 PRs` yields `4 of 2`,
* which is `currentPlan >= totalPlans`, which WRITES a terminal
* "Phase complete" status into the user's STATE.md. Refusing to guess is the
* behaviour #3840/`308c17505` settled for a malformed feature `order`, and it
* applies here for the same reason: a wrong parse and a right one are
* output-identical to the caller.
*
* The anchor that does the work is the one at the START. A trailing remainder
* is allowed after the total because `Plan: 2 of 5 in current phase` is a real,
* tested shape this field has always carried — the suffix is a human note, not
* a second number. Prose is still refused, because the refusal comes from
* requiring `of <total>` to follow the leading number IMMEDIATELY: in
* `4 — blocked on review of 2 PRs` what follows `4` is ` — blocked`, so there
* is nothing for the total to be read from.
*
* BOTH grammars carry the same trailing tolerance, deliberately. An earlier
* revision anchored `N` hard (`/^(\d+)\s*$/`) while leaving `N of M` open,
* which hard-errored on values base parsed happily via `parseInt`:
* `Total Plans in Phase: 5 phases` and `Current Plan: 3 (blocked)`. #3784's own
* brief puts "validating or normalizing plan numbers beyond this transition's
* read/write" out of scope, so refusing an annotation nobody complained about
* was a narrowing this issue does not license. The prose defect is closed by
* the START anchor, not by forbidding suffixes: with a `Total Plans in Phase`
* sibling present the total never comes from the value's text at all, and
* without one `4 — blocked on review of 2 PRs` still fails `N of M` because
* ` — blocked` does not follow the leading number with `of`.
*
* CRLF survives, but state the mechanism accurately: `stateExtractField`'s own
* `(.+)` capture stops before the `\r` — JS `.` excludes CR as a
* LineTerminator — so the value these grammars receive is already CR-free on
* the common path. The trailing group is what covers the case where a CR does
* reach here, and `\s` matching CR is why it works. It is belt-and-braces, not
* the primary defence.
*/
/** The two body field names a plan position is ever written under. */
type PlanFieldName = 'Plan' | 'Current Plan';
const PLAN_SHAPE_N = /^(\d+)(?:\s.*)?$/;
const PLAN_SHAPE_N_OF_M = /^(\d+)\s+of\s+(\d+)(?:\s.*)?$/;
/**
* Parse a decimal group into a plan number, or `null` if it is not a value we
* are willing to do arithmetic on.
*
* `parseInt` is deliberately not used on the raw field: it truncates (`"2 of 5"`
* -> 2), accepts a sign (`"+2"`), and silently loses precision past
* `Number.MAX_SAFE_INTEGER`, where the number we report and the string we write
* back stop agreeing. The grammars above already exclude signs and trailing
* text, so the only remaining hazard is magnitude.
*/
function planNumberFrom(digits: string): number | null {
const n = Number(digits);
return Number.isSafeInteger(n) ? n : null;
}
/**
* Advance the leading integer of a written plan value, preserving everything
* the author wrote around it: the zero-padding width ("04" -> "05") and any
* trailing remainder ("2 of 99" -> "3 of 99", and the `\r` of a CRLF file).
*
* The three parse branches disagree about the field NAME and about whether a
* total is carried inline, but they agree completely about this: only the
* leading digits are the plan number, and nothing else on the line belongs to
* this transition. Writing `String(newPlan)` instead — as the legacy branch
* did — discards the author's text on a branch nobody was reading.
*
* padStart never truncates, so 09 -> 10 widens rather than clipping.
*/
function bumpLeadingNumber(raw: string, next: number): string | null {
const digits = /^\d+/.exec(raw);
// Total rather than pass-through. `raw.replace(/^\d+/, …)` returns the input
// unchanged when there are no leading digits, so `+2` advanced in `data` and
// wrote the file untouched — the command reported progress it had not made
// and could be re-run forever. The grammars make that unreachable today;
// returning null keeps it unreachable if a fourth branch is ever added.
if (!digits) return null;
return raw.replace(/^\d+/, () => String(next).padStart(digits[0].length, '0'));
}
/**
* Update fields within the ## Current Position section for advancePlan.
* Mirrors `updateCurrentPositionFields` (state.cts:496) byte-for-behaviour:
@@ -1301,7 +1388,15 @@ function mutateCurrentPositionResume(
*/
function mutateCurrentPositionForAdvance(
content: string,
fields: { phase?: string; status?: string; lastActivity?: string; plan?: string },
fields: {
phase?: string;
status?: string;
lastActivity?: string;
/** Value for a section line spelled `Plan:`. */
plan?: string;
/** Value for a section line spelled `Current Plan:`. */
currentPlan?: string;
},
statusDefaults: string[] | null | undefined,
lastActivityDefaults: string[] | null | undefined,
): string {
@@ -1338,15 +1433,72 @@ function mutateCurrentPositionForAdvance(
if (replaced !== null && replaced !== sectionBody) { sectionBody = replaced; mutated = true; }
}
if (fields.plan) {
if (fields.plan || fields.currentPlan) {
// Plan is always replaced — system-derived, not executor-authored.
if (/^Plan:/m.test(sectionBody)) {
sectionBody = sectionBody.replace(/^Plan:.*$/m, `Plan: ${fields.plan}`);
mutated = true;
} else {
const replaced = stateReplaceField(sectionBody, 'Plan', fields.plan);
if (replaced !== null) { sectionBody = replaced; mutated = true; }
}
//
// Which NAME to write is decided by what the SECTION carries, not by which
// header field the value was read from. Mirroring the header was wrong in
// both directions: a legacy header with a `Current Plan:` section line left
// the section a plan behind, and a hybrid header with a `Plan:` section line
// mutated nothing at all. The invariant is per-name — every site spelled
// `Current Plan` gets the `Current Plan` value, every `Plan` site gets the
// `Plan` value — so both are passed in and each is written where its own
// name appears.
//
// Title-Case LITERALS reach both the regex and stateReplaceField
// (ADR-3408 §8.3(b)): a literal cannot collide with a lowercase/snake_case
// frontmatter key, whatever the caller passed.
//
// The replacements go through a replacer FUNCTION, never a replacement
// string. `fields.plan` is derived from file content, and `String.replace`
// expands `$&`, `` $` `` and `$'` in a replacement string — a STATE.md
// carrying `Current Plan: 04 of 06 $&` would splice part of itself into the
// document. `stateReplaceField` already uses a function for this reason;
// these arms now agree with it.
// Each name is written INDEPENDENTLY, and each falls back on its own.
//
// Two defects lived in the previous shape, both of which produced the
// split-brain document this arm exists to prevent:
//
// - The fallback was guarded by `!mutated`, and `mutated` is FUNCTION-wide
// — already set by the `phase`/`status`/`lastActivity` arms above, which
// `advancePlanCore` always populates. A section spelled `**Current
// Plan:**` (bold) or as a pipe-table row therefore skipped its fallback
// because an UNRELATED field had been refreshed, and the section stayed a
// plan behind the header.
// - The fallback then picked ONE name by ternary. In the legacy shape both
// values are populated, so it always chose `Current Plan` and a
// `**Plan:**` section line — which base did write — got nothing.
//
// `planWritten` is local, so nothing outside this arm can satisfy its guard.
let planWritten = false;
const writePlanField = (name: PlanFieldName, value: string | undefined): void => {
if (!value) return;
// Plain `Name:` line first. Title-Case LITERALS reach both the regex and
// stateReplaceField (ADR-3408 §8.3(b)), and the replacement goes through a
// replacer FUNCTION so a `$&` / `` $` `` / `$'` in the author's text is not
// expanded into the document.
if (name === 'Current Plan') {
if (/^Current Plan:/m.test(sectionBody)) {
sectionBody = sectionBody.replace(/^Current Plan:.*$/m, () => `Current Plan: ${value}`);
planWritten = true;
return;
}
const replaced = stateReplaceField(sectionBody, 'Current Plan', value);
if (replaced !== null) { sectionBody = replaced; planWritten = true; }
return;
}
if (/^Plan:/m.test(sectionBody)) {
sectionBody = sectionBody.replace(/^Plan:.*$/m, () => `Plan: ${value}`);
planWritten = true;
return;
}
const replaced = stateReplaceField(sectionBody, 'Plan', value);
if (replaced !== null) { sectionBody = replaced; planWritten = true; }
};
writePlanField('Current Plan', fields.currentPlan);
writePlanField('Plan', fields.plan);
if (planWritten) mutated = true;
}
if (!mutated) return content;
@@ -1360,8 +1512,10 @@ function mutateCurrentPositionForAdvance(
/**
* Apply an `advancePlan` transition to STATE.md content.
*
* Parses Current Plan / Total Plans (legacy separate fields or compound
* "Plan: X of Y" format), increments the plan number, updates body fields
* Parses Current Plan / Total Plans in any of three shapes — the legacy
* separate fields, the compound "Plan: X of Y", or the hybrid
* "Current Plan: X of Y" (legacy name, compound value, no Total Plans
* sibling) — increments the plan number, updates body fields
* and the ## Current Position section. When currentPlan >= totalPlans,
* takes the phase-complete branch (sets Status to "Phase complete — ready
* for verification") instead of advancing.
@@ -1410,31 +1564,109 @@ function advancePlanCore(content: string, deps: StateTransitionDeps): StateTrans
}
}
// Parse plan number — legacy first, then compound.
// Parse plan number — legacy pair first, then the hybrid, then compound.
//
// These branches decide ONE thing: which numbers the advance is computed
// from. They deliberately do not record which FIELD supplied them, because
// the write path no longer asks — every spelling is written back from its own
// raw text (#3791 review round 6, B1/M1). An earlier revision tracked a
// `planSourceField`/`planRawValue` pair here and then wrote the OTHER
// spelling from this one's numbers, which is precisely how a field ended up
// holding a value nothing had derived for it.
const legacyPlan = stateExtractField(content, 'Current Plan');
const legacyTotal = stateExtractField(content, 'Total Plans in Phase');
const planField = stateExtractField(content, 'Plan');
let currentPlan: number;
let totalPlans: number;
let useCompoundFormat = false;
// Every branch below reads its numbers out of an ANCHORED match's capture
// groups. Nothing here calls parseInt on a raw field value, so a value the
// grammar does not fully describe cannot half-parse into a plausible number.
const legacyNMatch = legacyPlan ? PLAN_SHAPE_N.exec(legacyPlan) : null;
const legacyNofMMatch = legacyPlan ? PLAN_SHAPE_N_OF_M.exec(legacyPlan) : null;
const totalNMatch = legacyTotal ? PLAN_SHAPE_N.exec(legacyTotal) : null;
const planNofMMatch = planField ? PLAN_SHAPE_N_OF_M.exec(planField) : null;
const planNMatch = planField ? PLAN_SHAPE_N.exec(planField) : null;
if (legacyPlan && legacyTotal) {
currentPlan = parseInt(legacyPlan, 10);
totalPlans = parseInt(legacyTotal, 10);
} else if (planField) {
currentPlan = parseInt(planField, 10);
const ofMatch = planField.match(/of\s+(\d+)/);
totalPlans = ofMatch ? parseInt(ofMatch[1], 10) : NaN;
useCompoundFormat = true;
} else {
currentPlan = NaN;
totalPlans = NaN;
let parsedCurrent: number | null = null;
let parsedTotal: number | null = null;
if (legacyPlan && legacyTotal && (legacyNMatch || legacyNofMMatch) && totalNMatch) {
// Legacy pair wins whenever both fields are present and both are readable,
// even if the Current Plan value also carries an "of M" — the explicit
// sibling field is the stated intent, so it supplies the total.
parsedCurrent = planNumberFrom((legacyNMatch ?? legacyNofMMatch)![1]);
parsedTotal = planNumberFrom(totalNMatch[1]);
} else if (legacyNofMMatch) {
// Hybrid: legacy field name, compound value, no readable Total Plans
// sibling. Written by hand (and by agents) often enough to be worth
// reading — #3784.
parsedCurrent = planNumberFrom(legacyNofMMatch[1]);
parsedTotal = planNumberFrom(legacyNofMMatch[2]);
} else if (planNofMMatch) {
parsedCurrent = planNumberFrom(planNofMMatch[1]);
parsedTotal = planNumberFrom(planNofMMatch[2]);
}
// No branch for a bare `Plan: N` paired with a `Total Plans in Phase: M`
// sibling and no `Current Plan` at all (#3791 review round 6, M2). A revision
// of this PR accepted it; base did not (its `else if (planField)` arm had no
// `of M` match and errored via NaN), and it is out of #3784's scope, which is
// the hybrid `Current Plan: N of M`. It cannot be given the schema-row +
// forcing-test coupling the other shapes have, either: `Plan` is body-only,
// `buildStateFrontmatter` never reads it into frontmatter, so there is no
// `current_*` key to hang a row on. An accepted shape with no schema row and
// no forcing test is exactly the drift this diff is otherwise built to
// prevent, so the shape is refused and named in the error instead.
if (isNaN(currentPlan) || isNaN(totalPlans)) {
if (parsedCurrent === null || parsedTotal === null) {
return { content: reassemble(body), updated: [], data: { error: true } };
}
const currentPlan = parsedCurrent;
const totalPlans = parsedTotal;
// Each SPELLING's own plan number, read from its own value (#3791 review
// round 6, B1/M1). The parse above picks ONE field to advance FROM; these are
// what each field independently claims, and they are the only honest basis
// for writing that field back.
const legacyOwnCurrent = legacyNMatch || legacyNofMMatch
? planNumberFrom((legacyNMatch ?? legacyNofMMatch)![1])
: null;
const planOwnCurrent = planNofMMatch || planNMatch
? planNumberFrom((planNofMMatch ?? planNMatch)![1])
: null;
// A document carrying BOTH spellings with DIFFERENT plan numbers disagrees
// with itself, and no rule here can say which half is right. Refuse.
//
// This is the #3807 posture one field over: name the conflict, let the caller
// resolve it, never pick. The alternative shipped in an earlier revision of
// this PR and was the round-6 Blocker — with `Plan` as the parse source, the
// write path re-stamped `Current Plan`'s value with the number it had just
// derived from `Plan`, so `Current Plan: 7` beside `Plan: 2 of 5` silently
// became `Current Plan: 3`. A number with no relationship to the field it was
// written into, no error, no diagnostic.
//
// Placed BEFORE the phase-complete branch deliberately. Guarding only the
// normal advance leaves `Current Plan: 7` beside `Plan: 5 of 5` writing a
// terminal "Phase complete — ready for verification" into a document whose
// two spellings never agreed on where execution was.
//
// Differing TOTALS are NOT a disagreement about position and are preserved,
// not resolved: `Current Plan: 2` / `Total Plans in Phase: 5` beside
// `Plan: 2 of 9` advances to `3` and `3 of 9`. Reconciling the two totals
// would be this transition inventing an answer to a question nobody asked it.
if (legacyOwnCurrent !== null && planOwnCurrent !== null && legacyOwnCurrent !== planOwnCurrent) {
return {
content: reassemble(body),
updated: [],
data: {
error: true,
reason: 'ambiguous_plan_position',
plan_candidates: [
`Current Plan: ${legacyPlan}`,
`Plan: ${planField}`,
],
},
};
}
const updated: string[] = [];
@@ -1460,13 +1692,73 @@ function advancePlanCore(content: string, deps: StateTransitionDeps): StateTrans
// Normal advance branch.
const newPlan = currentPlan + 1;
let planDisplayValue: string;
if (useCompoundFormat) {
planDisplayValue = (planField as string).replace(/^\d+/, String(newPlan));
// The value each SPELLING should carry after the advance. A document may hold
// both names (a `Current Plan:` header and a `Plan:` line in the section, or
// the reverse), and each has always rendered differently — the legacy field
// holds a bare/padded number while the section's `Plan:` line holds the
// compound `N of M`.
//
// Each is advanced from ITS OWN raw text, never from the other's numbers
// (#3791 review round 6, B1/M1). `bumpLeadingNumber` replaces only the leading
// digits, so the field's zero-padding width, its own ` of M` and any trailing
// annotation all survive — which is what the changeset claims, and what the
// previous revision did only for whichever field happened to be the parse
// source. The other field it re-stamped from numbers that were never its own.
const advanceOwn = (raw: string | null, own: number | null): string | undefined => {
if (raw === null) return undefined;
// Present but unreadable (`Plan: TBD`). Leave it exactly as authored: this
// transition cannot advance what it cannot read, and writing a derived
// number over it is the fabrication B1 was filed for. Stale-and-untouched is
// honest; refusing the whole document because an unrelated line is
// unreadable would be a narrowing #3784 does not license.
if (own === null) return undefined;
return bumpLeadingNumber(raw, newPlan) ?? undefined;
};
// Title-Case LITERALS to stateReplaceField (ADR-3408 §8.3(b)): a literal
// cannot collide with a lowercase/snake_case frontmatter key, so it is safe
// regardless of how the content argument was derived. `body` here is in fact
// `stripFrontmatter(content)`, but the write-path drift guard does not do
// dataflow tracking (by design), and satisfying its invariant by construction
// is better than asking a reader to re-derive that it holds.
// One value per SPELLING, then write both names everywhere they appear.
//
// `Current Plan` — whatever the author wrote, advanced in place: padding
// and any ` of M` preserved.
// `Plan` — likewise, so its OWN total survives. `Plan: 2 of 9`
// beside a `Total Plans in Phase: 5` advances to
// `3 of 9`, not `3 of 5`: the two totals disagreeing is
// the document's business, not this transition's to
// reconcile.
//
// The two are deliberately different strings for the legacy shape, which is
// why this is a per-name value rather than one shared display value. Writing
// only the name the value was PARSED from is what left the other name stale:
// a `**Plan:** 2 of 6` header beside a `Current Plan:` line advanced one and
// not the other, in whichever direction the precedence happened to fall.
//
// Each write is a no-op when that name is absent (`stateReplaceField` returns
// null), so a document carrying only one spelling is unaffected — and
// `undefined` means "present but not advanceable", which is left untouched
// rather than overwritten.
const currentPlanDisplayValue = advanceOwn(legacyPlan, legacyOwnCurrent);
const planDisplayValue = planField === null
// No top-level `Plan:` field to advance, but the `## Current Position`
// section may still carry a `Plan:` line in a shape `stateExtractField`
// does not read. There is no raw text here to preserve, so it gets the
// compound rendering that line has always carried.
? `${newPlan} of ${totalPlans}`
: advanceOwn(planField, planOwnCurrent);
if (currentPlanDisplayValue !== undefined) {
body = stateReplaceField(body, 'Current Plan', currentPlanDisplayValue) || body;
}
// Only touch `Plan` when the document actually declares one. Writing it
// unconditionally meant a `stateReplaceField` whose first match could be any
// `Plan:` line anywhere in the body — including prose outside
// `## Current Position` that was never a field. `planField` is the read of
// that same field from the top of this function, so the write is scoped to a
// document that has one.
if (planField !== null && planDisplayValue !== undefined) {
body = stateReplaceField(body, 'Plan', planDisplayValue) || body;
} else {
planDisplayValue = `${newPlan} of ${totalPlans}`;
body = stateReplaceField(body, 'Current Plan', String(newPlan)) || body;
}
body = stateReplaceFieldIfTemplate(body, 'Status', statusDefaults, 'Ready to execute') || body;
body = stateReplaceFieldIfTemplate(body, 'Last Activity', lastActivityDefaults, today) || body;
@@ -1474,9 +1766,21 @@ function advancePlanCore(content: string, deps: StateTransitionDeps): StateTrans
body = mutateCurrentPositionForAdvance(body, {
status: 'Ready to execute',
lastActivity: today,
// Both spellings, each with its own value. The section writes whichever
// name it actually carries; passing only the header's name is what left
// two sites disagreeing about where execution is.
plan: planDisplayValue,
currentPlan: currentPlanDisplayValue,
}, statusDefaults, lastActivityDefaults);
updated.push('Current Plan', 'Status', 'Last Activity', 'Current Position');
// Report `Current Plan` only when it actually moved. The write above is
// conditional now — a `Current Plan:` that is present but unreadable is left
// as authored — so an unconditional push here would report progress this
// transition had not made, which is the same sin `bumpLeadingNumber` returns
// null to avoid. `reconcileReportedFields` at the `state.cts` caller would
// catch it against the persisted bytes, but `transitionCore`'s own `updated`
// is consumed directly too and has to be true on its own.
if (currentPlanDisplayValue !== undefined) updated.push('Current Plan');
updated.push('Status', 'Last Activity', 'Current Position');
return {
content: reassemble(body),

58
src/state.cts
View File

@@ -829,6 +829,45 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string |
// ─── State Progression Engine ────────────────────────────────────────────────
/**
* The "I could not read the plan position" message, DERIVED from
* `STATE_FIELD_SCHEMA.current_plan.acceptedShapes` rather than transcribed
* beside it.
*
* The accepted-shape set had two owners: the parser branches in
* `advancePlanCore` and an English list hand-written here. Nothing coupled
* them, so adding a branch left this message stale and removing one left it
* advertising a shape that errors — and no test could see either. ADR-3473
* §8.3 is "one implementation per rule"; the schema row is that one owner, and
* rows 23/24/25 already hold the parser to it.
*
* `Plan: N of M` is spelled out separately because there is no schema row for
* the body-only `Plan` field: `buildStateFrontmatter` never reads it into
* frontmatter, so it has no `current_*` key to hang a row on. That asymmetry is
* the schema's, not this function's.
*/
function advancePlanShapeError(): string {
const shapes = stateMdSchemaMod.STATE_FIELD_SCHEMA['current_plan']?.acceptedShapes ?? [];
const spellings = shapes.map((shape) => (
shape === 'N'
? '`Current Plan: N` with `Total Plans in Phase: M`'
: `\`Current Plan: ${shape}\``
));
// The body-only `Plan` field has no schema row to derive from:
// `buildStateFrontmatter` never reads it into frontmatter, so there is no
// `current_*` key to hang a row on. Its ONE accepted spelling is named here.
//
// `Plan: N` with a `Total Plans in Phase: M` sibling is deliberately NOT
// listed (#3791 review round 6, M2): the parser does not accept it. A
// revision of this PR added both the branch and this spelling together, on
// the reasoning that the message must advertise exactly what the parser
// accepts. That reasoning still holds — which is why removing the branch
// removes the spelling in the same commit. The invariant is the lockstep,
// not the length of the list.
spellings.push('`Plan: N of M`');
return `Cannot read the plan position from STATE.md. Expected one of: ${spellings.join(', ')}.`;
}
/**
* Replace a STATE.md field with fallback field name support.
* Tries `primary` first, then `fallback` (if provided), returns content unchanged
@@ -897,6 +936,11 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
return result.content;
}, cwd, { divergedFields, preWriteState });
// `!resultData` is a type guard, not a second failure mode: the callback
// above assigns it unconditionally and only runs once STATE.md is known to
// exist (the missing-file case returns "STATE.md not found" earlier), and
// every `advancePlanCore` return path sets `data`. So the message below is
// the one a caller can actually receive.
if (!resultData || resultData['error']) {
// #3807: a multi-`Phase:` Current Position section carries its own cause
// and its own remedy (name the candidates; the caller resolves them).
@@ -908,7 +952,19 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void {
}, raw, undefined);
return;
}
output({ error: 'Cannot parse Current Plan or Total Plans in Phase from STATE.md' }, raw, undefined);
// #3791 review round 6 (B1): the document carries both plan-position
// spellings with DIFFERENT numbers. Same posture as the case above — name
// the candidates and let the caller resolve them. Advancing either one
// would write a number into the other that nothing derived for it.
if (resultData && resultData['reason'] === 'ambiguous_plan_position') {
output({
error: 'STATE.md carries two plan positions with different numbers — refusing to advance either. Resolve them to a single current plan and re-run.',
reason: resultData['reason'],
plan_candidates: resultData['plan_candidates'],
}, raw, undefined);
return;
}
output({ error: advancePlanShapeError() }, raw, undefined);
return;
}