diff --git a/crates/cli/tests/agent_target.rs b/crates/cli/tests/agent_target.rs index 0f054b95c..5c94dbf00 100644 --- a/crates/cli/tests/agent_target.rs +++ b/crates/cli/tests/agent_target.rs @@ -274,8 +274,8 @@ fn agent_target_roll_call_answers_busy_once_every_overlay_declined() { let held = s.hold(serde_json::json!({})); let pushed = a.next(|m| m["type"] == "agent_target"); let target_id = pushed["targetId"].as_str().unwrap().to_string(); - assert_eq!(s.claim(&target_id, "tab-a", false), serde_json::json!({ "ok": true, "granted": false })); - assert_eq!(s.claim(&target_id, "tab-b", false), serde_json::json!({ "ok": true, "granted": false })); + assert_eq!(s.claim(&target_id, "tab-a", false), serde_json::json!({ "ok": true, "granted": false, "pending": true }), "the first decline leaves the request pending"); + assert_eq!(s.claim(&target_id, "tab-b", false), serde_json::json!({ "ok": true, "granted": false, "pending": false }), "the last decline completes the roll call"); let (_, verdict) = held.join().unwrap(); assert_eq!(verdict["error"], serde_json::json!("busy")); assert_eq!(verdict["state"], serde_json::json!("CYCLING")); @@ -370,7 +370,7 @@ fn agent_target_roll_call_counts_overlays_not_connections() { let target_id = a.next(|m| m["type"] == "agent_target")["targetId"].as_str().unwrap().to_string(); // One overlay behind two connections reports busy once: that completes // the roll call instead of waiting on a "second" report until timeout. - assert_eq!(s.claim(&target_id, "tab-a", false), serde_json::json!({ "ok": true, "granted": false })); + assert_eq!(s.claim(&target_id, "tab-a", false), serde_json::json!({ "ok": true, "granted": false, "pending": false }), "one overlay behind two connections completes the roll call alone"); let (_, verdict) = held.join().unwrap(); assert_eq!(verdict["error"], serde_json::json!("busy")); assert!(started.elapsed() < Duration::from_millis(350), "the busy verdict did not wait for the timeout"); @@ -390,8 +390,8 @@ fn agent_target_answers_the_resolution_verdict_when_no_page_can_serve() { // 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 })); + assert_eq!(post_json(s.port, "/agent-target-claim", decline("tab-a", 0)).1, serde_json::json!({ "ok": true, "granted": false, "pending": true })); + assert_eq!(post_json(s.port, "/agent-target-claim", decline("tab-b", 2)).1, serde_json::json!({ "ok": true, "granted": false, "pending": false }), "the last decline completes the roll call"); let (_, verdict) = held.join().unwrap(); assert_eq!(verdict["error"], serde_json::json!("no_match"), "{verdict}"); assert_eq!(verdict["ok"], serde_json::json!(false)); @@ -412,7 +412,7 @@ fn agent_target_prefers_busy_over_no_match_across_pages() { // 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 })); + assert_eq!(s.claim(&target_id, "tab-a", false), serde_json::json!({ "ok": true, "granted": false, "pending": false })); let (_, verdict) = held.join().unwrap(); assert_eq!(verdict["error"], serde_json::json!("busy"), "{verdict}"); assert_eq!(verdict["reason"], serde_json::json!("session_active")); diff --git a/crates/live/src/server_state.rs b/crates/live/src/server_state.rs index 1631f7519..0a1c4a8e0 100644 --- a/crates/live/src/server_state.rs +++ b/crates/live/src/server_state.rs @@ -894,7 +894,12 @@ impl ServerState { pending.claimed_until = 0; } self.maybe_complete_agent_target_roll_call(target_id); - return json!({ "ok": true, "granted": false }); + // `pending` tells a declining overlay whether to keep watching + // for a change of its word (an element that mounts late, a + // session that ends); false once the roll call or a result + // resolved the request. + let still_pending = self.pending_agent_targets.iter().any(|(k, _)| k == target_id); + return json!({ "ok": true, "granted": false, "pending": still_pending }); } // An eligible claim is the client's latest word: drop any earlier // busy report, so a busy verdict only ever counts tabs still busy. diff --git a/docs/CLI-CONTRACT.md b/docs/CLI-CONTRACT.md index e902334bd..c8a5ee0d2 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, 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}`. | +| `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, pending}` (`pending` false once the request resolved, so a declining overlay knows whether to keep watching for a change of its word), 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 a12c7ec64..76cf0ce1e 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -7236,6 +7236,7 @@ function declineAgentTargetBusy(msg, busy) { busyDeclinedTargets.set(msg.targetId, msg); + noteAgentTarget(msg.targetId, 'declined'); claimAgentTarget(msg.targetId, { eligible: false, state, reason: busy }); } @@ -7265,7 +7266,8 @@ 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.granted) { noteAgentTarget(msg.targetId, 'acting'); actOnAgentTarget(msg); return; } + noteAgentTarget(msg.targetId, 'denied'); if (!claim.pending) return; setTimeout(() => claimAndActOnAgentTarget(msg), AGENT_TARGET_RESCUE_RETRY_MS); }); @@ -7279,11 +7281,18 @@ } } - // Targets this page already answered (claimed, declined, or acted on). - // The server replays pending targets to every connection that opens, and - // an EventSource reconnect opens one for a page that already heard the - // target, so a replay must not start a second claim or a second Go. - const agentTargetsSeen = []; + // This page's participation in each target it heard: 'acting' once a + // claim was granted (so a replay never starts a second Go), else the word + // it last gave. The server replays pending targets to every connection + // that opens. After a reconnect that overlapped the old connection the + // server still holds this page's word; after one that did not, it dropped + // the word on the close, so a replayed target is handled again: a busy or + // unresolvable page re-declines (idempotent), an idle page claims. + const agentTargetsSeen = new Map(); + function noteAgentTarget(targetId, status) { + agentTargetsSeen.set(targetId, status); + if (agentTargetsSeen.size > 100) agentTargetsSeen.delete(agentTargetsSeen.keys().next().value); + } // 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 @@ -7298,6 +7307,13 @@ // moment the element mounts, and only the last miss is reported. The // server's timeout still bounds the whole exchange. const AGENT_TARGET_RESOLVE_RETRY_MS = [300, 700, 1500]; + // After the quick re-checks the page reports the miss (so the roll call + // can complete on the other overlays' words) and keeps re-checking at + // this cadence for as long as the server says the request is pending, + // claiming the moment the element mounts; the server drops the stale + // report on an eligible claim and ends the watch by answering + // pending:false once the request resolved or timed out. + const AGENT_TARGET_RESOLVE_WATCH_MS = 1000; function declineAgentTargetUnresolvable(msg) { const probe = resolveAgentTargetElement(msg); @@ -7308,7 +7324,11 @@ function retryAgentTargetResolution(msg, attempt, lastError) { if (attempt >= AGENT_TARGET_RESOLVE_RETRY_MS.length) { - claimAgentTarget(msg.targetId, { eligible: false, state, reason: 'no_match', result: lastError }); + noteAgentTarget(msg.targetId, 'declined'); + claimAgentTarget(msg.targetId, { eligible: false, state, reason: 'no_match', result: lastError }).then((answer) => { + if (!answer.pending) return; + setTimeout(() => watchAgentTargetResolution(msg, lastError), AGENT_TARGET_RESOLVE_WATCH_MS); + }); return; } setTimeout(() => { @@ -7321,11 +7341,24 @@ }, AGENT_TARGET_RESOLVE_RETRY_MS[attempt]); } + function watchAgentTargetResolution(msg, lastError) { + if (agentTargetOverlayGone() || agentTargetsSeen.get(msg.targetId) === 'acting') return; + const busy = agentTargetBusyReason(); + if (busy) { declineAgentTargetBusy(msg, busy); return; } + const probe = resolveAgentTargetElement(msg); + if (!probe.error) { claimAndActOnAgentTarget(msg); return; } + // Still unresolvable: re-decline (idempotent) and let the answer say + // whether to keep watching. + claimAgentTarget(msg.targetId, { eligible: false, state, reason: 'no_match', result: probe.error || lastError }).then((answer) => { + if (!answer.pending) return; + setTimeout(() => watchAgentTargetResolution(msg, lastError), AGENT_TARGET_RESOLVE_WATCH_MS); + }); + } + function handleAgentTarget(msg) { if (!msg || typeof msg.targetId !== 'string') return; - if (agentTargetsSeen.includes(msg.targetId)) return; - agentTargetsSeen.push(msg.targetId); - if (agentTargetsSeen.length > 100) agentTargetsSeen.shift(); + if (agentTargetsSeen.get(msg.targetId) === 'acting') return; + noteAgentTarget(msg.targetId, 'heard'); const busy = agentTargetBusyReason(); if (busy) { // Roll call: a busy tab reports itself and never acts. The server diff --git a/tests/live-agent-target.test.mjs b/tests/live-agent-target.test.mjs index c19e2da6f..85cc18d8a 100644 --- a/tests/live-agent-target.test.mjs +++ b/tests/live-agent-target.test.mjs @@ -358,11 +358,11 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA token: server.token, selector: 'h1', action: 'bolder', count: 3, }); const pushed = await tabA.next((m) => m.type === 'agent_target'); - for (const clientId of ['tab-a', 'tab-b']) { + for (const [clientId, pending] of [['tab-a', true], ['tab-b', false]]) { const report = await (await postJson(server, '/agent-target-claim', { token: server.token, targetId: pushed.targetId, clientId, eligible: false, state: 'CYCLING', reason: 'session_active', })).json(); - assert.deepEqual(report, { ok: true, granted: false }); + assert.deepEqual(report, { ok: true, granted: false, pending }, 'a decline says whether the request is still pending'); } const verdict = await (await held).json(); assert.equal(verdict.error, 'busy'); @@ -654,12 +654,12 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA 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]]) { + for (const [clientId, raw, pending] of [['tab-a', 0, true], ['tab-b', 3, false]]) { 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 }); + assert.deepEqual(report, { ok: true, granted: false, pending }); } const verdict = await (await held).json(); assert.equal(verdict.error, 'no_match'); diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index 9585e006c..8a058f837 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -848,8 +848,13 @@ describe('live-browser source contracts', () => { ); assert.match( SOURCE, - /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', + /function handleAgentTarget\(msg\) \{[\s\S]{0,120}?if \(agentTargetsSeen\.get\(msg\.targetId\) === 'acting'\) return;/, + 'a replayed target this page is acting on must not start a second claim or Go; any other replay is handled again', + ); + assert.match( + SOURCE, + /if \(claim\.granted\) \{ noteAgentTarget\(msg\.targetId, 'acting'\); actOnAgentTarget\(msg\); return; \}/, + 'a granted claim marks the target as acting before 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 @@ -871,8 +876,13 @@ describe('live-browser source contracts', () => { ); assert.match( SOURCE, - /function retryAgentTargetResolution\(msg, attempt, lastError\) \{[\s\S]{0,200}?reason: 'no_match', result: lastError[\s\S]{0,600}?if \(!probe\.error\) \{ claimAndActOnAgentTarget\(msg\); return; \}/, - 'the page claims the moment the element mounts, and reports only the last miss', + /function retryAgentTargetResolution\(msg, attempt, lastError\) \{[\s\S]{0,300}?reason: 'no_match', result: lastError[\s\S]{0,120}?if \(!answer\.pending\) return;[\s\S]{0,120}?watchAgentTargetResolution\(msg, lastError\)/, + 'after the quick re-checks the page reports the miss and keeps watching while the server says the request is pending', + ); + assert.match( + SOURCE, + /function watchAgentTargetResolution\(msg, lastError\) \{[\s\S]{0,400}?if \(!probe\.error\) \{ claimAndActOnAgentTarget\(msg\); return; \}[\s\S]{0,500}?if \(!answer\.pending\) return;/, + 'a late mount turns into a claim, and the server ends the watch', ); // 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 @@ -890,8 +900,8 @@ describe('live-browser source contracts', () => { ); assert.equal( (SOURCE.match(/claimAndActOnAgentTarget\(msg\)/g) || []).length, - 5, - 'the first claim, the busy-to-idle re-claim, and the resolution re-check must share the rescue path (definition, three call sites, the retry)', + 6, + 'the first claim, the busy-to-idle re-claim, the resolution re-check, and the resolution watch must share the rescue path (definition, four call sites, the retry)', ); });