From 2ead1fca3f9cda743a4e9cf105a8b2f000e9698d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 14:42:32 +0000 Subject: [PATCH] Treat an approval and a server request as one waiting state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An approval and a server request both stop a turn until the user answers. The browser only ever treated the first as blocking: a thread waiting on a server request fell through to the generic active-turn branch, rendered as `o` — indistinguishable from merely busy — ranked below an approval when a row had to represent several descendants, never reached "Needs approval" on a sub-agent card, and fired no notification at all. A sub-agent blocked on a question was exactly as invisible as the case fixed in #125, on the path next to it. Codex itself already blurs the split: MCP tool approvals arrive as `requestUserInput` server requests and get promoted to approval cards by a marker. The distinction is a protocol artifact, not something worth surfacing, so activity ranking, the sidebar glyph, the sub-agent monitor and notifications now key off "waits on the user" rather than "is an approval". Card rendering deliberately stays per-kind — a decision has fixed choices, a server request has a per-method form — because that part was never duplicated. Names follow the behaviour. State that now holds both kinds is renamed away from "approval" (`activity-waiting`, `waitingNotificationKey`, `notifiedRequests`, `waitingRequestRowById`, and so on) so it stops claiming to be approval-only; `pendingApprovals` and `answeredApprovals` keep their names, because they still hold only approvals. The card label becomes "Waiting on you", which is true of both. Writing the test for it surfaced a second gap: the waiting state has to clear when the user acts, not when the harness confirms. Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes from the harness on its own schedule and may never come, so the sidebar kept demanding attention for something already answered. The browser now clears its own waiting state on send. The activity summary also stops being the raw JSON-RPC method name, which was reaching the notification body. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY --- crates/giskard-server/src/registry.rs | 2 +- crates/giskard-server/src/routes.rs | 2 +- crates/giskard-server/static/app.css | 18 +- crates/giskard-server/static/app.js | 375 ++++++++++++--------- crates/giskard-server/static/index.html | 4 +- crates/giskard-server/static/sw.js | 5 +- crates/giskard-server/tests/ui.rs | 171 +++++----- docs/subagents.md | 10 +- specs/giskard-specification.md | 17 +- tests/e2e/tests/approvals.spec.ts | 13 +- tests/e2e/tests/helpers.ts | 4 +- tests/e2e/tests/server-requests.spec.ts | 101 +++++- tests/e2e/tests/subagent-approvals.spec.ts | 36 +- 13 files changed, 466 insertions(+), 292 deletions(-) diff --git a/crates/giskard-server/src/registry.rs b/crates/giskard-server/src/registry.rs index ed79eea..ef148d6 100644 --- a/crates/giskard-server/src/registry.rs +++ b/crates/giskard-server/src/registry.rs @@ -3638,7 +3638,7 @@ fn thread_activity_from_event( server_request_id: request.id.to_string(), }; activity.active_turn = true; - activity.summary = Some(format!("{} request", request.method)); + activity.summary = Some("Waiting for your input".to_string()); } AgentEvent::ServerRequestResolved { .. } => { activity.summary = Some("Request resolved".into()); diff --git a/crates/giskard-server/src/routes.rs b/crates/giskard-server/src/routes.rs index ac62c37..0a8dd79 100644 --- a/crates/giskard-server/src/routes.rs +++ b/crates/giskard-server/src/routes.rs @@ -2833,7 +2833,7 @@ async fn send_activity_bootstrap( server_request_id: request.id.to_string(), }, active_turn: true, - summary: Some(format!("{} request", request.method)), + summary: Some("Waiting for your input".to_string()), }); } } diff --git a/crates/giskard-server/static/app.css b/crates/giskard-server/static/app.css index 48c317b..813c5c0 100644 --- a/crates/giskard-server/static/app.css +++ b/crates/giskard-server/static/app.css @@ -107,7 +107,8 @@ .thread-title { min-width:0; overflow:hidden; text-overflow:ellipsis; white-space:nowrap; } .thread.has-activity .thread-title { color:var(--fg); } .thread.activity-running .thread-status { color:#d29922; } - .thread.activity-approval .thread-status { color:#f0d98c; } + /* "Waiting on you" — an approval or a server request; to the reader they are one state. */ + .thread.activity-waiting .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". */ @@ -204,9 +205,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); } + /* A sub-agent waiting on the user — for an approval or an answer — is not waiting on the model, + so it gets the accent treatment to separate it from merely-running at a glance. */ + .subagents-btn.state-waiting { 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); @@ -225,9 +226,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-state.waiting { color:#ffd97d; font-weight:600; } + .subagent-card.waiting { border-color:#e3b341; background:rgba(227,179,65,.12); } + .subagents-summary.waiting { 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; } @@ -341,7 +342,8 @@ .plan-card-body[hidden] { display:none; } .plan-card-body .plan-explanation { margin-bottom:8px; } .msg.approval { background:#241d10; border:1px solid #d29922; max-width:100%; } - .msg.approval-target { outline:2px solid #f0d98c; outline-offset:3px; } + /* Focus highlight for a notification click — lands on an approval or a server-request card. */ + .msg.waiting-target { outline:2px solid #f0d98c; outline-offset:3px; } .msg.approval.resolved.decision-accept, .msg.approval.resolved.decision-session { background:#172217; border-color:#3fb950; } .msg.approval.resolved.decision-decline { background:#2b1618; border-color:#f85149; } diff --git a/crates/giskard-server/static/app.js b/crates/giskard-server/static/app.js index e19f136..8fa3f9d 100644 --- a/crates/giskard-server/static/app.js +++ b/crates/giskard-server/static/app.js @@ -55,7 +55,7 @@ let state = { threadReadOnly:false, readOnlyProvider:null, readOnlyMessage:null, pickerTypeahead:"", pickerTypeaheadTimer:null, pickerSelectedRow:null, currentPlan:null, planExpanded:localStorage.getItem("giskard.planExpanded")==="1", - threadActivity:new Map(), pendingApprovalFocus:null, notifiedApprovals:new Map(), bootstrapNotifiedApprovals:new Set(), approvalNotifications:new Map(), browserDiagnostics:[], + threadActivity:new Map(), pendingWaitingFocus:null, notifiedRequests:new Map(), bootstrapNotifiedRequests:new Set(), waitingNotifications:new Map(), browserDiagnostics:[], subagentImports:new Map(), projectThreads:new Map(), threadIndex:new Map(), lastNotificationPromptNoticeAt:0, swRegistration:null, pendingAttachments:[], attachmentGeneration:0, pendingAttachmentOperations:new Map(), @@ -280,15 +280,16 @@ async function notificationRegistration() { } // A notification was clicked — delivered by the service worker as a postMessage, or by the desktop -// Notification's onclick. Approval notifications jump to the pending approval. +// Notification's onclick. The click jumps to whatever the thread is waiting on — an approval card +// or a server-request card. function handleNotificationClick(data) { - if (data && data.threadId && data.approvalId) { - recordNotificationDiagnostic("approval_notification_clicked", { + if (data && data.threadId && data.requestId) { + recordNotificationDiagnostic("waiting_notify_clicked", { tid: data.threadId, - approval_id: data.approvalId + request_id: data.requestId }); - closeApprovalNotification(data.threadId, data.approvalId); - focusApprovalTarget(data.threadId, data.approvalId); + closeWaitingNotification(data.threadId, data.requestId); + focusWaitingRequest(data.threadId, data.requestId); } } @@ -323,7 +324,7 @@ function setNotificationButtonState(btn, label, disabled) { btn.textContent = "!"; btn.title = label; btn.setAttribute("aria-label", label); - btn.hidden = label === "Approval notifications enabled" || label === "Notifications unavailable"; + btn.hidden = label === "Notifications enabled" || label === "Notifications unavailable"; } else { btn.textContent = label; btn.title = label; @@ -334,13 +335,13 @@ function setNotificationButtonState(btn, label, disabled) { function refreshNotificationButton() { const buttons = notificationPermissionButtons(); if (!buttons.length || !("Notification" in window)) return; - let label = "Enable approval notifications"; + let label = "Enable notifications"; let disabled = false; if (!window.isSecureContext) { label = "Notifications require HTTPS or localhost"; disabled = true; } else if (Notification.permission === "granted") { - label = "Approval notifications enabled"; + label = "Notifications enabled"; disabled = true; } else if (Notification.permission === "denied") { label = "Notifications blocked by browser"; @@ -367,11 +368,11 @@ function browserDiagnosticsSnapshot() { focused: document.hasFocus ? document.hasFocus() : null, thread_id: state.threadId || null, ws_status: state.wsStatus, - notified_count: state.notifiedApprovals.size, + notified_count: state.notifiedRequests.size, dedup_window_ms: NOTIFICATION_DEDUP_MS, button_count: notificationPermissionButtons().length, - last_approval_decision: lastNotificationDiagnostic(isApprovalNotificationDecision), - recent_approval_decisions: recentNotificationDiagnostics(isApprovalNotificationDecision, 6), + last_waiting_notification: lastNotificationDiagnostic(isWaitingNotificationDiagnostic), + recent_waiting_notifications: recentNotificationDiagnostics(isWaitingNotificationDiagnostic, 6), diagnostics }; } @@ -380,15 +381,11 @@ function notificationDebugSnapshot() { return browserDiagnosticsSnapshot(); } -function isApprovalNotificationDecision(entry) { +function isWaitingNotificationDiagnostic(entry) { const reason = entry && entry.reason ? entry.reason : ""; const detail = entry && entry.detail ? entry.detail : {}; - return reason === "approval_notify_received" || - reason.startsWith("approval_notify_suppressed_") || - reason === "approval_notify_constructor_failed" || - reason === "approval_notify_created" || - (reason === "browser_notification_created" && detail.kind === "approval") || - (reason.startsWith("browser_notification_") && detail.kind === "approval"); + return reason.startsWith("waiting_notify_") || + (reason.startsWith("browser_notification_") && detail.kind === "waiting_request"); } function lastNotificationDiagnostic(predicate) { @@ -447,8 +444,8 @@ function renderBrowserDiagnosticsPanel(snapshot, reveal) { if (!log) return; snapshot = snapshot || browserDiagnosticsSnapshot(); const last = snapshot.diagnostics[snapshot.diagnostics.length - 1]; - const lastApproval = snapshot.last_approval_decision; - const approvalDetail = lastApproval && lastApproval.detail ? lastApproval.detail : {}; + const lastWaiting = snapshot.last_waiting_notification; + const waitingDetail = lastWaiting && lastWaiting.detail ? lastWaiting.detail : {}; const lines = [ `version: ${snapshot.version}`, `permission: ${snapshot.permission}`, @@ -458,18 +455,18 @@ function renderBrowserDiagnosticsPanel(snapshot, reveal) { `thread: ${snapshot.thread_id || "none"}`, `ws: ${snapshot.ws_status}`, `dedupMs: ${snapshot.dedup_window_ms}`, - `lastApproval: ${lastApproval ? lastApproval.reason : "none"}`, - `approvalSource: ${approvalDetail.source || "none"}`, - `approvalId: ${approvalDetail.approval_id || "none"}`, + `lastRequest: ${lastWaiting ? lastWaiting.reason : "none"}`, + `requestSource: ${waitingDetail.source || "none"}`, + `requestId: ${waitingDetail.request_id || "none"}`, `last: ${last ? last.reason : "none"}` ]; - const recent = snapshot.recent_approval_decisions || []; + const recent = snapshot.recent_waiting_notifications || []; if (recent.length) { - lines.push("recentApprovals:"); + lines.push("recentRequests:"); for (const entry of recent) { const detail = entry.detail || {}; const suffix = detail.age_ms !== undefined ? ` age=${detail.age_ms}ms` : ""; - lines.push(`- ${entry.reason} source=${detail.source || "none"} id=${detail.approval_id || "none"} visible=${entry.visibility} focused=${entry.focused}${suffix}`); + lines.push(`- ${entry.reason} source=${detail.source || "none"} id=${detail.request_id || "none"} visible=${entry.visibility} focused=${entry.focused}${suffix}`); } } const latest = snapshot.diagnostics.slice(-20); @@ -479,7 +476,7 @@ function renderBrowserDiagnosticsPanel(snapshot, reveal) { const detail = entry.detail || {}; const fields = []; if (detail.source) fields.push(`source=${detail.source}`); - if (detail.approval_id !== undefined && detail.approval_id !== null) fields.push(`approval=${detail.approval_id}`); + if (detail.request_id !== undefined && detail.request_id !== null) fields.push(`request=${detail.request_id}`); if (detail.status) fields.push(`status=${detail.status}`); if (detail.error) fields.push(`error=${detail.error}`); lines.push(`- ${entry.at} ${entry.category}:${entry.reason} visible=${entry.visibility} focused=${entry.focused}${fields.length ? " " + fields.join(" ") : ""}`); @@ -1047,12 +1044,27 @@ function clearThreadActivity(tid) { renderSubagentsButton(); } +// A thread is *waiting on the user* when it cannot proceed until the user answers something. Codex +// splits that into approvals and server requests — and already blurs the line itself, since MCP tool +// approvals arrive as `requestUserInput` and get promoted to approval cards — but to the person +// looking at the sidebar they are one state: you are being asked for something. +function activityWaitsOnUser(activity) { + if (!activity) return false; + return activity.kind === "approval_requested" || activity.kind === "server_request_received"; +} + +// The id of whatever the thread is waiting for, whichever kind it is. +function waitingRequestId(activity) { + if (!activity) return null; + return activity.approval_id || activity.server_request_id || null; +} + // 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. +// A blocked child must never be masked by a merely-running parent, so waiting-on-the-user outranks +// error outranks running. function threadActivityRank(activity) { if (!activity) return -1; - if (activity.kind === "approval_requested") return 3; + if (activityWaitsOnUser(activity)) return 3; if (activity.kind === "error") return 2; return activity.active_turn ? 1 : 0; } @@ -1106,16 +1118,16 @@ function renderThreadActivityIndicator(tid, hosts) { 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-waiting", visible && activityWaitsOnUser(activity)); 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-running", visible && activity && activity.active_turn && !activityWaitsOnUser(activity)); el.classList.toggle("activity-subagent", visible && !!origin); if (!visible) { status.textContent = ""; status.title = ""; return; } - if (activity.kind === "approval_requested") status.textContent = "!"; + if (activityWaitsOnUser(activity)) status.textContent = "!"; else if (activity.kind === "error") status.textContent = "x"; else if (activity.active_turn) status.textContent = "o"; else status.textContent = "*"; @@ -1728,10 +1740,10 @@ async function openThread(pid, tid, title, opts) { opts = opts || {}; saveComposerDraft(); if (!opts.firstTurnStarting) state.firstTurnStartingThreadId = null; - if (opts.focusApprovalId) { - state.pendingApprovalFocus = { + if (opts.focusRequestId) { + state.pendingWaitingFocus = { threadId:String(tid), - approvalId:String(opts.focusApprovalId), + requestId:String(opts.focusRequestId), attempts:0 }; } @@ -1801,7 +1813,7 @@ async function openThread(pid, tid, title, opts) { } } connectWs(); - schedulePendingApprovalFocus(); + schedulePendingWaitingFocus(); } function clearWsReconnectTimer() { @@ -2205,20 +2217,20 @@ function handleThreadActivity(msg) { if (!knownThreadMeta(tid)) noteUnresolvedThread(tid); renderThreadActivityIndicator(tid); renderSubagentsButton(); - if (activity.kind === "approval_requested") maybeNotifyApproval(tid, activity); + if (activityWaitsOnUser(activity)) maybeNotifyWaitingRequest(tid, activity); } -// Cross-thread activity the server replayed because we were not connected when it happened. Paints -// the badge exactly like a live event; notification is gated separately, because a reconnect is not -// news — see `maybeNotifyApproval`. // Undo a notification claim when the notification did not actually reach the user. Both records // must be released together: leaving the session-scoped one set would silence every later replay of -// an approval the user was never shown. -function releaseApprovalNotificationClaim(notificationKey) { - state.notifiedApprovals.delete(notificationKey); - state.bootstrapNotifiedApprovals.delete(notificationKey); +// a request the user was never shown. +function releaseWaitingNotificationClaim(notificationKey) { + state.notifiedRequests.delete(notificationKey); + state.bootstrapNotifiedRequests.delete(notificationKey); } +// Cross-thread activity the server replayed because we were not connected when it happened. Paints +// the badge exactly like a live event; notification is gated separately, because a reconnect is not +// news — see `maybeNotifyWaitingRequest`. function handleThreadActivityBootstrap(msg) { const activities = (msg && Array.isArray(msg.activities)) ? msg.activities : []; if (!activities.length) return; @@ -2228,62 +2240,62 @@ function handleThreadActivityBootstrap(msg) { } } -async function maybeNotifyApproval(tid, activity) { - const notificationKey = approvalNotificationKey(tid, activity && activity.approval_id); - if (!activity || activity.kind !== "approval_requested" || !notificationKey) { - recordNotificationDiagnostic("approval_notify_skipped_invalid_call", { tid, activity }); +async function maybeNotifyWaitingRequest(tid, activity) { + const notificationKey = waitingNotificationKey(tid, waitingRequestId(activity)); + if (!activityWaitsOnUser(activity) || !notificationKey) { + recordNotificationDiagnostic("waiting_notify_skipped_invalid_call", { tid, activity }); return; } - recordNotificationDiagnostic("approval_notify_received", { + recordNotificationDiagnostic("waiting_notify_received", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown", summary: activity.summary || "" }); const focused = document.hasFocus ? document.hasFocus() : true; if (document.visibilityState === "visible" && focused && String(tid) === String(state.threadId)) { - recordNotificationDiagnostic("approval_notify_suppressed_visible_current_thread", { + recordNotificationDiagnostic("waiting_notify_suppressed_visible_current_thread", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown" }); return; } if (!("Notification" in window)) { - recordNotificationDiagnostic("approval_notify_suppressed_unsupported", { + recordNotificationDiagnostic("waiting_notify_suppressed_unsupported", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown" }); return; } if (Notification.permission !== "granted") { - recordNotificationDiagnostic("approval_notify_suppressed_permission", { + recordNotificationDiagnostic("waiting_notify_suppressed_permission", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown" }); maybeNoticeNotificationPermission(); return; } - // A replayed approval is not news. `notifiedApprovals` cannot answer "have we ever alerted for + // A replayed request is not news. `notifiedRequests` cannot answer "have we ever alerted for // this?" because it is pruned on a 15s window, so a laptop resuming repeatedly would re-alert for // the same blocked approval. Track bootstrap alerts separately and permanently for this page // session: a reconnect stays silent, while a genuine reload starts a new session and re-alerts. - if (activity.source === "connect_bootstrap" && state.bootstrapNotifiedApprovals.has(notificationKey)) { - recordNotificationDiagnostic("approval_notify_suppressed_replay", { + if (activity.source === "connect_bootstrap" && state.bootstrapNotifiedRequests.has(notificationKey)) { + recordNotificationDiagnostic("waiting_notify_suppressed_replay", { tid, - approval_id: activity.approval_id + request_id: waitingRequestId(activity) }); return; } const now = Date.now(); pruneNotificationDedup(now); - const notifiedAt = state.notifiedApprovals.get(notificationKey); + const notifiedAt = state.notifiedRequests.get(notificationKey); if (notifiedAt && now - notifiedAt < NOTIFICATION_DEDUP_MS) { - recordNotificationDiagnostic("approval_notify_suppressed_duplicate", { + recordNotificationDiagnostic("waiting_notify_suppressed_duplicate", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown", age_ms: now - notifiedAt }); @@ -2293,38 +2305,42 @@ async function maybeNotifyApproval(tid, activity) { // 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); - state.bootstrapNotifiedApprovals.add(notificationKey); + state.notifiedRequests.set(notificationKey, now); + state.bootstrapNotifiedRequests.add(notificationKey); // 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 = approvalNotificationLabel(meta, tid); + const title = waitingNotificationLabel(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); + // Stable per-request 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 request resolves. + const notificationTag = waitingNotificationTag(tid, waitingRequestId(activity)); + // The two kinds share the *state*, not the wording: telling someone an approval is needed when + // the agent asked them a question sends them looking for a decision that does not exist. + const needed = activity.kind === "approval_requested" ? "approval" : "input"; + const headline = isSubagent ? `Giskard: sub-agent ${needed} needed` : `Giskard: ${needed} needed`; let result; try { - result = await showAppNotification(isSubagent ? "Giskard: sub-agent approval needed" : "Giskard: approval needed", { + result = await showAppNotification(headline, { body: activity.summary ? `${title}: ${activity.summary}` : title, tag: notificationTag, renotify: true, requireInteraction: true, - data: { threadId:tid, approvalId:activity.approval_id } + data: { threadId:tid, requestId:waitingRequestId(activity) } }, { - kind: "approval", + kind: "waiting_request", tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown", tag: notificationTag }); } catch (e) { - releaseApprovalNotificationClaim(notificationKey); - recordNotificationDiagnostic("approval_notify_constructor_failed", { + releaseWaitingNotificationClaim(notificationKey); + recordNotificationDiagnostic("waiting_notify_constructor_failed", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown", error: e && e.message ? e.message : String(e) }); @@ -2332,18 +2348,18 @@ async function maybeNotifyApproval(tid, activity) { return; } if (!result) { - releaseApprovalNotificationClaim(notificationKey); + releaseWaitingNotificationClaim(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) { - trackApprovalNotification(notificationKey, result.notification); - result.notification.onclick = () => handleNotificationClick({ threadId: tid, approvalId: activity.approval_id }); + trackWaitingNotification(notificationKey, result.notification); + result.notification.onclick = () => handleNotificationClick({ threadId: tid, requestId: waitingRequestId(activity) }); } - recordNotificationDiagnostic("approval_notify_created", { + recordNotificationDiagnostic("waiting_notify_created", { tid, - approval_id: activity.approval_id, + request_id: waitingRequestId(activity), source: activity.source || "unknown", title, tag: notificationTag, @@ -2369,9 +2385,9 @@ async function showAppNotification(title, options, diagnosticDetail) { notification.onshow = () => recordNotificationDiagnostic("browser_notification_show", diagnosticDetail); notification.onerror = () => recordNotificationDiagnostic("browser_notification_error", diagnosticDetail); notification.onclose = () => { - if (diagnosticDetail.kind === "approval") { - untrackApprovalNotification( - approvalNotificationKey(diagnosticDetail.tid, diagnosticDetail.approval_id), + if (diagnosticDetail.kind === "waiting_request") { + untrackWaitingNotification( + waitingNotificationKey(diagnosticDetail.tid, diagnosticDetail.request_id), notification ); } @@ -2382,67 +2398,67 @@ async function showAppNotification(title, options, diagnosticDetail) { function pruneNotificationDedup(now) { now = now || Date.now(); - for (const [key, notifiedAt] of state.notifiedApprovals) { - if (now - notifiedAt >= NOTIFICATION_DEDUP_MS) state.notifiedApprovals.delete(key); + for (const [key, notifiedAt] of state.notifiedRequests) { + if (now - notifiedAt >= NOTIFICATION_DEDUP_MS) state.notifiedRequests.delete(key); } } -function approvalNotificationKey(tid, approvalId) { +function waitingNotificationKey(tid, requestId) { const threadKey = tid === undefined || tid === null ? "" : String(tid); - const approvalKey = approvalId === undefined || approvalId === null ? "" : String(approvalId); - if (!threadKey || !approvalKey) return ""; - return `${threadKey}:${approvalKey}`; + const requestKey = requestId === undefined || requestId === null ? "" : String(requestId); + if (!threadKey || !requestKey) return ""; + return `${threadKey}:${requestKey}`; } // Stable OS notification tag for an approval — used to show, dedup, and (on the service-worker // path) close the notification without holding a Notification object. -function approvalNotificationTag(tid, approvalId) { - return `giskard-approval-${tid}-${approvalId}`; +function waitingNotificationTag(tid, requestId) { + return `giskard-waiting-${tid}-${requestId}`; } -function trackApprovalNotification(key, notification) { +function trackWaitingNotification(key, notification) { if (!key || !notification) return; - let notifications = state.approvalNotifications.get(key); + let notifications = state.waitingNotifications.get(key); if (!notifications) { notifications = new Set(); - state.approvalNotifications.set(key, notifications); + state.waitingNotifications.set(key, notifications); } notifications.add(notification); } -function untrackApprovalNotification(key, notification) { - const notifications = state.approvalNotifications.get(key); +function untrackWaitingNotification(key, notification) { + const notifications = state.waitingNotifications.get(key); if (!notifications) return; notifications.delete(notification); - if (notifications.size === 0) state.approvalNotifications.delete(key); + if (notifications.size === 0) state.waitingNotifications.delete(key); } -function closeApprovalNotification(tid, approvalId) { - const key = approvalNotificationKey(tid, approvalId); +function closeWaitingNotification(tid, requestId) { + const key = waitingNotificationKey(tid, requestId); if (!key) return; // Service-worker notifications aren't held as objects: fetch them back by tag and close them. const reg = state.swRegistration; if (reg && typeof reg.getNotifications === "function") { - const tag = approvalNotificationTag(tid, approvalId); + const tag = waitingNotificationTag(tid, requestId); reg.getNotifications({ tag }) .then((ns) => ns.forEach((n) => { try { n.close(); } catch {} })) .catch(() => {}); } // Desktop (constructor) notifications are tracked objects. - const notifications = state.approvalNotifications.get(key); + const notifications = state.waitingNotifications.get(key); if (!notifications) return; - state.approvalNotifications.delete(key); + state.waitingNotifications.delete(key); for (const notification of notifications) { try { notification.close(); - recordNotificationDiagnostic("approval_notification_closed", { + recordNotificationDiagnostic("waiting_notify_closed", { tid, - approval_id: approvalId + request_id: requestId }); } catch (e) { - recordNotificationDiagnostic("approval_notification_close_failed", { + recordNotificationDiagnostic("waiting_notify_close_failed", { tid, - approval_id: approvalId, + request_id: requestId, error: e && e.message ? e.message : String(e) }); } @@ -2454,13 +2470,14 @@ function maybeNoticeNotificationPermission() { const now = Date.now(); if (now - state.lastNotificationPromptNoticeAt < NOTIFICATION_PROMPT_NOTICE_INTERVAL_MS) return; state.lastNotificationPromptNoticeAt = now; - notice("Enable approval notifications from the sidebar alert button.", "warning"); + notice("Enable 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) { +// Name the thread a notification is about, whichever kind it is waiting on. 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" says nothing about what is +// blocked. +function waitingNotificationLabel(meta, tid) { const fallback = String(tid).slice(0,8); if (!meta) return fallback; const title = meta.title || fallback; @@ -2469,7 +2486,7 @@ function approvalNotificationLabel(meta, tid) { return parent && parent.title ? `${title} (in ${parent.title})` : title; } -async function focusApprovalTarget(tid, approvalId) { +async function focusWaitingRequest(tid, requestId) { window.focus(); let meta = threadMetaForId(tid); if (!meta || !meta.pid) { @@ -2479,22 +2496,22 @@ async function focusApprovalTarget(tid, approvalId) { meta = threadMetaForId(tid); } if (!meta || !meta.pid) { - notice("Approval thread is not in the current project list.", "warning"); + notice("That thread is not in the current project list.", "warning"); return; } if (String(state.threadId) !== String(tid)) { - await openThread(meta.pid, tid, meta.title, { focusApprovalId:approvalId }); + await openThread(meta.pid, tid, meta.title, { focusRequestId:requestId }); } else { - state.pendingApprovalFocus = { + state.pendingWaitingFocus = { threadId:String(tid), - approvalId:String(approvalId), + requestId:String(requestId), attempts:0 }; - schedulePendingApprovalFocus(); + schedulePendingWaitingFocus(); } } -function notifyApprovalRequest(request, tid, opts) { +function notifyIncomingApproval(request, tid, opts) { opts = opts || {}; if (!request || !request.id || !tid) { recordNotificationDiagnostic("incoming_approval_skipped_invalid_request", { @@ -2503,7 +2520,7 @@ function notifyApprovalRequest(request, tid, opts) { }); return; } - maybeNotifyApproval(String(tid), { + maybeNotifyWaitingRequest(String(tid), { kind:"approval_requested", active_turn:true, approval_id:String(request.id), @@ -2528,7 +2545,7 @@ function isApprovalAnswered(request) { function handleIncomingApprovalRequest(request, tid, opts) { opts = opts || {}; // A reconnect snapshot replays already-answered approvals so their cards can be redrawn in the - // resolved state. Those must not re-arm the "needs approval" sidebar activity or fire a + // resolved state. Those must not re-arm the waiting-on-you sidebar activity or fire a // notification — the user already answered them. Render the resolved card and stop. if (isApprovalAnswered(request)) { renderApprovalRequest(request); @@ -2540,7 +2557,7 @@ function handleIncomingApprovalRequest(request, tid, opts) { source: opts.source || "unknown", notify: opts.notify !== false }); - if (opts.notify !== false) notifyApprovalRequest(request, tid, { source: opts.source }); + if (opts.notify !== false) notifyIncomingApproval(request, tid, { source: opts.source }); setThreadActivity(tid, { kind:"approval_requested", active_turn:true, @@ -2985,12 +3002,31 @@ function handleEvent(ev) { source: "agent_event_approval_requested" }); break; - case "server_request_received": - setActiveThreadActivity("server_request_received", true, "Waiting for input", { - server_request_id:ev.request && ev.request.id ? String(ev.request.id) : null + case "server_request_received": { + const serverRequestId = ev.request && ev.request.id ? String(ev.request.id) : null; + // A reconnect snapshot replays every ServerRequestReceived, answered ones included. Those must + // not re-arm the waiting-on-you activity or fire a notification — the user already answered + // them. Mirrors the approval path's guard in `handleIncomingApprovalRequest`; + // `renderServerRequest` settles the card into its resolved state from the same answered set. + if (serverRequestId && state.answeredServerRequests.has(serverRequestId)) { + renderServerRequest(ev.request); + break; + } + setActiveThreadActivity("server_request_received", true, "Waiting for your input", { + server_request_id:serverRequestId }); renderServerRequest(ev.request); + maybeNotifyWaitingRequest(String(ev.thread || state.threadId || ""), { + kind:"server_request_received", + active_turn:true, + approval_id:null, + server_request_id:serverRequestId, + summary:"Waiting for your input", + source:"agent_event_server_request_received", + unread:false + }); break; + } case "server_request_resolved": resolveServerRequest(ev.request_id); break; // Render errors as a persistent transcript entry (tied to the turn/message that caused them) // rather than a toast that vanishes — so looking back at a thread explains why a message got @@ -3058,7 +3094,7 @@ function renderLiveTurnSnapshot(snap) { }); } for (const request of (snap.pending_server_requests || [])) { - setActiveThreadActivity("server_request_received", true, "Waiting for input", { + setActiveThreadActivity("server_request_received", true, "Waiting for your input", { server_request_id:request && request.id ? String(request.id) : null }); renderServerRequest(request); @@ -3149,37 +3185,38 @@ function renderApprovalRequest(request) { addApprovalButton(actions, id, "decline", "Decline", "danger", available); addApprovalButton(actions, id, "cancel", "Cancel", "", available); body.append(actions); - schedulePendingApprovalFocus(); + schedulePendingWaitingFocus(); } -function approvalRowById(id) { +// A notification click targets whatever the thread is waiting for, so look in both card kinds. +function waitingRequestRowById(id) { if (!id) return null; const target = String(id); - return Array.from(document.querySelectorAll("[data-approval-id]")) - .find(el => String(el.dataset.approvalId) === target) || null; + return Array.from(document.querySelectorAll("[data-approval-id],[data-server-request-id]")) + .find(el => String(el.dataset.approvalId || el.dataset.serverRequestId) === target) || null; } -function schedulePendingApprovalFocus() { - const pending = state.pendingApprovalFocus; - if (!pending || !pending.approvalId) return; +function schedulePendingWaitingFocus() { + const pending = state.pendingWaitingFocus; + if (!pending || !pending.requestId) return; if (!state.threadId || String(state.threadId) !== String(pending.threadId)) return; - const row = approvalRowById(pending.approvalId); + const row = waitingRequestRowById(pending.requestId); if (row) { - state.pendingApprovalFocus = null; + state.pendingWaitingFocus = null; row.scrollIntoView({ block:"center", behavior:"smooth" }); - row.classList.add("approval-target"); + row.classList.add("waiting-target"); row.setAttribute("tabindex", "-1"); row.focus({ preventScroll:true }); - setTimeout(() => row.classList.remove("approval-target"), 5000); + setTimeout(() => row.classList.remove("waiting-target"), 5000); return; } pending.attempts = (pending.attempts || 0) + 1; if (pending.attempts > 40) { - state.pendingApprovalFocus = null; - notice("Approval is no longer pending.", "warning"); + state.pendingWaitingFocus = null; + notice("That request is no longer pending.", "warning"); return; } - setTimeout(schedulePendingApprovalFocus, 150); + setTimeout(schedulePendingWaitingFocus, 150); } function approvalTitle(request) { @@ -3278,11 +3315,11 @@ function resolveApprovalRequest(id, decision) { if (id === undefined || id === null || String(id) === "") return; id = String(id); const entry = state.pendingApprovals.get(id); - const msg = entry ? entry.msg : approvalRowById(id); + const msg = entry ? entry.msg : waitingRequestRowById(id); const tid = entry && entry.request && entry.request.thread_id ? entry.request.thread_id : (msg && msg.dataset.threadId ? msg.dataset.threadId : state.threadId); - closeApprovalNotification(tid, id); + closeWaitingNotification(tid, id); clearApprovalThreadActivity(tid, id); if (entry) { state.answeredApprovals.set(entry.stateKey || (msg && msg.dataset.approvalStateKey) || approvalStateKey(id), { @@ -3692,6 +3729,13 @@ function respondServerRequest(id, response, label) { status.className = "meta server-request-sent"; status.textContent = `Sent: ${label}`; if (body) body.append(status); + // Stop claiming the thread is waiting on the user the moment they act. An approval clears here + // because answering it broadcasts `ApprovalResolved`; a server request's resolved event comes from + // the harness on its own schedule and may never come, so waiting for it would leave the sidebar + // demanding attention for something already answered. + const tid = entry.msg.dataset.threadId || state.threadId; + clearServerRequestThreadActivity(tid, String(id)); + closeWaitingNotification(tid, String(id)); } function objectValue(value) { return value && typeof value === "object" && !Array.isArray(value) ? value : null; @@ -4544,54 +4588,55 @@ function subagentThreadsForActiveProject() { // 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` +// `running` and `waitingOnUser` are separate flags rather than reads of the winning activity: a +// request for the user 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 waitingOnUser = 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 (activityWaitsOnUser(other)) waitingOnUser = true; if (threadActivityRank(other) <= threadActivityRank(activity)) continue; activity = other; origin = otherId === key ? null : otherId; } - return { running, needsApproval, activity, origin }; + return { running, waitingOnUser, activity, origin }; } function subagentCounts() { const agents = subagentThreadsForActiveProject(); let running = 0; - let awaitingApproval = 0; + let waitingOnUser = 0; for (const thread of agents) { const subtree = subagentSubtreeState(thread.id); if (subtree.running) running += 1; - if (subtree.needsApproval) awaitingApproval += 1; + if (subtree.waitingOnUser) waitingOnUser += 1; } - return { total:agents.length, running, awaitingApproval }; + return { total:agents.length, running, waitingOnUser }; } function renderSubagentsButton() { const btn = $("subagentsBtn"); if (!btn) return; const counts = subagentCounts(); - const stateName = counts.awaitingApproval ? "approval" : (counts.running ? "running" : "idle"); + const stateName = counts.waitingOnUser ? "waiting" : (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.awaitingApproval - ? `${counts.awaitingApproval} sub-agent${counts.awaitingApproval === 1 ? "" : "s"} waiting for approval · ${runningLabel}` + ? (counts.waitingOnUser + ? `${counts.waitingOnUser} sub-agent${counts.waitingOnUser === 1 ? "" : "s"} waiting on you · ${runningLabel}` : runningLabel) : "No sub-agents"; - $("subagentsCount").textContent = String(counts.awaitingApproval || counts.running || counts.total); + $("subagentsCount").textContent = String(counts.waitingOnUser || counts.running || counts.total); if (!$("subagentsMenu").hidden) renderSubagentsMenu(); } @@ -4600,9 +4645,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 waitingSummary = counts.waitingOnUser ? `${counts.waitingOnUser} waiting on you · ` : ""; const summary = agents.length - ? `
${approvalSummary}${counts.running} running · ${counts.total} total
` + ? `
${waitingSummary}${counts.running} running · ${counts.total} total
` : ""; menu.innerHTML = `
@@ -4622,9 +4667,9 @@ function renderSubagentsMenu() { } function renderSubagentCard(thread) { - const { running, needsApproval, activity, origin } = subagentSubtreeState(thread.id); - const stateLabel = needsApproval - ? "Needs approval" + const { running, waitingOnUser, activity, origin } = subagentSubtreeState(thread.id); + const stateLabel = waitingOnUser + ? "Waiting on you" : (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"; @@ -4633,8 +4678,8 @@ function renderSubagentCard(thread) { : parentLabel; const active = state.threadId && String(state.threadId) === String(thread.id); const name = subagentDisplayName(thread); - const stateClass = needsApproval ? " approval" : (running ? " running" : ""); - return ` + title="Enable notifications when the agent needs you" aria-label="Enable notifications when the agent needs you">!
@@ -47,7 +47,7 @@

Giskard

+ title="Enable browser notifications when the agent needs an approval or an answer">Enable notifications
diff --git a/crates/giskard-server/static/sw.js b/crates/giskard-server/static/sw.js index 91803aa..e056120 100644 --- a/crates/giskard-server/static/sw.js +++ b/crates/giskard-server/static/sw.js @@ -4,8 +4,9 @@ // constructor — notifications must be shown through `ServiceWorkerRegistration.showNotification()`, // and their clicks are delivered here rather than to an `onclick` handler on the page. This worker // activates immediately, then on a notification click focuses an existing Giskard tab (opening one -// if none is around) and forwards the notification's `data` so the page can jump to the relevant -// approval. +// if none is around) and forwards the notification's `data` so the page can jump to whatever the +// agent is waiting on — an approval or a server request. The payload is opaque here: the page sets +// it and the page reads it back, so this worker never needs to know which kind it is. self.addEventListener("install", () => self.skipWaiting()); diff --git a/crates/giskard-server/tests/ui.rs b/crates/giskard-server/tests/ui.rs index 50d30a0..b86bdb4 100644 --- a/crates/giskard-server/tests/ui.rs +++ b/crates/giskard-server/tests/ui.rs @@ -126,8 +126,8 @@ async fn index_page_is_served_and_public() { body.contains("case \"approval_resolved\"") && body.contains("resolveApprovalRequest(msg.request_id, msg.decision)") && body.contains("function resolveApprovalRequest(id, decision)") - && body.contains("closeApprovalNotification(tid, id)") - && body.contains("const msg = entry ? entry.msg : approvalRowById(id)") + && body.contains("closeWaitingNotification(tid, id)") + && body.contains("const msg = entry ? entry.msg : waitingRequestRowById(id)") && body.contains("state.pendingApprovals.delete(id)"), "approval decisions broadcast from another tab resolve pending approval cards" ); @@ -650,19 +650,19 @@ async fn index_page_is_served_and_public() { 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("function waitingNotificationLabel(meta, tid)") + && body.contains("`Giskard: sub-agent ${needed} 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"), + && body.contains("if (activityWaitsOnUser(other)) waitingOnUser = true;") + && body.contains("\"Waiting on you\"") + && body.contains(".subagent-card.waiting") + && body.contains(".subagents-btn.state-waiting") + && body.contains(".subagent-card-state.waiting"), "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" ); @@ -678,11 +678,11 @@ async fn index_page_is_served_and_public() { body.contains("msg.type === \"thread_activity_bootstrap\"") && body.contains("function handleThreadActivityBootstrap(msg)") && body.contains("{ source:\"connect_bootstrap\" }") - && body.contains("bootstrapNotifiedApprovals:new Set()") + && body.contains("bootstrapNotifiedRequests:new Set()") && body.contains( - "if (activity.source === \"connect_bootstrap\" && state.bootstrapNotifiedApprovals.has(notificationKey))" + "if (activity.source === \"connect_bootstrap\" && state.bootstrapNotifiedRequests.has(notificationKey))" ) - && body.contains("function releaseApprovalNotificationClaim(notificationKey)"), + && body.contains("function releaseWaitingNotificationClaim(notificationKey)"), "activity the client missed while disconnected is replayed on connect and repaints the \ badge, but alerts at most once per page session — the 15s dedup window cannot answer \ \"have we ever alerted for this?\", so a resuming laptop would otherwise re-alert for the \ @@ -707,10 +707,10 @@ async fn index_page_is_served_and_public() { ); assert!( body.contains( - "const { running, needsApproval, activity, origin } = subagentSubtreeState(thread.id);" + "const { running, waitingOnUser, 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 \ + a card reading \"Waiting on you\" 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" ); @@ -2463,10 +2463,13 @@ fn browser_diagnostics_panel_is_exposed_from_settings() { assert!(body.contains("BROWSER_DIAGNOSTIC_LIMIT")); assert!(body.contains("function browserDiagnosticsSnapshot()")); assert!(body.contains("function notificationDebugSnapshot()")); - assert!(body.contains("last_approval_decision: lastNotificationDiagnostic")); - assert!(body.contains("recent_approval_decisions: recentNotificationDiagnostics")); - assert!(body.contains("function isApprovalNotificationDecision(entry)")); - assert!(body.contains("detail.kind === \"approval\"")); + assert!(body.contains("last_waiting_notification: lastNotificationDiagnostic")); + assert!(body.contains("recent_waiting_notifications: recentNotificationDiagnostics")); + assert!(body.contains("function isWaitingNotificationDiagnostic(entry)")); + // One prefix test, not an enumeration: a per-reason list silently omits whatever reason is + // added next, which is how several waiting diagnostics fell out of the panel. + assert!(body.contains("return reason.startsWith(\"waiting_notify_\") ||")); + assert!(body.contains("detail.kind === \"waiting_request\"")); assert!(body.contains("function lastNotificationDiagnostic(predicate)")); assert!(body.contains("function recentNotificationDiagnostics(predicate, limit)")); assert!(body.contains("function recordBrowserDiagnostic(category, reason, detail)")); @@ -2475,12 +2478,12 @@ fn browser_diagnostics_panel_is_exposed_from_settings() { assert!(body.contains("recordBrowserDiagnostic(\"notification\", reason, detail);")); assert!(body.contains("function renderBrowserDiagnosticsPanel(snapshot, reveal)")); assert!(body.contains("log.textContent = lines.join(\"\\n\");")); - assert!(body.contains("`lastApproval: ${lastApproval ? lastApproval.reason : \"none\"}`")); + assert!(body.contains("`lastRequest: ${lastWaiting ? lastWaiting.reason : \"none\"}`")); assert!(body.contains("`dedupMs: ${snapshot.dedup_window_ms}`")); - assert!(body.contains("recentApprovals:")); + assert!(body.contains("recentRequests:")); assert!(body.contains("recentBrowserEvents:")); assert!(body.contains("visible=${entry.visibility} focused=${entry.focused}")); - assert!(body.contains("`approvalSource: ${approvalDetail.source || \"none\"}`")); + assert!(body.contains("`requestSource: ${waitingDetail.source || \"none\"}`")); assert!(body.contains("window.giskardBrowserDiagnostics = browserDiagnosticsSnapshot;")); assert!(body.contains("window.giskardNotificationDebug = notificationDebugSnapshot;")); assert!(body.contains("function showBrowserDiagnostics()")); @@ -2514,20 +2517,24 @@ fn sidebar_activity_notifications_target_approval_rows() { let body = app_js(); assert!(body.contains("threadActivity:new Map()")); - assert!(body.contains("pendingApprovalFocus:null")); - assert!(body.contains("notifiedApprovals:new Map()")); - assert!(body.contains("approvalNotifications:new Map()")); + assert!(body.contains("pendingWaitingFocus:null")); + assert!(body.contains("notifiedRequests:new Map()")); + assert!(body.contains("waitingNotifications:new Map()")); assert!(body.contains("lastNotificationPromptNoticeAt:0")); 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, hosts)")); assert!(body.contains("activity.kind === \"approval_requested\"")); - assert!(body.contains( - "if (activity.kind === \"approval_requested\") maybeNotifyApproval(tid, activity);" - )); + assert!( + body.contains( + "if (activityWaitsOnUser(activity)) maybeNotifyWaitingRequest(tid, activity);" + ) + ); assert!(body.contains("server_request_id: msg.server_request_id || null")); - assert!(body.contains("activity.active_turn && activity.kind !== \"approval_requested\"")); + // "Running" is now the complement of "waiting on the user", not of "approval" specifically — + // a thread waiting on a server request must not also read as merely running. + assert!(body.contains("activity.active_turn && !activityWaitsOnUser(activity)")); assert!(body.contains("else if (activity.active_turn) status.textContent = \"o\"")); assert!(!body.contains("else if (activity.active_turn) status.textContent = \">\"")); assert!(body.contains("function setActiveThreadActivity(kind, activeTurn, summary, extra)")); @@ -2548,65 +2555,61 @@ fn sidebar_activity_notifications_target_approval_rows() { assert!(body.contains("await Notification.requestPermission()")); assert!(body.contains("Browser notifications require HTTPS or localhost.")); assert!(body.contains("function refreshNotificationButton()")); - assert!(body.contains("label = \"Approval notifications enabled\"")); + assert!(body.contains("label = \"Notifications enabled\"")); assert!(body.contains("label = \"Notifications blocked by browser\"")); assert!(body.contains("label = \"Notifications require HTTPS or localhost\"")); assert!(body.contains("function maybeNoticeNotificationPermission()")); assert!(body.contains("Notification.permission !== \"default\"")); - assert!(body.contains("Enable approval notifications from the sidebar alert button.")); + assert!(body.contains("Enable notifications from the sidebar alert button.")); assert!(body.contains( - "const notificationKey = approvalNotificationKey(tid, activity && activity.approval_id);" + "const notificationKey = waitingNotificationKey(tid, waitingRequestId(activity));" )); assert!(body.contains("pruneNotificationDedup(now);")); - assert!(body.contains("const notifiedAt = state.notifiedApprovals.get(notificationKey);")); + assert!(body.contains("const notifiedAt = state.notifiedRequests.get(notificationKey);")); assert!(body.contains("now - notifiedAt < NOTIFICATION_DEDUP_MS")); - assert!(body.contains("state.notifiedApprovals.set(notificationKey, now);")); + assert!(body.contains("state.notifiedRequests.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 state.bootstrapNotifiedApprovals.add(notificationKey);\n // A sub-agent's very first approval" + " state.notifiedRequests.set(notificationKey, now);\n state.bootstrapNotifiedRequests.add(notificationKey);\n // A sub-agent's very first approval" )); // Both paths that give up without showing anything must release the claim, or the approval is // silenced for the rest of the page session. assert!( - body.matches("releaseApprovalNotificationClaim(notificationKey);") + body.matches("releaseWaitingNotificationClaim(notificationKey);") .count() >= 2 ); - assert!(body.contains("trackApprovalNotification(notificationKey, result.notification);")); + assert!(body.contains("trackWaitingNotification(notificationKey, result.notification);")); assert!(body.contains("function pruneNotificationDedup(now)")); - assert!(body.contains("function approvalNotificationKey(tid, approvalId)")); - assert!(body.contains("const approvalKey = approvalId === undefined || approvalId === null")); + assert!(body.contains("function waitingNotificationKey(tid, requestId)")); + assert!(body.contains("const requestKey = requestId === undefined || requestId === null")); assert!(body.contains("String(approvalId)")); - assert!(body.contains("if (!threadKey || !approvalKey) return \"\";")); - assert!(body.contains("return `${threadKey}:${approvalKey}`;")); - assert!(body.contains("function trackApprovalNotification(key, notification)")); + assert!(body.contains("if (!threadKey || !requestKey) return \"\";")); + assert!(body.contains("return `${threadKey}:${requestKey}`;")); + assert!(body.contains("function trackWaitingNotification(key, notification)")); assert!(body.contains("notifications = new Set();")); assert!(body.contains("notifications.add(notification);")); - assert!(body.contains("function untrackApprovalNotification(key, notification)")); + assert!(body.contains("function untrackWaitingNotification(key, notification)")); assert!(body.contains("notifications.delete(notification);")); - assert!( - body.contains("if (notifications.size === 0) state.approvalNotifications.delete(key);") - ); - assert!(body.contains("function closeApprovalNotification(tid, approvalId)")); - assert!(body.contains("const notifications = state.approvalNotifications.get(key);")); - assert!(body.contains("state.approvalNotifications.delete(key);")); + assert!(body.contains("if (notifications.size === 0) state.waitingNotifications.delete(key);")); + assert!(body.contains("function closeWaitingNotification(tid, requestId)")); + assert!(body.contains("const notifications = state.waitingNotifications.get(key);")); + assert!(body.contains("state.waitingNotifications.delete(key);")); assert!(body.contains("for (const notification of notifications)")); - assert!(body.contains("approval_notification_closed")); + assert!(body.contains("waiting_notify_closed")); assert!(!body.contains("closeAllApprovalNotifications")); - assert!( - body.contains( - "const notificationTag = approvalNotificationTag(tid, activity.approval_id);" - ) - ); - assert!(body.contains("return `giskard-approval-${tid}-${approvalId}`;")); + assert!(body.contains( + "const notificationTag = waitingNotificationTag(tid, waitingRequestId(activity));" + )); + assert!(body.contains("return `giskard-waiting-${tid}-${requestId}`;")); assert!(body.contains("tag: notificationTag")); - assert!(body.contains("function notifyApprovalRequest(request, tid, opts)")); + assert!(body.contains("function notifyIncomingApproval(request, tid, opts)")); assert!(body.contains("function handleIncomingApprovalRequest(request, tid, opts)")); assert!(body.contains( - "if (opts.notify !== false) notifyApprovalRequest(request, tid, { source: opts.source });" + "if (opts.notify !== false) notifyIncomingApproval(request, tid, { source: opts.source });" )); assert!(body.contains("source: \"agent_event_approval_requested\"")); assert!(body.contains("source: \"server_message_approval_request\"")); @@ -2614,10 +2617,10 @@ fn sidebar_activity_notifications_target_approval_rows() { // Live events default to "thread_activity"; a connect replay overrides it so the notification // gate can tell the two apart. assert!(body.contains("source: msg.source || \"thread_activity\"")); - assert!(body.contains("approval_notify_received")); - assert!(body.contains("approval_notify_suppressed_visible_current_thread")); - assert!(body.contains("approval_notify_constructor_failed")); - assert!(body.contains("approval_notify_created")); + assert!(body.contains("waiting_notify_received")); + assert!(body.contains("waiting_notify_suppressed_visible_current_thread")); + assert!(body.contains("waiting_notify_constructor_failed")); + assert!(body.contains("waiting_notify_created")); assert!(body.contains("async function showAppNotification(title, options, diagnosticDetail)")); // Prefer the service worker (works on Android); fall back to the Notification constructor. assert!(body.contains("const reg = await notificationRegistration();")); @@ -2630,36 +2633,34 @@ fn sidebar_activity_notifications_target_approval_rows() { assert!(body.contains("notification.onshow = () => recordNotificationDiagnostic")); assert!(body.contains("notification.onerror = () => recordNotificationDiagnostic")); assert!(body.contains("notification.onclose = () => {")); - assert!(body.contains("diagnosticDetail.kind === \"approval\"")); + assert!(body.contains("diagnosticDetail.kind === \"waiting_request\"")); assert!( - body.contains( - "approvalNotificationKey(diagnosticDetail.tid, diagnosticDetail.approval_id)," - ) + body.contains("waitingNotificationKey(diagnosticDetail.tid, diagnosticDetail.request_id),") ); - assert!(body.contains("untrackApprovalNotification(")); + assert!(body.contains("untrackWaitingNotification(")); assert!(body.contains( "recordNotificationDiagnostic(\"browser_notification_close\", diagnosticDetail);" )); - assert!(body.contains("function isApprovalNotificationDecision(entry)")); - assert!(body.contains("detail.kind === \"approval\"")); - assert!(body.contains("last_approval_decision: lastNotificationDiagnostic")); - assert!(body.contains("recent_approval_decisions: recentNotificationDiagnostics")); - assert!(body.contains("`lastApproval: ${lastApproval ? lastApproval.reason : \"none\"}`")); + assert!(body.contains("function isWaitingNotificationDiagnostic(entry)")); + assert!(body.contains("detail.kind === \"waiting_request\"")); + assert!(body.contains("last_waiting_notification: lastNotificationDiagnostic")); + assert!(body.contains("recent_waiting_notifications: recentNotificationDiagnostics")); + assert!(body.contains("`lastRequest: ${lastWaiting ? lastWaiting.reason : \"none\"}`")); assert!(body.contains("function sendTestNotification()")); assert!(body.contains("Giskard test notification")); assert!(body.contains("test_notify_created")); assert!(body.contains("test_notify_constructor_failed")); assert!(body.contains("requireInteraction: true")); - assert!(!body.contains("approval_notify_skipped_invalid_activity")); - assert!(body.contains("approval_notify_skipped_invalid_call")); - assert!(body.contains("`lastApproval: ${lastApproval ? lastApproval.reason : \"none\"}`")); - assert!(!body.contains("notifyApprovalRequest(ev.request")); - assert!(!body.contains("notifyApprovalRequest(msg.request")); - assert!(!body.contains("notifyApprovalRequest(snap.pending_approval")); + assert!(!body.contains("waiting_notify_skipped_invalid_activity")); + assert!(body.contains("waiting_notify_skipped_invalid_call")); + assert!(body.contains("`lastRequest: ${lastWaiting ? lastWaiting.reason : \"none\"}`")); + assert!(!body.contains("notifyIncomingApproval(ev.request")); + assert!(!body.contains("notifyIncomingApproval(msg.request")); + assert!(!body.contains("notifyIncomingApproval(snap.pending_approval")); // 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\"" + "const needed = activity.kind === \"approval_requested\" ? \"approval\" : \"input\";" )); assert!(body.contains("const focused = document.hasFocus ? document.hasFocus() : true;")); assert!(body.contains( @@ -2667,19 +2668,19 @@ fn sidebar_activity_notifications_target_approval_rows() { )); // Desktop notifications wire their click; service-worker clicks arrive via the worker postMessage. assert!(body.contains( - "result.notification.onclick = () => handleNotificationClick({ threadId: tid, approvalId: activity.approval_id });" + "result.notification.onclick = () => handleNotificationClick({ threadId: tid, requestId: waitingRequestId(activity) });" )); - assert!(body.contains("closeApprovalNotification(data.threadId, data.approvalId);")); - assert!(body.contains("openThread(meta.pid, tid, meta.title, { focusApprovalId:approvalId })")); - assert!(body.contains("function approvalRowById(id)")); + assert!(body.contains("closeWaitingNotification(data.threadId, data.requestId);")); + assert!(body.contains("openThread(meta.pid, tid, meta.title, { focusRequestId:requestId })")); + assert!(body.contains("function waitingRequestRowById(id)")); assert!(body.contains("row.scrollIntoView({ block:\"center\", behavior:\"smooth\" });")); - assert!(body.contains("row.classList.add(\"approval-target\")")); + assert!(body.contains("row.classList.add(\"waiting-target\")")); let index = include_str!("../static/index.html"); assert!(index.contains("id=\"notifyTopBtn\"")); assert!(index.contains("class=\"notify-permission-btn\"")); assert!(index.contains("id=\"notifyBtn\"")); - assert!(index.contains("Enable approval notifications")); + assert!(index.contains("Enable notifications")); let css = include_str!("../static/app.css"); assert!(css.contains(".sidebar-head")); @@ -2717,7 +2718,7 @@ fn browser_uses_service_worker_for_android_notifications() { // Resolution closes service-worker notifications by tag (no Notification object is held). let close = between( body, - "function closeApprovalNotification(tid, approvalId) {", + "function closeWaitingNotification(tid, requestId) {", "function maybeNoticeNotificationPermission()", ); assert!(close.contains("reg.getNotifications({ tag })")); diff --git a/docs/subagents.md b/docs/subagents.md index 9cbbf1a..8cedd06 100644 --- a/docs/subagents.md +++ b/docs/subagents.md @@ -96,10 +96,12 @@ report it: 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. +- **The header Sub-agents monitor.** Being asked for something also marks the turn active, so + "waiting on the user" is tracked separately from "running": the button takes a distinct state and + the child's card reads `Waiting on you`. That covers approvals *and* server requests — Codex + splits those, and already blurs the split itself by delivering MCP tool approvals as + `requestUserInput`, but to the person looking at the sidebar they are one state. 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. diff --git a/specs/giskard-specification.md b/specs/giskard-specification.md index 9bd121f..8159164 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.59 +**Version:** 1.60 > **Amendment — frontend approach (supersedes the Dioxus/WASM design below).** > This document was written targeting a **Dioxus fullstack / WebAssembly** frontend (`giskard-ui`), @@ -23,6 +23,21 @@ > below as historical design context, not a current requirement. The wire contract (`giskard-proto`) > and all backend design remain authoritative. +**Changelog (1.59 → 1.60), one "waiting on the user" state:** +- **SR8:** An approval and a server request both block a turn until the user answers, and the + browser must present them as one state. Previously only approvals were ranked and rendered as + blocked; a thread waiting on a server request fell through to the generic active-turn branch and + was indistinguishable from one that was merely busy, and fired no notification. Codex itself + already blurs the split — MCP tool approvals arrive as `requestUserInput` and are promoted to + approval cards — so the distinction is a protocol artifact, not something to surface. Activity + ranking, the sidebar glyph, the sub-agent monitor, and notifications now key off "waits on the + user" rather than "is an approval". Card rendering stays per-kind: a decision has fixed choices, a + server request has a per-method form. +- **SR9:** The waiting state must clear as soon as the user acts, not when the harness confirms. + Answering an approval broadcasts `ApprovalResolved`, but a server request's resolved event comes + from the harness on its own schedule and may never come, so the browser clears its own waiting + state on send. + **Changelog (1.58 → 1.59), answered server requests survive a reload:** - **SR6:** Answering a server request recorded nothing server-side. A request leaves the pending set only when the harness emits its resolved event, which arrives on the harness's own schedule and diff --git a/tests/e2e/tests/approvals.spec.ts b/tests/e2e/tests/approvals.spec.ts index d133a99..013bb21 100644 --- a/tests/e2e/tests/approvals.spec.ts +++ b/tests/e2e/tests/approvals.spec.ts @@ -23,6 +23,13 @@ test.describe("approval persistence across reload", () => { const approval = transcript.locator(".msg.approval"); await expect(approval).toBeVisible(); + // Hold on to this thread's row: `activity-waiting` now covers server requests too, so a + // page-wide assertion would fail on any unrelated thread that happens to be waiting. + const row = page.locator(".thread.active"); + await expect(row).toHaveCount(1); + const tid = await row.getAttribute("data-tid"); + expect(tid).toBeTruthy(); + // The card is actionable before it is answered. const acceptBtn = approval.getByRole("button", { name: "Accept", exact: true }); await expect(acceptBtn).toBeVisible(); @@ -53,8 +60,8 @@ test.describe("approval persistence across reload", () => { ).toHaveCount(0); await expect(page.locator("#transcript .msg.error")).toHaveCount(0); - // The sidebar must not nag that the thread still needs an approval: an answered approval - // replayed from the snapshot must not re-arm the "approval needed" thread indicator. - await expect(page.locator(".thread.activity-approval")).toHaveCount(0); + // The sidebar must not nag that this thread still needs an approval: an answered approval + // replayed from the snapshot must not re-arm its waiting indicator. + await expect(page.locator(`.thread[data-tid="${tid}"]`)).not.toHaveClass(/\bactivity-waiting\b/); }); }); diff --git a/tests/e2e/tests/helpers.ts b/tests/e2e/tests/helpers.ts index cbcd133..0390d45 100644 --- a/tests/e2e/tests/helpers.ts +++ b/tests/e2e/tests/helpers.ts @@ -28,6 +28,7 @@ export const SCRIPTED_APPROVAL_TRIGGER = "Trigger a scripted approval request."; */ export const SCRIPTED_SERVER_REQUEST_TRIGGER = "Trigger a scripted user input request."; export const SCRIPTED_SERVER_REQUEST_QUESTION = "Which branch should I use?"; +export const SCRIPTED_SERVER_REQUEST_ID = "scripted-server-request-1"; /** * Prompt that spawns a linked child which raises an approval and then holds its turn open, while the @@ -44,7 +45,8 @@ export type RecordedNotification = { title: string; body: string; tag: string; - data: { threadId?: string; approvalId?: string } | null; + /** `requestId` is whatever the thread is waiting on — an approval or a server request. */ + data: { threadId?: string; requestId?: string } | null; }; declare global { diff --git a/tests/e2e/tests/server-requests.spec.ts b/tests/e2e/tests/server-requests.spec.ts index d54938e..db9def5 100644 --- a/tests/e2e/tests/server-requests.spec.ts +++ b/tests/e2e/tests/server-requests.spec.ts @@ -1,8 +1,11 @@ import { test, expect } from "@playwright/test"; import { + SCRIPTED_SERVER_REQUEST_ID, SCRIPTED_SERVER_REQUEST_QUESTION, SCRIPTED_SERVER_REQUEST_TRIGGER, login, + recordedNotifications, + stubNotifications, } from "./helpers"; // Server requests (`requestUserInput` and friends) had no browser coverage at all, despite having @@ -15,6 +18,7 @@ import { // errors. The scripted harness deliberately never resolves, so this is exactly that window. test.describe("server requests", () => { test.beforeEach(async ({ page }) => { + await stubNotifications(page); await login(page); }); @@ -63,10 +67,105 @@ test.describe("server requests", () => { // longer claim to be waiting on the user for input they already gave. await expect(page.locator(".thread.active .thread-status")).not.toHaveAttribute( "title", - /Waiting for input/, + /Waiting for your input/, ); }); + // A server request blocks the turn on the user exactly as an approval does, so it must read the + // same way in the sidebar. Before this it fell through to the generic "active turn" branch and + // rendered as `o` — indistinguishable from a thread that was merely busy. + test("a thread waiting for input reads as waiting, not merely running", async ({ page }) => { + const project = page.locator(".proj", { hasText: "Demo" }); + await project.locator(".project-add").click(); + await page.locator("#input").fill(SCRIPTED_SERVER_REQUEST_TRIGGER); + await page.locator("#sendBtn").click(); + + const request = page.locator("#transcript .msg.server-request"); + await expect(request).toBeVisible(); + + const row = page.locator(".thread.active"); + await expect(row).toHaveClass(/\bactivity-waiting\b/); + await expect(row).not.toHaveClass(/\bactivity-running\b/); + const status = row.locator(".thread-status"); + await expect(status).toHaveText("!"); + await expect(status).toHaveAttribute("title", /Waiting for your input/); + + // Answering hands the turn back to the agent, so the row drops to plain running. + await request.locator("select.server-request-answer").selectOption("main"); + await request.getByRole("button", { name: "Continue", exact: true }).click(); + await expect(request.locator(".server-request-sent")).toBeVisible(); + await expect(row).not.toHaveClass(/\bactivity-waiting\b/); + await expect(status).not.toHaveText("!"); + }); + + // The two kinds share the waiting state but not the copy: telling someone an approval is needed + // when the agent asked them a question sends them looking for a decision that does not exist. + test("notifies about input rather than approval", async ({ page }) => { + const project = page.locator(".proj", { hasText: "Demo" }); + await project.locator(".project-add").click(); + await page.locator("#input").fill(SCRIPTED_SERVER_REQUEST_TRIGGER); + await page.locator("#sendBtn").click(); + await expect(page.locator("#transcript .msg.server-request")).toBeVisible(); + + // The current thread is visible and focused, so the live event suppresses its own notification; + // drive the shared entry point directly to inspect the headline it would produce. + const title = await page.evaluate(async () => { + const app = window as unknown as { + maybeNotifyWaitingRequest: (tid: string, activity: Record) => Promise; + }; + await app.maybeNotifyWaitingRequest("some-other-thread", { + kind: "server_request_received", + active_turn: true, + approval_id: null, + server_request_id: "sr-headline-probe", + summary: "Waiting for your input", + source: "test", + unread: true, + }); + const calls = (window as Window).__giskardNotifications ?? []; + return calls[calls.length - 1]?.title ?? null; + }); + expect(title).toBe("Giskard: input needed"); + }); + + // The reconnect snapshot replays every ServerRequestReceived, answered ones included. An answered + // one must not re-alert: the user already dealt with it. The approval path has always guarded + // this; without the same guard here a backgrounded tab gets pinged for work already done. + test("an answered request does not re-notify on reconnect", async ({ page }) => { + // Notifications are suppressed while the page is visible AND focused AND on that thread, which + // would mask the bug. Unfocus so the reconnect path is actually allowed to alert. + await page.addInitScript(() => { document.hasFocus = () => false; }); + + const project = page.locator(".proj", { hasText: "Demo" }); + await project.locator(".project-add").click(); + await page.locator("#input").fill(SCRIPTED_SERVER_REQUEST_TRIGGER); + await page.locator("#sendBtn").click(); + const request = page.locator("#transcript .msg.server-request"); + await expect(request).toBeVisible(); + const tid = await page.locator(".thread.active").getAttribute("data-tid"); + expect(tid).toBeTruthy(); + + await request.locator("select.server-request-answer").selectOption("main"); + await request.getByRole("button", { name: "Continue", exact: true }).click(); + await expect(request.locator(".server-request-sent")).toBeVisible(); + + // The scripted harness never resolves, so the answer is known only from the server's record. + await page.reload(); + await expect(page.locator("#app")).toHaveClass(/open/); + await expect(page.locator("#transcript .msg.server-request")).toHaveClass(/\bresolved\b/); + + await page.waitForTimeout(1000); + // Scope to this test's thread. The replay server is shared and earlier tests leave outstanding + // requests behind, all reusing the same scripted request id — the connect bootstrap replays + // those too, so filtering by id alone would count another test's leftovers. + const notifications = await recordedNotifications(page); + expect( + notifications.filter( + (n) => n.data?.requestId === SCRIPTED_SERVER_REQUEST_ID && n.data?.threadId === tid, + ), + ).toEqual([]); + }); + test("an unanswered request survives a reload as actionable", async ({ page }) => { const project = page.locator(".proj", { hasText: "Demo" }); await project.locator(".project-add").click(); diff --git a/tests/e2e/tests/subagent-approvals.spec.ts b/tests/e2e/tests/subagent-approvals.spec.ts index 899083b..8c85570 100644 --- a/tests/e2e/tests/subagent-approvals.spec.ts +++ b/tests/e2e/tests/subagent-approvals.spec.ts @@ -37,7 +37,7 @@ function childThreadId(page: Page, pid: string, parentTid: string): Promise { const notifications = await recordedNotifications(page); return notifications.filter( - (n) => n.data?.approvalId === SCRIPTED_SUBAGENT_APPROVAL_ID && n.data?.threadId === childThread, + (n) => n.data?.requestId === SCRIPTED_SUBAGENT_APPROVAL_ID && n.data?.threadId === childThread, ).length; } @@ -71,7 +71,7 @@ test.describe("sub-agent blocked on an approval", () => { // 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-waiting\b/); await expect(parentRow).toHaveClass(/\bactivity-subagent\b/); const status = parentRow.locator(".thread-status"); await expect(status).toHaveText("!"); @@ -88,22 +88,22 @@ test.describe("sub-agent blocked on an approval", () => { // 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 expect(subagentsBtn).toHaveClass(/\bstate-waiting\b/); + await expect(subagentsBtn).toHaveAttribute("title", /waiting on you/); 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(card).toHaveClass(/\bwaiting\b/); + await expect(card.locator(".subagent-card-state")).toHaveText("Waiting on you"); await expect(page.locator("#subagentsMenu .subagents-summary")).toHaveText( - /1 waiting for approval/, + /1 waiting on you/, ); 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 && n.data?.threadId === child, + (n) => n.data?.requestId === SCRIPTED_SUBAGENT_APPROVAL_ID && n.data?.threadId === child, ); expect(approval).toBeTruthy(); expect(approval!.title).toBe("Giskard: sub-agent approval needed"); @@ -115,7 +115,7 @@ test.describe("sub-agent blocked on an approval", () => { 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/); + await expect(parentRow).toHaveClass(/\bactivity-waiting\b/); const child = await childThreadId(page, parent.pid, parent.tid); expect(child).toBeTruthy(); @@ -124,8 +124,8 @@ test.describe("sub-agent blocked on an approval", () => { await page.evaluate( ({ child, approvalId }) => (window as unknown as { - handleNotificationClick: (data: { threadId: string; approvalId: string }) => void; - }).handleNotificationClick({ threadId: child, approvalId }), + handleNotificationClick: (data: { threadId: string; requestId: string }) => void; + }).handleNotificationClick({ threadId: child, requestId: approvalId }), { child: child!, approvalId: SCRIPTED_SUBAGENT_APPROVAL_ID }, ); @@ -147,8 +147,8 @@ test.describe("sub-agent blocked on an approval", () => { // 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/); + await expect(parentRow).not.toHaveClass(/\bactivity-waiting\b/); + await expect(page.locator("#subagentsBtn")).not.toHaveClass(/\bstate-waiting\b/); }); // The hoisting rules decide which of several states one row shows. Drive them through the app's @@ -225,7 +225,7 @@ test.describe("sub-agent blocked on an approval", () => { 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-waiting"); 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"); @@ -233,7 +233,7 @@ test.describe("sub-agent blocked on an approval", () => { 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-waiting"); expect(result.afterAnswer.rowClass).not.toContain("activity-subagent"); expect(result.cycle.host).toBeNull(); @@ -247,7 +247,7 @@ test.describe("sub-agent blocked on an approval", () => { test("a reload learns about an approval it was not connected for", 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/); + await expect(parentRow).toHaveClass(/\bactivity-waiting\b/); const child = await childThreadId(page, parent.pid, parent.tid); expect(child).toBeTruthy(); @@ -257,7 +257,7 @@ test.describe("sub-agent blocked on an approval", () => { // Rebuilt from nothing, the sidebar still reports the blocked descendant. const reloadedRow = page.locator(`.thread[data-tid="${parent.tid}"]`); - await expect(reloadedRow).toHaveClass(/\bactivity-approval\b/); + await expect(reloadedRow).toHaveClass(/\bactivity-waiting\b/); await expect(reloadedRow).toHaveClass(/\bactivity-subagent\b/); await expect(reloadedRow.locator(".thread-status")).toHaveText("!"); // A reload is a new page session, so the alert fires again rather than being suppressed. @@ -307,7 +307,7 @@ test.describe("sub-agent blocked on an approval", () => { 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/, + /\bactivity-waiting\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