From 41278e694e6458a9c6d9c3a0b627ad71879c5e3d Mon Sep 17 00:00:00 2001 From: daftpunkwav Date: Wed, 28 Oct 2026 20:38:06 +0800 Subject: [PATCH 1/3] fix: land the October review's contract decisions Five deferred decisions from the tracking issue take their final shape: - the four never-produced wire events (plan_proposed, plan_approved, goal_set, goal_completed) go away with their frontend consumers, the now-unreachable footer goal badge, and the TS SDK members - nothing ever emitted them, and dead wire surface invites dead protocol assumptions; - the app server answers every failed request with the same JSON {"error": ...} body: handler validation, the auth guard, and a custom ApiJson extractor that re-shapes axum's rejections while keeping their statuses (400/415/422); - mcp serve tools/list carries MCP annotations mirroring each tool's read-only/destructive classification; - web_fetch treats the reserved name localhost as the loopback literal it is (RFC 6761): the resolver answers it without DNS, so the local-dev reach a 127.0.0.1 literal already has covers the name too and no hosts override can point it elsewhere; the private-name vet is extracted as a testable pure function; - the system prompt's Shell line reports the shell the WAVECODE_SHELL override actually spawns (shared shell_name in infrastructure-base) instead of the login shell. --- CHANGELOG.md | 14 +++ crates/capabilities/tools/src/web_fetch.rs | 91 +++++++++++++++---- crates/foundation/wire/src/lib.rs | 14 --- .../foundation/wire/src/tests/round_trips.rs | 12 --- crates/foundation/wire/src/tests/tags.rs | 14 --- crates/frontends/console/src/chrome/footer.rs | 19 ---- crates/frontends/console/src/state.rs | 13 --- crates/frontends/console/src/ui.rs | 23 +---- crates/frontends/harness/src/exec.rs | 16 ---- crates/infrastructure/base/src/lib.rs | 31 +++++++ .../operations/bootstrap/src/environment.rs | 22 +---- crates/operations/gateway/src/app_server.rs | 89 ++++++++++++------ .../gateway/src/app_server_tests.rs | 51 +++++++++++ crates/operations/gateway/src/mcp_serve.rs | 18 ++++ sdk/typescript/src/types.ts | 8 -- 15 files changed, 253 insertions(+), 182 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fecdad8a..31dc14cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,6 +122,20 @@ accumulate under `Unreleased` until a `v*` tag publishes them. terminal (still capped at 80 columns on wide terminals). - `max_tool_rounds` default raised from 32 to 256, and goal continuations per turn from 5 to 8, for super-long-horizon turns. +- October-review contract decisions land: the four never-produced + wire events (`plan_proposed`, `plan_approved`, `goal_set`, + `goal_completed`) are removed together with their frontend consumers + and the TS SDK members (breaking wire change — nothing ever emitted + them); the app server answers every failed request, extractor + rejections included, with the same JSON `{"error": ...}` body; + `mcp serve` `tools/list` carries MCP `annotations` + (`readOnlyHint`/`destructiveHint`) mirroring each tool's effect + classification; `web_fetch` treats the reserved name `localhost` as + the loopback literal it is (RFC 6761, answered without DNS), so it + reaches local dev servers exactly like the already-allowed + `127.0.0.1` literal; and the system prompt's `Shell:` line reports + the shell the `WAVECODE_SHELL` override actually spawns instead of + the login shell. - Warnings and loop notices render in the theme's warning color instead of dim body text, and the round-limit warning names the `max_tool_rounds` config key. diff --git a/crates/capabilities/tools/src/web_fetch.rs b/crates/capabilities/tools/src/web_fetch.rs index ac7a995c..c6ad5330 100644 --- a/crates/capabilities/tools/src/web_fetch.rs +++ b/crates/capabilities/tools/src/web_fetch.rs @@ -181,6 +181,13 @@ fn is_private_addr(addr: std::net::IpAddr) -> bool { /// literals skip DNS entirely, so a literal redirect target is judged /// per hop by [`is_link_local_host`] (link-local refused; the /// loopback/RFC1918 allowance is documented there). +/// +/// The reserved name `localhost` is treated as the loopback literal it +/// is (RFC 6761: it never names a remote machine and must not be +/// forwarded to DNS): the resolver answers it with the loopback +/// addresses directly, so the same local-dev reach a `127.0.0.1` +/// literal already has covers the name too, and a hosts-file override +/// can never point it elsewhere. #[derive(Debug)] struct NonPrivateResolver; @@ -188,6 +195,16 @@ impl reqwest::dns::Resolve for NonPrivateResolver { fn resolve(&self, name: reqwest::dns::Name) -> reqwest::dns::Resolving { let name = name.as_str().to_string(); Box::pin(async move { + if name.eq_ignore_ascii_case("localhost") { + return Ok(Box::new( + [ + std::net::SocketAddr::from(([127, 0, 0, 1], 0)), + std::net::SocketAddr::from(([0u16, 0, 0, 0, 0, 0, 0, 1], 0)), + ] + .into_iter(), + ) + as Box + Send>); + } let host = name.clone(); let addrs = tokio::task::spawn_blocking(move || { std::net::ToSocketAddrs::to_socket_addrs(&(host.as_str(), 0u16)) @@ -196,23 +213,38 @@ impl reqwest::dns::Resolve for NonPrivateResolver { .await .map_err(|e| -> Box { Box::new(e) })? .map_err(|e| -> Box { Box::new(e) })?; - let vetted: Vec = addrs - .into_iter() - .filter(|addr| !is_private_addr(addr.ip())) - .collect(); - if vetted.is_empty() { - return Err(Box::new(std::io::Error::other(format!( - "host '{name}' resolves only into private address ranges; \ - this tool never reaches private networks by name" - ))) - as Box); - } + let vetted = vet_public_addrs(&name, addrs).map_err( + |message| -> Box { + Box::new(std::io::Error::other(message)) + }, + )?; Ok(Box::new(vetted.into_iter()) as Box + Send>) }) } } +/// Filter resolved addresses down to the publicly routable ones; empty +/// output means the name resolves only into private space and the fetch +/// is refused (a mixed answer keeps its public legs, so a rebinding +/// attempt cannot smuggle a private leg past the vet). +fn vet_public_addrs( + name: &str, + addrs: Vec, +) -> std::result::Result, String> { + let vetted: Vec = addrs + .into_iter() + .filter(|addr| !is_private_addr(addr.ip())) + .collect(); + if vetted.is_empty() { + return Err(format!( + "host '{name}' resolves only into private address ranges; \ + this tool never reaches private networks by name" + )); + } + Ok(vetted) +} + /// Decode a body as text: explicit latin-1 family charsets decode each /// byte to the same codepoint, everything else is UTF-8 (lossy fallback /// so a truncation-split multibyte tail cannot fail the fetch). @@ -660,15 +692,38 @@ mod tests { } } - /// A name resolving only into private space fails the resolver; - /// `localhost` is the universally available specimen (hosts file, - /// no network needed). + /// A name resolving only into private space fails the vet; a mixed + /// answer keeps only its public legs (rebinding cannot smuggle a + /// private leg past the filter). + #[test] + fn names_resolving_only_private_are_refused() { + let addr = |ip: [u8; 4]| std::net::SocketAddr::from((ip, 80u16)); + let err = vet_public_addrs("private.example", vec![addr([127, 0, 0, 1])]).unwrap_err(); + assert!(err.contains("private address ranges"), "{err}"); + let kept = vet_public_addrs( + "rebinding.example", + vec![addr([10, 0, 0, 5]), addr([8, 8, 8, 8])], + ) + .unwrap(); + assert_eq!(kept, vec![addr([8, 8, 8, 8])]); + } + + /// The reserved name `localhost` resolves as the loopback literal it + /// is (RFC 6761): the same reach a `127.0.0.1` literal has covers + /// the name, answered without DNS so no hosts override applies. #[tokio::test] - async fn resolver_blocks_names_resolving_private() { + async fn localhost_resolves_as_the_loopback_literal() { use reqwest::dns::Resolve as _; - let name: reqwest::dns::Name = "localhost".parse().unwrap(); - let result = NonPrivateResolver.resolve(name).await; - assert!(result.is_err(), "localhost must not resolve"); + for spelling in ["localhost", "LOCALHOST"] { + let name: reqwest::dns::Name = spelling.parse().unwrap(); + let addrs: Vec = + NonPrivateResolver.resolve(name).await.unwrap().collect(); + assert!(!addrs.is_empty(), "{spelling} resolves"); + assert!( + addrs.iter().all(|addr| addr.ip().is_loopback()), + "localhost answers loopback only: {addrs:?}" + ); + } } /// Redirect targets get the same literal check as the initial URL: diff --git a/crates/foundation/wire/src/lib.rs b/crates/foundation/wire/src/lib.rs index 3cc41350..8c8c3487 100644 --- a/crates/foundation/wire/src/lib.rs +++ b/crates/foundation/wire/src/lib.rs @@ -371,20 +371,6 @@ pub enum EventMsg { /// a warning instead of this event). turns: u32, }, - /// A reviewed plan was proposed and awaits approval (display routing). - PlanProposed { - /// Proposed plan text for transcript rendering. - text: String, - }, - /// A reviewed plan was approved. - PlanApproved, - /// A durable goal was set (display routing). - GoalSet { - /// Goal objective text for transcript rendering. - objective: String, - }, - /// A durable goal was completed. - GoalCompleted, /// Non-fatal condition worth surfacing. Warning { /// Human-readable warning text. diff --git a/crates/foundation/wire/src/tests/round_trips.rs b/crates/foundation/wire/src/tests/round_trips.rs index 4e208806..9c695922 100644 --- a/crates/foundation/wire/src/tests/round_trips.rs +++ b/crates/foundation/wire/src/tests/round_trips.rs @@ -99,10 +99,6 @@ fn event_tag(msg: &EventMsg) -> &'static str { EventMsg::CompactStarted { .. } => "compact_started", EventMsg::CompactCompleted { .. } => "compact_completed", EventMsg::HistoryRewound { .. } => "history_rewound", - EventMsg::PlanProposed { .. } => "plan_proposed", - EventMsg::PlanApproved => "plan_approved", - EventMsg::GoalSet { .. } => "goal_set", - EventMsg::GoalCompleted => "goal_completed", EventMsg::Warning { .. } => "warning", EventMsg::Error { .. } => "error", EventMsg::TurnCompleted { .. } => "turn_completed", @@ -177,14 +173,6 @@ fn every_event_msg_variant_round_trips_with_its_locked_tag() { // nosemgrep: codacy.yaml.security.hard-coded-tokens EventMsg::CompactCompleted { summary_tokens: 99 }, EventMsg::HistoryRewound { turns: 2 }, - EventMsg::PlanProposed { - text: "plan".to_string(), - }, - EventMsg::PlanApproved, - EventMsg::GoalSet { - objective: "ship".to_string(), - }, - EventMsg::GoalCompleted, EventMsg::Warning { message: "w".to_string(), }, diff --git a/crates/foundation/wire/src/tests/tags.rs b/crates/foundation/wire/src/tests/tags.rs index 95bd9058..896b50f6 100644 --- a/crates/foundation/wire/src/tests/tags.rs +++ b/crates/foundation/wire/src/tests/tags.rs @@ -147,20 +147,6 @@ fn wire_tags_event_msg_are_locked() { "compact_completed", ), (EventMsg::HistoryRewound { turns: 1 }, "history_rewound"), - ( - EventMsg::PlanProposed { - text: "x".to_string(), - }, - "plan_proposed", - ), - (EventMsg::PlanApproved, "plan_approved"), - ( - EventMsg::GoalSet { - objective: "x".to_string(), - }, - "goal_set", - ), - (EventMsg::GoalCompleted, "goal_completed"), ( EventMsg::Warning { message: "w".to_string(), diff --git a/crates/frontends/console/src/chrome/footer.rs b/crates/frontends/console/src/chrome/footer.rs index 9a4d9d0a..566cec86 100644 --- a/crates/frontends/console/src/chrome/footer.rs +++ b/crates/frontends/console/src/chrome/footer.rs @@ -43,10 +43,6 @@ pub fn row1(state: &AppState, tip: Option<&str>, columns: usize) -> String { &format!(" {} {branch}", crate::chrome::symbols::BRANCH), )); } - if let Some(goal) = &state.goal { - let minutes = (goal.since.elapsed().as_secs() / 60).max(1); - line.push_str(&theme.paint(Token::Primary, &format!(" goal ● {minutes}m"))); - } if let Some(tip) = tip { let used = width::width(&line); let tip_text = theme.paint(Token::TextMuted, &format!(" | {tip}")); @@ -168,21 +164,6 @@ mod tests { assert!(!width::strip_ansi(&bare).contains("⎇"), "{bare}"); } - #[test] - fn row1_shows_goal_badge_while_active() { - theme::set(theme::Theme::dark()); - let mut with_goal = state(); - with_goal.goal = Some(crate::state::GoalBadge { - since: std::time::Instant::now(), - }); - let line = row1(&with_goal, None, 120); - let plain = width::strip_ansi(&line); - assert!(plain.contains("goal ●"), "{plain}"); - assert!(plain.contains("1m"), "{plain}"); - let bare = row1(&state(), None, 120); - assert!(!width::strip_ansi(&bare).contains("goal ●"), "{bare}"); - } - #[test] fn row2_shows_context_and_exit_hint() { theme::set(theme::Theme::dark()); diff --git a/crates/frontends/console/src/state.rs b/crates/frontends/console/src/state.rs index e47dd7f3..a2797bf0 100644 --- a/crates/frontends/console/src/state.rs +++ b/crates/frontends/console/src/state.rs @@ -2,7 +2,6 @@ //! streaming phase, context usage, and queued input. use std::path::PathBuf; -use std::time::Instant; /// What the harness is doing right now; drives the input pulse. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -71,13 +70,6 @@ pub struct DialogueEntry { pub text: String, } -/// The active goal, shown as a timed badge in the footer row 1. -#[derive(Debug, Clone)] -pub struct GoalBadge { - /// When the goal was set (this run), for the elapsed timer. - pub since: Instant, -} - /// Mutable UI state snapshot; components read it during rendering. #[derive(Debug, Clone)] pub struct AppState { @@ -126,10 +118,6 @@ pub struct AppState { /// Full user/assistant dialogue for `/export` and `/copy`; unlike /// the transcript this is never trimmed. pub dialogue: Vec, - /// Active goal (GoalSet until GoalCompleted); renders as a timed - /// footer badge. Not restored on resume — the badge only reflects - /// goals seen this run. - pub goal: Option, /// First line of the external status-line command, replacing /// footer row 1 when present. pub status_line: Option, @@ -164,7 +152,6 @@ impl AppState { git_branch: None, update_notice: None, dialogue: Vec::new(), - goal: None, status_line: None, } } diff --git a/crates/frontends/console/src/ui.rs b/crates/frontends/console/src/ui.rs index 5d5cf211..c925be0d 100644 --- a/crates/frontends/console/src/ui.rs +++ b/crates/frontends/console/src/ui.rs @@ -60,7 +60,7 @@ use crate::messages::usage::UsagePanel; use crate::messages::{AssistantMessage, ExpandedFlag, StatusLine, Thinking, UserMessage}; use crate::panes; use crate::slash; -use crate::state::{AppState, GoalBadge, StreamingPhase, TodoEntry, TodoStatus}; +use crate::state::{AppState, StreamingPhase, TodoEntry, TodoStatus}; use crate::theme::{self, Token}; use crate::transcript::Transcript; use crate::welcome::Welcome; @@ -1018,27 +1018,6 @@ impl ConsoleUi { self.apply_rewound(*turns); true } - EventMsg::PlanProposed { text } => { - let text = tui_engine::sanitize::sanitize_terminal(text); - self.push_assistant_message(&text); - true - } - EventMsg::PlanApproved => { - self.push_status("plan approved", false); - true - } - EventMsg::GoalSet { objective } => { - self.state.goal = Some(GoalBadge { - since: Instant::now(), - }); - self.push_status(&format!("goal set: {objective}"), false); - true - } - EventMsg::GoalCompleted => { - self.state.goal = None; - self.push_status("goal completed", false); - true - } EventMsg::Warning { message } => { // Wire messages usually arrive bare, but senders that // prefix already (or pass-through notices) must not diff --git a/crates/frontends/harness/src/exec.rs b/crates/frontends/harness/src/exec.rs index 0b92e0f9..db15164b 100644 --- a/crates/frontends/harness/src/exec.rs +++ b/crates/frontends/harness/src/exec.rs @@ -124,22 +124,6 @@ pub(crate) fn render_event( stderr.push_str(&format!("[history rewound: {turns} turns]\n")); None } - EventMsg::PlanProposed { text } => { - stderr.push_str(&format!("[plan proposed]\n{}\n", clean(text))); - None - } - EventMsg::PlanApproved => { - stderr.push_str("[plan approved]\n"); - None - } - EventMsg::GoalSet { objective } => { - stderr.push_str(&format!("[goal set]\n{}\n", clean(objective))); - None - } - EventMsg::GoalCompleted => { - stderr.push_str("[goal completed]\n"); - None - } EventMsg::Warning { message } => { stderr.push_str(&format!("[warn] {}\n", clean(message))); None diff --git a/crates/infrastructure/base/src/lib.rs b/crates/infrastructure/base/src/lib.rs index cc362a2d..5b98a9c4 100644 --- a/crates/infrastructure/base/src/lib.rs +++ b/crates/infrastructure/base/src/lib.rs @@ -123,6 +123,27 @@ pub fn shell_invocation() -> (String, &'static str) { } } +/// Best-effort display name of the shell [`shell_invocation`] spawns: +/// the `WAVECODE_SHELL` override's file stem when set, else the +/// platform default (`cmd` / `sh`). +/// +/// The system prompt's `Shell:` line reports this name, so the model +/// sees the shell that will actually run its commands rather than the +/// login shell — the same resolution every spawn path uses. +pub fn shell_name() -> String { + let (program, _) = shell_invocation(); + display_shell_name(&program) +} + +/// File stem of a shell program path (`…\cmd.exe` → `cmd`, +/// `/bin/zsh` → `zsh`); a stemless value passes through. +fn display_shell_name(program: &str) -> String { + std::path::Path::new(program) + .file_stem() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| program.to_owned()) +} + /// Build the command that runs `command` as a command string. /// /// One place turns the shared resolution into a spawn, so every @@ -485,6 +506,16 @@ mod tests { use std::sync::Mutex; use std::sync::atomic::AtomicUsize; + /// The prompt's shell name is the file stem of the spawn program: + /// an override's path and the platform default both reduce to the + /// name the model should expect in its own command strings. + #[test] + fn display_shell_name_is_the_program_stem() { + assert_eq!(display_shell_name(r"C:\Windows\system32\cmd.exe"), "cmd"); + assert_eq!(display_shell_name("/bin/zsh"), "zsh"); + assert_eq!(display_shell_name("pwsh"), "pwsh"); + } + /// The blocking-pool runner must keep the runtime worker free: the /// work runs on the blocking pool, so a task scheduled on the same /// worker keeps making progress while it runs. A worker that ran diff --git a/crates/operations/bootstrap/src/environment.rs b/crates/operations/bootstrap/src/environment.rs index 42845136..34892bca 100644 --- a/crates/operations/bootstrap/src/environment.rs +++ b/crates/operations/bootstrap/src/environment.rs @@ -33,24 +33,12 @@ pub fn describe(cwd: &Path) -> String { lines.join("\n") } -/// Best-effort shell name: `$SHELL` basename on Unix, `%COMSPEC%` basename -/// (defaulting to cmd.exe) on Windows, "unknown" when nothing is set. +/// Best-effort shell name: the same resolution every command-spawn path +/// uses (`WAVECODE_SHELL` override, else the platform default), so the +/// prompt describes the shell that will actually run the commands, not +/// the login shell. fn shell_name() -> String { - let var = if cfg!(windows) { "COMSPEC" } else { "SHELL" }; - std::env::var(var) - .ok() - .and_then(|path| { - Path::new(&path) - .file_name() - .map(|name| name.to_string_lossy().into_owned()) - }) - .unwrap_or_else(|| { - if cfg!(windows) { - "cmd.exe".into() - } else { - "unknown".into() - } - }) + infrastructure_base::shell_name() } #[cfg(test)] diff --git a/crates/operations/gateway/src/app_server.rs b/crates/operations/gateway/src/app_server.rs index 3d946306..ff7f106f 100644 --- a/crates/operations/gateway/src/app_server.rs +++ b/crates/operations/gateway/src/app_server.rs @@ -49,6 +49,40 @@ use operations_actor::{AssembleOptions, DEFAULT_IDENTITY, SessionError, SessionS use tokio::sync::{Notify, broadcast, mpsc}; use wavecode_wire::{Event, EventMsg, Op, Submission}; +/// JSON body extractor whose rejections speak the server's error shape. +/// +/// Every failed request — middleware guard, handler validation, and +/// extractor rejection alike — answers with the same JSON `{"error": …}` +/// body, so programmatic clients parse one shape instead of sniffing +/// plain text. The rejection's own status is preserved (400 for a +/// syntax error, 415 for a missing content type, 422 for a body that +/// parses but fails serde). +struct ApiJson(serde_json::Value); + +impl axum::extract::FromRequest for ApiJson { + type Rejection = axum::response::Response; + + async fn from_request( + req: axum::extract::Request, + state: &S, + ) -> Result { + match axum::Json::::from_request(req, state).await { + Ok(Json(value)) => Ok(Self(value)), + Err(rejection) => Err(error_json(rejection.status(), rejection.body_text())), + } + } +} + +/// One error shape for every failed request: the status plus a JSON +/// `{"error": …}` body. +fn error_json(status: StatusCode, message: impl std::fmt::Display) -> axum::response::Response { + ( + status, + Json(serde_json::json!({"error": message.to_string()})), + ) + .into_response() +} + /// Idle TTL after which the reaper closes a served session. /// /// The longest legitimate quiet stretch inside one running turn is bounded @@ -325,7 +359,7 @@ async fn auth_and_host_guard( .and_then(|v| v.to_str().ok()) .is_some_and(|value| constant_time_eq(value, &expected)); if !authorized { - return (StatusCode::UNAUTHORIZED, "missing or invalid bearer token").into_response(); + return error_json(StatusCode::UNAUTHORIZED, "missing or invalid bearer token"); } } if !host_allowed( @@ -334,7 +368,7 @@ async fn auth_and_host_guard( .and_then(|v| v.to_str().ok()) .unwrap_or_default(), ) { - return (StatusCode::FORBIDDEN, "loopback hosts only").into_response(); + return error_json(StatusCode::FORBIDDEN, "loopback hosts only"); } next.run(request).await } @@ -382,7 +416,7 @@ fn parse_decision(value: &serde_json::Value) -> Option, - Json(body): Json, + ApiJson(body): ApiJson, ) -> axum::response::Response { // Capacity first: a refused creation must not pay for an assembly. if state.sessions.lock().await.len() >= state.max_sessions { @@ -497,13 +531,13 @@ async fn list_sessions(State(state): State) -> axum::response::Respons async fn prompt_session( State(state): State, Path(session_id): Path, - Json(body): Json, + ApiJson(body): ApiJson, ) -> axum::response::Response { let Some(text) = body.get("text").and_then(serde_json::Value::as_str) else { - return (StatusCode::BAD_REQUEST, "`text` is required").into_response(); + return error_json(StatusCode::BAD_REQUEST, "`text` is required"); }; if text.is_empty() { - return (StatusCode::BAD_REQUEST, "`text` must not be empty").into_response(); + return error_json(StatusCode::BAD_REQUEST, "`text` must not be empty"); } // Images parse strictly: a malformed entry answers 400 instead of // being silently dropped — a skipped image would otherwise leave the @@ -516,11 +550,10 @@ async fn prompt_session( match serde_json::from_value::(item.clone()) { Ok(image) => images.push(image), Err(_) => { - return ( + return error_json( StatusCode::BAD_REQUEST, "`images` entries must carry string `mime` and `base64` fields", - ) - .into_response(); + ); } } } @@ -539,44 +572,43 @@ async fn prompt_session( async fn answer_approval( State(state): State, Path((session_id, call_id)): Path<(String, String)>, - Json(body): Json, + ApiJson(body): ApiJson, ) -> axum::response::Response { let Some(decision) = parse_decision(&body) else { - return ( + return error_json( StatusCode::BAD_REQUEST, - "`decision` must be allow, always, or deny (with optional reason)".to_string(), - ) - .into_response(); + "`decision` must be allow, always, or deny (with optional reason)", + ); }; let sessions = state.sessions.lock().await; let Some(session) = sessions.get(&session_id) else { - return (StatusCode::NOT_FOUND, "unknown session").into_response(); + return error_json(StatusCode::NOT_FOUND, "unknown session"); }; session.touch(state.started); if session.approvals.decide(&call_id, decision) { StatusCode::OK.into_response() } else { - (StatusCode::NOT_FOUND, "no parked approval for that call id").into_response() + error_json(StatusCode::NOT_FOUND, "no parked approval for that call id") } } async fn answer_question( State(state): State, Path((session_id, call_id)): Path<(String, String)>, - Json(body): Json, + ApiJson(body): ApiJson, ) -> axum::response::Response { let Some(answer) = body.get("answer").and_then(serde_json::Value::as_str) else { - return (StatusCode::BAD_REQUEST, "`answer` is required").into_response(); + return error_json(StatusCode::BAD_REQUEST, "`answer` is required"); }; let sessions = state.sessions.lock().await; let Some(session) = sessions.get(&session_id) else { - return (StatusCode::NOT_FOUND, "unknown session").into_response(); + return error_json(StatusCode::NOT_FOUND, "unknown session"); }; session.touch(state.started); if session.questions.answer(&call_id, answer.to_string()) { StatusCode::OK.into_response() } else { - (StatusCode::NOT_FOUND, "no parked question for that call id").into_response() + error_json(StatusCode::NOT_FOUND, "no parked question for that call id") } } @@ -586,7 +618,7 @@ async fn cancel_session( ) -> axum::response::Response { let sessions = state.sessions.lock().await; let Some(session) = sessions.get(&session_id) else { - return (StatusCode::NOT_FOUND, "unknown session").into_response(); + return error_json(StatusCode::NOT_FOUND, "unknown session"); }; session.touch(state.started); session.interrupt.trigger(); @@ -603,7 +635,7 @@ async fn drop_session( // broadcast sender; the pump exits when the actor goes quiet. StatusCode::OK.into_response() } else { - (StatusCode::NOT_FOUND, "unknown session").into_response() + error_json(StatusCode::NOT_FOUND, "unknown session") } } @@ -632,7 +664,7 @@ async fn stream_events( ) -> axum::response::Response { let sessions = state.sessions.lock().await; let Some(session) = sessions.get(&session_id) else { - return (StatusCode::NOT_FOUND, "unknown session").into_response(); + return error_json(StatusCode::NOT_FOUND, "unknown session"); }; // Subscribing is client liveness: the per-turn stream is opened right // before a prompt, so it must not race the reaper. @@ -739,7 +771,7 @@ async fn submit(state: AppState, session_id: &str, op: Op) -> axum::response::Re let send_result = { let mut sessions = state.sessions.lock().await; let Some(session) = sessions.get_mut(session_id) else { - return (StatusCode::NOT_FOUND, "unknown session").into_response(); + return error_json(StatusCode::NOT_FOUND, "unknown session"); }; session.touch(state.started); session.submissions += 1; @@ -755,11 +787,10 @@ async fn submit(state: AppState, session_id: &str, op: Op) -> axum::response::Re // be retried once the pump drains, so answer 429 (not 409) and // keep the submission rejection visible instead of dropping it. Err(tokio::sync::mpsc::error::TrySendError::Full(_)) => { - let mut response = ( + let mut response = error_json( StatusCode::TOO_MANY_REQUESTS, "session queue is full; retry shortly", - ) - .into_response(); + ); response .headers_mut() .insert("Retry-After", axum::http::HeaderValue::from_static("1")); @@ -767,7 +798,7 @@ async fn submit(state: AppState, session_id: &str, op: Op) -> axum::response::Re } // The pump is gone: the session is closing. Err(tokio::sync::mpsc::error::TrySendError::Closed(_)) => { - (StatusCode::CONFLICT, "session is closing").into_response() + error_json(StatusCode::CONFLICT, "session is closing") } Ok(()) => match reply_rx.await { Ok(Ok(())) => ( @@ -780,7 +811,7 @@ async fn submit(state: AppState, session_id: &str, op: Op) -> axum::response::Re Json(serde_json::json!({"error": e})), ) .into_response(), - Err(_) => (StatusCode::NOT_FOUND, "session closed").into_response(), + Err(_) => error_json(StatusCode::NOT_FOUND, "session closed"), }, } } diff --git a/crates/operations/gateway/src/app_server_tests.rs b/crates/operations/gateway/src/app_server_tests.rs index 1d05383d..ab5f6a89 100644 --- a/crates/operations/gateway/src/app_server_tests.rs +++ b/crates/operations/gateway/src/app_server_tests.rs @@ -237,6 +237,57 @@ async fn healthz_is_open_and_other_routes_require_the_token() { let _ = done; } +/// Every failed request - handler validation, extractor rejection, and +/// the auth guard alike - answers the same JSON `{"error": ...}` shape. +#[tokio::test] +async fn errors_speak_one_json_shape() { + let (token, port, done) = start(vec![]).await; + let http = client(&token); + // Handler validation error. + let bad_prompt = http + .post(server_url(port, "/sessions/s-x/prompt")) + .json(&serde_json::json!({"no_text": true})) + .send() + .await + .unwrap(); + assert_eq!(bad_prompt.status(), StatusCode::BAD_REQUEST); + let body: serde_json::Value = bad_prompt.json().await.unwrap(); + assert!(body["error"].as_str().unwrap().contains("text"), "{body}"); + // Extractor rejection: a malformed body keeps its 400 but speaks + // the JSON shape too. + let malformed = http + .post(server_url(port, "/sessions/s-x/prompt")) + .header("content-type", "application/json") + .body("{not json".to_string()) + .send() + .await + .unwrap(); + assert_eq!(malformed.status(), StatusCode::BAD_REQUEST); + let body: serde_json::Value = malformed.json().await.unwrap(); + assert!(body["error"].is_string(), "{body}"); + // A missing JSON content type keeps its 415 and speaks the shape. + let untyped = http + .post(server_url(port, "/sessions/s-x/prompt")) + .body("{}".to_string()) + .send() + .await + .unwrap(); + assert_eq!(untyped.status(), StatusCode::UNSUPPORTED_MEDIA_TYPE); + let body: serde_json::Value = untyped.json().await.unwrap(); + assert!(body["error"].is_string(), "{body}"); + // Auth guard. + let anonymous = reqwest::Client::new(); + let refused = anonymous + .get(server_url(port, "/sessions")) + .send() + .await + .unwrap(); + assert_eq!(refused.status(), StatusCode::UNAUTHORIZED); + let body: serde_json::Value = refused.json().await.unwrap(); + assert!(body["error"].as_str().unwrap().contains("token"), "{body}"); + let _ = done; +} + /// Every frame on an SSE stream carries the wire `Event` shape — a /// correlation `id` plus the payload — including the one frame the /// server raises itself: a consumer that parses each `data:` payload as diff --git a/crates/operations/gateway/src/mcp_serve.rs b/crates/operations/gateway/src/mcp_serve.rs index 40b681fa..1ff4e901 100644 --- a/crates/operations/gateway/src/mcp_serve.rs +++ b/crates/operations/gateway/src/mcp_serve.rs @@ -217,15 +217,27 @@ fn initialize_result() -> serde_json::Value { } /// `tools/list` result from the registry specs (sorted by name there). +/// +/// Each tool carries MCP `annotations` mirroring the local per-tool +/// effect classification, so a client can style or pre-gate calls on +/// the same signal the local policy gate sees. fn tools_list(registry: &wavecode_tools::Registry) -> serde_json::Value { let tools: Vec = registry .specs() .into_iter() .map(|spec| { + let (read_only, destructive) = registry + .get(&spec.name) + .map(|tool| (tool.is_read_only(), tool.is_destructive())) + .unwrap_or((false, false)); serde_json::json!({ "name": spec.name, "description": spec.description, "inputSchema": spec.input_schema, + "annotations": { + "readOnlyHint": read_only, + "destructiveHint": destructive, + }, }) }) .collect(); @@ -536,6 +548,12 @@ mod tests { let echo = tools.iter().find(|t| t["name"] == "echo_tool").unwrap(); assert_eq!(echo["description"], "test echo tool"); assert_eq!(echo["inputSchema"], serde_json::json!({"type": "object"})); + // MCP annotations mirror the per-tool effect classification: the + // echo tool is read-only and not destructive. + assert_eq!( + echo["annotations"], + serde_json::json!({"readOnlyHint": true, "destructiveHint": false}) + ); } #[tokio::test] diff --git a/sdk/typescript/src/types.ts b/sdk/typescript/src/types.ts index cc63a613..5b0e7ad3 100644 --- a/sdk/typescript/src/types.ts +++ b/sdk/typescript/src/types.ts @@ -107,14 +107,6 @@ export type WireEventPayload = | { type: "compact_completed"; summary_tokens: number } /** A rewind landed: the dropped turns are gone from the history. */ | { type: "history_rewound"; turns: number } - /** A reviewed plan was proposed and awaits approval. */ - | { type: "plan_proposed"; text: string } - /** A reviewed plan was approved. */ - | { type: "plan_approved" } - /** A durable goal was set. */ - | { type: "goal_set"; objective: string } - /** A durable goal was completed. */ - | { type: "goal_completed" } /** Non-fatal condition worth surfacing. */ | { type: "warning"; message: string } /** Fatal condition for this run. */ From 258a7ae8b2537582bfcfcd8bc748fcd2e1063761 Mon Sep 17 00:00:00 2001 From: daftpunkwav Date: Wed, 28 Oct 2026 22:26:15 +0800 Subject: [PATCH 2/3] fix: keep the shell-stem test platform-correct and reformat The Windows-path assertion holds only where backslashes separate; the stem rule itself is cross-platform, so the Windows-shaped input asserts under the platform gate. Plus the fmt pass over the branch. --- crates/infrastructure/base/src/lib.rs | 4 ++++ crates/operations/gateway/src/app_server.rs | 5 +---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/infrastructure/base/src/lib.rs b/crates/infrastructure/base/src/lib.rs index 5b98a9c4..663063c7 100644 --- a/crates/infrastructure/base/src/lib.rs +++ b/crates/infrastructure/base/src/lib.rs @@ -511,6 +511,10 @@ mod tests { /// name the model should expect in its own command strings. #[test] fn display_shell_name_is_the_program_stem() { + // Backslashes separate only on Windows: the stem rule itself is + // cross-platform, so the Windows-shaped input is asserted where + // the shape means what it says. + #[cfg(windows)] assert_eq!(display_shell_name(r"C:\Windows\system32\cmd.exe"), "cmd"); assert_eq!(display_shell_name("/bin/zsh"), "zsh"); assert_eq!(display_shell_name("pwsh"), "pwsh"); diff --git a/crates/operations/gateway/src/app_server.rs b/crates/operations/gateway/src/app_server.rs index ff7f106f..c2dc4cb5 100644 --- a/crates/operations/gateway/src/app_server.rs +++ b/crates/operations/gateway/src/app_server.rs @@ -62,10 +62,7 @@ struct ApiJson(serde_json::Value); impl axum::extract::FromRequest for ApiJson { type Rejection = axum::response::Response; - async fn from_request( - req: axum::extract::Request, - state: &S, - ) -> Result { + async fn from_request(req: axum::extract::Request, state: &S) -> Result { match axum::Json::::from_request(req, state).await { Ok(Json(value)) => Ok(Self(value)), Err(rejection) => Err(error_json(rejection.status(), rejection.body_text())), From edf2fd98523f88d11f4082df969a5e9edaafd21d Mon Sep 17 00:00:00 2001 From: daftpunkwav Date: Thu, 29 Oct 2026 02:03:04 +0800 Subject: [PATCH 3/3] fix: advertise non-read-only tools at the protocol default tools_list copied the local is_destructive attribute straight into MCP's destructiveHint, but the trait default is false for 'unclassified' - so a non-read-only tool like shell was advertised additive-only, less cautious than MCP's protocol default for non-read-only tools (destructiveHint: true). Read-only tools stay additive-only; non-read-only tools now advertise the protocol default. A non-read-only test tool pins the mapping. --- crates/operations/gateway/src/mcp_serve.rs | 53 ++++++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/crates/operations/gateway/src/mcp_serve.rs b/crates/operations/gateway/src/mcp_serve.rs index 1ff4e901..61193a8e 100644 --- a/crates/operations/gateway/src/mcp_serve.rs +++ b/crates/operations/gateway/src/mcp_serve.rs @@ -228,8 +228,18 @@ fn tools_list(registry: &wavecode_tools::Registry) -> serde_json::Value { .map(|spec| { let (read_only, destructive) = registry .get(&spec.name) - .map(|tool| (tool.is_read_only(), tool.is_destructive())) - .unwrap_or((false, false)); + .map(|tool| { + let read_only = tool.is_read_only(); + // MCP defaults `destructiveHint` to true for a + // non-read-only tool. The local attribute only knows + // "explicitly destructive" versus "unclassified" (the + // trait default), so advertising the default false + // would claim additive-only for tools like `shell` + // that can delete. The cautious advertisement is the + // protocol default itself. + (read_only, !read_only) + }) + .unwrap_or((false, true)); serde_json::json!({ "name": spec.name, "description": spec.description, @@ -356,8 +366,36 @@ mod tests { } } + struct DestructiveTool; + struct FaultyTool; + #[async_trait::async_trait] + impl Tool for DestructiveTool { + fn name(&self) -> &str { + "remover_tool" + } + fn description(&self) -> &str { + "test destructive tool" + } + fn input_schema(&self) -> serde_json::Value { + serde_json::json!({"type": "object"}) + } + fn is_read_only(&self) -> bool { + false + } + async fn execute( + &self, + _input: serde_json::Value, + _ctx: &ToolCtx, + ) -> ToolResultAlias { + Ok(ToolOutput { + content: "gone".to_string(), + is_error: false, + }) + } + } + #[async_trait::async_trait] impl Tool for FaultyTool { fn name(&self) -> &str { @@ -416,6 +454,7 @@ mod tests { registry.register(Arc::new(EchoTool)); registry.register(Arc::new(FaultyTool)); registry.register(Arc::new(PrefixSuccessTool)); + registry.register(Arc::new(DestructiveTool)); Arc::new(registry) } @@ -549,11 +588,19 @@ mod tests { assert_eq!(echo["description"], "test echo tool"); assert_eq!(echo["inputSchema"], serde_json::json!({"type": "object"})); // MCP annotations mirror the per-tool effect classification: the - // echo tool is read-only and not destructive. + // echo tool is read-only and therefore advertised additive-only. assert_eq!( echo["annotations"], serde_json::json!({"readOnlyHint": true, "destructiveHint": false}) ); + // A non-read-only tool is advertised at the protocol default + // (potentially destructive), never less cautious than it — even + // though the local attribute leaves it "unclassified". + let faulty = tools.iter().find(|t| t["name"] == "remover_tool").unwrap(); + assert_eq!( + faulty["annotations"], + serde_json::json!({"readOnlyHint": false, "destructiveHint": true}) + ); } #[tokio::test]