From 70611b6fa7387de5ad933047bc7ba398410011c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 10:30:22 +0000 Subject: [PATCH] Surface a sub-agent blocked on an approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approvals already routed correctly into a managed sub-agent thread and were answerable once you opened it, but nothing told you it was blocked. A managed child is deliberately absent from the sidebar, so every browser affordance keyed to a thread id quietly no-opped for it: no status symbol on any row, a notification titled with a bare id prefix, and a click that dead-ended on "Approval thread is not in the current project list." The child just looked hung. Three fixes: - Resolve thread identity from the cached per-project thread summaries (which include managed sub-agents) instead of from the rendered sidebar row. This alone restores notification naming and click-to-navigate. Lookups go through an id index, since hoisting resolves ids once per activity entry, then per ancestor hop, then again per sidebar row. - Hoist a hidden child's activity onto the nearest ancestor that does have a row, showing the most urgent state among the row and its hidden descendants (approval > error > active turn), naming the originating child in the tooltip and marking the state as hoisted. Ancestor and descendant walks are bounded so cyclic ownership terminates. - Give the header sub-agent monitor and its cards a distinct "needs approval" state: an approval also marks the turn active, so a blocked child previously rendered identically to a busy one. A card covers its whole owned subtree, since nested descendants are not listed separately, and takes its summary from the same subtree activity that sets its state. Because the server materializes a child on its own, activity can arrive for a thread the browser has never listed; it now refreshes its cached lists before naming or navigating to such a thread, so the first approval from a brand-new child is still attributable. That refresh is bounded per id — some ids never resolve, such as trailing activity from a deleted thread, and an ungated retry would re-fetch every project list for the rest of the turn. It also exposed a latent bug: rebuilding the sidebar rows discards the selection highlight with the old DOM, and every existing caller happened to follow the reload with openThread, which re-derives it. loadThreads now restores it itself. Since the refresh sits between the notification dedup check and the record, the dedup key is claimed before awaiting it, so two events describing one approval cannot both notify. Covered by a new Playwright spec driving a scripted sub-agent that blocks on an approval: the ancestor badge, the monitor state, the notification contents, click-through navigation and answering, single-notification dedup, the bounded refresh, plus direct assertions on the hoisting priority and the cycle guard. The scripted child waits before raising its approval — thread activity is broadcast live and never replayed on connect, so firing immediately raced the browser's WebSocket. The replay harness tracks pending approvals by id rather than in a single slot, so a parent and a child blocked at the same time cannot overwrite each other's confirmation target. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY --- .../src/bin/giskard-server-replay.rs | 155 +++++++-- crates/giskard-server/static/app.css | 9 + crates/giskard-server/static/app.js | 321 ++++++++++++++++-- crates/giskard-server/tests/ui.rs | 95 +++++- docs/subagents.md | 31 ++ specs/giskard-specification.md | 34 +- tests/e2e/README.md | 9 +- tests/e2e/tests/helpers.ts | 86 +++++ tests/e2e/tests/subagent-approvals.spec.ts | 308 +++++++++++++++++ 9 files changed, 979 insertions(+), 69 deletions(-) create mode 100644 tests/e2e/tests/subagent-approvals.spec.ts diff --git a/crates/giskard-server/src/bin/giskard-server-replay.rs b/crates/giskard-server/src/bin/giskard-server-replay.rs index 846c2e9..580db04 100644 --- a/crates/giskard-server/src/bin/giskard-server-replay.rs +++ b/crates/giskard-server/src/bin/giskard-server-replay.rs @@ -18,6 +18,7 @@ //! * `GISKARD_REPLAY_PASSWORD` — the app password (default `giskard`); //! * `GISKARD_REPLAY_WORKSPACE` — the demo project's workspace dir (created if missing). +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -51,23 +52,44 @@ const SCRIPTED_SUBAGENT_PROMPT: &str = "Review the linked child task."; const SCRIPTED_SUBAGENT_REPLY: &str = "Child replay output"; const SCRIPTED_SUBAGENT_PREFIX: &str = "scripted-subagent|"; const SCRIPTED_NESTED_SUBAGENT_PREFIX: &str = "scripted-nested-subagent|"; +/// Prompt that spawns a linked child which raises an approval and then holds its turn open. The +/// parent turn completes normally, so browser tests can observe a blocked sub-agent while sitting on +/// the parent thread — the case where the child has no sidebar row of its own. +const SCRIPTED_SUBAGENT_APPROVAL_TRIGGER: &str = "Spawn a sub-agent that needs approval."; +const SCRIPTED_APPROVAL_SUBAGENT_PREFIX: &str = "scripted-approval-subagent|"; +const SCRIPTED_SUBAGENT_APPROVAL_ID: &str = "scripted-subagent-approval-1"; +const SCRIPTED_SUBAGENT_APPROVAL_COMMAND: &str = "rm -rf ./child-build"; +const SCRIPTED_SUBAGENT_AGENT_NAME: &str = "Replay child"; +const SCRIPTED_APPROVAL_SUBAGENT_AGENT_NAME: &str = "Approval child"; +/// How long the approval-blocked child waits before raising its approval, so the browser's +/// WebSocket is attached and receives the live thread-activity broadcast. +const SCRIPTED_SUBAGENT_APPROVAL_DELAY: std::time::Duration = + std::time::Duration::from_millis(1500); /// Prompt that makes the harness raise an approval and then keep the turn in-flight (it never /// completes). This lets browser tests answer the approval and reload mid-turn to assert the /// answered card is not re-surfaced as actionable. The approval id is fixed so tests can target it. const SCRIPTED_APPROVAL_TRIGGER: &str = "Trigger a scripted approval request."; const SCRIPTED_APPROVAL_ID: &str = "scripted-approval-1"; +/// How long a scripted turn waits for the server's event forwarder to subscribe before giving up. +const RECEIVER_WAIT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(2); +const RECEIVER_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_millis(10); /// A harness that speaks the neutral protocol but has no backend: every turn streams the same /// canned agent message, so the browser-visible transcript is fully deterministic. struct ScriptedHarness { capabilities: HarnessCapabilities, threads: tokio::sync::Mutex)>>, - /// The thread/turn of an in-flight scripted approval, so `respond_approval` can emit a - /// confirmation item on the still-open turn (used by the reload e2e test to know the server has - /// recorded the answer before it reconnects). - active_approval: tokio::sync::Mutex>, + /// Where each in-flight scripted approval was raised, so `respond_approval` can emit its + /// confirmation item on the right still-open turn (the reload e2e test uses that ack to know the + /// server has recorded the answer before it reconnects). Keyed by approval id rather than held + /// as a single slot: a parent and a sub-agent can be blocked at the same time, and a shared slot + /// would let the later one overwrite the earlier and misattribute its ack. Shared, because a + /// sub-agent's approval is raised from the detached task that drives the child's turn. + active_approvals: ActiveApprovals, } +type ActiveApprovals = Arc>>; + impl ScriptedHarness { fn new() -> Self { Self { @@ -86,8 +108,18 @@ impl ScriptedHarness { context_compaction: false, }, threads: tokio::sync::Mutex::new(Vec::new()), - active_approval: tokio::sync::Mutex::new(None), + active_approvals: Arc::new(tokio::sync::Mutex::new(HashMap::new())), + } + } + + /// Wait for the server's event forwarder to attach before scripting a turn. A `broadcast` sender + /// drops anything sent with no receivers, so every scripted turn must gate on this. + async fn wait_for_receiver(sender: &broadcast::Sender) -> bool { + let deadline = tokio::time::Instant::now() + RECEIVER_WAIT_TIMEOUT; + while sender.receiver_count() == 0 && tokio::time::Instant::now() < deadline { + tokio::time::sleep(RECEIVER_POLL_INTERVAL).await; } + sender.receiver_count() > 0 } async fn sender_for(&self, thread: ThreadId) -> Option> { @@ -99,11 +131,62 @@ impl ScriptedHarness { } fn subagent_parent(native_thread_id: &str) -> Option { - [SCRIPTED_SUBAGENT_PREFIX, SCRIPTED_NESTED_SUBAGENT_PREFIX] - .into_iter() - .find_map(|prefix| native_thread_id.strip_prefix(prefix)) - .and_then(|value| value.rsplit_once('|')) - .map(|(parent, _)| parent.to_owned()) + [ + SCRIPTED_SUBAGENT_PREFIX, + SCRIPTED_NESTED_SUBAGENT_PREFIX, + SCRIPTED_APPROVAL_SUBAGENT_PREFIX, + ] + .into_iter() + .find_map(|prefix| native_thread_id.strip_prefix(prefix)) + .and_then(|value| value.rsplit_once('|')) + .map(|(parent, _)| parent.to_owned()) + } + + /// Drive a child turn that blocks on an approval and never completes. The parent's own turn has + /// already finished by the time this runs, so the browser is left with a blocked thread that has + /// no sidebar row — the exact state the ancestor badge, the sub-agents button, and the approval + /// notification have to surface. + fn spawn_approval_subagent_turn( + sender: broadcast::Sender, + thread_id: ThreadId, + active_approvals: ActiveApprovals, + ) { + tokio::spawn(async move { + if !Self::wait_for_receiver(&sender).await { + return; + } + + // The forwarder is listening, but the browser opens its WebSocket a few milliseconds + // after the HTTP call that started the parent turn. Thread activity is broadcast live + // and never replayed on connect, so firing immediately would race the client and the + // approval would be broadcast to nobody. Give the browser time to attach. + tokio::time::sleep(SCRIPTED_SUBAGENT_APPROVAL_DELAY).await; + + let turn = TurnId::new(); + active_approvals.lock().await.insert( + ApprovalId(SCRIPTED_SUBAGENT_APPROVAL_ID.into()), + (thread_id, turn), + ); + let _ = sender.send(AgentEvent::TurnStarted { + thread: thread_id, + turn, + }); + tokio::task::yield_now().await; + let _ = sender.send(AgentEvent::ApprovalRequested { + thread: thread_id, + turn, + request: ApprovalRequest { + id: ApprovalId(SCRIPTED_SUBAGENT_APPROVAL_ID.into()), + kind: ApprovalKind::CommandExecution { + command: SCRIPTED_SUBAGENT_APPROVAL_COMMAND.into(), + cwd: "/tmp/demo".into(), + }, + reason: Some("The sub-agent wants to remove its build directory.".into()), + metadata: vec![], + available: vec![ApprovalDecision::Accept, ApprovalDecision::Decline], + }, + }); + }); } fn spawn_nested_subagent_turn( @@ -112,11 +195,7 @@ impl ScriptedHarness { parent_harness_thread_id: String, ) { tokio::spawn(async move { - let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_secs(2); - while sender.receiver_count() == 0 && tokio::time::Instant::now() < deadline { - tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; - } - if sender.receiver_count() == 0 { + if !Self::wait_for_receiver(&sender).await { return; } @@ -193,11 +272,7 @@ impl ScriptedHarness { parent_harness_thread_id: String, ) { tokio::spawn(async move { - let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_secs(2); - while sender.receiver_count() == 0 && tokio::time::Instant::now() < deadline { - tokio::time::sleep(tokio::time::Duration::from_millis(10)).await; - } - if sender.receiver_count() == 0 { + if !Self::wait_for_receiver(&sender).await { return; } @@ -285,9 +360,12 @@ impl AgentHarness for ScriptedHarness { drop(threads); let parent_harness_thread_id = Self::subagent_parent(&harness_thread_id); + let blocks_on_approval = harness_thread_id.starts_with(SCRIPTED_APPROVAL_SUBAGENT_PREFIX); if is_new && let Some(parent) = parent_harness_thread_id.clone() { if harness_thread_id.starts_with(SCRIPTED_NESTED_SUBAGENT_PREFIX) { Self::spawn_nested_subagent_turn(sender, thread, harness_thread_id.clone()); + } else if blocks_on_approval { + Self::spawn_approval_subagent_turn(sender, thread, self.active_approvals.clone()); } else { Self::spawn_subagent_turn(sender, thread, parent); } @@ -298,9 +376,13 @@ impl AgentHarness for ScriptedHarness { harness_thread_id, warning: None, resumed_model: Some(opts.initial_model), - agent_name: parent_harness_thread_id - .as_ref() - .map(|_| "Replay child".to_string()), + agent_name: parent_harness_thread_id.as_ref().map(|_| { + if blocks_on_approval { + SCRIPTED_APPROVAL_SUBAGENT_AGENT_NAME.to_string() + } else { + SCRIPTED_SUBAGENT_AGENT_NAME.to_string() + } + }), parent_harness_thread_id, }) } @@ -327,12 +409,19 @@ impl AgentHarness for ScriptedHarness { "{SCRIPTED_NESTED_SUBAGENT_PREFIX}{}|{turn}", thread.harness_thread_id )), + Some(SCRIPTED_SUBAGENT_APPROVAL_TRIGGER) => Some(format!( + "{SCRIPTED_APPROVAL_SUBAGENT_PREFIX}{}|{turn}", + thread.harness_thread_id + )), _ => None, }; let raise_approval = input_text == Some(SCRIPTED_APPROVAL_TRIGGER); if raise_approval { - *self.active_approval.lock().await = Some((thread_id, turn)); + self.active_approvals + .lock() + .await + .insert(ApprovalId(SCRIPTED_APPROVAL_ID.into()), (thread_id, turn)); } // Stream the canned reply the way a real harness would: start, incremental deltas, then a @@ -365,6 +454,12 @@ impl AgentHarness for ScriptedHarness { } if let Some(native_thread_id) = subagent_native_thread_id { + let child_name = if native_thread_id.starts_with(SCRIPTED_APPROVAL_SUBAGENT_PREFIX) + { + SCRIPTED_APPROVAL_SUBAGENT_AGENT_NAME + } else { + SCRIPTED_SUBAGENT_AGENT_NAME + }; let _ = sender.send(AgentEvent::TurnStarted { thread: thread_id, turn, @@ -378,11 +473,11 @@ impl AgentHarness for ScriptedHarness { harness_item_id: format!("scripted_subagent_link_{turn}"), payload: ItemPayload::Activity { title: "Sub-agent running".into(), - detail: Some("Replay child".into()), + detail: Some(child_name.into()), metadata: None, subagent: Some(SubagentLink { harness_thread_id: native_thread_id, - path: Some("Replay child".into()), + path: Some(child_name.into()), initial_prompt: Some(SCRIPTED_SUBAGENT_PROMPT.into()), action: SubagentAction::Started, status: None, @@ -471,13 +566,15 @@ impl AgentHarness for ScriptedHarness { async fn respond_approval( &self, - _req: giskard_core::ids::ApprovalId, + req: giskard_core::ids::ApprovalId, decision: giskard_core::approval::ApprovalDecision, ) -> Result<(), HarnessError> { // Emit a confirmation item on the still-open turn so tests have a deterministic signal that // the answer was routed. The turn stays in-flight (no TurnCompleted) so a reconnect still - // replays the answered approval from the live buffer. - if let Some((thread_id, turn)) = *self.active_approval.lock().await + // replays the answered approval from the live buffer. Look the location up by the answered + // id: several approvals can be pending at once, on different threads. + let raised_at = self.active_approvals.lock().await.remove(&req); + if let Some((thread_id, turn)) = raised_at && let Some(sender) = self.sender_for(thread_id).await { let label = match decision { diff --git a/crates/giskard-server/static/app.css b/crates/giskard-server/static/app.css index c1c93ea..48c317b 100644 --- a/crates/giskard-server/static/app.css +++ b/crates/giskard-server/static/app.css @@ -109,6 +109,9 @@ .thread.activity-running .thread-status { color:#d29922; } .thread.activity-approval .thread-status { color:#f0d98c; } .thread.activity-error .thread-status { color:#f85149; } + /* Hoisted from a hidden sub-agent: same urgency colour, outlined so the row still reads as + "something under here" rather than "this thread". */ + .thread.activity-subagent .thread-status { text-shadow:0 0 6px currentcolor; opacity:.9; } .thread-title-input { flex:1; min-width:0; height:28px; padding:4px 7px; font-size:13px; } .thread-menu-btn { width:28px; height:28px; padding:0; border-color:transparent; background:transparent; color:var(--muted); } /* Hover affordances only where a real hovering pointer exists. On touch, :hover "sticks" to the @@ -201,6 +204,9 @@ .subagents-btn { display:flex; align-items:center; gap:6px; } .subagents-btn.state-idle { color:var(--muted); } .subagents-btn.state-running { color:#f0d98c; border-color:#d29922; background:rgba(210,153,34,.08); } + /* A sub-agent blocked on an approval is waiting on the user, not on the model — it gets the + accent treatment so it separates from the merely-running state at a glance. */ + .subagents-btn.state-approval { color:#ffd97d; border-color:#e3b341; background:rgba(227,179,65,.18); } .subagents-popover { position:absolute; left:0; top:34px; z-index:35; width:min(460px, calc(100vw - 24px)); max-height:70vh; overflow:auto; padding:10px; border:1px solid var(--border); @@ -219,6 +225,9 @@ .subagent-card-name { flex:1; min-width:0; overflow:hidden; text-overflow:ellipsis; white-space:nowrap; } .subagent-card-state { font-size:11px; color:var(--muted); } .subagent-card-state.running { color:#f0d98c; } + .subagent-card-state.approval { color:#ffd97d; font-weight:600; } + .subagent-card.needs-approval { border-color:#e3b341; background:rgba(227,179,65,.12); } + .subagents-summary.needs-approval { color:#ffd97d; } .subagent-card-meta { color:var(--muted); font-size:12px; overflow:hidden; text-overflow:ellipsis; white-space:nowrap; } .usage-wrap { position:relative; margin-left:auto; } .usage-btn { display:flex; align-items:center; gap:6px; } diff --git a/crates/giskard-server/static/app.js b/crates/giskard-server/static/app.js index 0d4ed00..6f006de 100644 --- a/crates/giskard-server/static/app.js +++ b/crates/giskard-server/static/app.js @@ -19,6 +19,12 @@ const NOTIFICATION_PROMPT_NOTICE_INTERVAL_MS = 30000; const BROWSER_DIAGNOSTIC_LIMIT = 120; const NOTIFICATION_DEDUP_MS = 15000; const ACTIVE_THREAD_COMPLETED_MARK_MS = 2500; +// Debounce for re-fetching thread lists after activity arrives for a thread the browser has never +// seen (a sub-agent the server just materialized). Short enough that the sidebar catches up while +// the child is still blocked, long enough that a burst of child events costs one refresh. +const STALE_THREAD_LIST_REFRESH_MS = 400; +// Refresh attempts spent on any one unresolved thread id before giving up on it. +const STALE_THREAD_LIST_REFRESH_MAX_ATTEMPTS = 3; const BROWSER_DIAGNOSTIC_VERSION = "browser-diagnostics-v1"; const MAX_ATTACHMENTS_PER_MESSAGE = 8; const MAX_ATTACHMENT_BYTES = 25 * 1024 * 1024; @@ -50,7 +56,7 @@ let state = { pickerTypeahead:"", pickerTypeaheadTimer:null, pickerSelectedRow:null, currentPlan:null, planExpanded:localStorage.getItem("giskard.planExpanded")==="1", threadActivity:new Map(), pendingApprovalFocus:null, notifiedApprovals:new Map(), approvalNotifications:new Map(), browserDiagnostics:[], - subagentImports:new Map(), projectThreads:new Map(), + subagentImports:new Map(), projectThreads:new Map(), threadIndex:new Map(), lastNotificationPromptNoticeAt:0, swRegistration:null, pendingAttachments:[], attachmentGeneration:0, pendingAttachmentOperations:new Map(), collapsedProjects:new Set(loadCollapsedProjects()), pendingRemoveProject:null, projectDirs:{} @@ -641,6 +647,11 @@ async function loadThreads(pid) { box.append(label); appendThreadRows(box, pid, archived); } + // Rebuilding the rows discards the selection highlight with the old DOM. Callers that reload as + // part of opening a thread re-derive it themselves, but a reload triggered by anything else + // (say, catching up on a sub-agent the server just materialized) would otherwise leave the list + // with no visibly selected thread. + syncActiveThreadHighlight(); } catch {} } @@ -653,6 +664,7 @@ function rememberProjectThreads(pid, threads) { spawned_by_turn_id:t.spawned_by_turn_id ? String(t.spawned_by_turn_id) : null })); state.projectThreads.set(projectId, normalized); + reindexProjectThreads(projectId, normalized); // Link results are browser-local accelerators only. Discard them when the authoritative thread // list reloads; a later click resolves the trusted item coordinates idempotently on the server. @@ -668,6 +680,19 @@ function knownProjectThreads(pid) { return state.projectThreads.get(String(pid || state.projectId)) || []; } +// Thread id → { pid, thread }, rebuilt whenever a project's list is remembered. Activity hoisting +// resolves ids constantly — once per activity entry, then once per ancestor hop, then again for +// every sidebar row — so scanning each project's array per lookup is quadratic in thread count for +// a single repaint. The entries hold the same objects as `projectThreads`, so in-place edits (a +// renamed thread) stay visible through both. +function reindexProjectThreads(pid, threads) { + const projectId = String(pid); + for (const [tid, entry] of state.threadIndex) { + if (entry.pid === projectId) state.threadIndex.delete(tid); + } + for (const thread of threads) state.threadIndex.set(String(thread.id), { pid:projectId, thread }); +} + function appendThreadRows(box, pid, threads) { const byParent = new Map(); const ids = new Set(threads.map(t => String(t.id))); @@ -844,16 +869,125 @@ function threadRowForId(tid) { return document.querySelector(`.thread[data-tid="${tid}"]`); } +// Resolve a thread's project, display title, and ownership. The sidebar row is the fast path, but +// managed sub-agent threads are deliberately never rendered there, so fall back to the authoritative +// per-project thread lists. Without that fallback anything keyed off a thread id — approval +// notification naming, click-to-focus, the ancestor activity badge — silently no-ops for a +// sub-agent, which is exactly the thread the user most needs to be told about. function threadMetaForId(tid) { + if (!tid) return null; + const known = knownThreadMeta(tid); const el = threadRowForId(tid); - if (!el) return null; + if (!el) return known; + return { + pid: el.dataset.pid || (known && known.pid) || "", + tid: el.dataset.tid || String(tid), + title: currentThreadTitle(el), + kind: (known && known.kind) || "primary", + parent_thread_id: (known && known.parent_thread_id) || null + }; +} + +// Look a thread up across every cached project list. Managed sub-agents are present here even though +// `loadThreads` filters them out of the sidebar. +function knownThreadMeta(tid) { + const key = String(tid || ""); + if (!key) return null; + const entry = state.threadIndex.get(key); + if (!entry) return null; + const thread = entry.thread; return { - pid: el.dataset.pid || "", - tid: el.dataset.tid || tid, - title: currentThreadTitle(el) + pid: entry.pid, + tid: key, + title: threadDisplayTitle(thread), + kind: thread.kind || "primary", + parent_thread_id: thread.parent_thread_id ? String(thread.parent_thread_id) : null }; } +function threadDisplayTitle(thread) { + if (!thread) return ""; + if (thread.kind === "subagent") return subagentDisplayName(thread); + const title = String(thread.title || "").trim(); + return title || String(thread.id || "").slice(0,8); +} + +// True when `tid` sits anywhere under `ancestorId` in the ownership tree. Bounded by a seen-set so +// corrupted parent metadata (a cycle) cannot spin. +function threadDescendsFrom(tid, ancestorId) { + const target = String(ancestorId || ""); + if (!target) return false; + const seen = new Set([String(tid || "")]); + let meta = knownThreadMeta(tid); + while (meta && meta.parent_thread_id) { + const parentId = meta.parent_thread_id; + if (parentId === target) return true; + if (seen.has(parentId)) return false; + seen.add(parentId); + meta = knownThreadMeta(parentId); + } + return false; +} + +// The sidebar row that must display `tid`'s activity. A managed sub-agent has no row of its own, so +// its activity is hoisted to the nearest ancestor that does — otherwise a child blocked on an +// approval produces no visible signal anywhere in the sidebar. +function activityHostThreadId(tid) { + const key = String(tid || ""); + if (!key) return null; + if (threadRowForId(key)) return key; + const seen = new Set([key]); + let meta = knownThreadMeta(key); + while (meta && meta.parent_thread_id) { + const parentId = meta.parent_thread_id; + if (seen.has(parentId)) return null; + seen.add(parentId); + if (threadRowForId(parentId)) return parentId; + meta = knownThreadMeta(parentId); + } + return null; +} + +// The server materializes a sub-agent thread on its own, so activity can arrive for a thread the +// browser has never listed. Refresh the cached lists once per burst so naming, ancestor badges, and +// the sub-agents menu can resolve it; without this the first approval from a brand-new child is +// still anonymous. +let staleThreadListRefreshTimer = null; +// Attempts already spent per unresolved thread id. Some ids never resolve — trailing activity from +// a deleted thread, or one the server does not list — and an ungated retry would re-fetch every +// project list on every event for the rest of that turn. A few attempts still cover the case this +// exists for: a child whose activity beats its own persistence by a moment. +const staleThreadRefreshAttempts = new Map(); + +function noteUnresolvedThread(tid) { + const key = String(tid || ""); + if (!key) return; + const attempts = staleThreadRefreshAttempts.get(key) || 0; + if (attempts >= STALE_THREAD_LIST_REFRESH_MAX_ATTEMPTS) return; + staleThreadRefreshAttempts.set(key, attempts + 1); + scheduleStaleThreadListRefresh(); +} + +function scheduleStaleThreadListRefresh() { + if (staleThreadListRefreshTimer) return; + staleThreadListRefreshTimer = setTimeout(() => { + staleThreadListRefreshTimer = null; + refreshKnownThreadLists(); + }, STALE_THREAD_LIST_REFRESH_MS); +} + +async function refreshKnownThreadLists() { + const ids = Array.from(state.projectThreads.keys()); + await Promise.all(ids.map(pid => loadThreads(pid))); + // Ids this refresh resolved get their budget back, so a thread that later disappears and returns + // is not permanently barred from triggering one. + for (const tid of Array.from(staleThreadRefreshAttempts.keys())) { + if (knownThreadMeta(tid)) staleThreadRefreshAttempts.delete(tid); + } + renderAllThreadActivityIndicators(); + renderSubagentsButton(); +} + function knownThreadForId(pid, tid) { const key = String(tid || ""); if (!key) return null; @@ -913,17 +1047,69 @@ function clearThreadActivity(tid) { renderSubagentsButton(); } -function renderThreadActivityIndicator(tid) { +// Urgency order used when one row has to represent both its own state and its hidden sub-agents'. +// A blocked child must never be masked by a merely-running parent, so approval outranks error +// outranks running. +function threadActivityRank(activity) { + if (!activity) return -1; + if (activity.kind === "approval_requested") return 3; + if (activity.kind === "error") return 2; + return activity.active_turn ? 1 : 0; +} + +// The activity a sidebar row must display: its own, or the most urgent one belonging to a hidden +// managed sub-agent that resolves to this row. `origin` names the descendant when the winning state +// came from one, so the row can say which sub-agent it is reporting. +// Resolve every activity entry's host row once. Each resolution walks an ownership chain, so a full +// repaint that re-resolves them per row costs rows × activities × depth; sharing this makes the +// chain walking once-per-activity for the whole repaint. +function activityHostIndex() { + const hosts = new Map(); + for (const tid of state.threadActivity.keys()) hosts.set(tid, activityHostThreadId(tid)); + return hosts; +} + +function effectiveThreadActivity(tid, hosts) { + const key = String(tid); + let activity = state.threadActivity.get(key) || null; + let origin = null; + for (const [otherId, other] of state.threadActivity) { + if (otherId === key) continue; + if (threadActivityRank(other) <= threadActivityRank(activity)) continue; + const host = hosts ? hosts.get(otherId) : activityHostThreadId(otherId); + if (host !== key) continue; + activity = other; + origin = otherId; + } + return { activity, origin }; +} + +function threadActivityTooltip(activity, origin) { + const summary = (activity && activity.summary) || "Thread activity"; + if (!origin) return summary; + const meta = threadMetaForId(origin); + const name = (meta && meta.title) || String(origin).slice(0,8); + return `${name}: ${summary}`; +} + +function renderThreadActivityIndicator(tid, hosts) { const el = threadRowForId(tid); - if (!el) return; + if (!el) { + // A managed sub-agent has no row. Repaint the ancestor that stands in for it instead; that call + // takes the branch below, so this cannot recurse further. + const host = (hosts && hosts.get(String(tid))) || activityHostThreadId(tid); + if (host && host !== String(tid)) renderThreadActivityIndicator(host, hosts); + return; + } const status = el.querySelector(".thread-status"); if (!status) return; - const activity = state.threadActivity.get(String(tid)); + const { activity, origin } = effectiveThreadActivity(tid, hosts); const visible = !!activity && (activity.unread || activity.active_turn || activity.approval_id || activity.kind === "turn_completed" || activity.kind === "error"); el.classList.toggle("has-activity", visible); el.classList.toggle("activity-approval", visible && activity && activity.kind === "approval_requested"); el.classList.toggle("activity-error", visible && activity && activity.kind === "error"); el.classList.toggle("activity-running", visible && activity && activity.active_turn && activity.kind !== "approval_requested"); + el.classList.toggle("activity-subagent", visible && !!origin); if (!visible) { status.textContent = ""; status.title = ""; @@ -933,11 +1119,12 @@ function renderThreadActivityIndicator(tid) { else if (activity.kind === "error") status.textContent = "x"; else if (activity.active_turn) status.textContent = "o"; else status.textContent = "*"; - status.title = activity.summary || "Thread activity"; + status.title = threadActivityTooltip(activity, origin); } function renderAllThreadActivityIndicators() { - document.querySelectorAll(".thread").forEach(el => renderThreadActivityIndicator(el.dataset.tid)); + const hosts = activityHostIndex(); + document.querySelectorAll(".thread").forEach(el => renderThreadActivityIndicator(el.dataset.tid, hosts)); } // Mark a single thread row as the selected one (or not). `aria-current` mirrors the visual state for @@ -2008,6 +2195,10 @@ function handleThreadActivity(msg) { activity.unread = true; } state.threadActivity.set(tid, activity); + // Activity for a thread we have never listed means the server materialized it after our last + // load — almost always a sub-agent. Catch the list up so this activity can be attributed and + // hoisted onto a visible ancestor. + if (!knownThreadMeta(tid)) noteUnresolvedThread(tid); renderThreadActivityIndicator(tid); renderSubagentsButton(); if (activity.kind === "approval_requested") maybeNotifyApproval(tid, activity); @@ -2063,14 +2254,24 @@ async function maybeNotifyApproval(tid, activity) { }); return; } + // Claim the dedup key before any await. Two events can describe the same approval (the live + // activity broadcast and the live-turn snapshot path), and with the refresh below between the + // check above and the record after the notification, both would clear the gate and notify. + // Every path that returns without notifying releases it again. + state.notifiedApprovals.set(notificationKey, now); + // A sub-agent's very first approval routinely arrives before the browser has listed the thread + // the server just materialized. Resolve it now rather than shipping an unattributable id prefix — + // this notification is the only signal the user gets for a thread with no sidebar row. + if (!threadMetaForId(tid)) await refreshKnownThreadLists(); const meta = threadMetaForId(tid); - const title = meta && meta.title ? meta.title : tid.slice(0,8); + const title = approvalNotificationLabel(meta, tid); + const isSubagent = !!meta && meta.kind === "subagent"; // Stable per-approval tag: it dedups at the OS level and lets us close the notification by tag on // the service-worker path (where we never hold a Notification object) when the approval resolves. const notificationTag = approvalNotificationTag(tid, activity.approval_id); let result; try { - result = await showAppNotification("Giskard: approval needed", { + result = await showAppNotification(isSubagent ? "Giskard: sub-agent approval needed" : "Giskard: approval needed", { body: activity.summary ? `${title}: ${activity.summary}` : title, tag: notificationTag, renotify: true, @@ -2084,6 +2285,7 @@ async function maybeNotifyApproval(tid, activity) { tag: notificationTag }); } catch (e) { + state.notifiedApprovals.delete(notificationKey); recordNotificationDiagnostic("approval_notify_constructor_failed", { tid, approval_id: activity.approval_id, @@ -2093,8 +2295,10 @@ async function maybeNotifyApproval(tid, activity) { console.warn("Giskard notification failed", e); return; } - if (!result) return; - state.notifiedApprovals.set(notificationKey, now); + if (!result) { + state.notifiedApprovals.delete(notificationKey); + return; + } // Desktop (constructor) notifications are tracked so we can close them on resolution and dispatch // their click; service-worker notifications are closed by tag and click via the worker postMessage. if (result.via === "constructor" && result.notification) { @@ -2217,9 +2421,27 @@ function maybeNoticeNotificationPermission() { notice("Enable approval notifications from the sidebar alert button.", "warning"); } +// Name an approval's thread for a notification. A sub-agent's own title is often generic ("Sub-agent +// #2"), and it has no sidebar row to fall back on, so qualify it with the owning thread — an +// unattributable "3f2a91bc: Approval requested" tells the user nothing about what is blocked. +function approvalNotificationLabel(meta, tid) { + const fallback = String(tid).slice(0,8); + if (!meta) return fallback; + const title = meta.title || fallback; + if (meta.kind !== "subagent" || !meta.parent_thread_id) return title; + const parent = threadMetaForId(meta.parent_thread_id); + return parent && parent.title ? `${title} (in ${parent.title})` : title; +} + async function focusApprovalTarget(tid, approvalId) { window.focus(); - const meta = threadMetaForId(tid); + let meta = threadMetaForId(tid); + if (!meta || !meta.pid) { + // The thread may have been materialized after our last list load; one refresh is cheap and is + // the difference between navigating to the blocked sub-agent and a dead-end notification. + await refreshKnownThreadLists(); + meta = threadMetaForId(tid); + } if (!meta || !meta.pid) { notice("Approval thread is not in the current project list.", "warning"); return; @@ -4272,32 +4494,58 @@ function subagentThreadsForActiveProject() { ); } -function subagentActivityForThread(threadId) { - return state.threadActivity.get(String(threadId || "")) || null; -} - -function subagentIsRunning(thread) { - const activity = subagentActivityForThread(thread.id); - return !!(activity && activity.active_turn); +// Everything a card reports about the subtree it owns, from one walk of the activity map. A card +// stands for its whole subtree — a nested grandchild is not listed separately, since the menu lists +// direct children of the open thread — so an unreported descendant is reported nowhere. +// +// `running` and `needsApproval` are separate flags rather than reads of the winning activity: an +// approval also sets `active_turn`, so a blocked child would otherwise be indistinguishable from a +// busy one, and an erroring descendant outranks a running sibling without cancelling it. `activity` +// is the ranked winner that names the card's state, with `origin` set when it belongs to a +// descendant, so the summary can describe that descendant rather than this thread. +function subagentSubtreeState(threadId) { + const key = String(threadId); + let running = false; + let needsApproval = false; + let activity = null; + let origin = null; + for (const [otherId, other] of state.threadActivity) { + if (otherId !== key && !threadDescendsFrom(otherId, key)) continue; + if (other.active_turn) running = true; + if (other.kind === "approval_requested") needsApproval = true; + if (threadActivityRank(other) <= threadActivityRank(activity)) continue; + activity = other; + origin = otherId === key ? null : otherId; + } + return { running, needsApproval, activity, origin }; } function subagentCounts() { const agents = subagentThreadsForActiveProject(); - const running = agents.filter(subagentIsRunning).length; - return { total:agents.length, running }; + let running = 0; + let awaitingApproval = 0; + for (const thread of agents) { + const subtree = subagentSubtreeState(thread.id); + if (subtree.running) running += 1; + if (subtree.needsApproval) awaitingApproval += 1; + } + return { total:agents.length, running, awaitingApproval }; } function renderSubagentsButton() { const btn = $("subagentsBtn"); if (!btn) return; const counts = subagentCounts(); - const stateName = counts.running ? "running" : "idle"; + const stateName = counts.awaitingApproval ? "approval" : (counts.running ? "running" : "idle"); btn.className = `badge subagents-btn state-${stateName}`; btn.disabled = !state.projectId || (!counts.total && !counts.running); + const runningLabel = `${counts.running} running sub-agent${counts.running === 1 ? "" : "s"} · ${counts.total} total`; btn.title = counts.total - ? `${counts.running} running sub-agent${counts.running === 1 ? "" : "s"} · ${counts.total} total` + ? (counts.awaitingApproval + ? `${counts.awaitingApproval} sub-agent${counts.awaitingApproval === 1 ? "" : "s"} waiting for approval · ${runningLabel}` + : runningLabel) : "No sub-agents"; - $("subagentsCount").textContent = String(counts.running || counts.total); + $("subagentsCount").textContent = String(counts.awaitingApproval || counts.running || counts.total); if (!$("subagentsMenu").hidden) renderSubagentsMenu(); } @@ -4306,8 +4554,9 @@ function renderSubagentsMenu() { const agents = subagentThreadsForActiveProject(); const counts = subagentCounts(); const rows = agents.map(renderSubagentCard).join(""); + const approvalSummary = counts.awaitingApproval ? `${counts.awaitingApproval} waiting for approval · ` : ""; const summary = agents.length - ? `
${counts.running} running · ${counts.total} total
` + ? `
${approvalSummary}${counts.running} running · ${counts.total} total
` : ""; menu.innerHTML = `
@@ -4327,18 +4576,22 @@ function renderSubagentsMenu() { } function renderSubagentCard(thread) { - const activity = subagentActivityForThread(thread.id); - const running = !!(activity && activity.active_turn); - const stateLabel = running ? "Running" : (activity && activity.kind === "error" ? "Error" : "Idle"); + const { running, needsApproval, activity, origin } = subagentSubtreeState(thread.id); + const stateLabel = needsApproval + ? "Needs approval" + : (running ? "Running" : (activity && activity.kind === "error" ? "Error" : "Idle")); const parent = thread.parent_thread_id ? knownProjectThreads(state.projectId).find(t => String(t.id) === String(thread.parent_thread_id)) : null; const parentLabel = parent && parent.title ? `Parent: ${parent.title}` : "Parent thread"; - const summary = activity && activity.summary ? activity.summary : parentLabel; + const summary = activity && activity.summary + ? threadActivityTooltip(activity, origin) + : parentLabel; const active = state.threadId && String(state.threadId) === String(thread.id); const name = subagentDisplayName(thread); - return ``; diff --git a/crates/giskard-server/tests/ui.rs b/crates/giskard-server/tests/ui.rs index 6cb6981..c1a070d 100644 --- a/crates/giskard-server/tests/ui.rs +++ b/crates/giskard-server/tests/ui.rs @@ -634,6 +634,81 @@ async fn index_page_is_served_and_public() { && body.contains(".subagent-card-state.running"), "sub-agent monitor has button and card styling for idle/running states" ); + assert!( + body.contains("function knownThreadMeta(tid)") + && body.contains("const known = knownThreadMeta(tid);") + && body.contains("if (!el) return known;"), + "thread metadata falls back to the cached project thread lists, so managed sub-agents \ + (never rendered in the sidebar) can still be named and navigated to" + ); + assert!( + body.contains("function activityHostThreadId(tid)") + && body.contains("function effectiveThreadActivity(tid, hosts)") + && body.contains("function threadActivityRank(activity)") + && body.contains("el.classList.toggle(\"activity-subagent\", visible && !!origin);"), + "a hidden sub-agent's activity is hoisted onto the nearest visible ancestor row, ranked \ + so an approval is never masked by a merely-running thread" + ); + assert!( + body.contains("function approvalNotificationLabel(meta, tid)") + && body.contains("\"Giskard: sub-agent approval needed\"") + && body.contains("await refreshKnownThreadLists();"), + "an approval notification names the sub-agent and its parent, and clicking it refreshes \ + a stale thread list rather than dead-ending" + ); + assert!( + body.contains("function subagentSubtreeState(threadId)") + && body.contains("if (other.kind === \"approval_requested\") needsApproval = true;") + && body.contains("\"Needs approval\"") + && body.contains(".subagent-card.needs-approval") + && body.contains(".subagents-btn.state-approval") + && body.contains(".subagent-card-state.approval"), + "a sub-agent waiting on an approval is visually distinct from one that is merely running, \ + tracked as its own flag because an approval also marks the turn active" + ); + assert!( + body.contains("if (!knownThreadMeta(tid)) noteUnresolvedThread(tid);") + && body.contains("function noteUnresolvedThread(tid)") + && body.contains("if (attempts >= STALE_THREAD_LIST_REFRESH_MAX_ATTEMPTS) return;") + && body.contains("if (knownThreadMeta(tid)) staleThreadRefreshAttempts.delete(tid);"), + "activity for a thread the browser has never listed (a just-materialized sub-agent) \ + refreshes the cached lists so it can be attributed, but a permanently unresolvable id \ + (say, trailing activity from a deleted thread) spends a bounded number of attempts \ + instead of re-fetching every project list for the rest of the turn" + ); + assert!( + body.contains("threadIndex:new Map()") + && body.contains("function reindexProjectThreads(pid, threads)") + && body.contains("const entry = state.threadIndex.get(key);"), + "thread ids resolve through an index rather than scanning every project's list, because \ + hoisting resolves ids once per activity, per ancestor hop, and per sidebar row" + ); + assert!( + body.contains( + "const { running, needsApproval, activity, origin } = subagentSubtreeState(thread.id);" + ) && body.contains("? threadActivityTooltip(activity, origin)"), + "a sub-agent card's summary comes from the same subtree activity that sets its state, so \ + a card reading \"Needs approval\" because a descendant is blocked describes that \ + descendant instead of its own unrelated activity — and both come from one walk of the \ + activity map rather than one per question asked" + ); + assert!( + body.contains("function activityHostIndex()") + && body.contains("const hosts = activityHostIndex();") + && body.contains( + "const host = hosts ? hosts.get(otherId) : activityHostThreadId(otherId);" + ), + "a full sidebar repaint resolves each activity's host row once instead of re-walking the \ + ownership chain for every row" + ); + assert!( + body.contains( + " appendThreadRows(box, pid, archived);\n }\n // Rebuilding the rows discards \ + the selection highlight" + ) && body.contains(" syncActiveThreadHighlight();\n } catch {}"), + "reloading a project's threads re-derives the selection highlight, so a reload not driven \ + by opening a thread cannot leave the sidebar with nothing selected" + ); assert!( body.contains("function renderTaskCards") && body.contains("renderTaskCards($(\"tasksCommandList\"), commandTasks") @@ -2424,7 +2499,7 @@ fn sidebar_activity_notifications_target_approval_rows() { assert!(body.contains("NOTIFICATION_DEDUP_MS")); assert!(body.contains("function handleThreadActivity(msg)")); assert!(body.contains("if (msg && msg.type === \"thread_activity\")")); - assert!(body.contains("function renderThreadActivityIndicator(tid)")); + assert!(body.contains("function renderThreadActivityIndicator(tid, hosts)")); assert!(body.contains("activity.kind === \"approval_requested\"")); assert!(body.contains( "if (activity.kind === \"approval_requested\") maybeNotifyApproval(tid, activity);" @@ -2464,6 +2539,18 @@ fn sidebar_activity_notifications_target_approval_rows() { assert!(body.contains("const notifiedAt = state.notifiedApprovals.get(notificationKey);")); assert!(body.contains("now - notifiedAt < NOTIFICATION_DEDUP_MS")); assert!(body.contains("state.notifiedApprovals.set(notificationKey, now);")); + // The dedup key is claimed before the thread-list refresh await, not after the notification is + // shown: otherwise the live activity broadcast and the live-turn snapshot path can both clear + // the gate while one is awaiting and notify twice for the same approval. Paths that return + // without notifying release it again. + assert!(body.contains( + " state.notifiedApprovals.set(notificationKey, now);\n // A sub-agent's very first approval" + )); + assert!( + body.matches("state.notifiedApprovals.delete(notificationKey);") + .count() + >= 2 + ); assert!(body.contains("trackApprovalNotification(notificationKey, result.notification);")); assert!(body.contains("function pruneNotificationDedup(now)")); assert!(body.contains("function approvalNotificationKey(tid, approvalId)")); @@ -2543,7 +2630,11 @@ fn sidebar_activity_notifications_target_approval_rows() { assert!(!body.contains("notifyApprovalRequest(ev.request")); assert!(!body.contains("notifyApprovalRequest(msg.request")); assert!(!body.contains("notifyApprovalRequest(snap.pending_approval")); - assert!(body.contains("await showAppNotification(\"Giskard: approval needed\"")); + // A sub-agent's approval gets its own headline: it has no sidebar row, so the notification is + // the only place the user learns a delegated thread — not this one — is blocked. + assert!(body.contains( + "await showAppNotification(isSubagent ? \"Giskard: sub-agent approval needed\" : \"Giskard: approval needed\"" + )); assert!(body.contains("const focused = document.hasFocus ? document.hasFocus() : true;")); assert!(body.contains( "if (document.visibilityState === \"visible\" && focused && String(tid) === String(state.threadId))" diff --git a/docs/subagents.md b/docs/subagents.md index 5bfc78b..493e1ba 100644 --- a/docs/subagents.md +++ b/docs/subagents.md @@ -80,6 +80,37 @@ an absent or exited task. Linked evidence is processed in parent-event order, so observation cannot overtake an earlier active observation and leave a new idle monitor behind. Queued native child events take priority over terminal fallback output. +## Approvals raised inside a child + +A child's approval routes like any other: the harness maps it to the child's Giskard thread, the +passive monitor registers it, and answering it from the child transcript reaches the right harness. +The child is resumable, so its pending approval also survives a browser reload through the live-turn +snapshot. + +What needs care is telling the user, because a managed child has no sidebar row. Three surfaces +report it: + +- **The nearest visible ancestor row.** A hidden child's activity is hoisted to the closest ancestor + that is actually rendered. That row shows the most urgent state among itself and its hidden + descendants — an approval outranks an error, which outranks an active turn — so a blocked child is + never masked by a busy parent. The row's tooltip names the child, and a marker distinguishes a + hoisted state from the row's own. Walks up and down the ownership chain are bounded, so corrupted + or cyclic metadata terminates rather than spinning. +- **The header Sub-agents monitor.** An approval also marks the turn active, so "waiting for the + user" is tracked separately from "running": the button takes a distinct state and the child's card + reads `Needs approval`. A card covers its whole owned subtree, because nested grandchildren are + not listed separately. +- **A browser notification**, naming the child and its owning thread rather than an id prefix, and + saying a sub-agent is blocked. Clicking it opens the child with the approval focused. + +Because the server can materialize a child on its own, the browser may see activity for a thread it +has never listed. It refreshes its cached thread lists before naming or navigating to such a thread, +so the first approval from a brand-new child is still attributable. + +Thread activity is a live signal and is not replayed on connect. A browser that is not running when +a child raises an approval finds it in the thread's live-turn snapshot the next time it opens that +thread. + ## Prompts and transcript persistence When the delegated prompt is available, Giskard persists it as `Turn.user_input` and shows one diff --git a/specs/giskard-specification.md b/specs/giskard-specification.md index d346250..9af6805 100644 --- a/specs/giskard-specification.md +++ b/specs/giskard-specification.md @@ -9,7 +9,7 @@ **Document status:** Implementation-ready specification. **Audience:** An AI coding agent (and its human reviewer) implementing the system. -**Version:** 1.56 +**Version:** 1.57 > **Amendment — frontend approach (supersedes the Dioxus/WASM design below).** > This document was written targeting a **Dioxus fullstack / WebAssembly** frontend (`giskard-ui`), @@ -23,6 +23,25 @@ > below as historical design context, not a current requirement. The wire contract (`giskard-proto`) > and all backend design remain authoritative. +**Changelog (1.56 → 1.57), surfacing a blocked sub-agent:** +- **SB1:** Approval requests already route correctly to a managed sub-agent thread, but that thread + is deliberately absent from the sidebar, so every browser affordance keyed to a thread id used to + no-op for it. The browser must therefore resolve thread identity from the cached per-project + thread summaries — which include managed sub-agents — rather than from the rendered sidebar row. +- **SB2:** A managed sub-agent's `ThreadActivity` must be hoisted onto the nearest ancestor that + does have a sidebar row, and that row displays the most urgent state among itself and its hidden + descendants: `approval_requested` outranks `error`, which outranks an active turn. The row's + tooltip names the originating descendant, and a distinct marker separates a hoisted state from + the row's own. Ancestor walks are bounded so corrupted or cyclic ownership terminates. +- **SB3:** An approval notification for a sub-agent must name the child and its owning thread and + say that a sub-agent is blocked. Because the server materializes a child thread on its own, the + browser may receive activity for a thread it has never listed; it must refresh its cached thread + lists before naming or navigating to such a thread rather than falling back to an id prefix or + refusing to navigate. +- **SB4:** An approval also marks a turn active, so a sub-agent waiting on the user must be visually + distinct from one that is merely running, both on the header sub-agent monitor and on its card. + A card represents its whole owned subtree, since nested descendants are not listed separately. + **Changelog (1.55 → 1.56), image and file attachments:** - **A1:** User input may carry transient attachments. Browser requests include attachment metadata and base64 bytes, but persisted and in-memory cached `UserInput` values omit raw bytes. Image @@ -2855,6 +2874,19 @@ events through the same event handler used for live WebSocket events. when the page is hidden or the browser window is not focused. The browser must deduplicate the lightweight activity path against the later full approval event for the same `(thread_id, approval_id)`. +- **Activity for threads with no sidebar row (SB1–SB4):** managed sub-agent threads are hidden from + the sidebar but still produce `ThreadActivity`, so the browser must not derive thread identity + from the rendered row. It resolves the project, display title, kind, and parent from the cached + per-project thread summaries, and hoists a hidden thread's activity onto the nearest ancestor + that does have a row. That row shows the most urgent state among itself and its hidden + descendants — `approval_requested` > `error` > active turn — names the originating descendant in + its tooltip, and marks the state as hoisted. Ancestor and descendant walks are bounded so + corrupted or cyclic ownership terminates instead of spinning. A sub-agent's approval notification + names the child and its owning thread; since the server materializes a child on its own, activity + for an unlisted thread must trigger a thread-list refresh before naming or navigating to it. + `ThreadActivity` is a live signal and is not replayed on connect, so a browser that is not + connected when an approval is raised learns about it from the thread's live-turn snapshot when it + next opens that thread. - **Backpressure:** per-connection bounded queue; if a client falls behind, coalesce deltas (keep latest) rather than unbounded buffering. Heartbeat ping/pong; auto-reconnect on the client with resubscribe + state resync. Browser disconnects caused by mobile/tab suspension are diff --git a/tests/e2e/README.md b/tests/e2e/README.md index f857c3f..b114356 100644 --- a/tests/e2e/README.md +++ b/tests/e2e/README.md @@ -1,8 +1,8 @@ # End-to-end (Playwright) tests Browser tests that drive Giskard's real web UI — login, projects/threads, live message streaming, -linked sub-agent navigation/reload/prompt ordering/cascade deletion, and settings — through a -headless Chromium. +linked sub-agent navigation/reload/prompt ordering/cascade deletion, how a sub-agent blocked on an +approval is surfaced, and settings — through a headless Chromium. Everything runs **inside Docker**, so you don't need Node, npm, or the Playwright browsers on your host. The one thing you do need is Docker. @@ -20,7 +20,10 @@ REST/WebSocket API as the real `giskard-server`, but: the tests can log in and drive a thread with zero host-side setup. The sub-agent trigger, prompt, and reply constants in `giskard-server-replay` are mirrored by -`tests/helpers.ts`; update both locations together when changing that scenario. +`tests/helpers.ts`; update both locations together when changing that scenario. That includes the +approval-blocked sub-agent scenario (`SCRIPTED_SUBAGENT_APPROVAL_*`), whose child deliberately waits +before raising its approval: thread activity is broadcast live and never replayed on connect, so +firing immediately would race the browser's WebSocket and reach nobody. This keeps the suite hermetic: the production server needs a real, authenticated Codex CLI, which can't run unattended in CI. diff --git a/tests/e2e/tests/helpers.ts b/tests/e2e/tests/helpers.ts index e531f25..4fd5078 100644 --- a/tests/e2e/tests/helpers.ts +++ b/tests/e2e/tests/helpers.ts @@ -20,6 +20,92 @@ export const SCRIPTED_SUBAGENT_REPLY = "Child replay output"; */ export const SCRIPTED_APPROVAL_TRIGGER = "Trigger a scripted approval request."; +/** + * Prompt that spawns a linked child which raises an approval and then holds its turn open, while the + * parent turn completes normally. Kept in sync with `SCRIPTED_SUBAGENT_APPROVAL_TRIGGER` and friends + * in `crates/giskard-server/src/bin/giskard-server-replay.rs`. + */ +export const SCRIPTED_SUBAGENT_APPROVAL_TRIGGER = "Spawn a sub-agent that needs approval."; +export const SCRIPTED_SUBAGENT_APPROVAL_ID = "scripted-subagent-approval-1"; +export const SCRIPTED_SUBAGENT_APPROVAL_COMMAND = "rm -rf ./child-build"; +export const SCRIPTED_APPROVAL_SUBAGENT_NAME = "Approval child"; + +/** One entry recorded by the notification stub installed by {@link stubNotifications}. */ +export type RecordedNotification = { + title: string; + body: string; + tag: string; + data: { threadId?: string; approvalId?: string } | null; +}; + +declare global { + interface Window { + __giskardNotifications?: RecordedNotification[]; + } +} + +/** + * Replace both notification delivery paths (the `Notification` constructor and the service-worker + * registration's `showNotification`) with recorders, and pre-grant permission. Headless Chromium + * gives no way to read a real notification back, and the app picks its path at runtime, so both are + * stubbed into one array. Must be called before the page navigates. + */ +export async function stubNotifications(page: Page): Promise { + await page.addInitScript(() => { + const calls: RecordedNotification[] = []; + (window as Window).__giskardNotifications = calls; + const record = (title: string, options?: NotificationOptions) => { + calls.push({ + title, + body: (options && options.body) || "", + tag: (options && options.tag) || "", + data: (options && (options.data as RecordedNotification["data"])) || null, + }); + }; + class StubNotification { + static permission = "granted"; + static requestPermission() { return Promise.resolve("granted"); } + onclick: (() => void) | null = null; + onshow: (() => void) | null = null; + onerror: (() => void) | null = null; + onclose: (() => void) | null = null; + constructor(title: string, options?: NotificationOptions) { record(title, options); } + close() {} + } + Object.defineProperty(window, "Notification", { + configurable: true, writable: true, value: StubNotification, + }); + const registration = { + active: true, + scope: "/", + showNotification: (title: string, options?: NotificationOptions) => { + record(title, options); + return Promise.resolve(); + }, + getNotifications: () => Promise.resolve([]), + }; + Object.defineProperty(navigator, "serviceWorker", { + configurable: true, + value: { + ready: Promise.resolve(registration), + register: () => Promise.resolve(registration), + addEventListener: () => {}, + controller: null, + }, + }); + }); +} + +/** The notifications recorded so far, in order. */ +export function recordedNotifications(page: Page): Promise { + return page.evaluate(() => (window as Window).__giskardNotifications ?? []); +} + +/** The browser's currently selected `{ pid, tid }`, as persisted for reload restore. */ +export function selectedThread(page: Page): Promise<{ pid: string; tid: string } | null> { + return page.evaluate(() => JSON.parse(localStorage.getItem("giskard.lastThread") || "null")); +} + /** Log in through the real login form and wait for the app shell to become visible. */ export async function login(page: Page, password: string = PASSWORD): Promise { await page.goto("/"); diff --git a/tests/e2e/tests/subagent-approvals.spec.ts b/tests/e2e/tests/subagent-approvals.spec.ts new file mode 100644 index 0000000..c0c777b --- /dev/null +++ b/tests/e2e/tests/subagent-approvals.spec.ts @@ -0,0 +1,308 @@ +import { test, expect, type Page } from "@playwright/test"; +import { + SCRIPTED_APPROVAL_SUBAGENT_NAME, + SCRIPTED_SUBAGENT_APPROVAL_COMMAND, + SCRIPTED_SUBAGENT_APPROVAL_ID, + SCRIPTED_SUBAGENT_APPROVAL_TRIGGER, + login, + recordedNotifications, + selectedThread, + stubNotifications, +} from "./helpers"; + +/** Ask the server for the sub-agent child spawned under `parentTid`. */ +function childThreadId(page: Page, pid: string, parentTid: string): Promise { + return page.evaluate( + async ({ pid, parentTid }) => { + const res = await fetch(`/api/projects/${pid}/threads`); + const body = await res.json(); + const child = body.threads.find( + (t: { kind?: string; parent_thread_id?: string }) => + t.kind === "subagent" && String(t.parent_thread_id || "") === parentTid, + ); + return child ? String(child.id) : null; + }, + { pid, parentTid }, + ); +} + +/** Start a thread on the seeded Demo project with `text` and return its `{ pid, tid }`. */ +async function startThreadWith(page: Page, text: string): Promise<{ pid: string; tid: string }> { + const project = page.locator(".proj", { hasText: "Demo" }); + await project.locator(".project-add").click(); + await page.locator("#input").fill(text); + await page.locator("#sendBtn").click(); + await expect.poll(async () => (await selectedThread(page))?.tid ?? null).not.toBeNull(); + const selection = await selectedThread(page); + return { pid: selection!.pid, tid: selection!.tid }; +} + +// A managed sub-agent has no sidebar row of its own, so everything keyed off a thread id used to +// no-op for it: no status symbol anywhere, an anonymous notification, and a click that dead-ended on +// "Approval thread is not in the current project list." The child simply looked hung. These tests +// pin the three surfaces that now report it. +test.describe("sub-agent blocked on an approval", () => { + test.beforeEach(async ({ page }) => { + await stubNotifications(page); + await login(page); + }); + + test("reports on the parent row, the sub-agents button, and a named notification", async ({ page }) => { + // The parent's own turn completes; the child then blocks on an approval while the user is still + // sitting on the parent thread. + const parent = await startThreadWith(page, SCRIPTED_SUBAGENT_APPROVAL_TRIGGER); + const parentRow = page.locator(`.thread[data-tid="${parent.tid}"]`); + await expect(parentRow).toBeVisible(); + + // 1. The parent row carries the child's state: `!`, the approval colour, and the marker saying + // the state was hoisted from a descendant rather than being the row's own. + await expect(parentRow).toHaveClass(/\bactivity-approval\b/); + await expect(parentRow).toHaveClass(/\bactivity-subagent\b/); + const status = parentRow.locator(".thread-status"); + await expect(status).toHaveText("!"); + // The tooltip names the blocked sub-agent, so the row is not an unattributable badge. + await expect(status).toHaveAttribute( + "title", + new RegExp(`${SCRIPTED_APPROVAL_SUBAGENT_NAME}: Approval requested`), + ); + + // The child is deliberately absent from the sidebar; only the ancestor reports it. + const child = await childThreadId(page, parent.pid, parent.tid); + expect(child).toBeTruthy(); + await expect(page.locator(`.thread[data-tid="${child}"]`)).toHaveCount(0); + + // 2. The header monitor separates "waiting on the user" from "busy". + const subagentsBtn = page.locator("#subagentsBtn"); + await expect(subagentsBtn).toHaveClass(/\bstate-approval\b/); + await expect(subagentsBtn).toHaveAttribute("title", /waiting for approval/); + await subagentsBtn.click(); + const card = page.locator("#subagentsMenu .subagent-card"); + await expect(card).toHaveCount(1); + await expect(card).toHaveClass(/\bneeds-approval\b/); + await expect(card.locator(".subagent-card-state")).toHaveText("Needs approval"); + await expect(page.locator("#subagentsMenu .subagents-summary")).toHaveText( + /1 waiting for approval/, + ); + await page.locator("#subagentsClose").click(); + + // 3. The notification names the child and its parent instead of a bare id prefix. + await expect.poll(async () => (await recordedNotifications(page)).length).toBeGreaterThan(0); + const approval = (await recordedNotifications(page)).find( + (n) => n.data?.approvalId === SCRIPTED_SUBAGENT_APPROVAL_ID, + ); + expect(approval).toBeTruthy(); + expect(approval!.title).toBe("Giskard: sub-agent approval needed"); + expect(approval!.body).toContain(`${SCRIPTED_APPROVAL_SUBAGENT_NAME} (in `); + expect(approval!.body).toContain("Approval requested"); + expect(approval!.data?.threadId).toBe(child); + }); + + test("clicking the notification opens the child and the approval is answerable", async ({ page }) => { + const parent = await startThreadWith(page, SCRIPTED_SUBAGENT_APPROVAL_TRIGGER); + const parentRow = page.locator(`.thread[data-tid="${parent.tid}"]`); + await expect(parentRow).toHaveClass(/\bactivity-approval\b/); + const child = await childThreadId(page, parent.pid, parent.tid); + expect(child).toBeTruthy(); + + // Both delivery paths (service-worker postMessage and desktop Notification.onclick) funnel into + // this one handler, so driving it directly exercises the navigation fix faithfully. + await page.evaluate( + ({ child, approvalId }) => + (window as unknown as { + handleNotificationClick: (data: { threadId: string; approvalId: string }) => void; + }).handleNotificationClick({ threadId: child, approvalId }), + { child: child!, approvalId: SCRIPTED_SUBAGENT_APPROVAL_ID }, + ); + + // It navigates to the sub-agent instead of warning that the thread is not in the project list. + await expect.poll(async () => (await selectedThread(page))?.tid).toBe(child); + await expect(page.locator("#notices .warn")).toHaveCount(0); + await expect(page.getByRole("button", { name: /Back to parent thread:/ })).toBeVisible(); + + // The approval is live and answerable on the child thread. + const transcript = page.locator("#transcript"); + const approvalCard = transcript.locator(".msg.approval"); + await expect(approvalCard).toBeVisible(); + await expect(approvalCard).toContainText(SCRIPTED_SUBAGENT_APPROVAL_COMMAND); + await approvalCard.getByRole("button", { name: "Accept", exact: true }).click(); + await expect(approvalCard).toHaveClass(/\bresolved\b/); + await expect( + transcript.locator(".msg.agent", { hasText: "Approval recorded: accept" }), + ).toBeVisible(); + + // Opening the child clears the escalation from its ancestor: the row may still report the child + // as running, but it must stop demanding attention. + await expect(parentRow).not.toHaveClass(/\bactivity-approval\b/); + await expect(page.locator("#subagentsBtn")).not.toHaveClass(/\bstate-approval\b/); + }); + + // The hoisting rules decide which of several states one row shows. Drive them through the app's + // own entry points so the priority order and the cycle guard are pinned without depending on + // harness timing. + test("hoists the most urgent descendant state and survives corrupted ownership", async ({ page }) => { + const result = await page.evaluate(() => { + const app = window as unknown as { + rememberProjectThreads: (pid: string, threads: unknown[]) => void; + handleThreadActivity: (msg: Record) => void; + clearApprovalThreadActivity: (tid: string, approvalId: string) => void; + activityHostThreadId: (tid: string) => string | null; + threadDescendsFrom: (tid: string, ancestorId: string) => boolean; + effectiveThreadActivity: (tid: string) => { + activity: { kind: string } | null; + origin: string | null; + }; + renderThreadActivityIndicator: (tid: string) => void; + }; + + // A synthetic project: root (the only thread with a sidebar row) → child → grandchild, plus a + // self-referential record standing in for corrupted ownership metadata. + app.rememberProjectThreads("synthetic-project", [ + { id: "root", kind: "primary", parent_thread_id: null, title: "Root" }, + { id: "child", kind: "subagent", parent_thread_id: "root", title: "Sub-agent: Child" }, + { id: "grandchild", kind: "subagent", parent_thread_id: "child", title: "Sub-agent: Grandchild" }, + { id: "loop", kind: "subagent", parent_thread_id: "loop", title: "Sub-agent: Loop" }, + ]); + const row = document.createElement("div"); + row.className = "thread"; + row.dataset.tid = "root"; + row.dataset.pid = "synthetic-project"; + const status = document.createElement("span"); + status.className = "thread-status"; + row.append(status); + document.body.append(row); + + try { + // A merely-running root must not mask a grandchild blocked on an approval. + app.handleThreadActivity({ + thread_id: "root", kind: "progress", active_turn: true, summary: "Turn running", + }); + app.handleThreadActivity({ + thread_id: "grandchild", kind: "approval_requested", active_turn: true, + approval_id: "a1", summary: "Approval requested", + }); + const hoisted = { + host: app.activityHostThreadId("grandchild"), + winner: app.effectiveThreadActivity("root").activity?.kind ?? null, + origin: app.effectiveThreadActivity("root").origin, + statusText: status.textContent, + rowClass: row.className, + title: status.title, + descends: app.threadDescendsFrom("grandchild", "root"), + unrelated: app.threadDescendsFrom("root", "grandchild"), + }; + + // Once answered, the row falls back to root's own running state and drops the escalation. + app.clearApprovalThreadActivity("grandchild", "a1"); + const afterAnswer = { statusText: status.textContent, rowClass: row.className }; + + // Corrupted ownership (a thread that is its own parent) must terminate, not spin. + const cycle = { + host: app.activityHostThreadId("loop"), + descends: app.threadDescendsFrom("loop", "root"), + }; + return { hoisted, afterAnswer, cycle }; + } finally { + row.remove(); + } + }); + + expect(result.hoisted.host).toBe("root"); + expect(result.hoisted.winner).toBe("approval_requested"); + expect(result.hoisted.origin).toBe("grandchild"); + expect(result.hoisted.statusText).toBe("!"); + expect(result.hoisted.rowClass).toContain("activity-approval"); + expect(result.hoisted.rowClass).toContain("activity-subagent"); + // Named after the blocked descendant, not the row it is displayed on. + expect(result.hoisted.title).toBe("Grandchild: Approval requested"); + expect(result.hoisted.descends).toBe(true); + expect(result.hoisted.unrelated).toBe(false); + + expect(result.afterAnswer.statusText).toBe("o"); + expect(result.afterAnswer.rowClass).not.toContain("activity-approval"); + expect(result.afterAnswer.rowClass).not.toContain("activity-subagent"); + + expect(result.cycle.host).toBeNull(); + expect(result.cycle.descends).toBe(false); + }); + + // An id that no refresh can resolve — trailing activity from a deleted thread, say — must not + // re-fetch every project list on every event for the rest of the turn. + test("spends a bounded number of refreshes on an unresolvable thread", async ({ page }) => { + let threadListFetches = 0; + page.on("request", (request) => { + if (request.method() === "GET" && /\/api\/projects\/[^/]+\/threads$/.test(request.url())) { + threadListFetches += 1; + } + }); + // Settle any refresh the login/bootstrap path already scheduled. + await page.waitForTimeout(1000); + const before = threadListFetches; + + await page.evaluate(async () => { + const app = window as unknown as { + handleThreadActivity: (msg: Record) => void; + }; + // Twelve events for a thread the server will never list. Each is spaced past the burst + // debounce so an ungated implementation would schedule a refresh for every one. + for (let i = 0; i < 12; i += 1) { + app.handleThreadActivity({ + thread_id: "thread-that-does-not-exist", + kind: "progress", + active_turn: true, + summary: `tick ${i}`, + }); + await new Promise((resolve) => setTimeout(resolve, 120)); + } + }); + await page.waitForTimeout(1500); + + const projects = await page.evaluate(() => document.querySelectorAll(".proj").length); + // At most STALE_THREAD_LIST_REFRESH_MAX_ATTEMPTS (3) refreshes, each reloading every project. + expect(threadListFetches - before).toBeLessThanOrEqual(3 * projects); + expect(threadListFetches - before).toBeGreaterThan(0); + }); + + // Two events can describe one approval (the live activity broadcast and the live-turn snapshot + // replay). Only one notification may reach the user, even though the handler now awaits a + // thread-list refresh between the dedup check and the record. + test("notifies once when two events describe the same approval", async ({ page }) => { + const parent = await startThreadWith(page, SCRIPTED_SUBAGENT_APPROVAL_TRIGGER); + await expect(page.locator(`.thread[data-tid="${parent.tid}"]`)).toHaveClass( + /\bactivity-approval\b/, + ); + const child = await childThreadId(page, parent.pid, parent.tid); + // Without this the events below would fire with a null thread id, be ignored, and leave the + // final count at 1 — green without ever reaching the dedup gate. + expect(child).toBeTruthy(); + const seen = (await recordedNotifications(page)).filter( + (n) => n.data?.approvalId === SCRIPTED_SUBAGENT_APPROVAL_ID, + ).length; + expect(seen).toBe(1); + + // Fire two more events for the same approval in the same tick, so both reach the dedup gate + // before either records. Neither may notify again. + await page.evaluate( + ({ child, approvalId }) => { + const app = window as unknown as { + handleThreadActivity: (msg: Record) => void; + }; + const event = { + thread_id: child, + kind: "approval_requested", + active_turn: true, + approval_id: approvalId, + summary: "Approval requested", + }; + app.handleThreadActivity({ ...event }); + app.handleThreadActivity({ ...event }); + }, + { child: child!, approvalId: SCRIPTED_SUBAGENT_APPROVAL_ID }, + ); + await page.waitForTimeout(1000); + + const after = (await recordedNotifications(page)).filter( + (n) => n.data?.approvalId === SCRIPTED_SUBAGENT_APPROVAL_ID, + ).length; + expect(after).toBe(1); + }); +});