Skip to content

Commit 620f8ed

Browse files
rgarciaclaude
andcommitted
Settle reloads on dispose and fix first-turn screenshot ownership
Builds on the reentrant-reload coalescing already in place: - reload() returns an outcome (reloaded/coalesced/disposed). When a /reload coalesces onto an in-flight reload the reentrancy guard returns before the changes are applied, so the command now reports "a reload is already in progress" instead of a false success. - dispose() awaits an in-flight queued reload before tearing down, so the print/interactive/action cleanup `finally` blocks can't dispose the runner and close the browser mid-reload. Teardown is split into disposeNow() so a shutdown raised from inside reload() (an extension calling ctx.shutdown() during session_shutdown) tears down without awaiting its own reload — which would deadlock. - The first-turn screenshot is decided once at startup, before extensions load, and threaded via skipInitialScreenshot. An extension that sends a user message during session_start no longer flips the live prior-turn check and leaves the user's real first prompt without the browser frame; a startedUp guard keeps the host from consuming the screenshot during startup. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent d093a63 commit 620f8ed

8 files changed

Lines changed: 307 additions & 54 deletions

File tree

packages/cli/src/action/harness-runner.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -139,24 +139,14 @@ export async function runAction(
139139
}
140140

141141
async function maybeInitialScreenshot(opts: HarnessRunOptions): Promise<ImageContent[] | undefined> {
142+
// `skipInitialScreenshot` is decided once at startup (before extensions load),
143+
// so an extension's startup message can't suppress the user's first-turn frame.
142144
if (opts.skipInitialScreenshot) return undefined;
143-
const hasPriorTurn = await sessionHasPriorTurn(opts.session);
144-
if (hasPriorTurn) return undefined;
145145
const png = await captureScreenshot(opts.browserHandle.client, opts.browserHandle.browser.session_id);
146146
if (!png) return undefined;
147147
return [{ type: "image", data: png.toString("base64"), mimeType: "image/png" }];
148148
}
149149

150-
async function sessionHasPriorTurn(session: Session): Promise<boolean> {
151-
const entries = await session.getBranch();
152-
for (const entry of entries) {
153-
if (entry.type === "message" && (entry.message.role === "user" || entry.message.role === "assistant")) {
154-
return true;
155-
}
156-
}
157-
return false;
158-
}
159-
160150
function textFromAssistant(message: AssistantMessage): string {
161151
const parts: string[] = [];
162152
for (const block of message.content) {

packages/cli/src/cli-harness.ts

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,13 @@ interface HarnessRuntime {
351351
harness: ReturnType<typeof buildCuaHarness>;
352352
provider: string;
353353
modelRef: CuaModelRef;
354+
/**
355+
* Whether the first prompt should skip the initial browser screenshot.
356+
* Decided once here, before extensions load, so an extension that sends a
357+
* user message during startup can't flip the live prior-turn check and leave
358+
* the user's real first prompt without the screenshot.
359+
*/
360+
skipInitialScreenshot: boolean;
354361
/** Loaded pi-extension host. Undefined with --no-extensions or an untrusted project + no global extensions. */
355362
host?: HarnessExtensionHost;
356363
}
@@ -432,6 +439,11 @@ async function setupHarnessRuntime(
432439
const png = await captureScreenshot(handle.client, handle.browser.session_id);
433440
return png ? [{ type: "image", data: png.toString("base64"), mimeType: "image/png" }] : undefined;
434441
};
442+
// Decide the first-turn screenshot before extensions load: a resumed session
443+
// already has turns, and capturing it now (rather than re-reading the live
444+
// transcript at prompt time) keeps an extension's startup sendUserMessage from
445+
// flipping the check and leaving the user's real first prompt without it.
446+
const skipInitialScreenshot = resolved?.resumed === true || (await sessionHasPriorTurn(session));
435447
// A throwing extension load (e.g. a tool name colliding with a base tool)
436448
// must not leak the already-provisioned browser handle: the caller's finally
437449
// only runs once this returns, so close the handle here before rethrowing.
@@ -459,10 +471,20 @@ async function setupHarnessRuntime(
459471
harness,
460472
provider,
461473
modelRef: auth.modelRef,
474+
skipInitialScreenshot,
462475
host,
463476
};
464477
}
465478

479+
async function sessionHasPriorTurn(session: Session): Promise<boolean> {
480+
const entries = await session.getBranch();
481+
return entries.some(
482+
(entry) =>
483+
entry.type === "message" &&
484+
(entry.message.role === "user" || entry.message.role === "assistant"),
485+
);
486+
}
487+
466488
function hasExplicitSessionFlag(flags: HarnessCliFlags): boolean {
467489
return (
468490
!!flags.sessionRef ||
@@ -515,7 +537,7 @@ export async function runPrintCommand(prompt: string, flags: HarnessCliFlags): P
515537
provider: runtime.provider,
516538
prompt,
517539
skills: runtime.skills,
518-
skipInitialScreenshot: runtime.resolved?.resumed === true,
540+
skipInitialScreenshot: runtime.skipInitialScreenshot,
519541
verbose: flags.verbose,
520542
jsonlMode,
521543
jsonlIncludeDeltas: flags.jsonlIncludeDeltas,
@@ -560,7 +582,7 @@ export async function runInteractiveCommand(
560582
debugTui: flags.debugTui,
561583
resumed: runtime.resolved?.resumed === true,
562584
transcriptPath: runtime.resolved?.transcriptPath,
563-
skipInitialScreenshot: runtime.resolved?.resumed === true,
585+
skipInitialScreenshot: runtime.skipInitialScreenshot,
564586
host: runtime.host,
565587
});
566588
} finally {
@@ -596,7 +618,7 @@ export async function runActionCommand(
596618
harness: runtime.harness,
597619
browserHandle: runtime.handle,
598620
session: runtime.session,
599-
skipInitialScreenshot: runtime.resolved?.resumed === true,
621+
skipInitialScreenshot: runtime.skipInitialScreenshot,
600622
}, screenshotOut);
601623
return emitCompact(res);
602624
} finally {

packages/cli/src/extensions/host.ts

Lines changed: 76 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,16 @@ interface WriteExtensionDetails {
6060
hostLoadErrors: Array<{ path: string; error: string }>;
6161
}
6262

63+
/**
64+
* Result of a {@link HarnessExtensionHost.reload} call, so callers like the
65+
* `/reload` command can report honestly instead of assuming success:
66+
* - `reloaded` — extensions were re-discovered and re-applied.
67+
* - `coalesced` — a reload was already in flight; this request was latched onto
68+
* it rather than run concurrently, so nothing new was applied yet.
69+
* - `disposed` — the host was (or became) torn down, so no reload happened.
70+
*/
71+
export type ReloadOutcome = "reloaded" | "coalesced" | "disposed";
72+
6373
const WRITE_EXTENSION_TOOL_NAME = "write_extension";
6474

6575
const WRITE_EXTENSION_DESCRIPTION = [
@@ -146,13 +156,21 @@ export class HarnessExtensionHost {
146156
/** Set by `write_extension`; drained into a single reload at the next idle boundary. */
147157
private reloadRequested = false;
148158
/** The in-flight queued reload, so a drain can await one already running. */
149-
private pendingReload: Promise<void> | undefined;
159+
private pendingReload: Promise<ReloadOutcome> | undefined;
150160
/** Sticky shutdown request raised by `ctx.shutdown()` or owner disposal. */
151161
private shutdownRequested = false;
152162
/** Guards `dispose` so `ctx.shutdown()` and an owner call don't double-tear-down. */
153163
private disposed = false;
164+
/** Guards the actual teardown (`disposeNow`); `disposed` is set before the await. */
165+
private teardownDone = false;
154166
/** True once `load()` has run; guards against double-load and load-after-dispose. */
155167
private loaded = false;
168+
/**
169+
* False until `load()` finishes emitting the startup `session_start`. While
170+
* false, an extension-initiated `sendUserMessage` does not consume the
171+
* first-turn screenshot, so it can't pre-empt the user's real first prompt.
172+
*/
173+
private startedUp = false;
156174
private sessionName: string | undefined;
157175

158176
/** Load errors surfaced from the last discover; non-fatal. */
@@ -182,7 +200,9 @@ export class HarnessExtensionHost {
182200
getSignal: () => undefined,
183201
shutdown: () => this.requestShutdown(),
184202
});
185-
this.commandActions = makeExtensionCommandContextActions(this.harness, () => this.reload());
203+
this.commandActions = makeExtensionCommandContextActions(this.harness, async () => {
204+
await this.reload();
205+
});
186206
this.hostTools = options.selfExtend ? [this.makeWriteExtensionTool()] : [];
187207
}
188208

@@ -203,6 +223,10 @@ export class HarnessExtensionHost {
203223
await this.reapplyTools();
204224
this.installBridge();
205225
await this.runner?.emit({ type: "session_start", reason: "startup" });
226+
// Startup is over: from here an extension sendUserMessage may carry the
227+
// first-turn screenshot (it can no longer steal it from the user's first
228+
// prompt, which the CLI captured before extensions loaded).
229+
this.startedUp = true;
206230
// An extension that calls ctx.shutdown() during session_start disposes via
207231
// requestShutdown; honor it so load doesn't resolve a torn-down host as ready.
208232
if (this.shutdownRequested) await this.dispose();
@@ -215,14 +239,16 @@ export class HarnessExtensionHost {
215239
* the bridge, then emit `session_start`. No extension cache is cleared because
216240
* the loader imports each extension fresh from disk.
217241
*/
218-
async reload(): Promise<void> {
219-
if (this.disposed) return;
242+
async reload(): Promise<ReloadOutcome> {
243+
if (this.disposed) return "disposed";
220244
// Reentrancy guard: a reload triggered (e.g. via ctx.reload()) while one is
221245
// in flight must not run concurrently and double-tear-down the bridge. Re-arm
222-
// the latch so the in-flight reload's drain picks up the newer request.
246+
// the latch so the in-flight reload's loop picks up the newer request, and
247+
// report `coalesced` so a caller (e.g. the /reload command) doesn't claim a
248+
// completed reload it didn't perform.
223249
if (this.reloading) {
224250
this.reloadRequested = true;
225-
return;
251+
return "coalesced";
226252
}
227253
this.reloading = true;
228254
try {
@@ -238,19 +264,19 @@ export class HarnessExtensionHost {
238264
// scheduled off-stack at agent_end), so this resolves promptly and
239265
// cannot deadlock on an awaited-in-loop reload.
240266
await this.harness.waitForIdle();
241-
if (this.disposed) return;
267+
if (this.disposed) return "disposed";
242268
const flags = this.runner?.getFlagValues() ?? new Map<string, boolean | string>();
243269
await this.runner?.emit({ type: "session_shutdown", reason: "reload" });
244-
if (await this.disposeIfShutdownRequested()) return;
270+
if (await this.disposeIfShutdownRequested()) return "disposed";
245271
this.teardownBridge?.();
246272
this.teardownBridge = undefined;
247273
try {
248274
await this.buildRunner();
249-
if (await this.disposeIfShutdownRequested()) return;
275+
if (await this.disposeIfShutdownRequested()) return "disposed";
250276
for (const [name, value] of flags) this.runner?.setFlagValue(name, value);
251277

252278
await this.reapplyTools();
253-
if (await this.disposeIfShutdownRequested()) return;
279+
if (await this.disposeIfShutdownRequested()) return "disposed";
254280
this.installBridge();
255281
await this.runner?.emit({ type: "session_start", reason: "reload" });
256282
} catch (error) {
@@ -265,7 +291,13 @@ export class HarnessExtensionHost {
265291
this.reloading = false;
266292
}
267293
// Honor a shutdown requested during the final emit, after `reloading` cleared.
268-
if (this.shutdownRequested) await this.dispose();
294+
// Still inside reload() (pendingReload may point at us), so tear down via
295+
// disposeNow rather than dispose to avoid awaiting our own reload.
296+
if (this.shutdownRequested) {
297+
await this.disposeNow();
298+
return "disposed";
299+
}
300+
return "reloaded";
269301
}
270302

271303
/**
@@ -280,6 +312,29 @@ export class HarnessExtensionHost {
280312
if (this.disposed) return;
281313
this.shutdownRequested = true;
282314
this.disposed = true;
315+
// A write_extension reload scheduled off-stack at agent_end may still be in
316+
// flight (print/interactive/action cleanup runs in `finally`). Setting
317+
// `disposed` stops new reloads and makes the in-flight one bail at its next
318+
// await boundary; await it so teardown — and the caller closing the browser —
319+
// doesn't race a live reload. Shutdowns raised from inside reload() use
320+
// `disposeNow` directly, since awaiting the running reload from within its own
321+
// call stack would deadlock.
322+
const inFlight = this.pendingReload;
323+
if (inFlight) await inFlight.catch(() => {});
324+
await this.disposeNow();
325+
}
326+
327+
/**
328+
* The actual teardown, split from `dispose` so reload()'s own shutdown paths
329+
* can run it without awaiting the in-flight reload (which is their call stack).
330+
* Idempotent via `teardownDone` — `disposed` is set before `dispose` awaits, so
331+
* it can't double as the teardown guard.
332+
*/
333+
private async disposeNow(): Promise<void> {
334+
if (this.teardownDone) return;
335+
this.teardownDone = true;
336+
this.shutdownRequested = true;
337+
this.disposed = true;
283338
this.teardownBridge?.();
284339
this.teardownBridge = undefined;
285340
await this.runner?.emit({ type: "session_shutdown", reason: "quit" });
@@ -423,12 +478,14 @@ export class HarnessExtensionHost {
423478
* handler — reloading there would swap the runner out from under the in-flight
424479
* loop and the listener dispatching the event. The `reloading` guard keeps an
425480
* in-flight reload from being re-entered; a write during a reload re-arms the
426-
* latch for the next boundary. `disposed` makes this a no-op during teardown.
481+
* latch, which reload()'s own loop drains before it resolves. `disposed` makes
482+
* this a no-op during teardown.
427483
*/
428484
async drainPendingReload(): Promise<void> {
429485
// Await a reload already running (the bridge fires this fire-and-forget, so
430486
// a caller that awaits the drain — e.g. a test asserting the new tool is
431-
// live — must observe that reload settle).
487+
// live — must observe that reload settle). reload() drains any request
488+
// latched mid-reload via its own loop, so a single pass suffices here.
432489
if (this.pendingReload) {
433490
await this.pendingReload;
434491
return;
@@ -564,7 +621,9 @@ export class HarnessExtensionHost {
564621
/** Honor a shutdown latched during reload. Returns true if the host disposed. */
565622
private async disposeIfShutdownRequested(): Promise<boolean> {
566623
if (!this.shutdownRequested && !this.disposed) return false;
567-
await this.dispose();
624+
// disposeNow, not dispose: this runs inside reload(), whose promise is the
625+
// `pendingReload` dispose() would await — awaiting it here would deadlock.
626+
await this.disposeNow();
568627
return true;
569628
}
570629

@@ -590,6 +649,9 @@ export class HarnessExtensionHost {
590649

591650
private async maybeInitialScreenshot(): Promise<ImageContent[] | undefined> {
592651
if (!this.initialScreenshot) return undefined;
652+
// During startup the user's first prompt owns the first-turn screenshot; an
653+
// extension message here must not consume it (see `startedUp`).
654+
if (!this.startedUp) return undefined;
593655
if (await sessionHasPriorTurn(this.session)) return undefined;
594656
return this.initialScreenshot();
595657
}

packages/cli/src/print.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,9 @@ export async function runPrint(opts: RunPrintOptions): Promise<number> {
9393
}
9494

9595
async function maybeInitialScreenshot(opts: RunPrintOptions): Promise<ImageContent[] | undefined> {
96+
// `skipInitialScreenshot` is decided once at startup (before extensions load),
97+
// so an extension's startup message can't suppress the user's first-turn frame.
9698
if (opts.skipInitialScreenshot) return undefined;
97-
const hasPriorTurn = await sessionHasPriorTurn(opts.session);
98-
if (hasPriorTurn) return undefined;
9999
const png = await captureScreenshot(opts.browserHandle.client, opts.browserHandle.browser.session_id);
100100
if (!png) return undefined;
101101
return [
@@ -106,13 +106,3 @@ async function maybeInitialScreenshot(opts: RunPrintOptions): Promise<ImageConte
106106
},
107107
];
108108
}
109-
110-
async function sessionHasPriorTurn(session: Session): Promise<boolean> {
111-
const entries = await session.getBranch();
112-
for (const entry of entries) {
113-
if (entry.type === "message" && (entry.message.role === "user" || entry.message.role === "assistant")) {
114-
return true;
115-
}
116-
}
117-
return false;
118-
}

packages/cli/src/tui/main.ts

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -450,23 +450,14 @@ async function maybeInitialScreenshot(
450450
firstPromptSent: boolean,
451451
): Promise<ImageContent[] | undefined> {
452452
if (firstPromptSent) return undefined;
453+
// `skipInitialScreenshot` is decided once at startup (before extensions load),
454+
// so an extension's startup message can't suppress the user's first-turn frame.
453455
if (opts.skipInitialScreenshot) return undefined;
454-
if (await sessionHasPriorTurn(opts.session)) return undefined;
455456
const png = await captureScreenshot(opts.browserHandle.client, opts.browserHandle.browser.session_id);
456457
if (!png) return undefined;
457458
return [{ type: "image", data: png.toString("base64"), mimeType: "image/png" }];
458459
}
459460

460-
async function sessionHasPriorTurn(session: Session): Promise<boolean> {
461-
const entries = await session.getBranch();
462-
for (const entry of entries) {
463-
if (entry.type === "message" && (entry.message.role === "user" || entry.message.role === "assistant")) {
464-
return true;
465-
}
466-
}
467-
return false;
468-
}
469-
470461
async function applyModelCommand(
471462
opts: InteractiveOptions,
472463
footer: TelemetryFooter,
@@ -540,11 +531,15 @@ export async function applyReloadCommand(opts: InteractiveOptions, messages: Mes
540531
// reload() emits no harness event, so this helper is the only source of
541532
// feedback; surface loadErrors so a broken edited extension isn't silently
542533
// dropped with its tool missing.
543-
await opts.host.reload();
544-
if (opts.host.isDisposed()) {
534+
const outcome = await opts.host.reload();
535+
if (outcome === "disposed" || opts.host.isDisposed()) {
545536
// An extension calling ctx.shutdown() during the reload tears the host
546537
// down; don't claim a successful reload.
547538
messages.addNotice("session is shutting down; extensions were not reloaded");
539+
} else if (outcome === "coalesced") {
540+
// Another reload was already in flight (e.g. a self-extend reload); this
541+
// request was latched onto it, so nothing new has been applied yet.
542+
messages.addNotice("a reload is already in progress");
548543
} else if (opts.host.loadErrors.length > 0) {
549544
for (const { path, error } of opts.host.loadErrors) messages.addError(`${path}: ${error}`);
550545
} else {

0 commit comments

Comments
 (0)