From ddaf25cc38de5fd08f6a4ca43bf89f4358fa26f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 14:02:18 +0000 Subject: [PATCH] Keep an answered server request answered across a reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answering a server request recorded nothing on the server. A request leaves the pending set only when the harness emits its own resolved event, which arrives on the harness's schedule and may never arrive at all. Until then the replayed ServerRequestReceived still reads as outstanding, so a reload rendered the card actionable again — and answering a second time routes a stale id to the harness, which errors. That is the same defect already fixed for approvals, left open on the path beside it. The answer is now recorded against the in-flight turn the moment it is routed, and the reconnect snapshot carries answered_server_requests. Excluding them from pending_server_requests is not enough on its own: the request still rides along in `accumulated`, and replaying that renders an actionable card unless the client is told it was answered. The client seeds the answered set before replaying, exactly as it does for approvals. Answered requests are also excluded from the connect bootstrap, so a latecomer is not told a thread is waiting on input the user already gave. Server requests had no browser coverage at all despite having real UI — a card, per-question controls, Continue/Cancel and a resolved state. They do now, driven by a scripted user-input request whose harness deliberately never resolves it, which is precisely the window in question. Two tests: an answered request comes back resolved and never actionable, and the mirror image, an unanswered one comes back still actionable rather than being swallowed. The first was confirmed to fail without this fix. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NhKuYcqLiesiQEDJJ1gBCY --- crates/giskard-proto/src/lib.rs | 11 ++- .../src/bin/giskard-server-replay.rs | 41 +++++++++ crates/giskard-server/src/live_buffer.rs | 46 +++++++++- crates/giskard-server/src/registry.rs | 7 +- crates/giskard-server/src/routes.rs | 19 +++- crates/giskard-server/static/app.js | 13 ++- .../giskard-server/tests/server_requests.rs | 77 ++++++++++++++++ crates/giskard-server/tests/ui.rs | 8 ++ specs/giskard-specification.md | 27 +++++- tests/e2e/README.md | 10 ++- tests/e2e/tests/helpers.ts | 9 ++ tests/e2e/tests/server-requests.spec.ts | 90 +++++++++++++++++++ 12 files changed, 342 insertions(+), 16 deletions(-) create mode 100644 tests/e2e/tests/server-requests.spec.ts diff --git a/crates/giskard-proto/src/lib.rs b/crates/giskard-proto/src/lib.rs index 5ec80eb..fa191a8 100644 --- a/crates/giskard-proto/src/lib.rs +++ b/crates/giskard-proto/src/lib.rs @@ -24,7 +24,7 @@ pub use giskard_core::approval::{ pub use giskard_core::diff::{DiffHunk, DiffLine}; pub use giskard_core::error::HarnessError; pub use giskard_core::event::AgentEvent; -pub use giskard_core::ids::{ApprovalId, ItemId}; +pub use giskard_core::ids::{ApprovalId, ItemId, ServerRequestId}; pub use giskard_core::item::{ CommandExecutionStart, FileChangeEntry, FileChangeKind, ItemDelta, ItemKind, ItemStart, SubagentAction, SubagentLink, SubagentStatus, @@ -161,6 +161,13 @@ pub struct LiveTurnSnapshot { /// cards in their resolved state instead of re-prompting. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub answered_approvals: Vec, + /// Server requests the user already answered during this in-flight turn. + /// + /// A harness emits its own resolved event for these, but on its own schedule and not + /// guaranteed at all. Until that lands the request looks outstanding in the replayed events, so + /// a reload would render it actionable again and re-answering routes a stale id to the harness. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub answered_server_requests: Vec, } /// An approval the user resolved during an in-flight turn (part of [`LiveTurnSnapshot`]). @@ -988,6 +995,7 @@ mod tests { received_at: chrono::Utc::now(), }], answered_approvals: vec![], + answered_server_requests: vec![], }; let json = serde_json::to_value(&snapshot).unwrap(); assert_eq!(json["thread_id"], tid.to_string()); @@ -1011,6 +1019,7 @@ mod tests { request_id: ApprovalId("ap_1".into()), decision: ApprovalDecision::Accept, }], + answered_server_requests: vec![ServerRequestId("req_1".into())], }; let json = serde_json::to_value(&snapshot).unwrap(); assert_eq!(json["answered_approvals"][0]["request_id"], "ap_1"); diff --git a/crates/giskard-server/src/bin/giskard-server-replay.rs b/crates/giskard-server/src/bin/giskard-server-replay.rs index 580db04..bb13657 100644 --- a/crates/giskard-server/src/bin/giskard-server-replay.rs +++ b/crates/giskard-server/src/bin/giskard-server-replay.rs @@ -70,6 +70,12 @@ const SCRIPTED_SUBAGENT_APPROVAL_DELAY: std::time::Duration = /// answered card is not re-surfaced as actionable. The approval id is fixed so tests can target it. const SCRIPTED_APPROVAL_TRIGGER: &str = "Trigger a scripted approval request."; const SCRIPTED_APPROVAL_ID: &str = "scripted-approval-1"; +/// Prompt that raises a `requestUserInput` server request and then keeps the turn in-flight. This +/// harness deliberately never emits `ServerRequestResolved` when the answer is routed — modelling a +/// harness whose resolved event is late or absent, which is the window a reload has to survive. +const SCRIPTED_SERVER_REQUEST_TRIGGER: &str = "Trigger a scripted user input request."; +const SCRIPTED_SERVER_REQUEST_ID: &str = "scripted-server-request-1"; +const SCRIPTED_SERVER_REQUEST_QUESTION: &str = "Which branch should I use?"; /// How long a scripted turn waits for the server's event forwarder to subscribe before giving up. const RECEIVER_WAIT_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(2); const RECEIVER_POLL_INTERVAL: std::time::Duration = std::time::Duration::from_millis(10); @@ -424,10 +430,45 @@ impl AgentHarness for ScriptedHarness { .insert(ApprovalId(SCRIPTED_APPROVAL_ID.into()), (thread_id, turn)); } + let raise_server_request = input_text == Some(SCRIPTED_SERVER_REQUEST_TRIGGER); + // Stream the canned reply the way a real harness would: start, incremental deltas, then a // completed item and a turn-completed with token usage. Emitted off-task with yields so the // WebSocket layer observes distinct frames (the transcript renders progressively). tokio::spawn(async move { + if raise_server_request { + // Raise a user-input request and leave the turn in-flight. Answering it routes a + // response to `respond_server_request`, which deliberately stays silent: a browser + // reload must still render the card resolved, from the server's recorded answer + // rather than from a harness resolved event that never comes. + let _ = sender.send(AgentEvent::TurnStarted { + thread: thread_id, + turn, + }); + tokio::task::yield_now().await; + let _ = sender.send(AgentEvent::ServerRequestReceived { + thread: thread_id, + turn: Some(turn), + request: giskard_core::server_request::ServerRequest { + id: giskard_core::ids::ServerRequestId(SCRIPTED_SERVER_REQUEST_ID.into()), + method: "item/tool/requestUserInput".into(), + params: serde_json::json!({ + "questions": [{ + "id": "branch", + "header": "Branch", + "question": SCRIPTED_SERVER_REQUEST_QUESTION, + "options": [ + { "label": "main", "description": "The default branch" }, + { "label": "develop", "description": "The integration branch" } + ] + }] + }), + received_at: chrono::Utc::now(), + }, + }); + return; + } + if raise_approval { // Raise an approval and deliberately leave the turn in-flight (no TurnCompleted), so // the live buffer keeps the answered state for reconnect assertions. diff --git a/crates/giskard-server/src/live_buffer.rs b/crates/giskard-server/src/live_buffer.rs index 11d64e9..758f634 100644 --- a/crates/giskard-server/src/live_buffer.rs +++ b/crates/giskard-server/src/live_buffer.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use tokio::sync::Mutex; @@ -22,6 +22,14 @@ struct LiveTurn { /// otherwise recorded in `events` (there is no harness-emitted approval-resolved event), so /// without this the reconnect snapshot would replay every answered approval as still pending. resolved_approvals: HashMap, + /// Server requests the user answered during this turn. + /// + /// Unlike approvals these *do* have a harness-emitted resolved event, but it arrives on the + /// harness's own schedule — and a harness may never send one. Until it lands the request is + /// still "received but not resolved" as far as `events` is concerned, so a reload in that window + /// would replay it as actionable and re-answering routes a stale id to the harness. The answer + /// is recorded here the moment it is routed, which closes that window. + resolved_server_requests: HashSet, } pub struct LiveBufferStore { @@ -62,6 +70,7 @@ impl LiveBufferStore { user_input, events: Vec::new(), resolved_approvals: HashMap::new(), + resolved_server_requests: HashSet::new(), }, ); } @@ -85,6 +94,7 @@ impl LiveBufferStore { user_input, events: Vec::new(), resolved_approvals: HashMap::new(), + resolved_server_requests: HashSet::new(), }); Ok(()) } @@ -131,6 +141,16 @@ impl LiveBufferStore { } } + /// Record that the user answered a server request in the thread's in-flight turn. Mirrors + /// [`Self::resolve_approval`]: the harness's own resolved event may be late or may never come, + /// and until then a reconnect would replay the request as actionable. + pub async fn resolve_server_request(&self, thread_id: ThreadId, request_id: ServerRequestId) { + let mut buffers = self.buffers.lock().await; + if let Some(turn) = buffers.get_mut(&thread_id) { + turn.resolved_server_requests.insert(request_id); + } + } + pub async fn clear_turn(&self, thread_id: ThreadId) { let mut buffers = self.buffers.lock().await; buffers.remove(&thread_id); @@ -192,7 +212,24 @@ impl LiveBufferStore { .collect(); let accumulated: Vec = turn.events.iter().cloned().map(Into::into).collect(); - let pending_server_requests = pending_server_requests(&turn.events); + let mut pending_server_requests = pending_server_requests(&turn.events); + pending_server_requests + .retain(|request| !turn.resolved_server_requests.contains(&request.id)); + // Answered requests still ride along in `accumulated` as `ServerRequestReceived`, and + // replaying that renders an actionable card. Naming them lets the client render those + // resolved instead, exactly as `answered_approvals` does. + let answered_server_requests: Vec = turn + .events + .iter() + .filter_map(|e| match e { + AgentEvent::ServerRequestReceived { request, .. } + if turn.resolved_server_requests.contains(&request.id) => + { + Some(request.id.clone()) + } + _ => None, + }) + .collect(); LiveTurnSnapshot { thread_id, turn_id: turn.turn_id, @@ -201,6 +238,7 @@ impl LiveBufferStore { pending_approval, pending_server_requests, answered_approvals, + answered_server_requests, } }) } @@ -227,7 +265,9 @@ impl LiveBufferStore { } _ => None, }); - let server_requests = pending_server_requests(&turn.events); + let mut server_requests = pending_server_requests(&turn.events); + server_requests + .retain(|request| !turn.resolved_server_requests.contains(&request.id)); if approval.is_none() && server_requests.is_empty() { return None; } diff --git a/crates/giskard-server/src/registry.rs b/crates/giskard-server/src/registry.rs index f36e2d3..ed79eea 100644 --- a/crates/giskard-server/src/registry.rs +++ b/crates/giskard-server/src/registry.rs @@ -742,12 +742,13 @@ impl HarnessRegistry { Ok(thread_id) } - /// Route a non-approval server-request response to the harness that raised it. + /// Route a non-approval server-request response to the harness that raised it, returning the + /// thread it belonged to so the caller can record the answer against that thread's live turn. pub async fn respond_server_request( &self, request_id: ServerRequestId, response: ServerRequestResponse, - ) -> Result<(), HarnessError> { + ) -> Result { let thread_id = self .shared .server_requests @@ -777,7 +778,7 @@ impl HarnessRegistry { .respond_server_request(request_id.clone(), response) .await?; self.shared.server_requests.lock().await.remove(&request_id); - Ok(()) + Ok(thread_id) } pub async fn interrupt(&self, thread_id: ThreadId) -> Result<(), HarnessError> { diff --git a/crates/giskard-server/src/routes.rs b/crates/giskard-server/src/routes.rs index e37a222..ac62c37 100644 --- a/crates/giskard-server/src/routes.rs +++ b/crates/giskard-server/src/routes.rs @@ -3426,9 +3426,11 @@ async fn handle_client_msg( } => { let request_id_for_log = request_id.clone(); let req_id = giskard_core::ids::ServerRequestId(request_id); - tokio::time::timeout( + let thread_id = tokio::time::timeout( HARNESS_CONTROL_TIMEOUT, - state.registry.respond_server_request(req_id, response), + state + .registry + .respond_server_request(req_id.clone(), response), ) .await .map_err(|_| { @@ -3446,6 +3448,19 @@ async fn handle_client_msg( ) })? .map_err(|e| WsError::from_harness(e, "server_request_response", None))?; + // Record the answer against the in-flight turn. The harness emits its own resolved + // event, but on its own schedule and not guaranteed at all; until then the request + // still reads as outstanding in the replayed events, so a reload in that window would + // re-prompt and re-answering routes a stale id to the harness (spec §13.6). + state + .live_buffers + .resolve_server_request(thread_id, req_id) + .await; + debug!( + %thread_id, + request_id = %request_id_for_log, + "recorded server request resolution in live buffer for reconnect" + ); } ClientMessage::Interrupt { thread_id } => { tokio::time::timeout(HARNESS_CONTROL_TIMEOUT, state.registry.interrupt(thread_id)) diff --git a/crates/giskard-server/static/app.js b/crates/giskard-server/static/app.js index b81bb29..e19f136 100644 --- a/crates/giskard-server/static/app.js +++ b/crates/giskard-server/static/app.js @@ -43,7 +43,7 @@ let state = { currentRenderTurnId:null, newestPersistedTurnId:null, globalModels:[], models:[], modelsProject:null, modelsLoadingProject:null, pendingModelBeforeSelect:null, streamEl:null, streamItemId:null, pendingUserEl:null, pendingUserText:null, streamElsByItemId:new Map(), renderedItemIds:new Set(), renderedHarnessItemIds:new Set(), renderedItemBodyByKey:new Map(), itemKindsByItemId:new Map(), - pendingApprovals:new Map(), answeredApprovals:new Map(), answeredApprovalsById:new Map(), renderedApprovalStateKeys:new Set(), pendingServerRequests:new Map(), + pendingApprovals:new Map(), answeredApprovals:new Map(), answeredApprovalsById:new Map(), renderedApprovalStateKeys:new Set(), pendingServerRequests:new Map(), answeredServerRequests:new Set(), runningCommands:new Map(), commandBodyElsByItemId:new Map(), commandMsgElsByItemId:new Map(), commandStopRequestedByItemId:new Set(), selectedCommandId:null, commandPayloadsByItemId:new Map(), endedCommandsByItemId:new Map(), toolPayloadsByItemId:new Map(), toolBodyElsByItemId:new Map(), @@ -3045,6 +3045,12 @@ function renderLiveTurnSnapshot(snap) { state.answeredApprovalsById.set(String(answered.request_id), { decision: answered.decision }); } } + // Same reasoning for server requests: `accumulated` replays every ServerRequestReceived, and a + // harness's resolved event may be late or never arrive, so without this an answered request + // renders actionable again and re-answering routes a stale id to the harness. + for (const answered of (snap.answered_server_requests || [])) { + if (answered !== undefined && answered !== null) state.answeredServerRequests.add(String(answered)); + } for (const ev of (snap.accumulated||[])) handleEvent(ev); if (snap.pending_approval) { handleIncomingApprovalRequest(snap.pending_approval, snap.thread_id || state.threadId, { @@ -3370,6 +3376,10 @@ function renderServerRequest(request) { renderUnsupportedServerRequest(body, id, request, "Giskard cannot generate client attestation tokens."); } else renderUnknownServerRequest(body, id, request); + + // Built the card, now settle it: a request answered before this page load exists only as a + // replayed `ServerRequestReceived`, so it must not come back actionable. + if (state.answeredServerRequests.has(id)) resolveServerRequest(id); } function resolveServerRequest(id) { id = String(id || ""); @@ -5120,6 +5130,7 @@ function resetRenderState() { state.answeredApprovalsById = new Map(); state.renderedApprovalStateKeys = new Set(); state.pendingServerRequests = new Map(); + state.answeredServerRequests = new Set(); state.runningCommands = new Map(); state.commandBodyElsByItemId = new Map(); state.commandMsgElsByItemId = new Map(); diff --git a/crates/giskard-server/tests/server_requests.rs b/crates/giskard-server/tests/server_requests.rs index b65a606..e7f4cc8 100644 --- a/crates/giskard-server/tests/server_requests.rs +++ b/crates/giskard-server/tests/server_requests.rs @@ -31,6 +31,10 @@ struct ServerRequestHarness { active: Mutex>, responses: Mutex>, fail_next_response: Mutex>, + /// When set, routing a response does not emit `ServerRequestResolved`/`TurnCompleted`. Real + /// harnesses resolve on their own schedule and may never resolve at all, and that window is + /// exactly what the reconnect snapshot has to survive. + suppress_resolution: Mutex, } impl ServerRequestHarness { @@ -41,9 +45,14 @@ impl ServerRequestHarness { active: Mutex::new(None), responses: Mutex::new(Vec::new()), fail_next_response: Mutex::new(None), + suppress_resolution: Mutex::new(false), } } + async fn suppress_resolution(&self) { + *self.suppress_resolution.lock().await = true; + } + async fn fail_next_response(&self, error: HarnessError) { *self.fail_next_response.lock().await = Some(error); } @@ -155,6 +164,9 @@ impl AgentHarness for ServerRequestHarness { .lock() .await .push((req.clone(), response.clone())); + if *self.suppress_resolution.lock().await { + return Ok(()); + } let (thread, turn) = self.active.lock().await.take().unwrap_or_default(); let _ = self.tx.send(AgentEvent::ServerRequestResolved { thread, @@ -491,6 +503,71 @@ async fn websocket_subscribe_replays_pending_server_request_snapshot() { ); } +/// A server request the user answered must not come back actionable on reconnect, even when the +/// harness has not (or will never) emit its resolved event. Nothing recorded the answer server-side +/// before this, so the replayed `ServerRequestReceived` re-prompted and answering again routed a +/// stale id to the harness, which errors — the same defect already fixed for approvals. +#[tokio::test] +async fn answered_server_request_is_not_pending_after_reconnect() { + let (_tmp, harness, addr, cookie, thread_id) = spawn_test_app().await; + harness.suppress_resolution().await; + + let mut ws = connect_ws(addr, &cookie).await; + ws.send(ws_text(&ClientMessage::Subscribe { + thread_id, + since: None, + })) + .await + .unwrap(); + ws.send(ws_text(&ClientMessage::SendInput { + thread_id, + text: "ask me".into(), + attachments: Vec::new(), + })) + .await + .unwrap(); + wait_for_server_request(&mut ws).await; + + ws.send(ws_text(&ClientMessage::ServerRequestResponse { + request_id: "srv_1".into(), + response: ServerRequestResponse::result(serde_json::json!({ "answers": ["main"] })), + })) + .await + .unwrap(); + // The harness confirms it received the answer; it deliberately never resolves it. + let (answered_id, _) = harness.wait_for_response().await; + assert_eq!(answered_id, ServerRequestId("srv_1".into())); + + let mut reconnect = connect_ws(addr, &cookie).await; + reconnect + .send(ws_text(&ClientMessage::Subscribe { + thread_id, + since: None, + })) + .await + .unwrap(); + + let snapshot = wait_for_live_snapshot(&mut reconnect).await; + assert!( + snapshot.pending_server_requests.is_empty(), + "an answered request must not be replayed as pending, got {:?}", + snapshot.pending_server_requests + ); + assert_eq!( + snapshot.answered_server_requests, + vec![ServerRequestId("srv_1".into())], + "the reconnect snapshot must name the answered request so its card renders resolved" + ); + // It is still in the replayed events; naming it answered is what stops it re-prompting. + assert!( + snapshot.accumulated.iter().any(|event| matches!( + event, + giskard_proto::WireAgentEvent::ServerRequestReceived { .. } + )), + "the request should still appear in the accumulated events" + ); +} + #[tokio::test] async fn websocket_unknown_server_request_response_surfaces_error() { let (_tmp, _harness, addr, cookie, thread_id) = spawn_test_app().await; diff --git a/crates/giskard-server/tests/ui.rs b/crates/giskard-server/tests/ui.rs index 4bde857..50d30a0 100644 --- a/crates/giskard-server/tests/ui.rs +++ b/crates/giskard-server/tests/ui.rs @@ -666,6 +666,14 @@ async fn index_page_is_served_and_public() { "a sub-agent waiting on an approval is visually distinct from one that is merely running, \ tracked as its own flag because an approval also marks the turn active" ); + assert!( + body.contains("answeredServerRequests:new Set()") + && body.contains("for (const answered of (snap.answered_server_requests || []))") + && body.contains("if (state.answeredServerRequests.has(id)) resolveServerRequest(id);"), + "an answered server request is seeded from the reconnect snapshot before the accumulated \ + events replay, so the replayed ServerRequestReceived renders resolved instead of \ + re-prompting — a harness resolved event may be late or never arrive" + ); assert!( body.contains("msg.type === \"thread_activity_bootstrap\"") && body.contains("function handleThreadActivityBootstrap(msg)") diff --git a/specs/giskard-specification.md b/specs/giskard-specification.md index 04002e7..9bd121f 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.58 +**Version:** 1.59 > **Amendment — frontend approach (supersedes the Dioxus/WASM design below).** > This document was written targeting a **Dioxus fullstack / WebAssembly** frontend (`giskard-ui`), @@ -23,6 +23,19 @@ > below as historical design context, not a current requirement. The wire contract (`giskard-proto`) > and all backend design remain authoritative. +**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 + may never arrive at all. Until then the replayed `ServerRequestReceived` still reads as + outstanding, so a reload rendered the request actionable again and answering it a second time + routed a stale id to the harness, which errors — the defect AR1 already fixed for approvals. The + answer is now recorded against the in-flight turn the moment it is routed. +- **SR7:** `LiveTurnSnapshot` carries `answered_server_requests`, and answered requests are excluded + from both `pending_server_requests` and the SB5 connect bootstrap. The answered set is required + in addition to the exclusion: the request still rides along in `accumulated`, and replaying that + renders an actionable card unless the client is told it was answered — exactly as + `answered_approvals` works. + **Changelog (1.57 → 1.58), replaying missed cross-thread activity:** - **SB5:** `ThreadActivity` is broadcast live and was never replayed, so a browser that was closed or disconnected when a thread became blocked learned nothing about it: no sidebar badge and no @@ -2801,12 +2814,14 @@ signal still shows what is blocked; a separate message so clients can tell a rep event and apply SB6's alert-once-per-session rule), `ThreadState { thread_id, state }` (persisted snapshot on subscribe/resync), `LiveTurnSnapshot { thread_id, turn_id, user_input?, accumulated, pending_approval?, -pending_server_requests, answered_approvals }` (in-flight turn reconstruction on reconnect, carrying +pending_server_requests, answered_approvals, answered_server_requests }` (in-flight turn +reconstruction on reconnect, carrying the turn input when the server synthesized the turn context, `WireAgentEvent`s, the still-open `WireApprovalRequest`, unresolved `ServerRequest`s, and the `{ request_id, decision }` of approvals the user already answered this turn — approval resolution lives only in browser memory, so without this a reload would replay an answered approval as pending and answering it again routes a stale id -to the harness, which errors), +to the harness, which errors — and `answered_server_requests` for the same reason (SR6), since a +harness's own resolved event may be late or absent), `RunningTasks { thread_id, tasks: [RunningTask] }` (commands and tool/MCP calls still known to be running, including commands that outlived an interrupted turn), `TokenUpdate { scope, thread_id?, ledger }`, `ApprovalRequest { thread_id, request }` (a @@ -2828,6 +2843,12 @@ thread subscribers; each browser must remove the pending actions and render the card, and close only native browser notifications keyed to that request id. Duplicate/stale decisions for a removed request id remain protocol errors. +**Server-request resolution invariant (SR6):** the same holds for server requests, with one +difference: a harness emits its own resolved event, so the server does not synthesize one. It must +still record the answer against the in-flight turn when it routes the response, because that event +may be arbitrarily late or never sent, and until it lands a reconnect would replay the request as +actionable. + **Client rendering invariant (E6):** `ItemDelta { item_id }` and the later `ItemCompleted` for the same `Item.id` are one lifecycle. The UI must finalize or replace the streamed body in place when the completed item arrives. Scoped Giskard `(TurnId, ItemId)` is authoritative for diff --git a/tests/e2e/README.md b/tests/e2e/README.md index b114356..07e92f3 100644 --- a/tests/e2e/README.md +++ b/tests/e2e/README.md @@ -2,7 +2,8 @@ Browser tests that drive Giskard's real web UI — login, projects/threads, live message streaming, linked sub-agent navigation/reload/prompt ordering/cascade deletion, how a sub-agent blocked on an -approval is surfaced, and settings — through a headless Chromium. +approval is surfaced, server requests surviving a reload, and settings — through a headless +Chromium. Everything runs **inside Docker**, so you don't need Node, npm, or the Playwright browsers on your host. The one thing you do need is Docker. @@ -19,11 +20,14 @@ REST/WebSocket API as the real `giskard-server`, but: - boots with a **known password** (`giskard` by default) and one pre-seeded **"Demo"** project, so the tests can log in and drive a thread with zero host-side setup. -The sub-agent trigger, prompt, and reply constants in `giskard-server-replay` are mirrored by +The sub-agent, approval, and server-request trigger constants in `giskard-server-replay` are mirrored by `tests/helpers.ts`; update both locations together when changing that scenario. That includes the approval-blocked sub-agent scenario (`SCRIPTED_SUBAGENT_APPROVAL_*`), whose child deliberately waits before raising its approval: thread activity is broadcast live and never replayed on connect, so -firing immediately would race the browser's WebSocket and reach nobody. +firing immediately would race the browser's WebSocket and reach nobody. It also includes the +server-request scenario (`SCRIPTED_SERVER_REQUEST_*`), whose harness deliberately never emits a +resolved event when the answer is routed — modelling a harness whose resolved event is late or +absent, which is the window a reload has to survive. This keeps the suite hermetic: the production server needs a real, authenticated Codex CLI, which can't run unattended in CI. diff --git a/tests/e2e/tests/helpers.ts b/tests/e2e/tests/helpers.ts index 4fd5078..cbcd133 100644 --- a/tests/e2e/tests/helpers.ts +++ b/tests/e2e/tests/helpers.ts @@ -20,6 +20,15 @@ export const SCRIPTED_SUBAGENT_REPLY = "Child replay output"; */ export const SCRIPTED_APPROVAL_TRIGGER = "Trigger a scripted approval request."; +/** + * Prompt that raises a `requestUserInput` server request and holds the turn open. The scripted + * harness never emits a resolved event for it, modelling a harness whose resolved event is late or + * absent. Kept in sync with `SCRIPTED_SERVER_REQUEST_*` in + * `crates/giskard-server/src/bin/giskard-server-replay.rs`. + */ +export const SCRIPTED_SERVER_REQUEST_TRIGGER = "Trigger a scripted user input request."; +export const SCRIPTED_SERVER_REQUEST_QUESTION = "Which branch should I use?"; + /** * Prompt that spawns a linked child which raises an approval and then holds its turn open, while the * parent turn completes normally. Kept in sync with `SCRIPTED_SUBAGENT_APPROVAL_TRIGGER` and friends diff --git a/tests/e2e/tests/server-requests.spec.ts b/tests/e2e/tests/server-requests.spec.ts new file mode 100644 index 0000000..d54938e --- /dev/null +++ b/tests/e2e/tests/server-requests.spec.ts @@ -0,0 +1,90 @@ +import { test, expect } from "@playwright/test"; +import { + SCRIPTED_SERVER_REQUEST_QUESTION, + SCRIPTED_SERVER_REQUEST_TRIGGER, + login, +} from "./helpers"; + +// Server requests (`requestUserInput` and friends) had no browser coverage at all, despite having +// real UI: a card, per-question controls, Continue/Cancel, and a resolved state. +// +// The gap that matters is the one already fixed for approvals. A request is cleared from the live +// buffer by the harness's own resolved event, which arrives on the harness's schedule and may never +// arrive. Answering used to record nothing server-side, so a reload in that window replayed the +// request as actionable — and answering a second time routes a stale id to the harness, which +// errors. The scripted harness deliberately never resolves, so this is exactly that window. +test.describe("server requests", () => { + test.beforeEach(async ({ page }) => { + await login(page); + }); + + test("an answered user-input request stays resolved after a browser reload", async ({ page }) => { + const project = page.locator(".proj", { hasText: "Demo" }); + await project.locator(".project-add").click(); + + const input = page.locator("#input"); + await expect(input).toBeVisible(); + await input.fill(SCRIPTED_SERVER_REQUEST_TRIGGER); + await page.locator("#sendBtn").click(); + + const transcript = page.locator("#transcript"); + const request = transcript.locator(".msg.server-request"); + await expect(request).toBeVisible(); + await expect(request).toContainText("Agent needs your answer"); + await expect(request).toContainText(SCRIPTED_SERVER_REQUEST_QUESTION); + + // Actionable before it is answered: a question control and both actions. + const answer = request.locator("select.server-request-answer"); + await expect(answer).toBeVisible(); + const continueBtn = request.getByRole("button", { name: "Continue", exact: true }); + await expect(continueBtn).toBeEnabled(); + + await answer.selectOption("develop"); + await continueBtn.click(); + + // Answering disables the controls and records what was sent. The harness never emits a resolved + // event, so this is as far as the live UI goes. + await expect(request.locator(".server-request-sent")).toHaveText("Sent: Continue"); + await expect(continueBtn).toBeDisabled(); + + // Reload: in-memory state is wiped, so the resolved state has to be reconstructed entirely from + // the server's live-turn snapshot. + await page.reload(); + await expect(page.locator("#app")).toHaveClass(/open/); + + const after = page.locator("#transcript .msg.server-request"); + await expect(after).toBeVisible(); + await expect(after).toHaveClass(/\bresolved\b/); + // Never actionable again — re-answering would route a stale id to the harness. + await expect(after.getByRole("button", { name: "Continue", exact: true })).toBeDisabled(); + await expect(after.getByRole("button", { name: "Cancel", exact: true })).toBeDisabled(); + await expect(page.locator("#transcript .msg.error")).toHaveCount(0); + // The turn is still in flight, so the thread legitimately still shows activity — but it must no + // 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/, + ); + }); + + test("an unanswered request survives a reload as actionable", 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(); + + // The mirror image of the test above: nothing was answered, so the reload must bring the + // request back still actionable rather than swallowing it. + await page.reload(); + await expect(page.locator("#app")).toHaveClass(/open/); + + const after = page.locator("#transcript .msg.server-request"); + await expect(after).toBeVisible(); + await expect(after).not.toHaveClass(/\bresolved\b/); + await expect(after.getByRole("button", { name: "Continue", exact: true })).toBeEnabled(); + await expect(after.locator("select.server-request-answer")).toBeEnabled(); + }); +});