diff --git a/.changeset/lucky-tigers-gather.md b/.changeset/lucky-tigers-gather.md new file mode 100644 index 000000000..5238d2fc2 --- /dev/null +++ b/.changeset/lucky-tigers-gather.md @@ -0,0 +1,13 @@ +--- +type: Fixed +pr: 3791 +--- +**`state.advance-plan` now reads a plan position written as `Current Plan: N of M`, and advances every site that carries one instead of leaving the document disagreeing with itself.** The legacy field name carrying a compound value was accepted by neither parse branch, so the command reported a parse failure against a STATE.md whose plan numbers were plainly readable. + +The total is no longer read out of prose. Both accepted shapes are matched by a grammar anchored at the start of the value, and every number comes from a capture group. Previously an unanchored search for `of ` anywhere in the value made `Current Plan: 4 — blocked on review of 2 PRs` parse as "4 of 2", conclude the phase was over, and write `Status: Phase complete — ready for verification` into the file. A trailing annotation is still accepted on both shapes (`Plan: 2 of 5 in current phase`, `Total Plans in Phase: 5 phases`), and survives the write. + +Advancing rewrites only the leading digits, so the zero-padding width and everything after it survive: `04 of 06` advances to `05 of 06`, widening to `10 of 12` rather than truncating, and the legacy pair no longer collapses `2 of 99` into a bare `3` or `04` into `5`. That holds for **each** spelling independently — a `Plan: 2 of 9` line beside a `Total Plans in Phase: 5` advances to `3 of 9`, keeping its own total, because every field is advanced from its own text rather than re-stamped with the numbers some other field supplied. The `## Current Position` section advances alongside the header for every spelling — plain, bold and pipe-table — so the two can no longer report different plans. + +A document whose two plan positions carry **different numbers** — say `Current Plan: 7` beside `Plan: 2 of 5` — is now refused with `reason: "ambiguous_plan_position"` and both candidates named, rather than advancing one and silently stamping its number onto the other. A `Plan:` line that carries no readable number at all is left exactly as authored instead of being overwritten. When the position cannot be read at all, the error names the accepted shapes rather than asserting a cause it cannot know. + +Two narrowings against the old `parseInt` behaviour, both deliberate. A trailing annotation must be separated from the number by whitespace: `Total Plans in Phase: 5 phases` parses, `5phases` no longer does — `parseInt` read that as `5`, which is the half-parse this change exists to remove. And `Plan: N` paired with a `Total Plans in Phase: M` sibling and no `Current Plan` field is not an accepted shape; it was not accepted before this change either. diff --git a/docs/json-errors.md b/docs/json-errors.md index 33f41ff12..155ea7cda 100644 --- a/docs/json-errors.md +++ b/docs/json-errors.md @@ -151,7 +151,8 @@ fi 3. **Not every degraded result is an absent artifact.** A missing required argument is reported the same way — `gsd-tools state add-blocker` with no `--text` returns `{"error":"text required"}` and exits 0. So is unusable input: `gsd-tools state advance-plan` against a STATE.md it cannot parse - returns `{"error":"Cannot parse Current Plan or Total Plans in Phase from STATE.md"}`, also exit + returns an `{"error": …}` naming the plan-position shapes it accepts (the list is derived from + `STATE_FIELD_SCHEMA.current_plan.acceptedShapes`, so do not quote it verbatim), also exit 0. **The exit code does not distinguish absent from malformed from misinvoked** — see ADR-2980's Consequences, where this is recorded as a known cost. 4. **`message`/`error` text is not stable.** Assert on structure and on typed `reason` codes, never diff --git a/src/state-md-schema.cts b/src/state-md-schema.cts index ae336eeb0..3ea1306a0 100644 --- a/src/state-md-schema.cts +++ b/src/state-md-schema.cts @@ -186,19 +186,29 @@ export const STATE_FIELD_SCHEMA: Readonly> = 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, diff --git a/src/state-transition.cts b/src/state-transition.cts index 140e7f527..b64853f5d 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -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 ` 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), diff --git a/src/state.cts b/src/state.cts index a8e17ca72..aa53dab32 100644 --- a/src/state.cts +++ b/src/state.cts @@ -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; } diff --git a/tests/advance-plan-ambiguous-phase.test.cjs b/tests/advance-plan-ambiguous-phase.test.cjs index 907ba3eb5..03b44e2c6 100644 --- a/tests/advance-plan-ambiguous-phase.test.cjs +++ b/tests/advance-plan-ambiguous-phase.test.cjs @@ -97,4 +97,71 @@ describe('#3807: advance-plan refuses an ambiguous multi-entry Current Position' const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); assert.match(after, /Plan: 4 of 8/, 'the plan counter advanced'); }); + + // ─────────────────────────────────────────────────────────────────────────── + // #3784 x #3807 interaction. #3784 taught advancePlanCore a third value + // shape — the hybrid `Current Plan: N of M` (legacy field name, compound + // value, no `Total Plans in Phase` sibling). Both changes land on the same + // function, and the guard sits ABOVE the parse, so a document the guard + // refuses is never parsed at all. That ordering is the whole answer to + // "does the widened grammar bypass the refusal" — but ordering is a + // property of the source, and these two assert it as behaviour. + // + // Fail-first proven, not assumed: with the `phaseCandidates.length > 1` + // refusal disabled, the multi-entry case below advances the FIRST entry's + // `Current Plan: 04 of 06` to `05 of 06` and writes it — #3807's exact + // defect, reached through the shape #3784 added. + // ─────────────────────────────────────────────────────────────────────────── + + const HYBRID_ENTRY = (phase, plan) => [ + `Phase: ${phase}`, + `Current Plan: ${plan}`, + 'Status: In progress', + 'Last activity: 2026-08-24 — working', + '', + ]; + + test('#3784 x #3807: the hybrid `Current Plan: N of M` shape does not bypass the refusal', (t) => { + const tmpDir = createTempProject('gsd-3807-hybrid-amb-'); + t.after(() => cleanup(tmpDir)); + writeState(tmpDir, [ + '## Current Position', + '', + ...HYBRID_ENTRY('03.1 of 8 (some-phase)', '04 of 06'), + ...HYBRID_ENTRY('04 of 15 (other-phase)', '04 of 15'), + ].join('\n')); + const before = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + + const r = runAdvance(tmpDir); + const out = JSON.parse(r.output); + assert.equal( + out.reason, + 'ambiguous_position_phase', + `#3807's refusal must fire on the hybrid shape too, not #3784's parse; got ${r.output}`, + ); + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + assert.equal(after, before, 'refusing must leave STATE.md byte-identical on the hybrid shape'); + assert.ok( + !/Current Plan: 05 of 06/.test(after), + "the first entry's hybrid plan counter must NOT advance", + ); + }); + + test('#3784 x #3807 control: a single-entry hybrid section still advances, padding intact', (t) => { + const tmpDir = createTempProject('gsd-3807-hybrid-ctl-'); + t.after(() => cleanup(tmpDir)); + writeState(tmpDir, [ + '## Current Position', + '', + ...HYBRID_ENTRY('03.1 of 8 (some-phase)', '04 of 06'), + ].join('\n')); + + const r = runAdvance(tmpDir); + const out = JSON.parse(r.output); + assert.equal(out.advanced, true, `the hybrid shape still advances when unambiguous; got ${r.output}`); + assert.equal(out.current_plan, 5, '#3784: the hybrid value supplies the plan number'); + assert.equal(out.total_plans, 6, '#3784: the hybrid value supplies the total, with no sibling field'); + const after = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf8'); + assert.match(after, /Current Plan: 05 of 06/, '#3784: zero-padding survives the advance'); + }); }); diff --git a/tests/io.test.cjs b/tests/io.test.cjs index 52e8c5094..bda320b45 100644 --- a/tests/io.test.cjs +++ b/tests/io.test.cjs @@ -841,11 +841,11 @@ describe('#3912 A3-A5: output({error}) records DEGRADED — shape-exhaustive plu perFile, { 'commands.cts': 5, 'frontmatter.cts': 7, 'gsd2-import.cts': 2, 'phase.cts': 4, - 'roadmap.cts': 3, 'state.cts': 26, 'template.cts': 3, 'verify.cts': 8, 'workstream.cts': 7, // +1 #3807: advance-plan's ambiguous-position error + 'roadmap.cts': 3, 'state.cts': 27, 'template.cts': 3, 'verify.cts': 8, 'workstream.cts': 7, // +1 #3807: advance-plan's ambiguous-position error; +1 #3784: advance-plan's ambiguous-PLAN-position error (two plan spellings, different numbers) }, `per-file output({error}) census drifted: ${JSON.stringify(perFile)}`, ); - assert.strictEqual(total, 65, `enumerated output({error}) population drifted from the measured 65 (64 + #3807's ambiguous-position error): got ${total}`); + assert.strictEqual(total, 66, `enumerated output({error}) population drifted from the measured 66 (64 + #3807's ambiguous-position error + #3784's ambiguous-plan-position error): got ${total}`); }); }); diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index d9c3cf37c..c2ac1dcc7 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -578,7 +578,11 @@ describe('ADR-1769 Phase 2: advancePlan transition', () => { '', ].join('\n'); const result = transitionCore(input, { kind: 'advancePlan' }, deps); - assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '3'); + // #3784: was '3'. The dropped zero-padding this used to pin is the defect + // the issue reports, not behaviour worth preserving — a fixture written + // "02" must not come back "3". The characterization is updated rather than + // worked around, because the old value WAS the bug. + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '03'); assert.strictEqual(result.data && result.data.advanced, true); assert.strictEqual(result.data && result.data.current_plan, 3); assert.strictEqual(result.data && result.data.total_plans, 5); @@ -607,6 +611,488 @@ describe('ADR-1769 Phase 2: advancePlan transition', () => { assert.deepStrictEqual(result.updated, []); }); + // Hybrid shape: the legacy field NAME carrying the compound VALUE, with no + // `Total Plans in Phase` sibling. Neither documented branch handled it — + // `legacyTotal` is null so the legacy branch fell through, and the compound + // branch reads the `Plan` field through a `^Plan:` line-anchored pattern + // that never matches `Current Plan:`. Both produced NaN, and the caller + // reported a parse failure against a file whose plan numbers are plainly + // readable. + // + // Not hypothetical: an agent wrote this exact shape unprompted, believing + // it was the parseable form, and every later run inherited it. + test('hybrid format: "Current Plan: 4 of 6" with no Total Plans sibling', () => { + const input = [ + '# Project State', + '', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + '## Current Position', + '', + 'Current Plan: 4 of 6', + 'Status: Executing Phase 7', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(result.data && result.data.error, undefined); + assert.strictEqual(result.data && result.data.advanced, true); + assert.strictEqual(result.data && result.data.current_plan, 5); + assert.strictEqual(result.data && result.data.total_plans, 6); + }); + + // AC1: the hybrid must write back to the SAME field with padding preserved, + // not merely report the right numbers in `data`. + test('hybrid format: writes back to Current Plan with padding preserved', () => { + const input = [ + '# Project State', + '', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + '## Current Position', + '', + 'Current Plan: 04 of 06', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '05 of 06'); + // Identity, not a presence proxy: assert the whole section body, so a + // spurious extra field or a dropped line is visible rather than merely + // "no line starting with Plan:". + const section = result.content.slice(result.content.indexOf('## Current Position')); + assert.strictEqual(section.trimEnd(), ['## Current Position', '', 'Current Plan: 05 of 06'].join('\n')); + }); + + test('hybrid format: phase-complete branch still fires on the last plan', () => { + const input = [ + '# Project State', + '', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + '## Current Position', + '', + 'Current Plan: 6 of 6', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(result.data && result.data.advanced, false); + assert.strictEqual(result.data && result.data.reason, 'last_plan'); + }); + + test('compound format preserves zero-padding on both halves', () => { + const input = [ + '# Project State', + '', + '**Plan:** 04 of 06', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(stateExtractField(result.content, 'Plan'), '05 of 06'); + }); + + test('padding widens rather than truncates when the plan number grows', () => { + const input = [ + '# Project State', + '', + '**Plan:** 09 of 12', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(stateExtractField(result.content, 'Plan'), '10 of 12'); + }); + + test('unpadded compound stays unpadded', () => { + const input = [ + '# Project State', + '', + '**Plan:** 2 of 6', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(stateExtractField(result.content, 'Plan'), '3 of 6'); + }); + + // AC6: the shared field reader must NOT be loosened to make the hybrid work. + // Reading the hybrid is the transition's job; `stateExtractField('Plan')` is + // line-anchored (`^Plan:`) and has 13+ callers, so teaching it to match a + // field name that merely ENDS in "Plan" would be the wrong fix and would + // silently change what those callers read. This test fails if anyone tries it. + test('shared reader stays anchored: "Plan" does not match "Current Plan:"', () => { + const content = [ + '# Project State', + '', + '## Current Position', + '', + 'Current Plan: 04 of 06', + '', + ].join('\n'); + assert.strictEqual(stateExtractField(content, 'Plan'), null); + assert.strictEqual(stateExtractField(content, 'Current Plan'), '04 of 06'); + }); + + // The legacy pair must keep winning when both are present: a stray "of N" + // inside the Current Plan value must not override an explicit Total Plans. + test('legacy pair still takes precedence over an "of N" in Current Plan', () => { + const input = [ + '# Project State', + '', + '**Current Plan:** 2 of 99', + '**Total Plans in Phase:** 5', + '**Status:** Executing Phase 3', + '**Last Activity:** 2026-06-26', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(result.data && result.data.advanced, true); + assert.strictEqual(result.data && result.data.total_plans, 5); + // Assert the WRITE, not just the parse. Reading `data` alone cannot see a + // lossy write-back, and a regression test that cannot observe the + // regression is not coverage. The legacy branch used to write + // `String(newPlan)`, which turned "2 of 99" into a bare "3" — silently + // destroying the reader's own text on a branch nobody was looking at. + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '3 of 99'); + assert.strictEqual(stateExtractField(result.content, 'Total Plans in Phase'), '5'); + }); + + test('legacy pair preserves zero-padding on write-back', () => { + const input = [ + '# Project State', + '', + '**Current Plan:** 04', + '**Total Plans in Phase:** 06', + '**Status:** Executing Phase 3', + '**Last Activity:** 2026-06-26', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '05'); + }); + + // Findings 2+3 are one defect seen twice: the body-level write is single-shot + // and bold-preferring, so on a file carrying the field at BOTH the bold header + // and the `## Current Position` line it updates the header only — and + // `mutateCurrentPositionForAdvance` could not pick up the slack because its + // plan arm only ever looked for `Plan:`, never `Current Plan:`. The earlier + // hybrid write-back test passed only because its fixture was single-site. + test('hybrid format: header and Current Position both advance, no drift', () => { + const input = [ + '# Project State', + '', + '**Current Plan:** 04 of 06', + '**Status:** Executing Phase 7', + '**Last Activity:** 2026-06-26', + '', + '## Current Position', + '', + 'Current Plan: 04 of 06', + 'Status: Executing Phase 7', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + const advanced = result.content.match(/05 of 06/g) || []; + assert.strictEqual(advanced.length, 2, 'both sites must advance'); + assert.ok(!/04 of 06/.test(result.content), 'no site may be left behind'); + }); + + // Review round 4, Blocker 1. The section fallback was guarded by the + // FUNCTION-wide `mutated`, which the status/lastActivity arms had already set. + // The discriminating shape needs BOTH a header `Status:` (to absorb the + // body-level status write, so the section's own status is still a template + // default when the section arm runs) AND a bold section plan line (so the + // plain-line arm misses and only the fallback can write it). Without the + // header Status this passes even on the broken build. + test('B1: a bold section plan line advances when an unrelated field was also refreshed', () => { + const input = [ + '# Project State', + '', + '**Current Plan:** 04 of 06', + '**Status:** Ready to plan', + '', + '## Current Position', + '', + '**Current Plan:** 01 of 06', + 'Status: Ready to plan', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + const section = result.content.slice(result.content.indexOf('## Current Position')); + assert.match(section, /\*\*Current Plan:\*\* 05 of 06/, 'the bold section line must advance'); + assert.ok(!/01 of 06/.test(result.content), 'no site may be left behind'); + }); + + // Review round 4, Blocker 2. In the legacy shape BOTH plan values are + // populated, so a fallback that picked one name by ternary always chose + // `Current Plan` and never wrote a `**Plan:**` section line — which base did + // write. The header `**Plan:**` absorbs the body-level write, so only the + // section arm can advance the section copy. + test('B2: a bold **Plan:** section line advances in the legacy pair shape', () => { + const input = [ + '# Project State', + '', + '**Current Plan:** 3', + '**Total Plans in Phase:** 5', + '**Plan:** 3 of 5', + '**Status:** Ready to plan', + '', + '## Current Position', + '', + '**Plan:** 3 of 5', + 'Status: Ready to plan', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + const section = result.content.slice(result.content.indexOf('## Current Position')); + assert.match(section, /\*\*Plan:\*\* 4 of 5/, 'the section Plan line must advance'); + assert.ok(!/3 of 5/.test(result.content), 'no site may be left behind'); + }); + + // Review round 4, Blocker 3. `PLAN_SHAPE_N` was anchored harder than + // `PLAN_SHAPE_N_OF_M`, so annotated values base parsed via `parseInt` began to + // hard-error. Narrowing what the transition ACCEPTS is out of scope for #3784. + test('B3: annotated legacy values still parse, and keep their annotation', () => { + const drive = (plan, total) => transitionCore([ + '# Project State', '', + `**Current Plan:** ${plan}`, + `**Total Plans in Phase:** ${total}`, + '**Status:** Executing', '', + ].join('\n'), { kind: 'advancePlan' }, deps); + + const annotatedTotal = drive('3', '5 phases'); + assert.strictEqual(annotatedTotal.data.advanced, true, '"5 phases" must still supply a total'); + assert.strictEqual(annotatedTotal.data.total_plans, 5); + + const annotatedPlan = drive('3 (blocked)', '5'); + assert.strictEqual(annotatedPlan.data.advanced, true, '"3 (blocked)" must still advance'); + assert.strictEqual( + stateExtractField(annotatedPlan.content, 'Current Plan'), '4 (blocked)', + 'the annotation is the author\'s text and survives the advance', + ); + + // The prose case stays refused: the START anchor is what closes it, not the + // absence of a suffix. + const prose = transitionCore([ + '# Project State', '', '**Current Plan:** 4 — blocked on review of 2 PRs', + '**Status:** Executing', '', + ].join('\n'), { kind: 'advancePlan' }, deps); + assert.strictEqual(prose.data.error, true, 'a total must never be read out of prose'); + }); + + // Minor 1: the Number.isSafeInteger bound this PR introduces. + test('boundary: the safe-integer limit', () => { + const drive = (v) => transitionCore([ + '# Project State', '', `**Current Plan:** ${v}`, '**Status:** Executing', '', + ].join('\n'), { kind: 'advancePlan' }, deps).data; + const MAX = Number.MAX_SAFE_INTEGER; // 9007199254740991 + assert.strictEqual(drive(`${MAX - 1} of ${MAX}`).advanced, true, 'limit-1 advances'); + assert.strictEqual(drive(`${MAX} of ${MAX}`).reason, 'last_plan', 'limit itself is readable'); + assert.strictEqual(drive(`${MAX} of 9007199254740992`).error, true, 'limit+1 is refused, not rounded'); + }); + + // Boundary coverage (RULESET.TESTS.boundary-coverage) around the + // `currentPlan >= totalPlans` limit, on the newly readable hybrid shape. + // limit itself ("6 of 6") is covered by the phase-complete test above. + test('hybrid boundary: limit-1 advances', () => { + const input = [ + '# Project State', + '', + '## Current Position', + '', + 'Current Plan: 5 of 6', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.strictEqual(result.data && result.data.advanced, true); + assert.strictEqual(result.data && result.data.current_plan, 6); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '6 of 6'); + }); + + test('hybrid boundary: limit+1 takes the phase-complete branch', () => { + const input = [ + '# Project State', + '', + '## Current Position', + '', + 'Current Plan: 7 of 6', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + // Mirrors the compound branch's existing `>= totalPlans` semantics — an + // over-limit value is past the end, not a new advance. + assert.strictEqual(result.data && result.data.advanced, false); + assert.strictEqual(result.data && result.data.reason, 'last_plan'); + }); + + // CRLF: regexes matching only \n are a recurring defect class here, and the + // write path's `(.*)` capture eats a trailing \r. A file that arrives CRLF + // must not leave with one line silently converted to LF. + test('hybrid format: CRLF line endings survive the write-back', () => { + const input = [ + '# Project State', + '', + '## Current Position', + '', + 'Current Plan: 04 of 06', + 'Status: Executing Phase 7', + '', + ].join('\r\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + assert.ok(/Current Plan: 05 of 06\r\n/.test(result.content), + 'the advanced line must keep its CRLF terminator'); + assert.ok(!/(^|[^\r])\n/.test(result.content), 'no line may be downgraded to bare LF'); + }); + + // RULESET.TESTS.property-based-testing: this is a parsing/transformation with + // a format-preserving contract, so the contract gets a property, not just + // examples. Contract: advancing rewrites ONLY the leading integer, pads it to + // at least the original digit width, and leaves the rest of the value byte- + // identical. + // Drives BOTH compound spellings — `Plan:` and the hybrid `Current Plan:` + // this PR adds — because a property that only exercises the pre-existing + // branch says nothing about the branch under review. `n` ranges past 99 so + // the 99 -> 100 width transition is covered by the property rather than by a + // single example. + test('property: advancing preserves padding width and the " of M" remainder', () => { + fc.assert( + fc.property( + fc.integer({ min: 1, max: 150 }), + fc.integer({ min: 1, max: 4 }), + fc.integer({ min: 1, max: 4 }), + fc.constantFrom('Plan', 'Current Plan'), + (n, planWidth, totalWidth, fieldName) => { + const total = n + 2; // strictly greater, so the advance branch is taken + const planStr = String(n).padStart(planWidth, '0'); + const totalStr = String(total).padStart(totalWidth, '0'); + const input = [ + '# Project State', + '', + `**${fieldName}:** ${planStr} of ${totalStr}`, + '**Status:** Executing Phase 7', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + const expected = `${String(n + 1).padStart(planStr.length, '0')} of ${totalStr}`; + assert.strictEqual(stateExtractField(result.content, fieldName), expected); + }, + ), + { numRuns: 200 }, + ); + }); + + // The legacy pair's own format-preserving contract: the ` of M` remainder and + // the padding survive there too, and the sibling supplies the total. + test('property: the legacy pair preserves the written Current Plan value shape', () => { + fc.assert( + fc.property( + fc.integer({ min: 1, max: 150 }), + fc.integer({ min: 1, max: 4 }), + fc.option(fc.integer({ min: 1, max: 999 }), { nil: null }), + (n, planWidth, inlineTotal) => { + const total = n + 2; + const planStr = String(n).padStart(planWidth, '0'); + const written = inlineTotal === null ? planStr : `${planStr} of ${inlineTotal}`; + const input = [ + '# Project State', + '', + `**Current Plan:** ${written}`, + `**Total Plans in Phase:** ${total}`, + '**Status:** Executing Phase 7', + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + const bumped = String(n + 1).padStart(planStr.length, '0'); + const expected = inlineTotal === null ? bumped : `${bumped} of ${inlineTotal}`; + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), expected); + }, + ), + { numRuns: 200 }, + ); + }); + + // #3791 review round 6: the two properties above each exercise ONE spelling, + // so neither could see a document carrying both — which is where B1 and M1 + // both lived. This one crosses the two contracts, with agreeing and + // disagreeing numbers, and asserts the per-spelling contract on each half. + test('property: two spellings — each keeps its own shape when they agree, and neither moves when they do not', () => { + fc.assert( + fc.property( + fc.integer({ min: 1, max: 150 }), + fc.integer({ min: 1, max: 4 }), + fc.integer({ min: 1, max: 4 }), + // The `Plan:` line's OWN total, independent of the sibling field. + fc.integer({ min: 200, max: 400 }), + // 0 = the two spellings agree; anything else is the offset that makes + // them disagree. + fc.integer({ min: 0, max: 9 }), + (n, legacyWidth, planWidth, planOwnTotal, disagreeBy) => { + const siblingTotal = n + 2; // strictly greater, so the advance branch is taken + const legacyStr = String(n).padStart(legacyWidth, '0'); + const planN = n + disagreeBy; + const planStr = String(planN).padStart(planWidth, '0'); + const input = [ + '# Project State', + '', + `**Current Plan:** ${legacyStr}`, + `**Total Plans in Phase:** ${siblingTotal}`, + '**Status:** Executing Phase 7', + '', + '## Current Position', + '', + `Plan: ${planStr} of ${planOwnTotal}`, + '', + ].join('\n'); + const result = transitionCore(input, { kind: 'advancePlan' }, deps); + + if (disagreeBy !== 0) { + // Refused, and nothing written — not one field, not the status. + assert.strictEqual(result.data && result.data.error, true); + assert.strictEqual(result.data && result.data.reason, 'ambiguous_plan_position'); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), legacyStr); + assert.strictEqual(stateExtractField(result.content, 'Plan'), `${planStr} of ${planOwnTotal}`); + return; + } + + // They agree: both advance, and each keeps its OWN written shape — + // the legacy field its padding and bare form, the `Plan:` line its + // padding AND its own total, which is never the sibling's. + assert.strictEqual(result.data && result.data.advanced, true); + assert.strictEqual( + stateExtractField(result.content, 'Current Plan'), + String(n + 1).padStart(legacyStr.length, '0'), + ); + assert.strictEqual( + stateExtractField(result.content, 'Plan'), + `${String(n + 1).padStart(planStr.length, '0')} of ${planOwnTotal}`, + ); + }, + ), + { numRuns: 300 }, + ); + }); + + // m2: the degenerate end of the totalPlans threshold, and the shapes the + // anchored grammar must refuse. `0 of 0` is `currentPlan >= totalPlans`, so + // it is phase-complete rather than an error — pinned so the grammar + // tightening cannot silently reclassify it. + test('boundary: degenerate and malformed plan values', () => { + const drive = (value) => { + const input = ['# Project State', '', `**Current Plan:** ${value}`, '**Status:** Executing', ''].join('\n'); + return transitionCore(input, { kind: 'advancePlan' }, deps).data; + }; + assert.strictEqual(drive('0 of 0').reason, 'last_plan', '"0 of 0" is past the end, not an error'); + assert.strictEqual(drive('0 of 3').advanced, true, '"0 of 3" advances to 1'); + for (const bad of ['-1 of 6', '+2 of 6', '\u0663 of \u0665', '3 of', 'of 5', 'x of 5']) { + assert.strictEqual(drive(bad).error, true, `${JSON.stringify(bad)} must be refused`); + } + }); + test('compound format: "Plan: 2 of 6" preserves compound shape', () => { const input = [ '# Project State', @@ -646,6 +1132,201 @@ describe('ADR-1769 Phase 2: advancePlan transition', () => { // pins today's reality, and it is EXPECTED to go RED the moment the parser // changes underneath it. That is the forcing function working as designed // (§8.8 "checked, not generated"), not a broken test. +describe('#3791 review round 6 (B1/M1): every spelling advances from its own text', () => { + const deps = { clock: fixedClock }; + const advance = (lines) => transitionCore(lines.join('\n'), { kind: 'advancePlan' }, deps); + + // ─── M1: a field keeps its OWN total, padding and annotation ─────────────── + // + // The legacy pair wins precedence, but that only decides which numbers the + // ADVANCE is computed from. It does not license re-rendering the `Plan:` line + // from the legacy pair's numbers, which is what the previous revision did: + // `planDisplayValue` fell back to a bare `${newPlan} of ${totalPlans}` built + // from the sibling field. + + test('the Plan line keeps its own total when it differs from the sibling field', () => { + const result = advance([ + '# Project State', + '', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '**Status:** Executing', + '', + '## Current Position', + '', + 'Plan: 2 of 9', + 'Status: Executing', + '', + ]); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '3'); + // Was '3 of 5' — the sibling's total silently overwrote the line's own. + assert.strictEqual(stateExtractField(result.content, 'Plan'), '3 of 9'); + }); + + test('the Plan line keeps its own zero-padding when the legacy pair supplies the numbers', () => { + const result = advance([ + '# Project State', + '', + '**Current Plan:** 03', + '**Total Plans in Phase:** 05', + '**Status:** Executing', + '', + '## Current Position', + '', + 'Plan: 03 of 05', + 'Status: Executing', + '', + ]); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '04'); + // Was '4 of 5' — the changeset's "the zero-padding width and everything + // after it survive" was true only for the parse-source field. + assert.strictEqual(stateExtractField(result.content, 'Plan'), '04 of 05'); + }); + + test('a trailing annotation on the Plan line survives an advance driven by the legacy pair', () => { + const result = advance([ + '# Project State', + '', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '', + '## Current Position', + '', + 'Plan: 2 of 5 in current phase', + '', + ]); + assert.strictEqual(stateExtractField(result.content, 'Plan'), '3 of 5 in current phase'); + }); + + // ─── B1: two spellings, different numbers — refuse, never fabricate ──────── + + test('refuses when Current Plan and Plan carry different numbers', () => { + const result = advance([ + '# Project State', + '', + '**Current Plan:** 7', + '**Status:** Executing', + '', + '## Current Position', + '', + 'Plan: 2 of 5', + '', + ]); + assert.strictEqual(result.data && result.data.error, true); + assert.strictEqual(result.data && result.data.reason, 'ambiguous_plan_position'); + assert.deepStrictEqual(result.data && result.data.plan_candidates, + ['Current Plan: 7', 'Plan: 2 of 5']); + // Nothing is written. The previous revision advanced `Plan` to `3 of 5` and + // stamped `Current Plan: 3` — a number with no relationship to the 7 the + // author wrote. + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '7'); + assert.strictEqual(stateExtractField(result.content, 'Plan'), '2 of 5'); + }); + + test('the disagreement refusal runs BEFORE the phase-complete branch', () => { + // `Plan: 5 of 5` alone would advance=false / last_plan and write a terminal + // "Phase complete — ready for verification". Guarding only the normal + // advance path would let it do that to a document whose two spellings never + // agreed on where execution was. + const result = advance([ + '# Project State', + '', + '**Current Plan:** 7', + '**Status:** Executing', + '', + '## Current Position', + '', + 'Plan: 5 of 5', + '', + ]); + assert.strictEqual(result.data && result.data.error, true); + assert.strictEqual(result.data && result.data.reason, 'ambiguous_plan_position'); + assert.ok(!/Phase complete/.test(result.content), + 'a disagreeing document must not be marked phase-complete'); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '7'); + }); + + test('agreeing spellings are not a disagreement, whatever their totals say', () => { + const result = advance([ + '# Project State', + '', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '', + '## Current Position', + '', + 'Plan: 2 of 9', + '', + ]); + assert.strictEqual(result.data && result.data.error, undefined); + assert.strictEqual(result.data && result.data.advanced, true); + }); + + // ─── An unreadable sibling is left alone, not overwritten ────────────────── + + test('a Plan line with no readable number is left exactly as authored', () => { + const result = advance([ + '# Project State', + '', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '', + '## Current Position', + '', + 'Plan: TBD', + '', + ]); + // The advance still happens — refusing the whole document because an + // unrelated line is unreadable would be a narrowing #3784 does not license. + assert.strictEqual(result.data && result.data.advanced, true); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '3'); + // ...and the unreadable line is untouched rather than fabricated over. + assert.strictEqual(stateExtractField(result.content, 'Plan'), 'TBD'); + }); + + test('a Current Plan with no readable number is left alone, and not reported as updated', () => { + // The mirror of the case above: `Plan` is the parse source and the legacy + // field is the unreadable one. It exercises the other arm of the same skip, + // and it is the fixture where an unconditional `updated.push('Current Plan')` + // would claim a write that never happened. + const result = advance([ + '# Project State', + '', + '**Current Plan:** TBD', + '**Status:** Executing', + '', + '## Current Position', + '', + 'Plan: 2 of 5', + '', + ]); + assert.strictEqual(result.data && result.data.advanced, true); + assert.strictEqual(stateExtractField(result.content, 'Plan'), '3 of 5'); + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), 'TBD'); + assert.ok(!result.updated.includes('Current Plan'), + 'must not report a field it did not write'); + assert.ok(result.updated.includes('Status'), 'the fields it did write are still reported'); + }); + + // ─── M2: the bare `Plan: N` + sibling shape is not accepted ──────────────── + + test('a bare Plan: N with a Total sibling and no Current Plan is refused', () => { + const result = advance([ + '# Project State', + '', + '**Plan:** 2', + '**Total Plans in Phase:** 5', + '', + ]); + // Base refused this (its `else if (planField)` arm had no `of M` match and + // errored via NaN). A revision of this PR accepted it; #3791 review round 6 + // (M2) is that it cannot be given the schema-row + forcing-test coupling + // the other shapes have, because `Plan` is body-only. + assert.strictEqual(result.data && result.data.error, true); + assert.strictEqual(result.data && result.data.reason, undefined); + }); +}); + describe('#3873 phase-3 rows 23/24/25: parser accepts exactly the schema-declared shapes', () => { const deps = { clock: fixedClock }; @@ -716,17 +1397,34 @@ describe('#3873 phase-3 rows 23/24/25: parser accepts exactly the schema-declare // The worked case (#3784's three spellings): current_plan specifically. test('planNofMShapesAreExactlyTheDeclaredSet', () => { - assert.deepStrictEqual(Array.from(STATE_FIELD_SCHEMA.current_plan.acceptedShapes), ['N']); + // #3791 widened this row, exactly as the schema's own comment instructed. + assert.deepStrictEqual(Array.from(STATE_FIELD_SCHEMA.current_plan.acceptedShapes), ['N', 'N of M']); // Declared shape parses. assert.strictEqual(driveCurrentPlanShape('3', { withTotalSibling: true }), true, '"N" (paired with Total Plans in Phase) should parse'); - // The hybrid shape is NOT declared today (#3784/#3791 boundary) and does - // NOT parse standalone in the Current Plan field. - assert.strictEqual(driveCurrentPlanShape('3 of 5'), false, '"N of M" standalone in Current Plan should NOT parse today'); + // The hybrid shape is now declared AND parses standalone — #3784. + assert.strictEqual(driveCurrentPlanShape('3 of 5'), true, '"N of M" standalone in Current Plan should parse'); // A fourth, never-declared spelling fails rather than quietly joining. assert.strictEqual(driveCurrentPlanShape('3/5'), false, '"N/M" should NOT parse — it has never been declared'); + + // Declaring "N of M" is a claim about an ANCHORED grammar, not about the + // substring "of". Prose that merely contains it must still be refused — + // otherwise `4 — blocked on review of 2 PRs` reads as "4 of 2", which is + // `currentPlan >= totalPlans` and WRITES a terminal phase-complete status. + for (const prose of [ + '4 — blocked on review of 2 PRs', + '3 (waiting for refactor of 1 module)', + '2roof 5', + '+2 of 6', + '-1 of 6', + ]) { + assert.strictEqual( + driveCurrentPlanShape(prose), false, + `${JSON.stringify(prose)} must NOT parse — the declared shape is anchored`, + ); + } }); }); @@ -754,8 +1452,8 @@ describe('ADR-1769 Phase 2: advancePlan with frontmatter (#1255 pattern — code '', ].join('\n'); const result = transitionCore(input, { kind: 'advancePlan' }, deps); - // Body Current Plan must advance to 3. - assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '3'); + // Body Current Plan must advance to 3, keeping the written width (#3784). + assert.strictEqual(stateExtractField(result.content, 'Current Plan'), '03'); // Body Status must be updated (not the YAML status key). const bodyStatus = stateExtractField(result.content, 'Status'); assert.ok( diff --git a/tests/state.test.cjs b/tests/state.test.cjs index fc9b4d87c..c42c41c08 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -1912,7 +1912,40 @@ describe('cmdStateAdvancePlan (state advance-plan)', () => { const output = JSON.parse(result.output); assert.ok(output.error !== undefined, 'output should have error field'); - assert.ok(output.error.toLowerCase().includes('cannot parse'), 'error should mention Cannot parse'); + // Assert on what makes the message actionable, not on one literal phrase: + // it must say the plan position could not be read AND name the shapes that + // would work. The previous assertion only checked for "cannot parse", which + // a message can satisfy while leaving the reader no idea what to write. + assert.ok( + /cannot read the plan position/i.test(output.error), + `error should say the plan position could not be read; got: ${output.error}`, + ); + // Coupling, not transcription: the message is DERIVED from + // `STATE_FIELD_SCHEMA.current_plan.acceptedShapes`, so this walks the + // schema rather than restating a list beside it. Widening the schema + // without widening the message (or vice versa) goes red here. + const { STATE_FIELD_SCHEMA } = require('../gsd-core/bin/lib/state-md-schema.cjs'); + const declared = STATE_FIELD_SCHEMA['current_plan'].acceptedShapes; + assert.ok(declared.length > 0, 'schema must declare at least one shape, else this assertion is vacuous'); + for (const shape of declared) { + const spelling = shape === 'N' + ? 'Total Plans in Phase' + : `Current Plan: ${shape}`; + assert.ok( + output.error.includes(spelling), + `error should name declared shape ${JSON.stringify(shape)} as ${JSON.stringify(spelling)}; got: ${output.error}`, + ); + } + // The body-only `Plan` field has no schema row (buildStateFrontmatter never + // reads it into frontmatter), so it is named explicitly. + assert.ok(output.error.includes('`Plan: N of M`'), + `error should name \`Plan: N of M\`; got: ${output.error}`); + // ...and must NOT advertise a shape the parser refuses (#3791 review round + // 6, M2). `Plan: N` paired with a `Total Plans in Phase: M` sibling and no + // `Current Plan` is not an accepted shape; a message naming it would send + // the reader to write a STATE.md this command still cannot read. + assert.ok(!output.error.includes('`Plan: N` with'), + `error must not advertise the unaccepted bare-Plan+sibling shape; got: ${output.error}`); }); test('advances plan in compound "Plan: X of Y" format', () => {