From f5dfbcf80aec09119f2c6af13d5073c5214ee5c3 Mon Sep 17 00:00:00 2001 From: Austin Date: Sat, 1 Aug 2026 22:19:32 +0000 Subject: [PATCH] fix(claude-swe-agent): stop a fast turn from racing the Remote Control session URL (#183) The "watch live / take over the session" link is posted for most runs but dropped intermittently on the `ai-review` flow (issue #183). Root cause is a timing race in `runClaudeTurnRemoteControlled`'s poll loop: - the session URL is emitted the instant the transcript's `bridgeSessionId` line appears, and - that line is written when the bridge's ASYNC registration handshake with claude.ai completes -- not necessarily before the turn does. A long triage turn registers the bridge many polls before it finishes, so the URL is always emitted first. A fast, read-only review can finish in the same poll window the bridge is still registering in; the loop returned the instant it saw `turn_duration`, so which of the two the transcript flushed first decided whether the link was ever emitted. That is the non-deterministic "sometimes it fails to" the maintainer observed. Make it deterministic: when a turn completes and the URL has not been reported yet, hold the finished turn open for a bounded grace (`REMOTE_CONTROL_URL_GRACE_MS`, 15s, overridable) and keep re-reading the transcript so a just-registered bridge still surfaces its link before returning. Skipped entirely on the common path where the bridge registered mid-turn, so it costs nothing there; bounded so a bridge that never registers can't hang a completed review. Adds a `latebridge` fake-script mode and a regression test asserting the URL is still reported when the `bridgeSessionId` line lands after `turn_duration`. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01FLZRLE1mqh9ajMYyKyaWSr --- .../src/claude-runner-remote-control.test.ts | 38 +++++++++++++ .../src/claude-runner.ts | 57 +++++++++++++++++++ 2 files changed, 95 insertions(+) diff --git a/apps/claude-code-swe-agent/src/claude-runner-remote-control.test.ts b/apps/claude-code-swe-agent/src/claude-runner-remote-control.test.ts index 0e02af7..722b950 100644 --- a/apps/claude-code-swe-agent/src/claude-runner-remote-control.test.ts +++ b/apps/claude-code-swe-agent/src/claude-runner-remote-control.test.ts @@ -35,6 +35,11 @@ async function installFake(name: string, body: string): Promise { // // Modes: // complete - bridge line + assistant text + turn_duration, written at once +// latebridge- assistant text + turn_duration first, then the bridge line +// appended FAKE_LATE_BRIDGE_MS later. Models issue #183: a fast +// read-only turn that finishes before the bridge finishes its +// async registration with claude.ai, so the session URL is not in +// the transcript yet when turn_duration lands. // running - bridge line + assistant text, never completes (works, then wedges) // stalled - bridge line only (registers, but the turn never begins) // streaming - bridge line, then a tool_use entry every FAKE_STEP_MS for @@ -77,6 +82,13 @@ if (mode !== "nofile") { clearInterval(timer); fs.appendFileSync(file, assistant(process.env.FAKE_ASSISTANT_TEXT || "DONE") + "\\n" + done() + "\\n"); }, stepMs); + } else if (mode === "latebridge") { + // Turn finishes (assistant text + turn_duration) BEFORE the bridge line is + // written -- the bridge's registration handshake lands FAKE_LATE_BRIDGE_MS + // later. Reproduces issue #183's dropped session URL on fast turns. + fs.writeFileSync(file, assistant(process.env.FAKE_ASSISTANT_TEXT || "DONE") + "\\n" + done() + "\\n"); + const lateMs = Number(process.env.FAKE_LATE_BRIDGE_MS || 60); + setTimeout(() => { fs.appendFileSync(file, bridge() + "\\n"); }, lateMs); } else { const lines = [bridge()]; if (mode !== "stalled") lines.push(assistant(process.env.FAKE_ASSISTANT_TEXT || "DONE")); @@ -165,6 +177,32 @@ describe("runClaudeTurnRemoteControlled (interactive)", () => { expect(progress).toContainEqual({ message: "https://claude.ai/code/session_TESTBRIDGE", stage: "remote-control-url" }); }); + // Regression for issue #183 ("Review Agent Doesn't Post a Session Link"). The + // "watch live / take over the session" link was posted for most runs but + // dropped intermittently on the fast, read-only `ai-review` flow. Root cause: + // the turn can finish (turn_duration) BEFORE the Remote Control bridge writes + // its `bridgeSessionId` line (that line lands when the async registration with + // claude.ai completes), and the loop used to return the instant it saw + // completion -- so the URL was never emitted. Here the bridge line is written + // AFTER completion; the URL must still be reported (deterministically), not + // raffled off by which of the two the transcript flushed first. + it("still reports the session URL when the bridge line lands AFTER turn_duration (issue #183)", async () => { + const progress: Array<{ message: string; stage: string }> = []; + const result = await runClaudeTurnRemoteControlled("review the PR", { + cwd, + env: env({ FAKE_SCRIPT_MODE: "latebridge", FAKE_LATE_BRIDGE_MS: "60" }), + settings: {}, + runId: "run-late-bridge", + pollIntervalMs: 20, + urlGraceMs: 5000, + maxWaitMs: 5000, + onProgress: (message, stage) => progress.push({ message, stage }), + }); + + expect(result).toMatchObject({ finalMessage: "DONE", failed: false, authError: false }); + expect(progress).toContainEqual({ message: "https://claude.ai/code/session_TESTBRIDGE", stage: "remote-control-url" }); + }); + // Regression for issue #149. A feature-sized coding turn ran correctly for // half an hour and was then killed by an ABSOLUTE 30-minute cap and reported // to the user as "Timed out after 1800000ms waiting for the remote-control diff --git a/apps/claude-code-swe-agent/src/claude-runner.ts b/apps/claude-code-swe-agent/src/claude-runner.ts index 7f27492..6513ef5 100644 --- a/apps/claude-code-swe-agent/src/claude-runner.ts +++ b/apps/claude-code-swe-agent/src/claude-runner.ts @@ -57,6 +57,12 @@ export interface RemoteControlRunOptions extends ClaudeRunOptions { * {@link REMOTE_CONTROL_STARTUP_TIMEOUT_MS}; injectable for tests. */ startupTimeoutMs?: number; + /** + * How long a finished turn may be held open, after completion, waiting for the + * Remote Control bridge to surface its session URL when it has not already. + * Defaults to {@link REMOTE_CONTROL_URL_GRACE_MS}; injectable for tests. + */ + urlGraceMs?: number; /** * Optional ABSOLUTE wall-clock backstop. Defaults to no bound at all: a real * coding turn can legitimately run for hours, the idle bound above is what @@ -417,6 +423,34 @@ const REMOTE_CONTROL_WAITING_TIMEOUT_MS = 5 * 60_000; */ const REMOTE_CONTROL_STARTUP_TIMEOUT_MS = 5 * 60_000; +/** + * How long a FINISHED turn may be held open, after `turn_duration` lands, purely + * to let the Remote Control bridge surface its session URL (the transcript's + * `bridgeSessionId` line) before we return. + * + * This is the deterministic fix for issue #183: the "watch live / take over the + * session" link was posted for most runs but dropped intermittently on the + * `ai-review` flow. The two facts behind it are the reply loop's ordering (the + * URL is emitted the instant `bridgeSessionId` is seen, `turn_duration` ends the + * wait) and that the bridge's `bridgeSessionId` line is written when its ASYNC + * registration handshake with claude.ai completes -- NOT necessarily before the + * turn does. A long triage turn registers the bridge many polls before it + * finishes, so the URL is always emitted first. A fast, read-only review can + * finish in the same poll window the bridge is still registering in, and + * returning the instant `turn_duration` appears drops the link -- which of the + * two the transcript flushed first is a race, and that race is exactly the + * "sometimes it fails to" the maintainer saw. Reporting the URL is deterministic + * once the `bridgeSessionId` line exists; this bound just stops a fast + * completion from racing it out of existence. + * + * Only ever paid when a turn completes WITHOUT its URL already reported (the + * failing case) -- the common path, where the bridge registered early and the + * URL was emitted mid-turn, skips this entirely. Bounded because a bridge that + * has not registered within seconds of the turn finishing almost certainly + * never will (registration failed), and a completed review must not hang on it. + */ +const REMOTE_CONTROL_URL_GRACE_MS = 15_000; + /** * Builds the Remote Control URL from a transcript `bridgeSessionId`, mirroring * the CLI's own `toCompatSessionId` (decompiled v2.1.218): a `cse_`-prefixed @@ -880,6 +914,7 @@ export async function runClaudeTurnRemoteControlled( const idleStatusGraceMs = opts.idleStatusGraceMs ?? REMOTE_CONTROL_IDLE_STATUS_GRACE_MS; const waitingTimeoutMs = opts.waitingTimeoutMs ?? REMOTE_CONTROL_WAITING_TIMEOUT_MS; const startupTimeoutMs = opts.startupTimeoutMs ?? REMOTE_CONTROL_STARTUP_TIMEOUT_MS; + const urlGraceMs = opts.urlGraceMs ?? REMOTE_CONTROL_URL_GRACE_MS; const heartbeatMs = opts.heartbeatIntervalMs ?? HEARTBEAT_INTERVAL_MS; // No absolute cap by default -- see `maxWaitMs`'s doc comment and issue #149. const absoluteDeadline = opts.maxWaitMs === undefined ? Infinity : Date.now() + opts.maxWaitMs; @@ -999,6 +1034,28 @@ export async function runClaudeTurnRemoteControlled( sessionId: session.sessionId, }; } + // Issue #183: don't let a fast completion race the Remote Control + // URL out of existence. The URL is emitted the instant the + // transcript's `bridgeSessionId` line appears (line ~1010 above), + // but that line lands when the bridge's async registration with + // claude.ai completes -- which, for a quick read-only review, can be + // AFTER `turn_duration`. If we haven't reported the URL yet, hold the + // finished turn open briefly and keep re-reading the transcript so a + // just-registered bridge still surfaces its link before we return. + // Bounded by `urlGraceMs`; skipped entirely once the URL is known + // (the common case, where the bridge registered mid-turn), so it + // costs nothing on the happy path. The interactive child stays + // resident (killed only in the `finally` below), so its registration + // keeps progressing throughout this wait. + if (!urlReported) { + const graceDeadline = Date.now() + urlGraceMs; + while (!urlReported && !opts.signal?.aborted && Date.now() < graceDeadline) { + await sleep(pollIntervalMs, opts.signal); + const graceRaw = await readTranscript(homeDir, opts.cwd, session.sessionId); + const graceBridge = graceRaw ? parseTranscript(graceRaw).bridgeSessionId : null; + if (graceBridge) reportUrl(remoteControlUrlFromBridge(graceBridge)); + } + } return { finalMessage: st.finalText, failed: false, failureDetail: null, authError: false, sessionId: session.sessionId }; } }