Skip to content

Commit 6ceec4f

Browse files
serge-the-devclaude
andcommitted
fix(runner): a handoff whose page was closed is lost, not solved — the poll stops claiming a human acted (#641)
`browserHandoffStatus` returned `{ solved: true }` for a closed page, three lines below a comment stating the rule it broke: *a status poll must never relaunch the browser or claim a lost handoff is done*. The line sat ahead of all three per-reason branches, so it answered for every one of them: - `challenge` — the workflow resumed with the CAPTCHA unsolved AND recorded the page as solved (`solvedChallengeUrl`), which mutes captcha re-detection there for the rest of the round. The false claim silenced the detector that would otherwise have caught it. This is the branch with a demonstrated real-resume rate (4 of 6 handoffs in the production window on the apply instance), so it is the one most likely to be acted on rather than immediately re-detected. - `stuck` — resumed without the human doing the single step the handoff exists for; `humanDone` was never read. - `needs_input` — resumed with `value: undefined`, so `job-apply.ts` skipped the save and the brain re-asked next round for a value it had been handed. A closed page is the same category as a missing session, which the line above already answers `solved: false` correctly: there is nothing to resume ONTO, and the brain's next action would land on a freshly created blank tab. Staying unsolved times the wait out into "not resolved in time", which `browser-run.ts` maps to `escalated` → the board's "Needs you" — visible, and retryable. `needs_input` is the exception and is answered BEFORE page liveness, per the issue: its value arrives out of band through `browserSubmitInput` and never travelled through the page, so a value typed after the tab is gone is still a real answer. Rejected: the issue's simpler shape (treat a closed page exactly like a missing session, `solved: false` for all three). It is one character of change but it also discards a supplied `needs_input` value, which is a second, quieter version of the same dishonesty. Rejected: keeping the branch inline. `runner.ts` was already at its size pin, and #676's note on the ratchet says a pin was the easy answer and the wrong one — so the decision moved to `handoff-status.ts` as a pure function, where the reasoning fits and can be unit-tested per reason. It returns `null` for the one case that genuinely needs the live DOM (an open challenge, whose token clears in the page). Tests: `handoff-status.test.ts` covers every reason with a closed page; `runner-handoff.test.ts` proves the wiring against a real Chrome and a genuinely closed Playwright page (`close()` shuts the context and leaves `takeovers` alone, which is exactly the state the bug is about). Reintroducing the old early return fails 8 of the 10 new tests, including all three real-browser ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent b771120 commit 6ceec4f

4 files changed

Lines changed: 215 additions & 6 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
import { describe, expect, it } from "vitest";
2+
import { resolveHandoffStatus } from "./handoff-status.js";
3+
4+
/**
5+
* A handoff whose page was closed reported `solved: true` (#641) — three lines below a
6+
* comment stating that a status poll must never claim a lost handoff is done. The early
7+
* return sat ahead of all three per-reason branches, so one line answered for every reason.
8+
* There is a case per reason here for that reason: a fix that restores the shortcut for any
9+
* one of them has to fail something.
10+
*/
11+
describe("a handoff whose page is gone", () => {
12+
it("leaves a CHALLENGE unsolved — the captcha is no less unsolved for the tab being shut", () => {
13+
expect(resolveHandoffStatus({ reason: "challenge", pageClosed: true })).toEqual({ solved: false, challenge: null });
14+
});
15+
16+
it("leaves a STUCK handoff unsolved even when the human said they were done", () => {
17+
// Deliberate: `humanDone` says a step was performed, but the page it was performed
18+
// in no longer exists, so the brain has nothing to resume onto — its next action
19+
// would land on a fresh blank tab. Unsolved times out into "not resolved in time",
20+
// which the cloud maps to `escalated` → "Needs you", where the owner can retry.
21+
expect(resolveHandoffStatus({ reason: "stuck", pageClosed: true, humanDone: true })).toEqual({ solved: false, challenge: null });
22+
expect(resolveHandoffStatus({ reason: "stuck", pageClosed: true })).toEqual({ solved: false, challenge: null });
23+
});
24+
25+
it("still delivers a NEEDS_INPUT value, which never travelled through the page", () => {
26+
// The one reason whose answer arrives out of band (browserSubmitInput). Answering it
27+
// after the tab has gone is honest; dropping it would make the brain re-ask for a
28+
// value the owner already typed. `value` must survive — the workflow saves it to the
29+
// Profile only when it is actually present.
30+
expect(resolveHandoffStatus({ reason: "needs_input", pageClosed: true, inputValue: "3 years" })).toEqual({
31+
solved: true,
32+
challenge: null,
33+
value: "3 years",
34+
});
35+
});
36+
37+
it("holds a NEEDS_INPUT handoff that has no value yet", () => {
38+
expect(resolveHandoffStatus({ reason: "needs_input", pageClosed: true })).toMatchObject({ solved: false, challenge: null });
39+
expect(resolveHandoffStatus({ reason: "needs_input", pageClosed: false })).toMatchObject({ solved: false, challenge: null });
40+
});
41+
42+
it("treats an unrecognised reason as a challenge — lost, not done", () => {
43+
// `reason` is a string off the HTTP boundary, so an unknown one must land in the
44+
// conservative branch rather than fall through to `solved: true`.
45+
expect(resolveHandoffStatus({ reason: "something-new", pageClosed: true })).toEqual({ solved: false, challenge: null });
46+
});
47+
});
48+
49+
describe("a handoff whose page is alive", () => {
50+
it("defers a challenge to the live DOM", () => {
51+
// null = "ask the page". Only `browserHandoffStatus` can answer this one, by reading
52+
// the captcha token out of the DOM.
53+
expect(resolveHandoffStatus({ reason: "challenge", pageClosed: false })).toBeNull();
54+
expect(resolveHandoffStatus({ pageClosed: false })).toBeNull();
55+
});
56+
57+
it("resolves a stuck handoff on the human's explicit Resume, and only that", () => {
58+
expect(resolveHandoffStatus({ reason: "stuck", pageClosed: false })).toEqual({ solved: false, challenge: null });
59+
expect(resolveHandoffStatus({ reason: "stuck", pageClosed: false, humanDone: true })).toEqual({ solved: true, challenge: null });
60+
});
61+
});
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/**
2+
* What a handoff-status poll can answer WITHOUT looking at the live page (#641).
3+
*
4+
* A handoff is the runner asking a human for something the brain cannot do: solve a
5+
* challenge, perform one widget step it failed at, or supply a value it must not invent.
6+
* The remote Workflow polls `browserHandoffStatus` until it is told the human acted, and
7+
* `solved: true` is a licence to resume the application — so the only safe answers are the
8+
* ones the runner can actually stand behind.
9+
*
10+
* ── The rule
11+
*
12+
* A handoff whose PAGE IS GONE is LOST, not resolved. It is the same category as a handoff
13+
* whose SESSION is gone (the runner restarted, the takeover expired), which the caller
14+
* already answers `solved: false` under a comment stating the rule outright: *a status poll
15+
* must never relaunch the browser or claim a lost handoff is done.* Three lines below that
16+
* comment sat `if (page.isClosed()) return { solved: true }`, ahead of all three per-reason
17+
* branches, so it overrode every one of them:
18+
*
19+
* - `challenge` — the brain resumed with the CAPTCHA unsolved, and the claim was acted
20+
* on rather than re-checked: the workflow records `solvedChallengeUrl`, and the apply
21+
* loop then suppresses captcha re-detection on that page for the rest of the round. A
22+
* false "solved" muted the detector that would have caught it.
23+
* - `stuck` — resumed without the human doing the single step the handoff exists for;
24+
* `humanDone` was never consulted.
25+
* - `needs_input` — resumed with `value: undefined`, so the workflow skipped the save and
26+
* the brain re-asked on the next round for a value it had been given.
27+
*
28+
* There is nothing to resume ONTO in any of those cases: the page the human was working in
29+
* no longer exists, and the brain's next action would land on a freshly created blank tab.
30+
* Staying unsolved lets the wait time out into "not resolved in time", which the cloud maps
31+
* to `escalated` → the board's "Needs you" column — visible, and retryable by the owner.
32+
*
33+
* ── The one exception, and why it is not one
34+
*
35+
* `needs_input` is answered BEFORE page liveness is considered, because its answer never
36+
* involved the page: the value arrives out of band through `browserSubmitInput`, which
37+
* writes it onto the session. A value the owner typed is a real answer whether or not the
38+
* tab survived, and discarding it would re-ask for something already in hand.
39+
*/
40+
41+
/** The facts a status poll has about a handoff, with no browser in them — so this is pure. */
42+
export interface HandoffFacts {
43+
/** `challenge` (the default for an unrecognised reason), `stuck`, or `needs_input`. */
44+
reason?: string;
45+
/** Whether the page the human was handed is closed. */
46+
pageClosed: boolean;
47+
/** The value supplied through the ask-and-hold channel, if any. */
48+
inputValue?: string;
49+
/** Whether the human explicitly said they were finished. */
50+
humanDone?: boolean;
51+
}
52+
53+
export interface HandoffStatus {
54+
solved: boolean;
55+
challenge: string | null;
56+
value?: string;
57+
}
58+
59+
/**
60+
* The answer to a handoff-status poll, or `null` when only the live DOM can give one — a
61+
* `challenge` on a page that is still open, whose token clears in the page itself.
62+
*/
63+
export function resolveHandoffStatus(facts: HandoffFacts): HandoffStatus | null {
64+
// Out of band, and therefore unaffected by a page that has gone.
65+
if (facts.reason === "needs_input") return { solved: !!facts.inputValue, challenge: null, value: facts.inputValue };
66+
// Lost, not done. See the note above — this line is the fix for #641.
67+
if (facts.pageClosed) return { solved: false, challenge: null };
68+
// A stuck handoff resumes only when the human explicitly clicks Resume — there's
69+
// nothing to auto-detect.
70+
if (facts.reason === "stuck") return { solved: !!facts.humanDone, challenge: null };
71+
return null;
72+
}
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { mkdtempSync, rmSync } from "node:fs";
2+
import { tmpdir } from "node:os";
3+
import { join } from "node:path";
4+
import { afterEach, beforeEach, describe, expect, it } from "vitest";
5+
import { LocalRunner } from "./runner.js";
6+
7+
/**
8+
* The wiring half of #641, against a REAL Chrome and a REAL closed page.
9+
*
10+
* `handoff-status.test.ts` pins the decision; this pins that `browserHandoffStatus` asks it.
11+
* The two are separate because a poll that reasons correctly in a pure function and still
12+
* shortcuts in the method is the bug — the method's own `if (page.isClosed()) return { solved:
13+
* true }` sat below a comment forbidding exactly that, and no test held the ground.
14+
*/
15+
describe("browserHandoffStatus when the page the human was handed is gone (#641)", () => {
16+
let dir: string;
17+
let runner: LocalRunner;
18+
19+
beforeEach(() => {
20+
dir = mkdtempSync(join(tmpdir(), "pags-handoff-"));
21+
runner = new LocalRunner({ host: "127.0.0.1", port: 0, dataDir: dir, headless: true });
22+
});
23+
24+
afterEach(async () => {
25+
await runner.close();
26+
rmSync(dir, { recursive: true, force: true });
27+
}, 30_000);
28+
29+
/**
30+
* Register a real takeover on a real page, then take the browser away underneath it.
31+
*
32+
* `close()` shuts the browser context, which closes every page in it, and deliberately
33+
* leaves `takeovers` alone — so this produces exactly the state the bug is about: a live
34+
* handoff session holding a dead page. That is the real shape of it too: the human closes
35+
* the tab, the site replaces it in a popup flow, or Chrome dies mid-handoff.
36+
*/
37+
async function handoffThenLoseTheBrowser(reason: string, label: string): Promise<string> {
38+
const task = runner.createTask({ type: "job.apply_agent", input: {} });
39+
const handoff = await runner.browserHandoff(task.id, label, reason);
40+
expect(handoff.screenshotBase64).toMatch(/^data:image\/jpeg;base64,/);
41+
expect(runner.listTakeovers()).toContain(task.id);
42+
expect(runner.store.getTask(task.id)?.status).toBe("needs_human");
43+
return task.id;
44+
}
45+
46+
it("reports a CAPTCHA handoff unsolved — it is no less unsolved for the tab being shut", async () => {
47+
const taskId = await handoffThenLoseTheBrowser("challenge", "recaptcha");
48+
await runner.close();
49+
// Was `{ solved: true }`, and the workflow acted on it: it recorded the page as a
50+
// solved challenge, which mutes captcha re-detection there for the rest of the round.
51+
expect(await runner.browserHandoffStatus(taskId)).toEqual({ solved: false, challenge: null });
52+
}, 120_000);
53+
54+
it("reports a STUCK handoff unsolved, even after the human pressed Resume", async () => {
55+
const taskId = await handoffThenLoseTheBrowser("stuck", "Save and Continue");
56+
// The human says they did the step — `resumeTakeover` sets humanDone for a
57+
// workflow-driven task without completing it.
58+
expect(await runner.resumeTakeover(taskId)).toMatchObject({ resumed: true });
59+
await runner.close();
60+
// Still not resumable: the page that step happened in is gone, so the brain would be
61+
// handed a fresh blank tab. Unsolved times out into "not resolved in time" →
62+
// `escalated` → "Needs you", which the owner can see and retry.
63+
expect(await runner.browserHandoffStatus(taskId)).toEqual({ solved: false, challenge: null });
64+
}, 120_000);
65+
66+
it("still delivers a NEEDS_INPUT value supplied after the tab was gone", async () => {
67+
const taskId = await handoffThenLoseTheBrowser("needs_input", "Years of Python experience");
68+
await runner.close();
69+
// Nothing has been supplied yet: unsolved, and — the part the old shortcut got wrong
70+
// in the other direction — carrying no value for the workflow to save.
71+
expect(await runner.browserHandoffStatus(taskId)).toMatchObject({ solved: false, value: undefined });
72+
// The value channel never involved the page, so it still works.
73+
expect(runner.browserSubmitInput(taskId, "3 years")).toEqual({ ok: true });
74+
expect(await runner.browserHandoffStatus(taskId)).toEqual({ solved: true, challenge: null, value: "3 years" });
75+
}, 120_000);
76+
});

‎packages/browser-runner/src/runner.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { isTerminalTaskStatus, settleTaskOutcome } from "./apply-outcome.js";
1010
import { McpRuntime } from "./mcp-runtime.js";
1111
import { commitLabelRe, ELEMENT_PROBE_FN, FOCUS_PROBE_FN, refuseClick, refuseKey, type CommitGuardSpec, type ElementFacts, type FocusFacts } from "./commit-guard.js";
1212
import { HumanHandoffError, RunnerInputError } from "./errors.js";
13+
import { resolveHandoffStatus } from "./handoff-status.js";
1314
import { RunnerStore } from "./store.js";
1415
import { CodingRuntime } from "./coding/runtime.js";
1516
import { WORKFLOW_DRIVEN_TASKS } from "./task-types.js";
@@ -1162,12 +1163,11 @@ export class LocalRunner {
11621163
// must never relaunch the browser or claim a lost handoff is done.
11631164
if (!session) return { solved: false, challenge: null };
11641165
const page = session.page;
1165-
if (page.isClosed()) return { solved: true, challenge: null };
1166-
// A needs_input handoff resumes once the user supplies the value.
1167-
if (session.reason === "needs_input") return { solved: !!session.inputValue, challenge: null, value: session.inputValue };
1168-
// A stuck handoff resumes only when the human explicitly clicks Resume —
1169-
// there's nothing to auto-detect.
1170-
if (session.reason === "stuck") return { solved: !!session.humanDone, challenge: null };
1166+
// Every answer that needs no live DOM — including a GONE page, which used to report
1167+
// solved:true and resume past an unsolved challenge (#641) — comes from
1168+
// handoff-status.ts; null means only an open challenge's own DOM can answer.
1169+
const offline = resolveHandoffStatus({ reason: session.reason, pageClosed: page.isClosed(), inputValue: session.inputValue, humanDone: session.humanDone });
1170+
if (offline) return offline;
11711171
// A challenge resumes when the token/widget clears OR the human clicks Done —
11721172
// custom captchas (e.g. PageUp's "not a robot") have no detectable token, so
11731173
// the human's explicit Done is the authority; never strand them.

0 commit comments

Comments
 (0)