diff --git a/.github/workflows/_ci-relay.yml b/.github/workflows/_ci-relay.yml index 2f410dadf93..3e8c03ec5cd 100644 --- a/.github/workflows/_ci-relay.yml +++ b/.github/workflows/_ci-relay.yml @@ -484,6 +484,19 @@ jobs: VALUES ('00000000-0000-4000-8000-00000000c0de', 'localhost:3000') ON CONFLICT (lower(host)) DO NOTHING ;" + - name: Git default-branch route and clone regressions + env: + # The preceding step already applied and reconciled schema/schema.sql. + BUZZ_TEST_SCHEMA_MODE: desired + BUZZ_TEST_DATABASE_URL: postgres://buzz:${{ env.BUZZ_TEST_POSTGRES_PASSWORD }}@localhost:5432/buzz + BUZZ_TEST_REDIS_URL: redis://localhost:6379 + BUZZ_TEST_S3_ENDPOINT: http://localhost:9000 + BUZZ_TEST_S3_BUCKET: buzz-media + run: | + cargo nextest run \ + --archive-file target/ci/backend-integration-tests.tar.zst \ + -E 'package(buzz-relay) and test(/api::git::settings::tests::external_infra::/)' \ + --run-ignored ignored-only - name: Workflow message provenance unit tests # The relay's workflow_sink suite is not selected by the infra-free # unit job. Its ignored database cases run in the isolated PostgreSQL diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 41d9a214bdd..3d011eb8ebb 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -270,6 +270,10 @@ Forum event kinds: 4. **Prompting** — When events are pending and no prompt is in flight for that channel, drains all queued events for the oldest channel into a single batched prompt via ACP `session/prompt`. 5. **Agent response** — The agent processes the prompt and uses the Buzz CLI (`send_message`, `get_messages`, etc.) to interact with Buzz. 6. **Recovery** — If the agent crashes, the harness respawns it. If the relay disconnects, the harness reconnects with a `since` filter to avoid missing events. + If the inbound queue overflows, the harness attempts replay for affected + subscriptions when capacity and relay quota permit, with at least five seconds + between attempts. Recovery depends on available relay history and the consumer + making progress; complete delivery is not guaranteed. Each channel has at most one prompt in flight. Multiple channels can be processed concurrently when agents > 1. diff --git a/crates/buzz-acp/TESTING.md b/crates/buzz-acp/TESTING.md new file mode 100644 index 00000000000..8c8fcbee49a --- /dev/null +++ b/crates/buzz-acp/TESTING.md @@ -0,0 +1,58 @@ +# Pi adapter integration + +Buzz's Pi preset uses [salman1993/pi-acp](https://github.com/salman1993/pi-acp). +Requires Node.js 22 or newer. Install Pi and configure its model provider, +then install the adapter directly from the fork: + +```sh +npm install -g @earendil-works/pi-coding-agent +pi +npm install -g --install-links=true git+https://github.com/salman1993/pi-acp.git#main +``` + +Restart Buzz, then select **Pi** as the agent harness. Buzz starts `buzz-pi-acp` +automatically. Run the same adapter install command again to update it. +The unscoped `npm install -g pi-acp` command installs the upstream package, +without these extensions. Use fresh sessions to replace old user-framed +standing instructions. + +Buzz adds `-- --skill /.agents/skills` when launching `buzz-pi-acp`. +An existing separator and explicit Pi options are preserved. Managed agents +run from the Buzz nest (normally `~/.buzz`): Desktop sets the `buzz-acp` child +CWD through `default_agent_workdir()`, and adapters inherit it. The default +skill directory is that launch workspace's `.agents/skills`. Standalone CLI +launches use the caller's working directory. +The path is fixed at adapter launch and applies to every Pi subprocess. + +The full composed session prompt is sent as a replacement string through the +provisional `session/new.params._meta.systemPrompt` field. Buzz recognizes +`buzz-pi-acp` by the agent name returned during initialization, regardless of +protocol version. Upstream `pi-acp` does not receive this fork-specific metadata. +No custom capability negotiation or legacy Pi prompt fallback is used. +Session titles continue to use `_meta.sessionTitle`. + +## Validation + +Activate Hermit from the Buzz repository root, then run the package tests: + +```sh +. ./bin/activate-hermit +cargo test -p buzz-acp +``` + +To exercise the real adapter through Buzz's production session composer: + +```sh +BUZZ_TEST_PI_ACP=/absolute/pi-acp/dist/index.js \ + cargo test -p buzz-acp real_pi_preserves -- --ignored +``` + +This test requires Node and Pi on PATH. It isolates HOME and Pi settings, +disables extensions and context files, and uses a synthetic transcript without +model calls. It inspects Pi's effective prompt through RPC HTML export after +switching sessions and restarting the adapter. Base, persona, team, core memory, +huddle, canvas, and the extra skill must each appear once, without another +session's instructions or Pi's default coding preamble. + +HTML export reports the exporting process's current system prompt. It cannot +recover a historical prompt from an old transcript alone. diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 0d87bca028c..cd049830688 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -22,6 +22,9 @@ use crate::usage::{ /// Lines exceeding this limit are rejected to prevent OOM from rogue agents. const MAX_LINE_SIZE: usize = 10_000_000; // 10 MB +/// Package and binary name used by Buzz's Pi ACP fork. +pub(crate) const BUZZ_PI_ACP_NAME: &str = "buzz-pi-acp"; + /// An MCP server configuration passed to `session/new`. /// /// Corresponds to the `McpServerStdio` variant in the ACP schema. @@ -460,8 +463,17 @@ impl AcpClient { use std::process::Stdio; let mut cmd = tokio::process::Command::new(command); - cmd.args(args) - .stdin(Stdio::piped()) + cmd.args(args); + if crate::config::normalize_agent_command_identity(command) == BUZZ_PI_ACP_NAME { + if !args.iter().any(|arg| arg == "--") { + cmd.arg("--"); + } + // Desktop launches buzz-acp in the Buzz nest; adapters inherit that + // workspace. Keep managed skills tied to launch CWD across sessions. + cmd.arg("--skill") + .arg(std::env::current_dir()?.join(".agents/skills")); + } + cmd.stdin(Stdio::piped()) .stdout(Stdio::piped()) // Inherit stderr so agent logs are visible in the harness terminal. .stderr(Stdio::inherit()) @@ -637,14 +649,16 @@ impl AcpClient { /// /// - `None` — no system-prompt field in the request (legacy framing). /// - `Some(SystemPromptTransport::Field(text))` — bare `systemPrompt` field - /// (ACP protocol v2, buzz-agent, goose unused). + /// (ACP protocol v2, buzz-agent; goose unused). + /// - `Some(SystemPromptTransport::PiMeta(text))` — `_meta.systemPrompt` + /// as a replacement string for the Buzz pi-acp fork. /// - `Some(SystemPromptTransport::ClaudeMeta(text))` — `_meta.systemPrompt` /// as `{"append": text}`, keeping claude-agent-acp's native preset intact. /// /// `session_title` rides in `_meta.sessionTitle` when `Some`; `_meta` is /// omitted entirely otherwise, since adapters may distinguish an absent - /// member from a null one. When both `ClaudeMeta` and `session_title` are - /// present the two `_meta` members are merged into a single object. + /// member from a null one. Metadata prompt transports and the title are + /// merged into a single object. /// /// Callers use [`extract_model_config_options`] and [`extract_model_state`] /// to pull model info from the raw result. @@ -663,6 +677,9 @@ impl AcpClient { Some(SystemPromptTransport::Field(sp)) => { params["systemPrompt"] = serde_json::Value::String(sp.to_owned()); } + Some(SystemPromptTransport::PiMeta(sp)) => { + params["_meta"]["systemPrompt"] = serde_json::Value::String(sp.to_owned()); + } Some(SystemPromptTransport::ClaudeMeta(sp)) => { // Merge into _meta so sessionTitle (set below) is not clobbered. params["_meta"]["systemPrompt"] = serde_json::json!({ "append": sp }); @@ -670,7 +687,7 @@ impl AcpClient { None => {} } if let Some(title) = session_title { - // Merge — _meta may already carry systemPrompt from ClaudeMeta above. + // Merge — _meta may already carry a system prompt from an adapter extension. params["_meta"]["sessionTitle"] = serde_json::Value::String(title.to_owned()); } let result = self.send_request("session/new", params).await?; @@ -2131,9 +2148,9 @@ pub struct SessionNewResponse { /// How to deliver a system prompt on `session/new`. /// -/// The two variants match the two mechanisms supported by current adapters: -/// /// - **`Field`** — bare `systemPrompt` field (ACP protocol v2, buzz-agent). +/// - **`PiMeta`** — `_meta.systemPrompt: text`, used by the Buzz pi-acp fork +/// to replace Pi's native system prompt. /// - **`ClaudeMeta`** — `_meta.systemPrompt: {"append": text}`, used by /// `claude-agent-acp` to append to the adapter's own native system prompt /// while keeping its tool-use preset intact. @@ -2141,6 +2158,8 @@ pub struct SessionNewResponse { pub enum SystemPromptTransport<'a> { /// Deliver as a bare top-level `systemPrompt` field. Field(&'a str), + /// Deliver as `_meta.systemPrompt: text`. + PiMeta(&'a str), /// Deliver as `_meta.systemPrompt: {"append": text}`. ClaudeMeta(&'a str), } @@ -3631,123 +3650,7 @@ mod tests { // ── claude-agent-acp _meta.systemPrompt transport ───────────────────── - #[tokio::test] - async fn session_new_full_sends_claude_meta_system_prompt_when_claude_meta_transport() { - // When ClaudeMeta transport is requested, the prompt must appear as - // _meta.systemPrompt: {"append": text} — never as a bare systemPrompt field. - let script = r#" - read -t 2 _init - echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' - read -t 2 REQ - echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_claude","_receivedRequest":'"$REQ"'}}' - sleep 1 - "#; - let mut client = spawn_script(script).await; - client - .initialize() - .await - .expect("initialize should succeed"); - - let resp = client - .session_new_full( - "/tmp", - vec![], - Some(SystemPromptTransport::ClaudeMeta("Be concise")), - None, - ) - .await - .expect("session_new_full should succeed"); - - let received = &resp.raw["_receivedRequest"]; - assert!( - received["params"].get("systemPrompt").is_none(), - "bare systemPrompt must not be present for ClaudeMeta transport" - ); - assert_eq!( - received["params"]["_meta"]["systemPrompt"]["append"].as_str(), - Some("Be concise"), - "_meta.systemPrompt.append must carry the prompt text" - ); - } - - #[tokio::test] - async fn session_new_full_merges_claude_meta_and_session_title_into_single_meta_object() { - // Both ClaudeMeta prompt and session_title must coexist under _meta — - // the prompt must not clobber sessionTitle or vice versa. - let script = r#" - read -t 2 _init - echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' - read -t 2 REQ - echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_merged","_receivedRequest":'"$REQ"'}}' - sleep 1 - "#; - let mut client = spawn_script(script).await; - client - .initialize() - .await - .expect("initialize should succeed"); - - let resp = client - .session_new_full( - "/tmp", - vec![], - Some(SystemPromptTransport::ClaudeMeta("Be concise")), - Some("Fizz · #buzz-dev"), - ) - .await - .expect("session_new_full should succeed"); - - let received = &resp.raw["_receivedRequest"]; - assert_eq!( - received["params"]["_meta"]["systemPrompt"]["append"].as_str(), - Some("Be concise"), - "_meta.systemPrompt.append must be present" - ); - assert_eq!( - received["params"]["_meta"]["sessionTitle"].as_str(), - Some("Fizz · #buzz-dev"), - "_meta.sessionTitle must be present alongside systemPrompt" - ); - } - - // ── Goose-native steer scaffold (PR follow-up to #1160) ────────────── - - /// Helper: spawn an inert `cat` subprocess so we have a real AcpClient - /// to drive `handle_session_update` against. `cat` never writes back, - /// which is fine — these tests don't read from the agent, they just - /// feed JSON into the parser. - async fn spawn_inert_client() -> AcpClient { - AcpClient::spawn("cat", &[], &[], false) - .await - .expect("spawn cat as inert client") - } - - /// Build a `session/update` JSON-RPC notification carrying a - /// `session_info_update` with the given `_meta.goose.activeRunId` value. - /// Pass `None` to omit the `activeRunId` field entirely. - /// - /// `_meta` is nested inside the `update` object (per the ACP - /// `SessionInfoUpdate` schema), matching what goose and buzz-agent - /// emit on the wire. - fn session_info_update_msg(active_run_id: Option) -> serde_json::Value { - let mut goose = serde_json::Map::new(); - if let Some(v) = active_run_id { - goose.insert("activeRunId".to_string(), v); - } - let mut meta = serde_json::Map::new(); - meta.insert("goose".to_string(), serde_json::Value::Object(goose)); - serde_json::json!({ - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": "test-session", - "update": { - "sessionUpdate": "session_info_update", - "_meta": serde_json::Value::Object(meta), - }, - } - }) - } + include!("acp/system_prompt_tests.rs"); #[tokio::test] async fn active_run_id_sets_on_string() { diff --git a/crates/buzz-acp/src/acp/system_prompt_tests.rs b/crates/buzz-acp/src/acp/system_prompt_tests.rs new file mode 100644 index 00000000000..b90893e8f50 --- /dev/null +++ b/crates/buzz-acp/src/acp/system_prompt_tests.rs @@ -0,0 +1,154 @@ +#[tokio::test] +async fn session_new_full_sends_pi_replacement_prompt_in_meta() { + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 REQ + echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_pi","_receivedRequest":'"$REQ"'}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let resp = client + .session_new_full( + "/tmp", + vec![], + Some(SystemPromptTransport::PiMeta("Buzz instructions")), + Some("Pi · #buzz-dev"), + ) + .await + .expect("session_new_full should succeed"); + + let received = &resp.raw["_receivedRequest"]; + assert!(received["params"].get("systemPrompt").is_none()); + assert_eq!( + received["params"]["_meta"]["systemPrompt"].as_str(), + Some("Buzz instructions") + ); + assert_eq!( + received["params"]["_meta"]["sessionTitle"].as_str(), + Some("Pi · #buzz-dev") + ); +} + +#[tokio::test] +async fn session_new_full_sends_claude_meta_system_prompt_when_claude_meta_transport() { + // When ClaudeMeta transport is requested, the prompt must appear as + // _meta.systemPrompt: {"append": text} — never as a bare systemPrompt field. + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 REQ + echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_claude","_receivedRequest":'"$REQ"'}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let resp = client + .session_new_full( + "/tmp", + vec![], + Some(SystemPromptTransport::ClaudeMeta("Be concise")), + None, + ) + .await + .expect("session_new_full should succeed"); + + let received = &resp.raw["_receivedRequest"]; + assert!( + received["params"].get("systemPrompt").is_none(), + "bare systemPrompt must not be present for ClaudeMeta transport" + ); + assert_eq!( + received["params"]["_meta"]["systemPrompt"]["append"].as_str(), + Some("Be concise"), + "_meta.systemPrompt.append must carry the prompt text" + ); +} + +#[tokio::test] +async fn session_new_full_merges_claude_meta_and_session_title_into_single_meta_object() { + // Both ClaudeMeta prompt and session_title must coexist under _meta — + // the prompt must not clobber sessionTitle or vice versa. + let script = r#" + read -t 2 _init + echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}' + read -t 2 REQ + echo '{"jsonrpc":"2.0","id":1,"result":{"sessionId":"ses_merged","_receivedRequest":'"$REQ"'}}' + sleep 1 + "#; + let mut client = spawn_script(script).await; + client + .initialize() + .await + .expect("initialize should succeed"); + + let resp = client + .session_new_full( + "/tmp", + vec![], + Some(SystemPromptTransport::ClaudeMeta("Be concise")), + Some("Fizz · #buzz-dev"), + ) + .await + .expect("session_new_full should succeed"); + + let received = &resp.raw["_receivedRequest"]; + assert_eq!( + received["params"]["_meta"]["systemPrompt"]["append"].as_str(), + Some("Be concise"), + "_meta.systemPrompt.append must be present" + ); + assert_eq!( + received["params"]["_meta"]["sessionTitle"].as_str(), + Some("Fizz · #buzz-dev"), + "_meta.sessionTitle must be present alongside systemPrompt" + ); +} + +// ── Goose-native steer scaffold (PR follow-up to #1160) ────────────── + +/// Helper: spawn an inert `cat` subprocess so we have a real AcpClient +/// to drive `handle_session_update` against. `cat` never writes back, +/// which is fine — these tests don't read from the agent, they just +/// feed JSON into the parser. +async fn spawn_inert_client() -> AcpClient { + AcpClient::spawn("cat", &[], &[], false) + .await + .expect("spawn cat as inert client") +} + +/// Build a `session/update` JSON-RPC notification carrying a +/// `session_info_update` with the given `_meta.goose.activeRunId` value. +/// Pass `None` to omit the `activeRunId` field entirely. +/// +/// `_meta` is nested inside the `update` object (per the ACP +/// `SessionInfoUpdate` schema), matching what goose and buzz-agent +/// emit on the wire. +fn session_info_update_msg(active_run_id: Option) -> serde_json::Value { + let mut goose = serde_json::Map::new(); + if let Some(v) = active_run_id { + goose.insert("activeRunId".to_string(), v); + } + let mut meta = serde_json::Map::new(); + meta.insert("goose".to_string(), serde_json::Value::Object(goose)); + serde_json::json!({ + "jsonrpc": "2.0", + "method": "session/update", + "params": { + "sessionId": "test-session", + "update": { + "sessionUpdate": "session_info_update", + "_meta": serde_json::Value::Object(meta), + }, + } + }) +} diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index d67d46d9a73..285c8c4db21 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -5,7 +5,6 @@ mod config; mod engram_fetch; mod filter; mod observer; -mod pi_launcher; mod pool; mod pool_lifecycle; mod prompt_framing; @@ -2746,49 +2745,6 @@ async fn tokio_main() -> Result<()> { tracing::info!("buzz-acp starting: {}", config.summary()); - let cwd = current_working_directory()?; - let base_prompt_content = config.base_prompt_content.take(); - let base_prompt = if config.no_base_prompt { - None - } else { - // Build standing context once under the configured policy, before any - // agent process starts. Pi consumes this through its native - // `--system-prompt`; other ACP agents consume the same bytes through - // session/new or legacy first-turn framing. - Some( - config.session_policy.append_session_model( - base_prompt_content - .as_deref() - .unwrap_or(include_str!("base_prompt.md")), - ), - ) - }; - // PI_ACP_PI_COMMAND is Buzz-owned. Strip stale/user-provided copies from - // every adapter before optionally installing Buzz's generated Pi launcher. - config - .persona_env_vars - .retain(|(key, _)| !key.eq_ignore_ascii_case(pi_launcher::PI_ACP_PI_COMMAND_ENV)); - let managed_skills_dir = std::path::Path::new(&cwd).join(".agents/skills"); - let inherited_pi_command_is_set = - std::env::var_os(pi_launcher::PI_ACP_PI_COMMAND_ENV).is_some(); - let (pi_launch_override, base_prompt) = pi_launcher::PiLaunchOverride::prepare( - &config.agent_command, - base_prompt, - &managed_skills_dir, - inherited_pi_command_is_set, - ) - .context("failed to prepare Pi launch overrides")?; - if let Some(prepared) = pi_launch_override.as_ref() { - config.persona_env_vars.push(( - pi_launcher::PI_ACP_PI_COMMAND_ENV.to_string(), - prepared.launcher_path().to_string_lossy().into_owned(), - )); - tracing::info!( - skills_dir = %managed_skills_dir.display(), - "configured Pi to consume Buzz standing context and managed skills through native CLI flags" - ); - } - let observer = config .relay_observer .then(observer::ObserverHandle::in_process); @@ -3037,6 +2993,8 @@ async fn tokio_main() -> Result<()> { ); } + let base_prompt_content = config.base_prompt_content.take(); + let cwd = current_working_directory()?; let ctx = Arc::new(PromptContext { mcp_servers: build_mcp_servers(&config), initial_message: config.initial_message.clone(), @@ -3047,7 +3005,20 @@ async fn tokio_main() -> Result<()> { system_prompt: config.system_prompt.clone(), session_title: config.session_title.clone(), team_instructions: config.team_instructions.clone(), - base_prompt, + base_prompt: if config.no_base_prompt { + None + } else { + // Build standing context once under the configured policy, before + // any session/new. Both modern ACP and legacy first-turn framing + // consume this same assembled base (including custom base files). + Some( + config.session_policy.append_session_model( + base_prompt_content + .as_deref() + .unwrap_or(include_str!("base_prompt.md")), + ), + ) + }, heartbeat_prompt: config.heartbeat_prompt.clone(), cwd, rest_client: relay.rest_client(), @@ -4401,10 +4372,6 @@ async fn tokio_main() -> Result<()> { // for the background task to finish, rather than aborting immediately (#40). relay.shutdown().await; - // Pi may restore subprocesses throughout the pool lifetime. Remove its - // private prompt and launcher only after every adapter has shut down. - drop(pi_launch_override); - tracing::info!("buzz-acp stopped"); Ok(()) } @@ -5018,6 +4985,23 @@ fn handle_prompt_result( } else { hard_timeout_fate_suffix = Some(" — requeued for retry (recently active)"); } + } else if matches!( + &result.outcome, + PromptOutcome::Error(acp::AcpError::AgentError { code: -32002, message }) + if message.contains("model not found") + ) { + // Retrying the same missing model cannot repair its configuration. + tracing::warn!( + channel_id = %batch.channel_id, + events = batch.events.len(), + "dead-lettering batch immediately — model not found" + ); + let content = "⚠️ I couldn't process the last request: the configured model \ + wasn't found at the provider's endpoint. Open agent settings, select a \ + different model from the dropdown, and save your changes. Restart the agent \ + to apply the new configuration, then re-send your request." + .to_string(); + spawn_failure_notice(rest_client, &batch, content); } else if matches!(&result.outcome, PromptOutcome::Error(e) if is_auth_error(e)) { // Auth errors are non-retryable: the token won't self-repair // between retries, so requeueing only wastes attempt slots and @@ -11625,10 +11609,192 @@ mod error_outcome_emission_tests { ); } + #[tokio::test] + async fn model_not_found_posts_recovery_notice_without_retrying() { + use tokio::io::{AsyncBufReadExt, AsyncReadExt, AsyncWriteExt, BufReader}; + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let rest = relay::RestClient { + http: reqwest::Client::new(), + base_url: format!("http://{}", listener.local_addr().unwrap()), + keys: Keys::generate(), + auth_tag_json: None, + }; + let keys = Keys::generate(); + let root = nostr::EventId::from_byte_array([0xaa; 32]); + let parent = nostr::EventId::from_byte_array([0xbb; 32]); + let event = EventBuilder::new(Kind::Custom(9), "test") + .tags([ + nostr::Tag::parse(["e", &root.to_hex(), "", "root"]).unwrap(), + nostr::Tag::parse(["e", &parent.to_hex(), "", "reply"]).unwrap(), + ]) + .sign_with_keys(&keys) + .unwrap(); + let channel_id = uuid::Uuid::new_v4(); + let batch = FlushBatch { + channel_id, + scope: scope::SessionScope::Conversation { channel_id }, + events: vec![BatchEvent { + event, + prompt_tag: "test".into(), + received_at: std::time::Instant::now(), + }], + cancelled_events: vec![], + cancel_reason: None, + }; + + let raw_error = r#"llm model not found: (gpt-6-astra) 404 Not Found: {"error_code":"NOT_FOUND","message":"'gpt-6-astra' does not exist."}"#; + let model_error = AcpError::AgentError { + code: -32002, + message: raw_error.to_string(), + }; + let expected_error = model_error.to_string(); + let observer = ObserverHandle::in_process(); + + let agent = dummy_agent(0).await; + let mut pool = AgentPool::from_slots(vec![None]); + let task_id = pool.join_set.spawn(async {}).id(); + pool.task_map_mut().insert( + task_id, + crate::pool::TaskMeta { + agent_index: 0, + channel_id: None, + scope: None, + turn_id: "test-turn-id".to_string(), + recoverable_batch: None, + control_tx: None, + steer_tx: None, + successful_steer_deliveries: HashSet::new(), + }, + ); + let mut queue = EventQueue::new(config::DedupMode::Queue); + let config = test_config(); + let mut heartbeat_in_flight = false; + let removed_channels = std::collections::HashSet::new(); + let mut crash_history = vec![SlotCircuit { + crash_times: Vec::new(), + open_until: None, + respawn_in_flight: false, + }]; + let (respawn_tx, _respawn_rx) = mpsc::channel(8); + let mut respawn_tasks = tokio::task::JoinSet::new(); + let result = PromptResult { + agent, + source: PromptSource::Channel(scope::SessionScope::Conversation { channel_id }), + turn_id: "test-turn-id".to_string(), + outcome: PromptOutcome::Error(model_error), + batch: Some(batch), + }; + handle_prompt_result( + &mut pool, + &mut queue, + &config, + result, + &mut heartbeat_in_flight, + &removed_channels, + &mut crash_history, + &respawn_tx, + &mut respawn_tasks, + Some(observer.clone()), + Some(&rest), + ); + + // The batch must not be requeued: pending_channels returns 0. + assert_eq!( + queue.pending_channels(), + 0, + "model-not-found must stop immediately — batch must not be requeued" + ); + assert_eq!( + queue.queued_event_count(channel_id), + 0, + "model-not-found must stop immediately — no events should be pending" + ); + + assert!( + pool.agents_mut()[0].is_some(), + "healthy process remains reusable" + ); + assert!(respawn_tasks.is_empty()); + let errors: Vec<_> = observer + .snapshot() + .into_iter() + .filter(|event| event.kind == "turn_error") + .collect(); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].payload["code"], -32002); + assert_eq!(errors[0].payload["error"], expected_error); + + // Capture the real signed notice sent by handle_prompt_result, without a live relay. + let notice: nostr::Event = tokio::time::timeout(Duration::from_secs(3), async { + let (socket, _) = listener.accept().await.unwrap(); + let mut reader = BufReader::new(socket); + let mut line = String::new(); + reader.read_line(&mut line).await.unwrap(); + assert_eq!(line, "POST /events HTTP/1.1\r\n"); + let mut content_length = None; + for _ in 0..64 { + line.clear(); + assert_ne!(reader.read_line(&mut line).await.unwrap(), 0); + if line == "\r\n" { + break; + } + if let Some(value) = line.to_ascii_lowercase().strip_prefix("content-length:") { + content_length = Some(value.trim().parse::().unwrap()); + } + } + let size = content_length.expect("request Content-Length"); + assert!(size < 65536); + let mut body = vec![0; size]; + reader.read_exact(&mut body).await.unwrap(); + reader + .get_mut() + .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\n{}") + .await + .unwrap(); + serde_json::from_slice(&body).unwrap() + }) + .await + .expect("failure notice must be posted on the first failure"); + notice.verify().unwrap(); + assert_eq!(notice.pubkey, rest.keys.public_key()); + assert_eq!(notice.kind, Kind::Custom(9)); + assert_eq!( + notice.content, + "⚠️ I couldn't process the last request: the configured model wasn't found at the provider's endpoint. Open agent settings, select a different model from the dropdown, and save your changes. Restart the agent to apply the new configuration, then re-send your request." + ); + let tags = serde_json::to_value(¬ice.tags).unwrap(); + assert!(tags + .as_array() + .unwrap() + .iter() + .any(|tag| tag[0] == "h" && tag[1] == channel_id.to_string())); + let threading = queue::parse_thread_tags(¬ice); + assert_eq!(threading.root_event_id, Some(root.to_hex())); + assert_eq!(threading.parent_event_id, Some(parent.to_hex())); + } + /// A non-auth application error (e.g. usage credits) must still follow the /// standard requeue path so today's behavior is unchanged. #[tokio::test] async fn non_auth_application_error_is_requeued() { + assert_application_error_is_requeued(acp::AcpError::AgentError { + code: -32000, + message: "Usage credits required for 1M context".to_string(), + }) + .await; + } + + #[tokio::test] + async fn non_model_resource_not_found_is_requeued() { + assert_application_error_is_requeued(acp::AcpError::AgentError { + code: -32002, + message: "Resource not found: session no longer exists".to_string(), + }) + .await; + } + + async fn assert_application_error_is_requeued(error: acp::AcpError) { let keys = nostr::Keys::generate(); let event = nostr::EventBuilder::new(nostr::Kind::Custom(9), "test") .sign_with_keys(&keys) @@ -11646,12 +11812,6 @@ mod error_outcome_emission_tests { cancel_reason: None, }; - // Usage-credits error — AgentError but NOT an auth error. - let usage_error = acp::AcpError::AgentError { - code: -32000, - message: "Usage credits required for 1M context".to_string(), - }; - let agent = dummy_agent(0).await; let mut pool = AgentPool::from_slots(vec![None]); let task_id = pool.join_set.spawn(async {}).id(); @@ -11683,7 +11843,7 @@ mod error_outcome_emission_tests { agent, source: PromptSource::Channel(scope::SessionScope::Conversation { channel_id }), turn_id: "test-turn-id".to_string(), - outcome: PromptOutcome::Error(usage_error), + outcome: PromptOutcome::Error(error), batch: Some(batch), }; handle_prompt_result( diff --git a/crates/buzz-acp/src/pi_launcher.rs b/crates/buzz-acp/src/pi_launcher.rs deleted file mode 100644 index 892500bb200..00000000000 --- a/crates/buzz-acp/src/pi_launcher.rs +++ /dev/null @@ -1,380 +0,0 @@ -//! Pi-specific native launcher setup. -//! -//! `pi-acp` does not currently consume ACP `session/new.systemPrompt`, but it -//! does let callers replace the `pi` executable through -//! `PI_ACP_PI_COMMAND`. For Pi sessions, Buzz points that variable at a -//! private launcher which adds `--system-prompt ` and the canonical Buzz -//! `--skill ` before forwarding the adapter's RPC/session arguments -//! unchanged. - -use std::fs::{self, OpenOptions}; -use std::io::{self, Write}; -use std::path::{Path, PathBuf}; - -#[cfg(unix)] -use std::ffi::OsStr; - -use uuid::Uuid; - -pub(crate) const PI_ACP_PI_COMMAND_ENV: &str = "PI_ACP_PI_COMMAND"; - -/// Files backing the Pi launcher for one `buzz-acp` process. -/// -/// The guard must live as long as the ACP pool because `pi-acp` may start or -/// restore Pi subprocesses after its own initialization. -pub(crate) struct PiLaunchOverride { - directory: PathBuf, - launcher: PathBuf, -} - -impl PiLaunchOverride { - /// Prepare a Pi launcher when the configured ACP adapter is `pi-acp`. - /// - /// Returns the prompt that still needs ordinary ACP delivery. For Pi, the - /// base prompt moves into Pi's native system role and is therefore removed - /// from first-turn user framing. Other adapters receive it unchanged. - pub(crate) fn prepare( - agent_command: &str, - base_prompt: Option, - managed_skills_dir: &Path, - inherited_pi_command_is_set: bool, - ) -> io::Result<(Option, Option)> { - if crate::config::normalize_agent_command_identity(agent_command) != "pi-acp" { - return Ok((None, base_prompt)); - } - - if inherited_pi_command_is_set { - return Err(io::Error::new( - io::ErrorKind::AlreadyExists, - "PI_ACP_PI_COMMAND is managed by Buzz; unset it before starting a managed Pi agent", - )); - } - - // Buzz owns PI_ACP_PI_COMMAND and always uses it to point pi-acp at - // this generated launcher. The launcher resolves the ordinary `pi` - // command from Buzz's effective PATH. - let prepared = Self::create("pi", base_prompt.as_deref(), managed_skills_dir)?; - Ok((Some(prepared), None)) - } - - pub(crate) fn launcher_path(&self) -> &Path { - &self.launcher - } - - fn create( - pi_command: &str, - prompt: Option<&str>, - managed_skills_dir: &Path, - ) -> io::Result { - let directory = std::env::temp_dir().join(format!( - "buzz-acp-pi-launcher-{}-{}", - std::process::id(), - Uuid::new_v4() - )); - create_private_directory(&directory)?; - - let prompt_path = directory.join("SYSTEM.md"); - let launcher = directory.join(launcher_file_name()); - // Construct the cleanup guard before either file write. Any later `?` - // drops it, so a partial setup cannot strand the private prompt file. - let prepared = Self { - directory, - launcher, - }; - - if let Some(prompt) = prompt { - write_private_file(&prompt_path, prompt.as_bytes(), false)?; - } - - let script = launcher_script( - pi_command, - prompt.map(|_| prompt_path.as_path()), - managed_skills_dir, - )?; - write_private_file(&prepared.launcher, script.as_bytes(), true)?; - - Ok(prepared) - } -} - -impl Drop for PiLaunchOverride { - fn drop(&mut self) { - if let Err(error) = fs::remove_dir_all(&self.directory) { - if error.kind() != io::ErrorKind::NotFound { - tracing::warn!( - path = %self.directory.display(), - %error, - "failed to remove temporary Pi launcher" - ); - } - } - } -} - -#[cfg(unix)] -fn create_private_directory(path: &Path) -> io::Result<()> { - use std::os::unix::fs::DirBuilderExt; - - let mut builder = fs::DirBuilder::new(); - builder.mode(0o700).create(path) -} - -#[cfg(not(unix))] -fn create_private_directory(path: &Path) -> io::Result<()> { - fs::create_dir(path) -} - -fn write_private_file(path: &Path, content: &[u8], executable: bool) -> io::Result<()> { - let mut options = OpenOptions::new(); - options.write(true).create_new(true); - - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - options.mode(if executable { 0o700 } else { 0o600 }); - } - - #[cfg(not(unix))] - let _ = executable; - - let mut file = options.open(path)?; - file.write_all(content)?; - file.sync_all() -} - -#[cfg(unix)] -fn launcher_file_name() -> &'static str { - "pi-with-buzz-context" -} - -#[cfg(windows)] -fn launcher_file_name() -> &'static str { - "pi-with-buzz-context.cmd" -} - -#[cfg(not(any(unix, windows)))] -fn launcher_file_name() -> &'static str { - "pi-with-buzz-context" -} - -#[cfg(unix)] -fn launcher_script( - pi_command: &str, - prompt_path: Option<&Path>, - managed_skills_dir: &Path, -) -> io::Result { - let system_prompt_arg = match prompt_path { - Some(prompt_path) => format!(" --system-prompt {}", shell_quote(prompt_path.as_os_str())?), - None => String::new(), - }; - Ok(format!( - "#!/bin/sh\nexec {}{} --skill {} \"$@\"\n", - shell_quote(OsStr::new(pi_command))?, - system_prompt_arg, - shell_quote(managed_skills_dir.as_os_str())?, - )) -} - -#[cfg(unix)] -fn shell_quote(value: &OsStr) -> io::Result { - let value = value.to_str().ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidData, - "Pi launcher paths must be valid UTF-8", - ) - })?; - Ok(format!("'{}'", value.replace('\'', "'\"'\"'"))) -} - -#[cfg(windows)] -fn launcher_script( - pi_command: &str, - prompt_path: Option<&Path>, - managed_skills_dir: &Path, -) -> io::Result { - let system_prompt_arg = match prompt_path { - Some(prompt_path) => { - let prompt_path = prompt_path.to_str().ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidData, - "Pi launcher paths must be valid UTF-8", - ) - })?; - format!(" --system-prompt \"{}\"", batch_escape(prompt_path)) - } - None => String::new(), - }; - let managed_skills_dir = managed_skills_dir.to_str().ok_or_else(|| { - io::Error::new( - io::ErrorKind::InvalidData, - "Pi skill paths must be valid UTF-8", - ) - })?; - Ok(format!( - "@echo off\r\n\"{}\"{} --skill \"{}\" %*\r\nexit /b %ERRORLEVEL%\r\n", - batch_escape(pi_command), - system_prompt_arg, - batch_escape(managed_skills_dir), - )) -} - -#[cfg(windows)] -fn batch_escape(value: &str) -> String { - value.replace('%', "%%").replace('"', "\"\"") -} - -#[cfg(not(any(unix, windows)))] -fn launcher_script( - _pi_command: &str, - _prompt_path: Option<&Path>, - _managed_skills_dir: &Path, -) -> io::Result { - Err(io::Error::new( - io::ErrorKind::Unsupported, - "Pi launch overrides are unsupported on this platform", - )) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn non_pi_adapter_keeps_base_prompt_for_acp_delivery() { - let base = Some("Buzz base".to_string()); - let (prepared, remaining) = - PiLaunchOverride::prepare("goose", base.clone(), Path::new("/unused/skills"), true) - .expect("prepare"); - assert!(prepared.is_none()); - assert_eq!(remaining, base); - } - - #[test] - fn pi_adapter_rejects_inherited_pi_command() { - let error = PiLaunchOverride::prepare( - "pi-acp", - Some("Buzz base".to_string()), - Path::new("/unused/skills"), - true, - ) - .err() - .expect("inherited PI_ACP_PI_COMMAND must be rejected"); - - assert_eq!(error.kind(), io::ErrorKind::AlreadyExists); - assert!(error.to_string().contains("managed by Buzz")); - } - - #[test] - fn disabled_base_prompt_still_creates_pi_skills_launcher() { - let (prepared, remaining) = - PiLaunchOverride::prepare("pi-acp", None, Path::new("/unused/skills"), false) - .expect("prepare"); - let prepared = prepared.expect("Pi skills launcher"); - assert!(remaining.is_none()); - assert!(!prepared.directory.join("SYSTEM.md").exists()); - - #[cfg(unix)] - assert!(fs::read_to_string(prepared.launcher_path()) - .expect("read launcher") - .contains("--skill '/unused/skills'")); - } - - #[test] - fn pi_adapter_moves_buzz_base_out_of_ordinary_acp_delivery() { - let base = crate::scope::SessionPolicy::Thread - .append_session_model(include_str!("base_prompt.md")); - let (prepared, remaining) = PiLaunchOverride::prepare( - "/opt/bin/pi-acp", - Some(base.clone()), - Path::new("/buzz/.agents/skills"), - false, - ) - .expect("prepare"); - let prepared = prepared.expect("Pi launcher"); - - assert!(remaining.is_none()); - assert_eq!( - fs::read_to_string(prepared.directory.join("SYSTEM.md")).expect("read prompt"), - base - ); - assert!(base.contains("each thread gets its own")); - - #[cfg(unix)] - assert!(fs::read_to_string(prepared.launcher_path()) - .expect("read launcher") - .contains("exec 'pi'")); - } - - #[cfg(unix)] - #[test] - fn pi_launcher_replaces_system_prompt_and_forwards_adapter_args() { - use std::os::unix::fs::PermissionsExt; - use std::process::Command; - - let fixture_dir = - std::env::temp_dir().join(format!("buzz-acp-pi-system-prompt-test-{}", Uuid::new_v4())); - create_private_directory(&fixture_dir).expect("create fixture dir"); - let capture_path = fixture_dir.join("args.txt"); - let fake_pi = fixture_dir.join("fake-pi"); - let managed_skills_dir = fixture_dir.join("managed skills"); - let fake_script = format!( - "#!/bin/sh\nprintf '%s\\n' \"$@\" > {}\n", - shell_quote(capture_path.as_os_str()).expect("quote capture path") - ); - write_private_file(&fake_pi, fake_script.as_bytes(), true).expect("write fake pi"); - - let prepared = PiLaunchOverride::create( - fake_pi.to_str().expect("UTF-8 fake Pi path"), - Some("Buzz base\n\n## Session Model\nThread scoped"), - &managed_skills_dir, - ) - .expect("prepare Pi launcher"); - let prompt_path = prepared.directory.join("SYSTEM.md"); - - let status = Command::new(prepared.launcher_path()) - .args(["--mode", "rpc", "--session", "/tmp/session.jsonl"]) - .status() - .expect("run launcher"); - assert!(status.success()); - assert_eq!( - fs::read_to_string(&capture_path).expect("read captured args"), - format!( - "--system-prompt\n{}\n--skill\n{}\n--mode\nrpc\n--session\n/tmp/session.jsonl\n", - prompt_path.display(), - managed_skills_dir.display(), - ) - ); - assert_eq!( - fs::read_to_string(&prompt_path).expect("read system prompt"), - "Buzz base\n\n## Session Model\nThread scoped" - ); - assert_eq!( - fs::metadata(&prompt_path) - .expect("prompt metadata") - .permissions() - .mode() - & 0o777, - 0o600 - ); - assert_eq!( - fs::metadata(prepared.launcher_path()) - .expect("launcher metadata") - .permissions() - .mode() - & 0o777, - 0o700 - ); - assert_eq!( - fs::metadata(&prepared.directory) - .expect("directory metadata") - .permissions() - .mode() - & 0o777, - 0o700 - ); - - drop(prepared); - assert!(!prompt_path.exists()); - fs::remove_dir_all(fixture_dir).expect("remove fixture dir"); - } -} diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index dbeafedda70..250221badca 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -32,7 +32,7 @@ use uuid::Uuid; use crate::acp::{ extract_model_config_options, extract_model_state, extract_thought_level_config_id, model_in_catalog, resolve_model_switch_method, AcpClient, AcpError, EnvVar, McpServer, - ModelSwitchMethod, StopReason, SystemPromptTransport, + ModelSwitchMethod, StopReason, SystemPromptTransport, BUZZ_PI_ACP_NAME, }; use crate::config::{compose_scoped_session_title, DedupMode, PermissionMode}; use crate::observer; @@ -299,7 +299,7 @@ fn has_system_prompt_support( ) -> bool { if agent_name == "goose" { goose_system_prompt_supported == Some(true) - } else if agent_name == CLAUDE_AGENT_ACP_NAME { + } else if agent_name == BUZZ_PI_ACP_NAME || agent_name == CLAUDE_AGENT_ACP_NAME { true } else { protocol_version >= 2 @@ -312,7 +312,11 @@ fn session_new_system_prompt<'a>( agent_name: &str, prompt: Option<&'a str>, ) -> Option> { - if is_goose || (protocol_version < 2 && agent_name != CLAUDE_AGENT_ACP_NAME) { + if is_goose { + None + } else if agent_name == BUZZ_PI_ACP_NAME { + prompt.map(SystemPromptTransport::PiMeta) + } else if protocol_version < 2 && agent_name != CLAUDE_AGENT_ACP_NAME { None } else if agent_name == CLAUDE_AGENT_ACP_NAME { prompt.map(SystemPromptTransport::ClaudeMeta) @@ -5485,56 +5489,7 @@ mod tests { assert_eq!(composed, "\nbe helpful\n\n\ntick"); } - #[test] - fn goose_uses_system_prompt_only_after_custom_method_succeeds() { - assert!(!has_system_prompt_support(2, "goose", None)); - assert!(!has_system_prompt_support(2, "goose", Some(false))); - assert!(has_system_prompt_support(2, "goose", Some(true))); - assert!(has_system_prompt_support(1, "goose", Some(true))); - assert!(has_system_prompt_support(2, "buzz-agent", None)); - // Goose never receives system prompt via session/new (uses post-hoc method). - assert_eq!( - session_new_system_prompt(true, 2, "goose", Some("instructions")), - None - ); - // Protocol-v2 non-goose gets Field transport. - assert_eq!( - session_new_system_prompt(false, 2, "buzz-agent", Some("instructions")), - Some(SystemPromptTransport::Field("instructions")) - ); - // Protocol-v1 non-goose, non-claude gets None (legacy user-message framing). - assert_eq!( - session_new_system_prompt(false, 1, "codex", Some("instructions")), - None - ); - // claude-agent-acp gets ClaudeMeta transport regardless of protocol version. - assert_eq!( - session_new_system_prompt(false, 1, CLAUDE_AGENT_ACP_NAME, Some("instructions")), - Some(SystemPromptTransport::ClaudeMeta("instructions")) - ); - assert_eq!( - session_new_system_prompt(true, 1, CLAUDE_AGENT_ACP_NAME, Some("instructions")), - None, - "goose path must never produce a transport even when agent_name matches" - ); - } - - #[test] - fn claude_agent_acp_has_system_prompt_support_regardless_of_protocol_version() { - // claude-agent-acp declares protocolVersion:1 but supports _meta.systemPrompt; - // has_system_prompt_support must return true so user-message framing is suppressed. - assert!(has_system_prompt_support(1, CLAUDE_AGENT_ACP_NAME, None)); - assert!(has_system_prompt_support(2, CLAUDE_AGENT_ACP_NAME, None)); - } - - #[test] - fn old_zed_adapter_name_falls_through_to_protocol_version_gate() { - // The renamed @zed-industries package predates the _meta.systemPrompt support, - // so it must not be treated as capable and stays on legacy user-message framing. - let old_name = "@zed-industries/claude-code-acp"; - assert!(!has_system_prompt_support(1, old_name, None)); - assert!(has_system_prompt_support(2, old_name, None)); - } + include!("pool/system_prompt_tests.rs"); #[test] fn test_initial_message_legacy_agent_without_base_is_unchanged() { @@ -11214,3 +11169,7 @@ done"# ); } } + +#[cfg(all(test, unix))] +#[path = "pool/pi_prompt_tests.rs"] +mod pi_prompt_tests; diff --git a/crates/buzz-acp/src/pool/pi_prompt_tests.rs b/crates/buzz-acp/src/pool/pi_prompt_tests.rs new file mode 100644 index 00000000000..a918e3decb6 --- /dev/null +++ b/crates/buzz-acp/src/pool/pi_prompt_tests.rs @@ -0,0 +1,371 @@ +use super::*; +use std::os::unix::fs::PermissionsExt; + +fn owned_pi(acp: AcpClient, protocol_version: u32) -> OwnedAgent { + OwnedAgent { + index: 0, + acp, + state: SessionState::default(), + model_capabilities: None, + desired_model: None, + model_overridden: false, + desired_model_request_id: None, + desired_model_pending_ack: false, + startup_effort: None, + agent_name: BUZZ_PI_ACP_NAME.into(), + goose_system_prompt_supported: None, + protocol_version, + } +} + +fn fixture_dir() -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("buzz pi transport {}", Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +fn script_at(dir: &std::path::Path, script: &str) -> std::path::PathBuf { + let path = dir.join(BUZZ_PI_ACP_NAME); + std::fs::write(&path, format!("#!/bin/bash\n{script}\n")).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o700)).unwrap(); + path +} + +#[tokio::test] +async fn pi_composed_prompt_uses_meta_without_capability_negotiation() { + for version in [1, 2] { + let dir = fixture_dir(); + let init = serde_json::json!({"jsonrpc":"2.0", "id":0, "result": { + "protocolVersion":version, "agentInfo":{"name":"buzz-pi-acp", "version":"fixture"}, "agentCapabilities":{} + }}); + let path = script_at( + &dir, + &format!( + r#" + printf '%s\n' "$@" > '{dir}/args' + read -r request + echo '{init}' + read -r request + printf '%s\n' "$request" > '{dir}/request' + echo '{{"jsonrpc":"2.0","id":1,"result":{{"sessionId":"fixture"}}}}' + read -r request + "#, + dir = dir.display() + ), + ); + let mut acp = AcpClient::spawn(path.to_str().unwrap(), &[], &[], false) + .await + .unwrap(); + acp.initialize().await.unwrap(); + let mut agent = owned_pi(acp, version); + assert!(agent.has_system_prompt_support()); + let mut ctx = tests::make_prompt_context_no_owner(); + ctx.base_prompt = Some("BUZZ_BASE".into()); + ctx.system_prompt = Some("BUZZ_PERSONA".into()); + ctx.team_instructions = Some("BUZZ_TEAM".into()); + ctx.session_title = Some("Pi fixture".into()); + let core = "BUZZ_CORE"; + let canvas = "BUZZ_CANVAS"; + create_session_and_apply_model( + &mut agent, + &ctx, + Some(core), + NewSessionChannelContext { + huddle_instructions: Some("BUZZ_HUDDLE"), + canvas: Some(canvas), + name: Some("channel"), + scope: None, + channel_type: None, + }, + ) + .await + .unwrap(); + agent.acp.shutdown().await; + let request: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(dir.join("request")).unwrap()).unwrap(); + let params = &request["params"]; + assert!(params.get("systemPrompt").is_none()); + assert!(params["_meta"]["sessionTitle"] + .as_str() + .unwrap() + .contains("Pi fixture")); + let standing = crate::queue::StandingContext { + base_prompt: ctx.base_prompt.as_deref(), + system_prompt: ctx.system_prompt.as_deref(), + team_instructions: ctx.team_instructions.as_deref(), + agent_core: Some(core), + huddle_instructions: Some("BUZZ_HUDDLE"), + agent_canvas: Some(canvas), + }; + let user = prepend_standing_for_legacy(2, &standing, "EVENT"); + for marker in [ + "BUZZ_BASE", + "BUZZ_PERSONA", + "BUZZ_TEAM", + "BUZZ_CORE", + "BUZZ_HUDDLE", + "BUZZ_CANVAS", + ] { + assert_eq!( + params["_meta"]["systemPrompt"] + .as_str() + .unwrap() + .matches(marker) + .count(), + 1 + ); + assert!(!user.contains(marker)); + } + + let args = std::fs::read_to_string(dir.join("args")).unwrap(); + assert_eq!( + args, + format!( + "--\n--skill\n{}\n", + std::env::current_dir() + .unwrap() + .join(".agents/skills") + .display() + ) + ); + std::fs::remove_dir_all(dir).unwrap(); + } +} + +#[tokio::test] +async fn pi_launch_preserves_existing_skills_in_explicit_workspace() { + const FIXTURE_ENV: &str = "BUZZ_TEST_PI_LAUNCH_WORKSPACE"; + if let Some(dir) = std::env::var_os(FIXTURE_ENV) { + let path = std::path::PathBuf::from(dir).join(BUZZ_PI_ACP_NAME); + let mut client = AcpClient::spawn( + path.to_str().unwrap(), + &["--".into(), "--skill".into(), "/extra skills".into()], + &[], + false, + ) + .await + .unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(5), client.initialize()) + .await + .unwrap() + .unwrap_err(); + client.shutdown().await; + return; + } + + let dir = fixture_dir(); + let workspace = dir.join("chosen workspace"); + std::fs::create_dir_all(workspace.join(".agents/skills")).unwrap(); + // Canonicalize macOS's /var -> /private/var before comparing with getcwd. + let workspace = workspace.canonicalize().unwrap(); + script_at( + &dir, + r#"printf '%s\n' "$@" > "$(dirname "$0")/args" +pwd -P > "$(dirname "$0")/cwd""#, + ); + // Re-enter only this test in a separate process so parallel tests never + // share a mutated CWD. This models Desktop setting its harness child's CWD. + let output = tokio::time::timeout( + std::time::Duration::from_secs(20), + tokio::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "pool::pi_prompt_tests::pi_launch_preserves_existing_skills_in_explicit_workspace", + "--nocapture", + ]) + .kill_on_drop(true) + .env(FIXTURE_ENV, &dir) + .current_dir(&workspace) + .output(), + ) + .await + .unwrap() + .unwrap(); + assert!( + output.status.success(), + "child failed: {}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + std::fs::read_to_string(dir.join("cwd")).unwrap().trim(), + workspace.to_str().unwrap() + ); + assert_eq!( + std::fs::read_to_string(dir.join("args")).unwrap(), + format!( + "--\n--skill\n/extra skills\n--skill\n{}\n", + workspace.join(".agents/skills").display() + ) + ); + std::fs::remove_dir_all(dir).unwrap(); +} + +#[tokio::test] +async fn upstream_pi_acp_launch_does_not_receive_managed_skills() { + let dir = fixture_dir(); + let path = dir.join("pi-acp"); + std::fs::write( + &path, + format!( + r#"#!/bin/bash +printf '%s' "$*" > '{dir}/args' +read -r request +echo '{{"jsonrpc":"2.0","id":0,"result":{{"protocolVersion":1,"agentInfo":{{"name":"pi-acp","version":"fixture"}},"agentCapabilities":{{}}}}}}' +read -r request +"#, + dir = dir.display() + ), + ) + .unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o700)).unwrap(); + + let mut client = AcpClient::spawn(path.to_str().unwrap(), &[], &[], false) + .await + .unwrap(); + client.initialize().await.unwrap(); + client.shutdown().await; + + assert_eq!(std::fs::read_to_string(dir.join("args")).unwrap(), ""); + std::fs::remove_dir_all(dir).unwrap(); +} + +#[tokio::test] +#[ignore = "requires BUZZ_TEST_PI_ACP pointing to a built fork and Pi on PATH"] +async fn real_pi_preserves_buzz_prompt_and_launch_skills_on_restore() { + use base64::Engine; + let adapter = std::env::var("BUZZ_TEST_PI_ACP").expect("set BUZZ_TEST_PI_ACP"); + let dir = fixture_dir(); + let home = dir.join("home"); + let workspace = dir.join("workspace"); + let skill = dir.join("extra skills"); + for path in [&home, &workspace, &skill] { + std::fs::create_dir_all(path).unwrap(); + } + std::fs::write( + skill.join("SKILL.md"), + "---\nname: buzz-fixture\ndescription: BUZZ_SKILL_MARKER\n---\nSynthetic instructions.\n", + ) + .unwrap(); + let path = script_at(&dir, &format!( + "export HOME='{}' PI_CODING_AGENT_DIR='{}/agent' ANTHROPIC_API_KEY=synthetic-test-key PI_ACP_PI_COMMAND=pi\nexec node '{}' \"$@\"", + home.display(), dir.display(), adapter.replace('\'', "'\\''") + )); + let args = vec![ + "--".into(), + "--offline".into(), + "--no-extensions".into(), + "--no-context-files".into(), + "--skill".into(), + skill.to_string_lossy().into_owned(), + ]; + let mut acp = AcpClient::spawn(path.to_str().unwrap(), &args, &[], false) + .await + .unwrap(); + acp.initialize().await.unwrap(); + let mut agent = owned_pi(acp, 1); + assert!(agent.has_system_prompt_support()); + let mut ctx = tests::make_prompt_context_no_owner(); + ctx.cwd = workspace.to_string_lossy().into_owned(); + ctx.base_prompt = Some("BUZZ_BASE".into()); + ctx.system_prompt = Some("BUZZ_PERSONA".into()); + ctx.team_instructions = Some("BUZZ_TEAM".into()); + ctx.session_title = Some("Pi fixture".into()); + let id = create_session_and_apply_model( + &mut agent, + &ctx, + Some("BUZZ_CORE"), + NewSessionChannelContext { + huddle_instructions: Some("BUZZ_HUDDLE"), + canvas: Some("BUZZ_CANVAS"), + name: None, + scope: None, + channel_type: None, + }, + ) + .await + .unwrap(); + let metadata = std::fs::read_dir(home.join(".pi/buzz-pi-acp/sessions")) + .unwrap() + .find_map(|entry| { + let contents = std::fs::read_to_string(entry.ok()?.path()).ok()?; + let metadata: serde_json::Value = serde_json::from_str(&contents).ok()?; + (metadata["session"]["sessionId"].as_str() == Some(id.as_str())).then_some(metadata) + }) + .expect("adapter should persist metadata for the new session"); + let transcript = metadata["session"]["sessionFile"].as_str().unwrap(); + let timestamp = "2026-01-01T00:00:00.000Z"; + std::fs::create_dir_all(std::path::Path::new(transcript).parent().unwrap()).unwrap(); + std::fs::write(transcript, format!("{}\n{}\n", + serde_json::json!({"type":"session","version":3,"id":id,"timestamp":timestamp,"cwd":ctx.cwd}), + serde_json::json!({"type":"message","id":"00000001","parentId":null,"timestamp":timestamp,"message":{"role":"user","content":[{"type":"text","text":"fixture"}],"timestamp":1767225600000u64}}) + )).unwrap(); + agent + .acp + .session_new_full( + &ctx.cwd, + vec![], + Some(SystemPromptTransport::PiMeta("OTHER_SESSION")), + None, + ) + .await + .unwrap(); + for restart in [false, true] { + if restart { + agent.acp.shutdown().await; + agent.acp = AcpClient::spawn(path.to_str().unwrap(), &args, &[], false) + .await + .unwrap(); + agent.acp.initialize().await.unwrap(); + } + agent + .acp + .session_prompt_with_idle_timeout( + &id, + "/export", + Duration::from_secs(15), + Duration::from_secs(30), + ) + .await + .unwrap(); + let html = + std::fs::read_to_string(workspace.join(format!("pi-session-{id}.html"))).unwrap(); + let encoded = html + .split("id=\"session-data\"") + .nth(1) + .unwrap() + .split_once('>') + .unwrap() + .1 + .split("") + .next() + .unwrap() + .trim(); + let data: serde_json::Value = serde_json::from_slice( + &base64::engine::general_purpose::STANDARD + .decode(encoded) + .unwrap(), + ) + .unwrap(); + let prompt = data["systemPrompt"].as_str().unwrap(); + for marker in [ + "BUZZ_BASE", + "BUZZ_PERSONA", + "BUZZ_TEAM", + "BUZZ_CORE", + "BUZZ_HUDDLE", + "BUZZ_CANVAS", + "BUZZ_SKILL_MARKER", + ] { + assert_eq!( + prompt.matches(marker).count(), + 1, + "{marker}, restart={restart}" + ); + } + assert!(!prompt.contains("OTHER_SESSION")); + assert!(!prompt.contains("You are an expert coding assistant")); + } + agent.acp.shutdown().await; + std::fs::remove_dir_all(dir).unwrap(); +} diff --git a/crates/buzz-acp/src/pool/system_prompt_tests.rs b/crates/buzz-acp/src/pool/system_prompt_tests.rs new file mode 100644 index 00000000000..223065decbc --- /dev/null +++ b/crates/buzz-acp/src/pool/system_prompt_tests.rs @@ -0,0 +1,78 @@ +#[test] +fn goose_uses_system_prompt_only_after_custom_method_succeeds() { + assert!(!has_system_prompt_support(2, "goose", None)); + assert!(!has_system_prompt_support(2, "goose", Some(false))); + assert!(has_system_prompt_support(2, "goose", Some(true))); + assert!(has_system_prompt_support(1, "goose", Some(true))); + assert!(has_system_prompt_support(2, "buzz-agent", None)); + // Goose never receives system prompt via session/new (uses post-hoc method). + assert_eq!( + session_new_system_prompt(true, 2, "goose", Some("instructions")), + None + ); + // Protocol-v2 non-goose gets Field transport. + assert_eq!( + session_new_system_prompt(false, 2, "buzz-agent", Some("instructions")), + Some(SystemPromptTransport::Field("instructions")) + ); + // Protocol-v1 non-goose, non-claude gets None (legacy user-message framing). + assert_eq!( + session_new_system_prompt(false, 1, "codex", Some("instructions")), + None + ); + // claude-agent-acp gets ClaudeMeta transport regardless of protocol version. + assert_eq!( + session_new_system_prompt(false, 1, CLAUDE_AGENT_ACP_NAME, Some("instructions")), + Some(SystemPromptTransport::ClaudeMeta("instructions")) + ); + assert_eq!( + session_new_system_prompt(true, 1, CLAUDE_AGENT_ACP_NAME, Some("instructions")), + None, + "goose path must never produce a transport even when agent_name matches" + ); +} + +#[test] +fn claude_agent_acp_has_system_prompt_support_regardless_of_protocol_version() { + // claude-agent-acp declares protocolVersion:1 but supports _meta.systemPrompt; + // has_system_prompt_support must return true so user-message framing is suppressed. + assert!(has_system_prompt_support(1, CLAUDE_AGENT_ACP_NAME, None)); + assert!(has_system_prompt_support(2, CLAUDE_AGENT_ACP_NAME, None)); +} + +#[test] +fn old_zed_adapter_name_falls_through_to_protocol_version_gate() { + // The renamed @zed-industries package predates the _meta.systemPrompt support, + // so it must not be treated as capable and stays on legacy user-message framing. + let old_name = "@zed-industries/claude-code-acp"; + assert!(!has_system_prompt_support(1, old_name, None)); + assert!(has_system_prompt_support(2, old_name, None)); +} + +#[test] +fn pi_prompt_support_uses_metadata_regardless_of_protocol_version() { + for version in [1, 2] { + assert!(has_system_prompt_support(version, BUZZ_PI_ACP_NAME, None)); + assert_eq!( + session_new_system_prompt(false, version, BUZZ_PI_ACP_NAME, Some("instructions")), + Some(SystemPromptTransport::PiMeta("instructions")) + ); + assert_eq!( + session_new_system_prompt(false, version, BUZZ_PI_ACP_NAME, None), + None + ); + } +} + +#[test] +fn upstream_pi_acp_does_not_receive_fork_specific_prompt_metadata() { + assert!(!has_system_prompt_support(1, "pi-acp", None)); + assert_eq!( + session_new_system_prompt(false, 1, "pi-acp", Some("instructions")), + None + ); + assert_eq!( + session_new_system_prompt(false, 2, "pi-acp", Some("instructions")), + Some(SystemPromptTransport::Field("instructions")) + ); +} diff --git a/crates/buzz-acp/src/relay.rs b/crates/buzz-acp/src/relay.rs index 739a5670128..e1046c4d4a8 100644 --- a/crates/buzz-acp/src/relay.rs +++ b/crates/buzz-acp/src/relay.rs @@ -1167,10 +1167,8 @@ struct BgState { /// On reconnect resubscribe, `since` = min(last_seen, channel_dropped_since). /// Cleared per-channel after a successful resubscribe. channel_dropped_since: HashMap, - /// Set by the backpressure handler when the event channel is full. - /// The main loop checks this flag and triggers a proactive resubscribe - /// (without waiting for a disconnect) so dropped events are replayed. - proactive_resubscribe_needed: bool, + /// Rate/fairness bookkeeping only; replay cursors retain baseline semantics. + recovery: recovery::RecoverySchedule, /// Unix timestamp captured just before the relay connection was established. /// Used as the floor `since` for membership notification replay so events /// predating this session are never re-delivered. @@ -1244,7 +1242,7 @@ impl BgState { membership_sub_active: false, observer_control_sub_active: false, channel_dropped_since: HashMap::new(), - proactive_resubscribe_needed: false, + recovery: recovery::RecoverySchedule::default(), startup_watermark: None, subscribe_since: HashMap::new(), rate_limit_gate: None, @@ -1305,6 +1303,9 @@ impl BgState { /// Prevents stale replay on re-subscribe and avoids unbounded state growth /// for channels that are removed and never re-added. fn clear_channel_state(&mut self, channel_id: &Uuid) { + self.recovery + .last_attempt + .remove(&channel_sub_id(*channel_id)); self.last_seen.remove(channel_id); self.subscribe_since.remove(channel_id); self.channel_dropped_since.remove(channel_id); @@ -1833,82 +1834,6 @@ async fn run_background_task( let mut drain_pacing_next: Option = None; loop { - if state.proactive_resubscribe_needed { - state.proactive_resubscribe_needed = false; - info!("proactive resubscribe triggered by backpressure event loss"); - // Proactive resubscribe runs on the EXISTING socket — do NOT clear the - // rate-limit gate or pending queues. - match resubscribe_after_reconnect( - &mut ws, - &mut cmd_rx, - &mut state, - &agent_pubkey_hex, - false, // existing socket — preserve gate state - ) - .await - { - ResubscribeResult::Ok => {} - ResubscribeResult::Shutdown => return, - ResubscribeResult::RetryConnection => { - warn!("proactive resubscribe had failures — triggering reconnect"); - let _ = event_tx.try_send(None); - match try_autonomous_reconnect( - &mut ws, - &mut cmd_rx, - &mut state, - &keys, - &relay_url, - &agent_pubkey_hex, - &event_tx, - &observer_control_tx, - auth_tag.as_ref(), - ) - .await - { - ReconnectOutcome::Ok => { - if matches!( - drain_post_reconnect( - &mut ws, - &mut cmd_rx, - &mut state, - &agent_pubkey_hex - ) - .await, - ReconnectOutcome::Shutdown - ) { - return; - } - } - ReconnectOutcome::Shutdown => return, - ReconnectOutcome::Failed => { - if matches!( - wait_for_reconnect( - &mut ws, - &mut cmd_rx, - &mut state, - &keys, - &relay_url, - &agent_pubkey_hex, - &event_tx, - &observer_control_tx, - true, - auth_tag.as_ref(), - ) - .await, - ReconnectOutcome::Shutdown - ) { - return; - } - } - } - ping_sent = false; - last_pong = Instant::now(); - connected_since = Instant::now(); - stable_logged = false; - } - } - } - // Drain pending subs, one REQ per pacing tick within the relay's // admission window. let drain_window_open = drain_pacing_next.is_none_or(|t| tokio::time::Instant::now() >= t); @@ -1989,7 +1914,11 @@ async fn run_background_task( } } + let recovery_at = recovery::ready_at(&mut state); tokio::select! { + _ = recovery::ready(&event_tx, recovery_at) => { + recovery::recover_one(&mut ws, &mut state, &event_tx, &agent_pubkey_hex).await; + } raw = ws.next() => { // Determine if the socket is lost. let socket_lost = match raw { @@ -2365,12 +2294,10 @@ async fn handle_ws_message( // replay starts early enough to re-deliver it. state.membership_dropped_since = Some(state.membership_dropped_since.map_or(ts, |d| d.min(ts))); - // Proactively trigger resubscribe without waiting for a disconnect. - state.proactive_resubscribe_needed = true; warn!( channel_id = %channel_uuid, ts, - "membership notification dropped (backpressure) — proactive resubscribe queued" + "membership notification dropped (backpressure) — targeted recovery pending" ); } Err(mpsc::error::TrySendError::Closed(_)) => return false, @@ -2407,12 +2334,10 @@ async fn handle_ws_message( .entry(channel_id) .and_modify(|d| *d = (*d).min(ts)) .or_insert(ts); - // Proactively trigger resubscribe without waiting for a disconnect. - state.proactive_resubscribe_needed = true; warn!( channel_id = %channel_id, ts, - "event channel full — dropping event for channel {channel_id} — proactive resubscribe queued" + "event channel full — dropping event for channel {channel_id} — targeted recovery pending" ); } Err(mpsc::error::TrySendError::Closed(_)) => { @@ -4255,6 +4180,11 @@ async fn wait_for_any_ok( } } +mod recovery; + +#[cfg(test)] +mod recovery_tests; + #[cfg(test)] mod tests { use super::*; @@ -4782,7 +4712,7 @@ mod tests { .expect("signing should succeed") } - async fn test_ws_pair() -> (WsStream, WebSocketStream) { + pub(super) async fn test_ws_pair() -> (WsStream, WebSocketStream) { let listener = tokio::net::TcpListener::bind("127.0.0.1:0") .await .expect("bind test websocket"); @@ -4799,7 +4729,7 @@ mod tests { (client, server.await.expect("join test websocket server")) } - async fn next_test_frame( + pub(super) async fn next_test_frame( server: &mut WebSocketStream, ) -> serde_json::Value { let message = timeout(Duration::from_secs(1), server.next()) @@ -5042,14 +4972,14 @@ mod tests { )); } - fn test_channel_filter() -> ChannelFilter { + pub(super) fn test_channel_filter() -> ChannelFilter { ChannelFilter { kinds: Some(vec![9]), require_mention: false, } } - fn seed_test_subscription(state: &mut BgState, channel_id: Uuid) { + pub(super) fn seed_test_subscription(state: &mut BgState, channel_id: Uuid) { apply_command_to_state( state, RelayCommand::Subscribe { diff --git a/crates/buzz-acp/src/relay/recovery.rs b/crates/buzz-acp/src/relay/recovery.rs new file mode 100644 index 00000000000..62da119a4be --- /dev/null +++ b/crates/buzz-acp/src/relay/recovery.rs @@ -0,0 +1,132 @@ +//! Overflow recovery is an attempted replay, not an EOSE/consumer receipt. +//! Keep the existing IDs and cursor retirement rules; bound when work is sent. +use super::*; + +pub(super) const RECOVERY_INTERVAL: Duration = Duration::from_secs(5); + +#[derive(Default)] +pub(super) struct RecoverySchedule { + next_attempt: Option, + pub(super) last_attempt: HashMap, +} + +/// Attempt at most one affected subscription, with space for replay to arrive. +/// Failed writes retain the loss cursor and are paced too. No EOSE is interpreted +/// as completion: overlapping requests keep their existing stable wire IDs. +pub(super) async fn recover_one( + ws: &mut WsStream, + state: &mut BgState, + event_tx: &mpsc::Sender>, + agent_pubkey_hex: &str, +) { + let now = tokio::time::Instant::now(); + if event_tx.is_closed() + || event_tx.capacity() < event_tx.max_capacity().div_ceil(2) + || state.recovery.next_attempt.is_some_and(|next| now < next) + || state.check_rate_gate().is_some() + { + return; + } + + let channel = next_channel(state); + let Some(channel) = channel else { return }; + let sub = channel.map_or_else(|| MEMBERSHIP_NOTIF_SUB_ID.to_owned(), channel_sub_id); + state.recovery.last_attempt.insert(sub.clone(), now); + info!(subscription = sub, "attempting targeted overflow replay"); + + if let Some(ch) = channel { + if let Some(filter) = state.active_filters.get(&ch).cloned() { + let since = state.channel_since(&ch); + if send_subscribe(ws, state, ch, agent_pubkey_hex, since, &filter).await { + // Baseline retirement point: REQ write, NOT proven delivery. + // New overflow after this attempt creates another pending cursor. + state.channel_dropped_since.remove(&ch); + } + } + } else { + let since = match (state.membership_dropped_since, state.membership_last_seen) { + (Some(d), Some(l)) => Some(d.min(l)), + (Some(d), None) => Some(d), + (None, Some(l)) => Some(l), + (None, None) => state.startup_watermark, + }; + if send_membership_subscribe(ws, agent_pubkey_hex, since).await { + state.membership_dropped_since = None; + } + } + // Pace from the end of a potentially backpressured write. No catch-up burst. + // The existing bounded write timeout and read/ping owner detect socket loss. + state.recovery.next_attempt = Some(tokio::time::Instant::now() + RECOVERY_INTERVAL); +} + +/// No timer or capacity waiter when another authority owns all pending loss. +/// Only actual attempts advance the cooldown; closed gates wake at expiry. +pub(super) fn ready_at(state: &mut BgState) -> Option { + next_channel(state)?; + Some( + state + .recovery + .next_attempt + .unwrap_or_else(tokio::time::Instant::now) + .max( + state + .check_rate_gate() + .unwrap_or_else(tokio::time::Instant::now), + ), + ) +} + +/// Select-local readiness, not a send or a reservation carried across reads. +/// The socket task is the sole producer. `select!` drops this future (including +/// partial permits) BEFORE handling another frame/command, so live try_send +/// never competes with a recovery reservation. Receives only add capacity. +/// Keep this future inside select!: awaiting it alone would block the reader; +/// persisting it across iterations would steal capacity from live delivery. +pub(super) async fn ready( + event_tx: &mpsc::Sender>, + at: Option, +) { + if let Some(at) = at { + if tokio::time::Instant::now() < at { + tokio::time::sleep_until(at).await; + } + // Use the channel's own race-free capacity wake, not periodic samples. + // Return ALL permits before recover_one rechecks capacity and intent. + if let Ok(permits) = event_tx + .reserve_many(event_tx.max_capacity().div_ceil(2)) + .await + { + drop(permits); + return; + } + } + // No loss or a closed receiver: no immediate-ready/error wake loop. + std::future::pending::<()>().await; +} + +fn next_channel(state: &BgState) -> Option> { + // One record per active intent, not per loss or per request generation. + // Least recently attempted prevents a repeatedly overflowing channel from + // starving other channels or membership. Missing filters fail closed. + state + .channel_dropped_since + .keys() + .filter(|ch| { + state.active_subscriptions.contains_key(ch) + && state.active_filters.contains_key(ch) + && !state.rate_limited_pending.contains_key(ch) + && !state.resubscribe_retry.contains(ch) + }) + .copied() + .map(Some) + .chain( + (state.membership_sub_active + && state.membership_dropped_since.is_some() + && !state.membership_resub_needed) + .then_some(None), + ) + .min_by_key(|ch| { + let sub = ch.map_or_else(|| MEMBERSHIP_NOTIF_SUB_ID.to_owned(), channel_sub_id); + (state.recovery.last_attempt.get(&sub).copied(), sub) + }) +} diff --git a/crates/buzz-acp/src/relay/recovery_tests.rs b/crates/buzz-acp/src/relay/recovery_tests.rs new file mode 100644 index 00000000000..4e51a0c6147 --- /dev/null +++ b/crates/buzz-acp/src/relay/recovery_tests.rs @@ -0,0 +1,388 @@ +//! Bounded synthetic WebSocket fixtures; no relay service, proxy or real agent. +use super::tests::{next_test_frame, seed_test_subscription, test_channel_filter, test_ws_pair}; +use super::*; + +fn fixture_event(channel: Uuid, n: u64, kind: u16) -> Event { + let keys = + Keys::parse("0000000000000000000000000000000000000000000000000000000000000001").unwrap(); + EventBuilder::new(Kind::Custom(kind), format!("synthetic-{n}")) + .tags([Tag::parse(["h", &channel.to_string()]).unwrap()]) + .custom_created_at(nostr::Timestamp::from(1_000 + n)) + .sign_with_keys(&keys) + .unwrap() +} + +async fn dispatch( + client: &mut WsStream, + state: &mut BgState, + tx: &mpsc::Sender>, + frame: Value, +) { + let (control_tx, _control_rx) = mpsc::channel(1); + assert!( + handle_ws_message( + Message::Text(frame.to_string().into()), + client, + tx, + &control_tx, + state, + &Keys::generate(), + "ws://127.0.0.1:1", + "synthetic-agent", + None, + ) + .await + ); +} + +#[tokio::test] +async fn repeated_overflow_recovers_only_affected_channel_after_capacity() { + let (mut client, mut server) = test_ws_pair().await; + let mut state = BgState::new(); + let channels: Vec<_> = (0..18).map(|_| Uuid::new_v4()).collect(); + for ch in &channels { + seed_test_subscription(&mut state, *ch); + } + let ch = channels[0]; + let sub = channel_sub_id(ch); + let (tx, mut rx) = mpsc::channel(256); + // Relay history is newest-first; the oldest dropped event must survive + // a watermark already advanced by much newer successfully-enqueued events. + let events: Vec<_> = (0..320).rev().map(|n| fixture_event(ch, n, 9)).collect(); + for event in &events { + dispatch(&mut client, &mut state, &tx, json!(["EVENT", sub, event])).await; + recovery::recover_one(&mut client, &mut state, &tx, "synthetic-agent").await; + } + assert_eq!(rx.len(), 256); + assert_eq!(state.channel_dropped_since[&ch], 1_000); + assert!(timeout(Duration::from_millis(30), server.next()) + .await + .is_err()); + for event in &events[..256] { + assert_eq!(rx.recv().await.unwrap().unwrap().event.id, event.id); + } + recovery::recover_one(&mut client, &mut state, &tx, "synthetic-agent").await; + let req = next_test_frame(&mut server).await; + assert_eq!(req[0], "REQ"); + assert_eq!(req[1], sub); + assert_eq!(req[2]["#h"], json!([ch.to_string()])); + assert_eq!(req[2]["kinds"], json!([9])); + assert_eq!(req[2]["since"], 995); + // Concurrent timer ticks / duplicate arrivals cannot replace the replay. + for _ in 0..40 { + recovery::recover_one(&mut client, &mut state, &tx, "synthetic-agent").await; + } + assert!(timeout(Duration::from_millis(30), server.next()) + .await + .is_err()); + for event in &events { + dispatch(&mut client, &mut state, &tx, json!(["EVENT", sub, event])).await; + } + dispatch(&mut client, &mut state, &tx, json!(["EOSE", sub])).await; + assert_eq!(rx.len(), 64, "delivered IDs must remain deduplicated"); + for event in &events[256..] { + assert_eq!(rx.recv().await.unwrap().unwrap().event.id, event.id); + } + assert_eq!(state.channel_since(&ch), Some(1_319)); + recovery::recover_one(&mut client, &mut state, &tx, "synthetic-agent").await; + assert!(timeout(Duration::from_millis(30), server.next()) + .await + .is_err()); + let live = fixture_event(ch, 400, 9); + dispatch(&mut client, &mut state, &tx, json!(["EVENT", sub, live])).await; + assert_eq!(rx.recv().await.unwrap().unwrap().event.id, live.id); + println!("18 subscriptions; 320 newest-first arrivals; 64 losses coalesced; 0 REQ while full; 1 targeted REQ; 320 unique deliveries + live"); +} + +#[tokio::test] +async fn socket_owner_services_ping_shutdown_and_coalesces_overflow_ticks() { + let (client, mut server) = test_ws_pair().await; + let (tx, mut rx) = mpsc::channel(1); + let (control_tx, _control_rx) = mpsc::channel(1); + let (cmd_tx, cmd_rx) = mpsc::channel(64); + let task = tokio::spawn(run_background_task( + client, + VecDeque::new(), + tx, + control_tx, + cmd_rx, + Keys::generate(), + "ws://127.0.0.1:1".into(), + "synthetic-agent".into(), + None, + )); + let channels: Vec<_> = (0..18).map(|_| Uuid::new_v4()).collect(); + for ch in &channels { + cmd_tx + .send(RelayCommand::Subscribe { + channel_id: *ch, + filter: test_channel_filter(), + replay_since: Some(1_000), + }) + .await + .unwrap(); + } + let mut subscriptions = 0; + while subscriptions < 18 { + let frame = timeout(Duration::from_secs(2), server.next()) + .await + .unwrap() + .unwrap() + .unwrap(); + match frame { + Message::Text(text) => { + let req: Value = serde_json::from_str(&text).unwrap(); + assert_eq!(req[0], "REQ"); + server + .send(Message::Text(json!(["EOSE", req[1]]).to_string().into())) + .await + .unwrap(); + subscriptions += 1; + } + Message::Ping(payload) => server.send(Message::Pong(payload)).await.unwrap(), + other => panic!("unexpected {other:?}"), + } + } + let sub = channel_sub_id(channels[0]); + for n in 0..40 { + server + .send(Message::Text( + json!(["EVENT", sub, fixture_event(channels[0], n, 9)]) + .to_string() + .into(), + )) + .await + .unwrap(); + } + server.send(Message::Ping(vec![42].into())).await.unwrap(); + timeout(Duration::from_secs(2), async { + loop { + match server.next().await.unwrap().unwrap() { + Message::Ping(payload) => server.send(Message::Pong(payload)).await.unwrap(), + Message::Pong(payload) => { + assert_eq!(payload.as_ref(), &[42]); + break; + } + other => panic!("no immediate all-channel recovery before ping: {other:?}"), + } + } + }) + .await + .unwrap(); + // Wait across a recovery tick with the consumer still full. + assert!(socket_frame(&mut server, Duration::from_millis(5_100)) + .await + .is_none()); + rx.recv().await.unwrap().unwrap(); + let frame = timeout(Duration::from_secs(6), server.next()) + .await + .unwrap() + .unwrap() + .unwrap(); + let req: Value = serde_json::from_str(frame.to_text().unwrap()).unwrap(); + assert_eq!(req[1], sub); + assert_eq!(req[2]["since"], 996); + assert!(timeout(Duration::from_millis(100), server.next()) + .await + .is_err()); + // Sustained lag: each replay is followed by another burst, without EOSE. + // Recovery must stay paced, not permanently stall or sweep healthy channels. + for round in 1..=3 { + let started = tokio::time::Instant::now(); + for n in round * 40..(round + 1) * 40 { + server + .send(Message::Text( + json!(["EVENT", sub, fixture_event(channels[0], n, 9)]) + .to_string() + .into(), + )) + .await + .unwrap(); + } + server.send(Message::Ping(vec![43].into())).await.unwrap(); + let pong = timeout(Duration::from_secs(1), server.next()) + .await + .unwrap() + .unwrap() + .unwrap(); + assert!( + matches!(pong, Message::Pong(_)), + "recovery preempted ping: {pong:?}" + ); + rx.recv().await.unwrap().unwrap(); + let frame = timeout(Duration::from_secs(6), server.next()) + .await + .unwrap() + .unwrap() + .unwrap(); + let req: Value = serde_json::from_str(frame.to_text().unwrap()).unwrap(); + assert_eq!(req[1], sub, "healthy channels must not be swept"); + assert!( + started.elapsed() >= Duration::from_secs(4), + "unpaced repeat" + ); + } + cmd_tx.send(RelayCommand::Shutdown).await.unwrap(); + timeout(Duration::from_secs(1), task) + .await + .unwrap() + .unwrap(); + println!("socket-owner seam: 18 live REQs, 39 coalesced losses, PONG while full, zero recovery across full-capacity timer tick, four paced targeted REQs over sustained lag without EOSE, responsive shutdown"); +} + +#[tokio::test] +async fn recovery_is_fair_and_paced_even_with_new_loss_and_stale_eose() { + let (mut client, mut server) = test_ws_pair().await; + let mut state = BgState::new(); + let channels = [Uuid::new_v4(), Uuid::new_v4()]; + for ch in channels { + seed_test_subscription(&mut state, ch); + state.channel_dropped_since.insert(ch, 600); + } + state.membership_sub_active = true; + state.membership_dropped_since = Some(500); + let (tx, _rx) = mpsc::channel(1); + let mut visited = HashSet::new(); + for round in 0..9 { + timeout( + Duration::from_millis(100), + recovery::ready(&tx, recovery::ready_at(&mut state)), + ) + .await + .unwrap(); + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + let req = next_test_frame(&mut server).await; + let sub = req[1].as_str().unwrap(); + if round < 3 { + assert!(visited.insert(sub.to_owned()), "starved intent"); + } + if sub == MEMBERSHIP_NOTIF_SUB_ID { + assert_eq!(req[2]["since"], 495); + state.membership_dropped_since = Some(500); + } else { + let ch = channel_id_from_sub_id(sub).unwrap(); + assert_eq!(req[2]["since"], 595); + state.channel_dropped_since.insert(ch, 600); + } + // Neither stale nor current EOSE creates completion state or erases loss. + dispatch(&mut client, &mut state, &tx, json!(["EOSE", sub])).await; + for _ in 0..30 { + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + } + assert!(timeout(Duration::from_millis(1), server.next()) + .await + .is_err()); + advance_clock(recovery::RECOVERY_INTERVAL).await; + } + for ch in channels { + state.active_subscriptions.remove(&ch); + state.clear_channel_state(&ch); + assert!(!state + .recovery + .last_attempt + .contains_key(&channel_sub_id(ch))); + } + assert_eq!(state.recovery.last_attempt.len(), 1); +} + +#[tokio::test] +async fn gate_headroom_failed_writes_and_reconnect_preserve_pending_attempts() { + let (mut client, mut server) = test_ws_pair().await; + let mut state = BgState::new(); + let ch = Uuid::new_v4(); + seed_test_subscription(&mut state, ch); + state.channel_dropped_since.insert(ch, 700); + let (tx, mut rx) = mpsc::channel(4); + for _ in 0..3 { + tx.try_send(None).unwrap(); + } + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + assert!(state.recovery.last_attempt.is_empty()); + rx.recv().await; + state.rate_limit_gate = Some(tokio::time::Instant::now() + Duration::from_secs(10)); + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + assert!(state.recovery.last_attempt.is_empty()); + advance_clock(Duration::from_secs(10)).await; + // Close locally, so the actual production writer fails deterministically. + client.close(None).await.unwrap(); + server.next().await; + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + assert_eq!(state.channel_dropped_since[&ch], 700); + let attempted = state.recovery.last_attempt.clone(); + for _ in 0..30 { + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + } + assert_eq!(state.recovery.last_attempt, attempted); + advance_clock(recovery::RECOVERY_INTERVAL).await; + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + assert_ne!( + state.recovery.last_attempt, attempted, + "failed write must be retried" + ); + assert_eq!(state.channel_dropped_since[&ch], 700); + + let (mut client, mut server) = test_ws_pair().await; + let (_cmd_tx, mut cmd_rx) = mpsc::channel(1); + assert!(matches!( + resubscribe_after_reconnect(&mut client, &mut cmd_rx, &mut state, "agent", true,).await, + ResubscribeResult::Ok + )); + let req = next_test_frame(&mut server).await; + assert_eq!(req[1], channel_sub_id(ch)); + assert_eq!(req[2]["since"], 695); + assert!(!state.channel_dropped_since.contains_key(&ch)); + // This is deliberately the baseline write-retirement contract, not a receipt. +} + +async fn advance_clock(duration: Duration) { + tokio::time::pause(); + tokio::time::advance(duration).await; + tokio::time::resume(); +} + +#[tokio::test] +async fn blocked_recovery_write_is_bounded_and_retains_loss() { + let (mut client, _stalled_server) = test_ws_pair().await; + let mut state = BgState::new(); + let ch = Uuid::new_v4(); + seed_test_subscription(&mut state, ch); + // Bounded 16MB JSON request exceeds loopback TCP buffering. The server does + // not read it. This tests the real production write/timeout, not a mock sink. + state.active_filters.get_mut(&ch).unwrap().kinds = Some(vec![9; 8_000_000]); + state.channel_dropped_since.insert(ch, 700); + let (tx, _rx) = mpsc::channel(1); + let started = tokio::time::Instant::now(); + timeout( + Duration::from_secs(15), + recovery::recover_one(&mut client, &mut state, &tx, "agent"), + ) + .await + .unwrap(); + assert!(started.elapsed() >= Duration::from_secs(WS_SEND_TIMEOUT_SECS)); + assert_eq!(state.channel_dropped_since[&ch], 700); + let attempted = state.recovery.last_attempt.clone(); + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + assert_eq!(state.recovery.last_attempt, attempted); +} + +// Keep the fixture responsive to independent client keepalives while checking +// recovery traffic. Wall-clock scheduling may deliver the initial ping late. +async fn socket_frame( + server: &mut WebSocketStream, + duration: Duration, +) -> Option { + let deadline = tokio::time::Instant::now() + duration; + loop { + match tokio::time::timeout_at(deadline, server.next()).await { + Err(_) => return None, + Ok(Some(Ok(Message::Ping(payload)))) => { + server.send(Message::Pong(payload)).await.unwrap(); + } + Ok(Some(Ok(frame))) => return Some(frame), + other => panic!("unexpected socket state: {other:?}"), + } + } +} + +#[path = "recovery_wake_tests.rs"] +mod wake; diff --git a/crates/buzz-acp/src/relay/recovery_wake_tests.rs b/crates/buzz-acp/src/relay/recovery_wake_tests.rs new file mode 100644 index 00000000000..de27c7d06c9 --- /dev/null +++ b/crates/buzz-acp/src/relay/recovery_wake_tests.rs @@ -0,0 +1,507 @@ +//! Capacity-wake boundaries and the independently reproduced R1 schedule. +use super::*; + +// Reviewer-authored, exact-source comparison of recurring headroom at the socket owner. +// A small periodically refilled queue is empty for most of each five-second period. +async fn review_count_until( + server: &mut WebSocketStream, + deadline: tokio::time::Instant, +) -> usize { + let mut count = 0; + while tokio::time::Instant::now() < deadline { + let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); + match socket_frame(server, remaining).await { + None => break, + Some(Message::Text(text)) => { + let frame: Value = serde_json::from_str(&text).unwrap(); + assert_eq!(frame[0], "REQ"); + count += 1; + } + other => panic!("unexpected {other:?}"), + } + } + count +} + +async fn review_barrier(server: &mut WebSocketStream) -> usize { + server.send(Message::Ping(vec![77].into())).await.unwrap(); + let mut count = 0; + loop { + match socket_frame(server, Duration::from_secs(2)).await.unwrap() { + Message::Pong(payload) => { + assert_eq!(payload.as_ref(), &[77]); + return count; + } + Message::Text(text) => { + let frame: Value = serde_json::from_str(&text).unwrap(); + assert_eq!(frame[0], "REQ"); + count += 1; + } + other => panic!("unexpected {other:?}"), + } + } +} + +#[tokio::test] +async fn review_recurring_headroom_between_ticks_gets_an_attempt() { + let (client, mut server) = test_ws_pair().await; + let (tx, mut rx) = mpsc::channel(1); + let (control_tx, _control_rx) = mpsc::channel(1); + let (cmd_tx, cmd_rx) = mpsc::channel(8); + let start = tokio::time::Instant::now(); + let task = tokio::spawn(run_background_task( + client, + VecDeque::new(), + tx, + control_tx, + cmd_rx, + Keys::generate(), + "ws://127.0.0.1:1".into(), + "agent".into(), + None, + )); + let ch = Uuid::new_v4(); + let sub = channel_sub_id(ch); + cmd_tx + .send(RelayCommand::Subscribe { + channel_id: ch, + filter: test_channel_filter(), + replay_since: Some(1000), + }) + .await + .unwrap(); + let frame = socket_frame(&mut server, Duration::from_secs(2)) + .await + .unwrap(); + let req: Value = serde_json::from_str(frame.to_text().unwrap()).unwrap(); + assert_eq!(req[1], sub); + let lost = fixture_event(ch, 1, 9); + for event in [fixture_event(ch, 0, 9), lost.clone()] { + server + .send(Message::Text( + json!(["EVENT", sub, event]).to_string().into(), + )) + .await + .unwrap(); + } + let mut attempts = review_barrier(&mut server).await; + assert_eq!(rx.len(), 1); + rx.recv().await.unwrap().unwrap(); + for round in 1..=4 { + // Headroom until 1s before the tick; then only one live arrival, no new + // overflow. The queue stays full across the tick and drains 0.5s later. + attempts += review_count_until( + &mut server, + start + Duration::from_millis(round * 5000 - 1000), + ) + .await; + assert_eq!(rx.len(), 0); + server + .send(Message::Text( + json!(["EVENT", sub, fixture_event(ch, 10 + round, 9)]) + .to_string() + .into(), + )) + .await + .unwrap(); + attempts += review_barrier(&mut server).await; + assert_eq!(rx.len(), 1); + attempts += review_count_until( + &mut server, + start + Duration::from_millis(round * 5000 + 500), + ) + .await; + rx.recv().await.unwrap().unwrap(); + } + println!("REVIEW periodic consumer: 4 full-at-tick windows, empty >=3.5s each period, recovery_requests={attempts}"); + assert_eq!( + attempts, 1, + "recurring headroom must not strand the first attempt" + ); + // Return the missing event using the actual requested stable subscription. + server + .send(Message::Text( + json!(["EVENT", sub, lost]).to_string().into(), + )) + .await + .unwrap(); + assert_eq!(review_barrier(&mut server).await, 0); + assert_eq!(rx.recv().await.unwrap().unwrap().event.id, lost.id); + let after = review_count_until(&mut server, start + Duration::from_millis(25_500)).await; + assert_eq!( + after, 0, + "no timer churn or extra requests after successful write" + ); + println!( + "REVIEW continuous-headroom control: additional_requests={after}; missing event delivered" + ); + cmd_tx.send(RelayCommand::Shutdown).await.unwrap(); + timeout(Duration::from_secs(2), task) + .await + .unwrap() + .unwrap(); +} + +/// Same real socket owner as production, with no control of its internal state. +struct Owner { + server: WebSocketStream, + rx: mpsc::Receiver>, + cmd: mpsc::Sender, + task: tokio::task::JoinHandle<()>, + ch: Uuid, +} + +impl Owner { + async fn new(capacity: usize) -> Self { + let (client, server) = test_ws_pair().await; + let (tx, rx) = mpsc::channel(capacity); + let (control_tx, _control_rx) = mpsc::channel(1); + let (cmd, cmd_rx) = mpsc::channel(8); + let task = tokio::spawn(run_background_task( + client, + VecDeque::new(), + tx, + control_tx, + cmd_rx, + Keys::generate(), + "ws://127.0.0.1:1".into(), + "agent".into(), + None, + )); + let mut owner = Self { + server, + rx, + cmd, + task, + ch: Uuid::new_v4(), + }; + owner.subscribe().await; + owner + } + + async fn subscribe(&mut self) { + self.cmd + .send(RelayCommand::Subscribe { + channel_id: self.ch, + filter: test_channel_filter(), + replay_since: Some(1000), + }) + .await + .unwrap(); + let req = self.request(Duration::from_secs(2)).await; + assert_eq!(req[1], channel_sub_id(self.ch)); + } + + async fn event(&mut self, n: u64) { + self.server + .send(Message::Text( + json!([ + "EVENT", + channel_sub_id(self.ch), + fixture_event(self.ch, n, 9) + ]) + .to_string() + .into(), + )) + .await + .unwrap(); + } + + async fn request(&mut self, duration: Duration) -> Value { + let frame = socket_frame(&mut self.server, duration) + .await + .expect("recovery not woken"); + let req: Value = serde_json::from_str(frame.to_text().unwrap()).unwrap(); + assert_eq!(req[0], "REQ"); + req + } + + async fn shutdown(self) { + self.cmd.send(RelayCommand::Shutdown).await.unwrap(); + timeout(Duration::from_secs(1), self.task) + .await + .unwrap() + .unwrap(); + } +} + +#[tokio::test] +async fn capacity_flapping_cannot_storm_or_delay_an_allowed_attempt() { + let mut owner = Owner::new(1).await; + owner.event(0).await; + owner.event(1).await; + assert_eq!(review_barrier(&mut owner.server).await, 0); + owner.rx.recv().await.unwrap(); + let first = owner.request(Duration::from_millis(500)).await; + let first_at = tokio::time::Instant::now(); + assert_eq!(first[2]["since"], 996); + // New loss, then rapid full/empty transitions during the attempt cooldown. + // No socket activity or capacity transition may reset or bypass that bound. + for n in 1..=20 { + owner.event(n * 2).await; + owner.event(n * 2 + 1).await; + assert_eq!(review_barrier(&mut owner.server).await, 0); + owner.rx.recv().await.unwrap(); + assert!(socket_frame(&mut owner.server, Duration::from_millis(10)) + .await + .is_none()); + } + let req = owner.request(Duration::from_secs(6)).await; + assert_eq!(req[1], channel_sub_id(owner.ch)); + // Arrival timestamps approximate send completion; leave tolerance for TCP. + assert!(first_at.elapsed() >= Duration::from_millis(4_900)); + assert!(first_at.elapsed() < Duration::from_secs(6)); + assert_eq!(req[2]["since"], 998); + assert!(socket_frame(&mut owner.server, Duration::from_millis(100)) + .await + .is_none()); + owner.shutdown().await; +} + +#[tokio::test] +async fn partial_capacity_wait_does_not_steal_live_slots_and_cancels_on_unsubscribe() { + let mut owner = Owner::new(5).await; // odd capacity: threshold rounds UP to 3 + for n in 0..6 { + owner.event(n).await; + } + assert_eq!(review_barrier(&mut owner.server).await, 0); + owner.rx.recv().await.unwrap(); // partial reservation, insufficient for replay + assert!(socket_frame(&mut owner.server, Duration::from_millis(30)) + .await + .is_none()); + owner.event(6).await; + assert_eq!(review_barrier(&mut owner.server).await, 0); + assert_eq!( + owner.rx.len(), + 5, + "select must release partial permits BEFORE try_send" + ); + for _ in 0..2 { + owner.rx.recv().await.unwrap(); + } + assert!(socket_frame(&mut owner.server, Duration::from_millis(30)) + .await + .is_none()); + owner.rx.recv().await.unwrap(); // exactly three slots free, prompt capacity wake + let req = owner.request(Duration::from_millis(500)).await; + assert_eq!(req[2]["since"], 1000); + assert_eq!( + owner + .rx + .recv() + .await + .unwrap() + .unwrap() + .event + .created_at + .as_secs(), + 1004 + ); + assert_eq!( + owner + .rx + .recv() + .await + .unwrap() + .unwrap() + .event + .created_at + .as_secs(), + 1006 + ); + + // Wait out cooldown then create another pending loss and partial reservation. + tokio::time::sleep(recovery::RECOVERY_INTERVAL).await; + for n in 7..13 { + owner.event(n).await; + } + assert_eq!(review_barrier(&mut owner.server).await, 0); + owner.rx.recv().await.unwrap(); + assert!(socket_frame(&mut owner.server, Duration::from_millis(30)) + .await + .is_none()); + owner + .cmd + .send(RelayCommand::Unsubscribe { + channel_id: owner.ch, + }) + .await + .unwrap(); + let close = socket_frame(&mut owner.server, Duration::from_secs(1)) + .await + .unwrap(); + let close: Value = serde_json::from_str(close.to_text().unwrap()).unwrap(); + assert_eq!(close[0], "CLOSE"); + while owner.rx.try_recv().is_ok() {} + assert!(socket_frame(&mut owner.server, Duration::from_millis(100)) + .await + .is_none()); + owner.subscribe().await; + assert!(socket_frame(&mut owner.server, Duration::from_millis(100)) + .await + .is_none()); + // A re-added intent can record and recover fresh loss immediately. + for n in 20..26 { + owner.event(n).await; + } + assert_eq!(review_barrier(&mut owner.server).await, 0); + while owner.rx.try_recv().is_ok() {} + let req = owner.request(Duration::from_millis(500)).await; + assert_eq!(req[2]["since"], 1020); + owner.shutdown().await; +} + +#[tokio::test] +async fn shutdown_and_transport_loss_cancel_a_capacity_wait() { + let mut owner = Owner::new(1).await; + owner.event(0).await; + owner.event(1).await; + assert_eq!(review_barrier(&mut owner.server).await, 0); + // Full queue cannot block commands or processing an actual socket close. + owner.server.close(None).await.unwrap(); + owner.shutdown().await; + let mut owner = Owner::new(1).await; + owner.event(0).await; + owner.event(1).await; + assert_eq!(review_barrier(&mut owner.server).await, 0); + owner.shutdown().await; +} + +#[tokio::test] +async fn readiness_gate_ownership_and_attempt_deadlines_are_not_polling_ticks() { + let (mut client, mut server) = test_ws_pair().await; + let mut state = BgState::new(); + let ch = Uuid::new_v4(); + seed_test_subscription(&mut state, ch); + let (tx, mut rx) = mpsc::channel(4); + assert!(recovery::ready_at(&mut state).is_none()); + state.channel_dropped_since.insert(ch, 700); + state + .rate_limited_pending + .insert(ch, tokio::time::Instant::now()); + assert!(recovery::ready_at(&mut state).is_none()); + state.rate_limited_pending.clear(); + state.resubscribe_retry.insert(ch); + assert!(recovery::ready_at(&mut state).is_none()); + state.resubscribe_retry.clear(); + state.rate_limit_gate = Some(tokio::time::Instant::now() + Duration::from_millis(80)); + let at = recovery::ready_at(&mut state); + assert_eq!(at, state.rate_limit_gate); + assert!(timeout(Duration::from_millis(20), recovery::ready(&tx, at)) + .await + .is_err()); + timeout(Duration::from_millis(200), recovery::ready(&tx, at)) + .await + .unwrap(); + recovery::recover_one(&mut client, &mut state, &tx, "agent").await; + next_test_frame(&mut server).await; + state.channel_dropped_since.insert(ch, 800); + let at = recovery::ready_at(&mut state).unwrap(); + let remaining = at.saturating_duration_since(tokio::time::Instant::now()); + assert!(remaining > Duration::from_millis(4900)); + // Neither new loss nor an intervening gate shorter than cooldown delays it. + state.rate_limit_gate = Some(tokio::time::Instant::now() + Duration::from_millis(10)); + assert_eq!(recovery::ready_at(&mut state), Some(at)); + state.rate_limit_gate = Some(at + Duration::from_secs(1)); + assert_eq!(recovery::ready_at(&mut state), state.rate_limit_gate); + advance_clock(Duration::from_secs(6)).await; + // Cancellation releases partial permits. No hidden reservation survives it. + for _ in 0..3 { + tx.try_send(None).unwrap(); + } + assert!(timeout( + Duration::from_millis(20), + recovery::ready(&tx, recovery::ready_at(&mut state)) + ) + .await + .is_err()); + assert_eq!(tx.capacity(), 1); + rx.recv().await.unwrap(); + timeout( + Duration::from_millis(100), + recovery::ready(&tx, recovery::ready_at(&mut state)), + ) + .await + .unwrap(); + assert_eq!(tx.capacity(), 2); + drop(rx); + assert!(timeout( + Duration::from_millis(20), + recovery::ready(&tx, recovery::ready_at(&mut state)) + ) + .await + .is_err()); +} + +#[tokio::test] +async fn readiness_uses_channel_wakes_without_idle_churn_or_lost_capacity() { + use std::future::Future; + use std::sync::{ + atomic::{AtomicUsize, Ordering}, + Arc, + }; + use std::task::{Context, Wake, Waker}; + #[derive(Default)] + struct Wakes(AtomicUsize); + impl Wake for Wakes { + fn wake(self: Arc) { + self.0.fetch_add(1, Ordering::SeqCst); + } + fn wake_by_ref(self: &Arc) { + self.0.fetch_add(1, Ordering::SeqCst); + } + } + let wakes = Arc::new(Wakes::default()); + let waker = Waker::from(wakes.clone()); + let mut cx = Context::from_waker(&waker); + let (tx, mut rx) = mpsc::channel(4); + for _ in 0..4 { + tx.try_send(None).unwrap(); + } + let mut idle = Box::pin(recovery::ready(&tx, None)); + assert!(idle.as_mut().poll(&mut cx).is_pending()); + rx.recv().await.unwrap(); + rx.recv().await.unwrap(); + assert_eq!( + wakes.0.load(Ordering::SeqCst), + 0, + "no loss: no capacity subscription" + ); + drop(idle); + // Capacity freed before registration cannot be lost; ready on first poll. + let now = Some(tokio::time::Instant::now()); + let mut ready = Box::pin(recovery::ready(&tx, now)); + assert!(ready.as_mut().poll(&mut cx).is_ready()); + assert_eq!(tx.capacity(), 2, "successful readiness returns all permits"); + drop(ready); + for _ in 0..2 { + tx.try_send(None).unwrap(); + } + let mut ready = Box::pin(recovery::ready(&tx, now)); + assert!(ready.as_mut().poll(&mut cx).is_pending()); + assert_eq!(wakes.0.load(Ordering::SeqCst), 0); + rx.recv().await.unwrap(); + assert_eq!( + wakes.0.load(Ordering::SeqCst), + 0, + "below threshold: do not wake" + ); + rx.recv().await.unwrap(); + assert_eq!( + wakes.0.load(Ordering::SeqCst), + 1, + "threshold: channel wakes its waiter" + ); + assert!(ready.as_mut().poll(&mut cx).is_ready()); + assert_eq!(tx.capacity(), 2); + drop(ready); + drop(rx); + let mut closed = Box::pin(recovery::ready(&tx, now)); + let before = wakes.0.load(Ordering::SeqCst); + assert!(closed.as_mut().poll(&mut cx).is_pending()); + assert_eq!( + wakes.0.load(Ordering::SeqCst), + before, + "closed: no self-wake loop" + ); +} diff --git a/crates/buzz-auth/src/nip98.rs b/crates/buzz-auth/src/nip98.rs index d3ece3fdfc2..a5ed98167cb 100644 --- a/crates/buzz-auth/src/nip98.rs +++ b/crates/buzz-auth/src/nip98.rs @@ -161,9 +161,11 @@ pub fn verify_nip98_event( ))); } } - let payload_tag = event.tags.find(TagKind::Payload).and_then(|t| t.content()); - - if let (Some(payload_hex), Some(body_bytes)) = (payload_tag, body) { + // Keep a present-but-malformed tag distinct from an absent (optional) tag. + if let (Some(payload_tag), Some(body_bytes)) = (event.tags.find(TagKind::Payload), body) { + let payload_hex = payload_tag.content().ok_or_else(|| { + AuthError::Nip98Invalid("payload tag is missing its SHA-256 hash".to_string()) + })?; let computed: [u8; 32] = Sha256::digest(body_bytes).into(); let computed_hex = hex::encode(computed); if computed_hex != payload_hex { @@ -313,6 +315,26 @@ mod tests { assert!(matches!(result, Err(AuthError::Nip98Invalid(_)))); } + #[test] + fn payload_tag_without_hash_rejected_with_body() { + let keys = Keys::generate(); + for payload in [vec!["payload"], vec!["payload", ""]] { + let json = make_nip98_event_raw_tags( + &keys, + vec![ + nostr::Tag::parse(["u", TEST_URL]).unwrap(), + nostr::Tag::parse(["method", TEST_METHOD]).unwrap(), + nostr::Tag::parse(payload).unwrap(), + ], + ); + let result = verify_nip98_event(&json, TEST_URL, TEST_METHOD, Some(b"some body")); + assert!( + matches!(result, Err(AuthError::Nip98Invalid(_))), + "{result:?}" + ); + } + } + #[test] fn payload_tag_absent_with_body_passes() { // Contract: the shared verifier does NOT require a payload tag even when diff --git a/crates/buzz-cli/README.md b/crates/buzz-cli/README.md index ef9ce7c7921..85a04b47120 100644 --- a/crates/buzz-cli/README.md +++ b/crates/buzz-cli/README.md @@ -168,6 +168,7 @@ stored rules in `validation_error` so an owner can remove and repair them. | `repos` | `create` | Announce a git repository (NIP-34) | | | `get` | Get a repository announcement | | | `list` | List repository announcements | +| | `default-branch get/set` | Read or select an existing default branch (requires relay support) | | | `protect list` | List branch and tag protection rules | | | `protect set` | Create or replace a protection rule | | | `protect remove` | Remove a protection rule | @@ -196,3 +197,29 @@ stdout: raw relay JSON stderr: {"error": "category", "message": "detail"} exit: 0=ok 1=user 2=network 3=auth 4=other 5=write conflict ``` + + +### Default branch + +After deploying relay support, select an existing published branch without +renaming or deleting any branch: + +```bash +buzz repos default-branch get --owner --id my-repo +buzz repos default-branch set --owner --id my-repo --branch main +# For an explicitly reviewed version, use the manifest digest returned by get: +buzz repos default-branch set --owner --id my-repo --branch main \ + --expected-manifest +``` + +`--owner` defaults to the signing identity, not an agent's attested human owner. +`set` without `--expected-manifest` reads the current version first. Success +returns `branch`, `head`, `manifest` and `changed`; `get` omits `changed`. +A stale version returns conflict (exit 5). Ambiguous write failures return +`delivery_unknown` with `retryable:false` and the original digest: **read before +retrying**, and do not blindly re-run against a newly fetched version. + +The signer must be a current channel member and a repository manager, directly +or through an unrestricted, valid NIP-OA owner attestation; permission to push is +not permission to change the default. See the +[protocol and authorization contract](../../docs/git-on-object-storage.md#default-branch-management). diff --git a/crates/buzz-cli/src/client.rs b/crates/buzz-cli/src/client.rs index 75c87aa427f..6ade19f1cad 100644 --- a/crates/buzz-cli/src/client.rs +++ b/crates/buzz-cli/src/client.rs @@ -911,6 +911,56 @@ impl BuzzClient { .await } + /// Send a state-changing JSON command exactly once. Ambiguous delivery + /// never invites an automatic re-run with a newly observed version. + pub async fn post_json_once_authed( + &self, + path: &str, + body: &serde_json::Value, + ) -> Result { + let url = format!("{}{path}", self.relay_url); + let body = serde_json::to_vec(body).map_err(|e| CliError::Other(e.to_string()))?; + let auth = sign_nip98(&self.keys, "POST", &url, Some(&body))?; + let unknown = |detail: String| CliError::DeliveryUnknown(detail); + let http = reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .timeout(env_duration_secs("BUZZ_TIMEOUT_SECS", 30)) + .connect_timeout(env_duration_secs("BUZZ_CONNECT_TIMEOUT_SECS", 15)) + .build()?; + let response = self + .with_auth_tag( + http.post(&url) + .header("Authorization", auth) + .header("Content-Type", "application/json") + .body(body), + ) + .send() + .await + .map_err(|e| { + if e.is_connect() || e.is_builder() { + CliError::Network(e) + } else { + unknown(e.to_string()) + } + })?; + let status = response.status(); + let body = response.text().await.map_err(|e| unknown(e.to_string()))?; + let message = extract_relay_message_field(&body).unwrap_or_else(|| body.clone()); + if status.is_server_error() + || status.is_redirection() + || (status.as_u16() == 429 && !message.starts_with("rate-limited:")) + { + return Err(unknown(format!("HTTP {}: {message}", status.as_u16()))); + } + if !status.is_success() { + return Err(CliError::Relay { + status: status.as_u16(), + body: message, + }); + } + Ok(body) + } + /// Submit a signed Nostr event via POST /events. /// /// For non-idempotent moderation command kinds (9040–9044), an ambiguous diff --git a/crates/buzz-cli/src/commands/mod.rs b/crates/buzz-cli/src/commands/mod.rs index 7ed03f9d060..fed35ec201e 100644 --- a/crates/buzz-cli/src/commands/mod.rs +++ b/crates/buzz-cli/src/commands/mod.rs @@ -16,6 +16,7 @@ pub mod pr; pub mod project_channel; pub mod projects; pub mod reactions; +mod repo_default_branch; pub mod repos; pub mod social; pub mod upload; diff --git a/crates/buzz-cli/src/commands/repo_default_branch.rs b/crates/buzz-cli/src/commands/repo_default_branch.rs new file mode 100644 index 00000000000..23aba9f5426 --- /dev/null +++ b/crates/buzz-cli/src/commands/repo_default_branch.rs @@ -0,0 +1,314 @@ +//! Thin client for the relay's CAS-backed default-branch operation. + +use serde::Deserialize; +use serde_json::{json, Value}; + +use crate::{client::BuzzClient, error::CliError, ReposDefaultBranchCmd}; + +#[derive(Deserialize)] +struct DefaultBranch { + branch: String, + head: String, + manifest: String, +} + +fn parse_snapshot(raw: &str) -> Result { + let value: Value = serde_json::from_str(raw).map_err(|_| { + CliError::Other("relay did not return default-branch JSON; it may need updating".into()) + })?; + let snapshot: DefaultBranch = serde_json::from_value(value.clone()).map_err(|_| { + CliError::Other( + "relay response is missing default-branch state; it may need updating".into(), + ) + })?; + crate::validate::validate_hex64(&snapshot.manifest)?; + if snapshot.branch.is_empty() || snapshot.head != format!("refs/heads/{}", snapshot.branch) { + return Err(CliError::Other("relay returned an invalid HEAD".into())); + } + Ok(value) +} + +fn classify(error: CliError) -> CliError { + match error { + CliError::Relay { status: 409, body } => CliError::Conflict(body), + CliError::Relay { status: 401 | 403, body } => CliError::Auth(body), + CliError::Relay { status: 400, body } => CliError::Usage(body), + CliError::Relay { status: 404, body } => CliError::NotFound(format!("{body}; check repository access and that this relay supports default-branch management")), + other => other, + } +} + +pub(super) async fn dispatch( + command: ReposDefaultBranchCmd, + client: &BuzzClient, +) -> Result<(), CliError> { + let (id, owner, update) = match command { + ReposDefaultBranchCmd::Get { id, owner } => (id, owner, None), + ReposDefaultBranchCmd::Set { + id, + owner, + branch, + expected_manifest, + } => (id, owner, Some((branch, expected_manifest))), + }; + crate::validate::validate_repo_id(&id)?; + let owner = owner.unwrap_or_else(|| client.keys().public_key().to_hex()); + crate::validate::validate_hex64(&owner)?; + let path = format!("/git/{owner}/{id}/default-branch"); + let result = match update { + None => parse_snapshot(&client.get_authed(&path).await.map_err(classify)?)?, + Some((branch, expected)) => { + let expected = match expected { + Some(digest) => { + crate::validate::validate_hex64(&digest)?; + digest + } + None => { + let raw = client.get_authed(&path).await.map_err(classify)?; + let snapshot = parse_snapshot(&raw)?; + snapshot["manifest"] + .as_str() + .ok_or_else(|| CliError::Other("missing manifest".into()))? + .to_string() + } + }; + let uncertain = |detail: String| { + CliError::DeliveryUnknown(format!( + "{detail}; attempted branch {branch:?} against manifest {expected}. Read the current default branch before deciding what to do; do not blindly re-run set without --expected-manifest {expected}" + )) + }; + let raw = client + .post_json_once_authed( + &path, + &json!({"branch": branch, "expected_manifest": expected}), + ) + .await + .map_err(|e| match e { + CliError::DeliveryUnknown(detail) => uncertain(detail), + other => classify(other), + })?; + let result = parse_snapshot(&raw).map_err(|e| uncertain(e.to_string()))?; + if !result["changed"].is_boolean() || result["branch"].as_str() != Some(branch.as_str()) + { + return Err(uncertain( + "relay did not confirm the default-branch update".into(), + )); + } + result + } + }; + println!("{result}"); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use axum::{ + body::{Body, Bytes}, + http::{HeaderMap, Response, StatusCode}, + routing::get, + Router, + }; + use base64::Engine; + use clap::Parser; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{Arc, Mutex}; + + #[test] + fn default_branch_cli_parses_get_and_set() { + for operation in [ + vec!["get", "--id", "demo"], + vec![ + "set", + "--id", + "demo", + "--branch", + "release/v1", + "--expected-manifest", + &"a".repeat(64), + ], + ] { + let mut args = vec!["buzz", "repos", "default-branch"]; + args.extend(operation); + assert!(crate::Cli::try_parse_from(args).is_ok()); + } + assert!(crate::Cli::try_parse_from([ + "buzz", + "repos", + "default-branch", + "set", + "--id", + "demo" + ]) + .is_err()); + } + + #[tokio::test] + async fn default_branch_reads_validate_state_before_any_mutation() { + for (branch, head, valid) in [ + (Some("release/v1"), "refs/heads/release/v1", true), + (None, "refs/heads/main", false), + (Some("main"), "refs/tags/main", false), + (Some("main"), "refs/heads/other", false), + (Some(""), "refs/heads/", false), + ] { + let posts = Arc::new(AtomicUsize::new(0)); + let post_count = posts.clone(); + let keys = nostr::Keys::generate(); + let path = format!("/git/{}/demo/default-branch", keys.public_key().to_hex()); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let route = get(move || async move { + axum::Json(json!({"branch":branch, "head":head, "manifest":"a".repeat(64)})) + }) + .post(move || { + post_count.fetch_add(1, Ordering::SeqCst); + async { StatusCode::INTERNAL_SERVER_ERROR } + }); + let app = Router::new().route(&path, route); + let server = tokio::spawn(async { axum::serve(listener, app).await.unwrap() }); + let client = BuzzClient::new(url, keys, None, None).unwrap(); + let result = dispatch( + ReposDefaultBranchCmd::Get { + id: "demo".into(), + owner: None, + }, + &client, + ) + .await; + assert_eq!(result.is_ok(), valid, "{branch:?} {head}: {result:?}"); + if !valid { + let error = dispatch( + ReposDefaultBranchCmd::Set { + id: "demo".into(), + owner: None, + branch: "main".into(), + expected_manifest: None, + }, + &client, + ) + .await + .unwrap_err(); + assert!( + !matches!(error, CliError::DeliveryUnknown(_)), + "no mutation attempted: {error}" + ); + } + assert_eq!(posts.load(Ordering::SeqCst), 0); + server.abort(); + } + } + + #[tokio::test] + async fn default_branch_command_binds_observed_digest_and_does_not_retry_or_follow_redirects() { + let valid = json!({"head":"refs/heads/main", "branch":"main", "manifest":"b".repeat(64), "changed":true}); + let mut no_op = valid.clone(); + no_op["changed"] = json!(false); + no_op["manifest"] = json!("a".repeat(64)); + let mut cases = vec![(200u16, valid.clone(), true), (200, no_op, true)]; + for status in [307, 308, 409, 500, 502, 503, 504] { + cases.push((status, json!({"error":"test outcome"}), false)); + } + for (field, value) in [ + ("branch", None), + ("branch", Some(json!(""))), + ("head", Some(json!("refs/tags/main"))), + ("head", Some(json!("refs/heads/other"))), + ("manifest", Some(json!("not-a-digest"))), + ("changed", None), + ] { + let mut invalid = valid.clone(); + if let Some(value) = value { + invalid[field] = value; + } else { + invalid.as_object_mut().unwrap().remove(field); + } + cases.push((200, invalid, false)); + } + let mut other_branch = valid; + other_branch["branch"] = json!("other"); + other_branch["head"] = json!("refs/heads/other"); + cases.push((200, other_branch, false)); + for (status, reply, success) in cases { + let posts = Arc::new(AtomicUsize::new(0)); + let gets = Arc::new(AtomicUsize::new(0)); + let captured = Arc::new(Mutex::new(None)); + let keys = nostr::Keys::generate(); + let owner = keys.public_key().to_hex(); + let path = format!("/git/{owner}/demo/default-branch"); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let expected_url = format!("{url}{path}"); + let get_count = gets.clone(); + let post_count = posts.clone(); + let capture = captured.clone(); + let route = get(move || { + get_count.fetch_add(1, Ordering::SeqCst); + async { axum::Json(json!({"head":"refs/heads/legacy", "branch":"legacy", "manifest":"a".repeat(64)})) } + }).post(move |headers: HeaderMap, body: Bytes| { + let post_count = post_count.clone(); + let capture = capture.clone(); + let expected_url = expected_url.clone(); + let reply = reply.clone(); + async move { + post_count.fetch_add(1, Ordering::SeqCst); + let auth = headers["authorization"].to_str().unwrap().strip_prefix("Nostr ").unwrap(); + let event = String::from_utf8(base64::engine::general_purpose::STANDARD.decode(auth).unwrap()).unwrap(); + let event: nostr::Event = serde_json::from_str(&event).unwrap(); + event.verify().unwrap(); + assert!(event.tags.iter().any(|t| t.as_slice() == ["u", &expected_url])); + assert!(event.tags.iter().any(|t| t.as_slice() == ["method", "POST"])); + use sha2::Digest; + let digest = hex::encode(sha2::Sha256::digest(&body)); + assert!(event.tags.iter().any(|t| t.as_slice() == ["payload", &digest])); + *capture.lock().unwrap() = Some(serde_json::from_slice::(&body).unwrap()); + Response::builder().status(status).header("location", "/redirect-target") + .body(Body::from(reply.to_string())).unwrap() + } + }); + let redirected = posts.clone(); + let app = Router::new().route(&path, route).route( + "/redirect-target", + axum::routing::post(move || { + redirected.fetch_add(1, Ordering::SeqCst); + async { StatusCode::OK } + }), + ); + let server = tokio::spawn(async { axum::serve(listener, app).await.unwrap() }); + let client = BuzzClient::new(url, keys, None, None).unwrap(); + let result = crate::commands::repos::dispatch( + crate::ReposCmd::DefaultBranch(ReposDefaultBranchCmd::Set { + id: "demo".into(), + owner: None, + branch: "main".into(), + expected_manifest: None, + }), + &client, + ) + .await; + assert_eq!(gets.load(Ordering::SeqCst), 1); + assert_eq!( + posts.load(Ordering::SeqCst), + 1, + "HTTP {status} must not cause another POST" + ); + assert_eq!( + *captured.lock().unwrap(), + Some(json!({"branch":"main", "expected_manifest":"a".repeat(64)})) + ); + match status { + 200 if success => assert!(result.is_ok(), "{result:?}"), + 409 => assert!(matches!(result, Err(CliError::Conflict(_)))), + _ => { + let error = result.unwrap_err(); + assert!(matches!(error, CliError::DeliveryUnknown(_)), "{error}"); + assert!(!crate::error::is_retryable_error(&error)); + assert!(error.to_string().contains(&"a".repeat(64))); + assert!(error.to_string().contains("attempted branch \"main\"")); + } + } + server.abort(); + } + } +} diff --git a/crates/buzz-cli/src/commands/repos.rs b/crates/buzz-cli/src/commands/repos.rs index 886d6e04192..3e6b0ac15cf 100644 --- a/crates/buzz-cli/src/commands/repos.rs +++ b/crates/buzz-cli/src/commands/repos.rs @@ -442,6 +442,9 @@ pub async fn dispatch(cmd: crate::ReposCmd, client: &BuzzClient) -> Result<(), C ReposCmd::Get { id, owner } => cmd_get_repo(client, &id, owner.as_deref()).await, ReposCmd::List { owner, limit } => cmd_list_repos(client, owner.as_deref(), limit).await, ReposCmd::Bind { id, channel } => cmd_bind_repo(client, &id, &channel).await, + ReposCmd::DefaultBranch(command) => { + super::repo_default_branch::dispatch(command, client).await + } ReposCmd::Protect(command) => match command { ReposProtectCmd::List { id } => cmd_protect_list(client, &id).await, ReposProtectCmd::Set { diff --git a/crates/buzz-cli/src/lib.rs b/crates/buzz-cli/src/lib.rs index 8c5630d7c14..0e171254581 100644 --- a/crates/buzz-cli/src/lib.rs +++ b/crates/buzz-cli/src/lib.rs @@ -1237,6 +1237,38 @@ pub enum ReposCmd { /// Manage branch and tag protection rules on one of your repositories. #[command(subcommand)] Protect(ReposProtectCmd), + /// Inspect or change the relay-hosted repository's default branch. + #[command(subcommand)] + DefaultBranch(ReposDefaultBranchCmd), +} + +/// Commands for the authoritative Git default branch, not announcement metadata. +#[derive(Subcommand)] +pub enum ReposDefaultBranchCmd { + /// Read the default branch and observed manifest version. + Get { + /// Repository identifier. + #[arg(long)] + id: String, + /// Repository owner (64-char hex). Defaults to your signing identity. + #[arg(long)] + owner: Option, + }, + /// Select an existing branch without moving or deleting any refs. + Set { + /// Repository identifier. + #[arg(long)] + id: String, + /// Repository owner (64-char hex). Defaults to your signing identity. + #[arg(long)] + owner: Option, + /// Short branch name, e.g. main or release/v1 (not refs/heads/main). + #[arg(long)] + branch: String, + /// Manifest digest returned by get. Omit to read it before updating. + #[arg(long)] + expected_manifest: Option, + }, } /// Commands for inspecting and changing repository protection rules. @@ -2399,12 +2431,13 @@ mod tests { ); assert_eq!( names(&cmd, "repos"), - vec!["bind", "create", "get", "list", "protect"] + vec!["bind", "create", "default-branch", "get", "list", "protect"] ); let repos = cmd .get_subcommands() .find(|subcommand| subcommand.get_name() == "repos") .expect("repos command"); + assert_eq!(names(repos, "default-branch"), vec!["get", "set"]); let protect = repos .get_subcommands() .find(|subcommand| subcommand.get_name() == "protect") @@ -2476,7 +2509,7 @@ mod tests { ("pr", 5), ("projects", 8), ("reactions", 3), - ("repos", 5), + ("repos", 6), ("social", 7), ("upload", 1), ("users", 5), diff --git a/crates/buzz-relay/src/api/git/hydrate.rs b/crates/buzz-relay/src/api/git/hydrate.rs index 3ce809d18f7..080bbd5c1cc 100644 --- a/crates/buzz-relay/src/api/git/hydrate.rs +++ b/crates/buzz-relay/src/api/git/hydrate.rs @@ -243,7 +243,7 @@ pub async fn hydrate_for_write( /// /// `Ok(None)` if the pointer is absent (caller decides 404 vs first-push /// per call site). `Err(_)` on any below-pointer failure. -async fn load_pointer( +pub(super) async fn load_pointer( store: &GitStore, ctx: &TenantContext, owner: &str, diff --git a/crates/buzz-relay/src/api/git/mod.rs b/crates/buzz-relay/src/api/git/mod.rs index dd69d7dc36e..1db75ce39c5 100644 --- a/crates/buzz-relay/src/api/git/mod.rs +++ b/crates/buzz-relay/src/api/git/mod.rs @@ -30,6 +30,7 @@ pub mod manifest; pub mod manifest_event; pub mod pack_cache; pub mod policy; +mod settings; pub mod store; pub mod transport; diff --git a/crates/buzz-relay/src/api/git/settings.rs b/crates/buzz-relay/src/api/git/settings.rs new file mode 100644 index 00000000000..2e1f99811f6 --- /dev/null +++ b/crates/buzz-relay/src/api/git/settings.rs @@ -0,0 +1,418 @@ +//! Default-branch management of the authoritative Git manifest. +//! +//! This is a Git control-plane operation, not a replaceable announcement: +//! the pointer CAS is the commit point, shared with receive-pack. A separate +//! strict NIP-98 request prevents reusable Smart HTTP credentials authorizing +//! metadata changes (URL, method, payload and replay are all checked). + +use std::sync::Arc; + +use axum::{ + body::Bytes, + extract::{DefaultBodyLimit, Path, State}, + http::{header, HeaderMap, StatusCode}, + response::{IntoResponse, Response}, + routing::get, + Json, Router, +}; +use buzz_core::TenantContext; +use serde::Deserialize; +use serde_json::{json, Value}; + +use super::{ + binding::{resolve_repo_binding, RepoBinding}, + hydrate::load_pointer, + manifest::{is_safe_refname, pointer_key, Manifest}, + manifest_event::{build_ref_state_event, RefStateInputs}, + store::{CasOutcome, ETag, GitStore, Precond}, + transport::{authorize_git_read, deny_banned_git_principal, validate_repo_id}, +}; +use crate::{ + api::{api_error, bridge, relay_members}, + state::AppState, +}; + +fn error(status: StatusCode, message: &str) -> Response { + api_error(status, message).into_response() +} + +fn backend(error: impl std::fmt::Display) -> Response { + tracing::error!(%error, "git settings backend failure"); + self::error( + StatusCode::INTERNAL_SERVER_ERROR, + "git settings backend unavailable; read the default branch before retrying", + ) +} + +fn conflict() -> Response { + error( + StatusCode::CONFLICT, + "repository changed concurrently; read the latest manifest and retry", + ) +} + +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +struct SetDefaultBranch { + branch: String, + expected_manifest: String, +} + +/// A loaded snapshot cannot be rebound to another pointer or refreshed at CAS. +struct DefaultBranchSnapshot { + pointer: String, + etag: ETag, + digest: String, + manifest: Manifest, +} + +impl DefaultBranchSnapshot { + async fn load( + store: &GitStore, + tenant: &TenantContext, + owner: &str, + repo: &str, + ) -> Result { + let (etag, digest, manifest) = load_pointer(store, tenant, owner, repo) + .await + .map_err(backend)? + .ok_or_else(|| { + error( + StatusCode::NOT_FOUND, + "repository has no published Git state; push a branch first", + ) + })?; + Ok(Self { + pointer: pointer_key(tenant.community(), owner, repo), + etag, + digest, + manifest, + }) + } + + fn response(&self) -> Value { + json!({"branch": self.manifest.head.strip_prefix("refs/heads/"), "head": self.manifest.head, "manifest": self.digest}) + } + + async fn set( + mut self, + store: &GitStore, + request: SetDefaultBranch, + ) -> Result<(Self, bool), Response> { + if self.digest != request.expected_manifest { + return Err(conflict()); + } + let head = format!("refs/heads/{}", request.branch); + if request.branch.is_empty() + || request.branch.len() > 1024 + || request.branch.starts_with('-') + || !is_safe_refname(&head) + || request.branch.ends_with('.') + || request + .branch + .split('/') + .any(|part| part.starts_with('.') || part.ends_with(".lock")) + { + return Err(error( + StatusCode::BAD_REQUEST, + "invalid branch name; use a short branch name such as main or release/v1", + )); + } + if !self.manifest.refs.contains_key(&head) { + return Err(error( + StatusCode::BAD_REQUEST, + "default branch must name an existing published branch", + )); + } + let changed = self.manifest.head != head; + if changed { + self.manifest.head = head; + self.manifest.parent = Some(self.digest.clone()); + self.manifest.validate().map_err(backend)?; + let bytes = self.manifest.canonical_bytes().map_err(backend)?; + let key = store.put_manifest(&bytes).await.map_err(backend)?; + self.digest = key + .strip_prefix("manifests/") + .ok_or_else(|| backend("invalid manifest key"))? + .to_string(); + } + // Even a no-op checks the observed ETag: concurrent deletion/push must + // not be reported as a successful setting of a now-missing branch. + match store + .put_pointer( + &self.pointer, + self.digest.as_bytes(), + Precond::IfMatch(self.etag.clone()), + ) + .await + .map_err(backend)? + { + CasOutcome::Won(etag) => self.etag = etag, + CasOutcome::LostRace => return Err(conflict()), + } + Ok((self, changed)) + } +} + +struct SettingsAuth { + tenant: TenantContext, + caller: nostr::PublicKey, + delegated_owner: Option, +} + +async fn authenticate( + state: &Arc, + headers: &HeaderMap, + path: &str, + body: Option<&[u8]>, +) -> Result { + let host = headers + .get(header::HOST) + .and_then(|h| h.to_str().ok()) + .unwrap_or(""); + let tenant = crate::tenant::bind_community(&state.db, host) + .await + .map_err(|_| error(StatusCode::NOT_FOUND, "repository not found"))?; + let url = bridge::nip98_expected_url(&state.config.relay_url, &tenant, path); + let auth = bridge::verify_bridge_auth_with_options( + headers, + if body.is_some() { "POST" } else { "GET" }, + &url, + body, + true, + body.is_some(), + ) + .map_err(IntoResponse::into_response)?; + bridge::enforce_http_admission(state, &tenant, &auth.pubkey) + .await + .map_err(IntoResponse::into_response)?; + bridge::check_nip98_replay(state, &tenant, auth.event_id_bytes) + .await + .map_err(IntoResponse::into_response)?; + let tag = relay_members::extract_auth_tag_header(headers); + relay_members::enforce_relay_membership( + state, + tenant.community(), + auth.pubkey.as_bytes(), + tag, + auth.signed_created_at, + ) + .await + .map_err(IntoResponse::into_response)?; + deny_banned_git_principal( + &state.db, + tenant.community(), + &auth.pubkey, + tag, + auth.signed_created_at, + ) + .await?; + // Admission ignores kind= restrictions by design (NIP-AA). Repository + // management must not turn a message-only credential into write authority. + // This HTTP operation has no event kind: only kind-unrestricted credentials + // may inherit management authority. Temporal clauses are still enforced. + let delegated_owner = tag + .filter(|tag| { + serde_json::from_str::>(tag) + .ok() + .and_then(|parts| parts.get(2).cloned()) + .is_some_and(|conditions| { + !conditions + .split('&') + .any(|clause| clause.starts_with("kind=")) + }) + }) + .and_then(|tag| { + relay_members::extract_nip_oa_owner( + auth.pubkey.as_bytes(), + Some(tag), + auth.signed_created_at, + ) + }); + Ok(SettingsAuth { + tenant, + caller: auth.pubkey, + delegated_owner, + }) +} + +async fn authorize_management( + state: &AppState, + auth: &SettingsAuth, + repo: &nostr::Event, +) -> Result<(), Response> { + let RepoBinding::Bound(channel) = resolve_repo_binding(repo) else { + return Err(error(StatusCode::NOT_FOUND, "repository not found")); + }; + let community = auth.tenant.community(); + let bound = state + .db + .get_channel(community, channel) + .await + .map_err(backend)?; + if bound.archived_at.is_some() { + return Err(error( + StatusCode::FORBIDDEN, + "channel is archived (read-only)", + )); + } + let named_manager = |key: &nostr::PublicKey| { + *key == repo.pubkey + || repo.tags.iter().any(|tag| { + let values = tag.as_slice(); + values.first().is_some_and(|name| name == "maintainers") + && values + .iter() + .skip(1) + .any(|value| nostr::PublicKey::parse(value).ok().as_ref() == Some(key)) + }) + }; + // Direct authority is independent of an optional owner credential. + if named_manager(&auth.caller) + || state + .db + .is_agent_owner(community, repo.pubkey.as_bytes(), auth.caller.as_bytes()) + .await + .map_err(backend)? + { + return Ok(()); + } + if let Some(principal) = &auth.delegated_owner { + let role = state + .db + .get_member_role(community, channel, principal.as_bytes()) + .await + .map_err(backend)?; + if role.is_some_and(|r| r.parse::().is_ok()) + && (named_manager(principal) + || state + .db + .is_agent_owner(community, repo.pubkey.as_bytes(), principal.as_bytes()) + .await + .map_err(backend)?) + { + return Ok(()); + } + } + Err(error( + StatusCode::FORBIDDEN, + "only the repository owner or a named maintainer may change its default branch", + )) +} + +async fn get_default_branch( + State(state): State>, + Path((owner, repo)): Path<(String, String)>, + headers: HeaderMap, +) -> Result, Response> { + let path = format!("/git/{owner}/{repo}/default-branch"); + let repo_name = validate_repo_id(&owner, &repo)?; + let auth = authenticate(&state, &headers, &path, None).await?; + authorize_git_read( + &state.db, + auth.tenant.community(), + &auth.caller, + &owner, + repo_name, + ) + .await?; + let snapshot = + DefaultBranchSnapshot::load(&state.git_store, &auth.tenant, &owner, repo_name).await?; + Ok(Json(snapshot.response())) +} + +async fn set_default_branch( + State(state): State>, + Path((owner, repo)): Path<(String, String)>, + headers: HeaderMap, + body: Bytes, +) -> Result, Response> { + let path = format!("/git/{owner}/{repo}/default-branch"); + let repo_name = validate_repo_id(&owner, &repo)?; + let auth = authenticate(&state, &headers, &path, Some(&body)).await?; + let announcement = authorize_git_read( + &state.db, + auth.tenant.community(), + &auth.caller, + &owner, + repo_name, + ) + .await?; + authorize_management(&state, &auth, &announcement).await?; + let request: SetDefaultBranch = serde_json::from_slice(&body).map_err(|_| { + error( + StatusCode::BAD_REQUEST, + "expected branch and expected_manifest strings", + ) + })?; + let snapshot = + DefaultBranchSnapshot::load(&state.git_store, &auth.tenant, &owner, repo_name).await?; + let serving_write = buzz_deletion::acquire_serving_write( + &state.db, + auth.tenant.community(), + "git_default_branch", + ) + .await + .map_err(|_| { + error( + StatusCode::SERVICE_UNAVAILABLE, + "community writes are fenced", + ) + })?; + serving_write.verify().await.map_err(backend)?; + let (snapshot, changed) = serving_write + .protect(snapshot.set(&state.git_store, request)) + .await + .map_err(backend)??; + let publication = async { + if changed { + let actor = auth.caller.to_hex(); + let event = build_ref_state_event( + &RefStateInputs { + repo_id: repo_name, + head: &snapshot.manifest.head, + refs: &snapshot.manifest.refs, + actor_pubkey_hex: &actor, + }, + &state.relay_keypair, + ) + .map_err(backend)?; + let (stored, inserted) = state + .db + .insert_event_with_serving_write_guard(serving_write.lease(), &event, None) + .await + .map_err(backend)?; + if inserted { + crate::handlers::event::fan_out_event_to_local_subscribers( + &state, + auth.tenant.community(), + &stored, + ) + .await; + } + } + Ok::<(), Response>(()) + } + .await; + serving_write.finish().await.map_err(backend)?; + // Publication failure is not mistaken for a rolled-back manifest. + if publication.is_err() { + return Err(error(StatusCode::INTERNAL_SERVER_ERROR, "default branch committed but notification failed; read the current default branch before retrying")); + } + let mut response = snapshot.response(); + response["changed"] = json!(changed); + Ok(Json(response)) +} + +pub(super) fn router() -> Router> { + Router::new() + .route( + "/git/{owner}/{repo}/default-branch", + get(get_default_branch).post(set_default_branch), + ) + .layer(DefaultBodyLimit::max(4096)) +} + +#[cfg(test)] +#[path = "settings_tests.rs"] +mod tests; diff --git a/crates/buzz-relay/src/api/git/settings_tests.rs b/crates/buzz-relay/src/api/git/settings_tests.rs new file mode 100644 index 00000000000..55c04dddcf7 --- /dev/null +++ b/crates/buzz-relay/src/api/git/settings_tests.rs @@ -0,0 +1,880 @@ +//! Live route/store/clone regressions. Require explicit isolated service URLs; +//! never fall back to a developer's Desktop database. + +mod external_infra { + use super::super::*; + use axum::{ + body::{to_bytes, Body}, + http::Request, + }; + use base64::Engine; + use buzz_core::channel::MemberRole; + use nostr::{EventBuilder, Keys, Kind, Tag}; + use sha2::{Digest, Sha256}; + use tower::ServiceExt; + + struct Fixture { + state: Arc, + pool: sqlx::PgPool, + tenant: TenantContext, + owner: Keys, + member: Keys, + maintainer: Keys, + channel: uuid::Uuid, + repo: String, + scratch: tempfile::TempDir, + } + + impl Fixture { + async fn new() -> Self { + let database_url = std::env::var("BUZZ_TEST_DATABASE_URL") + .expect("explicit isolated BUZZ_TEST_DATABASE_URL"); + let redis_url = std::env::var("BUZZ_TEST_REDIS_URL") + .expect("explicit isolated BUZZ_TEST_REDIS_URL"); + let endpoint = std::env::var("BUZZ_TEST_S3_ENDPOINT") + .expect("explicit isolated BUZZ_TEST_S3_ENDPOINT"); + let scratch = tempfile::tempdir().unwrap(); + let mut config = crate::config::Config::from_env().unwrap(); + config.database_url = database_url; + config.redis_url = redis_url; + config.relay_url = "ws://127.0.0.1".into(); + config.require_relay_membership = false; + config.git_repo_path = scratch.path().to_path_buf(); + config.git_pack_cache_path = scratch.path().join("cache"); + config.media.s3_endpoint = endpoint; + config.media.s3_bucket = + std::env::var("BUZZ_TEST_S3_BUCKET").unwrap_or_else(|_| "buzz-git".into()); + config.media.s3_access_key = "buzz_dev".into(); + config.media.s3_secret_key = "buzz_dev_secret".into(); + let pool = sqlx::PgPool::connect(&config.database_url).await.unwrap(); + let db = buzz_db::Db::from_pool(pool.clone()); + // CI provisions schema/schema.sql with pgschema before this suite. + // Only migration-backed local fixtures own the migration lifecycle. + if std::env::var("BUZZ_TEST_SCHEMA_MODE").as_deref() != Ok("desired") { + db.migrate().await.unwrap(); + } + let redis_pool = deadpool_redis::Config::from_url(&config.redis_url) + .create_pool(Some(deadpool_redis::Runtime::Tokio1)) + .unwrap(); + let pubsub = Arc::new( + buzz_pubsub::PubSubManager::new(&config.redis_url, redis_pool.clone()) + .await + .unwrap(), + ); + let audit = buzz_audit::AuditService::new(pool.clone()); + let auth = buzz_auth::AuthService::new(config.auth.clone()); + let search = buzz_search::SearchService::new(pool.clone()); + let workflow = Arc::new(buzz_workflow::WorkflowEngine::new( + db.clone(), + buzz_workflow::WorkflowConfig::default(), + )); + let media = buzz_media::MediaStorage::new(&config.media).unwrap(); + let (state, _) = AppState::new( + config, + db, + redis_pool, + audit, + pubsub, + auth, + search, + workflow, + Keys::generate(), + media, + ); + let state = Arc::new(state); + let host = format!("settings-{}.example", uuid::Uuid::new_v4().simple()); + let community = state + .db + .ensure_configured_community(&host) + .await + .unwrap() + .id; + let tenant = TenantContext::resolved(community, &host); + let owner = Keys::generate(); + let member = Keys::generate(); + let maintainer = Keys::generate(); + let channel = uuid::Uuid::new_v4(); + state + .db + .ensure_user(community, owner.public_key().as_bytes()) + .await + .unwrap(); + state + .db + .create_channel_with_id( + community, + channel, + &format!("settings-{channel}"), + buzz_db::channel::ChannelType::Stream, + buzz_db::channel::ChannelVisibility::Open, + None, + owner.public_key().as_bytes(), + None, + ) + .await + .unwrap(); + for (key, role) in [ + (&member, MemberRole::Admin), + (&maintainer, MemberRole::Member), + (&owner, MemberRole::Owner), + ] { + state + .db + .ensure_user(community, key.public_key().as_bytes()) + .await + .unwrap(); + state + .db + .add_member( + community, + channel, + key.public_key().as_bytes(), + role, + Some(owner.public_key().as_bytes()), + ) + .await + .unwrap(); + } + let repo = format!("repo-{}", uuid::Uuid::new_v4().simple()); + let announcement = EventBuilder::new(Kind::Custom(30617), "") + .tags([ + Tag::parse(["d", &repo]).unwrap(), + Tag::parse(["buzz-channel", &channel.to_string()]).unwrap(), + Tag::parse(["maintainers", &maintainer.public_key().to_hex()]).unwrap(), + ]) + .sign_with_keys(&owner) + .unwrap(); + state + .db + .insert_event(community, &announcement, None) + .await + .unwrap(); + let f = Self { + state, + pool, + tenant, + owner, + member, + maintainer, + channel, + repo, + scratch, + }; + f.seed_git().await; + f + } + + fn path(&self) -> String { + format!( + "/git/{}/{}/default-branch", + self.owner.public_key().to_hex(), + self.repo + ) + } + + async fn snapshot(&self) -> DefaultBranchSnapshot { + DefaultBranchSnapshot::load( + &self.state.git_store, + &self.tenant, + &self.owner.public_key().to_hex(), + &self.repo, + ) + .await + .unwrap() + } + + async fn seed_git(&self) { + let source = self.scratch.path().join("source"); + std::fs::create_dir(&source).unwrap(); + git(&source, &["init", "--initial-branch=legacy"]).await; + git(&source, &["config", "user.name", "Git settings test"]).await; + git( + &source, + &["config", "user.email", "git-settings@example.invalid"], + ) + .await; + git(&source, &["commit", "--allow-empty", "-m", "legacy"]).await; + git(&source, &["branch", "main"]).await; + git(&source, &["checkout", "main"]).await; + std::fs::write(source.join("main.txt"), b"selected branch\n").unwrap(); + git(&source, &["add", "main.txt"]).await; + git(&source, &["commit", "-m", "main"]).await; + git(&source, &["checkout", "legacy"]).await; + super::super::super::cas_publish::cas_publish( + &self.state.git_store, + &self.tenant, + &source, + &self.owner.public_key().to_hex(), + &self.repo, + &super::super::super::cas_publish::ParentState::fresh(), + limits(0), + ) + .await + .unwrap(); + } + + async fn call( + &self, + key: &Keys, + body: Option, + tag: Option<&str>, + ) -> (StatusCode, Value) { + let body = body.map(|value| value.to_string()); + let method = if body.is_some() { "POST" } else { "GET" }; + let path = self.path(); + let token = token( + key, + method, + &format!("http://{}{path}", self.tenant.host()), + body.as_deref(), + ); + let mut request = Request::builder() + .method(method) + .uri(&path) + .header("host", self.tenant.host()) + .header("authorization", token); + if let Some(tag) = tag { + request = request.header("x-auth-tag", tag); + } + let request = request.body(Body::from(body.unwrap_or_default())).unwrap(); + response( + super::super::super::transport::git_router(self.state.clone()) + .oneshot(request) + .await + .unwrap(), + ) + .await + } + + async fn set(&self, key: &Keys, branch: &str, tag: Option<&str>) -> (StatusCode, Value) { + let digest = self.snapshot().await.digest; + self.call( + key, + Some(json!({"branch": branch, "expected_manifest": digest})), + tag, + ) + .await + } + + async fn add(&self, key: &Keys) { + self.state + .db + .ensure_user(self.tenant.community(), key.public_key().as_bytes()) + .await + .unwrap(); + self.state + .db + .add_member( + self.tenant.community(), + self.channel, + key.public_key().as_bytes(), + MemberRole::Bot, + Some(self.owner.public_key().as_bytes()), + ) + .await + .unwrap(); + } + } + + fn limits(parent_hydrated_bytes: u64) -> super::super::super::cas_publish::PublishLimits { + super::super::super::cas_publish::PublishLimits { + parent_hydrated_bytes, + max_pack_bytes: 1024 * 1024, + max_repo_bytes: 2 * 1024 * 1024, + } + } + + async fn git(path: &std::path::Path, args: &[&str]) -> String { + let mut command = tokio::process::Command::new("git"); + command.current_dir(path).args(args); + super::super::super::transport::harden_git_env(&mut command); + let result = command.output().await.unwrap(); + assert!( + result.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&result.stderr) + ); + String::from_utf8(result.stdout).unwrap() + } + + fn token(keys: &Keys, method: &str, url: &str, body: Option<&str>) -> String { + token_with_payload( + keys, + method, + url, + body.map(|body| Tag::parse(["payload", &hex::encode(Sha256::digest(body))]).unwrap()), + ) + } + + fn token_with_payload(keys: &Keys, method: &str, url: &str, payload: Option) -> String { + let mut tags = vec![ + Tag::parse(["u", url]).unwrap(), + Tag::parse(["method", method]).unwrap(), + Tag::parse(["nonce", &uuid::Uuid::new_v4().to_string()]).unwrap(), + ]; + if let Some(payload) = payload { + tags.push(payload); + } + let event = EventBuilder::new(Kind::Custom(27235), "") + .tags(tags) + .sign_with_keys(keys) + .unwrap(); + format!( + "Nostr {}", + base64::engine::general_purpose::STANDARD.encode(serde_json::to_vec(&event).unwrap()) + ) + } + + async fn response(response: Response) -> (StatusCode, Value) { + let status = response.status(); + let bytes = to_bytes(response.into_body(), 1024 * 1024).await.unwrap(); + ( + status, + serde_json::from_slice(&bytes) + .unwrap_or_else(|_| json!({"error": String::from_utf8_lossy(&bytes)})), + ) + } + + #[tokio::test] + #[ignore = "requires isolated Postgres, Redis and MinIO"] + async fn default_branch_route_permissions_and_protocol() { + let f = Fixture::new().await; + let before = f.snapshot().await; + assert_eq!( + f.call(&f.member, None, None).await.1["head"], + "refs/heads/legacy" + ); + assert_eq!( + f.set(&f.member, "main", None).await.0, + StatusCode::FORBIDDEN, + "push-capable channel admin is not a repo manager" + ); + assert_eq!( + f.set(&Keys::generate(), "main", None).await.0, + StatusCode::NOT_FOUND + ); + for branch in [ + "", + "absent", + "../main", + "refs/heads/main", + "main.lock", + "bad\nref", + "main/", + "-main", + ".main", + ] { + assert_eq!( + f.set(&f.owner, branch, None).await.0, + StatusCode::BAD_REQUEST, + "{branch:?}" + ); + } + assert_eq!( + f.snapshot().await.digest, + before.digest, + "denials do not write" + ); + let result = f.set(&f.maintainer, "main", None).await; + assert_eq!(result.0, StatusCode::OK, "{result:?}"); + assert_eq!(result.1["changed"], true); + let after = f.snapshot().await; + assert_eq!(after.manifest.head, "refs/heads/main"); + assert_eq!(after.manifest.refs, before.manifest.refs); + assert_eq!(after.manifest.packs, before.manifest.packs); + assert_eq!(after.manifest.parent.as_ref(), Some(&before.digest)); + let result = f.set(&f.owner, "main", None).await; + assert_eq!(result.0, StatusCode::OK); + assert_eq!(result.1["changed"], false); + assert_eq!(f.snapshot().await.digest, after.digest); + assert_eq!( + f.call( + &f.owner, + Some(json!({"branch":"legacy", "expected_manifest": before.digest})), + None + ) + .await + .0, + StatusCode::CONFLICT + ); + let notification_query = buzz_db::EventQuery { + kinds: Some(vec![30618]), + d_tag: Some(f.repo.clone()), + global_only: true, + ..buzz_db::EventQuery::for_community(f.tenant.community()) + }; + let events = f.state.db.query_events(¬ification_query).await.unwrap(); + let event_ids: Vec<_> = events.iter().map(|e| e.event.id).collect(); + assert!( + events.iter().any(|e| e + .event + .tags + .iter() + .any(|t| t.as_slice() == ["HEAD", "ref: refs/heads/main"])), + "committed default notification: {events:?}" + ); + + // Strict credentials: each mutated property must be rejected at the real route. + let body = json!({"branch":"legacy", "expected_manifest": after.digest}).to_string(); + let path = f.path(); + let url = format!("http://{}{path}", f.tenant.host()); + let requests = [ + token_with_payload( + &f.owner, + "POST", + &url, + Some(Tag::parse(["payload"]).unwrap()), + ), + token_with_payload( + &f.owner, + "POST", + &url, + Some(Tag::parse(["payload", ""]).unwrap()), + ), + token(&f.owner, "GET", &url, Some(&body)), + token(&f.owner, "POST", &url, None), + token(&f.owner, "POST", &url, Some("{}")), + token( + &f.owner, + "POST", + &url.replace(f.tenant.host(), "other.example"), + Some(&body), + ), + token( + &f.owner, + "GET", + url.trim_end_matches("/default-branch"), + None, + ), + ]; + for token in requests { + let request = Request::builder() + .method("POST") + .uri(&path) + .header("host", f.tenant.host()) + .header("authorization", token) + .body(Body::from(body.clone())) + .unwrap(); + let status = super::super::super::transport::git_router(f.state.clone()) + .oneshot(request) + .await + .unwrap() + .status(); + assert_eq!(status, StatusCode::UNAUTHORIZED); + assert_eq!( + f.snapshot().await.digest, + after.digest, + "auth denial changed pointer" + ); + let denied_events = f.state.db.query_events(¬ification_query).await.unwrap(); + assert_eq!( + denied_events.iter().map(|e| e.event.id).collect::>(), + event_ids, + "auth denial published kind:30618" + ); + } + let reusable = token(&f.owner, "GET", &url, None); + for expected in [StatusCode::OK, StatusCode::UNAUTHORIZED] { + let request = Request::builder() + .uri(&path) + .header("host", f.tenant.host()) + .header("authorization", &reusable) + .body(Body::empty()) + .unwrap(); + assert_eq!( + super::super::super::transport::git_router(f.state.clone()) + .oneshot(request) + .await + .unwrap() + .status(), + expected + ); + } + let other_host = format!("other-{}.example", uuid::Uuid::new_v4()); + f.state + .db + .ensure_configured_community(&other_host) + .await + .unwrap(); + let token = token(&f.owner, "GET", &format!("http://{other_host}{path}"), None); + let request = Request::builder() + .uri(&path) + .header("host", &other_host) + .header("authorization", token) + .body(Body::empty()) + .unwrap(); + assert_eq!( + super::super::super::transport::git_router(f.state.clone()) + .oneshot(request) + .await + .unwrap() + .status(), + StatusCode::NOT_FOUND + ); + } + + #[tokio::test] + #[ignore = "requires isolated Postgres, Redis and MinIO"] + async fn default_branch_delegation_and_revocation() { + let f = Fixture::new().await; + let agent = Keys::generate(); + f.add(&agent).await; + let tag = buzz_sdk::nip_oa::compute_auth_tag(&f.owner, &agent.public_key(), "").unwrap(); + assert_eq!(f.set(&agent, "main", None).await.0, StatusCode::FORBIDDEN); + let limited = + buzz_sdk::nip_oa::compute_auth_tag(&f.owner, &agent.public_key(), "kind=1").unwrap(); + assert_eq!( + f.set(&agent, "main", Some(&limited)).await.0, + StatusCode::FORBIDDEN + ); + let expired = + buzz_sdk::nip_oa::compute_auth_tag(&f.owner, &agent.public_key(), "created_at<1") + .unwrap(); + assert_eq!( + f.set(&agent, "main", Some(&expired)).await.0, + StatusCode::FORBIDDEN + ); + assert_eq!(f.set(&agent, "main", Some(&tag)).await.0, StatusCode::OK); + // Optional credential does not take direct authority away. + let absent_owner = Keys::generate(); + let own_tag = + buzz_sdk::nip_oa::compute_auth_tag(&absent_owner, &f.owner.public_key(), "").unwrap(); + assert_eq!( + f.set(&f.owner, "legacy", Some(&own_tag)).await.0, + StatusCode::OK + ); + // A human can administer a repository announced by their managed agent. + f.state + .db + .set_agent_owner( + f.tenant.community(), + f.owner.public_key().as_bytes(), + f.member.public_key().as_bytes(), + ) + .await + .unwrap(); + assert_eq!(f.set(&f.member, "main", None).await.0, StatusCode::OK); + f.state + .db + .add_member( + f.tenant.community(), + f.channel, + f.maintainer.public_key().as_bytes(), + MemberRole::Owner, + Some(f.owner.public_key().as_bytes()), + ) + .await + .unwrap(); + f.state + .db + .remove_member( + f.tenant.community(), + f.channel, + f.owner.public_key().as_bytes(), + f.owner.public_key().as_bytes(), + ) + .await + .unwrap(); + assert_eq!( + f.set(&agent, "legacy", Some(&tag)).await.0, + StatusCode::FORBIDDEN + ); + assert_eq!( + f.set(&f.owner, "legacy", None).await.0, + StatusCode::NOT_FOUND + ); + // Durable ban cascades even when the signer has independent maintainer rights. + let ban_tag = + buzz_sdk::nip_oa::compute_auth_tag(&f.member, &f.maintainer.public_key(), "").unwrap(); + f.state + .db + .ban_community_member( + f.tenant.community(), + f.member.public_key().as_bytes(), + f.member.public_key().as_bytes(), + Some("test"), + None, + ) + .await + .unwrap(); + assert_eq!( + f.set(&f.maintainer, "legacy", Some(&ban_tag)).await.0, + StatusCode::FORBIDDEN + ); + sqlx::query("UPDATE channels SET archived_at = NOW() WHERE community_id = $1 AND id = $2") + .bind(f.tenant.community().as_uuid()) + .bind(f.channel) + .execute(&f.pool) + .await + .unwrap(); + assert_eq!( + f.set(&f.maintainer, "legacy", None).await.0, + StatusCode::FORBIDDEN + ); + } + + #[tokio::test] + #[ignore = "requires isolated Postgres, Redis and MinIO"] + async fn default_branch_push_races_and_fresh_clone() { + let f = Fixture::new().await; + let a = f.snapshot().await; + let b = f.snapshot().await; + let old_digest = a.digest.clone(); + let (_, changed) = a + .set( + &f.state.git_store, + SetDefaultBranch { + branch: "main".into(), + expected_manifest: old_digest.clone(), + }, + ) + .await + .unwrap(); + assert!(changed); + let loser = b + .set( + &f.state.git_store, + SetDefaultBranch { + branch: "legacy".into(), + expected_manifest: old_digest, + }, + ) + .await + .err() + .unwrap(); + assert_eq!( + loser.status(), + StatusCode::CONFLICT, + "stale no-op must CAS too" + ); + // Snapshot a push before the metadata update; it must not restore stale HEAD. + let options = || super::super::super::hydrate::HydrationOptions { + pack_cache: &f.state.git_pack_cache, + scratch_dir: f.scratch.path(), + max_pack_bytes: 1024 * 1024, + max_repo_bytes: 2 * 1024 * 1024, + }; + let (push, parent) = super::super::super::hydrate::hydrate_for_write( + &f.state.git_store, + &f.tenant, + &f.owner.public_key().to_hex(), + &f.repo, + options(), + ) + .await + .unwrap(); + assert_eq!(f.set(&f.owner, "legacy", None).await.0, StatusCode::OK); + let result = super::super::super::cas_publish::cas_publish( + &f.state.git_store, + &f.tenant, + push.path(), + &f.owner.public_key().to_hex(), + &f.repo, + &parent, + limits(push.hydrated_bytes()), + ) + .await; + assert!(matches!( + result, + Err(super::super::super::cas_publish::CasError::Conflict { .. }) + )); + // Other direction: a push deletes the candidate after settings loaded it. + let stale = f.snapshot().await; + let digest = stale.digest.clone(); + let (push, parent) = super::super::super::hydrate::hydrate_for_write( + &f.state.git_store, + &f.tenant, + &f.owner.public_key().to_hex(), + &f.repo, + options(), + ) + .await + .unwrap(); + git(push.path(), &["update-ref", "-d", "refs/heads/main"]).await; + super::super::super::cas_publish::cas_publish( + &f.state.git_store, + &f.tenant, + push.path(), + &f.owner.public_key().to_hex(), + &f.repo, + &parent, + limits(push.hydrated_bytes()), + ) + .await + .unwrap(); + assert_eq!( + stale + .set( + &f.state.git_store, + SetDefaultBranch { + branch: "main".into(), + expected_manifest: digest + } + ) + .await + .err() + .unwrap() + .status(), + StatusCode::CONFLICT + ); + assert!(!f + .snapshot() + .await + .manifest + .refs + .contains_key("refs/heads/main")); + // Restore main and add release/v1, then select the non-main branch so + // Git's initial-branch default cannot mask a lost hydrated HEAD. + let (push, parent) = super::super::super::hydrate::hydrate_for_write( + &f.state.git_store, + &f.tenant, + &f.owner.public_key().to_hex(), + &f.repo, + options(), + ) + .await + .unwrap(); + let main = git(&f.scratch.path().join("source"), &["rev-parse", "main"]).await; + git(push.path(), &["update-ref", "refs/heads/main", main.trim()]).await; + git( + push.path(), + &["update-ref", "refs/heads/release/v1", main.trim()], + ) + .await; + super::super::super::cas_publish::cas_publish( + &f.state.git_store, + &f.tenant, + push.path(), + &f.owner.public_key().to_hex(), + &f.repo, + &parent, + limits(push.hydrated_bytes()), + ) + .await + .unwrap(); + assert_eq!(f.set(&f.owner, "release/v1", None).await.0, StatusCode::OK); + let (push, parent) = super::super::super::hydrate::hydrate_for_write( + &f.state.git_store, + &f.tenant, + &f.owner.public_key().to_hex(), + &f.repo, + options(), + ) + .await + .unwrap(); + git( + push.path(), + &["update-ref", "refs/heads/later", main.trim()], + ) + .await; + super::super::super::cas_publish::cas_publish( + &f.state.git_store, + &f.tenant, + push.path(), + &f.owner.public_key().to_hex(), + &f.repo, + &parent, + limits(push.hydrated_bytes()), + ) + .await + .unwrap(); + assert_eq!(f.snapshot().await.manifest.head, "refs/heads/release/v1"); + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + // Add a reachable host alias for the same tenant solely in this fixture. + sqlx::query("UPDATE communities SET host = $1 WHERE id = $2") + .bind(addr.to_string()) + .bind(f.tenant.community().as_uuid()) + .execute(&f.pool) + .await + .unwrap(); + let router = super::super::super::transport::git_router(f.state.clone()); + let server = tokio::spawn(async move { axum::serve(listener, router).await.unwrap() }); + let repo_url = format!( + "http://{addr}/git/{}/{}", + f.owner.public_key().to_hex(), + f.repo + ); + let auth = format!( + "http.extraHeader=Authorization: {}", + token(&f.owner, "GET", &repo_url, None) + ); + let refs = git( + f.scratch.path(), + &["-c", &auth, "ls-remote", "--symref", &repo_url, "HEAD"], + ) + .await; + assert!(refs.contains("ref: refs/heads/release/v1\tHEAD"), "{refs}"); + git( + f.scratch.path(), + &["-c", &auth, "clone", &repo_url, "clone"], + ) + .await; + assert_eq!( + git(&f.scratch.path().join("clone"), &["symbolic-ref", "HEAD"]) + .await + .trim(), + "refs/heads/release/v1" + ); + assert_eq!( + std::fs::read(f.scratch.path().join("clone/main.txt")).unwrap(), + b"selected branch\n" + ); + server.abort(); + } + + struct UnavailableReplayGuard; + + impl buzz_auth::Nip98ReplayGuard for UnavailableReplayGuard { + fn try_mark_in_scope<'a>( + &'a self, + _scope: &'a str, + _event_id: &'a nostr::EventId, + _ttl_secs: u64, + ) -> std::pin::Pin< + Box> + Send + 'a>, + > { + Box::pin(async { + Err(buzz_auth::AuthError::Nip98Invalid( + "injected backend failure".into(), + )) + }) + } + } + + #[tokio::test] + #[ignore = "requires isolated Postgres, Redis and MinIO"] + async fn default_branch_replay_outage_and_deletion_fail_closed() { + let mut f = Fixture::new().await; + let before = f.snapshot().await.digest; + let original = f.state.clone(); + let mut state = (*original).clone(); + state.nip98_replay = Arc::new(UnavailableReplayGuard); + f.state = Arc::new(state); + for body in [ + None, + Some(json!({"branch":"main", "expected_manifest":before})), + ] { + let (status, body) = f.call(&f.owner, body, None).await; + assert_eq!(status, StatusCode::UNAUTHORIZED, "{body}"); + assert!(body.to_string().contains("replay check unavailable")); + } + assert_eq!(f.snapshot().await.digest, before); + f.state = original; + // Enter the deletion executor's transaction scope in this disposable + // fixture; the DB correctly rejects unfenced ad-hoc state changes. + let mut tx = f.pool.begin().await.unwrap(); + sqlx::query("SELECT set_config('buzz.deletion_executor_community', $1, true), set_config('buzz.deletion_fence_generation', '0', true)") + .bind(f.tenant.community().to_string()) + .execute(&mut *tx).await.unwrap(); + sqlx::query("UPDATE communities SET deletion_state = 'quiescing' WHERE id = $1") + .bind(f.tenant.community().as_uuid()) + .execute(&mut *tx) + .await + .unwrap(); + tx.commit().await.unwrap(); + assert_ne!(f.set(&f.owner, "main", None).await.0, StatusCode::OK); + assert_eq!(f.snapshot().await.digest, before); + } +} diff --git a/crates/buzz-relay/src/api/git/transport.rs b/crates/buzz-relay/src/api/git/transport.rs index 638e3c7156b..704bbf1c1d6 100644 --- a/crates/buzz-relay/src/api/git/transport.rs +++ b/crates/buzz-relay/src/api/git/transport.rs @@ -247,7 +247,7 @@ impl axum::extract::FromRequestParts> for GitAuth { /// Cascades to the proven NIP-OA owner, matching the NIP-42 gate in /// `handlers::auth`: banning a human must also revoke their agents, or the ban /// is bypassable by cloning and pushing through an agent key. -async fn deny_banned_git_principal( +pub(super) async fn deny_banned_git_principal( db: &buzz_db::Db, community: buzz_core::CommunityId, pubkey: &nostr::PublicKey, @@ -362,7 +362,7 @@ fn git_expected_url( /// repo root — but the *name* validation stays because owner/repo are /// still used as object-store key components via `manifest::pointer_key`. #[allow(clippy::result_large_err)] // Response is the natural error type for axum handlers -fn validate_repo_id<'a>(owner: &str, repo: &'a str) -> Result<&'a str, Response> { +pub(super) fn validate_repo_id<'a>(owner: &str, repo: &'a str) -> Result<&'a str, Response> { // Owner must be exactly 64 lowercase hex chars. if owner.len() != 64 || !owner @@ -483,13 +483,13 @@ fn hydrate_error_to_response(owner: &str, repo: &str, err: HydrateError) -> Resp /// — so the remediation body leaks nothing, and only the author can rebind /// (kind:30617 is keyed by `(author, d)`). A *broken* binding stays generic /// even for the author: ambiguity fails closed. -async fn authorize_git_read( +pub(super) async fn authorize_git_read( db: &buzz_db::Db, community: buzz_core::CommunityId, caller: &nostr::PublicKey, owner_hex: &str, repo_name: &str, -) -> Result<(), Response> { +) -> Result { fn denied() -> Response { (StatusCode::NOT_FOUND, "repository not found").into_response() } @@ -552,7 +552,7 @@ async fn authorize_git_read( .get_member_role(community, channel_id, &caller.to_bytes()) .await { - Ok(role) if read_role_allows(role.as_deref()) => Ok(()), + Ok(role) if read_role_allows(role.as_deref()) => Ok(repo_event.event), Ok(_) => Err(denied()), Err(e) => { error!(repo = %repo_name, error = %e, "git read gate: role lookup failed (deny)"); @@ -2118,6 +2118,7 @@ pub fn git_router(state: Arc) -> Router { .route("/git/{owner}/{repo}/info/refs", get(info_refs)) .route("/git/{owner}/{repo}/git-upload-pack", post(upload_pack)) .route("/git/{owner}/{repo}/git-receive-pack", post(receive_pack)) + .merge(super::settings::router()) .layer(RequestBodyLimitLayer::new(body_limit)) .with_state(state) } @@ -3305,7 +3306,7 @@ mod sec005_postgres_tests { /// can assert on the exact bytes a git client would see. A blind /// `.is_err()` cannot distinguish the generic 404 from the remediation /// 404 — and that distinction IS the security property. - async fn denial_parts(result: Result<(), Response>) -> (StatusCode, String) { + async fn denial_parts(result: Result) -> (StatusCode, String) { let response = result.expect_err("expected a denial"); let status = response.status(); let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) diff --git a/crates/buzz-relay/src/handlers/event.rs b/crates/buzz-relay/src/handlers/event.rs index 3830f5c55a3..5011f794638 100644 --- a/crates/buzz-relay/src/handlers/event.rs +++ b/crates/buzz-relay/src/handlers/event.rs @@ -755,8 +755,21 @@ pub async fn handle_event(event: Event, conn: Arc, state: Arc { conn.send(RelayMessage::ok(&event_id_hex, true, "")); } - Err(message) => { - reject("invalid"); + Err(e) => { + // Rejections carry the ingest taxonomy so backend failures + // count as `error` — a Redis presence-storage outage is a + // server fault, not client misbehavior — while genuine + // client-input rejections (bad signature, non-member + // sender) stay `invalid`. The ephemeral handler only emits + // fixed, sanitized message strings, so unlike the + // persistent arm below, `Internal` text is safe to forward + // verbatim. + let (message, reason) = match e { + IngestError::Rejected(message) => (message, "invalid"), + IngestError::AuthFailed(message) => (message, "auth"), + IngestError::Internal(message) => (message, "error"), + }; + reject(reason); conn.send(RelayMessage::ok(&event_id_hex, false, &message)); } } @@ -804,6 +817,12 @@ pub async fn handle_event(event: Event, conn: Arc, state: Arc, state: Arc, -) -> Result<(), String> { +) -> Result<(), IngestError> { let event_clone = event.clone(); let event_id = event.id.to_hex(); let verify_result = tokio::task::spawn_blocking(move || verify_event(&event_clone)).await; match verify_result { Ok(Ok(())) => {} - Ok(Err(e)) => return Err(format!("invalid: {e}")), - Err(_) => return Err("error: internal error".to_string()), + Ok(Err(e)) => return Err(IngestError::Rejected(format!("invalid: {e}"))), + Err(_) => return Err(IngestError::Internal("error: internal error".to_string())), } // Special handling for presence events (kind:20001). @@ -841,16 +860,45 @@ async fn handle_ephemeral_event( raw }; + // Presence mutation is the inclusion contract for the live fan-out + // below: a client that observes the fanned-out event treats a later + // snapshot as reflecting it (see `synthesize_presence` in + // `api/bridge.rs`, which reads Redis). If the mutation failed we + // published nothing — so we must also fan out nothing and reject the + // ACK, or a snapshot later "confirms" stale storage over a live event + // the sender believes was delivered. if status == "offline" { - let _ = state + if let Err(e) = state .pubsub .clear_presence(&conn.tenant, &auth_pubkey) - .await; - } else { - let _ = state - .pubsub - .set_presence(&conn.tenant, &auth_pubkey, &status) - .await; + .await + { + warn!( + conn_id = %conn_id, + event_id = %event_id, + "Presence clear failed, refusing publish and fan-out: {e}" + ); + // Internal, not Rejected: a storage outage is a server + // failure, so the dispatcher must count it under the + // `error` reason, not as client-invalid input. + return Err(IngestError::Internal( + "error: presence storage unavailable".to_string(), + )); + } + } else if let Err(e) = state + .pubsub + .set_presence(&conn.tenant, &auth_pubkey, &status) + .await + { + warn!( + conn_id = %conn_id, + event_id = %event_id, + "Presence set failed, refusing publish and fan-out: {e}" + ); + // Internal for the same reason as the clear arm above. + return Err(IngestError::Internal( + "error: presence storage unavailable".to_string(), + )); } // Presence is a channel-less ephemeral event. After updating Redis @@ -860,8 +908,12 @@ async fn handle_ephemeral_event( // Check channel membership before publishing other ephemeral events. if let Some(ch_id) = super::ingest::extract_channel_id(&event) { + // Membership refusals are client-input rejections, and the shared + // gate's message text is surfaced verbatim exactly as before this + // typed classification; no behavior change on this path. super::ingest::check_channel_membership(&conn.tenant, &state, ch_id, &pubkey_bytes, None) - .await?; + .await + .map_err(IngestError::Rejected)?; // Mark as local before Redis publish to prevent double-delivery when // the event comes back through the Redis subscriber loop. @@ -1512,6 +1564,323 @@ mod tests { assert_eq!(frame[3], "restricted: read-only connection may not publish"); } + // PostgreSQL/Redis tests are discovered by the isolated postgres-ci lane. + mod presence_storage_postgres_tests { + use super::*; + use buzz_core::{CommunityId, TenantContext}; + use nostr::Filter; + + async fn exercise(storage_available: bool, statuses: &[&str]) { + let redis_url = std::env::var("REDIS_URL").expect("test Redis URL required"); + // Pick an unused local endpoint for a real connection failure. + let dead_socket = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let dead_port = dead_socket.local_addr().unwrap().port(); + drop(dead_socket); + let dead_url = format!("redis://127.0.0.1:{dead_port}"); + let state = fanout_access::test_state_with_redis_url(if storage_available { + &redis_url + } else { + &dead_url + }) + .await; + let pool = sqlx::PgPool::connect(&state.config.database_url) + .await + .unwrap(); + let community_uuid = Uuid::new_v4(); + let host = format!("presence-storage-{}.example", community_uuid.simple()); + sqlx::query("INSERT INTO communities (id, host) VALUES ($1, $2)") + .bind(community_uuid) + .bind(&host) + .execute(&pool) + .await + .expect("seed active community"); + let tenant = TenantContext::resolved(CommunityId::from_uuid(community_uuid), host); + let keys = Keys::generate(); + let (send_tx, mut send_rx) = mpsc::channel(10); + let (ctrl_tx, _ctrl_rx) = mpsc::channel(10); + let conn = Arc::new(crate::connection::ConnectionState { + conn_id: Uuid::new_v4(), + tenant: tenant.clone(), + remote_addr: "127.0.0.1:1234".parse().unwrap(), + auth_state: RwLock::new(crate::connection::AuthState::Authenticated { + ctx: buzz_auth::AuthContext { + pubkey: keys.public_key(), + scopes: vec![], + channel_ids: None, + auth_method: buzz_auth::AuthMethod::Nip42, + agent_owner_pubkey: None, + }, + class: crate::connection::ConnectionClass::Interactive, + }), + subscriptions: Arc::new(Mutex::new(HashMap::new())), + send_tx, + ctrl_tx, + cancel: CancellationToken::new(), + backpressure_count: Arc::new(AtomicU8::new(0)), + grace_limit: 3, + }); + let watcher = Uuid::new_v4(); + let (tx, mut rx) = mpsc::channel(10); + let (ctrl, _ctrl_rx) = mpsc::channel(10); + state.conn_manager.register( + watcher, + tx, + ctrl, + None, + CancellationToken::new(), + tenant.community(), + Arc::new(AtomicU8::new(0)), + Arc::new(Mutex::new(HashMap::new())), + 3, + ); + state.sub_registry.register_scoped( + tenant.community(), + watcher, + "presence".into(), + vec![Filter::new().kind(Kind::Custom(KIND_PRESENCE_UPDATE as u16))], + None, + ); + // Online followed by offline also proves DEL removes an existing value. + for &status in statuses { + let event = EventBuilder::new(Kind::Custom(KIND_PRESENCE_UPDATE as u16), status) + .sign_with_keys(&keys) + .unwrap(); + super::super::handle_event(event.clone(), conn.clone(), state.clone()).await; + let axum::extract::ws::Message::Text(text) = send_rx.try_recv().expect("ACK") + else { + panic!("expected text ACK"); + }; + let ack: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(ack[0], "OK"); + assert_eq!(ack[1], event.id.to_hex()); + assert_eq!(ack[2], storage_available); + if storage_available { + assert_eq!(ack[3], ""); + let stored = state + .pubsub + .get_presence(&tenant, &keys.public_key()) + .await + .unwrap(); + assert_eq!(stored.as_deref(), (status != "offline").then_some(status)); + let axum::extract::ws::Message::Text(text) = + rx.try_recv().expect("live fanout") + else { + panic!("expected text event"); + }; + let frame: serde_json::Value = serde_json::from_str(&text).unwrap(); + assert_eq!(frame[0], "EVENT"); + assert_eq!(frame[2]["id"], event.id.to_hex()); + } else { + assert_eq!(ack[3], "error: presence storage unavailable"); + assert!(state + .local_event_ids + .get(&(tenant.community(), event.id.to_bytes())) + .is_none()); + } + assert!(rx.try_recv().is_err(), "no extra or rejected-event fanout"); + assert!(send_rx.try_recv().is_err(), "exactly one ACK"); + } + sqlx::query("DELETE FROM communities WHERE id = $1") + .bind(community_uuid) + .execute(&pool) + .await + .unwrap(); + } + + #[tokio::test] + #[ignore = "requires PostgreSQL and Redis"] + async fn rejects_online_when_presence_storage_fails() { + exercise(false, &["online"]).await; + } + + #[tokio::test] + #[ignore = "requires PostgreSQL and Redis"] + async fn rejects_offline_when_presence_storage_fails() { + exercise(false, &["offline"]).await; + } + + #[tokio::test] + #[ignore = "requires PostgreSQL and Redis"] + async fn accepts_stores_and_fans_out_online_and_offline() { + exercise(true, &["online", "offline"]).await; + } + + /// Production-seam regression for the rejection classification: a + /// Redis presence-storage outage must count as a server `error`, + /// never as client `invalid`, while a genuinely invalid event + /// through the same seam stays `invalid`. Both rejections still ACK + /// `false` with no fan-out and no local-event marker (the shared + /// storage guard is unchanged); only the counter routing is under + /// test here. + /// + /// The recorder guard form (not `metrics::with_local_recorder`, + /// whose closure cannot host an await) keeps the thread-local + /// recorder installed across the `.await` points of this + /// single-threaded test runtime — the same convention as the + /// buzz-db route-decision counter tests. The isolated postgres-ci + /// lane runs each test in its own nextest process, so no parallel + /// test can race this counter snapshot. + #[tokio::test] + #[ignore = "requires PostgreSQL"] + async fn counts_presence_storage_failure_as_error_not_invalid() { + // Pick an unused local endpoint for a real connection failure. + let dead_socket = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let dead_port = dead_socket.local_addr().unwrap().port(); + drop(dead_socket); + let dead_url = format!("redis://127.0.0.1:{dead_port}"); + let state = fanout_access::test_state_with_redis_url(&dead_url).await; + let pool = sqlx::PgPool::connect(&state.config.database_url) + .await + .unwrap(); + let community_uuid = Uuid::new_v4(); + let host = format!("presence-metrics-{}.example", community_uuid.simple()); + sqlx::query("INSERT INTO communities (id, host) VALUES ($1, $2)") + .bind(community_uuid) + .bind(&host) + .execute(&pool) + .await + .expect("seed active community"); + let tenant = TenantContext::resolved(CommunityId::from_uuid(community_uuid), host); + let keys = Keys::generate(); + let (send_tx, mut send_rx) = mpsc::channel(10); + let (ctrl_tx, _ctrl_rx) = mpsc::channel(10); + let conn = Arc::new(crate::connection::ConnectionState { + conn_id: Uuid::new_v4(), + tenant: tenant.clone(), + remote_addr: "127.0.0.1:1234".parse().unwrap(), + auth_state: RwLock::new(crate::connection::AuthState::Authenticated { + ctx: buzz_auth::AuthContext { + pubkey: keys.public_key(), + scopes: vec![], + channel_ids: None, + auth_method: buzz_auth::AuthMethod::Nip42, + agent_owner_pubkey: None, + }, + class: crate::connection::ConnectionClass::Interactive, + }), + subscriptions: Arc::new(Mutex::new(HashMap::new())), + send_tx, + ctrl_tx, + cancel: CancellationToken::new(), + backpressure_count: Arc::new(AtomicU8::new(0)), + grace_limit: 3, + }); + // Same watcher registration as the ACK/fan-out cases, proving + // the storage failure still reaches no subscriber while its + // rejection is counted under `error`. + let watcher = Uuid::new_v4(); + let (tx, mut rx) = mpsc::channel(10); + let (ctrl, _ctrl_rx) = mpsc::channel(10); + state.conn_manager.register( + watcher, + tx, + ctrl, + None, + CancellationToken::new(), + tenant.community(), + Arc::new(AtomicU8::new(0)), + Arc::new(Mutex::new(HashMap::new())), + 3, + ); + state.sub_registry.register_scoped( + tenant.community(), + watcher, + "presence".into(), + vec![Filter::new().kind(Kind::Custom(KIND_PRESENCE_UPDATE as u16))], + None, + ); + + let valid = EventBuilder::new(Kind::Custom(KIND_PRESENCE_UPDATE as u16), "online") + .sign_with_keys(&keys) + .unwrap(); + // Invalid control through the same production seam: identical + // event, signature no longer verifies. The id is unchanged (it + // does not cover the signature), so ACKs are told apart by + // order and message text below. + use nostr::JsonUtil as _; + let mut json: serde_json::Value = + serde_json::from_str(&valid.as_json()).expect("parse event json"); + json["sig"] = serde_json::Value::String("0".repeat(128)); + let tampered = nostr::Event::from_json(json.to_string()).expect("parse tampered"); + + let recorder = metrics_util::debugging::DebuggingRecorder::new(); + let snapshotter = recorder.snapshotter(); + let acks: Vec = { + let _guard = metrics::set_default_local_recorder(&recorder); + super::super::handle_event(valid.clone(), conn.clone(), state.clone()).await; + super::super::handle_event(tampered, conn.clone(), state.clone()).await; + (0..2) + .map(|_| { + let axum::extract::ws::Message::Text(text) = + send_rx.try_recv().expect("ACK") + else { + panic!("expected text ACK"); + }; + serde_json::from_str(&text).unwrap() + }) + .collect() + }; + + // Storage failure: rejected ACK, no fan-out frame, no marker. + assert_eq!(acks[0][0], "OK"); + assert_eq!(acks[0][1], valid.id.to_hex()); + assert_eq!(acks[0][2], false); + assert_eq!(acks[0][3], "error: presence storage unavailable"); + // Invalid control through the same dispatcher arm. + assert_eq!(acks[1][0], "OK"); + assert_eq!(acks[1][1], valid.id.to_hex()); + assert_eq!(acks[1][2], false); + assert!( + acks[1][3].as_str().unwrap().starts_with("invalid:"), + "tampered control must stay an invalid rejection, got: {}", + acks[1][3] + ); + assert!(rx.try_recv().is_err(), "no fan-out for either rejection"); + assert!(send_rx.try_recv().is_err(), "exactly two ACKs"); + assert!(state + .local_event_ids + .get(&(tenant.community(), valid.id.to_bytes())) + .is_none()); + + let mut rejections: Vec<(String, String, u64)> = snapshotter + .snapshot() + .into_vec() + .into_iter() + .filter(|(key, ..)| key.key().name() == "buzz_events_rejected_total") + .map(|(key, _, _, value)| { + let metrics_util::debugging::DebugValue::Counter(count) = value else { + panic!("buzz_events_rejected_total must be a counter"); + }; + let labels: Vec<_> = key.key().labels().collect(); + let label = |name: &str| { + labels + .iter() + .find(|l| l.key() == name) + .map(|l| l.value().to_owned()) + .unwrap_or_default() + }; + (label("transport"), label("reason"), count) + }) + .collect(); + rejections.sort(); + assert_eq!( + rejections, + vec![ + ("ws".to_owned(), "error".to_owned(), 1), + ("ws".to_owned(), "invalid".to_owned(), 1), + ], + "storage outage must count as reason=\"error\" and the tampered \ + control as reason=\"invalid\"; got {rejections:?}" + ); + + sqlx::query("DELETE FROM communities WHERE id = $1") + .bind(community_uuid) + .execute(&pool) + .await + .unwrap(); + } + } + mod pubsub_fanout { use std::collections::HashMap; use std::sync::atomic::AtomicU8; diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 7f26cb3e9e6..b42b3e3393e 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -142,6 +142,8 @@ export default defineConfig({ "**/drafts-screenshots.spec.ts", "**/drafts-all-fix-screenshots.spec.ts", "**/inbox-refactor-screenshots.spec.ts", + "**/inbox-title-overlap.spec.ts", + "**/message-author-overlap.spec.ts", "**/buzz-theme-screenshots.spec.ts", "**/appearance-previews.spec.ts", "**/channel-sort.spec.ts", diff --git a/desktop/scripts/check-pubkey-truncation.mjs b/desktop/scripts/check-pubkey-truncation.mjs index d65db135454..67ce2a90910 100644 --- a/desktop/scripts/check-pubkey-truncation.mjs +++ b/desktop/scripts/check-pubkey-truncation.mjs @@ -6,8 +6,10 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url)); const projectRoot = path.resolve(__dirname, ".."); // Truncated pubkey prefixes are forgeable (vanity grinding), so all display -// truncation goes through the canonical `truncatePubkey` / `` — this -// guard keeps ad-hoc `pubkey.slice(0, N)` forms from fragmenting again. +// truncation goes through the canonical `truncateNpub` (identity keys — +// compact npub), `truncatePubkey` (generic hex identifiers such as event and +// blob IDs), or `` — this guard keeps ad-hoc `pubkey.slice(0, N)` +// forms from fragmenting again. const rules = [ { root: "src", diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index ccca7c4abfa..99dcc98145a 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -1167,7 +1167,7 @@ mod tests { // Simulate the minimum supported adapter version. std::fs::write( &bin, - "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.7'\nexit 0\n", + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.10.0'\nexit 0\n", ) .expect("write script"); std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)) @@ -1189,10 +1189,10 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let bin = dir.path().join("codex-acp"); - // A 1.x adapter below MIN_CODEX_ACP_VERSION must still be reinstalled. + // The observed adapter bundles Codex 0.148.x and must be upgraded. std::fs::write( &bin, - "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.5'\nexit 0\n", + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.6.2'\nexit 0\n", ) .expect("write script"); std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)) diff --git a/desktop/src-tauri/src/commands/channels/fetch.rs b/desktop/src-tauri/src/commands/channels/fetch.rs index 36c24a35b7d..993d40807e2 100644 --- a/desktop/src-tauri/src/commands/channels/fetch.rs +++ b/desktop/src-tauri/src/commands/channels/fetch.rs @@ -186,9 +186,9 @@ pub(super) enum DirectoryScope { /// - Phase 1 (parallel): member-chain (kind:39002→kind:39000), the non-member /// metadata source (pending-owned ids when member-only, else the all-open /// kind:39000 scan), and the hidden-DM snapshot (kind:30622). -/// - Phase 2 (parallel): member counts (kind:39002 batch) and last-message -/// timestamps (bounded per-channel human-visible activity batches), fanned -/// out over the merged set. Member-count failures degrade to zero; timestamp +/// - Phase 2 (parallel): missing member counts (kind:39002 batch) and last-message +/// timestamps (bounded per-channel human-visible activity batches). Reuse the +/// member-chain rosters; missing-count failures degrade to zero. Timestamp /// failures abort so cached recency is never replaced by a false /// authoritative empty result. pub(super) async fn fetch_channels( @@ -263,7 +263,7 @@ pub(super) async fn fetch_channels( Vec::new() }; - Ok::<_, String>(meta_events) + Ok::<_, String>((meta_events, collect_members_by_channel(&member_events))) }, // Step 3: non-member channel metadata (kind:39000). // - IncludeOpenDirectory: scan ALL open channels so the discovery @@ -321,7 +321,7 @@ pub(super) async fn fetch_channels( #[cfg(debug_assertions)] let t_phase1 = _profile_start.elapsed(); - let meta_events = member_chain_result?; + let (meta_events, mut membership) = member_chain_result?; let open_meta_events = open_meta_result?; // hidden_dms is already a resolved HashSet (tolerant path above) @@ -371,8 +371,8 @@ pub(super) async fn fetch_channels( } } - // Phase 2 — concurrent: member counts (step 4) and last-message timestamps - // (step 5). Member-count failures degrade to zero. Timestamp failures + // Phase 2 — concurrent: missing member counts (step 4) and last-message + // timestamps (step 5). Missing-count failures degrade to zero. Timestamp failures // abort this refresh so the frontend keeps its previous Recent ordering. let all_channel_ids: Vec = channels.iter().map(|c| c.id.clone()).collect(); if !all_channel_ids.is_empty() { @@ -381,16 +381,27 @@ pub(super) async fn fetch_channels( .map(|id| last_message_filter(id)) .collect(); - // Bind both filter arrays before the join so their lifetimes cover - // both branches of the concurrent pair. + // Step 1 already returned complete rosters, not just the matching p-tag. + // Only directory-only or still-pending channels need another read. Keep + // reuse local to this fetch so the next refresh sees membership changes. + let missing_member_ids: Vec<&String> = all_channel_ids + .iter() + .filter(|id| !membership.contains_key(*id)) + .collect(); let member_count_filters = [serde_json::json!({ "kinds": [39002], - "#d": &all_channel_ids, - "limit": all_channel_ids.len(), + "#d": &missing_member_ids, + "limit": missing_member_ids.len(), })]; let (members_result, message_result) = tokio::join!( - // Step 4: batch-fetch kind:39002 for member counts. - query_relay(state, &member_count_filters), + // Step 4: do not send an empty #d filter (an unscoped roster query). + async { + if missing_member_ids.is_empty() { + Ok(Vec::new()) + } else { + query_relay(state, &member_count_filters).await + } + }, // Step 5: preserve one indexed filter per channel while keeping // every relay request within its aggregate explicit-channel cap. query_last_messages(state, &last_msg_filters), @@ -400,7 +411,9 @@ pub(super) async fn fetch_channels( // empty result and clear every cached timestamp in the frontend. let messages = message_result?; - let membership = collect_members_by_channel(&members_result.unwrap_or_default()); + membership.extend(collect_members_by_channel( + &members_result.unwrap_or_default(), + )); for channel in &mut channels { if let Some(info) = membership.get(&channel.id) { channel.member_count = info.count; @@ -488,3 +501,7 @@ pub(super) fn collect_members_by_channel( } map } + +#[cfg(test)] +#[path = "fetch_tests.rs"] +mod tests; diff --git a/desktop/src-tauri/src/commands/channels/fetch_tests.rs b/desktop/src-tauri/src/commands/channels/fetch_tests.rs new file mode 100644 index 00000000000..439dfbd6c56 --- /dev/null +++ b/desktop/src-tauri/src/commands/channels/fetch_tests.rs @@ -0,0 +1,378 @@ +//! Exercise the production channel-list fetch over the native HTTP bridge. +use super::*; +use axum::{extract::State, http::StatusCode, routing::post, Json, Router}; +use nostr::{Event, EventBuilder, Keys, Kind, Tag, Timestamp}; +use serde_json::{json, Value}; +use std::sync::{Arc, Mutex}; + +#[derive(Default)] +struct Fixture { + events: Vec, + requests: Vec>, + fail_discovery: bool, + fail_fallback: bool, + fail_messages: bool, +} + +struct Relay { + data: Arc>, + url: String, + task: tokio::task::JoinHandle<()>, +} + +impl Drop for Relay { + fn drop(&mut self) { + self.task.abort(); + } +} + +async fn query( + State(data): State>>, + Json(filters): Json>, +) -> (StatusCode, Json) { + let mut data = data.lock().unwrap(); + data.requests.push(filters.clone()); + let mut result = Vec::new(); + for filter in &filters { + let kind = filter["kinds"][0].as_u64().unwrap(); + if (kind == 39002 && filter.get("#p").is_some() && data.fail_discovery) + || (kind == 39002 && filter.get("#d").is_some() && data.fail_fallback) + || (kind == 9 && data.fail_messages) + { + return ( + StatusCode::SERVICE_UNAVAILABLE, + Json(json!({"error": "fixture unavailable"})), + ); + } + let mut page: Vec<_> = data + .events + .iter() + .filter(|event| { + if !filter["kinds"] + .as_array() + .unwrap() + .contains(&json!(event.kind.as_u16())) + { + return false; + } + for name in ["p", "d", "h"] { + if let Some(values) = filter.get(format!("#{name}")).and_then(Value::as_array) { + if !event.tags.iter().any(|tag| { + let s = tag.as_slice(); + s.len() >= 2 && s[0] == name && values.contains(&json!(s[1])) + }) { + return false; + } + } + } + if let Some(until) = filter["until"].as_u64() { + let ts = event.created_at.as_secs(); + if ts > until + || (ts == until + && filter["before_id"] + .as_str() + .is_some_and(|id| event.id.to_hex().as_str() <= id)) + { + return false; + } + } + true + }) + .cloned() + .collect(); + page.sort_by(|a, b| { + b.created_at + .cmp(&a.created_at) + .then_with(|| a.id.cmp(&b.id)) + }); + page.truncate(filter["limit"].as_u64().unwrap() as usize); + result.extend(page); + } + (StatusCode::OK, Json(json!(result))) +} + +impl Relay { + async fn new(events: Vec) -> Self { + let data = Arc::new(Mutex::new(Fixture { + events, + ..Default::default() + })); + let router = Router::new() + .route("/query", post(query)) + .with_state(data.clone()); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = format!("http://{}", listener.local_addr().unwrap()); + let task = tokio::spawn(async move { axum::serve(listener, router).await.unwrap() }); + Self { data, url, task } + } + + fn state(&self, keys: &Keys) -> AppState { + let state = crate::app_state::build_app_state(); + *state.keys.lock().unwrap() = keys.clone(); + *state.relay_url_override.lock().unwrap() = Some(self.url.clone()); + state + } + + fn roster_fallbacks(&self) -> Vec { + self.data + .lock() + .unwrap() + .requests + .iter() + .flatten() + .filter(|filter| filter["kinds"] == json!([39002]) && filter.get("#d").is_some()) + .cloned() + .collect() + } +} + +fn event(keys: &Keys, kind: u16, tags: Vec>) -> Event { + EventBuilder::new(Kind::from_u16(kind), "") + .allow_self_tagging() + .tags(tags.into_iter().map(|tag| Tag::parse(tag).unwrap())) + .custom_created_at(Timestamp::from(1_700_000_000)) + .sign_with_keys(keys) + .unwrap() +} + +fn metadata(keys: &Keys, id: &str) -> Event { + event(keys, 39000, vec![vec!["d", id], vec!["name", id]]) +} + +fn roster(keys: &Keys, id: &str, members: &[&str]) -> Event { + let mut tags = vec![vec!["d", id]]; + tags.extend(members.iter().map(|pk| vec!["p", *pk, "", "member"])); + event(keys, 39002, tags) +} + +async fn fetch(state: &AppState, scope: DirectoryScope) -> Result, String> { + tokio::time::timeout( + std::time::Duration::from_secs(10), + fetch_channels(state, scope), + ) + .await + .expect("bounded channel fetch") +} + +#[tokio::test] +async fn member_rosters_are_reused_including_every_discovery_page() { + let _serial = crate::relay_admission::TEST_SERIAL.lock().await; + let keys = Keys::generate(); + let me = keys.public_key().to_hex(); + let other = Keys::generate().public_key().to_hex(); + for count in [265, 501] { + let mut events = Vec::new(); + for index in 0..count { + let id = format!("channel-{index:04}"); + events.push(metadata(&keys, &id)); + // Repeated p-tag must not inflate the count; non-self members must survive. + events.push(roster(&keys, &id, &[&me, &other, &other])); + } + let relay = Relay::new(events).await; + let state = relay.state(&keys); + let started = std::time::Instant::now(); + let channels = fetch(&state, DirectoryScope::MemberOnly).await.unwrap(); + assert_eq!(channels.len(), count); + assert!(channels.iter().all(|c| c.is_member + && c.member_count == 2 + && c.member_pubkeys == vec![me.clone(), other.clone()])); + assert!( + relay.roster_fallbacks().is_empty(), + "covered rosters must not be fetched twice" + ); + let data = relay.data.lock().unwrap(); + let discovery: Vec<_> = data + .requests + .iter() + .flatten() + .filter(|f| f["kinds"] == json!([39002])) + .collect(); + assert_eq!(discovery.len(), count / DIRECTORY_PAGE_SIZE + 1); + if count > DIRECTORY_PAGE_SIZE { + assert_eq!(discovery[1]["until"], json!(1_700_000_000)); + assert!(discovery[1]["before_id"].is_string()); + } + // Membership pages + member metadata + hidden DMs + bounded activity batches. + let expected_reads = count / DIRECTORY_PAGE_SIZE + 1 + 2 + count.div_ceil(128); + assert_eq!(data.requests.len(), expected_reads); + eprintln!( + "roster-reuse fixture channels={count} reads={} elapsed={:?}", + data.requests.len(), + started.elapsed() + ); + } +} + +#[tokio::test] +async fn directory_fetches_only_uncovered_rosters_and_keeps_hidden_dm_behavior() { + let _serial = crate::relay_admission::TEST_SERIAL.lock().await; + let keys = Keys::generate(); + let me = keys.public_key().to_hex(); + let other = Keys::generate().public_key().to_hex(); + let relay = Relay::new(vec![ + metadata(&keys, "joined"), + roster(&keys, "joined", &[&me, &other]), + metadata(&keys, "open"), + roster(&keys, "open", &[&other]), + metadata(&keys, "empty"), + roster(&keys, "empty", &[]), + event(&keys, 39000, vec![vec!["d", "hidden-dm"], vec!["t", "dm"]]), + roster(&keys, "hidden-dm", &[&me, &other]), + event( + &keys, + buzz_core_pkg::kind::KIND_DM_VISIBILITY.try_into().unwrap(), + vec![vec!["p", &me], vec!["h", "hidden-dm"]], + ), + event(&keys, 9, vec![vec!["h", "joined"]]), + ]) + .await; + let channels = fetch(&relay.state(&keys), DirectoryScope::IncludeOpenDirectory) + .await + .unwrap(); + assert_eq!(channels.len(), 3); + let joined = channels.iter().find(|c| c.id == "joined").unwrap(); + assert!(joined.is_member); + assert_eq!(joined.member_pubkeys, vec![me, other.clone()]); + assert!(joined.last_message_at.is_some()); + let open = channels.iter().find(|c| c.id == "open").unwrap(); + assert!(!open.is_member); + assert_eq!(open.member_count, 1); + assert_eq!(open.member_pubkeys, vec![other]); + let empty = channels.iter().find(|c| c.id == "empty").unwrap(); + assert!(!empty.is_member); + assert_eq!(empty.member_count, 0); + assert!(empty.member_pubkeys.is_empty()); + let fallbacks = relay.roster_fallbacks(); + assert_eq!(fallbacks.len(), 1); + let mut ids = fallbacks[0]["#d"].as_array().unwrap().clone(); + ids.sort_by_key(Value::to_string); + assert_eq!(ids, vec![json!("empty"), json!("open")]); + assert_eq!(fallbacks[0]["limit"], json!(2)); +} + +#[tokio::test] +async fn pending_owner_fallback_failure_preserves_covered_rosters() { + let _serial = crate::relay_admission::TEST_SERIAL.lock().await; + let keys = Keys::generate(); + let me = keys.public_key().to_hex(); + let other = Keys::generate().public_key().to_hex(); + let relay = Relay::new(vec![ + metadata(&keys, "joined"), + roster(&keys, "joined", &[&me, &other]), + metadata(&keys, "pending"), + roster(&keys, "pending", &[&other]), + metadata(&keys, "unpropagated"), + metadata(&keys, "someone-elses-pending"), + ]) + .await; + let state = relay.state(&keys); + state.mark_pending_owned_channel(&me, "joined"); + state.mark_pending_owned_channel(&me, "pending"); + state.mark_pending_owned_channel(&me, "unpropagated"); + state.mark_pending_owned_channel(&other, "someone-elses-pending"); + for fail in [false, true] { + relay.data.lock().unwrap().fail_fallback = fail; + let channels = fetch(&state, DirectoryScope::MemberOnly).await.unwrap(); + assert_eq!(channels.len(), 3); + let unpropagated = channels.iter().find(|c| c.id == "unpropagated").unwrap(); + assert!(unpropagated.is_member); + assert_eq!(unpropagated.member_count, 0); + assert!(unpropagated.member_pubkeys.is_empty()); + let pending = channels.iter().find(|c| c.id == "pending").unwrap(); + assert!(pending.is_member); + assert_eq!(pending.member_count, if fail { 0 } else { 1 }); + assert_eq!( + channels + .iter() + .find(|c| c.id == "joined") + .unwrap() + .member_count, + 2 + ); + assert!(!state.is_pending_owned_channel(&me, "joined")); + assert!(state.is_pending_owned_channel(&me, "pending")); + } + for fallback in relay.roster_fallbacks() { + let mut ids = fallback["#d"].as_array().unwrap().clone(); + ids.sort_by_key(Value::to_string); + assert_eq!(ids, vec![json!("pending"), json!("unpropagated")]); + assert_eq!(fallback["limit"], json!(2)); + } +} + +#[tokio::test] +async fn each_fetch_observes_roster_changes_and_the_current_identity_and_relay() { + let _serial = crate::relay_admission::TEST_SERIAL.lock().await; + let keys = Keys::generate(); + let me = keys.public_key().to_hex(); + let next_keys = Keys::generate(); + let next = next_keys.public_key().to_hex(); + let relay = Relay::new(vec![ + metadata(&keys, "same-id"), + roster(&keys, "same-id", &[&me]), + ]) + .await; + let state = relay.state(&keys); + assert_eq!( + fetch(&state, DirectoryScope::MemberOnly).await.unwrap()[0].member_pubkeys, + vec![me.clone()] + ); + relay.data.lock().unwrap().events[1] = roster(&keys, "same-id", &[&me, &next]); + assert_eq!( + fetch(&state, DirectoryScope::MemberOnly).await.unwrap()[0].member_count, + 2 + ); + relay.data.lock().unwrap().events[1] = roster(&keys, "same-id", &[&next]); + assert!(fetch(&state, DirectoryScope::MemberOnly) + .await + .unwrap() + .is_empty()); + *state.keys.lock().unwrap() = next_keys.clone(); + assert_eq!( + fetch(&state, DirectoryScope::MemberOnly).await.unwrap()[0].member_pubkeys, + vec![next.clone()] + ); + let second = Relay::new(vec![ + metadata(&keys, "same-id"), + roster(&keys, "same-id", &[&next, &me]), + ]) + .await; + *state.relay_url_override.lock().unwrap() = Some(second.url.clone()); + assert_eq!( + fetch(&state, DirectoryScope::MemberOnly).await.unwrap()[0].member_pubkeys, + vec![next, me] + ); + assert!(relay.roster_fallbacks().is_empty()); + assert!(second.roster_fallbacks().is_empty()); +} + +#[tokio::test] +async fn empty_membership_does_not_issue_metadata_roster_or_activity_queries() { + let _serial = crate::relay_admission::TEST_SERIAL.lock().await; + let keys = Keys::generate(); + let relay = Relay::new(vec![]).await; + assert!(fetch(&relay.state(&keys), DirectoryScope::MemberOnly) + .await + .unwrap() + .is_empty()); + assert_eq!(relay.data.lock().unwrap().requests.len(), 2); + assert!(relay.roster_fallbacks().is_empty()); +} + +#[tokio::test] +async fn discovery_and_activity_failures_still_abort_the_refresh() { + let _serial = crate::relay_admission::TEST_SERIAL.lock().await; + let keys = Keys::generate(); + let me = keys.public_key().to_hex(); + let relay = Relay::new(vec![ + metadata(&keys, "joined"), + roster(&keys, "joined", &[&me]), + ]) + .await; + let state = relay.state(&keys); + relay.data.lock().unwrap().fail_discovery = true; + assert!(fetch(&state, DirectoryScope::MemberOnly).await.is_err()); + relay.data.lock().unwrap().fail_discovery = false; + relay.data.lock().unwrap().fail_messages = true; + assert!(fetch(&state, DirectoryScope::MemberOnly).await.is_err()); +} diff --git a/desktop/src-tauri/src/commands/identity.rs b/desktop/src-tauri/src/commands/identity.rs index ec2357b85e7..8852fcb7e01 100644 --- a/desktop/src-tauri/src/commands/identity.rs +++ b/desktop/src-tauri/src/commands/identity.rs @@ -11,19 +11,46 @@ use crate::{ relay::{self, relay_api_base_url_with_override, relay_ws_url_with_override}, }; -/// Encode `pubkey` as npub bech32 and truncate it for display: first 10 chars -/// + "…" + last 4 chars. Returns the full bech32 when it is 16 chars or fewer. +/// Encode `pubkey` as npub bech32 and truncate it for display: first 8 +/// chars, an ellipsis, then the last 4 chars, mirroring the frontend +/// `truncateNpub` compact policy (`first8…last4` of the whole npub string). +/// Returns the full bech32 when it is 12 chars or fewer, mirroring +/// `truncatePubkey`'s short-string threshold. fn truncated_display_name(pubkey: &PublicKey) -> Result { let bech32 = pubkey .to_bech32() .map_err(|error| format!("bech32 encode failed: {error}"))?; - Ok(if bech32.len() > 16 { - format!("{}…{}", &bech32[..10], &bech32[bech32.len() - 4..]) + Ok(if bech32.len() > 12 { + format!("{}…{}", &bech32[..8], &bech32[bech32.len() - 4..]) } else { bech32 }) } +#[cfg(test)] +mod truncated_display_name_tests { + use super::truncated_display_name; + use nostr::{PublicKey, ToBech32}; + + #[test] + fn compacts_to_first_8_and_last_4_of_the_npub() { + // Vector shared with the frontend `truncateNpub` tests; the expected + // form is derived from the encoded npub, not hardcoded, so the test + // asserts the compaction policy rather than one key's string. + let hex = "ea9b4d7a7a78a3e3729e5568b14d764d4962be0e1f20f749bcf8d9dbbf9a9328"; + let pubkey = PublicKey::from_hex(hex).unwrap(); + let npub = pubkey.to_bech32().unwrap(); + let expected = format!("{}…{}", &npub[..8], &npub[npub.len() - 4..]); + assert_eq!(truncated_display_name(&pubkey).unwrap(), expected); + // 13 characters, matching the frontend compact form (the ellipsis is + // one char but three UTF-8 bytes, so count chars, not bytes). + assert_eq!(expected.chars().count(), 13); + assert!(expected.starts_with("npub1")); + // The compact form must not carry the raw hex key. + assert!(!expected.contains(hex)); + } +} + #[tauri::command] pub fn get_identity(state: State<'_, AppState>) -> Result { let keys = state.keys.lock().map_err(|error| error.to_string())?; diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index b781f8d9e68..a746cc80598 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -10,6 +10,8 @@ use reqwest::{ use serde::Serialize; use url::Url; +#[path = "link_preview_cancellation.rs"] +mod cancellation; #[path = "link_preview_image_retry.rs"] mod image_retry; #[path = "link_preview_rate_limit.rs"] @@ -17,15 +19,19 @@ mod rate_limit; #[path = "link_preview_youtube.rs"] mod youtube; -use rate_limit::{image_host_cooldown_remaining, retry_after_duration, set_image_host_cooldown}; +use rate_limit::{ + image_host_cooldown_remaining, image_host_gate, retry_after_duration, set_image_host_cooldown, +}; const MAX_PREVIEW_FETCH_BYTES: usize = 256 * 1024; const MAX_IMAGE_FETCH_BYTES: usize = 2 * 1024 * 1024; const MAX_IMAGE_DIMENSION: u32 = 4096; const MAX_IMAGE_PIXELS: u64 = 16_000_000; const MAX_SANITIZED_DIMENSION: u32 = 1200; -const PREVIEW_FETCH_TIMEOUT: Duration = Duration::from_secs(4); -const PREVIEW_TOTAL_TIMEOUT: Duration = Duration::from_secs(10); +const TRANSPORT_CONNECT_TIMEOUT: Duration = Duration::from_secs(15); +const TRANSPORT_IDLE_TIMEOUT: Duration = Duration::from_secs(30); +const DNS_RESOLUTION_TIMEOUT: Duration = Duration::from_secs(15); +const MAX_INLINE_IMAGE_COOLDOWN: Duration = Duration::from_secs(30); const MAX_REDIRECTS: usize = 3; const MAX_METADATA_CHARS: usize = 180; const MAX_METADATA_DESCRIPTION_CHARS: usize = 280; @@ -55,27 +61,46 @@ pub struct LinkPreviewMetadata { #[tauri::command] pub async fn fetch_link_preview_metadata( href: String, + request_id: Option, ) -> Result, String> { - tokio::time::timeout( - PREVIEW_TOTAL_TIMEOUT, - fetch_link_preview_metadata_inner(href), - ) - .await - .map_err(|_| "link preview request timed out".to_string())? + let cancellation = cancellation::begin(request_id.as_deref()); + let result = match cancellation { + Some(cancellation) => { + tokio::select! { + result = fetch_link_preview_metadata_for_url(href) => result, + () = cancellation.cancelled() => Err("link preview request cancelled".to_string()), + } + } + None => fetch_link_preview_metadata_for_url(href).await, + }; + cancellation::finish(request_id.as_deref()); + result } -async fn fetch_link_preview_metadata_inner( +/// Cancel renderer-owned metadata work, including an in-flight response body. +#[tauri::command] +pub fn cancel_link_preview_metadata(request_id: String) { + cancellation::cancel(&request_id); +} + +/// Release a renderer's cancellation record after its invocation settles. +#[tauri::command] +pub fn release_link_preview_metadata(request_id: String) { + cancellation::finish(Some(&request_id)); +} + +async fn fetch_link_preview_metadata_for_url( href: String, ) -> Result, String> { let mut url = Url::parse(href.trim()).map_err(|error| format!("invalid URL: {error}"))?; - validate_public_https_url(&url).await?; + validate_metadata_url(&url).await?; if youtube::is_video_url(&url) { return youtube::fetch_oembed_metadata(&url).await; } for redirect_count in 0..=MAX_REDIRECTS { - let response = send_pinned_request(&url, "text/html,application/xhtml+xml;q=0.9").await?; + let response = send_metadata_request(&url, "text/html,application/xhtml+xml;q=0.9").await?; if response.status().is_redirection() { if redirect_count == MAX_REDIRECTS { @@ -90,7 +115,7 @@ async fn fetch_link_preview_metadata_inner( url = url .join(location) .map_err(|error| format!("invalid link preview redirect: {error}"))?; - validate_public_https_url(&url).await?; + validate_metadata_url(&url).await?; continue; } @@ -107,28 +132,15 @@ async fn fetch_link_preview_metadata_inner( let (image_result, favicon_result) = tokio::join!( async { match image_url { - Some(image_url) => Some( - tokio::time::timeout( - PREVIEW_FETCH_TIMEOUT, - fetch_sanitized_image_with_retry(image_url, false), - ) - .await - .unwrap_or(Err(ImageFetchError::Transient { - retry_after: None, - retry_inline: false, - })), - ), + Some(image_url) => { + Some(fetch_sanitized_image_with_retry(image_url, false).await) + } None => None, } }, async { match favicon_url { - Some(favicon_url) => tokio::time::timeout( - PREVIEW_FETCH_TIMEOUT, - fetch_sanitized_image(favicon_url, true), - ) - .await - .ok(), + Some(favicon_url) => Some(fetch_sanitized_image(favicon_url, true).await), None => None, } } @@ -166,6 +178,15 @@ fn apply_image_result( } } +async fn validate_metadata_url(url: &Url) -> Result<(), String> { + #[cfg(test)] + if METADATA_TEST_SERVER.try_with(|_| ()).is_ok() { + return Ok(()); + } + + validate_public_https_url(url).await +} + async fn validate_public_https_url(url: &Url) -> Result<(), String> { if url.scheme() != "https" || url.username() != "" || url.password().is_some() { return Err("link previews require an HTTPS URL without credentials".to_string()); @@ -182,11 +203,15 @@ async fn validate_public_https_url(url: &Url) -> Result<(), String> { async fn resolve_public_addresses(host: &str) -> Result, String> { let host = host.to_string(); - let addresses = tokio::net::lookup_host((host.as_str(), 443)) - .await - .map_err(|error| format!("link preview DNS resolution failed: {error}"))? - .map(|address| address.ip()) - .collect::>(); + let addresses = tokio::time::timeout( + DNS_RESOLUTION_TIMEOUT, + tokio::net::lookup_host((host.as_str(), 443)), + ) + .await + .map_err(|_| "link preview DNS resolution timed out".to_string())? + .map_err(|error| format!("link preview DNS resolution failed: {error}"))? + .map(|address| address.ip()) + .collect::>(); if addresses.is_empty() { return Err("link preview DNS resolution returned no addresses".to_string()); @@ -198,6 +223,25 @@ async fn resolve_public_addresses(host: &str) -> Result, String> { Ok(addresses) } +#[cfg(test)] +tokio::task_local! { + static METADATA_TEST_SERVER: std::net::SocketAddr; +} + +async fn send_metadata_request(url: &Url, accept: &str) -> Result { + #[cfg(test)] + if let Ok(address) = METADATA_TEST_SERVER.try_with(|address| *address) { + return reqwest::Client::new() + .get(format!("http://{address}{}", url.path())) + .header(ACCEPT, accept) + .send() + .await + .map_err(|error| format!("link preview test request failed: {error}")); + } + + send_pinned_request(url, accept).await +} + async fn send_pinned_request(url: &Url, accept: &str) -> Result { let host = url .host_str() @@ -211,6 +255,8 @@ async fn send_pinned_request(url: &Url, accept: &str) -> Result Result bool { + if *waited_for_cooldown { + return false; + } + *waited_for_cooldown = true; + tokio::time::sleep(retry_after).await; + true +} + +fn retryable_image_cooldown( + url: &Url, + retry_after: Option, + waited_for_cooldown: &mut bool, +) -> Option { + let retry_after = retry_after?; + if *waited_for_cooldown { + return None; + } + set_image_host_cooldown(url, retry_after); + if retry_after > MAX_INLINE_IMAGE_COOLDOWN { + return None; + } + *waited_for_cooldown = true; + Some(retry_after) +} + async fn fetch_sanitized_image( - mut url: Url, + url: Url, preserve_transparency: bool, ) -> Result<(String, String), ImageFetchError> { - validate_public_https_url(&url) + fetch_sanitized_image_using( + url, + preserve_transparency, + |url| async move { validate_public_https_url(&url).await }, + |url, accept| async move { send_pinned_request(&url, accept).await }, + ) + .await +} + +async fn fetch_sanitized_image_using( + mut url: Url, + preserve_transparency: bool, + mut validate_url: V, + mut send_request: F, +) -> Result<(String, String), ImageFetchError> +where + V: FnMut(Url) -> VFut, + VFut: std::future::Future>, + F: FnMut(Url, &'static str) -> Fut, + Fut: std::future::Future>, +{ + validate_url(url.clone()) .await .map_err(|_| ImageFetchError::Rejected)?; - for redirect_count in 0..=MAX_REDIRECTS { + let mut redirect_count = 0; + let mut waited_for_cooldown = false; + while redirect_count <= MAX_REDIRECTS { if let Some(retry_after) = image_host_cooldown_remaining(&url) { - return Err(ImageFetchError::Transient { - retry_after: Some(retry_after), - retry_inline: false, - }); + if retry_after > MAX_INLINE_IMAGE_COOLDOWN + || !wait_for_image_host_cooldown(&mut waited_for_cooldown, retry_after).await + { + return Err(ImageFetchError::Transient { + retry_after: Some(retry_after), + retry_inline: false, + }); + } + continue; } - let response = send_pinned_request(&url, "image/jpeg,image/png,image/webp") + + let host_gate = image_host_gate(&url); + let host_guard = host_gate.lock().await; + if image_host_cooldown_remaining(&url).is_some() { + continue; + } + let response = send_request(url.clone(), "image/jpeg,image/png,image/webp") .await .map_err(|_| ImageFetchError::Transient { retry_after: None, - retry_inline: true, + retry_inline: !waited_for_cooldown, })?; if response.status().is_redirection() { if redirect_count == MAX_REDIRECTS { @@ -370,9 +479,10 @@ async fn fetch_sanitized_image( .and_then(|value| value.to_str().ok()) .ok_or(ImageFetchError::Rejected)?; url = url.join(location).map_err(|_| ImageFetchError::Rejected)?; - validate_public_https_url(&url) + validate_url(url.clone()) .await .map_err(|_| ImageFetchError::Rejected)?; + redirect_count += 1; continue; } if !response.status().is_success() { @@ -383,12 +493,18 @@ async fn fetch_sanitized_image( || status.is_server_error() { let retry_after = retry_after_duration(&response); - if let Some(retry_after) = retry_after { - set_image_host_cooldown(&url, retry_after); + if let Some(retry_after) = + retryable_image_cooldown(&url, retry_after, &mut waited_for_cooldown) + { + drop(host_guard); + tokio::time::sleep(retry_after).await; + continue; } return Err(ImageFetchError::Transient { retry_after, - retry_inline: status != reqwest::StatusCode::TOO_MANY_REQUESTS, + retry_inline: retry_after.is_none() + && status != reqwest::StatusCode::TOO_MANY_REQUESTS + && !waited_for_cooldown, }); } return Err(ImageFetchError::Rejected); @@ -677,315 +793,5 @@ fn decode_html_entities(value: &str) -> String { } #[cfg(test)] -mod tests { - use super::rate_limit::MAX_IMAGE_RETRY_AFTER; - use super::{ - apply_image_result, declares_animation, extract_favicon_url, extract_image_url, - extract_link_preview_metadata, is_html_response, read_bytes_prefix, retry_after_duration, - sanitize_image, ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, - MAX_METADATA_DESCRIPTION_CHARS, - }; - use axum::{body::Body, http::Response, routing::get, Router}; - use base64::Engine as _; - use bytes::Bytes; - use futures_util::stream; - use image::{DynamicImage, ImageFormat, Rgb, RgbImage, Rgba, RgbaImage}; - use std::{convert::Infallible, io::Cursor}; - use url::Url; - - async fn test_response(router: Router, path: &str) -> reqwest::Response { - let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); - let address = listener.local_addr().unwrap(); - tokio::spawn(async move { - axum::serve(listener, router).await.unwrap(); - }); - reqwest::get(format!("http://{address}{path}")) - .await - .unwrap() - } - - #[test] - fn metadata_prefers_open_graph_and_reads_site_name() { - let html = r#" - - - Fallback"#; - assert_eq!( - extract_link_preview_metadata(html), - Some(LinkPreviewMetadata { - title: "Rich previews & cards".to_string(), - site_name: Some("Buzz".to_string()), - description: Some("Safe & useful previews".to_string()), - image_data_url: None, - image_domain: None, - image_fetch_state: LinkPreviewImageFetchState::None, - image_retry_after_ms: None, - favicon_data_url: None, - }) - ); - } - - #[test] - fn image_results_preserve_absence_and_classify_recovery() { - let mut metadata = extract_link_preview_metadata("Preview result").unwrap(); - apply_image_result(&mut metadata, None); - assert_eq!(metadata.image_fetch_state, LinkPreviewImageFetchState::None); - - apply_image_result( - &mut metadata, - Some(Err(ImageFetchError::Transient { - retry_after: Some(std::time::Duration::from_secs(15)), - retry_inline: false, - })), - ); - assert_eq!( - metadata.image_fetch_state, - LinkPreviewImageFetchState::TransientFailure - ); - assert_eq!(metadata.image_retry_after_ms, Some(15_000)); - - apply_image_result( - &mut metadata, - Some(Ok(( - "data:image/jpeg;base64,abc".to_string(), - "images.example.com".to_string(), - ))), - ); - assert_eq!( - metadata.image_fetch_state, - LinkPreviewImageFetchState::Image - ); - assert_eq!(metadata.image_domain.as_deref(), Some("images.example.com")); - } - - #[test] - fn metadata_falls_back_to_twitter_then_title() { - assert_eq!( - extract_link_preview_metadata("") - .map(|metadata| metadata.title), - Some("Tweet title".to_string()) - ); - assert_eq!( - extract_link_preview_metadata(" Plain title ") - .map(|metadata| metadata.title), - Some("Plain title".to_string()) - ); - } - - #[test] - fn metadata_preserves_description_line_breaks() { - let html = r#" - "#; - assert_eq!( - extract_link_preview_metadata(html).and_then(|metadata| metadata.description), - Some("First paragraph.\n\nAgents:\n- One\n- Two".to_string()) - ); - } - - #[test] - fn metadata_description_supports_standard_x_posts() { - let description = "x".repeat(MAX_METADATA_DESCRIPTION_CHARS + 1); - let html = format!( - r#""# - ); - let extracted = extract_link_preview_metadata(&html) - .and_then(|metadata| metadata.description) - .unwrap(); - assert_eq!(extracted.chars().count(), MAX_METADATA_DESCRIPTION_CHARS); - } - - #[test] - fn favicon_metadata_resolves_relative_icon_links() { - let page = Url::parse("https://example.com/articles/one").unwrap(); - let html = r#" - "#; - assert_eq!( - extract_favicon_url(html, &page).unwrap().as_str(), - "https://example.com/favicon.png" - ); - } - - #[test] - fn favicon_metadata_prefers_a_supported_raster_candidate() { - let page = Url::parse("https://github.com/block/buzz").unwrap(); - let html = r#" - - "#; - assert_eq!( - extract_favicon_url(html, &page).unwrap().as_str(), - "https://assets.example/favicon.png" - ); - } - - #[test] - fn favicon_metadata_uses_touch_icon_before_unsupported_ico() { - let page = Url::parse("https://twitter.com/tellaho").unwrap(); - let html = r#" - "#; - assert_eq!( - extract_favicon_url(html, &page).unwrap().as_str(), - "https://twitter.com/apple-touch-icon.png" - ); - } - - #[test] - fn image_metadata_resolves_relative_urls_and_prefers_open_graph() { - let page = Url::parse("https://example.com/articles/one").unwrap(); - let html = r#" - "#; - assert_eq!( - extract_image_url(html, &page).unwrap().as_str(), - "https://example.com/preview.png" - ); - } - - #[tokio::test] - async fn oversized_html_uses_metadata_within_the_bounded_prefix() { - const LIMIT: usize = 256; - let metadata = r#""#; - let body = format!("{metadata}{}", "x".repeat(LIMIT)); - let response = test_response( - Router::new().route( - "/declared", - get(move || { - let body = body.clone(); - async move { - Response::builder() - .header("content-type", "text/html") - .body(Body::from(body)) - .unwrap() - } - }), - ), - "/declared", - ) - .await; - assert!(response - .content_length() - .is_some_and(|size| size > LIMIT as u64)); - assert!(is_html_response(&response)); - - let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); - assert_eq!(prefix.len(), LIMIT); - let html = String::from_utf8_lossy(&prefix); - assert_eq!( - extract_link_preview_metadata(&html).map(|metadata| metadata.title), - Some("Prefix title".to_string()) - ); - assert!(extract_image_url(&html, &Url::parse("https://example.com").unwrap()).is_some()); - } - - #[tokio::test] - async fn image_retry_after_uses_bounded_delta_seconds() { - let response = test_response( - Router::new().route( - "/rate-limited", - get(|| async { - Response::builder() - .status(429) - .header("retry-after", "900") - .body(Body::empty()) - .unwrap() - }), - ), - "/rate-limited", - ) - .await; - assert_eq!( - retry_after_duration(&response), - Some(std::time::Duration::from_secs(900)) - ); - - let response = test_response( - Router::new().route( - "/excessive", - get(|| async { - Response::builder() - .status(429) - .header("retry-after", "7200") - .body(Body::empty()) - .unwrap() - }), - ), - "/excessive", - ) - .await; - assert_eq!(retry_after_duration(&response), Some(MAX_IMAGE_RETRY_AFTER)); - } - - #[tokio::test] - async fn oversized_chunked_html_ignores_metadata_beyond_the_bounded_prefix() { - const LIMIT: usize = 256; - let response = test_response( - Router::new().route( - "/chunked", - get(|| async { - let chunks = stream::iter([ - Ok::<_, Infallible>(Bytes::from(vec![b'x'; LIMIT])), - Ok(Bytes::from_static( - br#""#, - )), - ]); - Response::builder() - .header("content-type", "text/html") - .body(Body::from_stream(chunks)) - .unwrap() - }), - ), - "/chunked", - ) - .await; - assert_eq!(response.content_length(), None); - - let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); - assert_eq!(prefix.len(), LIMIT); - let html = String::from_utf8_lossy(&prefix); - assert_eq!(extract_link_preview_metadata(&html), None); - assert_eq!( - extract_image_url(&html, &Url::parse("https://example.com").unwrap()), - None - ); - } - - #[test] - fn sanitizer_rejects_mime_mismatch_and_outputs_static_jpeg() { - let source = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); - let mut png = Cursor::new(Vec::new()); - source.write_to(&mut png, ImageFormat::Png).unwrap(); - assert!(sanitize_image(png.get_ref(), "image/jpeg", false).is_err()); - let sanitized = sanitize_image(png.get_ref(), "image/png", false).unwrap(); - assert!(sanitized.starts_with("data:image/jpeg;base64,")); - } - - #[test] - fn favicon_sanitizer_preserves_png_transparency() { - let source = DynamicImage::ImageRgba8(RgbaImage::from_pixel(2, 2, Rgba([36, 41, 47, 0]))); - let mut png = Cursor::new(Vec::new()); - source.write_to(&mut png, ImageFormat::Png).unwrap(); - - let sanitized = sanitize_image(png.get_ref(), "image/png", true).unwrap(); - assert!(sanitized.starts_with("data:image/png;base64,")); - let encoded = sanitized.split_once(',').unwrap().1; - let bytes = base64::engine::general_purpose::STANDARD - .decode(encoded) - .unwrap(); - assert!(image::load_from_memory(&bytes).unwrap().color().has_alpha()); - } - - #[test] - fn animation_markers_are_rejected_before_decode() { - let mut apng = b"\x89PNG\r\n\x1a\n".to_vec(); - apng.extend_from_slice(b"junkacTLjunk"); - assert!(declares_animation(&apng, ImageFormat::Png)); - - let mut webp = b"RIFF\x00\x00\x00\x00WEBPVP8X\x0a\x00\x00\x00".to_vec(); - webp.push(0x02); - assert!(declares_animation(&webp, ImageFormat::WebP)); - } - - #[test] - fn metadata_requires_a_non_empty_title() { - assert_eq!(extract_link_preview_metadata(" "), None); - assert_eq!(extract_link_preview_metadata(""), None); - } -} +#[path = "link_preview_tests.rs"] +mod tests; diff --git a/desktop/src-tauri/src/commands/link_preview_cancellation.rs b/desktop/src-tauri/src/commands/link_preview_cancellation.rs new file mode 100644 index 00000000000..38be2b3a386 --- /dev/null +++ b/desktop/src-tauri/src/commands/link_preview_cancellation.rs @@ -0,0 +1,89 @@ +use std::{ + collections::HashMap, + sync::{LazyLock, Mutex}, +}; + +use tokio_util::sync::CancellationToken; + +#[derive(Default)] +struct LinkPreviewCancellations { + tokens: HashMap, +} + +impl LinkPreviewCancellations { + fn begin(&mut self, request_id: &str) -> CancellationToken { + if let Some(cancellation) = self.tokens.get(request_id).cloned() { + return cancellation; + } + let cancellation = CancellationToken::new(); + self.tokens + .insert(request_id.to_string(), cancellation.clone()); + cancellation + } + + fn cancel(&mut self, request_id: &str) { + self.tokens + .entry(request_id.to_string()) + .or_default() + .cancel(); + } + + fn finish(&mut self, request_id: &str) { + self.tokens.remove(request_id); + } +} + +static LINK_PREVIEW_CANCELLATIONS: LazyLock> = + LazyLock::new(|| Mutex::new(LinkPreviewCancellations::default())); + +pub(super) fn begin(request_id: Option<&str>) -> Option { + let request_id = request_id?; + LINK_PREVIEW_CANCELLATIONS + .lock() + .ok() + .map(|mut fetches| fetches.begin(request_id)) +} + +pub(super) fn cancel(request_id: &str) { + if let Ok(mut fetches) = LINK_PREVIEW_CANCELLATIONS.lock() { + fetches.cancel(request_id); + } +} + +pub(super) fn finish(request_id: Option<&str>) { + let Some(request_id) = request_id else { + return; + }; + if let Ok(mut fetches) = LINK_PREVIEW_CANCELLATIONS.lock() { + fetches.finish(request_id); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn cancellation_before_begin_is_retained() { + let mut fetches = LinkPreviewCancellations::default(); + fetches.cancel("cancel-before-begin"); + + let cancellation = fetches.begin("cancel-before-begin"); + + assert!(cancellation.is_cancelled()); + fetches.finish("cancel-before-begin"); + assert!(fetches.tokens.is_empty()); + } + + #[test] + fn cancellation_reaches_active_owner() { + let mut fetches = LinkPreviewCancellations::default(); + let cancellation = fetches.begin("active-fetch"); + + fetches.cancel("active-fetch"); + + assert!(cancellation.is_cancelled()); + fetches.finish("active-fetch"); + assert!(fetches.tokens.is_empty()); + } +} diff --git a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs index c3f7ed7188c..46032fc5f01 100644 --- a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs +++ b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs @@ -1,16 +1,31 @@ use std::{ - collections::HashMap, - sync::{Mutex, OnceLock}, - time::{Duration, Instant}, + collections::{hash_map::DefaultHasher, HashMap}, + hash::{Hash, Hasher}, + sync::{LazyLock, Mutex, OnceLock}, + time::Duration, }; use reqwest::header::RETRY_AFTER; +use tokio::{sync::Mutex as AsyncMutex, time::Instant}; use url::Url; pub(super) const MAX_IMAGE_RETRY_AFTER: Duration = Duration::from_secs(60 * 60); const MAX_IMAGE_HOST_COOLDOWNS: usize = 128; +const IMAGE_HOST_GATE_COUNT: usize = 64; static IMAGE_HOST_COOLDOWNS: OnceLock>> = OnceLock::new(); +// A bounded stripe table serializes image requests by host without retaining an +// unbounded attacker-controlled hostname map. Hash collisions only make two +// unrelated hosts wait for one another; they never weaken the host boundary. +static IMAGE_HOST_GATES: LazyLock<[AsyncMutex<()>; IMAGE_HOST_GATE_COUNT]> = + LazyLock::new(|| std::array::from_fn(|_| AsyncMutex::new(()))); + +pub(super) fn image_host_gate(url: &Url) -> &'static AsyncMutex<()> { + let mut hasher = DefaultHasher::new(); + url.host_str().unwrap_or_default().hash(&mut hasher); + let index = (hasher.finish() as usize) % IMAGE_HOST_GATE_COUNT; + &IMAGE_HOST_GATES[index] +} pub(super) fn retry_after_duration(response: &reqwest::Response) -> Option { response diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs new file mode 100644 index 00000000000..05ab0eec2ce --- /dev/null +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -0,0 +1,604 @@ +use super::rate_limit::MAX_IMAGE_RETRY_AFTER; +use super::{ + apply_image_result, cancel_link_preview_metadata, declares_animation, extract_favicon_url, + extract_image_url, extract_link_preview_metadata, fetch_link_preview_metadata, + fetch_sanitized_image_using, is_html_response, read_bytes_prefix, retry_after_duration, + retryable_image_cooldown, sanitize_image, ImageFetchError, LinkPreviewImageFetchState, + LinkPreviewMetadata, MAX_INLINE_IMAGE_COOLDOWN, MAX_METADATA_DESCRIPTION_CHARS, +}; +use axum::{body::Body, http::Response, routing::get, Router}; +use base64::Engine as _; +use bytes::Bytes; +use futures_util::stream; +use image::{DynamicImage, ImageFormat, Rgb, RgbImage, Rgba, RgbaImage}; +use std::{ + convert::Infallible, + io::Cursor, + sync::{Arc, Mutex}, +}; +use tokio::sync::oneshot; +use url::Url; + +async fn start_test_server(router: Router) -> std::net::SocketAddr { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, router).await.unwrap(); + }); + address +} + +async fn test_response(router: Router, path: &str) -> reqwest::Response { + let address = start_test_server(router).await; + reqwest::get(format!("http://{address}{path}")) + .await + .unwrap() +} + +#[tokio::test(start_paused = true)] +async fn metadata_pipeline_remains_pending_beyond_former_aggregate_deadline() { + let (request_started_tx, request_started_rx) = oneshot::channel::<()>(); + let request_started_tx = Arc::new(Mutex::new(Some(request_started_tx))); + let (release_response_tx, release_response_rx) = oneshot::channel::<()>(); + let release_response_rx = Arc::new(Mutex::new(Some(release_response_rx))); + let address = start_test_server(Router::new().route( + "/preview", + get(move || { + let request_started_tx = Arc::clone(&request_started_tx); + let release_response_rx = Arc::clone(&release_response_rx); + async move { + request_started_tx + .lock() + .unwrap() + .take() + .unwrap() + .send(()) + .unwrap(); + let release_response_rx = release_response_rx.lock().unwrap().take().unwrap(); + release_response_rx.await.unwrap(); + Response::builder() + .header("content-type", "text/html") + .body(Body::from("User-paced metadata")) + .unwrap() + } + }), + )) + .await; + let fetch = tokio::spawn(super::METADATA_TEST_SERVER.scope( + address, + fetch_link_preview_metadata("https://user-paced.example/preview".to_string(), None), + )); + + request_started_rx.await.unwrap(); + tokio::time::advance(std::time::Duration::from_secs(11)).await; + assert!(!fetch.is_finished()); + + release_response_tx.send(()).unwrap(); + let metadata = fetch.await.unwrap().unwrap().unwrap(); + assert_eq!(metadata.title, "User-paced metadata"); +} + +#[tokio::test] +async fn metadata_command_cancellation_drops_an_in_flight_response() { + let (request_started_tx, request_started_rx) = oneshot::channel::<()>(); + let request_started_tx = Arc::new(Mutex::new(Some(request_started_tx))); + let (_release_response_tx, release_response_rx) = oneshot::channel::<()>(); + let release_response_rx = Arc::new(Mutex::new(Some(release_response_rx))); + let address = start_test_server(Router::new().route( + "/preview", + get(move || { + let request_started_tx = Arc::clone(&request_started_tx); + let release_response_rx = Arc::clone(&release_response_rx); + async move { + request_started_tx + .lock() + .unwrap() + .take() + .unwrap() + .send(()) + .unwrap(); + let release_response_rx = release_response_rx.lock().unwrap().take().unwrap(); + let _ = release_response_rx.await; + Response::builder() + .header("content-type", "text/html") + .body(Body::from("Too late")) + .unwrap() + } + }), + )) + .await; + let request_id = "cancel-in-flight".to_string(); + let fetch = tokio::spawn(super::METADATA_TEST_SERVER.scope( + address, + fetch_link_preview_metadata( + "https://cancel.example/preview".to_string(), + Some(request_id.clone()), + ), + )); + + request_started_rx.await.unwrap(); + cancel_link_preview_metadata(request_id); + + assert_eq!( + fetch.await.unwrap(), + Err("link preview request cancelled".to_string()) + ); +} + +#[tokio::test(start_paused = true)] +async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() { + let cooldown = std::time::Duration::from_secs(20); + let rate_limited_path = "/rate-limited.png"; + let success_path = "/success.png"; + let url = Url::parse(&format!( + "https://rate-limit-regression.example{rate_limited_path}" + )) + .unwrap(); + let attempts = Arc::new(Mutex::new(0)); + let collision_attempts = Arc::new(Mutex::new(0)); + let image = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); + let mut png = Cursor::new(Vec::new()); + image.write_to(&mut png, ImageFormat::Png).unwrap(); + let image_bytes = png.into_inner(); + let server_attempts = Arc::clone(&attempts); + let address = start_test_server(Router::new().route( + "/{image}", + get( + move |axum::extract::Path(image): axum::extract::Path| { + let image_bytes = image_bytes.clone(); + let server_attempts = Arc::clone(&server_attempts); + async move { + if image == "rate-limited.png" { + let attempt = { + let mut attempts = server_attempts.lock().unwrap(); + *attempts += 1; + *attempts + }; + if attempt == 1 { + return Response::builder() + .status(429) + .header("retry-after", cooldown.as_secs()) + .body(Body::empty()) + .unwrap(); + } + } + Response::builder() + .header("content-type", "image/png") + .body(Body::from(image_bytes)) + .unwrap() + } + }, + ), + )) + .await; + let test_client = reqwest::Client::new(); + let request = move |url: Url, _accept: &'static str| { + let test_client = test_client.clone(); + async move { + test_client + .get(format!("http://{address}{}", url.path())) + .send() + .await + .map_err(|error| error.to_string()) + } + }; + let collision_request = { + let collision_attempts = Arc::clone(&collision_attempts); + let test_client = reqwest::Client::new(); + move |url: Url, _accept: &'static str| { + let collision_attempts = Arc::clone(&collision_attempts); + let test_client = test_client.clone(); + async move { + *collision_attempts.lock().unwrap() += 1; + test_client + .get(format!("http://{address}{}", url.path())) + .send() + .await + .map_err(|error| error.to_string()) + } + } + }; + let validate = |_url: Url| async { Ok(()) }; + let first = tokio::spawn(fetch_sanitized_image_using( + url.clone(), + false, + validate, + request.clone(), + )); + while super::image_host_cooldown_remaining(&url).is_none() { + tokio::task::yield_now().await; + } + assert!(!first.is_finished()); + assert_eq!(*attempts.lock().unwrap(), 1); + assert_eq!(super::image_host_cooldown_remaining(&url), Some(cooldown)); + + let colliding_url = (0..10_000) + .map(|index| { + Url::parse(&format!("https://collision-{index}.example{success_path}")).unwrap() + }) + .find(|candidate| { + std::ptr::eq( + super::image_host_gate(candidate), + super::image_host_gate(&url), + ) + }) + .expect("a different host sharing the bounded gate stripe"); + + let (collision_started_tx, collision_started_rx) = oneshot::channel(); + tokio::spawn(async move { + let collision = + fetch_sanitized_image_using(colliding_url, false, validate, collision_request); + tokio::pin!(collision); + assert!(futures_util::poll!(&mut collision).is_pending()); + collision_started_tx.send(()).ok(); + assert!(collision.await.is_ok()); + }); + collision_started_rx.await.unwrap(); + assert_eq!(*collision_attempts.lock().unwrap(), 1); + assert_eq!(*attempts.lock().unwrap(), 1); + + let queued = tokio::spawn(fetch_sanitized_image_using(url, false, validate, request)); + tokio::task::yield_now().await; + assert!(!queued.is_finished()); + assert_eq!(*attempts.lock().unwrap(), 1); + + tokio::time::advance(cooldown - std::time::Duration::from_millis(1)).await; + assert!(!first.is_finished()); + assert!(!queued.is_finished()); + assert_eq!(*attempts.lock().unwrap(), 1); + + tokio::time::advance(std::time::Duration::from_millis(1)).await; + let (first, queued) = tokio::join!(first, queued); + assert!(first.unwrap().is_ok()); + assert!(queued.unwrap().is_ok()); + assert_eq!(*attempts.lock().unwrap(), 3); +} + +#[tokio::test(start_paused = true)] +async fn transport_failure_after_cooldown_does_not_renew_wait_on_outer_retry() { + let cooldown = std::time::Duration::from_secs(20); + let url = Url::parse("https://transport-after-cooldown.example/image.png").unwrap(); + super::set_image_host_cooldown(&url, cooldown); + let attempts = Arc::new(Mutex::new(0)); + + let result = super::image_retry::retry_transient_image_fetch(|| { + let url = url.clone(); + let attempts = Arc::clone(&attempts); + async move { + fetch_sanitized_image_using( + url, + false, + |_url| async { Ok(()) }, + move |_url, _accept| { + let attempts = Arc::clone(&attempts); + async move { + *attempts.lock().unwrap() += 1; + Err("connection failed".to_string()) + } + }, + ) + .await + } + }) + .await; + + assert_eq!( + result, + Err(ImageFetchError::Transient { + retry_after: None, + retry_inline: false, + }) + ); + assert_eq!(*attempts.lock().unwrap(), 1); +} + +#[test] +fn image_cooldown_wait_is_short_and_one_shot() { + let url = Url::parse("https://bounded-cooldown.example/image.png").unwrap(); + let mut waited = false; + assert_eq!( + retryable_image_cooldown(&url, Some(MAX_INLINE_IMAGE_COOLDOWN), &mut waited,), + Some(MAX_INLINE_IMAGE_COOLDOWN) + ); + assert!(waited); + assert_eq!( + retryable_image_cooldown(&url, Some(MAX_INLINE_IMAGE_COOLDOWN), &mut waited,), + None + ); + let excessive_url = Url::parse("https://excessive-cooldown.example/image.png").unwrap(); + let mut excessive_waited = false; + assert_eq!( + retryable_image_cooldown( + &excessive_url, + Some(MAX_INLINE_IMAGE_COOLDOWN + std::time::Duration::from_secs(1)), + &mut excessive_waited, + ), + None + ); + assert!(!excessive_waited); +} + +#[test] +fn metadata_prefers_open_graph_and_reads_site_name() { + let html = r#" + + + Fallback"#; + assert_eq!( + extract_link_preview_metadata(html), + Some(LinkPreviewMetadata { + title: "Rich previews & cards".to_string(), + site_name: Some("Buzz".to_string()), + description: Some("Safe & useful previews".to_string()), + image_data_url: None, + image_domain: None, + image_fetch_state: LinkPreviewImageFetchState::None, + image_retry_after_ms: None, + favicon_data_url: None, + }) + ); +} + +#[test] +fn image_results_preserve_absence_and_classify_recovery() { + let mut metadata = extract_link_preview_metadata("Preview result").unwrap(); + apply_image_result(&mut metadata, None); + assert_eq!(metadata.image_fetch_state, LinkPreviewImageFetchState::None); + + apply_image_result( + &mut metadata, + Some(Err(ImageFetchError::Transient { + retry_after: Some(std::time::Duration::from_secs(15)), + retry_inline: false, + })), + ); + assert_eq!( + metadata.image_fetch_state, + LinkPreviewImageFetchState::TransientFailure + ); + assert_eq!(metadata.image_retry_after_ms, Some(15_000)); + + apply_image_result( + &mut metadata, + Some(Ok(( + "data:image/jpeg;base64,abc".to_string(), + "images.example.com".to_string(), + ))), + ); + assert_eq!( + metadata.image_fetch_state, + LinkPreviewImageFetchState::Image + ); + assert_eq!(metadata.image_domain.as_deref(), Some("images.example.com")); +} + +#[test] +fn metadata_falls_back_to_twitter_then_title() { + assert_eq!( + extract_link_preview_metadata("") + .map(|metadata| metadata.title), + Some("Tweet title".to_string()) + ); + assert_eq!( + extract_link_preview_metadata(" Plain title ") + .map(|metadata| metadata.title), + Some("Plain title".to_string()) + ); +} + +#[test] +fn metadata_preserves_description_line_breaks() { + let html = r#" + "#; + assert_eq!( + extract_link_preview_metadata(html).and_then(|metadata| metadata.description), + Some("First paragraph.\n\nAgents:\n- One\n- Two".to_string()) + ); +} + +#[test] +fn metadata_description_supports_standard_x_posts() { + let description = "x".repeat(MAX_METADATA_DESCRIPTION_CHARS + 1); + let html = format!( + r#""# + ); + let extracted = extract_link_preview_metadata(&html) + .and_then(|metadata| metadata.description) + .unwrap(); + assert_eq!(extracted.chars().count(), MAX_METADATA_DESCRIPTION_CHARS); +} + +#[test] +fn favicon_metadata_resolves_relative_icon_links() { + let page = Url::parse("https://example.com/articles/one").unwrap(); + let html = r#" + "#; + assert_eq!( + extract_favicon_url(html, &page).unwrap().as_str(), + "https://example.com/favicon.png" + ); +} + +#[test] +fn favicon_metadata_prefers_a_supported_raster_candidate() { + let page = Url::parse("https://github.com/block/buzz").unwrap(); + let html = r#" + + "#; + assert_eq!( + extract_favicon_url(html, &page).unwrap().as_str(), + "https://assets.example/favicon.png" + ); +} + +#[test] +fn favicon_metadata_uses_touch_icon_before_unsupported_ico() { + let page = Url::parse("https://twitter.com/tellaho").unwrap(); + let html = r#" + "#; + assert_eq!( + extract_favicon_url(html, &page).unwrap().as_str(), + "https://twitter.com/apple-touch-icon.png" + ); +} + +#[test] +fn image_metadata_resolves_relative_urls_and_prefers_open_graph() { + let page = Url::parse("https://example.com/articles/one").unwrap(); + let html = r#" + "#; + assert_eq!( + extract_image_url(html, &page).unwrap().as_str(), + "https://example.com/preview.png" + ); +} + +#[tokio::test] +async fn oversized_html_uses_metadata_within_the_bounded_prefix() { + const LIMIT: usize = 256; + let metadata = r#""#; + let body = format!("{metadata}{}", "x".repeat(LIMIT)); + let response = test_response( + Router::new().route( + "/declared", + get(move || { + let body = body.clone(); + async move { + Response::builder() + .header("content-type", "text/html") + .body(Body::from(body)) + .unwrap() + } + }), + ), + "/declared", + ) + .await; + assert!(response + .content_length() + .is_some_and(|size| size > LIMIT as u64)); + assert!(is_html_response(&response)); + + let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); + assert_eq!(prefix.len(), LIMIT); + let html = String::from_utf8_lossy(&prefix); + assert_eq!( + extract_link_preview_metadata(&html).map(|metadata| metadata.title), + Some("Prefix title".to_string()) + ); + assert!(extract_image_url(&html, &Url::parse("https://example.com").unwrap()).is_some()); +} + +#[tokio::test] +async fn image_retry_after_uses_bounded_delta_seconds() { + let response = test_response( + Router::new().route( + "/rate-limited", + get(|| async { + Response::builder() + .status(429) + .header("retry-after", "900") + .body(Body::empty()) + .unwrap() + }), + ), + "/rate-limited", + ) + .await; + assert_eq!( + retry_after_duration(&response), + Some(std::time::Duration::from_secs(900)) + ); + + let response = test_response( + Router::new().route( + "/excessive", + get(|| async { + Response::builder() + .status(429) + .header("retry-after", "7200") + .body(Body::empty()) + .unwrap() + }), + ), + "/excessive", + ) + .await; + assert_eq!(retry_after_duration(&response), Some(MAX_IMAGE_RETRY_AFTER)); +} + +#[tokio::test] +async fn oversized_chunked_html_ignores_metadata_beyond_the_bounded_prefix() { + const LIMIT: usize = 256; + let response = test_response( + Router::new().route( + "/chunked", + get(|| async { + let chunks = stream::iter([ + Ok::<_, Infallible>(Bytes::from(vec![b'x'; LIMIT])), + Ok(Bytes::from_static( + br#""#, + )), + ]); + Response::builder() + .header("content-type", "text/html") + .body(Body::from_stream(chunks)) + .unwrap() + }), + ), + "/chunked", + ) + .await; + assert_eq!(response.content_length(), None); + + let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); + assert_eq!(prefix.len(), LIMIT); + let html = String::from_utf8_lossy(&prefix); + assert_eq!(extract_link_preview_metadata(&html), None); + assert_eq!( + extract_image_url(&html, &Url::parse("https://example.com").unwrap()), + None + ); +} + +#[test] +fn sanitizer_rejects_mime_mismatch_and_outputs_static_jpeg() { + let source = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); + let mut png = Cursor::new(Vec::new()); + source.write_to(&mut png, ImageFormat::Png).unwrap(); + assert!(sanitize_image(png.get_ref(), "image/jpeg", false).is_err()); + let sanitized = sanitize_image(png.get_ref(), "image/png", false).unwrap(); + assert!(sanitized.starts_with("data:image/jpeg;base64,")); +} + +#[test] +fn favicon_sanitizer_preserves_png_transparency() { + let source = DynamicImage::ImageRgba8(RgbaImage::from_pixel(2, 2, Rgba([36, 41, 47, 0]))); + let mut png = Cursor::new(Vec::new()); + source.write_to(&mut png, ImageFormat::Png).unwrap(); + + let sanitized = sanitize_image(png.get_ref(), "image/png", true).unwrap(); + assert!(sanitized.starts_with("data:image/png;base64,")); + let encoded = sanitized.split_once(',').unwrap().1; + let bytes = base64::engine::general_purpose::STANDARD + .decode(encoded) + .unwrap(); + assert!(image::load_from_memory(&bytes).unwrap().color().has_alpha()); +} + +#[test] +fn animation_markers_are_rejected_before_decode() { + let mut apng = b"\x89PNG\r\n\x1a\n".to_vec(); + apng.extend_from_slice(b"junkacTLjunk"); + assert!(declares_animation(&apng, ImageFormat::Png)); + + let mut webp = b"RIFF\x00\x00\x00\x00WEBPVP8X\x0a\x00\x00\x00".to_vec(); + webp.push(0x02); + assert!(declares_animation(&webp, ImageFormat::WebP)); +} + +#[test] +fn metadata_requires_a_non_empty_title() { + assert_eq!(extract_link_preview_metadata(" "), None); + assert_eq!(extract_link_preview_metadata(""), None); +} diff --git a/desktop/src-tauri/src/commands/link_preview_youtube.rs b/desktop/src-tauri/src/commands/link_preview_youtube.rs index a0a5a753dcd..b759046d8f0 100644 --- a/desktop/src-tauri/src/commands/link_preview_youtube.rs +++ b/desktop/src-tauri/src/commands/link_preview_youtube.rs @@ -5,8 +5,8 @@ use url::Url; use super::{ apply_image_result, fetch_sanitized_image, normalize_metadata_description, - normalize_metadata_text, read_limited_bytes, send_pinned_request, ImageFetchError, - LinkPreviewImageFetchState, LinkPreviewMetadata, PREVIEW_FETCH_TIMEOUT, + normalize_metadata_text, read_limited_bytes, send_pinned_request, LinkPreviewImageFetchState, + LinkPreviewMetadata, }; const MAX_OEMBED_FETCH_BYTES: usize = 64 * 1024; @@ -54,17 +54,7 @@ pub(super) async fn fetch_oembed_metadata( return Ok(None); }; let image_result = match thumbnail_url { - Some(thumbnail_url) => Some( - tokio::time::timeout( - PREVIEW_FETCH_TIMEOUT, - fetch_sanitized_image(thumbnail_url, false), - ) - .await - .unwrap_or(Err(ImageFetchError::Transient { - retry_after: None, - retry_inline: false, - })), - ), + Some(thumbnail_url) => Some(fetch_sanitized_image(thumbnail_url, false).await), None => None, }; apply_image_result(&mut metadata, image_result); diff --git a/desktop/src-tauri/src/egress_guard_tests.rs b/desktop/src-tauri/src/egress_guard_tests.rs index d0c3600cb70..ad503e1542c 100644 --- a/desktop/src-tauri/src/egress_guard_tests.rs +++ b/desktop/src-tauri/src/egress_guard_tests.rs @@ -275,6 +275,7 @@ const EVENTS_INVENTORY: &[(&str, usize, usize)] = &[ ("src/native_websocket.rs", 0, 2), // boundary 8 (WS frames; no events URL) // Test-only fixtures — no production egress, no guard: ("src/relay_admission.rs", 1, 0), + ("src/native_relay_client_transport_tests.rs", 1, 0), ("src/archive/mod_tests.rs", 1, 0), ("src/managed_agents/persona_events/tests.rs", 1, 0), ("src/commands/team_snapshot/tests.rs", 1, 0), diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 670b6a1b188..3b3e2787a73 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -548,6 +548,8 @@ pub fn run() { get_relay_http_url, get_media_proxy_port, fetch_link_preview_metadata, + cancel_link_preview_metadata, + release_link_preview_metadata, discover_acp_auth_methods, discover_acp_providers, discover_git_bash_prerequisite, diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 37e0a782554..586e7b4c609 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -783,12 +783,14 @@ pub(crate) fn classify_runtime( /// The oldest `codex-acp` version supported by Buzz managed agents. /// /// Older 1.x adapters are detected successfully, but can still bundle a Codex runtime -/// that does not reliably give `buzz` CLI subprocesses outbound relay access. +/// that cannot use newer models. Adapter 1.6.2 bundles Codex 0.148.x, which rejects +/// GPT-6 Astra even when the separately installed Codex CLI has been updated. +/// Published adapter 1.10.0 depends on `@openai/codex ^0.153.3`. /// /// Bump policy: raise this only when a newer adapter fixes a defect that breaks managed /// agents, and only to a version already published on npm — every user below the floor is /// offered a reinstall on their next discovery pass. -pub(crate) const MIN_CODEX_ACP_VERSION: (u64, u64, u64) = (1, 1, 7); +pub(crate) const MIN_CODEX_ACP_VERSION: (u64, u64, u64) = (1, 10, 0); /// Probe the full version of a `codex-acp` binary by running `--version`. /// diff --git a/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs b/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs index a716dee9f56..ad3a4dad23d 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/login_shell_spawn_probe.rs @@ -13,9 +13,96 @@ pub(crate) fn record() { } pub(crate) fn reset() { + #[cfg(unix)] + assert!( + is_isolated_process(), + "counter measurements must run in an isolated test process" + ); COUNT.store(0, Ordering::SeqCst); } pub(crate) fn count() -> usize { COUNT.load(Ordering::SeqCst) } + +#[cfg(unix)] +const CHILD_TEST: &str = "BUZZ_LOGIN_SHELL_PROBE_TEST"; + +#[cfg(unix)] +fn is_isolated_process() -> bool { + std::thread::current() + .name() + .is_some_and(|name| std::env::var(CHILD_TEST).as_deref() == Ok(name)) +} + +/// Run a counter-owning test body alone in a fresh libtest process. +/// +/// The counter deliberately stays process-wide: discovery's auth workers must +/// count too. A voluntary PATH lock cannot exclude unrelated, unlocked probe +/// callers in the full suite. Process isolation excludes those callers without +/// changing production code or requiring new workers to inherit test context. +/// Call while holding the PATH lock so the child inherits stable environment. +#[cfg(unix)] +pub(crate) fn run_in_isolated_process(test: impl FnOnce()) { + use std::process::Command; + use std::time::Duration; + + let thread = std::thread::current(); + let name = thread.name().expect("libtest names its test threads"); + let completed = format!("completed isolated login-shell probe test: {name}"); + if is_isolated_process() { + test(); + // Receipt only after the assertion body returns, never on entry. + println!("{completed}"); + return; + } + + let mut command = Command::new(std::env::current_exe().expect("test executable")); + command + .args(["--exact", name, "--nocapture", "--test-threads=1"]) + .env(CHILD_TEST, name); + let output = super::bounded_command::output_with_timeout(command, Duration::from_secs(300)) + .expect("isolated probe test must finish within five minutes and the output cap"); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + output.status.success() && stdout.lines().any(|line| line.contains(&completed)), + "isolated probe test {name} failed or did not run: {}\n{stdout}\n{stderr}", + output.status + ); +} + +#[cfg(unix)] +#[test] +fn isolated_counter_excludes_parent_probes_but_counts_own_workers() { + let _guard = crate::managed_agents::lock_path_mutex(); + let probe_on_worker = || { + std::thread::spawn(|| { + // A real production probe, independent of the shared PATH cache. + super::find_via_login_shell("buzz-absent-probe-isolation-xyzzy") + }) + .join() + .expect("probe worker must finish") + }; + if !is_isolated_process() { + assert!(probe_on_worker().is_none()); + assert!(count() >= 1, "parent worker must reach the real probe"); + } + run_in_isolated_process(|| { + // Before reset: unrelated parent probes must not enter this process. + assert_eq!(count(), 0, "the child must start with its own counter"); + reset(); + assert!(probe_on_worker().is_none()); + }); + if is_isolated_process() { + // Outside the closure so omitting its invocation cannot pass this test. + assert_eq!(count(), 1, "the child's assertion body and worker must run"); + } +} + +#[cfg(unix)] +#[test] +#[should_panic(expected = "counter measurements must run in an isolated test process")] +fn reset_rejects_unisolated_measurement() { + reset(); +} diff --git a/desktop/src-tauri/src/managed_agents/discovery/presets.rs b/desktop/src-tauri/src/managed_agents/discovery/presets.rs index fa22142f1bd..f4d9d53dfb4 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/presets.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/presets.rs @@ -107,13 +107,13 @@ pub(super) const PRESET_HARNESSES: &[PresetHarness] = &[ PresetHarness { id: "pi", label: "Pi", - command: "pi-acp", + command: "buzz-pi-acp", args: &[], - install_instructions_url: "https://github.com/svkozak/pi-acp", - install_hint: "Install the Pi ACP adapter with npm install -g pi-acp.", + install_instructions_url: "https://github.com/salman1993/pi-acp", + install_hint: "Requires Node.js 22 or newer. Install the Pi ACP adapter with `npm install -g --install-links=true git+https://github.com/salman1993/pi-acp.git#main`. Restart Waggle, then select Pi as the agent harness. Run the same install command again to update the adapter.", underlying_cli: Some("pi"), underlying_cli_install_hint: Some( - "Install Pi with npm install -g --ignore-scripts @earendil-works/pi-coding-agent.", + "Install Pi with `npm install -g @earendil-works/pi-coding-agent`, then run `pi` to configure its model provider.", ), underlying_cli_install_instructions_url: Some( "https://github.com/badlogic/pi-mono/tree/main/packages/coding-agent", @@ -404,17 +404,17 @@ mod tests { .expect("Pi preset should be present"); assert_eq!(preset.label, "Pi"); - assert_eq!(preset.command, "pi-acp"); + assert_eq!(preset.command, "buzz-pi-acp"); assert!(preset.args.is_empty()); assert_eq!(preset.underlying_cli, Some("pi")); let available = preset_catalog_entry(preset, |command| match command { - "pi-acp" => Some(PathBuf::from("/usr/local/bin/pi-acp")), + "buzz-pi-acp" => Some(PathBuf::from("/usr/local/bin/buzz-pi-acp")), "pi" => Some(PathBuf::from("/usr/local/bin/pi")), _ => None, }); assert_eq!(available.availability, AcpAvailabilityStatus::Available); - assert_eq!(available.command.as_deref(), Some("pi-acp")); + assert_eq!(available.command.as_deref(), Some("buzz-pi-acp")); assert!(available.default_args.is_empty()); assert!(available.install_hint.is_empty()); assert!(available.requires_external_cli); @@ -434,21 +434,21 @@ mod tests { assert!(adapter_missing.default_args.is_empty()); assert_eq!( adapter_missing.install_hint, - "Install the Pi ACP adapter with npm install -g pi-acp." + "Requires Node.js 22 or newer. Install the Pi ACP adapter with `npm install -g --install-links=true git+https://github.com/salman1993/pi-acp.git#main`. Restart Waggle, then select Pi as the agent harness. Run the same install command again to update the adapter." ); assert_eq!( adapter_missing.install_instructions_url, - "https://github.com/svkozak/pi-acp" + "https://github.com/salman1993/pi-acp" ); let cli_missing = preset_catalog_entry(preset, |command| { - (command == "pi-acp").then(|| PathBuf::from("/usr/local/bin/pi-acp")) + (command == "buzz-pi-acp").then(|| PathBuf::from("/usr/local/bin/buzz-pi-acp")) }); assert_eq!(cli_missing.availability, AcpAvailabilityStatus::CliMissing); - assert_eq!(cli_missing.command.as_deref(), Some("pi-acp")); + assert_eq!(cli_missing.command.as_deref(), Some("buzz-pi-acp")); assert_eq!( cli_missing.install_hint, - "Install Pi with npm install -g --ignore-scripts @earendil-works/pi-coding-agent." + "Install Pi with `npm install -g @earendil-works/pi-coding-agent`, then run `pi` to configure its model provider." ); assert_eq!( cli_missing.install_instructions_url, @@ -462,7 +462,7 @@ mod tests { ); assert_eq!( not_installed.install_hint, - "Install Pi with npm install -g --ignore-scripts @earendil-works/pi-coding-agent. Install the Pi ACP adapter with npm install -g pi-acp." + "Install Pi with `npm install -g @earendil-works/pi-coding-agent`, then run `pi` to configure its model provider. Requires Node.js 22 or newer. Install the Pi ACP adapter with `npm install -g --install-links=true git+https://github.com/salman1993/pi-acp.git#main`. Restart Waggle, then select Pi as the agent harness. Run the same install command again to update the adapter." ); } @@ -601,4 +601,34 @@ mod tests { "uncapped preset (devin) must have max_parallelism: None" ); } + + /// Fork branding: no preset's user-visible setup guidance may name the + /// upstream app. This fork ships the desktop app as Waggle + /// (`docs/fork-branding.md`), so an instruction to "Restart Buzz" points a + /// user at a different installation while the running Waggle process keeps + /// its stale PATH. Upstream authors these hints, so every fork sync can + /// reintroduce one -- assert the whole class, not just the preset that + /// happened to carry it. Lowercase command names (`buzz-pi-acp`, + /// `buzz-acp`) are binaries, not the brand, and stay allowed. + #[test] + fn preset_setup_hints_name_the_fork_app() { + for preset in PRESET_HARNESSES { + for (field, hint) in [ + ("install_hint", Some(preset.install_hint)), + ( + "underlying_cli_install_hint", + preset.underlying_cli_install_hint, + ), + ] { + let Some(hint) = hint else { + continue; + }; + assert!( + !hint.contains("Buzz"), + "preset `{}` {field} names the upstream app; this fork ships as Waggle: {hint}", + preset.id + ); + } + } + } } diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 000e97876ca..07c562bc14f 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -649,7 +649,7 @@ fn codex_adapter_availability_available_for_minimum_supported_binary() { let bin = dir.join("codex-acp"); std::fs::write( &bin, - "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.7'\nexit 0\n", + "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.10.0'\nexit 0\n", ) .expect("write script"); std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); @@ -686,27 +686,6 @@ fn codex_adapter_availability_outdated_for_0x_binary() { ); } -#[cfg(unix)] -#[test] -fn codex_adapter_availability_outdated_for_older_1x_binary() { - use std::os::unix::fs::PermissionsExt; - - let dir = tempfile::tempdir().expect("temp dir"); - let bin = dir.path().join("codex-acp"); - std::fs::write( - &bin, - "#!/bin/sh\necho '@agentclientprotocol/codex-acp 1.1.5'\nexit 0\n", - ) - .expect("write script"); - std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)).expect("chmod script"); - - assert_eq!( - codex_adapter_availability(&bin), - AcpAvailabilityStatus::AdapterOutdated, - "a 1.x adapter below the floor must be offered an upgrade" - ); -} - /// The strict three-component parse fails closed: a version Buzz cannot compare /// against the floor is treated as outdated rather than assumed current. #[cfg(unix)] diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs b/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs index 82bfd27f325..19053865cbe 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests/codex_version.rs @@ -46,3 +46,30 @@ fn probe_codex_acp_version_uses_augmented_path_for_env_shebang_interpreter() { "the injected augmented PATH should allow /usr/bin/env to find node" ); } + +#[cfg(unix)] +#[test] +fn codex_adapter_availability_outdated_for_older_1x_binary() { + use super::super::{codex_adapter_availability, codex_adapter_is_outdated}; + use crate::managed_agents::AcpAvailabilityStatus; + use std::os::unix::fs::PermissionsExt; + + for version in ["1.1.5", "1.1.7", "1.6.2", "1.9.0"] { + let dir = tempfile::tempdir().expect("temp dir"); + let bin = dir.path().join("codex-acp"); + std::fs::write( + &bin, + format!("#!/bin/sh\necho '@agentclientprotocol/codex-acp {version}'\nexit 0\n"), + ) + .expect("write script"); + std::fs::set_permissions(&bin, std::fs::Permissions::from_mode(0o755)) + .expect("chmod script"); + + assert_eq!( + codex_adapter_availability(&bin), + AcpAvailabilityStatus::AdapterOutdated, + "adapter {version} must be offered an upgrade" + ); + assert!(codex_adapter_is_outdated(&bin)); + } +} diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests/forced_discovery.rs b/desktop/src-tauri/src/managed_agents/discovery/tests/forced_discovery.rs index cfbad365e3a..4b2ed0e908f 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests/forced_discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests/forced_discovery.rs @@ -108,6 +108,14 @@ fn forced_discovery_probes_auth_but_cheap_discovery_reuses_cached_status() { #[cfg(unix)] #[test] fn cheap_discovery_reports_absent_before_any_forced_probe() { + let _path_guard = crate::managed_agents::lock_path_mutex(); + super::super::login_shell_spawn_probe::run_in_isolated_process( + cheap_discovery_reports_absent_before_any_forced_probe_body, + ); +} + +#[cfg(unix)] +fn cheap_discovery_reports_absent_before_any_forced_probe_body() { use crate::managed_agents::custom_harnesses::registry_test_lock; use crate::managed_agents::discovery::{ clear_resolve_cache, discover_acp_runtimes_from, login_shell_spawn_probe, @@ -115,7 +123,6 @@ fn cheap_discovery_reports_absent_before_any_forced_probe() { use crate::managed_agents::{AcpAvailabilityStatus, AuthStatus}; use std::os::unix::fs::PermissionsExt; - let _path_guard = crate::managed_agents::lock_path_mutex(); let _registry_guard = registry_test_lock(); let dir = tempfile::tempdir().expect("tempdir"); diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests/managed_path_resolution.rs b/desktop/src-tauri/src/managed_agents/discovery/tests/managed_path_resolution.rs index aab5cd45298..dce4c2910cb 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests/managed_path_resolution.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests/managed_path_resolution.rs @@ -126,6 +126,14 @@ fn resolve_command_prefers_buzz_managed_npm_shim_over_path() { #[cfg(unix)] #[test] fn cheap_discovery_never_spawns_login_shell_even_when_cold() { + let _path_guard = crate::managed_agents::lock_path_mutex(); + super::super::login_shell_spawn_probe::run_in_isolated_process( + cheap_discovery_never_spawns_login_shell_even_when_cold_body, + ); +} + +#[cfg(unix)] +fn cheap_discovery_never_spawns_login_shell_even_when_cold_body() { use crate::managed_agents::custom_harnesses::registry_test_lock; use crate::managed_agents::discovery::{ clear_resolve_cache, discover_acp_runtimes_from, login_shell_spawn_probe, @@ -133,9 +141,6 @@ fn cheap_discovery_never_spawns_login_shell_even_when_cold() { use std::fs; use tempfile::tempdir; - // Serialize with every other test that spawns a login shell: the spawn - // counter and the PATH/login-shell caches are process-global. - let _path_guard = crate::managed_agents::lock_path_mutex(); let _registry = registry_test_lock(); // A custom harness whose command cannot resolve anywhere, so the resolver diff --git a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs index 7aa886c672a..dc38c3d126f 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs @@ -145,14 +145,6 @@ fn reserved_keys_include_agent_owner_for_legacy_records() { assert!(merged.is_empty()); } -#[test] -fn reserved_keys_include_pi_acp_command() { - assert!(is_reserved_env_key("PI_ACP_PI_COMMAND")); - let agent = map(&[("PI_ACP_PI_COMMAND", "/tmp/custom-pi")]); - let merged = merged_user_env(&BTreeMap::new(), &agent); - assert!(merged.is_empty()); -} - #[test] fn reserved_keys_include_respond_to_gate() { // Respond-to mode + allowlist control who the agent answers. diff --git a/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs b/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs index d83bb212cb1..86bc4c28c80 100644 --- a/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs +++ b/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs @@ -41,9 +41,6 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[ "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", "BUZZ_ACP_MCP_COMMAND", - // pi-acp's executable override is reserved for Buzz's generated launcher, - // which injects the managed system prompt and skills. - "PI_ACP_PI_COMMAND", // Control-plane parallelism: the Desktop resolves the effective // worker-pool size (applying any per-harness cap) and writes it into // launch.policy_env. A user-supplied BUZZ_ACP_AGENTS would bypass the diff --git a/desktop/src-tauri/src/native_relay_client.rs b/desktop/src-tauri/src/native_relay_client.rs index 2237076a926..19740dd0197 100644 --- a/desktop/src-tauri/src/native_relay_client.rs +++ b/desktop/src-tauri/src/native_relay_client.rs @@ -771,11 +771,16 @@ impl ClosedRetry { self.due_at = None; } ClosedClass::RateLimited => { - // Arm the process-wide gate so the HTTP bridge backs off too, - // rather than keeping a second private notion of the same - // relay's back-pressure. + // WS quota/concurrency limits do not consume HTTP's ApiCalls + // budget. Only an explicit failure of the shared admission + // service warrants damping the other transport too. let hint = parse_retry_in_seconds(message); - crate::relay_admission::activate_rate_limit(hint); + if message + .trim() + .eq_ignore_ascii_case("rate-limited: shared admission unavailable") + { + crate::relay_admission::activate_rate_limit(None); + } let hinted = hint .map(Duration::from_secs) .unwrap_or(CLOSED_RATE_LIMIT_DEFAULT); diff --git a/desktop/src-tauri/src/native_relay_client_tests.rs b/desktop/src-tauri/src/native_relay_client_tests.rs index 96ec39a4bf0..ca2324327f3 100644 --- a/desktop/src-tauri/src/native_relay_client_tests.rs +++ b/desktop/src-tauri/src/native_relay_client_tests.rs @@ -487,7 +487,6 @@ async fn a_reconcile_preserves_the_backoff_of_a_still_desired_subscription() { reconcile, got {reopened:?}" ); - crate::relay_admission::reset_rate_limit_gate(); session.shutdown(); } @@ -726,7 +725,6 @@ fn a_rate_limited_closed_waits_at_least_the_relay_hint() { due >= Instant::now() + Duration::from_secs(11), "a 12s hint must not be undercut by the base backoff" ); - crate::relay_admission::reset_rate_limit_gate(); } #[test] @@ -739,7 +737,6 @@ fn a_hintless_rate_limited_closed_uses_the_shared_default() { due >= Instant::now() + CLOSED_RATE_LIMIT_DEFAULT - Duration::from_secs(1), "a hintless rate-limit must fall back to the shared default window" ); - crate::relay_admission::reset_rate_limit_gate(); } #[test] @@ -894,3 +891,6 @@ async fn the_first_lease_installs_a_session_the_archive_then_reuses() { replacing an identically scoped one" ); } + +#[path = "native_relay_client_transport_tests.rs"] +mod transport_tests; diff --git a/desktop/src-tauri/src/native_relay_client_transport_tests.rs b/desktop/src-tauri/src/native_relay_client_transport_tests.rs new file mode 100644 index 00000000000..d064b521e52 --- /dev/null +++ b/desktop/src-tauri/src/native_relay_client_transport_tests.rs @@ -0,0 +1,169 @@ +//! Real persistent-WS receive loop -> real HTTP submit admission regressions. +use super::*; +use crate::relay_admission::{reset_rate_limit_gate, TEST_SERIAL}; +use axum::{routing::post, Json, Router}; + +async fn http_relay() -> ( + String, + mpsc::Receiver<(nostr::Event, std::time::Instant)>, + tokio::task::JoinHandle<()>, +) { + let (sent, received) = mpsc::channel(4); + let router = Router::new() + .route( + "/query", + post(|| async { + ( + axum::http::StatusCode::TOO_MANY_REQUESTS, + Json(serde_json::json!({"error": "rate-limited: quota exceeded; retry in 1s"})), + ) + }), + ) + .route( + "/events", + post(move |Json(event): Json| { + let sent = sent.clone(); + async move { + let received_at = std::time::Instant::now(); + let id = event.id.to_hex(); + assert!(event.verify().is_ok()); + sent.send((event, received_at)).await.unwrap(); + Json(serde_json::json!({"event_id": id, "accepted": true, "message": ""})) + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + axum::serve(listener, router).await.unwrap(); + }); + (format!("http://{address}"), received, server) +} + +fn reply(keys: &Keys) -> nostr::Event { + EventBuilder::new(nostr::Kind::Custom(9), "startup reply") + .tags([ + nostr::Tag::parse(["h", "5b130804-d759-40ad-a564-d64cc907fa8e"]).unwrap(), + nostr::Tag::parse(["e", &"a".repeat(64), "", "reply"]).unwrap(), + ]) + .sign_with_keys(keys) + .unwrap() +} + +async fn closed_then_http_submit(message: &str, shared_unavailable: bool) { + let _serial = TEST_SERIAL.lock().await; + reset_rate_limit_gate(); + let (ws_url, mut frames, commands) = stub_relay().await; + let keys = Keys::generate(); + let (session, mut events) = start(ws_url, keys.clone(), None).await; + session + .set_subscriptions(vec![ + probe_subscription(), + Subscription { + id: "barrier".into(), + filter: serde_json::json!({"kinds": [1], "limit": 0}), + }, + ]) + .await; + assert_eq!(next_req(&mut frames, "probe REQ").await, PROBE_ID); + assert_eq!(next_req(&mut frames, "barrier REQ").await, "barrier"); + commands + .send(StubCommand::Closed(PROBE_ID.into(), message.into())) + .await + .unwrap(); + // An ordered frame on an unaffected persistent subscription proves CLOSED + // went through the receive loop. No test-side gate activation or sleeps. + let barrier = EventBuilder::text_note("barrier") + .sign_with_keys(&keys) + .unwrap(); + commands + .send(StubCommand::Event( + "barrier".into(), + serde_json::to_value(barrier).unwrap(), + )) + .await + .unwrap(); + assert_eq!( + tokio::time::timeout(Duration::from_secs(3), events.recv()) + .await + .unwrap() + .unwrap() + .subscription_id, + "barrier" + ); + + let (http_url, mut submitted, server) = http_relay().await; + let state = crate::app_state::build_app_state(); + let event = reply(&keys); + let submit = crate::relay::submit_signed_event_at_with_keys(&event, &state, &http_url, &keys); + let outcome = tokio::time::timeout(Duration::from_secs(1), submit).await; + session.shutdown(); + server.abort(); + reset_rate_limit_gate(); + if shared_unavailable { + assert!( + outcome.is_err(), + "shared admission outage must still damp HTTP" + ); + assert!( + submitted.try_recv().is_err(), + "HTTP must not dispatch during the shared outage" + ); + } else { + let response = outcome + .expect("WS quota must not withhold HTTP submission") + .unwrap(); + assert!(response.accepted); + assert_eq!(response.event_id, event.id.to_hex()); + assert_eq!(submitted.recv().await.unwrap().0, event); + } + assert!( + frames.try_recv().is_err(), + "the limited WS subscription must not reopen early" + ); +} + +#[tokio::test] +async fn persistent_ws_quota_does_not_withhold_http_reply() { + closed_then_http_submit("rate-limited: quota exceeded; retry in 50s", false).await; +} + +#[tokio::test] +async fn persistent_ws_concurrency_does_not_withhold_http_reply() { + closed_then_http_submit("rate-limited: too many concurrent requests", false).await; +} + +#[tokio::test] +async fn persistent_ws_shared_unavailable_still_withholds_http_reply() { + closed_then_http_submit("rate-limited: shared admission unavailable", true).await; +} + +#[tokio::test] +async fn http_429_still_withholds_http_reply_then_accepts_it() { + let _serial = TEST_SERIAL.lock().await; + reset_rate_limit_gate(); + let (http_url, mut submitted, server) = http_relay().await; + let state = crate::app_state::build_app_state(); + let keys = Keys::generate(); + let event = reply(&keys); + let error = crate::relay::query_relay_at(&state, &http_url, &[serde_json::json!({"limit": 1})]) + .await + .unwrap_err(); + assert_eq!(error, "relay rate-limited: retry in 1s"); + let before = std::time::Instant::now(); + let outcome = tokio::time::timeout( + Duration::from_secs(3), + crate::relay::submit_signed_event_at_with_keys(&event, &state, &http_url, &keys), + ) + .await; + server.abort(); + reset_rate_limit_gate(); + let response = outcome.unwrap().unwrap(); + let (received, received_at) = submitted.recv().await.unwrap(); + assert!( + received_at.duration_since(before) >= Duration::from_millis(900), + "HTTP submit must honour its own cooldown" + ); + assert!(response.accepted); + assert_eq!(received, event); +} diff --git a/desktop/src-tauri/src/relay.rs b/desktop/src-tauri/src/relay.rs index 676b9656ff2..2484fc7d14c 100644 --- a/desktop/src-tauri/src/relay.rs +++ b/desktop/src-tauri/src/relay.rs @@ -316,7 +316,7 @@ pub async fn relay_error_message(response: reqwest::Response) -> String { }; // 429 Too Many Requests → typed `relay rate-limited:` prefix so the TS - // client can activate the rate-limit gate without confusing it with a + // client can report back-pressure without confusing it with a // connectivity failure (`relay unreachable:`). Also arm the Rust-side // admission gate here — the one place every relay HTTP error funnels // through — so the next relay-backed command waits out the quota window @@ -324,10 +324,8 @@ pub async fn relay_error_message(response: reqwest::Response) -> String { if status == reqwest::StatusCode::TOO_MANY_REQUESTS { let hint = extract_retry_in_hint(&body); // Clamp the hint to MAX_HINT_SECONDS before arming the Rust gate AND - // before embedding it in the returned string. Every consumer (Rust gate - // via `activate_rate_limit` and TS gate via `applyTauriRateLimitIfNeeded`) - // must see the same capped value — a single policy point prevents the TS - // gate from receiving an uncapped hint from an untrusted relay. + // before embedding it in the returned string, so the caller sees the + // same bounded hint the native HTTP gate actually honours. let capped_hint = hint.map(|s| s.min(crate::relay_admission::MAX_HINT_SECONDS)); crate::relay_admission::activate_rate_limit(capped_hint); if let Some(secs) = capped_hint { diff --git a/desktop/src-tauri/src/relay/tests.rs b/desktop/src-tauri/src/relay/tests.rs index 0fcbc891b79..f2928cbf612 100644 --- a/desktop/src-tauri/src/relay/tests.rs +++ b/desktop/src-tauri/src/relay/tests.rs @@ -45,7 +45,7 @@ fn overlong_digit_string_returns_none() { // // Verify that an oversized relay hint is capped in the returned message // string, not just inside `activate_rate_limit()`. This guarantees every -// consumer — including the TS gate via `applyTauriRateLimitIfNeeded` — +// consumer of the returned error — // receives the capped value rather than the raw untrusted relay value. #[tokio::test] diff --git a/desktop/src-tauri/src/relay_admission.rs b/desktop/src-tauri/src/relay_admission.rs index 4b0dd1f3696..29d6cdb309b 100644 --- a/desktop/src-tauri/src/relay_admission.rs +++ b/desktop/src-tauri/src/relay_admission.rs @@ -17,6 +17,10 @@ //! are driven by user-initiated file transfers rather than bridge event flow, //! and they have independent retry logic. //! +//! The native WS client also arms this gate for the explicit +//! `shared admission unavailable` signal. Quota/concurrency CLOSEDs stay on +//! WebSocket; they do not consume the HTTP bridge's separate ApiCalls budget. +//! //! **Community scope:** the gate is reset on every `apply_workspace` call, //! mirroring the TS gate's `resetRateLimitGate()` on community switch in //! `useCommunityInit.ts`. A 429 from community A cannot stall community B. @@ -36,8 +40,7 @@ const DEFAULT_RATE_LIMIT_SECONDS: u64 = 10; /// Prevents an untrusted relay from pinning traffic for an unreasonable window /// or overflowing `Instant` arithmetic. /// Exposed `pub` so `relay.rs` can clamp the hint before embedding it in the -/// returned error string — ensuring every consumer (Rust gate and TS gate via -/// `applyTauriRateLimitIfNeeded`) sees the same capped value. +/// returned error string, matching the window the native HTTP gate honours. pub const MAX_HINT_SECONDS: u64 = 300; static GATE_EXPIRY: Mutex> = Mutex::new(None); diff --git a/desktop/src/app/useTrayMenu.ts b/desktop/src/app/useTrayMenu.ts index 355c8e5d4f2..04ce9256a30 100644 --- a/desktop/src/app/useTrayMenu.ts +++ b/desktop/src/app/useTrayMenu.ts @@ -10,7 +10,7 @@ import { useManagedAgentsQuery, useRelayAgentsQuery, } from "@/features/agents/hooks"; -import { normalizePubkey, truncatePubkey } from "@/shared/lib/pubkey"; +import { normalizePubkey, truncateNpub } from "@/shared/lib/pubkey"; import { useNow } from "@/shared/lib/useNow"; import { formatElapsed } from "@/features/agents/ui/agentSessionUtils"; import type { Channel } from "@/shared/api/types"; @@ -72,7 +72,7 @@ export function useTrayMenu({ activityId: `${channelTurn.channelId}:${normalizePubkey(pubkey)}`, agentName: agentNames.get(normalizePubkey(pubkey)) ?? - `Agent ${truncatePubkey(pubkey)}`, + `Agent ${truncateNpub(pubkey)}`, channelId: channelTurn.channelId, channelName: channelNames.get(channelTurn.channelId) ?? "Unknown channel", diff --git a/desktop/src/features/agents/lib/respondToAllowlist.test.mjs b/desktop/src/features/agents/lib/respondToAllowlist.test.mjs index bbe07f72040..0d494652e74 100644 --- a/desktop/src/features/agents/lib/respondToAllowlist.test.mjs +++ b/desktop/src/features/agents/lib/respondToAllowlist.test.mjs @@ -6,6 +6,10 @@ import { mergeAllowlist, parsePubkeyInput } from "./respondToAllowlist.ts"; const HEX_A = "a".repeat(64); const HEX_B = "b".repeat(64); const HEX_A_UPPER = "A".repeat(64); +// Handoff vector, round-trip verified with nostr-tools nip19. +const HEX = "ea9b4d7a7a78a3e3729e5568b14d764d4962be0e1f20f749bcf8d9dbbf9a9328"; +const HEX_NPUB = + "npub1a2d567n60z37xu57245tzntkf4yk90swrus0wjdulrvah0u6jv5qusyp60"; test("parsePubkeyInput splits on commas, whitespace, and newlines", () => { const input = `${HEX_A}, ${HEX_B}\n${HEX_A_UPPER}`; @@ -25,11 +29,15 @@ test("parsePubkeyInput surfaces invalid entries separately", () => { assert.deepEqual(result.invalid, ["notgood", "z".repeat(64)]); }); -test("parsePubkeyInput rejects npub-style strings (hex only)", () => { - const npub = `npub1${"a".repeat(59)}`; - const result = parsePubkeyInput(npub); - assert.deepEqual(result.valid, []); - assert.deepEqual(result.invalid, [npub]); +test("parsePubkeyInput accepts npub entries, normalizes to hex, and dedupes across spellings", () => { + // One invalid npub-shaped token proves classification at this seam; the + // full codec negative matrix lives at the shared parser (nostrUtils). + const corrupt = `${HEX_NPUB.slice(0, -2)}qq`; + const result = parsePubkeyInput( + `${HEX_NPUB} ${HEX} ${HEX.toUpperCase()} ${corrupt}`, + ); + assert.deepEqual(result.valid, [HEX]); + assert.deepEqual(result.invalid, [corrupt]); }); test("parsePubkeyInput rejects wrong-length entries", () => { @@ -60,3 +68,8 @@ test("mergeAllowlist skips invalid additions silently", () => { const merged = mergeAllowlist([HEX_A], ["not-hex", HEX_B]); assert.deepEqual(merged, [HEX_A, HEX_B]); }); + +test("mergeAllowlist normalizes npub additions to canonical hex and dedupes", () => { + assert.deepEqual(mergeAllowlist([HEX_A], [HEX_NPUB]), [HEX_A, HEX]); + assert.deepEqual(mergeAllowlist([HEX], [HEX_NPUB]), [HEX]); +}); diff --git a/desktop/src/features/agents/lib/respondToAllowlist.ts b/desktop/src/features/agents/lib/respondToAllowlist.ts index c376aa1d1af..237a457cc30 100644 --- a/desktop/src/features/agents/lib/respondToAllowlist.ts +++ b/desktop/src/features/agents/lib/respondToAllowlist.ts @@ -5,9 +5,14 @@ * `desktop/src-tauri/src/managed_agents/types.rs::validate_respond_to_allowlist`). * These helpers exist to give the UI immediate, inline feedback before the * round-trip, and to normalize input so the Rust validator sees clean data. + * + * Entry pieces may be 64-char hex pubkeys or bech32 `npub1…` strings; both are + * normalized to the canonical lowercase hex via the shared + * `parsePubkeyInput`, so npub and hex spellings of the same key dedupe to one + * entry (users copy npubs from profile/verify surfaces elsewhere in the app). */ -const HEX_64 = /^[0-9a-f]{64}$/i; +import { parsePubkeyInput as parseCanonicalPubkey } from "@/shared/lib/nostrUtils"; export type ParsedAllowlist = { /** Successfully parsed entries — lowercase hex, deduplicated, in order. */ @@ -22,9 +27,9 @@ export type ParsedAllowlist = { * pattern used by `ChannelMemberInviteCard` so users have one mental model. * * - Splits on `/[\s,]+/`. - * - Trims and lowercases each entry. - * - Validates each entry is exactly 64 hex chars. - * - Deduplicates while preserving insertion order. + * - Accepts 64-char hex (any case) or `npub1…` bech32 per piece, normalizing + * to the canonical lowercase hex pubkey. + * - Deduplicates the canonical form while preserving insertion order. */ export function parsePubkeyInput(raw: string): ParsedAllowlist { const seen = new Set(); @@ -33,14 +38,14 @@ export function parsePubkeyInput(raw: string): ParsedAllowlist { for (const piece of raw.split(/[\s,]+/)) { const trimmed = piece.trim(); if (trimmed.length === 0) continue; - if (!HEX_64.test(trimmed)) { + const canonical = parseCanonicalPubkey(trimmed); + if (canonical === null) { invalid.push(trimmed); continue; } - const lower = trimmed.toLowerCase(); - if (!seen.has(lower)) { - seen.add(lower); - valid.push(lower); + if (!seen.has(canonical)) { + seen.add(canonical); + valid.push(canonical); } } return { valid, invalid }; @@ -48,16 +53,20 @@ export function parsePubkeyInput(raw: string): ParsedAllowlist { /** * Merge an existing allowlist with newly-added pubkeys, normalizing and - * deduplicating without reordering existing entries. + * deduplicating without reordering existing entries. Both hex and npub + * spellings normalize to the canonical hex, so the same key cannot enter + * twice regardless of the form it was added in. */ export function mergeAllowlist(existing: string[], add: string[]): string[] { - const seen = new Set(existing.map((p) => p.toLowerCase())); - const out = [...existing.map((p) => p.toLowerCase())]; + const normalize = (pubkey: string): string => + parseCanonicalPubkey(pubkey) ?? pubkey.toLowerCase(); + const out = existing.map(normalize); + const seen = new Set(out); for (const candidate of add) { - const lower = candidate.toLowerCase(); - if (!HEX_64.test(lower) || seen.has(lower)) continue; - seen.add(lower); - out.push(lower); + const canonical = parseCanonicalPubkey(candidate); + if (canonical === null || seen.has(canonical)) continue; + seen.add(canonical); + out.push(canonical); } return out; } diff --git a/desktop/src/features/agents/ui/AddAgentToChannelDialog.tsx b/desktop/src/features/agents/ui/AddAgentToChannelDialog.tsx index 2ce997a9f42..b5c804925a8 100644 --- a/desktop/src/features/agents/ui/AddAgentToChannelDialog.tsx +++ b/desktop/src/features/agents/ui/AddAgentToChannelDialog.tsx @@ -7,7 +7,11 @@ import { useChannelsQuery, } from "@/features/channels/hooks"; import type { Channel, ChannelRole, ManagedAgent } from "@/shared/api/types"; -import { normalizePubkey } from "@/shared/lib/pubkey"; +import { + canonicalNpub, + normalizePubkey, + UNAVAILABLE_KEY_LABEL, +} from "@/shared/lib/pubkey"; import { Button } from "@/shared/ui/button"; import { Dialog, @@ -85,6 +89,10 @@ export function AddAgentToChannelDialog({ ); }, [agent?.pubkey, membersQuery.data]); + // The agent's public key displays as its full canonical npub; an + // unencodable key renders the neutral label and is not copyable. + const agentNpub = agent?.pubkey ? canonicalNpub(agent.pubkey) : null; + const selectedChannel = channels.find((channel) => channel.id === channelId) ?? null; @@ -184,10 +192,12 @@ export function AddAgentToChannelDialog({

- {agent?.pubkey ?? "No agent selected"} + {agent + ? (agentNpub ?? UNAVAILABLE_KEY_LABEL) + : "No agent selected"} - {agent ? ( - + {agent && agentNpub ? ( + ) : null}
diff --git a/desktop/src/features/agents/ui/PersonaShareRecipients.tsx b/desktop/src/features/agents/ui/PersonaShareRecipients.tsx index 7db7c32bfa5..89588b57567 100644 --- a/desktop/src/features/agents/ui/PersonaShareRecipients.tsx +++ b/desktop/src/features/agents/ui/PersonaShareRecipients.tsx @@ -15,7 +15,7 @@ import { ProfileAvatar } from "@/features/profile/ui/ProfileAvatar"; import { SelectedRecipientChip } from "@/features/profile/ui/SelectedRecipientChip"; import { useIdentityQuery } from "@/shared/api/hooks"; import type { UserSearchResult } from "@/shared/api/types"; -import { normalizePubkey, truncatePubkey } from "@/shared/lib/pubkey"; +import { normalizePubkey, truncateNpub } from "@/shared/lib/pubkey"; import { Popover, PopoverAnchor, PopoverContent } from "@/shared/ui/popover"; import { Skeleton } from "@/shared/ui/skeleton"; @@ -25,7 +25,7 @@ export function formatShareRecipientName(user: UserSearchResult) { return ( user.displayName?.trim() || user.nip05Handle?.trim() || - truncatePubkey(user.pubkey) + truncateNpub(user.pubkey) ); } diff --git a/desktop/src/features/agents/ui/RespondToField.tsx b/desktop/src/features/agents/ui/RespondToField.tsx index 400f93af7c5..d910bfc8a6f 100644 --- a/desktop/src/features/agents/ui/RespondToField.tsx +++ b/desktop/src/features/agents/ui/RespondToField.tsx @@ -4,7 +4,8 @@ import { mergeAllowlist, parsePubkeyInput, } from "@/features/agents/lib/respondToAllowlist"; -import { truncatePubkey } from "@/shared/lib/pubkey"; +import { parsePubkeyInput as parseCanonicalPubkey } from "@/shared/lib/nostrUtils"; +import { truncateNpub } from "@/shared/lib/pubkey"; import { PubKey } from "@/shared/ui/PubKey"; import { useIsArchivedPredicate } from "@/features/identity-archive/hooks"; import { useUserSearchQuery } from "@/features/profile/hooks"; @@ -59,7 +60,7 @@ function formatSearchUserName(user: UserSearchResult) { return ( user.displayName?.trim() || user.nip05Handle?.trim() || - truncatePubkey(user.pubkey) + truncateNpub(user.pubkey) ); } @@ -69,7 +70,7 @@ function formatSearchUserSecondary(user: UserSearchResult) { if (displayName && nip05Handle) { return nip05Handle; } - return truncatePubkey(user.pubkey); + return truncateNpub(user.pubkey); } const RESPOND_TO_OPTIONS: PersonaDropdownOption[] = [ @@ -278,8 +279,6 @@ export function CreateAgentRespondToField({ ); } -const HEX_64_RE = /^[0-9a-f]{64}$/i; - function AllowlistPicker({ allowlist, deferredQuery, @@ -325,10 +324,12 @@ function AllowlistPicker({ }) { const isPersona = variant === "persona"; - // Detect if the query is a valid hex pubkey that's not already in the list. - const queryIsHexPubkey = - HEX_64_RE.test(deferredQuery) && - !allowlist.some((p) => p.toLowerCase() === deferredQuery.toLowerCase()); + // Detect if the query is a pubkey (npub or hex) not already in the list; + // direct entry offers the canonical hex for storage. + const queryPubkey = parseCanonicalPubkey(deferredQuery); + const queryIsDirectPubkey = + queryPubkey !== null && + !allowlist.some((p) => p.toLowerCase() === queryPubkey); return (
))}
- ) : queryIsHexPubkey ? ( + ) : queryIsDirectPubkey ? (