diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9238aac1..cee20f77 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -299,6 +299,9 @@ jobs: - uses: jdx/mise-action@v4 - uses: Swatinem/rust-cache@v2 - run: mise build:rs:release + # The authority on "no test seam ships": the release binaries themselves, scanned for the + # seam prefix (a grep over what was just built; seconds). + - run: mise check:release-seams # Mutation testing on what this PR changed: every line the diff touches in the gateway and the # catalog is mutated, and a mutant the tests don't catch fails the job. Cost scales with the diff # (each mutant is a rebuild plus a test run), so the mutants are split across 8 runners: a typical diff --git a/crates/agent/ARCHITECTURE.md b/crates/agent/ARCHITECTURE.md index 68307321..fd91368a 100644 --- a/crates/agent/ARCHITECTURE.md +++ b/crates/agent/ARCHITECTURE.md @@ -4312,7 +4312,7 @@ backend an operator points it at. | `BEYOND_AI_AGENT_TEST_SLOW_SESSION_OPEN_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: makes every `Persistence::open` sleep this many milliseconds, standing in for a slow network-backed session directory. `tests/serve_startup_stall.rs` uses it to prove session start-up is off the shared runtime thread. A release binary contains no latency-injection path at all. | | `AI_AGENT_OAUTH_CALLBACK_HOST` | unset (`127.0.0.1`) | The host the fixed-port OAuth login callback listener (Anthropic 53692, OpenAI Codex 1455) binds. **Loopback only** — an address in `127.0.0.0/8`, `::1`, or `localhost` — because the listener receives an authorization code; anything else (`0.0.0.0`, a routable address, a name) fails the login with a clear error before any bind (`oauth::callback_server::callback_host`). Tests give a login its own loopback address so concurrent runs do not collide on the fixed port. | | `BEYOND_AI_AGENT_TEST_SLOW_CHECKPOINT_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: makes every mid-run checkpoint write in `serve` take this many milliseconds longer, inside the write itself, standing in for a slow disk — so an acknowledgement sent before the write would be caught. `tests/serve_session_lifecycle.rs` uses it to prove a tool does not start before its `tool_use` is on disk. A release binary contains no latency-injection path at all. | -| `BEYOND_AI_AGENT_TEST_FAIL_CHECKPOINT` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: fails `serve`'s checkpoint writes — `all` of them, or only the `n`th (a number), counted per session so a daemon's sessions do not shift each other's count (`serve_session_isolation`: `an_injected_checkpoint_failure_counts_each_sessions_writes`). `tests/serve_session_lifecycle.rs` uses it to prove a failed write before a tool fails closed (the tool never runs) and that a best-effort one is superseded. A release binary contains no fault-injection path and none of these names (`tests/release_seams.rs`). | +| `BEYOND_AI_AGENT_TEST_FAIL_CHECKPOINT` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: fails `serve`'s checkpoint writes — `all` of them, or only the `n`th (a number), counted per session so a daemon's sessions do not shift each other's count (`serve_session_isolation`: `an_injected_checkpoint_failure_counts_each_sessions_writes`). `tests/serve_session_lifecycle.rs` uses it to prove a failed write before a tool fails closed (the tool never runs) and that a best-effort one is superseded. A release binary contains no fault-injection path and none of these names: CI's `build (release)` job scans every release binary for the `BEYOND_AI_AGENT_TEST_` prefix (`mise check:release-seams`, `scripts/scan-release-seams.sh`) and fails on any hit; `tests/release_seams.rs` is the fast source-level warning and pins the scanner. | | `BEYOND_AI_AGENT_TEST_SLOW_SESSION_END_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: delays a failed daemon session's `error` broadcast after its loop has ended. `tests/serve_service_session_end.rs` uses it to prove the reason still reaches the client before the close. A release binary contains no latency-injection path at all. | | `BEYOND_AI_AGENT_TEST_SLOW_ATTACH_MS` | unset (no delay) | **Debug builds only** (`#[cfg(debug_assertions)]`), and only for tests: delays a daemon connection's registration of its output. `tests/serve_service_session_end.rs` uses it to prove a connection registering after its session ended is still told why. A release binary contains no latency-injection path at all. | | `--idle-timeout-ms` / `AI_AGENT_IDLE_TIMEOUT_MS` (`run`-only) | `agent_core::client::READ_TIMEOUT` (~600s) | Task #19 (pi-parity feature): overrides the gateway HTTP client's idle-read timeout between response chunks (`GatewayClient::with_idle_timeout`, previously unused anywhere in this codebase) — matters most for a direct-routed/custom `models.json` `base_url` override, which bypasses the gateway's own upstream-timeout assumption entirely | @@ -4608,7 +4608,9 @@ mcp_events_subscribe (any session) ──► owned by that session the draft treats as "stop"). Before any state is read, a daemon with a callback URL answers every unknown token `503` from its start until each session it starts to restore callbacks (the events session, and every session holding runtime subscriptions) has read its state and held its tokens - (`webhook::restore_pending` / `restored`, also bounded at 10 min); after that an unknown token is + (`webhook::restore_pending` / `restored`, also bounded at 10 min) — or will not: a listed session + that fails to start, ends with an error, starts with no events hub, or panics is released from the + window at once; after that an unknown token is `410` at once. A runtime subscription the server ended is not restored, so its token is not held and its old callback answers `410`. No subscription makes a request — configured, restored after a restart or a panic, or resubscribing — until the session can take the server's questions (its diff --git a/crates/agent/scripts/scan-release-seams.sh b/crates/agent/scripts/scan-release-seams.sh new file mode 100755 index 00000000..51e8bd26 --- /dev/null +++ b/crates/agent/scripts/scan-release-seams.sh @@ -0,0 +1,26 @@ +#!/bin/sh +# Fail if any given build artifact carries a test seam's name. +# +# A test seam (`BEYOND_AI_AGENT_TEST_SLOW_CHECKPOINT_MS`, ...) changes how the binary behaves when an +# environment variable is set: fine in a debug test binary, never in a shipped one. Every seam is +# compiled only under `#[cfg(debug_assertions)]`, so a release binary must not contain the prefix at +# all. This scans the artifact itself: whatever the source looks like (a name assembled with +# `concat!`, one the source checker in `tests/release_seams.rs` misreads), a name the compiler kept +# is a contiguous string in the binary, and this finds it. CI runs it on every release binary the +# `build (release)` job produces (`mise check:release-seams`). +# +# Usage: scan-release-seams.sh ... +set -eu +if [ "$#" -eq 0 ]; then + echo "usage: $0 ..." >&2 + exit 2 +fi +status=0 +for artifact in "$@"; do + if grep -aq 'BEYOND_AI_AGENT_TEST_' "$artifact"; then + echo "$artifact carries test seam names (they must exist only in debug builds):" >&2 + grep -ao 'BEYOND_AI_AGENT_TEST_[A-Z0-9_]*' "$artifact" | sort -u | sed 's/^/ /' >&2 + status=1 + fi +done +exit "$status" diff --git a/crates/agent/src/serve.rs b/crates/agent/src/serve.rs index 7237921e..6a17166d 100644 --- a/crates/agent/src/serve.rs +++ b/crates/agent/src/serve.rs @@ -2931,6 +2931,10 @@ pub(crate) async fn serve_session( // server may raise a nested request during the very first `events/poll`, and asked before // then it would be declined as having no client. mcp_events = Some(hub); + } else { + // No events hub, so nothing of this session's will be restored: the daemon's restore + // window (`mcp_events::restore_pending`) must not wait on it. + crate::tools::mcp_events::restored(persistence.session_id()); } // SEP-2640 skills from every connected server, seen through this session's MCP gate. let mcp_skills = diff --git a/crates/agent/src/serve_ws.rs b/crates/agent/src/serve_ws.rs index bb54e75e..4aaf22aa 100644 --- a/crates/agent/src/serve_ws.rs +++ b/crates/agent/src/serve_ws.rs @@ -529,6 +529,9 @@ fn serve_session_body(base: ServeConfig) -> SessionBody { Box::pin(async move { if let Err(e) = serve_session(cfg, input_rx, out_conn, running, keep_alive).await { eprintln!("serve: session {id} ended: {e}"); + // Failed (to start, or later): whatever it would still have restored, the + // restore window does not wait on it. + crate::tools::mcp_events::restored(&id); simulated_slow_session_end().await; lock_ignoring_poison(&out_err).end(OutFrame::Value(json!({ "type": "error", @@ -1894,10 +1897,14 @@ pub async fn serve_ws( supervisor.unpin(&p.id, p.incarnation); eprintln!("serve: MCP Events session `{id}` owns the configured subscriptions"); } - Err(e) => eprintln!( - "serve: could not start the MCP Events session `{id}`: {}", - HttpError::from(e) - ), + Err(e) => { + eprintln!( + "serve: could not start the MCP Events session `{id}`: {}", + HttpError::from(e) + ); + // It will reserve nothing: the restore window must not wait on it. + crate::tools::mcp_events::restored(&id); + } } } @@ -1915,6 +1922,10 @@ pub async fn serve_ws( let mut backoff: std::collections::HashMap = std::collections::HashMap::new(); while let Some(id) = panicked_rx.recv().await { + // A panicked incarnation reserves nothing more, and one that panics on every start + // never will: the restore window does not wait on it. (Tokens it already reserved + // stay held; a restart reserves its own again.) + crate::tools::mcp_events::restored(&id); let holds_runtime = match &session_dir { Some(dir) => { let dir = dir.clone(); @@ -1974,10 +1985,13 @@ pub async fn serve_ws( supervisor.unpin(&p.id, p.incarnation); eprintln!("serve: restored session `{id}` for its MCP Events subscriptions"); } - Err(e) => eprintln!( - "serve: could not restore session `{id}` for its MCP Events subscriptions: {}", - HttpError::from(e) - ), + Err(e) => { + eprintln!( + "serve: could not restore session `{id}` for its MCP Events subscriptions: {}", + HttpError::from(e) + ); + crate::tools::mcp_events::restored(&id); + } } } } diff --git a/crates/agent/src/tools/mcp_events/mod.rs b/crates/agent/src/tools/mcp_events/mod.rs index 96a04799..0f34d73d 100644 --- a/crates/agent/src/tools/mcp_events/mod.rs +++ b/crates/agent/src/tools/mcp_events/mod.rs @@ -54,7 +54,9 @@ use crate::settings::{McpEventAction, McpEventDelivery, McpEventSubscription}; use crate::tools::mcp::McpCatalog; use state::{PendingEvent, PersistedSub, StateStore}; -pub use webhook::{MAX_WEBHOOK_BODY, WebhookReply, receive_webhook, restore_pending, route_exists}; +pub use webhook::{ + MAX_WEBHOOK_BODY, WebhookReply, receive_webhook, restore_pending, restored, route_exists, +}; pub use wire::NotificationRouter; use wire::{Conn, RpcError, StreamMsg}; diff --git a/crates/agent/src/tools/mcp_events/webhook.rs b/crates/agent/src/tools/mcp_events/webhook.rs index f3a5a2b3..94e9aa85 100644 --- a/crates/agent/src/tools/mcp_events/webhook.rs +++ b/crates/agent/src/tools/mcp_events/webhook.rs @@ -136,9 +136,10 @@ pub fn restore_pending(sessions: impl IntoIterator) { }); } -/// `session` has reserved every token it persisted. Once every session restore was waiting on has, -/// unknown tokens are `410` again. -pub(super) fn restored(session: &str) { +/// `session` has reserved every token it persisted — or will not: it failed to start, ended with no +/// events hub, or panicked. Once every session restore was waiting on is done either way, unknown +/// tokens are `410` again, rather than for the rest of [`RESERVE_FOR`]. +pub fn restored(session: &str) { let mut state = restoring_state(); if let Some(r) = state.as_mut() { r.sessions.remove(session); diff --git a/crates/agent/tests/mcp_events_receiver.rs b/crates/agent/tests/mcp_events_receiver.rs index 0eaa4c18..e8271e63 100644 --- a/crates/agent/tests/mcp_events_receiver.rs +++ b/crates/agent/tests/mcp_events_receiver.rs @@ -700,3 +700,88 @@ async fn a_session_with_runtime_subscriptions_is_restarted_after_a_panic() { ); assert_eq!(daemon_sessions(port).await.get("watcher"), Some(&true)); } + +/// Make a runtime webhook subscription in session `runtime-owner` on a first daemon, wait for its +/// spec and callback on disk, then kill that daemon: what a restart finds to restore. +async fn persist_a_runtime_subscription( + home: &std::path::Path, + base: &str, + held: &HeldPort, + mcp_url: &str, +) { + let mut servers = hooks(mcp_url, "notify"); + servers[0]["events"] = json!([]); + write_settings(home, servers); + let mut first = daemon(home, base, held, &[]); + let mut ws = ws_connect(held.port(), Some("runtime-owner")).await; + ws_send( + &mut ws, + json!({ "type": "mcp_events_subscribe", "id": "s", "server": "hooks", "name": "ticket.updated", + "delivery": "webhook", "action": "notify" }), + ) + .await; + let r = ws_next(&mut ws, Duration::from_secs(20), "the subscribe", |f| { + f["type"] == "response" && f["id"] == "s" + }) + .await; + assert_eq!(r["success"], true, "{r:#}"); + eventually( + Duration::from_secs(10), + "the runtime spec and its callback on disk", + || { + std::fs::read_dir(home.join("sessions")) + .ok()? + .map(|e| e.unwrap().path()) + .filter(|p| p.to_string_lossy().ends_with(".mcp-events.json")) + .any(|p| { + let text = std::fs::read_to_string(p).unwrap_or_default(); + text.contains("\"runtime\"") && text.contains("\"token\"") + }) + .then_some(()) + }, + ); + drop(ws); + first.kill().unwrap(); + let _ = first.wait(); +} + +/// The restore window must not wait on a session that will never reserve anything. Here the +/// session holding the runtime subscription gets no events hub on the restart (its server is no +/// longer configured): a token nothing holds is `410` again within moments, not after the window's +/// ten-minute bound. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_restoring_session_with_no_events_hub_closes_the_restore_window() { + let (_fx, mcp_url, _fixture) = spawn_http_fixture(&[("MCP_FIXTURE_ALLOW_HTTP_CALLBACK", "1")]); + let home = tempfile::tempdir().unwrap(); + let (base, _bodies) = spawn_model_server_routed(vec![], turn_text("noted")); + let held = HeldPort::bind(); + persist_a_runtime_subscription(home.path(), &base, &held, &mcp_url).await; + write_settings(home.path(), json!([])); + let _second = daemon(home.path(), &base, &held, &[]); + eventually(Duration::from_secs(10), "a dead token answered 410", || { + (dead_token_status(held.port()) == 410).then_some(()) + }); +} + +/// Likewise for a listed session that fails to start (here its transcript no longer opens). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_restoring_session_that_fails_to_start_closes_the_restore_window() { + let (_fx, mcp_url, _fixture) = spawn_http_fixture(&[("MCP_FIXTURE_ALLOW_HTTP_CALLBACK", "1")]); + let home = tempfile::tempdir().unwrap(); + let (base, _bodies) = spawn_model_server_routed(vec![], turn_text("noted")); + let held = HeldPort::bind(); + persist_a_runtime_subscription(home.path(), &base, &held, &mcp_url).await; + let transcript = std::fs::read_dir(home.path().join("sessions")) + .unwrap() + .map(|e| e.unwrap().path()) + .find(|p| { + let name = p.file_name().unwrap().to_string_lossy().into_owned(); + name.ends_with("runtime-owner.jsonl") + }) + .expect("the session's transcript"); + std::fs::write(&transcript, b"not a session file\n").unwrap(); + let _second = daemon(home.path(), &base, &held, &[]); + eventually(Duration::from_secs(10), "a dead token answered 410", || { + (dead_token_status(held.port()) == 410).then_some(()) + }); +} diff --git a/crates/agent/tests/release_seams.rs b/crates/agent/tests/release_seams.rs index 2ef3c6f2..4072fa74 100644 --- a/crates/agent/tests/release_seams.rs +++ b/crates/agent/tests/release_seams.rs @@ -5,7 +5,13 @@ //! shipped one. Each lives in a function (or a match arm) under `#[cfg(debug_assertions)]`, with a //! no-op twin for release, so a release binary has no injection path and carries none of the names. //! A name written at a call site instead — `delay("BEYOND_AI_AGENT_TEST_…")` into a gated helper — -//! still compiles into release as a string the binary carries; this test fails on that. +//! still compiles into release as a string the binary carries. +//! +//! **The authority is the release binary itself**: CI's `build (release)` job runs +//! `scripts/scan-release-seams.sh` on every release binary it built (`mise check:release-seams`), +//! and fails on any `BEYOND_AI_AGENT_TEST_` string the compiler kept, however the source spelled it +//! (`concat!`, a `//` inside a string literal, anything a text heuristic misreads). The source check +//! here is only the fast early warning; the scanner's own behaviour is pinned below. #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] /// Where a seam name may appear: in a function, or a match arm, marked `#[cfg(debug_assertions)]`. @@ -146,3 +152,40 @@ async fn body() { "; assert_eq!(ungated_seam_names("f.rs", gated_neighbour).len(), 1); } + +/// The artifact scanner CI trusts: it fails on a binary that carries seam names — this test's own +/// debug build of the agent does, by design — and passes one that does not. A name assembled at +/// compile time (`concat!`) is a contiguous string in the artifact, so it is caught the same way. +#[test] +fn the_release_artifact_scan_fails_on_any_seam_name() { + let script = + std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("scripts/scan-release-seams.sh"); + let scan = |artifact: &std::path::Path| { + std::process::Command::new("sh") + .arg(&script) + .arg(artifact) + .output() + .unwrap() + }; + let debug = scan(std::path::Path::new(env!("CARGO_BIN_EXE_beyond-ai-agent"))); + assert_eq!(debug.status.code(), Some(1), "{debug:?}"); + assert!( + String::from_utf8_lossy(&debug.stderr).contains("BEYOND_AI_AGENT_TEST_SLOW_CHECKPOINT_MS"), + "{debug:?}" + ); + let dir = tempfile::tempdir().unwrap(); + let clean = dir.path().join("clean"); + std::fs::write( + &clean, + b"\x7fELF BEYOND_AI_AGENT_MCP_IDLE_SECS nothing to see", + ) + .unwrap(); + assert_eq!(scan(&clean).status.code(), Some(0)); + let assembled = dir.path().join("assembled"); + std::fs::write( + &assembled, + concat!("\x7fELF ", "BEYOND_AI_AGENT_", "TEST_", "SLOW_X_MS").as_bytes(), + ) + .unwrap(); + assert_eq!(scan(&assembled).status.code(), Some(1)); +} diff --git a/mise.toml b/mise.toml index e28a8243..97da9d75 100644 --- a/mise.toml +++ b/mise.toml @@ -18,6 +18,10 @@ run = "cargo build" [tasks."build:rs:release"] run = "cargo build --release" +[tasks."check:release-seams"] +description = "Fail if a release binary carries a test seam's name (run after build:rs:release)." +run = "crates/agent/scripts/scan-release-seams.sh $(find target/release -maxdepth 1 -type f -perm -u+x)" + [tasks."ai:mint-dev-key"] description = "Mint a deterministic DEV-ONLY signing pubkey + bai_v1 token (fixed seed) for wiring up the dev config. Not for production." run = "cargo run -p beyond-ai --example mint_dev_key"