From b5210471fbadcd8cb43ad7af499b9ff587bf6740 Mon Sep 17 00:00:00 2001 From: Abdul Wahab Date: Thu, 10 Sep 2026 00:48:15 +0500 Subject: [PATCH] Address review: the generate event resolves the agent target it serves A winning overlay could reload after handleGo() minted a session but before its result post landed. The close released its lease, the server replayed the still-pending target, and another tab (or the reloaded page, once it abandoned the unknown session) could claim it and fire a second Go for a request that already had a session. The overlay now names the target on the generate event it fires for it (`agentTarget: {targetId, result}`, the same result it posts), and the helper resolves the pending request the moment that event is accepted, stripping the envelope before journaling. Whichever of the event and the result post lands first answers; a page that dies between Go and its result cannot leave the request pending, and a request whose event never reached the helper is served exactly once by the rescuer. Tests: a Rust integration case and a Node protocol case (claim, Go event without a result post, verdict carries the session, a late claim finds nothing pending, the journal carries no envelope), contract pins for the handoff, and the contract doc. Written with AI assistance (Claude). Co-Authored-By: Claude Fable 5 --- crates/cli/tests/agent_target.rs | 33 +++++++++++++++++++++++++++ crates/live/src/live_server.rs | 19 ++++++++++++++++ docs/CLI-CONTRACT.md | 2 +- skill/scripts/live-browser.js | 30 ++++++++++++++++++++++++- tests/live-agent-target.test.mjs | 36 ++++++++++++++++++++++++++++++ tests/live-browser-source.test.mjs | 10 +++++++++ 6 files changed, 128 insertions(+), 2 deletions(-) diff --git a/crates/cli/tests/agent_target.rs b/crates/cli/tests/agent_target.rs index 1d4a670b4..5aac8037c 100644 --- a/crates/cli/tests/agent_target.rs +++ b/crates/cli/tests/agent_target.rs @@ -468,3 +468,36 @@ fn agent_target_late_overlay_first_no_match_extends_the_grace() { assert!(reported_at.elapsed() < Duration::from_millis(400)); let _ = &mut b; } + +#[test] +fn agent_target_resolves_from_the_generate_event_when_the_result_never_lands() { + let s = Server::start("event-backstop"); + 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(); + assert_eq!(s.claim(&target_id, "tab-a", true)["granted"], serde_json::json!(true)); + // Tab A fires Go: its generate event names the target it serves. Its + // own result post never lands (the page reloaded right after Go). + let result = serde_json::json!({ "ok": true, "matchCount": 1, "sessionId": "aabbccdd", "action": "bolder", "count": 3, "element": { "tag": "h1" } }); + let (status, ack) = post_json(s.port, "/events", serde_json::json!({ + "token": s.token, "type": "generate", "id": "aabbccdd", "action": "bolder", "count": 3, "pageUrl": "/", + "element": { "tagName": "h1", "outerHTML": "

Hero

" }, + "agentTarget": { "targetId": target_id, "result": result }, + })); + assert_eq!(status, 200, "{ack}"); + let (_, verdict) = held.join().unwrap(); + assert_eq!(verdict["sessionId"], serde_json::json!("aabbccdd"), "{verdict}"); + assert_eq!(verdict["targetId"], serde_json::json!(target_id)); + // Nothing is left pending for a rescuer to take over with a second Go. + let late = s.claim(&target_id, "tab-b", true); + assert_eq!(late["granted"], serde_json::json!(false), "{late}"); + assert_eq!(late["pending"], serde_json::json!(false), "{late}"); + // The journal carries the event without the envelope. + let journal = std::fs::read_to_string(s.dir.join(".impeccable/live/sessions/aabbccdd.jsonl")).unwrap(); + assert!(journal.contains("generate"), "{journal}"); + assert!(!journal.contains("agentTarget"), "{journal}"); + let _ = &mut b; +} diff --git a/crates/live/src/live_server.rs b/crates/live/src/live_server.rs index 2b4bd7fab..0b04aa8b3 100644 --- a/crates/live/src/live_server.rs +++ b/crates/live/src/live_server.rs @@ -1183,7 +1183,19 @@ fn handle_events_post( respond(stream, cors, json_res(400, json!({ "error": error }))); return; } + // A generate event may name the agent target it serves. The helper + // resolves that request from the event as well as from + // /agent-target-result, so a page that dies between Go and its result + // cannot leave the request pending for a second Go elsewhere. The + // envelope never reaches the journal or the poller. + let mut msg = msg; let mut msg_obj = msg_obj; + let agent_target = if ty == "generate" { + msg_obj.remove("agentTarget"); + msg.as_object_mut().and_then(|o| o.remove("agentTarget")) + } else { + None + }; crate::server_state::strip_poller_owned_event_fields(&mut msg_obj); let mut st = lock(shared); if ty == "agent_phase" { @@ -1260,6 +1272,13 @@ fn handle_events_post( if ty != "checkpoint" && ty != "variant_mounted" && !orphaned_discard { st.enqueue_event(msg_obj); } + if let Some(Value::Object(envelope)) = agent_target { + if let (Some(Value::String(target_id)), Some(result @ Value::Object(_))) = + (envelope.get("targetId"), envelope.get("result")) + { + st.resolve_agent_target(target_id, result.clone()); + } + } drop(st); respond(stream, cors, json_res(200, json!({ "ok": true }))); } diff --git a/docs/CLI-CONTRACT.md b/docs/CLI-CONTRACT.md index e3b6b2b67..e7457f95b 100644 --- a/docs/CLI-CONTRACT.md +++ b/docs/CLI-CONTRACT.md @@ -1480,7 +1480,7 @@ Binds `127.0.0.1:PORT`. CORS: if request has `Origin` and (origin is loopback ht | `POST /manual-edit-discard?token=&pageUrl=` | 401 | see 10 | | `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-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). A `generate` event on `POST /events` may carry `agentTarget: {targetId, result}`: once the event is accepted, the server resolves that pending target with `result` (the envelope is stripped before journaling and never reaches the poller), so a page that dies between Go and its result cannot leave the request pending for a second Go elsewhere; whichever of the event and the result post lands first answers. | | `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 each overlay's first such report (a late reporter extends the grace by the full window; a page whose element mounts late keeps re-checking while its decline answers `pending:true`, an eligible claim drops its stale report, and the overlay declines rather than posting a result when the element is gone after its claim), 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` | diff --git a/skill/scripts/live-browser.js b/skill/scripts/live-browser.js index 276eb8b95..80659f4b5 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -7211,6 +7211,10 @@ // it, and only the tab that holds the lease can renew it. const AGENT_TARGET_CLIENT_ID = id8(); + // The agent target an agent-initiated Go is serving: set by + // actOnAgentTarget around its handleGo call, read once by handleGo. + let agentTargetForGo = null; + function claimAgentTarget(targetId, report) { return fetch('http://localhost:' + PORT + '/agent-target-claim?token=' + TOKEN, { method: 'POST', @@ -7446,7 +7450,14 @@ updateBarContent('configure'); const input = uiGetById(PREFIX + '-input'); if (input) input.value = msg.prompt || ''; + // The target rides on the generate event too: the helper resolves + // the request from whichever lands first, so a page that dies + // between Go and its result cannot leave the request pending for a + // second Go elsewhere. + const candidate = describeAgentTargetCandidate(el); + agentTargetForGo = { targetId: msg.targetId, matchCount: resolved.matchCount, action: msg.action, count: msg.count, element: candidate }; handleGo(); + agentTargetForGo = null; if (state === 'GENERATING' && currentSessionId) { reply({ ok: true, @@ -7454,7 +7465,7 @@ sessionId: currentSessionId, action: msg.action, count: msg.count, - element: describeAgentTargetCandidate(el), + element: candidate, }); } else { reply({ ok: false, error: 'go_failed', state }); @@ -8175,6 +8186,23 @@ }; if (snapshot.comments.length > 0) basePayload.comments = snapshot.comments; if (snapshot.strokes.length > 0) basePayload.strokes = snapshot.strokes; + if (agentTargetForGo) { + // An agent-initiated Go names the target it serves (see + // actOnAgentTarget): the helper resolves that request from this event + // as well as from the overlay's own result post. + basePayload.agentTarget = { + targetId: agentTargetForGo.targetId, + result: { + ok: true, + matchCount: agentTargetForGo.matchCount, + sessionId: currentSessionId, + action: agentTargetForGo.action, + count: agentTargetForGo.count, + element: agentTargetForGo.element, + }, + }; + agentTargetForGo = null; + } // Hide the interactive overlay so it doesn't linger during generation. hideAnnotOverlay(); diff --git a/tests/live-agent-target.test.mjs b/tests/live-agent-target.test.mjs index 11f4369f1..45eab63a8 100644 --- a/tests/live-agent-target.test.mjs +++ b/tests/live-agent-target.test.mjs @@ -739,6 +739,42 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA } }); + it('resolves the request from the generate event that serves it, so a page that dies before its result cannot leave it pending', 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'); + const claim = await (await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId: 'tab-a', eligible: true, + })).json(); + assert.equal(claim.granted, true); + const result = { ok: true, matchCount: 1, sessionId: 'aabbccdd', action: 'bolder', count: 3, element: { tag: 'h1' } }; + const ack = await postJson(server, '/events', { + token: server.token, type: 'generate', id: 'aabbccdd', action: 'bolder', count: 3, pageUrl: '/', + element: { tagName: 'h1', outerHTML: '

Hero

' }, + agentTarget: { targetId: pushed.targetId, result }, + }); + assert.equal(ack.status, 200); + const verdict = await (await held).json(); + assert.equal(verdict.ok, true); + assert.equal(verdict.sessionId, 'aabbccdd'); + const late = await (await postJson(server, '/agent-target-claim', { + token: server.token, targetId: pushed.targetId, clientId: 'tab-b', eligible: true, + })).json(); + assert.deepEqual(late, { ok: true, granted: false, pending: false }, 'nothing is left for a rescuer to serve twice'); + const journal = readFileSync(join(tmp, '.impeccable/live/sessions/aabbccdd.jsonl'), 'utf-8'); + assert.ok(!journal.includes('agentTarget'), 'the envelope never reaches the journal'); + } 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' }); diff --git a/tests/live-browser-source.test.mjs b/tests/live-browser-source.test.mjs index a306c3b9b..b1b220010 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -865,6 +865,16 @@ describe('live-browser source contracts', () => { /function watchAgentTargetResolution\(msg, lastError\) \{\s*if \(agentTargetOverlayGone\(\) \|\| agentTargetTaken\(msg\.targetId\)\) return;/, 'the late-mount watch stops once this page took the lease', ); + assert.match( + SOURCE, + /agentTargetForGo = \{ targetId: msg\.targetId, matchCount: resolved\.matchCount, action: msg\.action, count: msg\.count, element: candidate \};\s*handleGo\(\);\s*agentTargetForGo = null;/, + 'the target rides on the Go event it serves, so the helper resolves it even if this page dies before its result lands', + ); + assert.match( + SOURCE, + /if \(agentTargetForGo\) \{[\s\S]{0,600}?basePayload\.agentTarget = \{[\s\S]{0,300}?sessionId: currentSessionId/, + 'handleGo attaches the agent target with the session it minted', + ); assert.match( SOURCE, /if \(claim\.granted\) \{ noteAgentTarget\(msg\.targetId, 'acting'\); actOnAgentTarget\(msg\); return; \}/,