Skip to content

Commit acd8b26

Browse files
committed
fix(replay): add resume.alternateFrom so caution/manual dual-path never advertises a --from the daemon refuses (#1262)
The dual-path text guidance offered `--from N + 1` whenever resuming AT `N` was allowed, but `--from N + 1` needs its OWN preflight — which additionally requires the diverged step `N` to be skip-safe. When `N` is a runScript (outputEnv producer) or inside runtime control flow, preflight(N) passes while preflight(N+1) fails, so the text advertised a command the daemon then refused (the text-vs-structured disagreement #1260 blocker 2 banned). - Add optional `resume.alternateFrom` to the decision-4 wire shape (`ReplayDivergenceResume`). The daemon populates it (`N + 1`) for caution/manual ONLY when `evaluateReplayResumePreflight({ from: N + 1 })` passes — the same acceptance condition on both the mid-plan (un-gated, in range) and last-step (watermark-stamp) paths. Its checked range is a strict superset of `from`'s, so alternateFrom present implies allowed. - The text renderer gates the `N + 1` command on `alternateFrom`'s PRESENCE and renders its value verbatim, never re-deriving resumability — so text and the structured wire can never disagree on the advertised next command. - This also closes a parity gap: a JSON/MCP caller now gets both ordinals (previously the dual-path was text-only, structured callers saw only `from`). - ADR-0012 decision-4 documents alternateFrom as additive/optional; projection and trigger tests cover both positions (runScript/control-flow → no alternateFrom, no `--from N + 1`; skip-safe → alternateFrom present).
1 parent 62e204c commit acd8b26

8 files changed

Lines changed: 383 additions & 60 deletions

File tree

‎docs/adr/0012-interactive-replay.md‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ executable plan and must be in range. It is never a YAML line number, fractional
475475
repeat iteration label. Static includes, platform conditions, and fixed-count repeats expand before
476476
indexing, so repeated source lines are distinguished by their plan index.
477477

478-
Every divergence includes `resume: { allowed, from, reason?, planDigest, repairSessionHeld? }`.
478+
Every divergence includes `resume: { allowed, from, reason?, planDigest, alternateFrom?, repairSessionHeld? }`.
479479
`from` is not merely the failed step's ordinal — it is the ordinal the caller should actually pass to
480480
`--from`, computed from the same `repairHint` carried alongside it (decision 6, R2/R3): for `record-and-heal`,
481481
`from` is the failed step's index **+ 1** (the agent performs that step manually before resuming, so
@@ -485,6 +485,22 @@ block and the `repairHint` text guidance below in agreement — a JSON/MCP-first
485485
at `resume.from` gets the same continuation a text caller reads out of the rendered guidance, never a stale
486486
`from` that loops the caller back onto the step it just repaired.
487487

488+
`alternateFrom` is an **additive optional** ordinal (#1262) that makes the `caution`/`manual` dual-path
489+
structured-caller-legible, not text-only. Those two hints have TWO legitimate repairs the daemon cannot
490+
disambiguate at divergence time: an app-state fix (`--no-record`, then re-run the unchanged step at `from` =
491+
`N`), and a record-and-heal-shaped correction (perform the diverged step's intent as a recorded action, then
492+
resume PAST it at `N + 1`). `from` carries the first; `alternateFrom` carries the second (`N + 1`), present
493+
**only when a `--from N + 1` request for this divergence would actually be accepted** — the daemon computes
494+
it as `evaluateReplayResumePreflight({ from: N + 1, actions }).allowed`, which additionally requires the
495+
diverged step `N` itself to be skip-safe (so it is absent when `N` is a `runScript` outputEnv producer or
496+
sits inside runtime control flow, where `--from N + 1` would be refused). Because that preflight's checked
497+
range is a strict superset of `from`'s, `alternateFrom` present implies `allowed: true` — it never
498+
contradicts the primary. Absent for `record-and-heal` (its `from` already IS the `N + 1` continuation) and
499+
`state-repair` (no recorded-action alternate). The `repairHint` text guidance renders the `N + 1` command
500+
**iff `alternateFrom` is present**, never re-deriving resumability client-side, so the text surface and the
501+
structured wire advertise the identical next command — closing both the text-vs-structured disagreement a
502+
client-side re-derivation would reintroduce and the parity gap where a structured caller saw only `from`.
503+
488504
If the shifted `from` equals `actions.length + 1` (the diverged step was the plan's LAST step), that is a
489505
legal EMPTY-TAIL resume, not an error: there is nothing left to replay, so the resumed run executes zero
490506
device actions and falls straight through to the normal end-of-plan completion path, correctly flipping an

‎src/daemon/handlers/__tests__/session-replay-repair-empty-tail.test.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,12 +219,15 @@ test('a manual divergence (unannotated action-failure) on the LAST step resumes
219219
const divergence = leg1.error.details?.divergence as {
220220
kind: string;
221221
repairHint: string;
222-
resume: { allowed: boolean; from: number; planDigest: string };
222+
resume: { allowed: boolean; from: number; planDigest: string; alternateFrom?: number };
223223
};
224224
expect(divergence.kind).toBe('action-failure');
225225
expect(divergence.repairHint).toBe('manual');
226226
expect(divergence.resume.allowed).toBe(true);
227227
expect(divergence.resume.from).toBe(2);
228+
// #1262: the wire carries the record-and-heal-shaped alternate (N + 1 = 3)
229+
// — the diverged step is skip-safe, so `--from 3` would be accepted.
230+
expect(divergence.resume.alternateFrom).toBe(3);
228231

229232
const session = sessionStore.get(sessionName)!;
230233
expect(session.actions.map((a) => a.command)).toEqual(['open']);
@@ -348,12 +351,14 @@ test('a caution (identity-mismatch) divergence on the LAST step resumes with an
348351
const divergence = leg1.error.details?.divergence as {
349352
kind: string;
350353
repairHint: string;
351-
resume: { allowed: boolean; from: number; planDigest: string };
354+
resume: { allowed: boolean; from: number; planDigest: string; alternateFrom?: number };
352355
};
353356
expect(divergence.kind).toBe('identity-mismatch');
354357
expect(divergence.repairHint).toBe('caution');
355358
expect(divergence.resume.allowed).toBe(true);
356359
expect(divergence.resume.from).toBe(2);
360+
// #1262: the wire carries the record-and-heal-shaped alternate (N + 1 = 3).
361+
expect(divergence.resume.alternateFrom).toBe(3);
357362

358363
const session = sessionStore.get(sessionName)!;
359364
expect(session.actions.map((a) => a.command)).toEqual(['open']);

‎src/daemon/handlers/__tests__/session-replay-resume.test.ts‎

Lines changed: 133 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,11 +138,14 @@ test('buildReplayDivergenceResume reports allowed:true with from/planDigest for
138138
action({ command: 'open', positionals: ['Demo'] }),
139139
action({ command: 'click', positionals: ['label="Save"'] }),
140140
];
141+
// `state-repair` carries no `alternateFrom` (no recorded-action alternate),
142+
// so this stays a clean base-shape assertion — the caution/manual
143+
// `alternateFrom` cases are covered by the dedicated tests below.
141144
const resume = buildReplayDivergenceResume({
142145
failedIndex: 2,
143146
actions,
144147
planDigest: 'abc123',
145-
repairHint: 'manual',
148+
repairHint: 'state-repair',
146149
});
147150
assert.deepEqual(resume, { allowed: true, from: 2, planDigest: 'abc123' });
148151
});
@@ -229,6 +232,135 @@ test('buildReplayDivergenceResume with repairHint record-and-heal still rejects
229232
assert.match(resume.reason, /control flow/);
230233
});
231234

235+
// --- buildReplayDivergenceResume: `resume.alternateFrom` (#1262). The
236+
// `caution`/`manual` dual-path's SECOND ordinal (`failedIndex + 1`), present
237+
// ONLY when a `--from failedIndex + 1` request would actually be accepted —
238+
// i.e. `evaluateReplayResumePreflight({ from: failedIndex + 1 })` passes,
239+
// which additionally requires the DIVERGED step itself to be skip-safe. This
240+
// closes the parity bug where the text renderer offered `--from N + 1` based
241+
// on `N`'s preflight (which does not check step `N`'s own skip-safety). ---
242+
243+
test('buildReplayDivergenceResume: caution mid-plan (skip-safe diverged step) carries alternateFrom = failedIndex + 1', () => {
244+
const actions: SessionAction[] = [
245+
action({ command: 'open' }),
246+
action({ command: 'click' }),
247+
action({ command: 'click' }),
248+
];
249+
const resume = buildReplayDivergenceResume({
250+
failedIndex: 2,
251+
actions,
252+
planDigest: 'abc123',
253+
repairHint: 'caution',
254+
});
255+
assert.equal(resume.allowed, true);
256+
assert.equal(resume.from, 2); // unshifted
257+
if (!resume.allowed) return;
258+
assert.equal(resume.alternateFrom, 3);
259+
});
260+
261+
test('buildReplayDivergenceResume: manual last-step (skip-safe diverged step) carries alternateFrom = actions.length + 1', () => {
262+
const actions: SessionAction[] = [action({ command: 'open' }), action({ command: 'click' })];
263+
const resume = buildReplayDivergenceResume({
264+
failedIndex: 2,
265+
actions,
266+
planDigest: 'abc123',
267+
repairHint: 'manual',
268+
});
269+
assert.equal(resume.allowed, true);
270+
assert.equal(resume.from, 2);
271+
if (!resume.allowed) return;
272+
assert.equal(resume.alternateFrom, 3); // empty-tail ordinal
273+
});
274+
275+
test('buildReplayDivergenceResume: caution whose diverged step is a runScript carries NO alternateFrom (mid-plan AND last-step)', () => {
276+
// The `N + 1` alternate would skip the runScript (outputEnv producer) at
277+
// step N, which `evaluateReplayResumePreflight({ from: N + 1 })` refuses —
278+
// so alternateFrom is absent even though resuming AT N stays allowed.
279+
const midPlan: SessionAction[] = [
280+
action({ command: 'open' }),
281+
action({ command: '__maestroRunScript', positionals: ['./setup.js'] }),
282+
action({ command: 'click' }),
283+
];
284+
const midPlanResume = buildReplayDivergenceResume({
285+
failedIndex: 2,
286+
actions: midPlan,
287+
planDigest: 'abc123',
288+
repairHint: 'caution',
289+
});
290+
assert.equal(midPlanResume.allowed, true); // resuming AT 2 is fine
291+
assert.equal(midPlanResume.from, 2);
292+
if (!midPlanResume.allowed) return;
293+
assert.equal(midPlanResume.alternateFrom, undefined);
294+
295+
const lastStep: SessionAction[] = [
296+
action({ command: 'open' }),
297+
action({ command: '__maestroRunScript', positionals: ['./setup.js'] }),
298+
];
299+
const lastStepResume = buildReplayDivergenceResume({
300+
failedIndex: 2,
301+
actions: lastStep,
302+
planDigest: 'abc123',
303+
repairHint: 'caution',
304+
});
305+
assert.equal(lastStepResume.allowed, true);
306+
assert.equal(lastStepResume.from, 2);
307+
if (!lastStepResume.allowed) return;
308+
assert.equal(lastStepResume.alternateFrom, undefined);
309+
});
310+
311+
test('buildReplayDivergenceResume: manual whose diverged step is inside runtime control flow carries NO alternateFrom (mid-plan AND last-step)', () => {
312+
// A control-flow diverged step is not even resumable AT (you cannot resume
313+
// INTO a retry/runFlowWhen wrapper), so `resume.allowed` is false and the
314+
// `allowed: false` shape has no `alternateFrom` field at all — the key
315+
// invariant is that no `N + 1` alternate is ever advertised for it.
316+
const controlStep = () =>
317+
action({
318+
command: 'back',
319+
positionals: [],
320+
replayControl: { kind: 'retry', maxRetries: 1, actions: [action({ command: 'back' })] },
321+
});
322+
const midPlan: SessionAction[] = [
323+
action({ command: 'open' }),
324+
controlStep(),
325+
action({ command: 'click' }),
326+
];
327+
const midPlanResume = buildReplayDivergenceResume({
328+
failedIndex: 2,
329+
actions: midPlan,
330+
planDigest: 'abc123',
331+
repairHint: 'manual',
332+
});
333+
assert.equal('alternateFrom' in midPlanResume, false);
334+
335+
const lastStep: SessionAction[] = [action({ command: 'open' }), controlStep()];
336+
const lastStepResume = buildReplayDivergenceResume({
337+
failedIndex: 2,
338+
actions: lastStep,
339+
planDigest: 'abc123',
340+
repairHint: 'manual',
341+
});
342+
assert.equal('alternateFrom' in lastStepResume, false);
343+
});
344+
345+
test('buildReplayDivergenceResume: record-and-heal and state-repair never carry alternateFrom (no separate recorded-action alternate)', () => {
346+
const actions: SessionAction[] = [
347+
action({ command: 'open' }),
348+
action({ command: 'click' }),
349+
action({ command: 'click' }),
350+
];
351+
for (const repairHint of ['record-and-heal', 'state-repair'] as const) {
352+
const resume = buildReplayDivergenceResume({
353+
failedIndex: 2,
354+
actions,
355+
planDigest: 'abc123',
356+
repairHint,
357+
});
358+
assert.equal(resume.allowed, true);
359+
if (!resume.allowed) return;
360+
assert.equal(resume.alternateFrom, undefined, `expected no alternateFrom for ${repairHint}`);
361+
}
362+
});
363+
232364
// --- stampPendingRecordAndHealWatermark (#1262): the watermark is now ALSO
233365
// stamped for `caution`/`manual`, but ONLY for the LAST-step empty-tail
234366
// alternate (`failedIndex === actions.length`, targeting `failedIndex + 1`).

‎src/daemon/handlers/__tests__/session-replay-runtime-failure.test.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,23 @@ test('a failing replay step returns REPLAY_DIVERGENCE with cause preserved and c
8282

8383
expect(divergence.suggestions).toEqual([]);
8484
expect(divergence.suggestionCount).toBe(0);
85-
const resume = divergence.resume as { allowed: boolean; from: number; planDigest: string };
85+
const resume = divergence.resume as {
86+
allowed: boolean;
87+
from: number;
88+
planDigest: string;
89+
alternateFrom?: number;
90+
};
8691
// Step 1 ("open") is a plain action with no control flow and no outputEnv
87-
// production, so resuming at the failed step (2) is safe.
88-
expect(resume).toEqual({ allowed: true, from: 2, planDigest: expect.any(String) });
92+
// production, so resuming at the failed step (2) is safe. This unannotated
93+
// action-failure routes to `manual`, and the diverged step (2) is the plan's
94+
// LAST and is itself skip-safe, so the #1262 record-and-heal-shaped alternate
95+
// ordinal (`alternateFrom = 3`) rides the wire alongside the unshifted `from`.
96+
expect(resume).toEqual({
97+
allowed: true,
98+
from: 2,
99+
planDigest: expect.any(String),
100+
alternateFrom: 3,
101+
});
89102
expect(resume.planDigest).toMatch(/^[0-9a-f]{64}$/);
90103
});
91104

‎src/daemon/handlers/session-replay-resume.ts‎

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,10 @@ function producesOutputEnv(action: SessionAction): boolean {
8484
* COMPLETE. Rejecting it here would send the agent to `close` instead —
8585
* which discards the just-recorded corrective action, since commit is gated
8686
* on that same COMPLETE flag.
87+
*
88+
* `alternateFrom` (#1262) is the `caution`/`manual` dual-path's SECOND
89+
* ordinal (`failedIndex + 1`, the record-and-heal-shaped alternate) — see
90+
* `computeReplayResumeAlternateFrom`.
8791
*/
8892
export function buildReplayDivergenceResume(params: {
8993
failedIndex: number; // 1-based
@@ -94,9 +98,47 @@ export function buildReplayDivergenceResume(params: {
9498
const { failedIndex, actions, planDigest, repairHint } = params;
9599
const from = repairHint === 'record-and-heal' ? failedIndex + 1 : failedIndex;
96100
const preflight = evaluateReplayResumePreflight({ from, actions });
97-
return preflight.allowed
98-
? { allowed: true, from, planDigest }
99-
: { allowed: false, from, planDigest, reason: preflight.reason };
101+
if (!preflight.allowed) return { allowed: false, from, planDigest, reason: preflight.reason };
102+
const alternateFrom = computeReplayResumeAlternateFrom({ failedIndex, actions, repairHint });
103+
return {
104+
allowed: true,
105+
from,
106+
planDigest,
107+
...(alternateFrom !== undefined ? { alternateFrom } : {}),
108+
};
109+
}
110+
111+
/**
112+
* ADR 0012 decision 4 / #1262: the `caution`/`manual` dual-path's SECOND
113+
* ordinal, the record-and-heal-shaped alternate (`failedIndex + 1`). Returned
114+
* ONLY when a `--from failedIndex + 1` request for this divergence would
115+
* actually be accepted by the daemon — i.e. exactly its own resume preflight
116+
* passes (`evaluateReplayResumePreflight`, which unlike `from`'s own preflight
117+
* ALSO requires the DIVERGED step to be skip-safe, since reaching
118+
* `failedIndex + 1` skips the diverged step). That preflight is the shared
119+
* acceptance condition on BOTH the mid-plan path (un-gated, in range) and the
120+
* last-step empty-tail path (`computeRecordAndHealWatermark` stamps on exactly
121+
* this condition). Its checked range is a strict superset of `from`'s, so
122+
* `alternateFrom` present implies `resume.allowed` — never a contradiction.
123+
*
124+
* Absent for `record-and-heal` (its `from` already IS `failedIndex + 1`) and
125+
* `state-repair` (no recorded-action alternate); absent when the diverged step
126+
* is a `runScript` (outputEnv producer) or inside runtime control flow, so the
127+
* alternate `--from failedIndex + 1` would be refused. The text renderer gates
128+
* the `N + 1` command on this field's PRESENCE rather than re-deriving
129+
* resumability, keeping text and the structured wire in agreement.
130+
*/
131+
function computeReplayResumeAlternateFrom(params: {
132+
failedIndex: number;
133+
actions: SessionAction[];
134+
repairHint: ReplayRepairHint;
135+
}): number | undefined {
136+
const { failedIndex, actions, repairHint } = params;
137+
if (repairHint !== 'caution' && repairHint !== 'manual') return undefined;
138+
const alternateFrom = failedIndex + 1;
139+
return evaluateReplayResumePreflight({ from: alternateFrom, actions }).allowed
140+
? alternateFrom
141+
: undefined;
100142
}
101143

102144
/**

‎src/mcp/__tests__/command-tools.test.ts‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1089,6 +1089,53 @@ test('MCP tool error is a ref-issuing result: isError, structuredContent, and pi
10891089
assert.deepEqual(runCalls[1]?.input, { target: { kind: 'ref', ref: '@e5~s12' } });
10901090
});
10911091

1092+
// --- #1262: a `caution` divergence's dual-path must reach a structured caller,
1093+
// not only text. `resume.alternateFrom` rides the MCP structuredContent
1094+
// projection (parity with a text caller, who reads the second `--from`
1095+
// command), and the MCP text carries BOTH concrete commands. ---
1096+
test("MCP projections carry a caution divergence's alternateFrom (structuredContent) and both --from commands (text)", async () => {
1097+
const executor = createCommandToolExecutor({
1098+
createClient: () => ({}) as AgentDeviceClient,
1099+
runCommand: async (_client, name) => {
1100+
if (name === 'replay') {
1101+
throw new AppError('REPLAY_DIVERGENCE', 'Replay diverged at step 2 (click label="Save")', {
1102+
divergence: {
1103+
version: 1,
1104+
kind: 'identity-mismatch',
1105+
step: { index: 2, source: { path: '/tmp/flow.ad', line: 3 } },
1106+
action: 'click label="Save"',
1107+
cause: { code: 'IDENTITY_MISMATCH', message: 'resolved a different element' },
1108+
screen: {
1109+
state: 'available',
1110+
refsGeneration: 4,
1111+
refs: [{ ref: 'e1', role: 'button' }],
1112+
},
1113+
suggestions: [],
1114+
suggestionCount: 0,
1115+
resume: { allowed: true, from: 2, planDigest: 'deadbeef', alternateFrom: 3 },
1116+
repairHint: 'caution',
1117+
},
1118+
});
1119+
}
1120+
return {};
1121+
},
1122+
});
1123+
1124+
const result = await executor.execute('replay', { positionals: ['/tmp/flow.ad'] });
1125+
assert.equal(result.isError, true);
1126+
const structured = result.structuredContent as {
1127+
details?: { divergence?: { resume?: { from?: number; alternateFrom?: number } } };
1128+
};
1129+
// Parity: the structured caller gets BOTH ordinals, not just `from`.
1130+
assert.equal(structured.details?.divergence?.resume?.from, 2);
1131+
assert.equal(structured.details?.divergence?.resume?.alternateFrom, 3);
1132+
// The MCP text carries both concrete commands, the second from alternateFrom.
1133+
const text = result.content[0]?.text ?? '';
1134+
assert.match(text, /Repair hint: caution/);
1135+
assert.match(text, /if you fixed app state with --no-record actions: replay --from 2/);
1136+
assert.match(text, /if you performed the step's intent as a recorded action: replay --from 3/);
1137+
});
1138+
10921139
test('MCP tool error without a divergence never touches existing pins (merge-only)', async () => {
10931140
const runCalls: Array<{ name: string; input: unknown }> = [];
10941141
const executor = createCommandToolExecutor({

0 commit comments

Comments
 (0)