Skip to content

Commit c1bf116

Browse files
committed
fix(daemon): add sessionActive to MCP schema, real-producer tests, ADR fix
Addresses the second review pass on #1390: - MCP replay output schema (src/mcp/command-output-schemas.ts) omitted the new required sessionActive field entirely; add it. - ADR 0016 still claimed "the absence of close changes ... nor the success response shape", contradicting the new required field this PR adds. Amend it to document the sessionActive contract and why the real CLI/IPC client needs it (issue #1384). - All prior lifecycle tests exercised sessionActive only through a fake HTTP response in the client-layer tests, so deleting either real producer line (session-replay-runtime.ts, session-replay- maestro-response.ts) would not have failed anything. Add tests against the real runReplayScriptFile producer (native .ad close-less -> true, terminal close -> false, Maestro close-less -> true) and strengthen the provider-scenario (real daemon route) test with the same assertion. Verified each new test fails when its corresponding producer line is reverted, then restored. Live-validated the fix on real backends (booted iOS 16 simulator and a running Android Pixel 9 Pro XL emulator), replaying issue #1384's exact repro end to end: the owning daemon and its session both survive a close-less replay and remain fully addressable via the hinted --state-dir/--session on both platforms. That validation surfaced a separate, pre-existing bug -- `session list` (no explicit --session) omits cwd-scoped sessions opened via a replay's internal `open` dispatch, because session-open.ts's resolveImplicitSessionScope(req) sees a different req than the top-level replay request and leaves session.sessionScope unset -- filed as #1394, out of scope here since it is a sessionScope-propagation gap unrelated to the client-side teardown this PR fixes; the session itself is never actually lost.
1 parent 7631235 commit c1bf116

4 files changed

Lines changed: 103 additions & 3 deletions

File tree

‎docs/adr/0016-active-session-script-publication.md‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,16 @@ enough for this boundary.
107107
On consumption, a script without `close` preserves the existing replay behavior: the named session stays
108108
active and the successful `ReplayCommandResult` returns its `session` id. The caller binds subsequent
109109
commands to that returned id. Replay reports success only after the destination guard completes; the
110-
absence of `close` changes neither action dispatch nor the success response shape.
110+
absence of `close` changes neither action dispatch nor when replay reports success.
111+
112+
**Response shape (issue #1384).** `ReplayCommandResult` carries a required `sessionActive: boolean`,
113+
computed from whether `session` still exists in the daemon's own store when the response is built — never
114+
by re-parsing the script for `close`. This is what lets the real CLI/IPC client (not just the in-process
115+
handler) actually honor "the session stays active": without an explicit signal in the response, the
116+
client's one-shot `replay`/`test` teardown had no way to distinguish a still-active handoff from a
117+
finished one, and tore the owning daemon down regardless (`src/daemon/client/daemon-client-lifecycle.ts`).
118+
`sessionActive` is `true` for every close-less run (including a `--from` resume) and `false` once the
119+
script's terminal `close` — or a repair-armed run's deferred equivalent — has executed.
111120

112121
### Sensitive inputs
113122

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

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,61 @@ test('a successful replay prints one line with the step count and wall time', as
4545
expect(data.message).toMatch(/^Replayed 2 steps in \d+\.\ds$/);
4646
});
4747

48+
// --- ADR 0016 / issue #1384: `sessionActive` is derived from the REAL
49+
// producer (`completeReplayRun`'s `sessionStore.get(sessionName)` check), not
50+
// asserted against a hand-crafted fixture — deleting that line would fail
51+
// these, unlike the client-lifecycle tests in
52+
// `src/utils/__tests__/daemon-client-lifecycle.test.ts`, which only prove the
53+
// CLIENT'S reaction to a `sessionActive` value it is handed. ---
54+
55+
test('a close-less replay reports sessionActive: true (real producer, session still in the store)', async () => {
56+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replay-session-active-'));
57+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
58+
const sessionName = 'default';
59+
sessionStore.set(sessionName, makeIosSession(sessionName));
60+
const filePath = writeReplayFile(root, ['open "Demo"', 'click "Save"']);
61+
62+
const response = await runReplayScriptFile({
63+
req: baseReq({ positionals: [filePath] }),
64+
sessionName,
65+
logPath: path.join(root, 'daemon.log'),
66+
sessionStore,
67+
invoke: async () => ({ ok: true, data: {} }),
68+
});
69+
70+
expect(response.ok).toBe(true);
71+
if (!response.ok) return;
72+
expect(sessionStore.get(sessionName)).toBeDefined();
73+
expect((response.data as { sessionActive: boolean }).sessionActive).toBe(true);
74+
});
75+
76+
test('a replay whose terminal close removes the session reports sessionActive: false', async () => {
77+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replay-session-closed-'));
78+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
79+
const sessionName = 'default';
80+
sessionStore.set(sessionName, makeIosSession(sessionName));
81+
const filePath = writeReplayFile(root, ['open "Demo"', 'close']);
82+
83+
const response = await runReplayScriptFile({
84+
req: baseReq({ positionals: [filePath] }),
85+
sessionName,
86+
logPath: path.join(root, 'daemon.log'),
87+
sessionStore,
88+
// Mirrors the one real effect of `handleCloseCommand` this test needs:
89+
// removing the session from the store. Every other command is a no-op,
90+
// same as the rest of this file's `invoke` stubs.
91+
invoke: async (req) => {
92+
if (req.command === 'close') sessionStore.delete(sessionName);
93+
return { ok: true, data: {} };
94+
},
95+
});
96+
97+
expect(response.ok).toBe(true);
98+
if (!response.ok) return;
99+
expect(sessionStore.get(sessionName)).toBeUndefined();
100+
expect((response.data as { sessionActive: boolean }).sessionActive).toBe(false);
101+
});
102+
48103
test('Maestro YAML uses the typed engine while .ad remains generic', async () => {
49104
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-typed-maestro-route-'));
50105
const sessionStore = new SessionStore(path.join(root, 'sessions'));
@@ -94,6 +149,31 @@ test('Maestro YAML uses the typed engine while .ad remains generic', async () =>
94149
expect(commands).toEqual(['open']);
95150
});
96151

152+
test('ADR 0016 / #1384: a Maestro replay reports sessionActive: true (real producer, no fake HTTP)', async () => {
153+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-maestro-session-active-'));
154+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
155+
const sessionName = 'default';
156+
sessionStore.set(sessionName, makeIosSession(sessionName));
157+
const yamlPath = path.join(root, 'flow.yaml');
158+
fs.writeFileSync(yamlPath, 'appId: com.example.app\n---\n- launchApp\n');
159+
160+
const response = await runReplayScriptFile({
161+
req: baseReq({
162+
positionals: [yamlPath],
163+
flags: { replayBackend: 'maestro', platform: 'ios' },
164+
}),
165+
sessionName,
166+
logPath: path.join(root, 'daemon.log'),
167+
sessionStore,
168+
invoke: async () => ({ ok: true, data: {} }),
169+
});
170+
171+
expect(response.ok).toBe(true);
172+
if (!response.ok) return;
173+
expect(sessionStore.get(sessionName)).toBeDefined();
174+
expect((response.data as { sessionActive: boolean }).sessionActive).toBe(true);
175+
});
176+
97177
test('typed Maestro nested commands receive the runtime hints bound into the plan', async () => {
98178
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-maestro-runtime-envelope-'));
99179
const sessionStore = new SessionStore(path.join(root, 'sessions'));

‎src/mcp/command-output-schemas.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -607,11 +607,14 @@ export const COMMAND_OUTPUT_SCHEMAS = {
607607
replayed: numberSchema(),
608608
healed: numberSchema(),
609609
session: stringSchema(),
610+
sessionActive: booleanSchema(
611+
'True iff the session is still active — the script had no terminal close.',
612+
),
610613
artifactPaths: stringArraySchema,
611614
snapshotDiagnostics: looseObjectSchema(),
612615
message: stringSchema(),
613616
},
614-
['replayed', 'healed', 'session', 'artifactPaths', 'message'],
617+
['replayed', 'healed', 'session', 'sessionActive', 'artifactPaths', 'message'],
615618
),
616619
test: objectSchema(
617620
{

‎test/integration/provider-scenarios/active-session-script-publication.test.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,15 @@ test('provider route publishes and replays an open-to-destination script with a
3434
await client.sessions.close();
3535

3636
const replay = await world.daemon.callCommand('replay', [scriptPath], world.selection);
37-
assert.equal(assertRpcOk<{ session?: string }>(replay).session, 'default');
37+
const replayData = assertRpcOk<{ session?: string; sessionActive?: boolean }>(replay);
38+
assert.equal(replayData.session, 'default');
39+
// ADR 0016 / #1384: the published script has no terminal `close`, so the
40+
// real (not test-fixture-injected) daemon response reports the session
41+
// still active — this is the exact producer line the client-lifecycle
42+
// tests in daemon-client-lifecycle.test.ts assume but cannot themselves
43+
// exercise, since they inject sessionActive via a fake HTTP fixture.
44+
assert.equal(replayData.sessionActive, true);
45+
assert.ok(world.daemon.session(), 'a close-less replay must not tear down the session');
3846
const replaySnapshot = await client.capture.snapshot({ interactiveOnly: true });
3947
assert.ok(replaySnapshot.nodes.some((node) => node.label === 'Search'));
4048
await client.sessions.close();

0 commit comments

Comments
 (0)