From be9329b10b5477aebbdb7d31e1590396cf1aaef7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 12:38:23 -0400 Subject: [PATCH] fix(#3374): phase.complete stops harvesting stale body stopped_at (#3491) * fix(#3374): phase.complete stops harvesting stale body stopped_at Variant A: cmdPhaseComplete's adapter calls syncStateFrontmatter directly (deliberately - STATE.md commits atomically with ROADMAP/REQUIREMENTS), which also bypassed the #948/#1230 preservation pass every RMW write gets. A stale body 'Stopped at:' line then silently clobbered a fresher frontmatter stopped_at on every phase completion, with warnings: []. Three layers close it without reversing #3517's refresh expectation: - completePhaseCore now refreshes the body continuity line it implies ('Phase N complete, ready to plan Phase N+1'; ADR-2207 phrasing on the last phase), session-scoped via the new stateReplaceFieldInSession seam so a decoy bold Stopped-at line in an unrelated section cannot absorb the refresh. Replace-only - a layout with no session line keeps its shape and its frontmatter value survives via the preservation delta. - the RMW post-sync preservation chunk (snapshots + table-driven applyStatePreservation + #2736 re-assert, full bodyDeltas wired) is extracted into the shared applyPostSyncPreservation helper; the phase.complete adapter and writeStateMd (milestone complete / state sync - the gap the closed PR #3442 review flagged) now run it too. - cmdStateRecordSession pushed 'Stopped At' onto updated[] on any label MATCH, including a value already on disk - reporting a write that never changed a byte. It now reports only on real change, and the match is tracked separately so an identical value does not arm the #944 DWIM section rewrite (which would reset an executor-authored resume file to None). * docs(#3374): backfill changeset pr field to 3491 * fix(#3374): drop the writeStateMd preservation pass - state sync's #905 contract is body-wins CI on this PR caught what the closed PR #3442 review's MAJOR remediation option (a) would have broken: state sync's #905 contract ('body annotation beats existing frontmatter when both are present') is the opposite by design - sync exists to re-derive frontmatter from the body. A blanket applyStatePreservation pass on writeStateMd re-locked stale frontmatter (current_phase 3 over the body's 5) on every sync. Take the review's sanctioned option (b) instead: the scope claim is accurate (phase.complete only) and the milestone complete / state sync exposure is tracked as follow-up issue #3492. --------- Co-authored-by: sim --- .changeset/plucky-wolves-leap.md | 5 + src/phase.cts | 30 ++- src/state-document.cts | 30 ++- src/state-transition.cts | 26 ++- src/state.cts | 337 +++++++++++++++++++------------ tests/frontmatter.test.cjs | 177 ++++++++++++++++ tests/state.test.cjs | 80 ++++++++ 7 files changed, 551 insertions(+), 134 deletions(-) create mode 100644 .changeset/plucky-wolves-leap.md diff --git a/.changeset/plucky-wolves-leap.md b/.changeset/plucky-wolves-leap.md new file mode 100644 index 000000000..3f41ee4bb --- /dev/null +++ b/.changeset/plucky-wolves-leap.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3491 +--- +phase.complete no longer rewrites STATE.md frontmatter stopped_at with a stale body 'Stopped at:' line: the completion now refreshes the session continuity line it implies ('Phase N complete, ready to plan Phase N+1') and applies the standard field-preservation policy on its atomic commit path. state record-session no longer reports 'Stopped At' as updated when the value is already current. diff --git a/src/phase.cts b/src/phase.cts index f745f1a5d..644516d8e 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -91,6 +91,7 @@ const { stateExtractField, stateReplaceField, syncStateFrontmatter, + applyPostSyncPreservation, withStateLock, updatePerformanceMetricsSection, } = stateMod; @@ -2964,7 +2965,9 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { // this adapter: they are section-table / disk-scan concerns, not // classified fields, and `syncStateFrontmatter` is the post-sync this // transaction needs (it does NOT go through readModifyWriteStateMd - // because STATE.md is committed atomically with ROADMAP/REQUIREMENTS). + // because STATE.md is committed atomically with ROADMAP/REQUIREMENTS — + // the post-sync preservation pass runs via applyPostSyncPreservation + // instead, #3374). const nextPhaseDisplayName = phaseDisplayNameFromRoadmap(roadmapContent, nextPhaseNum) ?? phaseDisplayNameFromSlug(nextPhaseName); @@ -3018,7 +3021,30 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { current_phase_name: nextPhaseDisplayName, } : undefined; - stateContent = syncStateFrontmatter(stateContent, cwd, authoritativeFm); + const synced = syncStateFrontmatter(stateContent, cwd, authoritativeFm); + // #3374: the direct sync above deliberately bypasses + // readModifyWriteStateMd (STATE.md is committed atomically with + // ROADMAP/REQUIREMENTS), which also bypassed the #948/#1230 + // preservation pass every RMW write gets — so a stale body + // `Stopped at:` line silently clobbered a fresher frontmatter + // stopped_at on every completion. Run the shared post-sync pass: + // snapshots from the on-disk pre-image (originalStateContent) and the + // transformed content, table-driven applyStatePreservation, then the + // #2736 authoritative re-assert (which restores the #3350 pairing + // override the preserve-always restore may have reverted). resync=true + // is the lifecycle-transition posture (progress recomputed from disk; + // only the preserve-when-unchanged deltas apply). Fields the + // transition legitimately rewrote (Status, Phase, Stopped At via + // completePhaseCore's #3374 continuity line) have changed body + // sources, so their deltas do not fire. + stateContent = applyPostSyncPreservation( + originalStateContent, + stateContent, + synced, + statePath, + true, + authoritativeFm, + ); writes.push({ filePath: statePath, before: originalStateContent, after: stateContent }); } diff --git a/src/state-document.cts b/src/state-document.cts index a47aa9f8b..5d0c5fe0c 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -9,7 +9,7 @@ import { splitTableRow } from './markdown-table.cjs'; import { clampPercentFromFraction } from './phase-lifecycle.cjs'; -import { collectSection } from './markdown-sectionizer.cjs'; +import { collectSection, withSection } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-scope.cjs is an export= CommonJS module @@ -373,6 +373,34 @@ export function stateReplaceFieldWithFallback(content: string, primary: string, return content; } +/** + * #3374: session-scoped variant of stateReplaceFieldWithFallback for the + * `## Session` continuity fields. The post-sync harvest (state.cts's + * matchSessionSection → buildStateFrontmatter) reads these fields ONLY from + * the session section, so a writer that refreshes one must target the same + * scope — a whole-body replace lets a decoy `**Stopped at:**` line in an + * unrelated (e.g. archive) section absorb the refresh while the harvested + * session value stays stale. + * + * Section preference mirrors the reader exactly: the normalized `## Session` + * block wins over the bootstrap `## Session Continuity` heading when both + * exist (legacy duplicate files); the continuity heading is only consulted + * when no canonical `## Session` section exists. `levelBounded` heading + * matching also excludes `## Session Continuity Archive` (the #2444 scoping). + * + * Replace-only (no insertion): returns `content` unchanged when no session + * section exists or the field is absent from it, so a STATE.md layout without + * the line keeps its shape and the post-sync preservation pass decides the + * frontmatter value (see #3374). + */ +export function stateReplaceFieldInSession(content: string, primary: string, fallback: string | null | undefined, value: string): string { + const isSession = (h: HeadingToken): boolean => h.level === 2 && h.text.trim().toLowerCase() === 'session'; + const isSessionContinuity = (h: HeadingToken): boolean => h.level === 2 && h.text.trim().toLowerCase() === 'session continuity'; + const hasCanonicalSession = collectSection(content, isSession, { levelBounded: true }) !== null; + const target = hasCanonicalSession ? isSession : isSessionContinuity; + return withSection(content, target, (sectionBody) => stateReplaceFieldWithFallback(sectionBody, primary, fallback, value)); +} + export function normalizeStateStatus(status: string | null | undefined, pausedAt: unknown): string { let normalizedStatus = status || 'unknown'; const statusLower = (status || '').toLowerCase(); diff --git a/src/state-transition.cts b/src/state-transition.cts index d694c4c44..d6c803587 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -16,7 +16,7 @@ // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); -import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback } from './state-document.cjs'; +import { stateReplaceField, stateExtractField, stateReplaceFieldIfTemplate, stateReplaceFieldWithFallback, stateReplaceFieldInSession } from './state-document.cjs'; import { KNOWN_TEMPLATE_DEFAULTS } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; @@ -1057,6 +1057,7 @@ function completePhaseCore( 'current_plan', 'last_activity', 'last_activity_desc', + 'stopped_at', 'progress', ]) { const cls = getFieldClassification(fmKey); @@ -1157,6 +1158,29 @@ function completePhaseCore( updated.push('Last Activity Description'); } + // Stopped At — #3374: write the continuity line this transition implies. + // The frontmatter `stopped_at` is a projection of this body line + // (source: 'body' in FIELD_CLASSIFICATION), and phase completion is exactly + // the event the line describes — leaving it stale made the post-sync harvest + // overwrite a fresher frontmatter value with pre-completion prose on every + // completion (#3374), and left the workflow's later prose refresh as a + // divergence source. Session-SCOPED replace (stateReplaceFieldInSession): + // the harvest reads only the session section, so the write must target the + // same scope — a whole-body replace let a decoy `**Stopped at:**` line in an + // unrelated section absorb the refresh. Replace-only (no insertion): a + // STATE.md with no session continuity line keeps its shape, and the + // unchanged body source then lets the preservation delta keep an existing + // frontmatter value. Last-phase wording reuses the ADR-2207 status phrase; + // milestone termination wording stays owned by milestoneCompleteCore. + const stoppedAtLine = intent.isLastPhase + ? `Phase ${intent.phaseNum} complete — all phases complete` + : `Phase ${intent.phaseNum} complete${intent.nextPhaseNum ? `, ready to plan Phase ${intent.nextPhaseNum}` : ''}`; + const stoppedAfter = stateReplaceFieldInSession(body, 'Stopped At', 'Stopped at', stoppedAtLine); + if (stoppedAfter !== body) { + body = stoppedAfter; + updated.push('Stopped At'); + } + // Progress block — re-derive completed/total phases from the roadmap when // available (milestone-wide source of truth), then recompute the percent. // Only runs when a Completed Phases field exists (the existing guard). diff --git a/src/state.cts b/src/state.cts index 855dc0928..287f42c61 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1254,10 +1254,22 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, if (result) { content = result; updated.push('Last Date'); } // Update Stopped at + // #3374 Variant B: stateReplaceField returns the replaced string on any + // label MATCH, including when the value is already the target. Pushing + // 'Stopped At' on match alone reported a write that never changed a byte + // (and that the #948 no-op guard may then discard entirely), leaving a + // stale frontmatter stopped_at undetectable to the caller. Report only on + // real change — and track the match separately so an identical value does + // not read as "label missing" to the #944 DWIM insertion below (whose + // section rewrite would reset an executor-authored resume file to None). + let stoppedAtMatched = false; if (options.stopped_at) { result = stateReplaceField(content, 'Stopped At', options.stopped_at); if (!result) result = stateReplaceField(content, 'Stopped at', options.stopped_at); - if (result) { content = result; updated.push('Stopped At'); } + if (result) { + stoppedAtMatched = true; + if (result !== content) { content = result; updated.push('Stopped At'); } + } } // Update Resume File — only when the caller explicitly passed a value OR the @@ -1308,7 +1320,10 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, // missing canonical fields are inserted while the heading and any prose are // preserved (#1101). Only append a brand-new section when NEITHER heading exists. const callerSuppliedValues = !!(options.stopped_at || (options.resume_file !== undefined && options.resume_file !== null)); - const needsStoppedAt = options.stopped_at && !updated.includes('Stopped At'); + // #3374: keyed on the label MATCH, not on updated[] — a matched-but- + // identical value is already persisted on disk and must not trigger the + // insertion rewrite below. + const needsStoppedAt = options.stopped_at && !stoppedAtMatched; const needsResumeFile = options.resume_file !== undefined && options.resume_file !== null && !updated.includes('Resume File'); const needsLastSession = !updated.includes('Last session') && !updated.includes('Last Date'); @@ -2408,13 +2423,20 @@ function syncStateFrontmatter(content: string, cwd: string | undefined, authorit // survive every writeStateMd call. // // For stopped_at / paused_at: the original #905 "fall back when derived is - // absent" rule is preserved here. The stale-body-overwrites-frontmatter - // scenario from #948 is prevented by the no-op guard in - // readModifyWriteStateMd: when the transform produces no change the file is - // never written, so syncStateFrontmatter never even runs. Attempting to - // "always prefer frontmatter" here breaks legitimate callers like phase.complete - // that intentionally write a new stopped_at value to the body and expect - // syncStateFrontmatter to pick it up. + // absent" rule is preserved here — this block handles the EMPTY case only. + // The disagreeing case (a present-but-stale body value vs a fresher + // frontmatter value, #948/#3374) is NOT handled here: it is governed by + // applyStatePreservation's preserve-when-unchanged delta, applied post-sync + // by the shared applyPostSyncPreservation pass — run by + // readModifyWriteStateMd and by cmdPhaseComplete's adapter (the one caller + // that deliberately bypasses the RMW wrapper for the atomic + // ROADMAP/REQUIREMENTS/STATE commit; #3374). The writeStateMd path + // (state sync) intentionally derives from the body instead — its #905 + // contract is body-beats-frontmatter. "Always prefer frontmatter" here + // would still be wrong: it would break transforms that legitimately write a + // new body value and expect this sync to project it — the #1230 delta + // ("did THIS write change the body source?") is what distinguishes those + // from a stale harvest. if (!derivedFm['stopped_at'] && existingFm['stopped_at']) { derivedFm['stopped_at'] = existingFm['stopped_at']; } @@ -2748,6 +2770,165 @@ function writeStateMd(statePath: string, content: string, cwd?: string, clock?: } } +/** + * #3374: the shared post-sync preservation pass — the pre/post body-source + * snapshot + table-driven `applyStatePreservation` + #2736 authoritative + * re-assert sequence. Extracted from readModifyWriteStateMd so + * `cmdPhaseComplete`'s atomic-commit adapter (phase.cts) — which syncs + * STATE.md directly because it is committed atomically with + * ROADMAP/REQUIREMENTS and so cannot go through the RMW wrapper — applies the + * identical policy instead of a second, weaker encoding. Previously the + * adapter had no preservation at all, letting a stale body `Stopped at:` line + * silently clobber a fresher frontmatter `stopped_at` on every phase + * completion (#3374 Variant A). + * + * NOT applied on the writeStateMd path: `state sync`'s contract is the + * opposite by design (#905 — "body annotation beats existing frontmatter when + * both are present": sync exists to re-derive frontmatter from the body), so a + * blanket preservation pass there re-locks stale frontmatter. The + * milestone-complete equivalent of the #3374 exposure is tracked as a + * follow-up (see PR #3491 / the closed PR #3442 review's MAJOR finding). + * + * `originalContent` is the pre-write on-disk content (drives the #1230 + * pre-snapshots), `transformedContent` is the post-transform content (the + * sync only rewrites the frontmatter block, so its body IS the post-write + * body), and `syncedContent` is what `syncStateFrontmatter` produced. + */ +function applyPostSyncPreservation( + originalContent: string, + transformedContent: string, + syncedContent: string, + statePath: string, + resync: boolean, + authoritativeFm?: Record, + deriveProgressKeys?: boolean, +): string { + // Snapshot the existing progress block BEFORE the transform so we can + // restore it when resync is false. + const preFm = resync ? null : extractFrontmatter(originalContent, statePath) as Record; + + // Bug #1230: delta heuristic — snapshot pre-transform body source fields so + // we can detect whether THIS write changed them. syncStateFrontmatter + // re-derives frontmatter status/stopped_at from the body on every write; + // when the body's source field was NOT changed by the transform, the + // existing frontmatter value (e.g. a hand-set 'completed') must win over + // the body-derived value (e.g. 'verifying' from a stale "Status: Verifying + // Phase 3" line that an earlier tool wrote). We do NOT disturb `preFm` + // above (null when resync:true) — these are independent snapshots. + // Strip frontmatter before calling stateExtractField so the YAML `status:` + // key in the frontmatter block cannot shadow the body field we are tracking. + const preBody = stripFrontmatter(originalContent); + const preFmSnapshot = extractFrontmatter(originalContent, statePath) as Record; + const preBodyStatus = stateExtractField(preBody, 'Status'); + // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, + // mirroring buildStateFrontmatter's sessionBodyScope logic. + // A stale "Stopped at:" in a non-Session section (e.g. Session Continuity + // Archive prose) must not interfere with the delta comparison. + const preSessionMatch = matchSessionSection(preBody); + const preSessionScope = preSessionMatch ?? preBody; + const preBodyStoppedAt = stateExtractField(preSessionScope, 'Stopped At') || stateExtractField(preSessionScope, 'Stopped at'); + + // ADR-1769 Phase 6 / #1743 / #1695: snapshot the body source for the curated + // current_phase_name (the `Phase:` line parseProsePhaseField harvests). When + // this write does NOT change that line, the curated frontmatter value must + // win over syncStateFrontmatter's body re-derivation (which can harvest a + // wrong parenthetical aside — #1695). Gated by the field-classification + // table's preserve-always row so the rule lives in one place. + const preBodyPhaseSource = stateExtractField(preBody, 'Phase'); + + // #3258: snapshot the body sources for the additional preserve-when-unchanged + // rows applyStatePreservation now honors (last_activity_desc, paused_at, + // current_phase, current_plan). Each mirrors buildStateFrontmatter's + // derivation so the #1230 delta ("did THIS write change the source?") is + // accurate: current_phase combines `Current Phase` with the prose `Phase:` + // fallback (parseProsePhaseField, scoped to ## Current Position); paused_at + // is session-scoped (mirrors stopped_at); last_activity_desc combines the + // `Last Activity Description` field with the prose desc fallback. + const preCurrentPositionScope = matchCurrentPositionSection(preBody) ?? preBody; + const preBodyCurrentPlan = stateExtractField(preBody, 'Current Plan'); + const preBodyCurrentPhase = stateExtractField(preBody, 'Current Phase') + ?? parseProsePhaseField(stateExtractField(preCurrentPositionScope, 'Phase')).phase; + const preBodyPausedAt = stateExtractField(preSessionScope, 'Paused At'); + const preBodyLastActivityRaw = stateExtractField(preBody, 'Last Activity') + ?? stateExtractField(preBody, 'Last activity'); + const preBodyLastActivityDesc = stateExtractField(preBody, 'Last Activity Description') + ?? parseProseLastActivityField(preBodyLastActivityRaw).description; + + // Post-transform body source fields used for the delta comparison (#1230). + // Use `transformedContent` (not `syncedContent`): syncStateFrontmatter only + // rewrites the frontmatter block, so the body is identical in both — and we + // need the body the transform produced. Strip frontmatter so the YAML + // status key cannot shadow the body field we are tracking. + const postBody = stripFrontmatter(transformedContent); + const postBodyStatus = stateExtractField(postBody, 'Status'); + // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, + // consistent with the pre-transform snapshot above and buildStateFrontmatter. + const postSessionMatch = matchSessionSection(postBody); + const postSessionScope = postSessionMatch ?? postBody; + const postBodyStoppedAt = stateExtractField(postSessionScope, 'Stopped At') || stateExtractField(postSessionScope, 'Stopped at'); + // ADR-1769 Phase 6 / #1695: post-transform body Phase source for the + // current_phase_name delta comparison. + const postBodyPhaseSource = stateExtractField(postBody, 'Phase'); + // #3258: post-transform body sources for the preserve-when-unchanged rows + // added in #3258 (mirrors the pre-transform block above). + const postCurrentPositionScope = matchCurrentPositionSection(postBody) ?? postBody; + const postBodyCurrentPlan = stateExtractField(postBody, 'Current Plan'); + const postBodyCurrentPhase = stateExtractField(postBody, 'Current Phase') + ?? parseProsePhaseField(stateExtractField(postCurrentPositionScope, 'Phase')).phase; + const postBodyPausedAt = stateExtractField(postSessionScope, 'Paused At'); + const postBodyLastActivityRaw = stateExtractField(postBody, 'Last Activity') + ?? stateExtractField(postBody, 'Last activity'); + const postBodyLastActivityDesc = stateExtractField(postBody, 'Last Activity Description') + ?? parseProseLastActivityField(postBodyLastActivityRaw).description; + const bodyDeltas = { + last_activity_desc: { pre: preBodyLastActivityDesc, post: postBodyLastActivityDesc }, + paused_at: { pre: preBodyPausedAt, post: postBodyPausedAt }, + current_phase: { pre: preBodyCurrentPhase, post: postBodyCurrentPhase }, + current_plan: { pre: preBodyCurrentPlan, post: postBodyCurrentPlan }, + }; + + // ADR-1769 #1796 (Path A — finish the consolidation): the post-sync + // preservation block is now the pure, table-driven `applyStatePreservation` + // in the STATE.md Transition Module. progress / status / stopped_at / + // current_phase_name are all governed by their FIELD_CLASSIFICATION row — + // one policy source, not three drifting encodings. #3258 extends the same + // pass to last_activity_desc / paused_at / current_phase / current_plan + // (preserve-when-unchanged) and milestone / milestone_name (preserve-if- + // placeholder). Behavior-identical to the pre-#1796 inline block for the + // original four fields; this is the absorption ADR-1769 / CONTEXT.md + // already claimed shipped. + const postFm = extractFrontmatter(syncedContent, statePath) as Record; + const preservation = applyStatePreservation({ + preFm, postFm, preFmSnapshot, resync, + deriveProgressKeys: deriveProgressKeys === true, + bodyDeltas, + preBodyStatus, postBodyStatus, + preBodyStoppedAt, postBodyStoppedAt, + preBodyPhaseSource, postBodyPhaseSource, + }); + // #2736: re-assert the intent-first values AFTER preservation. On STATE.md + // layouts with no body `Phase:` line, both phase-source snapshots are null + // (equal), so the #1695 restore fires and would put the stale pre-transition + // name back over the authoritative one. Intent beats both the prose + // re-derivation and the curated restore — the transition just resolved it. + let authoritativeReasserted = false; + if (authoritativeFm) { + for (const [key, value] of Object.entries(authoritativeFm)) { + if (typeof value === 'string' && value.trim().length > 0 && preservation.postFm[key] !== value) { + preservation.postFm[key] = value; + authoritativeReasserted = true; + } + } + } + + if (preservation.mutated || authoritativeReasserted) { + const yamlStr = reconstructFrontmatter(preservation.postFm as unknown as Frontmatter); + const body = stripFrontmatter(syncedContent); + return `---\n${yamlStr}\n---\n\n${body}`; + } + return syncedContent; +} + /** * Atomic read-modify-write for STATE.md. * Holds the lock across the entire read -> transform -> write cycle, @@ -2773,56 +2954,6 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string const lockPath = acquireStateLock(statePath, clock); try { const content = platformReadSync(statePath) || ''; - // Snapshot the existing progress block BEFORE the transform so we can - // restore it when resync is false. - const preFm = resync ? null : extractFrontmatter(content, statePath) as Record; - - // Bug #1230: delta heuristic — snapshot pre-transform body source fields so - // we can detect whether THIS write changed them. syncStateFrontmatter - // re-derives frontmatter status/stopped_at from the body on every write; - // when the body's source field was NOT changed by the transform, the - // existing frontmatter value (e.g. a hand-set 'completed') must win over - // the body-derived value (e.g. 'verifying' from a stale "Status: Verifying - // Phase 3" line that an earlier tool wrote). We do NOT disturb `preFm` - // above (null when resync:true) — these are independent snapshots. - // Strip frontmatter before calling stateExtractField so the YAML `status:` - // key in the frontmatter block cannot shadow the body field we are tracking. - const preBody = stripFrontmatter(content); - const preFmSnapshot = extractFrontmatter(content, statePath) as Record; - const preBodyStatus = stateExtractField(preBody, 'Status'); - // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, - // mirroring buildStateFrontmatter's sessionBodyScope logic (line ~1172). - // A stale "Stopped at:" in a non-Session section (e.g. Session Continuity - // Archive prose) must not interfere with the delta comparison. - const preSessionMatch = matchSessionSection(preBody); - const preSessionScope = preSessionMatch ?? preBody; - const preBodyStoppedAt = stateExtractField(preSessionScope, 'Stopped At') || stateExtractField(preSessionScope, 'Stopped at'); - - // ADR-1769 Phase 6 / #1743 / #1695: snapshot the body source for the curated - // current_phase_name (the `Phase:` line parseProsePhaseField harvests). When - // this write does NOT change that line, the curated frontmatter value must - // win over syncStateFrontmatter's body re-derivation (which can harvest a - // wrong parenthetical aside — #1695). Gated by the field-classification - // table's preserve-always row so the rule lives in one place. - const preBodyPhaseSource = stateExtractField(preBody, 'Phase'); - - // #3258: snapshot the body sources for the additional preserve-when-unchanged - // rows applyStatePreservation now honors (last_activity_desc, paused_at, - // current_phase, current_plan). Each mirrors buildStateFrontmatter's - // derivation so the #1230 delta ("did THIS write change the source?") is - // accurate: current_phase combines `Current Phase` with the prose `Phase:` - // fallback (parseProsePhaseField, scoped to ## Current Position); paused_at - // is session-scoped (mirrors stopped_at); last_activity_desc combines the - // `Last Activity Description` field with the prose desc fallback. - const preCurrentPositionScope = matchCurrentPositionSection(preBody) ?? preBody; - const preBodyCurrentPlan = stateExtractField(preBody, 'Current Plan'); - const preBodyCurrentPhase = stateExtractField(preBody, 'Current Phase') - ?? parseProsePhaseField(stateExtractField(preCurrentPositionScope, 'Phase')).phase; - const preBodyPausedAt = stateExtractField(preSessionScope, 'Paused At'); - const preBodyLastActivityRaw = stateExtractField(preBody, 'Last Activity') - ?? stateExtractField(preBody, 'Last activity'); - const preBodyLastActivityDesc = stateExtractField(preBody, 'Last Activity Description') - ?? parseProseLastActivityField(preBodyLastActivityRaw).description; const modified = transformFn(content); @@ -2838,77 +2969,17 @@ function readModifyWriteStateMd(statePath: string, transformFn: (content: string } let synced = syncStateFrontmatter(modified, cwd, options?.authoritativeFm); - - // Post-transform body source fields used for the delta comparison (#1230). - // Use `modified` (not `synced`): syncStateFrontmatter only rewrites the frontmatter block, so the body is identical in both — and we need the body the transform produced. - // Strip frontmatter so the YAML status key cannot shadow the body field we are tracking. - const postBody = stripFrontmatter(modified); - const postBodyStatus = stateExtractField(postBody, 'Status'); - // Bug #1230 / Change B: scope stopped_at delta to the ## Session section, - // consistent with the pre-transform snapshot above and buildStateFrontmatter. - const postSessionMatch = matchSessionSection(postBody); - const postSessionScope = postSessionMatch ?? postBody; - const postBodyStoppedAt = stateExtractField(postSessionScope, 'Stopped At') || stateExtractField(postSessionScope, 'Stopped at'); - // ADR-1769 Phase 6 / #1695: post-transform body Phase source for the - // current_phase_name delta comparison. - const postBodyPhaseSource = stateExtractField(postBody, 'Phase'); - // #3258: post-transform body sources for the preserve-when-unchanged rows - // added in #3258 (mirrors the pre-transform block above). - const postCurrentPositionScope = matchCurrentPositionSection(postBody) ?? postBody; - const postBodyCurrentPlan = stateExtractField(postBody, 'Current Plan'); - const postBodyCurrentPhase = stateExtractField(postBody, 'Current Phase') - ?? parseProsePhaseField(stateExtractField(postCurrentPositionScope, 'Phase')).phase; - const postBodyPausedAt = stateExtractField(postSessionScope, 'Paused At'); - const postBodyLastActivityRaw = stateExtractField(postBody, 'Last Activity') - ?? stateExtractField(postBody, 'Last activity'); - const postBodyLastActivityDesc = stateExtractField(postBody, 'Last Activity Description') - ?? parseProseLastActivityField(postBodyLastActivityRaw).description; - const bodyDeltas = { - last_activity_desc: { pre: preBodyLastActivityDesc, post: postBodyLastActivityDesc }, - paused_at: { pre: preBodyPausedAt, post: postBodyPausedAt }, - current_phase: { pre: preBodyCurrentPhase, post: postBodyCurrentPhase }, - current_plan: { pre: preBodyCurrentPlan, post: postBodyCurrentPlan }, - }; - - // ADR-1769 #1796 (Path A — finish the consolidation): the post-sync - // preservation block is now the pure, table-driven `applyStatePreservation` - // in the STATE.md Transition Module. progress / status / stopped_at / - // current_phase_name are all governed by their FIELD_CLASSIFICATION row — - // one policy source, not three drifting encodings. #3258 extends the same - // pass to last_activity_desc / paused_at / current_phase / current_plan - // (preserve-when-unchanged) and milestone / milestone_name (preserve-if- - // placeholder). Behavior-identical to the pre-#1796 inline block for the - // original four fields; this is the absorption ADR-1769 / CONTEXT.md - // already claimed shipped. - const postFm = extractFrontmatter(synced, statePath) as Record; - const preservation = applyStatePreservation({ - preFm, postFm, preFmSnapshot, resync, - deriveProgressKeys: options?.deriveProgressKeys === true, - bodyDeltas, - preBodyStatus, postBodyStatus, - preBodyStoppedAt, postBodyStoppedAt, - preBodyPhaseSource, postBodyPhaseSource, - }); - // #2736: re-assert the intent-first values AFTER preservation. On STATE.md - // layouts with no body `Phase:` line, both phase-source snapshots are null - // (equal), so the #1695 restore fires and would put the stale pre-transition - // name back over the authoritative one. Intent beats both the prose - // re-derivation and the curated restore — the transition just resolved it. - let authoritativeReasserted = false; - if (options?.authoritativeFm) { - for (const [key, value] of Object.entries(options.authoritativeFm)) { - if (typeof value === 'string' && value.trim().length > 0 && preservation.postFm[key] !== value) { - preservation.postFm[key] = value; - authoritativeReasserted = true; - } - } - } - - if (preservation.mutated || authoritativeReasserted) { - const yamlStr = reconstructFrontmatter(preservation.postFm as unknown as Frontmatter); - const body = stripFrontmatter(synced); - synced = `---\n${yamlStr}\n---\n\n${body}`; - } + // #3374: the post-sync preservation pass (snapshots, table-driven + // applyStatePreservation, #2736 re-assert) — see applyPostSyncPreservation. + synced = applyPostSyncPreservation( + content, + modified, + synced, + statePath, + resync, + options?.authoritativeFm, + options?.deriveProgressKeys === true, + ); platformWriteSync(statePath, synced); return true; @@ -4267,6 +4338,12 @@ export = { writeStateMd, readModifyWriteStateMd, syncStateFrontmatter, + // #3374: the shared post-sync preservation pass (snapshots + table-driven + // applyStatePreservation + #2736 re-assert). Exported for cmdPhaseComplete's + // atomic-commit adapter in phase.cts, which syncs STATE.md directly (it is + // committed atomically with ROADMAP/REQUIREMENTS) and must apply the same + // preservation policy the RMW path applies. + applyPostSyncPreservation, readStateHeadFreshness, withStateLock, updatePerformanceMetricsSection, diff --git a/tests/frontmatter.test.cjs b/tests/frontmatter.test.cjs index 11afffe7d..2e41bfd18 100644 --- a/tests/frontmatter.test.cjs +++ b/tests/frontmatter.test.cjs @@ -2218,6 +2218,183 @@ test('extractFrontmatter handles large frontmatter blocks without body bleed', ( } +// ──────────────────────────────────────────────────────────────────────── +// #3374 — `phase complete`'s adapter calls syncStateFrontmatter directly +// (deliberately bypassing readModifyWriteStateMd for the atomic +// ROADMAP/REQUIREMENTS/STATE commit), which also bypassed the #948/#1230 +// preservation pass every RMW write gets. A stale body `Stopped at:` line then +// silently clobbered a fresher frontmatter `stopped_at` on every phase +// completion. Placed beside the #2736 suite (the same defect family: the +// adapter's post-sync policy diverging from the RMW path's). The fix is +// two-layered: completePhaseCore now refreshes the body continuity line it +// implies (`Phase N complete, ready to plan Phase N+1`) — session-scoped, so a +// decoy `**Stopped at:**` line in an unrelated section cannot absorb the +// refresh — so the harvest projects a value this very completion produced +// (keeping #3517's refresh expectation), and the adapter runs the RMW post-sync +// preservation pass (applyPostSyncPreservation) so a body source this write did +// not refresh cannot beat a fresher frontmatter value. +// ──────────────────────────────────────────────────────────────────────── +{ + const { describe: __d3374, test: __t3374, beforeEach: __be3374, afterEach: __ae3374 } = require('node:test'); + const __assert3374 = require('node:assert/strict'); + const __fs3374 = require('node:fs'); + const __path3374 = require('node:path'); + const { runGsdTools: __run3374, createTempProject: __mk3374, cleanup: __rm3374 } = require('./helpers.cjs'); + const { extractFrontmatter: __extractFm3374 } = require('../gsd-core/bin/lib/frontmatter.cjs'); + const { stateExtractField: __extractField3374 } = require('../gsd-core/bin/lib/state-document.cjs'); + + const FRESH_3374 = 'Phase 2 gap closure executed — FRESH frontmatter value'; + const STALE_3374 = 'Phase 1 complete, ready to plan Phase 2'; + const COMPLETION_LINE_3374 = 'Phase 2 complete, ready to plan Phase 3'; + + // Mirrors the issue's repro: a 3-phase roadmap completing phase 2 (not-last), + // body `## Session Continuity` holding a stale plain-label `Stopped at:` line + // that phase.complete's transition previously never touched. + function writeCompleteFixture3374(tmpDir, { fmStoppedAt = null, sessionStoppedAt = STALE_3374, decoy = false } = {}) { + const planningDir = __path3374.join(tmpDir, '.planning'); + const phase2Dir = __path3374.join(planningDir, 'phases', '02-second-phase'); + __fs3374.mkdirSync(phase2Dir, { recursive: true }); + + __fs3374.writeFileSync( + __path3374.join(planningDir, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '### Phase 1: First phase', + '**Plans:** 1 plans', + '', + '### Phase 2: Second phase', + '**Plans:** 1 plans', + '', + '### Phase 3: Third phase', + '**Plans:** 1 plans', + '', + '## Progress', + '', + '- [x] **Phase 1: First phase** - done', + '- [ ] **Phase 2: Second phase** - pending', + '- [ ] **Phase 3: Third phase** - pending', + '', + ].join('\n'), + ); + + const sessionLines = [ + 'Last session: 2026-08-10', + ...(sessionStoppedAt === null ? [] : [`Stopped at: ${sessionStoppedAt}`]), + 'Resume file: None', + ]; + __fs3374.writeFileSync( + __path3374.join(planningDir, 'STATE.md'), + [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v1.0', + 'current_phase: 2', + 'current_phase_name: Second phase', + 'status: executing', + ...(fmStoppedAt ? [`stopped_at: "${fmStoppedAt}"`] : []), + '---', + '', + '# Project State', + ...(decoy ? ['', '## Archive notes', '', '**Stopped at:** old prose from June'] : []), + '', + '## Session Continuity', + '', + ...sessionLines, + '', + ].join('\n'), + ); + + __fs3374.writeFileSync(__path3374.join(phase2Dir, '02-01-PLAN.md'), '# Plan\n'); + __fs3374.writeFileSync(__path3374.join(phase2Dir, '02-01-SUMMARY.md'), '# Summary\n'); + __fs3374.writeFileSync( + __path3374.join(phase2Dir, '02-VERIFICATION.md'), + ['---', 'status: passed', '---', '', '# Verification', ''].join('\n'), + ); + } + + __d3374('#3374: phase complete must not harvest a stale body Stopped at over fresher frontmatter', () => { + let tmpDir; + const statePath = () => __path3374.join(tmpDir, '.planning', 'STATE.md'); + + __be3374(() => { tmpDir = __mk3374(); }); + __ae3374(() => { __rm3374(tmpDir); }); + + __t3374('AC1: the stale body Stopped at never reaches the frontmatter — the transition refreshes the line it implies', () => { + writeCompleteFixture3374(tmpDir, { fmStoppedAt: FRESH_3374 }); + + const result = __run3374(['phase', 'complete', '2'], tmpDir); + __assert3374.ok(result.success, `phase complete failed: ${result.error}`); + + const stateContent = __fs3374.readFileSync(statePath(), 'utf-8'); + const fm = __extractFm3374(stateContent); + __assert3374.notStrictEqual( + fm.stopped_at, + STALE_3374, + 'phase.complete harvested the stale pre-completion body value into the frontmatter (#3374 Variant A)', + ); + __assert3374.strictEqual( + fm.stopped_at, + COMPLETION_LINE_3374, + `the frontmatter must project the continuity line this completion wrote, never the stale value; got ${JSON.stringify(fm.stopped_at)}`, + ); + __assert3374.strictEqual( + __extractField3374(stateContent, 'Stopped at'), + COMPLETION_LINE_3374, + 'the body continuity line must be refreshed by the transition itself, not left for a later prose step', + ); + }); + + __t3374('AC1 scoping: a decoy Stopped at in a non-session section cannot absorb the continuity refresh', () => { + writeCompleteFixture3374(tmpDir, { fmStoppedAt: FRESH_3374, decoy: true }); + + const result = __run3374(['phase', 'complete', '2'], tmpDir); + __assert3374.ok(result.success, `phase complete failed: ${result.error}`); + + // The harvest reads ONLY the session scope, so the projected frontmatter + // value proves the session line (not the decoy) was the one refreshed. + const fm = __extractFm3374(__fs3374.readFileSync(statePath(), 'utf-8')); + __assert3374.strictEqual( + fm.stopped_at, + COMPLETION_LINE_3374, + `the session-scoped continuity write must win over the whole-body decoy; got ${JSON.stringify(fm.stopped_at)}`, + ); + }); + + __t3374('AC1 preservation leg: with no session Stopped at line to refresh, the fresher frontmatter value survives', () => { + writeCompleteFixture3374(tmpDir, { fmStoppedAt: FRESH_3374, sessionStoppedAt: null }); + + const result = __run3374(['phase', 'complete', '2'], tmpDir); + __assert3374.ok(result.success, `phase complete failed: ${result.error}`); + + // Replace-only continuity write missed → nothing to harvest → the + // pre-existing (fresher) frontmatter value must be preserved, not + // dropped or replaced with pre-completion prose. + const fm = __extractFm3374(__fs3374.readFileSync(statePath(), 'utf-8')); + __assert3374.strictEqual( + fm.stopped_at, + FRESH_3374, + `with no body source refreshed by this write, the existing frontmatter value must survive; got ${JSON.stringify(fm.stopped_at)}`, + ); + }); + + __t3374('AC2 (no-regress): with no pre-existing frontmatter stopped_at, the body line populates it', () => { + writeCompleteFixture3374(tmpDir, {}); + + const result = __run3374(['phase', 'complete', '2'], tmpDir); + __assert3374.ok(result.success, `phase complete failed: ${result.error}`); + + const fm = __extractFm3374(__fs3374.readFileSync(statePath(), 'utf-8')); + __assert3374.strictEqual( + fm.stopped_at, + COMPLETION_LINE_3374, + `first-write population from the (refreshed) body line must keep working; got ${JSON.stringify(fm.stopped_at)}`, + ); + }); + }); +} + + // ──────────────────────────────────────────────────────────────────────── // Folded from tests/fix-2847-gap-closure-frontmatter.test.cjs — test-hygiene sweep #3335 (H3 Wave 3) // ──────────────────────────────────────────────────────────────────────── diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 855101d9f..9ddded1e4 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -2207,6 +2207,86 @@ describe('cmdStateRecordSession (state record-session)', () => { assert.strictEqual(output.recorded, false, 'recorded should be false when no session fields found'); assert.ok(output.reason !== undefined, 'should have a reason'); }); + + // #3374 Variant B: stateReplaceField returns the replaced string on any label + // MATCH, including when the value is already the target — so `updated` used + // to report 'Stopped At' for a write that never changed a byte (and that the + // #948 no-op guard then discarded entirely), leaving a stale frontmatter + // stopped_at undetectable to the caller. The report must reflect real change, + // and a matched-but-identical value must NOT arm the #944 DWIM insertion + // branch (which wholesale-rewrites the session section and would reset an + // executor-authored resume file to 'None'). + test('#3374: --stopped-at with the value already in the body is not reported updated and writes nothing', () => { + const PINNED_MS = Date.parse('2020-09-01T09:00:00.000Z'); + const PINNED_ISO = '2020-09-01T09:00:00.000Z'; + const executorResume = '.planning/phases/02/02-01-PLAN.md'; + const fixture = [ + '# Project State', + '', + '## Session Continuity', + '', + `**Last session:** ${PINNED_ISO}`, + '**Stopped at:** Phase 2, Plan 1', + `**Resume file:** ${executorResume}`, + ].join('\n') + '\n'; + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, fixture); + + const result = runGsdTools('state record-session --stopped-at "Phase 2, Plan 1"', tmpDir, { + GSD_TEST_MODE: '1', + GSD_NOW_MS: String(PINNED_MS), + }); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + !((output.updated || []).includes('Stopped At')), + `updated must not report a Stopped At write that changed nothing; got ${JSON.stringify(output.updated)} (#3374 Variant B)`, + ); + + // The pinned clock makes the Last-session replacement an identity too, so + // the whole transform is a no-op and the #948 no-op guard must skip the + // write entirely — the file must be byte-identical. + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual( + after, + fixture, + 'no field changed, so no write may occur (#3374 Variant B)', + ); + assert.strictEqual( + stateDocument.stateExtractField(after, 'Resume file'), + executorResume, + 'an identical --stopped-at must not arm the #944 DWIM section rewrite (executor-authored resume file reset to None)', + ); + }); + + test('#3374: --stopped-at with a new value still reports Stopped At and syncs the frontmatter', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), sessionFixture); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + const newValue = 'Phase 3 complete, ready to plan Phase 4'; + + const result = runGsdTools(['state', 'record-session', '--stopped-at', newValue], tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + (output.updated || []).includes('Stopped At'), + `a real change must keep reporting Stopped At; got ${JSON.stringify(output.updated)}`, + ); + + const after = fs.readFileSync(statePath, 'utf-8'); + assert.strictEqual( + stateDocument.stateExtractField(after, 'Stopped at'), + newValue, + 'the body Stopped at line should carry the new value', + ); + const fm = frontmatterLib.extractFrontmatter(after); + assert.strictEqual( + fm.stopped_at, + newValue, + `the RMW sync must reflect the new value in frontmatter; got ${JSON.stringify(fm.stopped_at)}`, + ); + }); }); // ─────────────────────────────────────────────────────────────────────────────