Skip to content

Commit 4fa09b2

Browse files
committed
fix: review follow-ups for #1558 (help text, test strength, docs)
- Give replay --save-script its own help text instead of the shared open/close "arm on open, publish on close" description: replay's flag arms an ADR 0012 repair transaction, a disjoint lifecycle. Adds CommandSchema.flagDescriptionOverrides so a command can swap a shared flag's usageDescription without duplicating the FlagDefinition entry (which would have shown --save-script twice in `help replay`). Pinned in src/cli/parser/__tests__/cli-help-command-usage.test.ts (open/close keep the shared text unchanged; replay gets the new one). - Strengthen the never-armed close --save-script regression test in session-close-shutdown.test.ts: the fixture now carries real cleanup-bearing state (an active iOS simulator recording, reusing makeIosSimulatorRecordingSession/recordingKillMock) with spies proving no teardown hook (recorder kill, runner stop) runs on the rejected request, then that a follow-up plain close does tear it down. The prior fixture had nothing for teardown to observably touch, so moving the guard after stopBestEffortSessionResources would have passed it silently. Also fixes a latent test-isolation leak this exposed: an earlier test set a persistent mockStopIosRunnerSession rejection (vi.clearAllMocks() clears call history, not implementations), which would have poisoned any later Apple-platform close test; scoped it to mockRejectedValueOnce. - Point the migration guide (website/docs/docs/migrating-gestures.md) at `open --save-script` → interact → `close` instead of the now-rejected `open` → interact → `close --save-script`, matching the new guard and the corrected help text. _Generated by [Claude Code](https://claude.ai/code)_
1 parent 316df87 commit 4fa09b2

7 files changed

Lines changed: 99 additions & 20 deletions

File tree

‎src/cli-schema/types.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ export type CommandSchema = {
1111
defaults?: Partial<CliFlags>;
1212
usageOverride?: string;
1313
listUsageOverride?: string;
14+
// Swaps a shared flag's usageDescription for this command only, when the flag's generic
15+
// documentation (flag-definitions-*.ts) does not fit every command it is allowed on — for
16+
// example `--save-script` arms authoring on open/close but a repair transaction on replay.
17+
flagDescriptionOverrides?: Partial<Record<FlagKey, string>>;
1418
};
1519

1620
export type CommandSchemaOverride = Partial<CommandSchema>;

‎src/cli/parser/__tests__/cli-help-command-usage.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -407,3 +407,35 @@ test('removed trigger aliases are no longer documented as commands', async () =>
407407
const help = await usageForCommand('trigger-screenshot-notification');
408408
assert.equal(help, null);
409409
});
410+
411+
// #1558 follow-up: replay's --save-script arms an ADR 0012 repair transaction, not the
412+
// open/close authoring lifecycle (arm-on-open, publish-on-close) the shared flag describes.
413+
// `flagDescriptionOverrides` (CommandSchema) swaps the text for replay only; open/close keep the
414+
// shared FlagDefinition description unchanged.
415+
test('replay --save-script help describes arming a repair transaction, not open/close authoring', async () => {
416+
const help = await usageForCommand('replay');
417+
if (help === null) throw new Error('Expected replay help text');
418+
assert.match(help, /Arm a repair transaction from this replay \(ADR 0012\)/);
419+
assert.match(help, /commits when the repair-armed session tears down/);
420+
assert.doesNotMatch(help, /Arm evidence capture on open, publish the armed recording on close/);
421+
});
422+
423+
test('open --save-script help keeps the shared open\\/close authoring description', async () => {
424+
const help = await usageForCommand('open');
425+
if (help === null) throw new Error('Expected open help text');
426+
assert.match(
427+
help,
428+
/Arm evidence capture on open, publish the armed recording on close; close --save-script alone \(without an armed open\) is rejected/,
429+
);
430+
assert.doesNotMatch(help, /Arm a repair transaction from this replay/);
431+
});
432+
433+
test('close --save-script help keeps the shared open\\/close authoring description', async () => {
434+
const help = await usageForCommand('close');
435+
if (help === null) throw new Error('Expected close help text');
436+
assert.match(
437+
help,
438+
/Arm evidence capture on open, publish the armed recording on close; close --save-script alone \(without an armed open\) is rejected/,
439+
);
440+
assert.doesNotMatch(help, /Arm a repair transaction from this replay/);
441+
});

‎src/cli/parser/cli-help.ts‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1243,6 +1243,21 @@ function listHelpFlags(keys: ReadonlySet<FlagKey>): FlagDefinition[] {
12431243
);
12441244
}
12451245

1246+
// Command-specific override for a shared flag's help text (see CommandSchema.flagDescriptionOverrides):
1247+
// keeps the FlagDefinition registry as one shared row per flag while letting a command whose
1248+
// semantics genuinely differ (e.g. `replay --save-script` arms a repair transaction, not the
1249+
// open/close authoring lifecycle) show its own description without duplicating the flag entry.
1250+
function applyFlagDescriptionOverrides(
1251+
definitions: FlagDefinition[],
1252+
overrides: Partial<Record<FlagKey, string>> | undefined,
1253+
): FlagDefinition[] {
1254+
if (!overrides) return definitions;
1255+
return definitions.map((definition) => {
1256+
const override = overrides[definition.key];
1257+
return override === undefined ? definition : { ...definition, usageDescription: override };
1258+
});
1259+
}
1260+
12461261
function renderFlagSection(title: string, definitions: FlagDefinition[]): string {
12471262
return renderAlignedSection(
12481263
title,
@@ -1302,7 +1317,10 @@ export function buildCommandUsageText(commandName: string): string | null {
13021317
const schema = getCommandSchema(commandName);
13031318
if (!schema) return null;
13041319
const usage = buildCommandUsage(commandName, schema);
1305-
const commandFlags = listHelpFlags(new Set<FlagKey>(schema.allowedFlags ?? []));
1320+
const commandFlags = applyFlagDescriptionOverrides(
1321+
listHelpFlags(new Set<FlagKey>(schema.allowedFlags ?? [])),
1322+
schema.flagDescriptionOverrides,
1323+
);
13061324
const sections: string[] = [];
13071325
if (commandFlags.length > 0) {
13081326
sections.push(renderFlagSection('Command flags:', commandFlags));

‎src/commands/command-explain.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -280,22 +280,26 @@ function describeCliSurface(command: string, schema: CommandSchema): CommandExpl
280280
return {
281281
usage: buildCommandUsage(command, schema),
282282
positionalArgs: schema.positionalArgs ?? [],
283-
commandFlags: describeFlags(schema.allowedFlags ?? []),
284-
supportedFlags: describeFlags(schema.supportedFlags ?? []),
283+
commandFlags: describeFlags(schema.allowedFlags ?? [], schema.flagDescriptionOverrides),
284+
supportedFlags: describeFlags(schema.supportedFlags ?? [], schema.flagDescriptionOverrides),
285285
globalFlags: describeFlags([...GLOBAL_FLAG_KEYS]),
286286
};
287287
}
288288

289-
function describeFlags(keys: readonly FlagKey[]): CommandFlagExplanation[] {
289+
function describeFlags(
290+
keys: readonly FlagKey[],
291+
overrides?: Partial<Record<FlagKey, string>>,
292+
): CommandFlagExplanation[] {
290293
return [...new Set(keys)].sort().map((key) => {
291294
const definitions = flagDefinitionsByKey.get(key) ?? [];
292295
const preferred = definitions.find((definition) => definition.usageLabel) ?? definitions[0];
296+
const description = overrides?.[key] ?? preferred?.usageDescription;
293297
return {
294298
key,
295299
syntax:
296300
preferred?.usageLabel ??
297301
[...new Set(definitions.flatMap((definition) => definition.names))].join('/'),
298-
...(preferred?.usageDescription ? { description: preferred.usageDescription } : {}),
302+
...(description ? { description } : {}),
299303
};
300304
});
301305
}

‎src/commands/replay/index.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,13 @@ const replayCliSchema = {
112112
'saveScript',
113113
'force',
114114
],
115+
// ADR 0012 decision 6: on replay, --save-script arms a repair transaction from step 1 (not the
116+
// open/close authoring lifecycle the shared flag description documents) and the healed script
117+
// commits on that transaction's own teardown, not on a plain close.
118+
flagDescriptionOverrides: {
119+
saveScript:
120+
'Arm a repair transaction from this replay (ADR 0012): recording starts at step 1, and the healed script commits when the repair-armed session tears down (close, close --save-script, or idle-reap). Independent of the open/close authoring arm-on-open. Optional custom output path.',
121+
},
115122
} as const satisfies CommandSchemaOverride;
116123

117124
const testCliSchema = {

‎src/daemon/handlers/__tests__/session-close-shutdown.test.ts‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1097,7 +1097,11 @@ test('targeted close skips platform dispatch and preserves the error when the re
10971097
reason: 'runner_stop_failed',
10981098
hint: 'Retry once the runner is reachable.',
10991099
});
1100-
mockStopIosRunnerSession.mockRejectedValue(preCloseError);
1100+
// Scoped to this test's own two internal calls (pre-close stop, then the later
1101+
// independent-cleanup retry) — a persistent `mockRejectedValue` here would leak into every
1102+
// later test that exercises an Apple-platform runner stop, since `vi.clearAllMocks()` in
1103+
// `beforeEach` clears call history but not a mock's implementation.
1104+
mockStopIosRunnerSession.mockRejectedValueOnce(preCloseError);
11011105

11021106
await expect(
11031107
handleSessionCommands({
@@ -1140,18 +1144,16 @@ test('targeted close skips platform dispatch and preserves the error when the re
11401144
// can retry, and no script file is written.
11411145
test('close --save-script on a never-armed session is rejected before teardown, with no script written', async () => {
11421146
const sessionStore = makeSessionStore();
1143-
const sessionName = 'android-unarmed-close-save-script-session';
1147+
const sessionName = 'ios-unarmed-close-save-script-session';
11441148
const scriptPath = path.join(os.tmpdir(), `agent-device-unarmed-close-${Date.now()}.ad`);
1145-
sessionStore.set(
1146-
sessionName,
1147-
makeSession(sessionName, {
1148-
platform: 'android',
1149-
id: 'emulator-5554',
1150-
name: 'Pixel_9_API_35',
1151-
kind: 'emulator',
1152-
booted: true,
1153-
}),
1154-
);
1149+
// The fixture must carry real cleanup-bearing state (here: an active recording, like
1150+
// `makeIosSimulatorRecordingSession`'s other consumers) so this test can actually prove the
1151+
// guard runs *before* `stopBestEffortSessionResources` — not just that the response rejects.
1152+
// Without it, moving the guard after teardown would still pass: there would be nothing for
1153+
// teardown to observably touch.
1154+
const session = makeIosSimulatorRecordingSession(sessionName);
1155+
const kill = recordingKillMock(session);
1156+
sessionStore.set(sessionName, session);
11551157

11561158
await expect(
11571159
handleSessionCommands({
@@ -1178,8 +1180,15 @@ test('close --save-script on a never-armed session is rejected before teardown,
11781180
// The rejection does not tear down the session — it stays retryable/recoverable.
11791181
expect(sessionStore.get(sessionName)).toBeDefined();
11801182
expect(fs.existsSync(scriptPath)).toBe(false);
1181-
1182-
// A plain close (no --save-script) still closes the same session cleanly afterward.
1183+
// No teardown hook ran: the recording is still live (recorder never signaled) and the runner
1184+
// was never told to stop. This is the assertion that goes red if the guard moves after
1185+
// `stopBestEffortSessionResources` — see the counterfactual in the PR description.
1186+
expect(kill).not.toHaveBeenCalled();
1187+
expect(session.recording).toBeDefined();
1188+
expect(mockStopIosRunnerSession).not.toHaveBeenCalled();
1189+
1190+
// A plain close (no --save-script) still closes the same session cleanly afterward, and now
1191+
// teardown genuinely does run: the recorder is signaled and the session deleted.
11831192
const plainClose = await handleSessionCommands({
11841193
req: {
11851194
token: 't',
@@ -1194,6 +1203,9 @@ test('close --save-script on a never-armed session is rejected before teardown,
11941203
invoke: noopInvoke,
11951204
});
11961205
expect(plainClose?.ok).toBe(true);
1206+
expect(kill).toHaveBeenCalledWith('SIGINT');
1207+
expect(session.recording).toBeUndefined();
1208+
expect(mockStopIosRunnerSession).toHaveBeenCalledWith(session.device.id);
11971209
expect(sessionStore.get(sessionName)).toBeUndefined();
11981210
});
11991211

‎website/docs/docs/migrating-gestures.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,9 @@ grep -rnE "\\bgesture[[:space:]]+rotate([[:space:]]+$num){4}" --include='*.ad' .
148148
```
149149

150150
Re-recording also produces a migrated script: the recorder writes the canonical form, so a fresh
151-
`open` → interact → `close --save-script` run is a valid alternative to editing by hand.
151+
`open --save-script` → interact → `close` run is a valid alternative to editing by hand. Recording
152+
evidence is only captured from action zero, so arm at `open`; a bare `close --save-script` on a
153+
session that was not armed at `open` is rejected.
152154

153155
### Maestro flows
154156

0 commit comments

Comments
 (0)