diff --git a/Cargo.lock b/Cargo.lock index 4f81923..22310a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -651,6 +651,7 @@ dependencies = [ "giskard-persist", "giskard-proto", "hmac", + "indexmap", "pulldown-cmark", "rand 0.8.6", "regex", diff --git a/Cargo.toml b/Cargo.toml index 164fe32..90ed110 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,3 +56,4 @@ hex = "0.4" syntect = { version = "5", default-features = false, features = ["default-fancy"] } two-face = { version = "0.5.1", default-features = false, features = ["syntect-fancy"] } regex = "1" +indexmap = "2" diff --git a/crates/giskard-proto/src/lib.rs b/crates/giskard-proto/src/lib.rs index cfeb7a2..d820e35 100644 --- a/crates/giskard-proto/src/lib.rs +++ b/crates/giskard-proto/src/lib.rs @@ -157,8 +157,6 @@ pub struct LiveTurnSnapshot { /// approvals at once (three commands proposed together, say), and a single field would name /// only the most recently raised one and silently drop the rest. pub accumulated: Vec, - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub pending_server_requests: Vec, /// Approvals the user already answered during this in-flight turn, with the decision they made. /// /// Approval resolution lives only in browser memory, so a reload would otherwise re-surface an @@ -985,28 +983,32 @@ mod tests { } #[test] - fn live_turn_snapshot_includes_pending_server_requests() { - let tid = ThreadId::new(); + fn live_turn_snapshot_replays_server_requests_from_accumulated() { let turn = TurnId::new(); + let thread = ThreadId::new(); let snapshot = LiveTurnSnapshot { - thread_id: tid, + thread_id: thread, turn_id: turn, user_input: None, - accumulated: vec![], - pending_server_requests: vec![ServerRequest { - id: giskard_core::ids::ServerRequestId("req_1".into()), - method: "item/tool/call".into(), - params: serde_json::json!({ "tool": "example" }), - received_at: chrono::Utc::now(), + accumulated: vec![WireAgentEvent::ServerRequestReceived { + thread, + turn: Some(turn), + request: ServerRequest { + id: giskard_core::ids::ServerRequestId("req_1".into()), + method: "item/tool/call".into(), + params: serde_json::json!({ "tool": "example" }), + 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()); - assert_eq!(json["pending_server_requests"][0]["id"], "req_1"); + assert_eq!(json["thread_id"], thread.to_string()); + assert_eq!(json["accumulated"][0]["kind"], "server_request_received"); + assert_eq!(json["accumulated"][0]["request"]["id"], "req_1"); assert_eq!( - json["pending_server_requests"][0]["method"], + json["accumulated"][0]["request"]["method"], "item/tool/call" ); } @@ -1018,7 +1020,6 @@ mod tests { turn_id: TurnId::new(), user_input: None, accumulated: vec![], - pending_server_requests: vec![], answered_approvals: vec![AnsweredApproval { request_id: ApprovalId("ap_1".into()), decision: ApprovalDecision::Accept, diff --git a/crates/giskard-server/Cargo.toml b/crates/giskard-server/Cargo.toml index 72ed003..f5ea23c 100644 --- a/crates/giskard-server/Cargo.toml +++ b/crates/giskard-server/Cargo.toml @@ -36,6 +36,7 @@ two-face = { workspace = true } regex = { workspace = true } reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } pulldown-cmark = { version = "0.13.4", default-features = false } +indexmap = { workspace = true } [build-dependencies] sha2 = { workspace = true } diff --git a/crates/giskard-server/src/bin/giskard-server-replay.rs b/crates/giskard-server/src/bin/giskard-server-replay.rs index 08a2a7d..c2b9131 100644 --- a/crates/giskard-server/src/bin/giskard-server-replay.rs +++ b/crates/giskard-server/src/bin/giskard-server-replay.rs @@ -83,6 +83,13 @@ const SCRIPTED_APPROVAL_THEN_ERROR_MESSAGE: &str = "Scripted non-fatal harness e 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?"; +/// Raises a server request and then streams a harness error in the same still-open turn. The error +/// is the last activity-bearing event, so a reconnect that took the replayed events at face value +/// would land on "errored, no active turn" and lose the fact that the turn is still blocked on the +/// user. Re-asserting the outstanding set after the replay is what restores it (SR11b). +const SCRIPTED_SERVER_REQUEST_THEN_ERROR_TRIGGER: &str = + "Trigger a server request followed by an error."; +const SCRIPTED_SERVER_REQUEST_THEN_ERROR_MESSAGE: &str = "Scripted non-fatal harness error."; /// 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); @@ -439,7 +446,10 @@ impl AgentHarness for ScriptedHarness { .insert(ApprovalId(SCRIPTED_APPROVAL_ID.into()), (thread_id, turn)); } - let raise_server_request = input_text == Some(SCRIPTED_SERVER_REQUEST_TRIGGER); + let raise_server_request_then_error = + input_text == Some(SCRIPTED_SERVER_REQUEST_THEN_ERROR_TRIGGER); + let raise_server_request = + input_text == Some(SCRIPTED_SERVER_REQUEST_TRIGGER) || raise_server_request_then_error; // 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 @@ -475,6 +485,16 @@ impl AgentHarness for ScriptedHarness { received_at: chrono::Utc::now(), }, }); + if raise_server_request_then_error { + tokio::task::yield_now().await; + let _ = sender.send(AgentEvent::Error { + thread: thread_id, + turn: Some(turn), + error: giskard_core::error::HarnessError::Protocol( + SCRIPTED_SERVER_REQUEST_THEN_ERROR_MESSAGE.into(), + ), + }); + } return; } diff --git a/crates/giskard-server/src/live_buffer.rs b/crates/giskard-server/src/live_buffer.rs index 817babb..dffb6ff 100644 --- a/crates/giskard-server/src/live_buffer.rs +++ b/crates/giskard-server/src/live_buffer.rs @@ -1,3 +1,4 @@ +use indexmap::IndexMap; use std::collections::{HashMap, HashSet}; use tokio::sync::Mutex; @@ -201,9 +202,6 @@ impl LiveBufferStore { .collect(); let accumulated: Vec = turn.events.iter().cloned().map(Into::into).collect(); - 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. @@ -224,7 +222,6 @@ impl LiveBufferStore { turn_id: turn.turn_id, user_input: turn.user_input.clone(), accumulated, - pending_server_requests, answered_approvals, answered_server_requests, } @@ -245,9 +242,8 @@ impl LiveBufferStore { .iter() .filter_map(|(thread_id, turn)| { let approvals = pending_approvals(&turn.events, &turn.resolved_approvals); - let mut server_requests = pending_server_requests(&turn.events); - server_requests - .retain(|request| !turn.resolved_server_requests.contains(&request.id)); + let server_requests = + pending_server_requests(&turn.events, &turn.resolved_server_requests); if approvals.is_empty() && server_requests.is_empty() { return None; } @@ -298,20 +294,34 @@ fn pending_approvals( .collect() } -fn pending_server_requests(events: &[AgentEvent]) -> Vec { - let mut requests: HashMap = HashMap::new(); +fn pending_server_requests( + events: &[AgentEvent], + resolved_server_requests: &HashSet, +) -> Vec { + // An `IndexMap` keeps arrival order deterministically (unlike `HashMap::into_values`, which + // iterated by hash bucket) so the bootstrap names the same request in the sidebar every reload. + // `insert` on receive updates the payload in place when the id is already present, or appends + // it when it is new; `shift_remove` on resolve drops it. A re-sent id after a resolution + // re-inserts at the end, so a reopen moves to the back rather than keeping its first-seen + // position — the reopened request is the newest thing demanding attention. Mirrors + // `outstandingServerRequests` in the browser. + let mut pending: IndexMap = IndexMap::new(); for event in events { match event { AgentEvent::ServerRequestReceived { request, .. } => { - requests.insert(request.id.clone(), request.clone()); + pending.insert(request.id.clone(), request); } AgentEvent::ServerRequestResolved { request_id, .. } => { - requests.remove(request_id); + pending.shift_remove(request_id); } _ => {} } } - requests.into_values().collect() + pending + .into_iter() + .filter(|(id, _)| !resolved_server_requests.contains(id)) + .map(|(_, request)| request.clone()) + .collect() } fn completed_command_item_id(event: &AgentEvent) -> Option { @@ -465,6 +475,35 @@ mod tests { order } + /// The server requests a reconnecting client would still treat as actionable, in arrival + /// order. Mirrors `outstandingServerRequests` in the browser so a server-side test can assert + /// what the client will derive from the snapshot. + fn outstanding_server_requests(snapshot: &LiveTurnSnapshot) -> Vec { + let answered: HashSet = + snapshot.answered_server_requests.iter().cloned().collect(); + // An `IndexSet` keeps arrival order deterministically. `insert` on receive updates in place + // when the id is already present, or appends it when it is new; `shift_remove` on resolve + // drops it. A re-sent id after a resolution re-inserts at the end, so a reopen moves to the + // back rather than keeping its first-seen position. Mirrors `outstandingServerRequests` in + // the browser and `pending_server_requests` on the server. + let mut pending: indexmap::IndexSet = indexmap::IndexSet::new(); + for event in &snapshot.accumulated { + match event { + WireAgentEvent::ServerRequestReceived { request, .. } => { + pending.insert(request.id.clone()); + } + WireAgentEvent::ServerRequestResolved { request_id, .. } => { + pending.shift_remove(request_id); + } + _ => {} + } + } + pending + .into_iter() + .filter(|id| !answered.contains(id)) + .collect() + } + fn command_start(item_id: ItemId) -> ItemStart { ItemStart { id: item_id, @@ -694,8 +733,99 @@ mod tests { .await; let snapshot = store.snapshot(thread).await.expect("snapshot"); - assert_eq!(snapshot.pending_server_requests.len(), 1); - assert_eq!(snapshot.pending_server_requests[0].id, pending); + // The harness resolved one with its own event, the other is still open. The outstanding + // server requests are derived from `accumulated` plus `answered_server_requests`, so only + // the still-open one is reported as pending. + assert_eq!( + outstanding_server_requests(&snapshot), + vec![pending.clone()] + ); + } + + // A harness may resolve a server request and then re-raise the same id with a fresher payload + // (a re-send). The outstanding derivation must reopen it, not keep it hidden behind the earlier + // resolution — a reconnected user has to be able to answer it. + #[tokio::test] + async fn a_server_request_reopened_after_resolution_is_outstanding_again() { + let store = LiveBufferStore::new(); + let thread = ThreadId::new(); + let turn = TurnId::new(); + let id = ServerRequestId("srv".into()); + + store.start_turn(thread).await; + store + .append(thread, AgentEvent::TurnStarted { thread, turn }) + .await; + store + .append(thread, server_request_received(thread, turn, &id)) + .await; + store + .append( + thread, + AgentEvent::ServerRequestResolved { + thread, + turn: Some(turn), + request_id: id.clone(), + }, + ) + .await; + // The harness re-raises the same id after resolving it. + store + .append(thread, server_request_received(thread, turn, &id)) + .await; + + let snapshot = store.snapshot(thread).await.expect("snapshot"); + assert_eq!( + outstanding_server_requests(&snapshot), + vec![id], + "a re-sent id after resolution must reopen the request" + ); + } + + // A reopen moves the request to the end of the arrival order, not back to its first-seen + // position. The sequence received(A), received(B), resolved(A), received(A) yields [B, A]: + // the second receive(A) re-inserts A at the back, so the reopened request is the newest thing + // demanding attention. Mirrors `pending_server_requests` on the server and + // `outstandingServerRequests` in the browser. + #[tokio::test] + async fn a_reopened_server_request_moves_to_the_end_of_arrival_order() { + let store = LiveBufferStore::new(); + let thread = ThreadId::new(); + let turn = TurnId::new(); + let a = ServerRequestId("a".into()); + let b = ServerRequestId("b".into()); + + store.start_turn(thread).await; + store + .append(thread, AgentEvent::TurnStarted { thread, turn }) + .await; + store + .append(thread, server_request_received(thread, turn, &a)) + .await; + store + .append(thread, server_request_received(thread, turn, &b)) + .await; + store + .append( + thread, + AgentEvent::ServerRequestResolved { + thread, + turn: Some(turn), + request_id: a.clone(), + }, + ) + .await; + // A re-opens after being resolved. + store + .append(thread, server_request_received(thread, turn, &a)) + .await; + + let snapshot = store.snapshot(thread).await.expect("snapshot"); + assert_eq!( + outstanding_server_requests(&snapshot), + vec![b.clone(), a.clone()], + "a reopen moves the request to the end, it does not restore its first-seen position" + ); } #[tokio::test] @@ -790,6 +920,19 @@ mod tests { } } + fn server_request_received(thread: ThreadId, turn: TurnId, id: &ServerRequestId) -> AgentEvent { + AgentEvent::ServerRequestReceived { + thread, + turn: Some(turn), + request: ServerRequest { + id: id.clone(), + method: "item/tool/call".into(), + params: serde_json::json!({ "tool": "example" }), + received_at: Utc::now(), + }, + } + } + #[tokio::test] async fn answered_approval_is_not_resurfaced_as_pending() { let store = LiveBufferStore::new(); @@ -920,6 +1063,57 @@ mod tests { ); } + // A turn blocked on several server requests must report all of them in first-seen order, both + // in the reconnect snapshot and the SB5 connect bootstrap. The bootstrap feeds the sidebar's + // "what is this thread waiting on" and notification-click focus, so its order must be + // deterministic, not decided by hash iteration. + #[tokio::test] + async fn every_unanswered_server_request_is_reported_in_arrival_order() { + let store = LiveBufferStore::new(); + let thread = ThreadId::new(); + let turn = TurnId::new(); + let ids: Vec = ["sr_first", "sr_second", "sr_third"] + .into_iter() + .map(|s| ServerRequestId(s.into())) + .collect(); + + store.start_turn(thread).await; + store + .append(thread, AgentEvent::TurnStarted { thread, turn }) + .await; + for id in &ids { + store + .append(thread, server_request_received(thread, turn, id)) + .await; + } + + let snapshot = store.snapshot(thread).await.expect("snapshot"); + assert_eq!( + outstanding_server_requests(&snapshot), + ids, + "the snapshot derivation is first-seen ordered" + ); + + // The SB5 connect bootstrap (the production `pending_attention`) is first-seen ordered too. + let [attention] = store + .pending_attention() + .await + .into_iter() + .filter(|entry| entry.thread_id == thread) + .collect::>() + .try_into() + .expect("one pending-attention entry for the thread"); + assert_eq!( + attention + .server_requests + .iter() + .map(|request| request.id.clone()) + .collect::>(), + ids, + "the connect bootstrap must preserve first-seen order, not hash iteration order" + ); + } + #[tokio::test] async fn resolve_approval_is_a_no_op_without_a_live_turn() { let store = LiveBufferStore::new(); diff --git a/crates/giskard-server/static/app.js b/crates/giskard-server/static/app.js index 8abfec4..e1d70b8 100644 --- a/crates/giskard-server/static/app.js +++ b/crates/giskard-server/static/app.js @@ -3139,7 +3139,7 @@ function renderLiveTurnSnapshot(snap) { source: "live_turn_snapshot_outstanding_approval" }); } - for (const request of (snap.pending_server_requests || [])) { + for (const request of outstandingServerRequests(snap)) { setActiveThreadActivity("server_request_received", true, "Waiting for your input", { server_request_id:request && request.id ? String(request.id) : null }); @@ -3171,6 +3171,38 @@ function outstandingApprovals(snap) { .filter(request => request && !answered.has(String(request.id))); } +// Every server request the replayed turn is still waiting on the user for, oldest first. A +// request leaves the outstanding set when the user answered it (named in +// `snap.answered_server_requests`) or when the harness closed it (`server_request_resolved` in +// `accumulated`). A re-sent id is the same request with a fresher payload, so the latest +// `server_request_received` wins and the order is the first occurrence of each id. +function outstandingServerRequests(snap) { + const answered = new Set(); + for (const id of (snap.answered_server_requests || [])) { + if (id !== undefined && id !== null) answered.add(String(id)); + } + // A `Map` keeps arrival order (iterating a Map yields entries in insertion order). `set` on + // receive updates the payload in place when the id is already present, or appends it when it is + // new; `delete` on resolve drops it. A re-sent id after a resolution re-inserts at the end, so a + // reopen moves to the back rather than keeping its first-seen position. Mirrors + // `pending_server_requests` on the server. + const pending = new Map(); + for (const ev of (snap.accumulated || [])) { + if (!ev || !ev.kind) continue; + if (ev.kind === "server_request_received") { + const request = ev.request; + const id = request && request.id !== undefined && request.id !== null ? String(request.id) : null; + if (!id) continue; + pending.set(id, request); + } else if (ev.kind === "server_request_resolved") { + const id = ev.request_id !== undefined && ev.request_id !== null ? String(ev.request_id) : null; + if (id) pending.delete(id); + } + } + return Array.from(pending.values()) + .filter(request => request && !answered.has(String(request.id))); +} + function renderLiveTurnUserInput(turnId, userInput) { const text = persistedUserInputDisplayText(userInput); if (!turnId || !text) return; diff --git a/crates/giskard-server/tests/server_requests.rs b/crates/giskard-server/tests/server_requests.rs index e7f4cc8..3b42380 100644 --- a/crates/giskard-server/tests/server_requests.rs +++ b/crates/giskard-server/tests/server_requests.rs @@ -18,7 +18,7 @@ use giskard_harness::{ AgentEventStream, AgentHarness, HarnessCapabilities, OpenThreadOptions, ThreadHandle, }; use giskard_persist::store::ProjectConfig; -use giskard_proto::{ClientMessage, ServerMessage, WireAgentEvent}; +use giskard_proto::{ClientMessage, LiveTurnSnapshot, ServerMessage, WireAgentEvent}; use giskard_server::{AppState, HarnessFactory, build_app}; use tokio::sync::{Mutex, broadcast}; use tokio::time::{Duration, Instant}; @@ -492,14 +492,17 @@ async fn websocket_subscribe_replays_pending_server_request_snapshot() { let snapshot = wait_for_live_snapshot(&mut reconnect).await; assert_eq!(snapshot.thread_id, thread_id); - assert_eq!(snapshot.pending_server_requests.len(), 1); - assert_eq!( - snapshot.pending_server_requests[0].id, - ServerRequestId("srv_1".into()) - ); - assert_eq!( - snapshot.pending_server_requests[0].method, - "item/tool/requestUserInput" + // The outstanding server request is derived from `accumulated` plus + // `answered_server_requests`, so the still-open one is reported as pending. + let rows = server_request_rows(&snapshot); + let [(request, resolved)] = &rows[..] else { + panic!("expected exactly one server request row, got {rows:?}"); + }; + assert_eq!(request.id, ServerRequestId("srv_1".into())); + assert_eq!(request.method, "item/tool/requestUserInput"); + assert!( + !resolved, + "nobody answered it, so its row must still read open" ); } @@ -548,10 +551,16 @@ async fn answered_server_request_is_not_pending_after_reconnect() { .unwrap(); let snapshot = wait_for_live_snapshot(&mut reconnect).await; + // The request is still replayed — its own row is what says it was answered, so the card renders + // resolved instead of re-prompting. + let rows = server_request_rows(&snapshot); + let [(request, resolved)] = &rows[..] else { + panic!("expected exactly one server request row, got {rows:?}"); + }; + assert_eq!(request.id, ServerRequestId("srv_1".into())); assert!( - snapshot.pending_server_requests.is_empty(), - "an answered request must not be replayed as pending, got {:?}", - snapshot.pending_server_requests + resolved, + "an answered request's row must be stamped resolved so it is not replayed as actionable" ); assert_eq!( snapshot.answered_server_requests, @@ -625,6 +634,31 @@ async fn wait_for_ws_error(ws: &mut TestWs) -> giskard_proto::ErrorInfo { panic!("error message not observed"); } +/// Every server-request row in the snapshot, paired with whether a reconnecting client would +/// render it resolved (answered by the user or closed by the harness). Mirrors the client's +/// `outstandingServerRequests` derivation. +fn server_request_rows(snapshot: &LiveTurnSnapshot) -> Vec<(ServerRequest, bool)> { + let answered: std::collections::HashSet = + snapshot.answered_server_requests.iter().cloned().collect(); + let mut closed = std::collections::HashSet::new(); + for ev in &snapshot.accumulated { + if let WireAgentEvent::ServerRequestResolved { request_id, .. } = ev { + closed.insert(request_id.clone()); + } + } + snapshot + .accumulated + .iter() + .filter_map(|event| match event { + WireAgentEvent::ServerRequestReceived { request, .. } => Some(( + request.clone(), + answered.contains(&request.id) || closed.contains(&request.id), + )), + _ => None, + }) + .collect() +} + async fn wait_for_live_snapshot(ws: &mut TestWs) -> giskard_proto::LiveTurnSnapshot { let deadline = Instant::now() + Duration::from_secs(5); while Instant::now() < deadline { diff --git a/crates/giskard-server/tests/ui.rs b/crates/giskard-server/tests/ui.rs index 68c3242..099a90a 100644 --- a/crates/giskard-server/tests/ui.rs +++ b/crates/giskard-server/tests/ui.rs @@ -287,8 +287,10 @@ async fn index_page_is_served_and_public() { "UI consumes server request resolution events" ); assert!( - body.contains("pending_server_requests"), - "UI replays pending server requests from live snapshots" + body.contains("function outstandingServerRequests(snap)") + && body.contains("for (const request of outstandingServerRequests(snap))"), + "the UI derives outstanding server requests from the replayed rows, which carry their \ + own answers, rather than from a companion list" ); assert!( body.contains("type:\"server_request_response\""), diff --git a/specs/giskard-specification.md b/specs/giskard-specification.md index 3c9b970..b30b671 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.61 +**Version:** 1.62 > **Amendment — frontend approach (supersedes the Dioxus/WASM design below).** > This document was written targeting a **Dioxus fullstack / WebAssembly** frontend (`giskard-ui`), @@ -23,6 +23,26 @@ > below as historical design context, not a current requirement. The wire contract (`giskard-proto`) > and all backend design remain authoritative. +**Changelog (1.61 → 1.62), drop `pending_server_requests`:** +- **SR11a:** `LiveTurnSnapshot` no longer carries `pending_server_requests: Vec`. + It was a precomputed outstanding set that duplicated a derivation already available on both ends: + the outstanding server requests are the rows in `accumulated` whose latest event is a + `server_request_received` and which are neither in `answered_server_requests` (the user answered) + nor closed by a `server_request_resolved` later in `accumulated` (the harness closed it). They + are reported in arrival order: a `server_request_received` appends the id (or updates its payload + in place), a `server_request_resolved` drops it, and a re-sent id after a resolution re-appends + at the end — so a reopen moves to the back, it does not restore its first-seen position. The + client derives this with `outstandingServerRequests`, exactly as it already derives + `outstandingApprovals`; the server's `pending_attention` derives it for the SB5 connect bootstrap. + `answered_server_requests` is kept: it is the only record of a user's answer before/without the + harness's own resolved event, so dropping it would replay an answered request as actionable and + re-answering routes a stale id to the harness (SR6). +- **SR11b:** The client re-asserts the outstanding server requests *after* replaying `accumulated`, + mirroring the approval path (SR10b): a later `error` overwrites the thread's activity and clears + the active turn, so re-asserting last gives the waiting state the last word and keeps a turn + blocked on a server request that then errored reading as waiting on the user rather than + "errored, idle". + **Changelog (1.60 → 1.61), drop the single `pending_approval`:** - **SR10a:** `LiveTurnSnapshot` no longer carries `pending_approval: Option`. It was derived with `.iter().rev().find_map(...)`, so it named only the most recently raised @@ -2845,18 +2865,19 @@ above, sent once to a connecting client only and never broadcast, so a browser t signal still shows what is blocked; a separate message so clients can tell a replay from a live 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_server_requests, +`LiveTurnSnapshot { thread_id, turn_id, user_input?, accumulated, answered_approvals, answered_server_requests }` (in-flight turn reconstruction on reconnect, carrying the turn input when the server synthesized the turn context, the `WireAgentEvent`s of the -turn so far, unresolved `ServerRequest`s, and the `{ request_id, decision }` of approvals the user -already answered this turn. Every `ApprovalRequested` rides along in `accumulated`, answered ones -included; the client renders answered ones resolved using `answered_approvals` and treats the rest -as actionable. There is no separate "pending approval" field: a turn can be blocked on several -approvals at once, and a single field would name only the most recently raised one and silently drop -the rest. Approval resolution lives only in browser memory, so without `answered_approvals` a reload -would replay an answered approval as pending and answering it again routes a stale id 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), +turn so far. Every `ApprovalRequested` and `ServerRequestReceived` rides along in `accumulated`, +answered ones included; the client derives what is still outstanding while replaying, with no +companion pending list. A request is outstanding unless the user answered it (named in +`answered_approvals` / `answered_server_requests`) or, for server requests, the harness closed it +(`server_request_resolved` later in `accumulated`). There is no separate "pending" field for either +kind: a turn can be blocked on several at once, and a precomputed list would duplicate a derivation +already available on both ends. Approval resolution lives only in browser memory, so without +`answered_approvals` a reload would replay an answered approval as pending and answering it again +routes a stale id 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 diff --git a/tests/e2e/tests/helpers.ts b/tests/e2e/tests/helpers.ts index cd14558..be6dd4d 100644 --- a/tests/e2e/tests/helpers.ts +++ b/tests/e2e/tests/helpers.ts @@ -37,6 +37,15 @@ export const SCRIPTED_APPROVAL_THEN_ERROR_MESSAGE = "Scripted non-fatal harness 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 raises a server request and then streams a harness error in the same still-open + * turn, so the error is the last activity-bearing event of the turn. Kept in sync with + * `SCRIPTED_SERVER_REQUEST_THEN_ERROR_*` in + * `crates/giskard-server/src/bin/giskard-server-replay.rs`. + */ +export const SCRIPTED_SERVER_REQUEST_THEN_ERROR_TRIGGER = + "Trigger a server request followed by an error."; +export const SCRIPTED_SERVER_REQUEST_THEN_ERROR_MESSAGE = "Scripted non-fatal harness error."; /** * Prompt that spawns a linked child which raises an approval and then holds its turn open, while the diff --git a/tests/e2e/tests/server-requests.spec.ts b/tests/e2e/tests/server-requests.spec.ts index db9def5..9c38ec0 100644 --- a/tests/e2e/tests/server-requests.spec.ts +++ b/tests/e2e/tests/server-requests.spec.ts @@ -3,6 +3,8 @@ import { SCRIPTED_SERVER_REQUEST_ID, SCRIPTED_SERVER_REQUEST_QUESTION, SCRIPTED_SERVER_REQUEST_TRIGGER, + SCRIPTED_SERVER_REQUEST_THEN_ERROR_MESSAGE, + SCRIPTED_SERVER_REQUEST_THEN_ERROR_TRIGGER, login, recordedNotifications, stubNotifications, @@ -187,3 +189,47 @@ test.describe("server requests", () => { await expect(after.locator("select.server-request-answer")).toBeEnabled(); }); }); + +// The reconnect snapshot re-asserts what the turn is still waiting on *after* replaying the +// accumulated events, mirroring the approval path. An `error` declares the turn inactive, so a +// turn blocked on a server request that then errored would come back reading "errored, idle" — +// no sidebar glyph, no waiting rank — while the request sat in the transcript with nothing +// pointing at it. Re-asserting the outstanding server requests last gives the waiting state the +// last word, so it outranks the error. +test.describe("a still-blocked turn survives a reload that replays a later error", () => { + test.beforeEach(async ({ page }) => { + await login(page); + }); + + test("a thread blocked on a server request still reads as waiting after an error", 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_THEN_ERROR_TRIGGER); + await page.locator("#sendBtn").click(); + + // Both arrive in the same still-open turn, the error last. + await expect(page.locator("#transcript .msg.server-request")).toBeVisible(); + await expect(page.locator("#transcript .msg.error")).toContainText( + SCRIPTED_SERVER_REQUEST_THEN_ERROR_MESSAGE, + ); + + await page.reload(); + await expect(page.locator("#app")).toHaveClass(/open/); + + // The error is replayed and still shown — it happened, and the transcript is the record. + await expect(page.locator("#transcript .msg.error")).toContainText( + SCRIPTED_SERVER_REQUEST_THEN_ERROR_MESSAGE, + ); + // But the turn is still blocked on the user, and that outranks the error in the sidebar. + const row = page.locator(".thread.active"); + await expect(row).toHaveClass(/\bactivity-waiting\b/); + await expect(row.locator(".thread-status")).toHaveText("!"); + // And the request is still answerable, not stranded in a thread that reads as finished. + await expect( + page.locator("#transcript .msg.server-request").getByRole("button", { name: "Continue", exact: true }), + ).toBeEnabled(); + }); +});