fix(#4763): surface the displaced session record and pass --phase from the executor decision loop (#4919)
* test(#4763): failing-first — replaced-record payload and executor --phase pins * fix(#4763): surface the displaced session record and pass --phase from the executor decision loop state record-session keeps its last-writer-wins write (the recorded single-slot handoff design) but no longer displaces silently: when a non-empty Stopped At or authored Resume File record is replaced, the payload carries the full prior text under replacedRecord. Same-value rewrites, the insert path, and the #944 template-default DWIM are not displacements and report nothing. The executor decision loop now passes --phase "${PHASE}" to state.add-decision, matching execute-plan.md, so decisions stop inheriting whichever phase the global pointer names (#4763 case 2). advance-plan is unchanged (#3311 by-design). Emitted-Drift-Ack-Growth: gsd-executor.md — the decision loop gained its --phase guard and a comment naming why (#4763) * docs(#4763): add the changeset fragment * docs(#4763): backfill the changeset PR number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/merry-jays-frolic.md
Normal file
5
.changeset/merry-jays-frolic.md
Normal file
@@ -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)
|
||||
@@ -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)
|
||||
|
||||
@@ -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<string, unknown> = { 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<string, string> = {};
|
||||
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
|
||||
|
||||
@@ -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 <subcommand>` CLI surface descriptively ("CLI invocations go through..."); not an agent instruction' },
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user