diff --git a/crates/cli/tests/agent_target.rs b/crates/cli/tests/agent_target.rs index 5c94dbf00..1263202ef 100644 --- a/crates/cli/tests/agent_target.rs +++ b/crates/cli/tests/agent_target.rs @@ -139,6 +139,7 @@ impl Server { // override exists exactly for this. The lease shrinks with it. .env("IMPECCABLE_AGENT_TARGET_TIMEOUT_MS", "400") .env("IMPECCABLE_AGENT_TARGET_CLAIM_LEASE_MS", "250") + .env("IMPECCABLE_AGENT_TARGET_RESOLVE_GRACE_MS", "150") .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .spawn() @@ -391,12 +392,15 @@ fn agent_target_answers_the_resolution_verdict_when_no_page_can_serve() { // 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, "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"); + // Every page said no_match: the roll call stays open for the resolution + // grace (150ms here), so the last decline is still answered pending. + assert_eq!(post_json(s.port, "/agent-target-claim", decline("tab-b", 2)).1, serde_json::json!({ "ok": true, "granted": false, "pending": true }), "an all-no_match roll call stays open for the grace"); 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 elapsed = started.elapsed(); + assert!(elapsed >= Duration::from_millis(140) && elapsed < Duration::from_millis(380), "answered when the grace lapsed, not before and not by the timeout: {elapsed:?}"); let _ = &mut b; } @@ -418,3 +422,23 @@ fn agent_target_prefers_busy_over_no_match_across_pages() { assert_eq!(verdict["reason"], serde_json::json!("session_active")); let _ = &mut b; } + +#[test] +fn agent_target_lets_a_late_mount_claim_within_the_resolution_grace() { + let s = Server::start("late-mount"); + let mut a = Overlay::connect(s.port, &s.token, "tab-a"); + a.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 only page cannot resolve the target yet: its decline leaves the + // request pending for the grace instead of answering no_match. + let decline = serde_json::json!({ "token": s.token, "targetId": target_id, "clientId": "tab-a", "eligible": false, "state": "IDLE", "reason": "no_match", "result": { "ok": false, "error": "no_match", "matchCount": 0, "rawMatchCount": 0 } }); + assert_eq!(post_json(s.port, "/agent-target-claim", decline).1, serde_json::json!({ "ok": true, "granted": false, "pending": true })); + std::thread::sleep(Duration::from_millis(60)); + // The element mounted: the same page claims and serves. + assert_eq!(s.claim(&target_id, "tab-a", true), serde_json::json!({ "ok": true, "granted": true, "pending": true })); + post_json(s.port, "/agent-target-result", serde_json::json!({ "token": s.token, "targetId": target_id, "ok": true, "sessionId": "aabbccdd" })); + let (_, verdict) = held.join().unwrap(); + assert_eq!(verdict["ok"], serde_json::json!(true), "{verdict}"); + assert_eq!(verdict["sessionId"], serde_json::json!("aabbccdd")); +} diff --git a/crates/live/src/server_state.rs b/crates/live/src/server_state.rs index 0a1c4a8e0..b3798561c 100644 --- a/crates/live/src/server_state.rs +++ b/crates/live/src/server_state.rs @@ -65,6 +65,9 @@ pub struct AgentTargetPending { pub claimed_until: i64, pub reports: Vec, pub timer_gen: u64, + /// While every report says `no_match`, the roll call stays open until + /// this instant: a page whose element mounts late can still claim. + pub resolve_grace_until: Option, } /// One pre-apply file snapshot entry (`{ exists, content }`). @@ -736,6 +739,17 @@ impl ServerState { .unwrap_or(3_000) } + /// A page's `no_match` is a provisional word: an element can mount after + /// the page first looked (a route still rendering, an HMR swap). When + /// every connected overlay says `no_match`, the roll call stays open for + /// this long after the first such report, so a page that keeps watching + /// can still claim; a busy report answers at once regardless. + pub fn agent_target_resolve_grace_ms(&self) -> i64 { + env_positive_ms(&self.env, "IMPECCABLE_AGENT_TARGET_RESOLVE_GRACE_MS") + .map(|v| v as i64) + .unwrap_or(3_000) + } + /// Hold a new agent target: mint its id, broadcast the push, arm the /// timeout. Returns the id and the receiver the route blocks on. pub fn register_agent_target(&mut self, mut payload: Map) -> (String, Receiver) { @@ -763,6 +777,7 @@ impl ServerState { claimed_until: 0, reports: Vec::new(), timer_gen, + resolve_grace_until: None, }, )); self.broadcast(&payload); @@ -810,22 +825,56 @@ impl ServerState { /// whenever a report lands and whenever an overlay leaves. pub fn maybe_complete_agent_target_roll_call(&mut self, target_id: &str) { let connected = self.connected_overlay_count(); + let now = now_i64(); let verdict = self .pending_agent_targets .iter() .find(|(k, _)| k == target_id) .and_then(|(_, p)| { if p.owner.is_some() || p.reports.is_empty() || p.reports.len() < connected { - None - } else { - Some(agent_target_verdict_from_reports(p)) + return None; } + let all_no_match = p.reports.iter().all(|r| r.reason.as_str() == Some("no_match")); + if all_no_match && p.resolve_grace_until.map(|until| now < until).unwrap_or(false) { + // Every page says no_match, but one may still be + // watching a late mount: the grace timer re-runs this + // check when it lapses. + return None; + } + Some(agent_target_verdict_from_reports(p)) }); if let Some(verdict) = verdict { self.resolve_agent_target(target_id, verdict); } } + /// Arm the resolution grace on the first `no_match` report: the roll + /// call is re-judged when it lapses (the lapse alone never resolves; the + /// check re-reads the reports, so a claim or a busy word in between + /// takes precedence). + fn arm_agent_target_resolve_grace(&mut self, target_id: &str) { + let grace_ms = self.agent_target_resolve_grace_ms(); + let Some((_, pending)) = self + .pending_agent_targets + .iter_mut() + .find(|(k, _)| k == target_id) + else { + return; + }; + if pending.resolve_grace_until.is_some() { + return; + } + pending.resolve_grace_until = Some(now_i64() + grace_ms); + let weak = self.self_ref.clone(); + let id = target_id.to_string(); + std::thread::spawn(move || { + std::thread::sleep(Duration::from_millis(grace_ms.max(0) as u64 + 5)); + if let Some(shared) = weak.upgrade() { + lock(&shared).maybe_complete_agent_target_roll_call(&id); + } + }); + } + /// A disconnected overlay's word no longer counts: drop its busy report, /// hand back a lease it held (a rescuer's next claim is granted at once /// instead of after the lease lapses), and re-judge each roll call @@ -879,6 +928,7 @@ impl ServerState { return json!({ "ok": true, "granted": false, "pending": false }); }; if !eligible { + let reason_is_no_match = reason.as_str() == Some("no_match"); pending.reports.retain(|r| r.client_id != client_id); pending.reports.push(AgentTargetReport { client_id: client_id.to_string(), @@ -893,6 +943,9 @@ impl ServerState { pending.owner = None; pending.claimed_until = 0; } + if reason_is_no_match { + self.arm_agent_target_resolve_grace(target_id); + } self.maybe_complete_agent_target_roll_call(target_id); // `pending` tells a declining overlay whether to keep watching // for a change of its word (an element that mounts late, a diff --git a/docs/CLI-CONTRACT.md b/docs/CLI-CONTRACT.md index c8a5ee0d2..ab305dff7 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, 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}`. | +| `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}` at once; when every report is `no_match` the roll call stays open for `IMPECCABLE_AGENT_TARGET_RESOLVE_GRACE_MS` (default 3000) after the first such report (a page whose element mounts late keeps re-checking while its decline answers `pending:true`, and an eligible claim drops its stale report), then answers 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 76cf0ce1e..8b59c454d 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -7306,39 +7306,28 @@ // it is re-checked a few times over about two seconds, claiming the // 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; + // The page reports the miss at once (so the other overlays' words can + // complete the roll call) and keeps re-checking at this cadence for as + // long as the server says the request is pending: the server holds an + // all-no_match roll call open for a short grace precisely so a late mount + // can still be claimed, 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 = 500; function declineAgentTargetUnresolvable(msg) { const probe = resolveAgentTargetElement(msg); if (!probe.error) return false; - retryAgentTargetResolution(msg, 0, probe.error); + reportAgentTargetUnresolvable(msg, probe.error); return true; } - function retryAgentTargetResolution(msg, attempt, lastError) { - if (attempt >= AGENT_TARGET_RESOLVE_RETRY_MS.length) { - 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(() => { - if (agentTargetOverlayGone()) return; - const busy = agentTargetBusyReason(); - if (busy) { declineAgentTargetBusy(msg, busy); return; } - const probe = resolveAgentTargetElement(msg); - if (!probe.error) { claimAndActOnAgentTarget(msg); return; } - retryAgentTargetResolution(msg, attempt + 1, probe.error); - }, AGENT_TARGET_RESOLVE_RETRY_MS[attempt]); + function reportAgentTargetUnresolvable(msg, error) { + noteAgentTarget(msg.targetId, 'declined'); + claimAgentTarget(msg.targetId, { eligible: false, state, reason: 'no_match', result: error }).then((answer) => { + if (!answer.pending) return; + setTimeout(() => watchAgentTargetResolution(msg, error), AGENT_TARGET_RESOLVE_WATCH_MS); + }); } function watchAgentTargetResolution(msg, lastError) { @@ -7347,12 +7336,9 @@ 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); - }); + // Still unresolvable: re-report (idempotent); the answer says whether + // the server is still holding the request open. + reportAgentTargetUnresolvable(msg, probe.error || lastError); } function handleAgentTarget(msg) { diff --git a/tests/live-agent-target.test.mjs b/tests/live-agent-target.test.mjs index 85cc18d8a..d1f63391d 100644 --- a/tests/live-agent-target.test.mjs +++ b/tests/live-agent-target.test.mjs @@ -158,7 +158,7 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA // exists exactly for this. server = await startServer(8497, { cwd: tmp, - env: { IMPECCABLE_AGENT_TARGET_TIMEOUT_MS: '400' }, + env: { IMPECCABLE_AGENT_TARGET_TIMEOUT_MS: '400', IMPECCABLE_AGENT_TARGET_RESOLVE_GRACE_MS: '150' }, }); }); @@ -654,24 +654,56 @@ 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, pending] of [['tab-a', 0, true], ['tab-b', 3, false]]) { + 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, pending }); + // Every page said no_match: the roll call stays open for the + // resolution grace (150ms here), so both declines are answered pending. + assert.deepEqual(report, { ok: true, granted: false, pending: true }); } 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'); + const elapsed = Date.now() - startedAt; + assert.ok(elapsed >= 140 && elapsed < 380, `answered when the grace lapsed, not before and not by the timeout (${elapsed}ms)`); } finally { tabA.close(); tabB.close(); } }); + it('lets a page that declined as unresolvable claim once its element mounts, within the grace', async () => { + const tab = await openSseClient(server, { clientId: 'tab-a' }); + try { + await tab.next((m) => m.type === 'connected'); + const held = postJson(server, '/agent-target', { + token: server.token, selector: 'h1', action: 'bolder', count: 3, + }); + const pushed = await tab.next((m) => m.type === 'agent_target'); + const declined = await (await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId: 'tab-a', eligible: false, state: 'IDLE', reason: 'no_match', + result: { ok: false, error: 'no_match', matchCount: 0, rawMatchCount: 0 }, + })).json(); + assert.deepEqual(declined, { ok: true, granted: false, pending: true }, 'the only page declining leaves the request pending for the grace'); + await new Promise((r) => setTimeout(r, 60)); + const claim = await (await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId: 'tab-a', eligible: true, + })).json(); + assert.deepEqual(claim, { ok: true, granted: true, pending: true }, 'the late mount is served'); + await postJson(server, '/agent-target-result', { + token: server.token, targetId: pushed.targetId, ok: true, matchCount: 1, sessionId: 'aabbccdd', + }); + const verdict = await (await held).json(); + assert.equal(verdict.ok, true); + assert.equal(verdict.sessionId, 'aabbccdd'); + } finally { + tab.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' }); diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index 8a058f837..be1d4f89a 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -871,18 +871,18 @@ describe('live-browser source contracts', () => { ); assert.match( SOURCE, - /function declineAgentTargetUnresolvable\(msg\) \{[\s\S]{0,200}?resolveAgentTargetElement\(msg\)[\s\S]{0,120}?retryAgentTargetResolution\(msg, 0, probe\.error\)/, - 'a failed resolution is re-checked before it becomes this page\'s word', + /function declineAgentTargetUnresolvable\(msg\) \{[\s\S]{0,200}?resolveAgentTargetElement\(msg\)[\s\S]{0,120}?reportAgentTargetUnresolvable\(msg, probe\.error\)/, + 'a failed resolution is reported at once so the roll call can proceed on the other overlays\' words', ); assert.match( SOURCE, - /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', + /function reportAgentTargetUnresolvable\(msg, error\) \{[\s\S]{0,300}?reason: 'no_match', result: error[\s\S]{0,120}?if \(!answer\.pending\) return;[\s\S]{0,120}?watchAgentTargetResolution\(msg, error\)/, + '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', + /function watchAgentTargetResolution\(msg, lastError\) \{[\s\S]{0,400}?if \(!probe\.error\) \{ claimAndActOnAgentTarget\(msg\); return; \}[\s\S]{0,300}?reportAgentTargetUnresolvable\(msg, probe\.error \|\| lastError\)/, + 'a late mount turns into a claim; otherwise the page re-reports 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 @@ -900,8 +900,8 @@ describe('live-browser source contracts', () => { ); assert.equal( (SOURCE.match(/claimAndActOnAgentTarget\(msg\)/g) || []).length, - 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)', + 5, + 'the first claim, the busy-to-idle re-claim, and the resolution watch must share the rescue path (definition, three call sites, the retry)', ); });