Merge pull request #442 from Li-john1021/fix/441-navigate-timeout-exit

fix(cli): exit 4 when navigate lifecycle wait times out

Author: iuyo5678

crates/bsk-cli/src/cli/navigate.rs

@@ -295,6 +295,13 @@ fn render_navigate(reply: &NavigateResult, format: Format) -> Result<(), CliErro
             print_dialog_summaries(&reply.dialogs);
         }
     }
+    // `--timeout` is documented as a hard timeout; treat lifecycle wait
+    // expiry as exit 4 (design §3.1) while still printing the payload so
+    // agents can inspect `final_url` / `error_text`. Distinct from
+    // `wait-for-element` / `wait-for-navigation`, which intentionally stay 0.
+    if reply.reached == "timeout" {
+        return Err(CliError::RenderedExit { exit_code: 4 });
+    }
     Ok(())
 }
 
@@ -325,6 +332,9 @@ fn render_history(
             print_dialog_summaries(&view.dialogs);
         }
     }
+    if view.reached == "timeout" {
+        return Err(CliError::RenderedExit { exit_code: 4 });
+    }
     Ok(())
 }
 
@@ -414,4 +424,63 @@ mod tests {
             Duration::from_secs(60) + Duration::from_secs(15)
         );
     }
+
+    fn sample_navigate(reached: &str) -> NavigateResult {
+        NavigateResult {
+            tab_id: 1,
+            url: "https://example.com/".into(),
+            final_url: Some("https://example.com/".into()),
+            reached: reached.into(),
+            error_text: if reached == "timeout" {
+                Some("timed out waiting for load".into())
+            } else {
+                None
+            },
+            dialogs: vec![],
+        }
+    }
+
+    #[test]
+    fn render_navigate_exits_4_on_timeout_after_printing() {
+        let err = render_navigate(&sample_navigate("timeout"), Format::Json).unwrap_err();
+        match err {
+            CliError::RenderedExit { exit_code } => assert_eq!(exit_code, 4),
+            other => panic!("expected RenderedExit(4), got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn render_navigate_ok_when_lifecycle_reached() {
+        assert!(render_navigate(&sample_navigate("load"), Format::Json).is_ok());
+    }
+
+    #[test]
+    fn render_history_exits_4_on_timeout() {
+        let view = OwnedHistoryView {
+            tab_id: 2,
+            previous_url: Some("https://a.example/".into()),
+            final_url: Some("https://b.example/".into()),
+            reached: "timeout".into(),
+            error_text: Some("timed out waiting for load".into()),
+            dialogs: vec![],
+        };
+        let err = render_history(&view, "reload", Format::Json).unwrap_err();
+        match err {
+            CliError::RenderedExit { exit_code } => assert_eq!(exit_code, 4),
+            other => panic!("expected RenderedExit(4), got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn render_history_ok_when_lifecycle_reached() {
+        let view = OwnedHistoryView {
+            tab_id: 2,
+            previous_url: None,
+            final_url: Some("https://example.com/".into()),
+            reached: "load".into(),
+            error_text: None,
+            dialogs: vec![],
+        };
+        assert!(render_history(&view, "reload", Format::Json).is_ok());
+    }
 }

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

@@ -369,12 +369,46 @@ export function bskInstallMessage(bskPath: string): string {
   );
 }
 
+/**
+ * Options for interpreting a finished `bsk` run.
+ *
+ * `allowNavigationTimeout`: when the CLI prints a success-shaped navigation
+ * payload with `reached: "timeout"` and then exits 4 (RenderedExit), return
+ * that payload instead of throwing. Exit 4 alone is not enough — session-busy
+ * and other failures also use 4 — so the payload must explicitly say
+ * `reached: "timeout"`.
+ */
+export interface ParseBskJsonOptions {
+  allowNavigationTimeout?: boolean;
+}
+
+/**
+ * True when stdout is a navigation/history payload whose lifecycle wait
+ * expired (`reached: "timeout"`). Used to distinguish that soft timeout from
+ * other exit-4 failures (e.g. session_busy).
+ */
+export function isNavigationLifecycleTimeoutPayload(value: unknown): value is {
+  reached: "timeout";
+  [key: string]: unknown;
+} {
+  return (
+    typeof value === "object" &&
+    value !== null &&
+    !Array.isArray(value) &&
+    (value as { reached?: unknown }).reached === "timeout"
+  );
+}
+
 /**
  * Interpret one finished run: throw `BskError` on timeout / non-zero exit
  * (parsing the CLI's JSON error envelope when present), otherwise parse and
  * return the stdout JSON payload.
  */
-export function parseBskJson(result: BskRunResult, commandLabel: string): unknown {
+export function parseBskJson(
+  result: BskRunResult,
+  commandLabel: string,
+  options: ParseBskJsonOptions = {},
+): unknown {
   if (result.timedOut) {
     throw new BskError(`bsk ${commandLabel} timed out`, { timedOut: true });
   }
@@ -385,6 +419,20 @@ export function parseBskJson(result: BskRunResult, commandLabel: string): unknow
     throw new BskError(`bsk ${commandLabel} was interrupted (process killed)`);
   }
   if (result.code !== 0) {
+    // Navigate / back / forward / reload print the success-shaped body then
+    // exit 4 on lifecycle wait expiry. Callers that opt in (combined session
+    // start-with-URL) can keep the session and surface the timeout instead of
+    // treating it as a hard failure.
+    if (options.allowNavigationTimeout === true && result.code === 4) {
+      try {
+        const payload: unknown = JSON.parse(body);
+        if (isNavigationLifecycleTimeoutPayload(payload)) {
+          return payload;
+        }
+      } catch {
+        // Fall through to the normal error path.
+      }
+    }
     let parsed: BskErrorBody | undefined;
     try {
       parsed = JSON.parse(body) as BskErrorBody;

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

@@ -25,6 +25,8 @@ import {
   type BskRunner,
   bskInstallMessage,
   isCommandNotFound,
+  isNavigationLifecycleTimeoutPayload,
+  type ParseBskJsonOptions,
   parseBskJson,
   runWithSessionBusyRetry,
 } from "./runner";
@@ -88,6 +90,7 @@ async function runBsk(
   observeSession?: string,
   runnerTimeoutMs?: number,
   initializing = false,
+  parseOptions: ParseBskJsonOptions = {},
 ): Promise<unknown> {
   const releaseForeground =
     observeSession !== undefined ? deps.observation.acquireForeground(observeSession) : undefined;
@@ -133,7 +136,7 @@ async function runBsk(
       error.name = "AbortError";
       throw error;
     }
-    return parseBskJson(result, label);
+    return parseBskJson(result, label, parseOptions);
   } catch (error) {
     actionError = error instanceof Error ? error.message.split("\n")[0] : String(error);
     throw error;
@@ -213,6 +216,8 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar):
             browserInstanceId: { type: "string", required: true },
             url: { type: "string" },
             device: { type: "string" },
+            reached: { type: "string" },
+            warning: { type: "string" },
           },
         },
         render: (_args, value) => [
@@ -221,7 +226,10 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar):
             text:
               `started browser session ${value.sessionId}` +
               (value.url !== undefined ? ` and navigated to ${value.url}` : "") +
-              (value.device !== undefined ? ` (device: ${value.device})` : ""),
+              (value.device !== undefined ? ` (device: ${value.device})` : "") +
+              (value.reached === "timeout"
+                ? " — initial navigation timed out before the requested lifecycle phase; session is usable"
+                : ""),
           },
         ],
       },
@@ -266,28 +274,77 @@ function defineBrowserOperations(deps: ToolDeps, register: DefinitionRegistrar):
               true,
             );
           }
+          let navigationTimeout:
+            | {
+                reached: "timeout";
+                url?: string;
+                final_url?: string;
+                error_text?: string;
+              }
+            | undefined;
           if (args.url !== undefined) {
-            await runBsk(
+            // Combined start-with-URL: a lifecycle wait timeout (exit 4 +
+            // reached:"timeout") must not cancel the session that was already
+            // created — the page may still be usable. Genuine navigate failures
+            // still fall through to starts.fail below.
+            const navigateReply = await runBsk(
               deps,
               exec,
               ["navigate", "--session", reply.session_id, args.url],
               "navigate",
               reply.session_id,
               undefined,
               true,
+              { allowNavigationTimeout: true },
             );
+            if (isNavigationLifecycleTimeoutPayload(navigateReply)) {
+              navigationTimeout = {
+                reached: "timeout",
+                url: typeof navigateReply.url === "string" ? navigateReply.url : undefined,
+                final_url:
+                  typeof navigateReply.final_url === "string" ? navigateReply.final_url : undefined,
+                error_text:
+                  typeof navigateReply.error_text === "string"
+                    ? navigateReply.error_text
+                    : undefined,
+              };
+              deps.observation.setUrl(
+                reply.session_id,
+                navigationTimeout.final_url ?? navigationTimeout.url ?? args.url,
+              );
+            } else {
+              const okReply = navigateReply as {
+                url?: string;
+                final_url?: string;
+              };
+              deps.observation.setUrl(
+                reply.session_id,
+                okReply.final_url ?? okReply.url ?? args.url,
+              );
+            }
           }
           await starts.claim(record, exec.signal);
+          return {
+            sessionId: reply.session_id,
+            browserInstanceId: reply.browser_instance_id,
+            ...(args.url !== undefined ? { url: args.url } : {}),
+            ...(args.device !== undefined ? { device: args.device } : {}),
+            ...(navigationTimeout !== undefined
+              ? {
+                  reached: "timeout" as const,
+                  warning:
+                    "Initial navigation timed out waiting for the page lifecycle; " +
+                    "the session remains open — observe the page and decide how to proceed." +
+                    (navigationTimeout.error_text !== undefined
+                      ? ` (${navigationTimeout.error_text})`
+                      : ""),
+                }
+              : {}),
+          };
         } catch (error) {
           await starts.fail(record).catch(() => {});
           throw error;
         }
-        return {
-          sessionId: reply.session_id,
-          browserInstanceId: reply.browser_instance_id,
-          ...(args.url !== undefined ? { url: args.url } : {}),
-          ...(args.device !== undefined ? { device: args.device } : {}),
-        };
       },
       presentCall: (args) => ({
         card: "terminal",

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

@@ -9,6 +9,7 @@ import {
   type BskRunResult,
   createBskRunner,
   isCommandNotFound,
+  isNavigationLifecycleTimeoutPayload,
   isSessionBusyResult,
   parseBskJson,
   runWithSessionBusyRetry,
@@ -535,6 +536,41 @@ describe("parseBskJson", () => {
       /did not produce JSON/,
     );
   });
+
+  it("returns navigation lifecycle timeout payloads when opted in", () => {
+    const payload = {
+      tab_id: 1,
+      url: "https://slow.example/",
+      final_url: "https://slow.example/",
+      reached: "timeout",
+      error_text: "timed out waiting for load",
+    };
+    const result = parseBskJson({ ...base, code: 4, stdout: JSON.stringify(payload) }, "navigate", {
+      allowNavigationTimeout: true,
+    });
+    expect(result).toEqual(payload);
+    expect(isNavigationLifecycleTimeoutPayload(result)).toBe(true);
+  });
+
+  it("still throws exit 4 without reached: timeout even when opted in", () => {
+    const busy = {
+      code: "timeout",
+      message: "session already has an unfinished command",
+      data: { reason: "session_busy" },
+    };
+    expect(() =>
+      parseBskJson({ ...base, code: 4, stdout: JSON.stringify(busy) }, "navigate", {
+        allowNavigationTimeout: true,
+      }),
+    ).toThrow(/session already has an unfinished command/);
+  });
+
+  it("throws navigation timeout payloads by default (no opt-in)", () => {
+    const payload = { tab_id: 1, url: "https://x/", reached: "timeout" };
+    expect(() =>
+      parseBskJson({ ...base, code: 4, stdout: JSON.stringify(payload) }, "navigate"),
+    ).toThrow(/failed/);
+  });
 });
 
 describe("isCommandNotFound", () => {