diff --git a/crates/cli/tests/agent_target.rs b/crates/cli/tests/agent_target.rs index 1263202ef..1d4a670b4 100644 --- a/crates/cli/tests/agent_target.rs +++ b/crates/cli/tests/agent_target.rs @@ -442,3 +442,29 @@ fn agent_target_lets_a_late_mount_claim_within_the_resolution_grace() { assert_eq!(verdict["ok"], serde_json::json!(true), "{verdict}"); assert_eq!(verdict["sessionId"], serde_json::json!("aabbccdd")); } + +#[test] +fn agent_target_late_overlay_first_no_match_extends_the_grace() { + let s = Server::start("late-grace"); + 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(); + let decline = |cid: &str| serde_json::json!({ "token": s.token, "targetId": target_id, "clientId": cid, "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("tab-a")).1["pending"], serde_json::json!(true)); + // Tab A's grace (150ms) lapses before tab B says its first word. + std::thread::sleep(Duration::from_millis(200)); + let reported_at = Instant::now(); + let answer = post_json(s.port, "/agent-target-claim", decline("tab-b")).1; + assert_eq!(answer["pending"], serde_json::json!(true), "a late overlay's first no_match word extends the grace: {answer}"); + // Tab B's watcher finds the element within its grace and claims. + std::thread::sleep(Duration::from_millis(60)); + assert_eq!(s.claim(&target_id, "tab-b", true)["granted"], serde_json::json!(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["sessionId"], serde_json::json!("aabbccdd"), "{verdict}"); + assert!(reported_at.elapsed() < Duration::from_millis(400)); + let _ = &mut b; +} diff --git a/crates/hook/src/hook.rs b/crates/hook/src/hook.rs index da0e3d655..0f962c40f 100644 --- a/crates/hook/src/hook.rs +++ b/crates/hook/src/hook.rs @@ -240,6 +240,20 @@ pub fn run_hook(rt: &Runtime, stdin: &str) -> RunResult { } } + // A live variant session owns a file carrying preview markers: stand + // down before the per-session edit cap can turn the variants wrap + // into a suppression notice. + if primary_files.contains(file_path) { + if let Ok(bytes) = std::fs::read(file_path) { + if crate::hook_lib::has_live_preview_markers(&String::from_utf8_lossy(&bytes)) { + if live_preview_edit.is_none() { + live_preview_edit = Some(file_path.clone()); + } + last_skip = "live-preview"; + continue; + } + } + } let use_html_engine = match configured { Some(c) => c.engine == "html", None => ext == ".html" || ext == ".htm", diff --git a/crates/hook/tests/hook_tests.rs b/crates/hook/tests/hook_tests.rs index 1a26c681b..614efe546 100644 --- a/crates/hook/tests/hook_tests.rs +++ b/crates/hook/tests/hook_tests.rs @@ -2282,3 +2282,35 @@ fn run_hook_stands_down_for_the_whole_edit_when_the_primary_carries_live_markers let audited = audit_str(&skipped.audit, "file").unwrap_or("").replace('\\', "/"); assert!(audited.ends_with("src/App.jsx"), "the audit names the edited file, not the companion: {audited}"); } + +#[test] +fn run_hook_stands_down_before_the_edit_cap_can_suppress_a_live_file() { + // A file edited past the per-session cap would be skipped as + // "suppressed" (with the notice) before its content is read. A live + // wrap on such a file must stand down instead, every time. + let t = Tmp::new(); + let cwd = t.path(); + std::fs::create_dir_all(t.0.join(".impeccable")).unwrap(); + let r = rt(&cwd); + // Seven plain edits cross the cap: the 7th carries the notice. + let css = t.write("src/b.css", GRADIENT_CSS); + let mut outputs = Vec::new(); + for _ in 0..7 { + outputs.push(hook::run_hook(&r, &edit_event(&cwd, &css, "cap"))); + } + assert_eq!(outputs[6].audit["suppressed"], json!(true)); + assert!(outputs[6].stdout.contains("Suppressing further design hints")); + // Now a live session carbonizes into that same file: stand down, never + // suppress. + t.write( + "src/b.css", + &format!("/* impeccable-carbonize-start ab12cd34 */\n{GRADIENT_CSS}/* impeccable-carbonize-end ab12cd34 */\n"), + ); + for i in 0..3 { + let out = hook::run_hook(&r, &edit_event(&cwd, &css, "cap")); + assert_eq!(out.stdout, "", "edit {i}: nothing emitted"); + assert_eq!(out.audit["skipped"], json!("live-preview"), "edit {i}"); + assert!(out.audit.get("suppressed").is_none(), "edit {i}: {:?}", out.audit); + assert!(out.audit.get("editCount").is_none(), "edit {i}: the cap is not bumped for a live wrap"); + } +} diff --git a/crates/live/src/live_generate.rs b/crates/live/src/live_generate.rs index ac0be1330..5869d2d8c 100644 --- a/crates/live/src/live_generate.rs +++ b/crates/live/src/live_generate.rs @@ -137,7 +137,13 @@ fn instructions_for(result: &Map, self_cmd: &str) -> Option format!("The selector matched {} elements. Either target their common container instead, or disambiguate with --text \"\" or --index <1-based position>. The candidates are listed in this output.", n("matchCount")), "index_out_of_range" => format!("--index is out of range: only {} match(es). Use an index from 1 to {}.", n("matchCount"), n("matchCount")), - "busy" => format!("A live session is already mid-flight (browser state {}). Let the user finish or discard it in the browser, or handle the pending event in your poll loop, then rerun.", s("state")), + "busy" => { + if s("reason") == "agent_target_in_flight" { + "That tab is already acting on another generate request. Handle that request's pending event in your poll loop, or wait for its session to end, then rerun.".to_string() + } else { + format!("A live session is already mid-flight (browser state {}). Let the user finish or discard it in the browser, or handle the pending event in your poll loop, then rerun.", s("state")) + } + } "go_failed" => format!("The overlay could not start generation from the picked state (browser state {}). Reload the app page and rerun this command.", s("state")), "server_stopping" => format!("The live helper server is shutting down. Re-run the live boot ({} live), reopen the page, then rerun this command.", self_cmd), _ => return None, @@ -346,3 +352,25 @@ pub fn run(args: &[String], io: &mut Io) -> i32 { 1 } } + +#[cfg(test)] +mod tests { + use super::*; + + fn busy(reason: &str) -> Map { + let mut m = Map::new(); + m.insert("ok".into(), json!(false)); + m.insert("error".into(), json!("busy")); + m.insert("state".into(), json!("CONFIGURING")); + m.insert("reason".into(), json!(reason)); + m + } + + #[test] + fn busy_instructions_tell_the_agent_whose_session_is_in_the_way() { + let own = instructions_for(&busy("agent_target_in_flight"), "impeccable").unwrap(); + assert!(own.contains("already acting on another generate request"), "{own}"); + let user = instructions_for(&busy("session_active"), "impeccable").unwrap(); + assert!(user.contains("browser state CONFIGURING"), "{user}"); + } +} diff --git a/crates/live/src/server_state.rs b/crates/live/src/server_state.rs index b3798561c..d484447f5 100644 --- a/crates/live/src/server_state.rs +++ b/crates/live/src/server_state.rs @@ -848,10 +848,12 @@ impl ServerState { } } - /// 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). + /// Each overlay's first `no_match` word extends the resolution grace by + /// the full window, so a page that reports after another page's grace + /// lapsed still gets its watch; the roll call is re-judged when the + /// latest grace lapses (the lapse alone never resolves; the check + /// re-reads the reports, so a claim or a busy word in between takes + /// precedence). The target's timeout bounds the sum. fn arm_agent_target_resolve_grace(&mut self, target_id: &str) { let grace_ms = self.agent_target_resolve_grace_ms(); let Some((_, pending)) = self @@ -861,10 +863,11 @@ impl ServerState { else { return; }; - if pending.resolve_grace_until.is_some() { + let until = now_i64() + grace_ms; + if pending.resolve_grace_until.map(|u| u >= until).unwrap_or(false) { return; } - pending.resolve_grace_until = Some(now_i64() + grace_ms); + pending.resolve_grace_until = Some(until); let weak = self.self_ref.clone(); let id = target_id.to_string(); std::thread::spawn(move || { @@ -929,6 +932,12 @@ impl ServerState { }; if !eligible { let reason_is_no_match = reason.as_str() == Some("no_match"); + // Only an overlay's first no_match word extends the grace: its + // re-reports while watching must not keep the roll call open. + let first_no_match_from_client = !pending + .reports + .iter() + .any(|r| r.client_id == client_id && r.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(), @@ -943,7 +952,7 @@ impl ServerState { pending.owner = None; pending.claimed_until = 0; } - if reason_is_no_match { + if reason_is_no_match && first_no_match_from_client { self.arm_agent_target_resolve_grace(target_id); } self.maybe_complete_agent_target_roll_call(target_id); diff --git a/docs/CLI-CONTRACT.md b/docs/CLI-CONTRACT.md index ab305dff7..e3b6b2b67 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}` 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}`. | +| `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` | 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 8b59c454d..54c992095 100644 --- a/skill/scripts/live-browser.js +++ b/skill/scripts/live-browser.js @@ -7221,9 +7221,16 @@ .catch(() => ({ granted: false, pending: false })); } - function agentTargetBusyReason() { + // `exceptTargetId` is the target this call is about: a tab acting on it + // is not busy for itself, but it is busy for every other target, or two + // held requests could both be claimed here and the second Go would + // overwrite the session the first one minted. + function agentTargetBusyReason(exceptTargetId) { if (pendingApplyInFlight) return 'manual_apply_in_flight'; if (state !== 'IDLE' && state !== 'PICKING' && state !== 'CONFIGURING') return 'session_active'; + for (const [targetId, status] of agentTargetsSeen) { + if (status === 'acting' && targetId !== exceptTargetId) return 'agent_target_in_flight'; + } return null; } @@ -7262,7 +7269,7 @@ // and the busy-to-idle re-claim share this. function claimAndActOnAgentTarget(msg) { if (agentTargetOverlayGone()) return; - const busy = agentTargetBusyReason(); + const busy = agentTargetBusyReason(msg.targetId); if (busy) { declineAgentTargetBusy(msg, busy); return; } if (declineAgentTargetUnresolvable(msg)) return; claimAgentTarget(msg.targetId, { eligible: true }).then((claim) => { @@ -7332,7 +7339,7 @@ function watchAgentTargetResolution(msg, lastError) { if (agentTargetOverlayGone() || agentTargetsSeen.get(msg.targetId) === 'acting') return; - const busy = agentTargetBusyReason(); + const busy = agentTargetBusyReason(msg.targetId); if (busy) { declineAgentTargetBusy(msg, busy); return; } const probe = resolveAgentTargetElement(msg); if (!probe.error) { claimAndActOnAgentTarget(msg); return; } @@ -7345,7 +7352,7 @@ if (!msg || typeof msg.targetId !== 'string') return; if (agentTargetsSeen.get(msg.targetId) === 'acting') return; noteAgentTarget(msg.targetId, 'heard'); - const busy = agentTargetBusyReason(); + const busy = agentTargetBusyReason(msg.targetId); if (busy) { // Roll call: a busy tab reports itself and never acts. The server // answers `busy` the moment every connected overlay has reported, so @@ -7363,8 +7370,10 @@ function actOnAgentTarget(msg) { if (agentTargetOverlayGone()) return; - const reply = (result) => postAgentTargetResult(msg.targetId, result); - const busy = agentTargetBusyReason(); + // Every exit ends this tab's acting state, so a later target is not + // refused for a Go that already happened or never will. + const reply = (result) => { noteAgentTarget(msg.targetId, 'done'); postAgentTargetResult(msg.targetId, result); }; + const busy = agentTargetBusyReason(msg.targetId); if (busy) { // Turned busy between claim and act: report it, which also hands the // lease back so the roll call can complete or a rescuer can claim. @@ -7372,7 +7381,13 @@ return; } const resolved = resolveAgentTargetElement(msg); - if (resolved.error) { reply(resolved.error); return; } + if (resolved.error) { + // The element went away between claim and act. A result would end the + // request for every tab; a decline hands the lease back so another + // page or a remount can still serve it. + reportAgentTargetUnresolvable(msg, resolved.error); + return; + } const el = resolved.el; if (msg.dryRun) { reply({ @@ -7392,7 +7407,7 @@ // lease lapsed while it scrolled (a rescuer took over) stops here, so // one request never gets two Go presses. claimAgentTarget(msg.targetId, { eligible: true }).then((renewal) => { - if (!renewal.granted) return; + if (!renewal.granted) { noteAgentTarget(msg.targetId, 'done'); return; } // An insert placement left mid-configure gives way, exactly as a // click outside it does in handleClick. if (state === 'CONFIGURING' && configureKind === 'insert') cancelInsertConfigure(); @@ -12049,8 +12064,11 @@ void main() { /** Full teardown: remove all UI, disconnect SSE, clean up. */ function teardown() { // Declined targets die with the overlay: the IDLE transition below must - // not re-claim a lease this page can no longer act on. + // not re-claim a lease this page can no longer act on. So does the + // target ledger: an 'acting' entry from a Go that never happened must + // not refuse every target the next connection hears. busyDeclinedTargets.clear(); + agentTargetsSeen.clear(); stopAgentStatusPoll(); hideAgentPollTooltip(); if (agentPollTooltipEl) { diff --git a/tests/live-agent-target.test.mjs b/tests/live-agent-target.test.mjs index d1f63391d..11f4369f1 100644 --- a/tests/live-agent-target.test.mjs +++ b/tests/live-agent-target.test.mjs @@ -704,6 +704,41 @@ describe('POST /agent-target', { skip: ENGINE_BIN ? false : ENGINE_MISSING_MESSA } }); + it('extends the grace on a late overlay\'s first no_match word, so it still gets its watch', 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 decline = (clientId) => 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', matchCount: 0, rawMatchCount: 0 }, + }); + assert.equal((await (await decline('tab-a')).json()).pending, true); + // Tab A's grace (150ms) lapses before tab B says its first word. + await new Promise((r) => setTimeout(r, 200)); + const late = await (await decline('tab-b')).json(); + assert.equal(late.pending, true, 'a late overlay\'s first no_match word extends 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-b', eligible: true, + })).json(); + assert.equal(claim.granted, true, 'the late overlay\'s watcher claims within its grace'); + 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.sessionId, 'aabbccdd'); + } 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 be1d4f89a..b3f8a3fd2 100644 --- a/tests/live-browser-source.test.mjs +++ b/tests/live-browser-source.test.mjs @@ -826,6 +826,10 @@ describe('live-browser source contracts', () => { it('re-claims busy-declined agent targets only while the overlay can still serve them', () => { const teardownSource = SOURCE.match(/function teardown\(\) \{[\s\S]*?\n \}/)?.[0] || ''; const clearAt = teardownSource.indexOf('busyDeclinedTargets.clear();'); + assert.ok( + teardownSource.includes('agentTargetsSeen.clear();'), + 'teardown clears the target ledger, so a stale acting entry never refuses the next connection\'s targets', + ); const idleAt = teardownSource.indexOf("setLiveState('IDLE')"); assert.ok(clearAt >= 0 && idleAt > clearAt, 'teardown must drop declined targets before its IDLE transition, or a dead overlay re-claims a lease'); assert.match( @@ -856,6 +860,16 @@ describe('live-browser source contracts', () => { /if \(claim\.granted\) \{ noteAgentTarget\(msg\.targetId, 'acting'\); actOnAgentTarget\(msg\); return; \}/, 'a granted claim marks the target as acting before Go', ); + assert.match( + SOURCE, + /function agentTargetBusyReason\(exceptTargetId\) \{[\s\S]{0,500}?status === 'acting' && targetId !== exceptTargetId\) return 'agent_target_in_flight';/, + 'a tab acting on one target is busy for every other target, so two held requests can never both mint a session here', + ); + assert.match( + SOURCE, + /function actOnAgentTarget\(msg\) \{[\s\S]{0,900}?if \(resolved\.error\) \{[\s\S]{0,400}?reportAgentTargetUnresolvable\(msg, resolved\.error\);/, + 'a miss after a granted claim declines (handing the lease back) instead of ending the request for every tab', + ); // 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.