From 25263adc5146f9d813ebc5a1547cb083f744ae8c Mon Sep 17 00:00:00 2001 From: Abdul Wahab Date: Wed, 9 Sep 2026 20:39:30 +0500 Subject: [PATCH] Roll call: a page that cannot resolve the target declines instead of claiming Field-testing with two pages open showed the first-wins claim letting the wrong page answer: a tab whose page lacks the element won the claim, resolved the selector locally, and replied no_match while another page had the element. The overlay now resolves the selector before any claim and, when its page cannot resolve it, declines with reason no_match and the resolution verdict; the same check runs on the busy-to-idle re-claim. The server records that verdict on the report and, once every connected overlay has declined, prefers a report that could serve later (a tab mid-session or with an apply in flight, which answers busy so the agent retries) over no_match, and returns the resolution verdict only when no page can serve; the timeout uses the same precedence. Also from the same field tests: a tab on another page of the app was resuming this page's session from the per-origin localStorage cache after a dev-server reload re-initialised it, then sat in GENERATING for a wrapper it never renders and declined every later target. restoreSessionWithoutWrapper now resumes a cached session only on the page that saved it, the check the server-adoption branch beside it already applied. Covered by two Rust integration cases, two protocol cases, and contract assertions for the resolve-before-claim path and the page gate; the cross-page scenario of the field harness passes on a two-page site. AI-assisted: found by field tests and fixed with Claude Code under maintainer direction. Co-Authored-By: Claude Fable 5 --- crates/cli/tests/agent_target.rs | 42 ++++++++++++++++++++++ crates/live/src/live_server.rs | 3 +- crates/live/src/server_state.rs | 32 ++++++++++++++--- docs/CLI-CONTRACT.md | 2 +- skill/scripts/live-browser.js | 22 +++++++++++- tests/live-agent-target.test.mjs | 57 ++++++++++++++++++++++++++++++ tests/live-browser-source.test.mjs | 26 ++++++++++++++ 7 files changed, 177 insertions(+), 7 deletions(-) diff --git a/crates/cli/tests/agent_target.rs b/crates/cli/tests/agent_target.rs index 647d8645a..0f054b95c 100644 --- a/crates/cli/tests/agent_target.rs +++ b/crates/cli/tests/agent_target.rs @@ -376,3 +376,45 @@ fn agent_target_roll_call_counts_overlays_not_connections() { assert!(started.elapsed() < Duration::from_millis(350), "the busy verdict did not wait for the timeout"); let _ = &mut a2; } + +#[test] +fn agent_target_answers_the_resolution_verdict_when_no_page_can_serve() { + let s = Server::start("no-match"); + let mut a = Overlay::connect(s.port, &s.token, "tab-a"); + let mut b = Overlay::connect(s.port, &s.token, "tab-b"); + a.next(|m| m["type"] == "connected"); + b.next(|m| m["type"] == "connected"); + let started = Instant::now(); + let held = s.hold(serde_json::json!({})); + let target_id = a.next(|m| m["type"] == "agent_target")["targetId"].as_str().unwrap().to_string(); + // Both idle pages lack the element: each declines with its resolution + // verdict instead of claiming. + let decline = |cid: &str, raw: u64| serde_json::json!({ "token": s.token, "targetId": target_id, "clientId": cid, "eligible": false, "state": "IDLE", "reason": "no_match", "result": { "ok": false, "error": "no_match", "selector": "h1", "matchCount": 0, "rawMatchCount": raw } }); + assert_eq!(post_json(s.port, "/agent-target-claim", decline("tab-a", 0)).1, serde_json::json!({ "ok": true, "granted": false })); + assert_eq!(post_json(s.port, "/agent-target-claim", decline("tab-b", 2)).1, serde_json::json!({ "ok": true, "granted": false })); + let (_, verdict) = held.join().unwrap(); + assert_eq!(verdict["error"], serde_json::json!("no_match"), "{verdict}"); + assert_eq!(verdict["ok"], serde_json::json!(false)); + assert_eq!(verdict["targetId"], serde_json::json!(target_id)); + assert!(started.elapsed() < Duration::from_millis(350), "answered by the roll call, not the timeout"); + let _ = &mut b; +} + +#[test] +fn agent_target_prefers_busy_over_no_match_across_pages() { + let s = Server::start("busy-wins"); + let mut a = Overlay::connect(s.port, &s.token, "tab-a"); + let mut b = Overlay::connect(s.port, &s.token, "tab-b"); + a.next(|m| m["type"] == "connected"); + b.next(|m| m["type"] == "connected"); + let held = s.hold(serde_json::json!({})); + let target_id = a.next(|m| m["type"] == "agent_target")["targetId"].as_str().unwrap().to_string(); + // The page that has the element is mid-session; the other page lacks it. + // The agent should retry later, so busy outranks no_match. + post_json(s.port, "/agent-target-claim", serde_json::json!({ "token": s.token, "targetId": target_id, "clientId": "tab-b", "eligible": false, "state": "IDLE", "reason": "no_match", "result": { "ok": false, "error": "no_match", "matchCount": 0, "rawMatchCount": 0 } })); + assert_eq!(s.claim(&target_id, "tab-a", false), serde_json::json!({ "ok": true, "granted": false })); + let (_, verdict) = held.join().unwrap(); + assert_eq!(verdict["error"], serde_json::json!("busy"), "{verdict}"); + assert_eq!(verdict["reason"], serde_json::json!("session_active")); + let _ = &mut b; +} diff --git a/crates/live/src/live_server.rs b/crates/live/src/live_server.rs index 6b2837ea8..2b4bd7fab 100644 --- a/crates/live/src/live_server.rs +++ b/crates/live/src/live_server.rs @@ -2797,7 +2797,8 @@ fn handle_agent_target_claim_post( let eligible = msg.get("eligible").and_then(Value::as_bool) == Some(true); let state = msg.get("state").cloned().unwrap_or(Value::Null); let reason = msg.get("reason").cloned().unwrap_or(Value::Null); - let body = lock(shared).claim_agent_target(&target_id, &client_id, eligible, state, reason); + let result = msg.get("result").filter(|r| r.is_object()).cloned(); + let body = lock(shared).claim_agent_target(&target_id, &client_id, eligible, state, reason, result); respond(stream, cors, json_res(200, body)); } diff --git a/crates/live/src/server_state.rs b/crates/live/src/server_state.rs index f2e48d6c9..1631f7519 100644 --- a/crates/live/src/server_state.rs +++ b/crates/live/src/server_state.rs @@ -49,6 +49,9 @@ pub struct AgentTargetReport { pub client_id: String, pub state: Value, pub reason: Value, + /// The overlay's resolution verdict when it declined because its page + /// cannot resolve the target (`reason: no_match`). + pub result: Option, } /// A held-open `POST /agent-target` (the `generate` command): resolved by @@ -780,7 +783,7 @@ impl ServerState { let verdict = if pending.reports.is_empty() { json!({ "ok": false, "error": "browser_timeout", "timeoutMs": timeout_ms }) } else { - agent_target_busy_verdict(pending) + agent_target_verdict_from_reports(pending) }; st.resolve_agent_target(&id, verdict); } @@ -815,7 +818,7 @@ impl ServerState { if p.owner.is_some() || p.reports.is_empty() || p.reports.len() < connected { None } else { - Some(agent_target_busy_verdict(p)) + Some(agent_target_verdict_from_reports(p)) } }); if let Some(verdict) = verdict { @@ -864,6 +867,7 @@ impl ServerState { eligible: bool, state: Value, reason: Value, + result: Option, ) -> Value { let lease_ms = self.agent_target_lease_ms(); let now = now_i64(); @@ -880,6 +884,7 @@ impl ServerState { client_id: client_id.to_string(), state, reason, + result, }); // A holder that turned busy hands the lease back, so the roll // call can complete and an eligible tab's retry is granted at @@ -1478,8 +1483,27 @@ fn env_positive_ms(env: &Env, key: &str) -> Option { .filter(|v| *v > 0) } -/// The busy verdict for a held target: the first report's state and reason. -pub fn agent_target_busy_verdict(pending: &AgentTargetPending) -> Value { +/// The verdict for a held target once every connected overlay declined. A +/// tab that could serve later (mid-session, an apply in flight) outranks a +/// page that simply lacks the element, so the agent retries instead of +/// giving up; only when no page can resolve the target does the resolution +/// verdict (`no_match`, `invalid_selector`, ...) come back. +pub fn agent_target_verdict_from_reports(pending: &AgentTargetPending) -> Value { + let busy = pending + .reports + .iter() + .find(|r| r.reason.as_str() != Some("no_match")) + .or_else(|| pending.reports.first()); + if let Some(r) = busy.filter(|r| r.reason.as_str() != Some("no_match")) { + return json!({ "ok": false, "error": "busy", "state": r.state, "reason": r.reason }); + } + if let Some(result) = pending.reports.iter().find_map(|r| r.result.as_ref()) { + let mut verdict = result.clone(); + if let Some(obj) = verdict.as_object_mut() { + obj.insert("ok".into(), json!(false)); + } + return verdict; + } let first = pending.reports.first(); json!({ "ok": false, diff --git a/docs/CLI-CONTRACT.md b/docs/CLI-CONTRACT.md index 3b29b1041..e902334bd 100644 --- a/docs/CLI-CONTRACT.md +++ b/docs/CLI-CONTRACT.md @@ -1481,7 +1481,7 @@ Binds `127.0.0.1:PORT`. CORS: if request has `Origin` and (origin is loopback ht | `POST /manual-edit` | | 410 `{"error":"/manual-edit is removed; use /manual-edit-stash and /manual-edit-commit for staged copy edits."}` | | `POST /agent-target` | body JSON `token` mismatch → 401 `{"error":"Unauthorized"}`; invalid JSON → 400 `{"error":"Invalid JSON"}` | Agent-initiated targeting (the `generate` command). Validation (400 `{"error":}`, messages verbatim): `agent_target: selector is required`, `agent_target: selector too long` (>1000 chars), `agent_target: invalid action (valid: )`, `agent_target: count must be 1-8`, `agent_target: text must be a string of at most 500 chars`, `agent_target: index must be a positive integer (1-based)`, `agent_target: prompt must be a string of at most 2000 chars`, `agent_target: dryRun must be a boolean`. No SSE client → 200 `{ok:false, error:'no_browser_connected'}`. Otherwise mint an 8-hex `targetId`, broadcast `agent_target` (see 6.2), and **hold the response** until `/agent-target-result` resolves it, every connected overlay has declined (busy roll call, see `/agent-target-claim`), or `IMPECCABLE_AGENT_TARGET_TIMEOUT_MS` (default 15000) elapses: busy verdict `{ok:false, error:'busy', state, reason}` from the first report when any report exists, else `{ok:false, error:'browser_timeout', timeoutMs}`. The held reply is 200 `{targetId, ...result}`; shutdown resolves every held request with `{ok:false, error:'server_stopping'}`. | | `POST /agent-target-result` | 401 / 400 Invalid JSON | `targetId` (non-empty string) required else 400 `{"error":"agent_target_result: missing targetId"}`; the remaining body fields (minus `token`) resolve the held request; 200 `{ok:true, delivered:boolean}` (`delivered:false` when nothing awaits that id). | -| `POST /agent-target-claim` | 401 / 400 Invalid JSON | `targetId` and `clientId` (non-empty strings) required else 400 `{"error":"agent_target_claim: missing targetId or clientId"}`. Roll call plus a first-wins lease, so exactly one overlay acts on a broadcast target. Unknown or resolved target → `{ok:true, granted:false, pending:false}` (ends a rescuer's retry loop). `eligible !== true` → record `{state, reason}` under `clientId` (replacing an earlier report), release the lease if this client holds it, answer `{ok:true, granted:false}`, then complete the roll call when no owner holds the lease and reports ≥ connected SSE clients (verdict from the first report). `eligible === true` → drop this client's earlier report; `granted` when no owner, the same owner (renew), or the lease lapsed (`IMPECCABLE_AGENT_TARGET_CLAIM_LEASE_MS`, default 3000); answer `{ok:true, granted, pending:true}`. | +| `POST /agent-target-claim` | 401 / 400 Invalid JSON | `targetId` and `clientId` (non-empty strings) required else 400 `{"error":"agent_target_claim: missing targetId or clientId"}`. Roll call plus a first-wins lease, so exactly one overlay acts on a broadcast target. Unknown or resolved target → `{ok:true, granted:false, pending:false}` (ends a rescuer's retry loop). `eligible !== true` → record `{state, reason, result?}` under `clientId` (replacing an earlier report; `result` is the overlay's resolution verdict when `reason` is `no_match`, i.e. its page cannot resolve the selector), release the lease if this client holds it, answer `{ok:true, granted:false}`, then complete the roll call when no owner holds the lease and reports ≥ connected overlays. Verdict precedence: a report whose `reason` is not `no_match` (a tab that could serve later) → `{ok:false, error:'busy', state, reason}`; otherwise the first report's `result` (e.g. `no_match` with `rawMatchCount`, `invalid_selector`); the timeout uses the same precedence when any report exists. `eligible === true` → drop this client's earlier report; `granted` when no owner, the same owner (renew), or the lease lapsed (`IMPECCABLE_AGENT_TARGET_CLAIM_LEASE_MS`, default 3000); answer `{ok:true, granted, pending:true}`. | | anything else | | 404 `Not found` | Pending-event summary in `/status.pendingEvents[]`: `{id, type, leased:boolean, leaseUntil:number|null}` plus for `manual_edit_apply`: `pageUrl, chunk, repair, evidencePath, agentAction, manualApplySummary:{pageUrl, chunk, entryCount, opCount, files[]}`. diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index 9d6a3d1ba..055dafa0c 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -7263,6 +7263,7 @@ if (agentTargetOverlayGone()) return; const busy = agentTargetBusyReason(); if (busy) { declineAgentTargetBusy(msg, busy); return; } + if (declineAgentTargetUnresolvable(msg)) return; claimAgentTarget(msg.targetId, { eligible: true }).then((claim) => { if (claim.granted) { actOnAgentTarget(msg); return; } if (!claim.pending) return; @@ -7284,6 +7285,19 @@ // target, so a replay must not start a second claim or a second Go. const agentTargetsSeen = []; + // Only a page that can resolve the target claims it. A tab whose page + // lacks the element declines with its resolution verdict instead, so a + // first-wins claim never lets the wrong page answer for a target that + // another page has. The server prefers a busy report (a tab that could + // serve later) over these, and returns the resolution verdict only when + // no connected page can serve. + function declineAgentTargetUnresolvable(msg) { + const probe = resolveAgentTargetElement(msg); + if (!probe.error) return false; + claimAgentTarget(msg.targetId, { eligible: false, state, reason: 'no_match', result: probe.error }); + return true; + } + function handleAgentTarget(msg) { if (!msg || typeof msg.targetId !== 'string') return; if (agentTargetsSeen.includes(msg.targetId)) return; @@ -7297,6 +7311,7 @@ declineAgentTargetBusy(msg, busy); return; } + if (declineAgentTargetUnresolvable(msg)) return; // Eligible tabs race for the server's lease and only the holder acts. A // hidden tab yields a short head start so a visible one wins when both // exist, and still serves the request on its own: the user finds the @@ -9376,7 +9391,12 @@ void main() { } function restoreSessionWithoutWrapper(reason, activeSessions) { - const cached = loadSession(); + // The session cache is per origin, so a tab on another page of the same + // app sees this page's session too. Only the page that saved it may + // resume it: the server-adoption branch below already applies the same + // check, and a tab on another page has nothing to render for it. + const cachedRaw = loadSession(); + const cached = cachedRaw?.id && !pageMatchesCurrent(cachedRaw.pageUrl) ? null : cachedRaw; // localStorage is a cache, not a gate. A cleared tab, a second browser // profile, or a teardown that dropped local state all leave the durable // server session as the only record of work in progress; adopt it instead diff --git a/tests/live-agent-target.test.mjs b/tests/live-agent-target.test.mjs index 320cf1538..c19e2da6f 100644 --- a/tests/live-agent-target.test.mjs +++ b/tests/live-agent-target.test.mjs @@ -641,6 +641,63 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA } }); + it('answers the resolution verdict when every idle page declined as unable to resolve', async () => { + // Two idle tabs on pages that lack the element decline with their + // resolution verdicts; the request resolves as no_match, not busy. + const tabA = await openSseClient(server, { clientId: 'tab-a' }); + const tabB = await openSseClient(server, { clientId: 'tab-b' }); + try { + await tabA.next((m) => m.type === 'connected'); + await tabB.next((m) => m.type === 'connected'); + const startedAt = Date.now(); + const held = postJson(server, '/agent-target', { + token: server.token, selector: 'h1', action: 'bolder', count: 3, + }); + const pushed = await tabA.next((m) => m.type === 'agent_target'); + for (const [clientId, raw] of [['tab-a', 0], ['tab-b', 3]]) { + const report = await (await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId, eligible: false, state: 'IDLE', reason: 'no_match', + result: { ok: false, error: 'no_match', selector: 'h1', matchCount: 0, rawMatchCount: raw }, + })).json(); + assert.deepEqual(report, { ok: true, granted: false }); + } + const verdict = await (await held).json(); + assert.equal(verdict.error, 'no_match'); + assert.equal(verdict.ok, false); + assert.equal(verdict.targetId, pushed.targetId); + assert.ok(Date.now() - startedAt < 350, 'answered by the roll call, not the timeout'); + } finally { + tabA.close(); + tabB.close(); + } + }); + + it('prefers busy over no_match, so the agent retries when the right page is mid-session', async () => { + const tabA = await openSseClient(server, { clientId: 'tab-a' }); + const tabB = await openSseClient(server, { clientId: 'tab-b' }); + try { + await tabA.next((m) => m.type === 'connected'); + await tabB.next((m) => m.type === 'connected'); + const held = postJson(server, '/agent-target', { + token: server.token, selector: 'h1', action: 'bolder', count: 3, + }); + const pushed = await tabA.next((m) => m.type === 'agent_target'); + await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId: 'tab-b', eligible: false, state: 'IDLE', reason: 'no_match', + result: { ok: false, error: 'no_match', matchCount: 0, rawMatchCount: 0 }, + }); + await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId: 'tab-a', eligible: false, state: 'CYCLING', reason: 'session_active', + }); + const verdict = await (await held).json(); + assert.equal(verdict.error, 'busy'); + assert.equal(verdict.reason, 'session_active'); + } finally { + tabA.close(); + tabB.close(); + } + }); + it('completes the roll call when the last silent overlay disconnects', async () => { // Tab A reported busy; tab B never answered and then left. Every overlay // still connected has declined, so the verdict is busy now, not at the diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index b2958d27d..2338a862f 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -851,6 +851,32 @@ describe('live-browser source contracts', () => { /function handleAgentTarget\(msg\) \{[\s\S]{0,120}?if \(agentTargetsSeen\.includes\(msg\.targetId\)\) return;/, 'a replayed target this page already handled must not start a second claim or Go', ); + // A page that cannot resolve the target never claims it: a first-wins + // claim would otherwise let the wrong page answer no_match for a target + // another page has. + assert.match( + SOURCE, + /function handleAgentTarget\(msg\) \{[\s\S]{0,700}?if \(declineAgentTargetUnresolvable\(msg\)\) return;/, + 'the first claim resolves the selector on this page first', + ); + assert.match( + SOURCE, + /function claimAndActOnAgentTarget\(msg\) \{[\s\S]{0,300}?if \(declineAgentTargetUnresolvable\(msg\)\) return;/, + 'the re-claim resolves the selector on this page first', + ); + assert.match( + SOURCE, + /function declineAgentTargetUnresolvable\(msg\) \{[\s\S]{0,200}?resolveAgentTargetElement\(msg\)[\s\S]{0,200}?reason: 'no_match', result: probe\.error/, + 'the decline carries the resolution verdict for the server to return when no page can serve', + ); + // The per-origin session cache must not let a tab on another page of + // the app resume this page's session (it would sit in GENERATING for a + // wrapper it never renders, and decline every later agent target). + assert.match( + SOURCE, + /function restoreSessionWithoutWrapper\(reason, activeSessions\) \{[\s\S]{0,600}?const cached = cachedRaw\?\.id && !pageMatchesCurrent\(cachedRaw\.pageUrl\) \? null : cachedRaw;/, + 'a cached session is resumed only by the page that saved it', + ); assert.match(helper, /setTimeout\(\(\) => claimAndActOnAgentTarget\(msg\), AGENT_TARGET_RESCUE_RETRY_MS\);/, 'a denied claim on a live request retries until the lease lapses'); assert.match( SOURCE,