From 76db41821296b4da14af3c60c520ed9c17c293f5 Mon Sep 17 00:00:00 2001 From: Abdul Wahab Date: Thu, 10 Sep 2026 01:15:48 +0500 Subject: [PATCH] Address review: every answered agent target fences a late Go Only targets answered with a session were fenced against a delayed generate event. A request that timed out (or ended in another failure verdict the CLI already reported) was simply forgotten, so a Go whose capture outlasted the timeout still opened a session nobody was told about. `resolve_agent_target` now records every terminal resolution, with the answering session when the verdict carried one, and `agent_target_refusal` refuses a generate event for any answered target unless it comes from the answering session itself. The browser_timeout instructions no longer send the agent to live-status for a session that can no longer start. The overlay's refusal toast covers both causes. Tests: a Rust integration case and a Node protocol case (claim, time out, late Go refused with 409 and nothing journaled), a unit test for the timeout instruction; contract doc updated. Written with AI assistance (Claude). Co-Authored-By: Claude Fable 5 --- crates/cli/tests/agent_target.rs | 25 +++++++++++- crates/live/src/live_generate.rs | 11 ++++- crates/live/src/live_server.rs | 14 ++++--- crates/live/src/server_state.rs | 70 ++++++++++++++++++++------------ docs/CLI-CONTRACT.md | 2 +- skill/scripts/live-browser.js | 11 ++--- tests/live-agent-target.test.mjs | 29 +++++++++++++ 7 files changed, 123 insertions(+), 39 deletions(-) diff --git a/crates/cli/tests/agent_target.rs b/crates/cli/tests/agent_target.rs index 43fe008c0..568c82b7b 100644 --- a/crates/cli/tests/agent_target.rs +++ b/crates/cli/tests/agent_target.rs @@ -145,7 +145,10 @@ impl Server { .spawn() .expect("spawn live-server"); let pid_file = dir.join(".impeccable/live/server.json"); - assert!(wait_for(&pid_file, 10), "server pid file never appeared"); + // Sixteen servers spawn at once under the default test parallelism; a + // loaded machine has taken more than ten seconds to write the first + // pid file. + assert!(wait_for(&pid_file, 30), "server pid file never appeared"); let info: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&pid_file).unwrap()).unwrap(); let port = info["port"].as_u64().expect("port") as u16; @@ -559,3 +562,23 @@ fn agent_target_welcomes_the_generate_event_of_the_session_that_answered() { assert_eq!(status, 200, "{body}"); assert!(s.dir.join(".impeccable/live/sessions/cccccccc.jsonl").exists()); } + +#[test] +fn agent_target_fences_a_generate_event_that_lands_after_the_timeout() { + let s = Server::start("fenced"); + 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(); + assert_eq!(s.claim(&target_id, "tab-a", true)["granted"], serde_json::json!(true)); + // The holder never answers: the request times out (400ms) and the CLI + // reports it. Its Go lands after that: refused, nothing journaled, so + // no session exists that the agent was never told about. + let (_, verdict) = held.join().unwrap(); + assert_eq!(verdict["error"], serde_json::json!("browser_timeout"), "{verdict}"); + let (status, body) = post_json(s.port, "/events", generate_event_for(&s, &target_id, "eeeeeeee", "tab-a")); + assert_eq!(status, 409, "{body}"); + assert_eq!(body["error"], serde_json::json!("agent_target_already_served")); + assert!(body.get("sessionId").is_none(), "{body}"); + assert!(!s.dir.join(".impeccable/live/sessions/eeeeeeee.jsonl").exists()); +} diff --git a/crates/live/src/live_generate.rs b/crates/live/src/live_generate.rs index 5869d2d8c..6e9702db1 100644 --- a/crates/live/src/live_generate.rs +++ b/crates/live/src/live_generate.rs @@ -126,7 +126,7 @@ fn instructions_for(result: &Map, self_cmd: &str) -> Option "No page with the live overlay is connected. Open the app URL that serves a pageFiles entry yourself with your harness browser tool, then rerun this command. Only when no browser tool exists: give the user the URL and rerun with --wait-for-browser 120000 so the command fires as soon as they open the page.".to_string(), - "browser_timeout" => format!("The overlay did not answer in time. The page may be mid-reload: run {} live-status to check whether a session started anyway, reload the app page, then rerun this command.", self_cmd), + "browser_timeout" => "The overlay did not answer in time, and no session was started for this request (a Go that lands late is refused). The page may be mid-reload: reload the app page, then rerun this command.".to_string(), "invalid_selector" => "The selector is not valid CSS. Fix the selector syntax and rerun.".to_string(), "no_match" => { if n("rawMatchCount") > 0 { @@ -366,6 +366,15 @@ mod tests { m } + #[test] + fn timeout_instructions_promise_no_stray_session() { + let mut m = Map::new(); + m.insert("ok".into(), json!(false)); + m.insert("error".into(), json!("browser_timeout")); + let text = instructions_for(&m, "impeccable").unwrap(); + assert!(text.contains("no session was started"), "{text}"); + } + #[test] fn busy_instructions_tell_the_agent_whose_session_is_in_the_way() { let own = instructions_for(&busy("agent_target_in_flight"), "impeccable").unwrap(); diff --git a/crates/live/src/live_server.rs b/crates/live/src/live_server.rs index bd5bf32ce..9b82cc99d 100644 --- a/crates/live/src/live_server.rs +++ b/crates/live/src/live_server.rs @@ -184,7 +184,7 @@ pub fn run(args: &[String], io: &mut Io) -> i32 { next_apply_timer_gen: 0, pending_agent_targets: Vec::new(), next_agent_target_timer_gen: 0, - served_agent_targets: Vec::new(), + resolved_agent_targets: Vec::new(), shutting_down: false, cleaned_up: false, log_tx, @@ -1240,17 +1240,19 @@ fn handle_events_post( } } if let Some(envelope) = agent_target.as_ref().and_then(Value::as_object) { - if let Some(served) = st.agent_target_served_elsewhere(envelope, id_str.as_deref()) { + if let Some(refusal) = st.agent_target_refusal(envelope, id_str.as_deref()) { // A superseded Go: this page's lease lapsed while it was still - // capturing and another page served the request. Journal - // nothing, so one request never gets two sessions. + // capturing and another page served the request, or the + // request was already answered (a timeout or a failure the CLI + // has reported). Journal nothing, so one request never gets a + // second session, or a session nobody was told about. drop(st); let mut body = json!({ "error": "agent_target_already_served", "targetId": envelope.get("targetId").cloned().unwrap_or(Value::Null), }); - if !served.is_empty() { - body["sessionId"] = Value::String(served); + if let Some(sid) = refusal.session_id { + body["sessionId"] = Value::String(sid); } respond(stream, cors, json_res(409, body)); return; diff --git a/crates/live/src/server_state.rs b/crates/live/src/server_state.rs index 168c1d5a5..393bddb1c 100644 --- a/crates/live/src/server_state.rs +++ b/crates/live/src/server_state.rs @@ -45,6 +45,13 @@ pub struct SseClient { } /// One overlay's roll-call report on an agent target: its busy state and why. +/// A generate event refused because its agent target is spoken for; see +/// `ServerState::agent_target_refusal`. +pub struct AgentTargetRefusal { + /// The session that answered the request, when the verdict carried one. + pub session_id: Option, +} + pub struct AgentTargetReport { pub client_id: String, pub state: Value, @@ -116,10 +123,12 @@ pub struct ServerState { /// Held-open agent targets keyed by targetId, in arrival order. pub pending_agent_targets: Vec<(String, AgentTargetPending)>, pub next_agent_target_timer_gen: u64, - /// Agent targets answered with a session, oldest first (bounded): a - /// generate event that names one of these under another session id is - /// a superseded Go and is refused. - pub served_agent_targets: Vec<(String, String)>, + /// Every agent target already answered, oldest first (bounded), with + /// the session that answered it when the verdict carried one: a + /// generate event that names one of these under another session id, or + /// after a verdict without a session (a timeout, a failure), is a + /// superseded Go and is refused. + pub resolved_agent_targets: Vec<(String, Option)>, pub last_poll_at: i64, pub timed_out_apply_ids: Vec<(String, TimedOutApply)>, pub next_poll_id: u64, @@ -820,30 +829,36 @@ impl ServerState { return false; }; let (_, pending) = self.pending_agent_targets.remove(pos); - if result.get("ok") == Some(&Value::Bool(true)) { - if let Some(sid) = result.get("sessionId").and_then(Value::as_str) { - self.served_agent_targets - .push((target_id.to_string(), sid.to_string())); - if self.served_agent_targets.len() > 64 { - self.served_agent_targets.remove(0); - } - } + let session = if result.get("ok") == Some(&Value::Bool(true)) { + result + .get("sessionId") + .and_then(Value::as_str) + .map(str::to_string) + } else { + None + }; + self.resolved_agent_targets + .push((target_id.to_string(), session)); + if self.resolved_agent_targets.len() > 64 { + self.resolved_agent_targets.remove(0); } let _ = pending.tx.send(result); true } - /// Whether a generate event naming `envelope.targetId`, sent by - /// `envelope.clientId` under `session_id`, is a superseded Go: the - /// target is still pending but another page holds a live lease on it - /// (this page's lease lapsed while it was capturing), or the request - /// was already answered with a different session. Returns the serving - /// session id, empty while the rival has not minted one yet. - pub fn agent_target_served_elsewhere( + /// Why a generate event naming `envelope.targetId`, sent by + /// `envelope.clientId` under `session_id`, must not open a session: + /// the target is still pending but another page holds a live lease on + /// it (this page's lease lapsed while it was capturing), or the request + /// was already answered, with a different session or with none (a + /// timeout or a failure verdict the CLI has already reported). None + /// when the event is welcome, which includes the answering session's + /// own event. + pub fn agent_target_refusal( &self, envelope: &Map, session_id: Option<&str>, - ) -> Option { + ) -> Option { let target_id = envelope.get("targetId").and_then(Value::as_str)?; let client_id = envelope .get("clientId") @@ -856,17 +871,22 @@ impl ServerState { { return match &pending.owner { Some(owner) if owner != client_id && pending.claimed_until > now_i64() => { - Some(String::new()) + Some(AgentTargetRefusal { session_id: None }) } _ => None, }; } - self.served_agent_targets + let (_, answered_by) = self + .resolved_agent_targets .iter() .rev() - .find(|(t, _)| t == target_id) - .filter(|(_, sid)| Some(sid.as_str()) != session_id) - .map(|(_, sid)| sid.clone()) + .find(|(t, _)| t == target_id)?; + if answered_by.as_deref() == session_id && session_id.is_some() { + return None; + } + Some(AgentTargetRefusal { + session_id: answered_by.clone(), + }) } /// Every connected overlay has declined: answer busy now, not at the diff --git a/docs/CLI-CONTRACT.md b/docs/CLI-CONTRACT.md index edba94c28..589612762 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). 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. The envelope also carries `clientId`: a generate event naming a target that another page now holds (a live lease, this page's having lapsed while it captured) or that was already answered with a different session is refused with 409 `{"error":"agent_target_already_served", targetId, sessionId?}` and journals nothing, and the overlay drops that local session. | +| `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. The envelope also carries `clientId`: a generate event naming a target that another page now holds (a live lease, this page's having lapsed while it captured) or that was already answered, with a different session or with none (a timeout or a failure verdict the CLI has reported), is refused with 409 `{"error":"agent_target_already_served", targetId, sessionId?}` and journals nothing, and the overlay drops that local session; the answering session's own event is welcome. | | `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 0ecfee894..5a0b9b7cc 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -7736,9 +7736,10 @@ }).then(async res => { if (res.ok) return res; const body = await res.json().catch(() => ({})); - // The helper refused to open a second session for an agent target - // another page already served (this page's lease lapsed while it was - // capturing): drop the local session and hand the surface back. + // The helper refused to open a session for an agent target it has + // already answered (another page served it after this page's lease + // lapsed mid-capture, or the request timed out): drop the local + // session and hand the surface back. if (body.error === 'agent_target_already_served' && msg.type === 'generate' && msg.id && msg.id === currentSessionId) { abandonSupersededGo(msg.id); @@ -7767,10 +7768,10 @@ function abandonSupersededGo(sessionId) { if (sessionId !== currentSessionId) return; - console.warn('[impeccable] Another page already served this agent target; clearing session ' + sessionId + '.'); + console.warn('[impeccable] The helper already answered this agent target; clearing session ' + sessionId + '.'); markSessionHandled(); cleanup({ instantChrome: true }); - showToast('Another tab already served this request, so this session was cleared.', 6000); + showToast('The helper already answered this request, so this session was cleared. Pick an element to start fresh.', 6000); } let abandonedForeignSessionId = null; diff --git a/tests/live-agent-target.test.mjs b/tests/live-agent-target.test.mjs index b9e37a099..5d8c78ecf 100644 --- a/tests/live-agent-target.test.mjs +++ b/tests/live-agent-target.test.mjs @@ -808,6 +808,35 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA } }); + it('fences a generate event that lands after the request timed out, so no session the agent was never told about starts', async () => { + const tabA = await openSseClient(server, { clientId: 'tab-a' }); + try { + await tabA.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 verdict = await (await held).json(); + assert.equal(verdict.error, 'browser_timeout'); + const late = await postJson(server, '/events', { + token: server.token, type: 'generate', id: 'eeeeeeee', action: 'bolder', count: 3, pageUrl: '/', + element: { tagName: 'h1', outerHTML: '

Hero

' }, + agentTarget: { targetId: pushed.targetId, clientId: 'tab-a', result: { ok: true, matchCount: 1, sessionId: 'eeeeeeee', action: 'bolder', count: 3 } }, + }); + assert.equal(late.status, 409); + const body = await late.json(); + assert.equal(body.error, 'agent_target_already_served'); + assert.equal(body.sessionId, undefined); + assert.ok(!existsSync(join(tmp, '.impeccable/live/sessions/eeeeeeee.jsonl')), 'a fenced Go journals nothing'); + } finally { + tabA.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' });