From 1871b9ab2c5bbc6507e8916c31c76bd7efa94c0f Mon Sep 17 00:00:00 2001 From: Gnani Nutakki Date: Thu, 23 Jul 2026 13:41:06 -0500 Subject: [PATCH] fix(terminal): terminal.session fails honestly instead of minting a false receipt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `terminal.session` was a silent no-op: after the capability check it read `session_id`/`action`, never touched `command`, and returned `{"status":"ok"}` while minting a receipt marked `permitted: true`. No session was created, no command run, nothing closed — yet the signed receipt chain recorded a permitted action that never happened. False success plus false audit evidence. The runtime mints a tool receipt only on the Ok path (fused-runtime runtime.rs: a tool `Err` releases the reservation and returns without pushing a ToolCallReceipt), so returning an error is the honest-failure path. Since the persistent-session backend does not exist yet, return a new `ToolError::NotImplemented` after the capability check. Callers now see an explicit not-implemented error and no permitted receipt is minted for unperformed work. `terminal.exec`, which runs for real, is untouched. Regression tests: `terminal_session_fails_honestly_and_mints_no_receipt` (fails against the old ok+receipt behavior) and `terminal_session_denies_unauthorized_before_not_implemented` (pins the cap check ordering). Fixes #354 Checkpoint: architect/sessions/issue-354-terminal-session-honest/journal.md Signed-off-by: Gnani Nutakki --- crates/terminal/src/tools.rs | 71 ++++++++++++++++++++++++++----- crates/tool-registry/src/error.rs | 7 +++ 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/crates/terminal/src/tools.rs b/crates/terminal/src/tools.rs index 308d9858..9d7bcc4d 100644 --- a/crates/terminal/src/tools.rs +++ b/crates/terminal/src/tools.rs @@ -197,19 +197,20 @@ impl Tool for TerminalSessionTool { async fn invoke( &self, ctx: &ToolContext, - args: serde_json::Value, + _args: serde_json::Value, ) -> Result { + // Authorize first so an unauthorized caller still sees a capability + // denial rather than a not-implemented error. ensure_authorized(ctx, Capability::ShellExec)?; - let session_id = args - .get("session_id") - .and_then(|v| v.as_str()) - .unwrap_or(""); - let action = args.get("action").and_then(|v| v.as_str()).unwrap_or(""); - Ok(ToolOutput { - content: json!({"session_id": session_id, "action": action, "status": "ok"}), - cost: CostTuple::default(), - receipt_data: receipt("terminal.session", BackendKind::Local, action), - }) + // The persistent-session backend does not exist yet. Previously this + // returned `{"status":"ok"}` and minted a `permitted` receipt without + // creating a session, running the command, or closing anything — + // fabricating both a success and a signed audit record for work that + // never happened. Fail honestly instead: return an explicit error so + // the runtime mints no permitted receipt for the unperformed action. + Err(ardur_tool_registry::ToolError::NotImplemented( + "terminal.session: persistent session backend is not implemented".to_string(), + )) } fn required_capabilities(&self) -> &[Capability] { @@ -239,4 +240,52 @@ mod tests { let tool = TerminalSessionTool::new(); assert_eq!(tool.id().as_str(), "terminal.session"); } + + fn ctx_with_token(token: &str) -> ToolContext { + use ardur_tool_registry::{CapTokenRef, InvocationId, SessionId}; + ToolContext { + cap_token: CapTokenRef(token.to_string()), + session_id: SessionId::new(), + invocation_id: InvocationId::new(), + cwd: std::path::PathBuf::from("."), + env: std::collections::HashMap::new(), + cost_budget_cents: u32::MAX, + } + } + + /// Regression for #354: `terminal.session` used to return `{"status":"ok"}` + /// and mint a `permitted` receipt without creating a session or running the + /// command. It must now fail honestly with `NotImplemented` so the runtime + /// mints no permitted receipt for work that never ran. + #[tokio::test] + async fn terminal_session_fails_honestly_and_mints_no_receipt() { + let tool = TerminalSessionTool::new(); + let ctx = ctx_with_token("cap-token"); + let result = tool + .invoke( + &ctx, + json!({"session_id": "s1", "action": "exec", "command": "echo hi"}), + ) + .await; + match result { + Err(ardur_tool_registry::ToolError::NotImplemented(_)) => {} + other => panic!("expected NotImplemented, got {other:?}"), + } + } + + /// The capability check still fires before the not-implemented error, so an + /// unauthorized caller sees a capability denial rather than leaking the + /// not-implemented state. + #[tokio::test] + async fn terminal_session_denies_unauthorized_before_not_implemented() { + let tool = TerminalSessionTool::new(); + let ctx = ctx_with_token(""); + let result = tool + .invoke(&ctx, json!({"session_id": "s1", "action": "create"})) + .await; + assert!(matches!( + result, + Err(ardur_tool_registry::ToolError::CapabilityDenied(_)) + )); + } } diff --git a/crates/tool-registry/src/error.rs b/crates/tool-registry/src/error.rs index 004ac3a0..4a3b1a44 100644 --- a/crates/tool-registry/src/error.rs +++ b/crates/tool-registry/src/error.rs @@ -46,6 +46,13 @@ pub enum ToolError { #[error("tool timed out")] Timeout, + /// The tool is registered but its backend is not yet implemented. Returned + /// instead of fabricating a success: an unimplemented tool must surface an + /// explicit error so the runtime mints **no** permitted receipt for work it + /// never performed. + #[error("tool not implemented: {0}")] + NotImplemented(String), + /// Running the tool would exceed its /// [`cost_budget_cents`](crate::ToolContext::cost_budget_cents) ceiling. #[error("tool invocation exceeds its cost ceiling")]