+ 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