Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion crates/giskard-proto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<AnsweredApproval>,
/// 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<ServerRequestId>,
}

/// An approval the user resolved during an in-flight turn (part of [`LiveTurnSnapshot`]).
Expand Down Expand Up @@ -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());
Expand All @@ -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");
Expand Down
41 changes: 41 additions & 0 deletions crates/giskard-server/src/bin/giskard-server-replay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down
46 changes: 43 additions & 3 deletions crates/giskard-server/src/live_buffer.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};

use tokio::sync::Mutex;

Expand All @@ -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<ApprovalId, ApprovalDecision>,
/// 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<ServerRequestId>,
}

pub struct LiveBufferStore {
Expand Down Expand Up @@ -62,6 +70,7 @@ impl LiveBufferStore {
user_input,
events: Vec::new(),
resolved_approvals: HashMap::new(),
resolved_server_requests: HashSet::new(),
},
);
}
Expand All @@ -85,6 +94,7 @@ impl LiveBufferStore {
user_input,
events: Vec::new(),
resolved_approvals: HashMap::new(),
resolved_server_requests: HashSet::new(),
});
Ok(())
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -192,7 +212,24 @@ impl LiveBufferStore {
.collect();
let accumulated: Vec<WireAgentEvent> =
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<ServerRequestId> = 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,
Expand All @@ -201,6 +238,7 @@ impl LiveBufferStore {
pending_approval,
pending_server_requests,
answered_approvals,
answered_server_requests,
}
})
}
Expand All @@ -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;
}
Expand Down
7 changes: 4 additions & 3 deletions crates/giskard-server/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ThreadId, HarnessError> {
let thread_id = self
.shared
.server_requests
Expand Down Expand Up @@ -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> {
Expand Down
19 changes: 17 additions & 2 deletions crates/giskard-server/src/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(|_| {
Expand All @@ -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))
Expand Down
13 changes: 12 additions & 1 deletion crates/giskard-server/static/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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 || "");
Expand Down Expand Up @@ -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();
Expand Down
Loading
Loading