diff --git a/.changeset/merry-jays-frolic.md b/.changeset/merry-jays-frolic.md new file mode 100644 index 000000000..dcf4f6f0f --- /dev/null +++ b/.changeset/merry-jays-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4919 +--- +**`state record-session` reports the record it replaces** — overwriting a prior Stopped At or Resume File handoff now names the displaced text in the verb's payload instead of succeeding silently, and the executor decision loop passes --phase so decisions stop inheriting whichever phase the global pointer names. (#4763) diff --git a/agents/gsd-executor.md b/agents/gsd-executor.md index 213cacdf2..c09d90db3 100644 --- a/agents/gsd-executor.md +++ b/agents/gsd-executor.md @@ -777,8 +777,10 @@ gsd_run query state.record-metric \ --tasks "${TASK_COUNT}" --files "${FILE_COUNT}" # Add decisions (extract from SUMMARY.md key-decisions) +# --phase is required here: without it the verb falls back to STATE.md's global +# pointer, which misattributes decisions when plans execute out of pointer order (#4763). for decision in "${DECISIONS[@]}"; do - gsd_run query state.add-decision --summary "${decision}" + gsd_run query state.add-decision --phase "${PHASE}" --summary "${decision}" done # Update session info (stopped-at, resume-file; timestamp set automatically) diff --git a/src/state.cts b/src/state.cts index bb5a51a35..40fa016bd 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1961,11 +1961,32 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, const updated: string[] = []; let sessionCreated = false; const divergedFields: string[] = []; + // #4763 (1): last-writer-wins stays (the recorded single-slot handoff design), + // but a displaced record is no longer silent. The pre-write session record is + // captured below and surfaced in the payload under `replacedRecord` whenever a + // replacement actually changes a non-empty prior value. + const priorRecord: { stoppedAt?: string; resumeFile?: string } = {}; // ADR-3473 §8.7 (#3872): caller-allocated out-param, filled with the // transaction's own pre-write snapshot + body by `applyPostSyncPreservation`. const preWriteState: StatePreWriteSnapshot = {}; readModifyWriteStateMd(statePath, (content) => { + // #4763 (1): read the pre-write session record. The capture mirrors the + // WRITER, not the snapshot reader: stateReplaceField replaces the FIRST + // case-insensitive label match anywhere in the document, so the capture is + // document-wide too — scoping it to ## Session would miss an archive-section + // line the writer actually displaced. Continuation lines join via + // stateFieldContinuation (the sanctioned joiner — stateExtractField alone is + // first-line-only), so a wrapped multi-line handoff is surfaced whole. + const capturePrior = (fieldName: string): string | undefined => { + const first = stateExtractField(content, fieldName); + if (first === null) return undefined; + const cont = stateFieldContinuation(content, fieldName); + return cont ? `${first}\n${cont}` : first; + }; + priorRecord.stoppedAt = capturePrior('Stopped At'); + priorRecord.resumeFile = capturePrior('Resume File'); + // Update Last session / Last Date let result = stateReplaceField(content, 'Last session', now); if (result) { content = result; updated.push('Last session'); } @@ -2181,6 +2202,27 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, if (reconciledUpdated.length > 0) { const result: Record = { recorded: true, updated: reconciledUpdated }; if (sessionCreated) result['created'] = true; + // #4763 (1): surface any non-empty prior record the write displaced. Gated + // on reconciledUpdated (the fields that actually persisted, post-#3957 + // reconcile) rather than the pre-reconciliation updated[]. Only fields with + // real prior content differing from the caller's value count — same-value + // rewrites, the insert path (no prior label), and the #944 template-default + // DWIM (defaults match case-insensitively, so a case-variant 'none' → + // 'None' rewrite is normalization, not displacement) are excluded. + const isResumeTemplateDefault = priorRecord.resumeFile !== undefined + && KNOWN_TEMPLATE_DEFAULTS['Resume File'].some( + (d) => d.toLowerCase() === priorRecord.resumeFile?.toLowerCase()); + const replacedRecord: Record = {}; + if (reconciledUpdated.includes('Stopped At') && priorRecord.stoppedAt + && priorRecord.stoppedAt !== options.stopped_at) { + replacedRecord['Stopped At'] = priorRecord.stoppedAt; + } + if (reconciledUpdated.includes('Resume File') && priorRecord.resumeFile + && !isResumeTemplateDefault + && priorRecord.resumeFile !== (options.resume_file ?? undefined)) { + replacedRecord['Resume File'] = priorRecord.resumeFile; + } + if (Object.keys(replacedRecord).length > 0) result['replacedRecord'] = replacedRecord; output(result, raw, 'true'); } else if (updated.length === 0) { // Nothing was ever attempted — no --stopped-at/--resume-file supplied diff --git a/tests/no-bare-gsd-tools-command-position.test.cjs b/tests/no-bare-gsd-tools-command-position.test.cjs index 52f613b2f..0aa544c77 100644 --- a/tests/no-bare-gsd-tools-command-position.test.cjs +++ b/tests/no-bare-gsd-tools-command-position.test.cjs @@ -103,7 +103,7 @@ const BARE_COMMAND_RE = new RegExp( // Each entry MUST carry a one-line reason; the test prints the allowlist on // failure so a reviewer can see exactly what is sanctioned. const PROSE_ALLOWLIST = [ - { file: 'agents/gsd-executor.md', line: 826, reason: 'describes the SDK return envelope of `gsd-tools query commit`; not an instruction to run the bare word (#4670 shifted it from 823; #4834 shifted it from 825: the delegation @-include replaced the inline preamble above it, the mention is unchanged)' }, + { file: 'agents/gsd-executor.md', line: 828, reason: 'describes the SDK return envelope of `gsd-tools query commit`; not an instruction to run the bare word (#4670 shifted it from 823; #4834 shifted it from 825: the delegation @-include replaced the inline preamble above it; #4763 shifted it from 826: the decision loop gained its --phase line above, the mention is unchanged)' }, { file: 'agents/gsd-phase-researcher.md', line: 33, reason: 'package-legitimacy provenance rule names the command as the source of an OK verdict; descriptive' }, { file: 'agents/gsd-roadmapper.md', line: 660, reason: 'parenthetical "e.g." naming SDK queries a user *could* run; not an agent instruction (#4134 shifted it from 647: the H1 template section added above moved the line, the mention is unchanged)' }, { file: 'agents/gsd-intel-updater.md', line: 40, reason: 'cross-platform note names the `gsd-tools intel ` CLI surface descriptively ("CLI invocations go through..."); not an agent instruction' }, diff --git a/tests/state.test.cjs b/tests/state.test.cjs index bac8724da..eea9df10e 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -3331,6 +3331,230 @@ describe('cmdStateRecordSession (state record-session)', () => { assert.ok(resumeMatch[1].trim() === 'None', 'Resume file should be None when not specified'); }); + // ── #4763 (1): last-writer-wins stays (recorded single-slot handoff design), + // but a displaced record is no longer silent — the payload carries the FULL + // prior value whenever a non-empty Stopped At / Resume File record is actually + // replaced. Same-value rewrites, the insert path, and the #944 template-default + // DWIM are not displacements and must not fabricate one. + test('#4763: replacing a non-empty Stopped At record surfaces the displaced record in the payload', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), sessionFixture); + + const result = runGsdTools('state record-session --stopped-at "Phase 3, Plan 2"', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.recorded, true, 'recorded should be true'); + assert.ok( + output.replacedRecord && typeof output.replacedRecord['Stopped At'] === 'string', + `expected replacedRecord["Stopped At"] in the payload, got: ${result.output}`, + ); + assert.strictEqual( + output.replacedRecord['Stopped At'], + 'Phase 2, Plan 1', + 'the displaced record must be the FULL prior text, not a summary', + ); + // Last-writer-wins unchanged: the disk now carries only the new value. + const updated = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(updated.includes('**Stopped at:** Phase 3, Plan 2'), 'disk must carry the new value'); + assert.ok(!updated.includes('Phase 2, Plan 1'), 'the prior record is still replaced on disk'); + }); + + test('#4763: displacing an authored Resume File surfaces it in the payload', () => { + const authored = [ + '# Project State', + '', + '## Session Continuity', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** Phase 2, Plan 1', + '**Resume file:** .planning/phases/02/02-01-resume.md', + ].join('\n') + '\n'; + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), authored); + + const result = runGsdTools( + 'state record-session --stopped-at "Phase 3, Plan 2" --resume-file "None"', + tmpDir, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + output.replacedRecord + && output.replacedRecord['Resume File'] === '.planning/phases/02/02-01-resume.md', + `expected the authored Resume File surfaced in replacedRecord, got: ${result.output}`, + ); + assert.strictEqual(output.replacedRecord['Stopped At'], 'Phase 2, Plan 1', + 'both displaced fields land in the one replaced record'); + }); + + test('#4763: a same-value re-record fabricates no replaced record', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), sessionFixture); + + const result = runGsdTools('state record-session --stopped-at "Phase 2, Plan 1"', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + !output.replacedRecord, + `nothing was displaced when the value is identical, got: ${result.output}`, + ); + }); + + test('#4763: a brand-new session reports no replaced record', () => { + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), '# Project State\n'); + + const result = runGsdTools( + 'state record-session --stopped-at "Phase 1, Plan 1" --resume-file ".planning/phases/01/01-01-PLAN.md"', + tmpDir, + ); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.created, true, 'insert path reports created'); + assert.ok(!output.replacedRecord, 'nothing prior existed — nothing was displaced'); + }); + + test('#4763: the #944 template-default Resume File rewrite is not a displacement', () => { + // `**Resume file:** None` is a KNOWN_TEMPLATE_DEFAULTS value, so the #944 + // DWIM rewrites it to the same 'None' — a template default is not authored + // content. The displaced Stopped At IS surfaced; the Resume File is not. + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), sessionFixture); + + const result = runGsdTools('state record-session --stopped-at "Phase 2, Plan 2"', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.replacedRecord['Stopped At'], 'Phase 2, Plan 1', + 'the authored Stopped At displacement is surfaced'); + assert.ok( + output.replacedRecord['Resume File'] === undefined, + `a template-default Resume File rewrite is not a displacement, got: ${result.output}`, + ); + }); + + test('#4763: an archive-section Stopped At displaced by the document-wide writer is surfaced', () => { + // The writer replaces the FIRST case-insensitive label match anywhere in + // the document; a Session Continuity Archive line can precede the live + // block. The capture mirrors the writer (document-wide), so the displaced + // archive record is surfaced instead of silently lost. + const withArchive = [ + '# Project State', + '', + '## Session Continuity Archive', + '', + '**Stopped at:** archived Phase 1 record', + '', + '## Session', + '', + '**Last session:** 2024-01-10', + '**Resume file:** None', + ].join('\n') + '\n'; + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), withArchive); + + const result = runGsdTools('state record-session --stopped-at "Phase 2"', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.replacedRecord['Stopped At'], + 'archived Phase 1 record', + 'the displaced archive line must be surfaced — the writer is document-wide', + ); + }); + + test('#4763: a wrapped multi-line Stopped At record is surfaced whole', () => { + // stateExtractField alone is first-line-only; the displaced record joins + // its continuation lines via stateFieldContinuation so a wrapped handoff + // is not truncated in the payload. + const wrapped = [ + '# Project State', + '', + '## Session', + '', + '**Last session:** 2024-01-10', + '**Stopped at:** Phase 2, Plan 1 — handoff:', + 'the verifier asked for a re-run of the failing probe', + '**Resume file:** None', + ].join('\n') + '\n'; + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), wrapped); + + const result = runGsdTools('state record-session --stopped-at "Phase 2, Plan 2"', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.replacedRecord['Stopped At'], + 'Phase 2, Plan 1 — handoff:\nthe verifier asked for a re-run of the failing probe', + 'the displaced record includes its continuation lines', + ); + }); + + test('#4763: a case-variant template-default Resume File rewrite is not a displacement', () => { + // Defaults match case-insensitively (#944 DWIM): 'none' -> 'None' is + // normalization of a template default, not displacement of authored content. + const lowerNone = sessionFixture.replace('**Resume file:** None', '**Resume file:** none'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), lowerNone); + + const result = runGsdTools('state record-session --stopped-at "Phase 2, Plan 2"', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.ok( + output.replacedRecord['Resume File'] === undefined, + `a case-variant template-default rewrite is not a displacement, got: ${result.output}`, + ); + }); + + // ── #4763 (2): the executor's decision loop must pass --phase explicitly. + // The #3231/#3481 pointer fallback stays for genuinely phase-less callers, + // but an in-scope caller relying on the global pointer is exactly the + // mis-attribution shape the issue measured (decisions landed on Phase 661 + // while phase 658 executed). + // + // allow-test-rule: source-text-is-the-product (#4763) — agents/gsd-executor.md + // is shipped content; its text IS the deployed contract the runtime loads. + test('#4763: the executor decision loop passes --phase explicitly', () => { + const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + const executor = fs.readFileSync( + path.join(__dirname, '..', 'agents', 'gsd-executor.md'), 'utf8'); + const addDecisionLines = splitLines(executor) + .filter((l) => l.includes('state.add-decision')); + assert.ok( + addDecisionLines.length >= 1, + 'agents/gsd-executor.md must carry the add-decision loop', + ); + for (const line of addDecisionLines) { + assert.match( + line, + /--phase\s+"\$\{PHASE\}"/, + `every add-decision invocation must pass --phase "${'${PHASE}'}": ${line.trim()}`, + ); + } + }); + + test('#4763 parity: the execute-plan add-decision call site also passes --phase', () => { + // Generative-fix divergence guard (CLAUDE.md): the two add-decision call + // surfaces share one contract; execute-plan.md was fixed first, and this + // pin keeps it from regressing while the executor catches up. + const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); + const plan = fs.readFileSync( + path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-plan.md'), 'utf8'); + const lines = splitLines(plan); + const idx = lines.findIndex((l) => l.includes('state.add-decision')); + assert.ok(idx !== -1, 'gsd-core/workflows/execute-plan.md must carry the add-decision call'); + // The invocation may continue over `\`-continued lines; the contract is on + // the whole call, not the first physical line. + const invocation = []; + for (let i = idx; i < lines.length && (i === idx || lines[i - 1].trimEnd().endsWith('\\')); i++) { + invocation.push(lines[i]); + } + assert.match( + invocation.join(' '), + /--phase\s+"\$\{PHASE\}"/, + 'the execute-plan add-decision invocation must pass --phase "${PHASE}"', + ); + }); + test('returns error when STATE.md missing', () => { // #4186: supply a value so this test keeps exercising the STATE.md-missing // decline rather than the (now earlier) no-args usage error.