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(); + }); +});