Skip to content

Commit e5a4885

Browse files
committed
fix(dsh): keep session on start-with-URL navigation timeout
When combined session.start + url hits CLI exit 4 with reached:"timeout", claim and return the usable sessionId with an explicit warning instead of starts.fail cleanup. Genuine init failures and other exit-4 payloads still cancel. Regression coverage for keep/cleanup and subsequent observe.
1 parent 4dd38c8 commit e5a4885

4 files changed

Lines changed: 243 additions & 10 deletions

File tree

‎packages/dsh-plugin-browserskill/src/runner.ts‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,12 +369,46 @@ export function bskInstallMessage(bskPath: string): string {
369369
);
370370
}
371371

372+
/**
373+
* Options for interpreting a finished `bsk` run.
374+
*
375+
* `allowNavigationTimeout`: when the CLI prints a success-shaped navigation
376+
* payload with `reached: "timeout"` and then exits 4 (RenderedExit), return
377+
* that payload instead of throwing. Exit 4 alone is not enough — session-busy
378+
* and other failures also use 4 — so the payload must explicitly say
379+
* `reached: "timeout"`.
380+
*/
381+
export interface ParseBskJsonOptions {
382+
allowNavigationTimeout?: boolean;
383+
}
384+
385+
/**
386+
* True when stdout is a navigation/history payload whose lifecycle wait
387+
* expired (`reached: "timeout"`). Used to distinguish that soft timeout from
388+
* other exit-4 failures (e.g. session_busy).
389+
*/
390+
export function isNavigationLifecycleTimeoutPayload(value: unknown): value is {
391+
reached: "timeout";
392+
[key: string]: unknown;
393+
} {
394+
return (
395+
typeof value === "object" &&
396+
value !== null &&
397+
!Array.isArray(value) &&
398+
(value as { reached?: unknown }).reached === "timeout"
399+
);
400+
}
401+
372402
/**
373403
* Interpret one finished run: throw `BskError` on timeout / non-zero exit
374404
* (parsing the CLI's JSON error envelope when present), otherwise parse and
375405
* return the stdout JSON payload.
376406
*/
377-
export function parseBskJson(result: BskRunResult, commandLabel: string): unknown {
407+
export function parseBskJson(
408+
result: BskRunResult,
409+
commandLabel: string,
410+
options: ParseBskJsonOptions = {},
411+
): unknown {
378412
if (result.timedOut) {
379413
throw new BskError(`bsk ${commandLabel} timed out`, { timedOut: true });
380414
}
@@ -385,6 +419,20 @@ export function parseBskJson(result: BskRunResult, commandLabel: string): unknow
385419
throw new BskError(`bsk ${commandLabel} was interrupted (process killed)`);
386420
}
387421
if (result.code !== 0) {
422+
// Navigate / back / forward / reload print the success-shaped body then
423+
// exit 4 on lifecycle wait expiry. Callers that opt in (combined session
424+
// start-with-URL) can keep the session and surface the timeout instead of
425+
// treating it as a hard failure.
426+
if (options.allowNavigationTimeout === true && result.code === 4) {
427+
try {
428+
const payload: unknown = JSON.parse(body);
429+
if (isNavigationLifecycleTimeoutPayload(payload)) {
430+
return payload;
431+
}
432+
} catch {
433+
// Fall through to the normal error path.
434+
}
435+
}
388436
let parsed: BskErrorBody | undefined;
389437
try {
390438
parsed = JSON.parse(body) as BskErrorBody;

‎packages/dsh-plugin-browserskill/src/tools.ts‎

Lines changed: 66 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import {
2525
type BskRunner,
2626
bskInstallMessage,
2727
isCommandNotFound,
28+
isNavigationLifecycleTimeoutPayload,
29+
type ParseBskJsonOptions,
2830
parseBskJson,
2931
runWithSessionBusyRetry,
3032
} from "./runner";
@@ -88,6 +90,7 @@ async function runBsk(
8890
observeSession?: string,
8991
runnerTimeoutMs?: number,
9092
initializing = false,
93+
parseOptions: ParseBskJsonOptions = {},
9194
): Promise<unknown> {
9295
const releaseForeground =
9396
observeSession !== undefined ? deps.observation.acquireForeground(observeSession) : undefined;
@@ -133,7 +136,7 @@ async function runBsk(
133136
error.name = "AbortError";
134137
throw error;
135138
}
136-
return parseBskJson(result, label);
139+
return parseBskJson(result, label, parseOptions);
137140
} catch (error) {
138141
actionError = error instanceof Error ? error.message.split("\n")[0] : String(error);
139142
throw error;
@@ -212,6 +215,8 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar):
212215
browserInstanceId: { type: "string", required: true },
213216
url: { type: "string" },
214217
device: { type: "string" },
218+
reached: { type: "string" },
219+
warning: { type: "string" },
215220
},
216221
},
217222
render: (_args, value) => [
@@ -220,7 +225,10 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar):
220225
text:
221226
`started browser session ${value.sessionId}` +
222227
(value.url !== undefined ? ` and navigated to ${value.url}` : "") +
223-
(value.device !== undefined ? ` (device: ${value.device})` : ""),
228+
(value.device !== undefined ? ` (device: ${value.device})` : "") +
229+
(value.reached === "timeout"
230+
? " — initial navigation timed out before the requested lifecycle phase; session is usable"
231+
: ""),
224232
},
225233
],
226234
},
@@ -264,28 +272,77 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar):
264272
true,
265273
);
266274
}
275+
let navigationTimeout:
276+
| {
277+
reached: "timeout";
278+
url?: string;
279+
final_url?: string;
280+
error_text?: string;
281+
}
282+
| undefined;
267283
if (args.url !== undefined) {
268-
await runBsk(
284+
// Combined start-with-URL: a lifecycle wait timeout (exit 4 +
285+
// reached:"timeout") must not cancel the session that was already
286+
// created — the page may still be usable. Genuine navigate failures
287+
// still fall through to starts.fail below.
288+
const navigateReply = await runBsk(
269289
deps,
270290
exec,
271291
["navigate", "--session", reply.session_id, args.url],
272292
"navigate",
273293
reply.session_id,
274294
undefined,
275295
true,
296+
{ allowNavigationTimeout: true },
276297
);
298+
if (isNavigationLifecycleTimeoutPayload(navigateReply)) {
299+
navigationTimeout = {
300+
reached: "timeout",
301+
url: typeof navigateReply.url === "string" ? navigateReply.url : undefined,
302+
final_url:
303+
typeof navigateReply.final_url === "string" ? navigateReply.final_url : undefined,
304+
error_text:
305+
typeof navigateReply.error_text === "string"
306+
? navigateReply.error_text
307+
: undefined,
308+
};
309+
deps.observation.setUrl(
310+
reply.session_id,
311+
navigationTimeout.final_url ?? navigationTimeout.url ?? args.url,
312+
);
313+
} else {
314+
const okReply = navigateReply as {
315+
url?: string;
316+
final_url?: string;
317+
};
318+
deps.observation.setUrl(
319+
reply.session_id,
320+
okReply.final_url ?? okReply.url ?? args.url,
321+
);
322+
}
277323
}
278324
await starts.claim(record, exec.signal);
325+
return {
326+
sessionId: reply.session_id,
327+
browserInstanceId: reply.browser_instance_id,
328+
...(args.url !== undefined ? { url: args.url } : {}),
329+
...(args.device !== undefined ? { device: args.device } : {}),
330+
...(navigationTimeout !== undefined
331+
? {
332+
reached: "timeout" as const,
333+
warning:
334+
"Initial navigation timed out waiting for the page lifecycle; " +
335+
"the session remains open — observe the page and decide how to proceed." +
336+
(navigationTimeout.error_text !== undefined
337+
? ` (${navigationTimeout.error_text})`
338+
: ""),
339+
}
340+
: {}),
341+
};
279342
} catch (error) {
280343
await starts.fail(record).catch(() => {});
281344
throw error;
282345
}
283-
return {
284-
sessionId: reply.session_id,
285-
browserInstanceId: reply.browser_instance_id,
286-
...(args.url !== undefined ? { url: args.url } : {}),
287-
...(args.device !== undefined ? { device: args.device } : {}),
288-
};
289346
},
290347
presentCall: (args) => ({
291348
card: "terminal",

‎packages/dsh-plugin-browserskill/tests/runner.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
type BskRunResult,
1010
createBskRunner,
1111
isCommandNotFound,
12+
isNavigationLifecycleTimeoutPayload,
1213
isSessionBusyResult,
1314
parseBskJson,
1415
runWithSessionBusyRetry,
@@ -535,6 +536,41 @@ describe("parseBskJson", () => {
535536
/did not produce JSON/,
536537
);
537538
});
539+
540+
it("returns navigation lifecycle timeout payloads when opted in", () => {
541+
const payload = {
542+
tab_id: 1,
543+
url: "https://slow.example/",
544+
final_url: "https://slow.example/",
545+
reached: "timeout",
546+
error_text: "timed out waiting for load",
547+
};
548+
const result = parseBskJson({ ...base, code: 4, stdout: JSON.stringify(payload) }, "navigate", {
549+
allowNavigationTimeout: true,
550+
});
551+
expect(result).toEqual(payload);
552+
expect(isNavigationLifecycleTimeoutPayload(result)).toBe(true);
553+
});
554+
555+
it("still throws exit 4 without reached: timeout even when opted in", () => {
556+
const busy = {
557+
code: "timeout",
558+
message: "session already has an unfinished command",
559+
data: { reason: "session_busy" },
560+
};
561+
expect(() =>
562+
parseBskJson({ ...base, code: 4, stdout: JSON.stringify(busy) }, "navigate", {
563+
allowNavigationTimeout: true,
564+
}),
565+
).toThrow(/session already has an unfinished command/);
566+
});
567+
568+
it("throws navigation timeout payloads by default (no opt-in)", () => {
569+
const payload = { tab_id: 1, url: "https://x/", reached: "timeout" };
570+
expect(() =>
571+
parseBskJson({ ...base, code: 4, stdout: JSON.stringify(payload) }, "navigate"),
572+
).toThrow(/failed/);
573+
});
538574
});
539575

540576
describe("isCommandNotFound", () => {

‎packages/dsh-plugin-browserskill/tests/tools.test.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,15 @@ function fakeRunner(responses: Record<string, unknown>) {
8282
typeof response === "object" && response !== null && "__seq" in response
8383
? (response as { __seq: unknown[] }).__seq.shift()
8484
: response;
85+
// Allow tests to return a full BskRunResult (custom exit code / body).
86+
if (
87+
typeof reply === "object" &&
88+
reply !== null &&
89+
"__result" in reply &&
90+
typeof (reply as { __result: unknown }).__result === "object"
91+
) {
92+
return (reply as { __result: BskRunResult }).__result;
93+
}
8594
return {
8695
code: 0,
8796
stdout: JSON.stringify(reply),
@@ -404,6 +413,89 @@ describe("session.start", () => {
404413
expect(registry.current()).toBeUndefined();
405414
});
406415

416+
it("keeps the session when combined start-with-URL hits a navigation lifecycle timeout", async () => {
417+
// CLI exit 4 + reached:"timeout" must not cancel the newly created session
418+
// (review on #442): the agent should get sessionId + an explicit warning and
419+
// be able to observe afterwards. Exit 4 alone is not treated as success.
420+
const { tools, calls, registry } = setup({
421+
"session start": START_REPLY("s1"),
422+
navigate: {
423+
__result: {
424+
code: 4,
425+
stdout: JSON.stringify({
426+
tab_id: 7,
427+
url: "https://slow.example/",
428+
final_url: "https://slow.example/",
429+
reached: "timeout",
430+
error_text: "timed out waiting for load",
431+
}),
432+
stderr: "",
433+
timedOut: false,
434+
aborted: false,
435+
},
436+
},
437+
observe: {
438+
text: '- heading "Partial" [ref=@e1]',
439+
ref_count: 1,
440+
tab_id: 7,
441+
truncated: false,
442+
},
443+
});
444+
const start = tools.get("session.start");
445+
const value = (await start?.execute({ url: "https://slow.example/" }, makeExec())) as {
446+
sessionId: string;
447+
reached?: string;
448+
warning?: string;
449+
url?: string;
450+
};
451+
expect(value).toMatchObject({
452+
sessionId: "s1",
453+
url: "https://slow.example/",
454+
reached: "timeout",
455+
});
456+
expect(value.warning).toMatch(/timed out/i);
457+
expect(value.warning).toMatch(/remains open|usable|observe/i);
458+
// Claimed and active — no cancel/cleanup issued.
459+
expect(registry.current()).toBe("s1");
460+
expect(calls.some((c) => c.args[1] === "request" && c.args.includes("--cancel"))).toBe(false);
461+
expect(calls.some((c) => c.args.join(" ").startsWith("session stop"))).toBe(false);
462+
463+
const observe = tools.get("inspect.observe");
464+
const observed = (await observe?.execute({}, makeExec())) as {
465+
session: string;
466+
refCount: number;
467+
};
468+
expect(observed).toMatchObject({ session: "s1", refCount: 1 });
469+
expect(calls.some((c) => c.args[0] === "observe" && c.args.includes("s1"))).toBe(true);
470+
});
471+
472+
it("still cleans up when exit 4 lacks reached: timeout on combined start", async () => {
473+
// Exit 4 is also used for session_busy / other errors — must not keep the session.
474+
const { tools, calls, registry } = setup({
475+
"session start": START_REPLY("s1"),
476+
"session stop": {},
477+
navigate: {
478+
__result: {
479+
code: 4,
480+
stdout: JSON.stringify({
481+
code: "timeout",
482+
message: "session already has an unfinished command",
483+
data: { reason: "session_busy" },
484+
}),
485+
stderr: "",
486+
timedOut: false,
487+
aborted: false,
488+
},
489+
},
490+
});
491+
const tool = tools.get("session.start");
492+
await expect(tool?.execute({ url: "https://example.com" }, makeExec())).rejects.toThrow(
493+
/navigate/,
494+
);
495+
expect(calls.some((c) => c.args[1] === "request" && c.args.includes("--cancel"))).toBe(true);
496+
expect(registry.current()).toBeUndefined();
497+
});
498+
407499
it("returns the session when bsk exits but something still holds its stdio pipes", async () => {
408500
// Issue #180: `bsk session start` printed its JSON and exited, but the daemon
409501
// it auto-spawned kept the stdio pipes open, so the child's `close` never

0 commit comments

Comments
 (0)