From 200123bcf933e1e2768f1b38e10b83e8ff42861a Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 8 Oct 2026 17:24:32 -0700 Subject: [PATCH 1/8] feat(transport): shared reqwest client factory and opt-in client attribution Add `transport::reqwest_client_builder()`, a preconfigured `reqwest::ClientBuilder` for code that needs a plain `reqwest::Client` (progenitor-generated clients, hand-rolled streaming/multipart) and cannot go through `HttpClient`. It applies the process-wide user-agent and default headers, a 5s connect timeout, and a 30s idle read timeout, so outbound policy is defined once in the engine instead of per call site. `HttpClient` now builds on the same factory. Add opt-in client attribution (`CliConfig::with_client_attribution`) so a CLI can tell the services it calls what kind of caller it is without any telemetry channel: `mode/` and `agent/` User-Agent tokens, plus a correlation header carrying a salted SHA-256 prefix of the harness session id (never the raw id). Detection uses the `is-ai-agent` crate. `_NO_SESSION_ID=1` drops the header. Off by default. See docs/attribution.md. The process-wide user-agent and the new default headers now share one lock (`ClientIdentity`) rather than adding a third global. New public API: `default_user_agent`, `set_default_headers`, `default_headers`, `DEFAULT_CONNECT_TIMEOUT`, `DEFAULT_READ_TIMEOUT`, `AttributionConfig`. `sha2` is no longer optional. Behavior change: `HttpClient` previously had no timeouts; it now has the connect/idle-read timeouts above. No total-request timeout is applied, so long streaming transfers are unaffected. Co-Authored-By: Claude Sonnet 5.5 --- Cargo.lock | 7 + cli-engine/Cargo.toml | 5 +- cli-engine/docs/attribution.md | 52 ++++ cli-engine/docs/concepts.md | 4 + cli-engine/src/cli/config.rs | 19 ++ cli-engine/src/cli/flags_apply.rs | 58 +++- cli-engine/src/cli/mod.rs | 28 +- cli-engine/src/cli/run.rs | 2 +- cli-engine/src/transport/attribution.rs | 230 +++++++++++++++ cli-engine/src/transport/attribution/tests.rs | 171 ++++++++++++ cli-engine/src/transport/client/factory.rs | 262 ++++++++++++++++++ cli-engine/src/transport/client/mod.rs | 83 ++++-- cli-engine/src/transport/mod.rs | 11 +- 13 files changed, 901 insertions(+), 31 deletions(-) create mode 100644 cli-engine/docs/attribution.md create mode 100644 cli-engine/src/transport/attribution.rs create mode 100644 cli-engine/src/transport/attribution/tests.rs create mode 100644 cli-engine/src/transport/client/factory.rs diff --git a/Cargo.lock b/Cargo.lock index d32c707..9f42023 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -452,6 +452,7 @@ dependencies = [ "clap_complete", "cli-engine-macros", "inquire", + "is-ai-agent", "jmespath", "jsonwebtoken", "keyring", @@ -1366,6 +1367,12 @@ version = "2.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d98f6fed1fde3f8c21bc40a1abb88dd75e67924f9cffc3ef95607bad8017f8e2" +[[package]] +name = "is-ai-agent" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce5562708736fdf9e8dcc50719e342b0ac0c3d1e365ed76bc0762d5a9e36094a" + [[package]] name = "is-docker" version = "0.2.0" diff --git a/cli-engine/Cargo.toml b/cli-engine/Cargo.toml index 0e8c24c..8329afc 100644 --- a/cli-engine/Cargo.toml +++ b/cli-engine/Cargo.toml @@ -22,12 +22,13 @@ jsonwebtoken = { version = "11", optional = true, default-features = false } keyring = { version = "3.6.1", optional = true, default-features = false } open = { version = "5.4.1", optional = true } rand = { version = "0.9", optional = true } -sha2 = { version = "0.10.9", optional = true } +sha2 = "0.10.9" url = { version = "2.5.4", optional = true } zeroize = { version = "1.8", optional = true, features = ["derive"] } chrono = { version = "0.4.42", default-features = false, features = ["clock", "serde"] } clap = { version = "4.5.53", features = ["derive", "std", "string"] } clap_complete = "4" +is-ai-agent = "0.6" jmespath = "0.5.0" reqwest = { version = "0.13", default-features = false, features = ["json", "multipart", "form", "rustls"] } regex = "1.12.2" @@ -53,7 +54,7 @@ keyring = { version = "3.6.1", optional = true, default-features = false, featur keyring = { version = "3.6.1", optional = true, default-features = false, features = ["windows-native"] } [features] -pkce-auth = ["dep:jsonwebtoken", "dep:keyring", "dep:open", "dep:rand", "dep:sha2", "dep:url", "dep:zeroize"] +pkce-auth = ["dep:jsonwebtoken", "dep:keyring", "dep:open", "dep:rand", "dep:url", "dep:zeroize"] [dev-dependencies] pretty_assertions = "1.4.1" diff --git a/cli-engine/docs/attribution.md b/cli-engine/docs/attribution.md new file mode 100644 index 0000000..971cb33 --- /dev/null +++ b/cli-engine/docs/attribution.md @@ -0,0 +1,52 @@ +# Client attribution + +Client attribution lets a CLI built on cli-engine tell the services it calls what kind of caller it is (a person at a terminal, a CI job, a script, or an AI coding agent) so usage can be understood without any telemetry channel. It adds a few tokens to the `User-Agent` header and, when an AI harness exposes a session id, one correlation header. It sends nothing to any endpoint the CLI was not already calling, writes nothing to disk, and never contacts a collector. + +It is off by default. A CLI opts in with `CliConfig::with_client_attribution`. + +## What is sent + +| Where | Value | When | +| --- | --- | --- | +| `User-Agent` | `/ mode/` | Always, once enabled | +| `User-Agent` | `agent/` (for example `agent/claude-code`) | A known AI harness is detected | +| `x-client-session` (name configurable) | 16 hex characters: a salted SHA-256 prefix of the harness session id | The harness exposes a session id, and the user has not opted out | + +`` is the first that applies: `agent` (a harness marker is present), `ci` (the `CI` variable is set to anything other than `0`, `false`, `no`, or `off`), `interactive` (stdin and stderr are terminals), or `script` (none of the above). + +An example, from a Claude Code session: `User-Agent: gddy/1.4.0 mode/agent agent/claude-code` and `x-gddy-session: 3f9a0c51d27be8a4`. + +## What is not sent + +The raw session id never leaves the process. Harness ids are often meaningful on the user's machine (transcript file names, resume handles), so only a one-way hash is sent. The hash is salted with the CLI's app id and the agent slug, so the same session hashes differently in different CLIs and cannot be joined across unrelated products. + +No persistent identifier is created. For a person at a terminal, or any caller without a harness session id, no session header is sent at all. Nothing is stored between runs. + +## Opting out + +Users can drop the session header with `_NO_SESSION_ID=1`, where `` is the CLI's app id uppercased with non-alphanumerics replaced by `_` (`gddy` becomes `GDDY_NO_SESSION_ID`). The user-agent tokens are not affected by this variable: they describe the kind of caller in the same way the binary name and version already do. CLI authors can also disable the header entirely with `AttributionConfig::without_session_id`. + +## Seeing exactly what goes out + +Run any command with `--debug=transport` to print each outbound request's headers to stderr, including the user-agent and the session header. + +## How detection works, and its limits + +Detection reads environment variables that AI harnesses publish to the processes they launch, using the [`is-ai-agent`](https://github.com/sdairs/is-ai-agent) crate. It is cooperative and heuristic. A match does not prove a model issued this particular command (a human can run commands in a terminal a harness opened), and no match does not prove a human did. Treat the result as attribution to a harness, not as proof of intent. Session ids have different scopes per harness (a conversation, a thread, a single run), so the hash correlates calls within one harness only. + +## Enabling it (CLI authors) + +```rust +use cli_engine::{BuildInfo, CliConfig}; +use cli_engine::transport::AttributionConfig; + +let config = CliConfig::new("my-cli", "Team CLI", "my-cli") + .with_build(BuildInfo::new(env!("CARGO_PKG_VERSION"))) + .with_client_attribution( + AttributionConfig::new().with_session_header("x-my-cli-session"), + ); +``` + +The engine resolves attribution once per execution, before any command runs, and publishes it process-wide. It is applied to every `HttpClient` and to every client built from `transport::reqwest_client_builder()` (the entry point for generated or hand-rolled `reqwest` clients). The user-agent tokens also reach the engine's own OAuth token requests; the session header does not. + +Because the headers are process-wide defaults, they are sent to whatever host those clients call. Build clients that talk to third-party hosts from a plain `reqwest::Client` if the session header should not reach them. diff --git a/cli-engine/docs/concepts.md b/cli-engine/docs/concepts.md index c8a52ea..d2e0330 100644 --- a/cli-engine/docs/concepts.md +++ b/cli-engine/docs/concepts.md @@ -783,6 +783,10 @@ The transport module provides a `reqwest`-based HTTP client with: Auth injectors include bearer token, provider bearer, cookie, basic auth, API key, client credentials, and no-op injectors. +Code that needs a plain `reqwest::Client` (progenitor-generated clients, hand-rolled streaming or multipart uploads) builds it from `transport::reqwest_client_builder()` instead of `reqwest::Client::builder()`. The builder comes preconfigured with the process-wide user-agent and default headers, a connect timeout, and an idle read timeout, so outbound policy is defined once in the engine. Callers layer their own settings on top. + +A CLI can opt into client attribution (user-agent tokens for the kind of caller, and a hashed harness session id) with `CliConfig::with_client_attribution`. See [attribution.md](attribution.md) for exactly what is sent, how to opt out, and the limits of detection. + ### HTTP debug logging The global `--debug` flag drives transport diagnostics through the `transport` component. Bare `--debug` enables every component; to select one, use the `=` form so the value is not mistaken for the command: `--debug=transport`, or `--debug='*,-transport'` to keep everything else but silence HTTP. (As an optional-value global flag, `--debug` only attaches a space-separated value when it appears after the leaf command; before the command, write `--debug=transport`.) `flags::debug_component_enabled` parses the comma-separated pattern. diff --git a/cli-engine/src/cli/config.rs b/cli-engine/src/cli/config.rs index 89e96f4..d734df3 100644 --- a/cli-engine/src/cli/config.rs +++ b/cli-engine/src/cli/config.rs @@ -134,6 +134,9 @@ pub struct CliConfig { /// the engine derives `name/version` from this config. See /// [`CliConfig::user_agent_string`]. pub user_agent: Option, + /// Opt-in client attribution (user-agent tokens and a hashed session + /// header). See [`CliConfig::with_client_attribution`]. + pub attribution: Option, /// Extra HTTP header names to redact in `--debug transport` output, on top /// of the built-in sensitive set (`authorization`, `proxy-authorization`, /// `cookie`, `set-cookie`, `x-api-key`). Set CLI-specific secret-bearing @@ -389,6 +392,22 @@ impl CliConfig { self } + /// Opts this CLI into client attribution. + /// + /// On execution the engine appends `mode/` + /// (and `agent/` for a detected AI harness) to the outbound + /// User-Agent, and, when the harness exposes a session id, sends a salted + /// hash of it in a correlation header. Nothing is sent to any endpoint the + /// CLI was not already calling. See `docs/attribution.md`. + #[must_use] + pub fn with_client_attribution( + mut self, + attribution: crate::transport::AttributionConfig, + ) -> Self { + self.attribution = Some(attribution); + self + } + /// Adds HTTP header names to redact in `--debug transport` output, on top of /// the built-in sensitive set. /// diff --git a/cli-engine/src/cli/flags_apply.rs b/cli-engine/src/cli/flags_apply.rs index 2be580c..5655b1f 100644 --- a/cli-engine/src/cli/flags_apply.rs +++ b/cli-engine/src/cli/flags_apply.rs @@ -412,6 +412,7 @@ pub(super) fn prescan_env_flag(mut args: impl Iterator) -> Option mod user_agent_tests { use super::*; use crate::cli::{BuildInfo, Cli, CliConfig}; + use crate::transport::Signals; #[test] fn user_agent_string_derives_name_and_version_by_default() { @@ -435,7 +436,7 @@ mod user_agent_tests { } #[test] - fn install_default_user_agent_publishes_config_value() { + fn install_client_identity_publishes_config_value() { let _guard = crate::transport::client::UA_TEST_LOCK .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); @@ -444,13 +445,66 @@ mod user_agent_tests { let cli = Cli::new( CliConfig::new("uatest", "UA test", "uatest").with_build(BuildInfo::new("4.5.6")), ); - cli.install_default_user_agent(); + cli.install_client_identity(); assert_eq!( crate::transport::client::default_user_agent(), "uatest/4.5.6" ); } + fn harness_signals(app_id: &str) -> Signals { + Signals::from_lookup( + app_id, + |name| match name { + "CLAUDECODE" => Some("1".to_owned()), + "CLAUDE_CODE_SESSION_ID" => Some("session-abc".to_owned()), + _ => None, + }, + |_| false, + false, + ) + } + + #[test] + fn client_identity_without_attribution_is_the_plain_user_agent() { + let cli = Cli::new( + CliConfig::new("attr", "Attr test", "attr").with_build(BuildInfo::new("1.0.0")), + ); + + let (user_agent, headers) = cli.client_identity(&harness_signals("attr")); + + assert_eq!(user_agent, "attr/1.0.0"); + assert!(headers.is_empty()); + } + + #[test] + fn client_identity_with_attribution_extends_the_base_user_agent() { + let cli = Cli::new( + CliConfig::new("attr", "Attr test", "attr") + .with_build(BuildInfo::new("1.0.0")) + .with_client_attribution(crate::transport::AttributionConfig::new()), + ); + + let (user_agent, headers) = cli.client_identity(&harness_signals("attr")); + + assert_eq!(user_agent, "attr/1.0.0 mode/agent agent/claude-code"); + assert_eq!(headers.len(), 1); + assert!(headers.contains_key("x-client-session")); + } + + #[test] + fn client_identity_extends_an_explicit_user_agent_override() { + let cli = Cli::new( + CliConfig::new("attr", "Attr test", "attr") + .with_user_agent("custom/9") + .with_client_attribution(crate::transport::AttributionConfig::new()), + ); + + let (user_agent, _) = cli.client_identity(&harness_signals("attr")); + + assert!(user_agent.starts_with("custom/9 mode/agent")); + } + #[test] fn install_debug_transport_logger_tracks_the_debug_pattern() { // Asserts on `debug_transport_logger_for`'s decision directly rather diff --git a/cli-engine/src/cli/mod.rs b/cli-engine/src/cli/mod.rs index 5f6b40c..d7c33e3 100644 --- a/cli-engine/src/cli/mod.rs +++ b/cli-engine/src/cli/mod.rs @@ -33,6 +33,7 @@ use crate::{ flags::{register_global_flags, register_reason_flag}, module::ModuleContext, output::{global_human_view_registry_snapshot, global_schema_registry_snapshot}, + transport::{Attribution, Signals}, }; pub use argv0::{Argv0LinkMethod, Argv0Route}; @@ -402,15 +403,32 @@ impl Cli { run::execute_from_until_signal(self, args, stdout, stderr, shutdown).await } - /// Publishes the configured outbound User-Agent process-wide so that - /// command [`HttpClient`](crate::transport::HttpClient)s and the engine's - /// own OAuth token requests share it. + /// Publishes the configured outbound identity process-wide so that + /// command [`HttpClient`](crate::transport::HttpClient)s, clients from + /// [`reqwest_client_builder`](crate::transport::reqwest_client_builder), + /// and the engine's own OAuth token requests share it. /// /// Called from the execution entrypoints rather than [`Cli::new`] so that /// merely constructing a `Cli` (as tests do in bulk) does not mutate global /// state. See [`CliConfig::user_agent_string`] for resolution order. - fn install_default_user_agent(&self) { - crate::transport::set_default_user_agent(self.config.user_agent_string()); + fn install_client_identity(&self) { + let (user_agent, headers) = + self.client_identity(&Signals::from_process(&self.config.app_id)); + crate::transport::set_default_user_agent(user_agent); + crate::transport::set_default_headers(headers); + } + + /// Computes the outbound User-Agent and default headers for `signals`: + /// the configured base user-agent, plus attribution tokens and headers + /// when the CLI opted in. + fn client_identity(&self, signals: &Signals) -> (String, BTreeMap) { + let mut user_agent = self.config.user_agent_string(); + let Some(config) = &self.config.attribution else { + return (user_agent, BTreeMap::new()); + }; + let attribution = Attribution::resolve(config, &self.config.app_id, signals); + user_agent.push_str(&attribution.user_agent_suffix); + (user_agent, attribution.headers) } /// Registers an auth provider after construction. diff --git a/cli-engine/src/cli/run.rs b/cli-engine/src/cli/run.rs index 71048cb..b772345 100644 --- a/cli-engine/src/cli/run.rs +++ b/cli-engine/src/cli/run.rs @@ -87,7 +87,7 @@ where E: Write, Shutdown: Future, { - cli.install_default_user_agent(); + cli.install_client_identity(); let output = run_until_signal(cli.run(args), shutdown).await; if output.exit_code == 130 && output.rendered == "command interrupted\n" diff --git a/cli-engine/src/transport/attribution.rs b/cli-engine/src/transport/attribution.rs new file mode 100644 index 0000000..d3119c6 --- /dev/null +++ b/cli-engine/src/transport/attribution.rs @@ -0,0 +1,230 @@ +//! Client attribution: how a CLI tells the services it calls what kind of +//! caller it is, without any telemetry channel of its own. +//! +//! Everything here rides on requests the user already asked for: +//! +//! - **User-Agent tokens** — `mode/` and, for a +//! detected AI harness, `agent/`. +//! - **A correlation header** — when the harness exposes a session id, a salted +//! hash of it (never the raw id) so a service can group one session's calls. +//! +//! Detection is cooperative and heuristic: it reads environment markers that +//! harnesses publish to their subprocesses. A match does not prove a model +//! issued the command, and no match does not prove a human did. + +use std::{collections::BTreeMap, path::Path}; + +use sha2::{Digest, Sha256}; + +use crate::flags::{app_id_env_prefix, detect_interactive}; + +const DEFAULT_SESSION_HEADER: &str = "x-client-session"; +const SESSION_HASH_BYTES: usize = 8; + +/// Opts a CLI into client attribution: `mode/` +/// and `agent/` tokens on the User-Agent, plus a correlation header +/// carrying a salted hash of the harness session id when one is available. +/// The raw session id is never sent. See `docs/attribution.md` for the full +/// user-facing description. +/// +/// Attribution is off unless a CLI calls +/// [`CliConfig::with_client_attribution`](crate::CliConfig::with_client_attribution). +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AttributionConfig { + session_header: String, + send_session: bool, +} + +impl Default for AttributionConfig { + fn default() -> Self { + Self { + session_header: DEFAULT_SESSION_HEADER.to_owned(), + send_session: true, + } + } +} + +impl AttributionConfig { + /// Creates the default configuration: user-agent tokens plus a hashed + /// session id in the `x-client-session` header when a harness supplies one. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + /// Sets the request header that carries the hashed session id, for services + /// that expect a product-specific name (for example `x-gddy-session`). + /// + /// An invalid header name disables the session header rather than failing + /// startup; the user-agent tokens are unaffected. + #[must_use] + pub fn with_session_header(mut self, name: impl Into) -> Self { + self.session_header = name.into().to_ascii_lowercase(); + self + } + + /// Never sends a session header, only the user-agent tokens. + #[must_use] + pub fn without_session_id(mut self) -> Self { + self.send_session = false; + self + } +} + +/// Coarse caller classification, in precedence order. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ClientMode { + /// A known AI harness marker is present. + Agent, + /// `CI` is set. + Ci, + /// A person at a terminal. + Interactive, + /// Neither: a script, cron job, or other unattended caller. + Script, +} + +impl ClientMode { + fn as_str(self) -> &'static str { + match self { + Self::Agent => "agent", + Self::Ci => "ci", + Self::Interactive => "interactive", + Self::Script => "script", + } + } +} + +/// A detected AI harness. +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct AgentSignal { + slug: &'static str, + session_id: Option, +} + +/// What the process environment says about the caller. +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct Signals { + agent: Option, + ci: bool, + interactive: bool, + session_opted_out: bool, +} + +impl Signals { + /// Reads the live process environment and terminal state. + pub(crate) fn from_process(app_id: &str) -> Self { + Self::from_lookup( + app_id, + |name| std::env::var(name).ok(), + |path| Path::new(path).exists(), + detect_interactive(), + ) + } + + /// Builds signals from injected lookups so tests never touch the real + /// environment. + pub(crate) fn from_lookup( + app_id: &str, + env: impl Fn(&str) -> Option, + file_exists: impl Fn(&str) -> bool, + interactive: bool, + ) -> Self { + let agent = is_ai_agent::detect_with(&env, file_exists).map(|agent| AgentSignal { + slug: agent.id.as_str(), + session_id: agent.session_id, + }); + let truthy = |name: &str| env(name).is_some_and(|value| is_truthy(&value)); + Self { + agent, + ci: truthy("CI"), + interactive, + session_opted_out: truthy(&format!("{}_NO_SESSION_ID", app_id_env_prefix(app_id))), + } + } + + fn mode(&self) -> ClientMode { + if self.agent.is_some() { + ClientMode::Agent + } else if self.ci { + ClientMode::Ci + } else if self.interactive { + ClientMode::Interactive + } else { + ClientMode::Script + } + } +} + +/// The resolved outbound identity additions. +#[derive(Clone, Debug, Default, Eq, PartialEq)] +pub(crate) struct Attribution { + /// Space-prefixed product tokens to append to the base user-agent. + pub(crate) user_agent_suffix: String, + /// Headers to send on every outbound request. + pub(crate) headers: BTreeMap, +} + +impl Attribution { + pub(crate) fn resolve(config: &AttributionConfig, app_id: &str, signals: &Signals) -> Self { + let mode = signals.mode(); + let mut user_agent_suffix = format!(" mode/{}", mode.as_str()); + if let Some(agent) = &signals.agent { + user_agent_suffix.push_str(" agent/"); + user_agent_suffix.push_str(agent.slug); + } + + let mut headers = BTreeMap::new(); + if config.send_session + && !signals.session_opted_out + && is_valid_header_name(&config.session_header) + && let Some(agent) = &signals.agent + && let Some(session_id) = &agent.session_id + { + headers.insert( + config.session_header.clone(), + hash_session(app_id, agent.slug, session_id), + ); + } + Self { + user_agent_suffix, + headers, + } + } +} + +/// Salted, truncated SHA-256 of the harness session id. +/// +/// Harness ids are often meaningful on the user's machine (transcript file +/// names, resume handles), so the raw value never leaves the process. The app +/// id and agent slug salt it so the same id hashes differently per CLI and the +/// result cannot be joined across unrelated products. +fn hash_session(app_id: &str, agent_slug: &str, session_id: &str) -> String { + let mut hasher = Sha256::new(); + for part in [app_id, agent_slug, session_id] { + hasher.update(part.as_bytes()); + hasher.update([0]); + } + hasher + .finalize() + .iter() + .take(SESSION_HASH_BYTES) + .map(|byte| format!("{byte:02x}")) + .collect() +} + +fn is_valid_header_name(name: &str) -> bool { + reqwest::header::HeaderName::from_bytes(name.as_bytes()).is_ok() +} + +/// Env-flag semantics: set, non-blank, and not an explicit "off" spelling. +fn is_truthy(value: &str) -> bool { + let value = value.trim(); + !value.is_empty() + && !["0", "false", "no", "off"] + .iter() + .any(|off| value.eq_ignore_ascii_case(off)) +} + +#[cfg(test)] +mod tests; diff --git a/cli-engine/src/transport/attribution/tests.rs b/cli-engine/src/transport/attribution/tests.rs new file mode 100644 index 0000000..13d4450 --- /dev/null +++ b/cli-engine/src/transport/attribution/tests.rs @@ -0,0 +1,171 @@ +use super::*; + +const SECRET_SESSION: &str = "0b7e9c52-raw-session-id"; + +fn signals(env: &[(&str, &str)], interactive: bool) -> Signals { + Signals::from_lookup( + "gddy", + |name| { + env.iter() + .find(|(key, _)| *key == name) + .map(|(_, value)| (*value).to_owned()) + }, + |_| false, + interactive, + ) +} + +fn claude_env() -> Vec<(&'static str, &'static str)> { + vec![ + ("CLAUDECODE", "1"), + ("CLAUDE_CODE_SESSION_ID", SECRET_SESSION), + ] +} + +fn resolve(signals: &Signals) -> Attribution { + Attribution::resolve(&AttributionConfig::new(), "gddy", signals) +} + +#[test] +fn detected_harness_adds_mode_and_agent_tokens() { + let attribution = resolve(&signals(&claude_env(), false)); + + assert_eq!( + attribution.user_agent_suffix, + " mode/agent agent/claude-code" + ); +} + +#[test] +fn session_header_carries_a_hash_never_the_raw_id() { + let attribution = resolve(&signals(&claude_env(), false)); + + let value = attribution + .headers + .get("x-client-session") + .expect("session header is sent"); + assert_eq!(value.len(), 16); + assert!(value.bytes().all(|byte| byte.is_ascii_hexdigit())); + assert!(!value.contains("raw-session")); + assert!(!attribution.user_agent_suffix.contains(SECRET_SESSION)); +} + +#[test] +fn session_hash_is_stable_and_salted_per_app() { + let same_a = hash_session("gddy", "claude-code", SECRET_SESSION); + let same_b = hash_session("gddy", "claude-code", SECRET_SESSION); + let other_app = hash_session("other-cli", "claude-code", SECRET_SESSION); + let other_session = hash_session("gddy", "claude-code", "different"); + + assert_eq!(same_a, same_b); + assert_ne!(same_a, other_app); + assert_ne!(same_a, other_session); +} + +#[test] +fn harness_without_a_session_id_sends_tokens_but_no_header() { + let attribution = resolve(&signals(&[("CLAUDECODE", "1")], false)); + + assert_eq!( + attribution.user_agent_suffix, + " mode/agent agent/claude-code" + ); + assert!(attribution.headers.is_empty()); +} + +#[test] +fn modes_follow_precedence_agent_ci_interactive_script() { + assert_eq!( + resolve(&signals(&[("CI", "true")], true)).user_agent_suffix, + " mode/ci" + ); + assert_eq!( + resolve(&signals(&[], true)).user_agent_suffix, + " mode/interactive" + ); + assert_eq!( + resolve(&signals(&[], false)).user_agent_suffix, + " mode/script" + ); + + let mut agent_in_ci = claude_env(); + agent_in_ci.push(("CI", "true")); + assert!( + resolve(&signals(&agent_in_ci, true)) + .user_agent_suffix + .starts_with(" mode/agent") + ); +} + +#[test] +fn explicit_off_spellings_do_not_count_as_ci() { + for value in ["", " ", "0", "false", "FALSE", "no", "off"] { + assert_eq!( + resolve(&signals(&[("CI", value)], false)).user_agent_suffix, + " mode/script", + "CI={value:?}" + ); + } +} + +#[test] +fn app_scoped_env_var_opts_out_of_the_session_header_only() { + let mut env = claude_env(); + env.push(("GDDY_NO_SESSION_ID", "1")); + + let attribution = resolve(&signals(&env, false)); + + assert!(attribution.headers.is_empty()); + assert_eq!( + attribution.user_agent_suffix, + " mode/agent agent/claude-code" + ); +} + +#[test] +fn opt_out_env_var_name_is_derived_from_the_app_id() { + let env = [ + ("CLAUDECODE", "1"), + ("CLAUDE_CODE_SESSION_ID", SECRET_SESSION), + ("MY_CLI_NO_SESSION_ID", "true"), + ]; + let lookup = |name: &str| { + env.iter() + .find(|(key, _)| *key == name) + .map(|(_, value)| (*value).to_owned()) + }; + + let signals = Signals::from_lookup("my-cli", lookup, |_| false, false); + + assert!(signals.session_opted_out); +} + +#[test] +fn config_can_disable_the_session_header() { + let config = AttributionConfig::new().without_session_id(); + + let attribution = Attribution::resolve(&config, "gddy", &signals(&claude_env(), false)); + + assert!(attribution.headers.is_empty()); + assert!(attribution.user_agent_suffix.contains("agent/claude-code")); +} + +#[test] +fn config_can_rename_the_session_header() { + let config = AttributionConfig::new().with_session_header("X-GDDY-Session"); + + let attribution = Attribution::resolve(&config, "gddy", &signals(&claude_env(), false)); + + assert!(attribution.headers.contains_key("x-gddy-session")); + assert!(!attribution.headers.contains_key("x-client-session")); +} + +#[test] +fn invalid_header_name_drops_the_header_but_keeps_the_tokens() { + let config = AttributionConfig::new().with_session_header("not a header"); + + let attribution = Attribution::resolve(&config, "gddy", &signals(&claude_env(), false)); + + assert!(attribution.headers.is_empty()); + assert!(attribution.user_agent_suffix.contains("mode/agent")); +} diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs new file mode 100644 index 0000000..4c8d8a5 --- /dev/null +++ b/cli-engine/src/transport/client/factory.rs @@ -0,0 +1,262 @@ +//! Shared `reqwest` client construction. +//! +//! Code that needs a plain [`reqwest::Client`] — progenitor-generated API +//! clients, hand-rolled multipart or streaming uploads, anything that cannot go +//! through [`super::HttpClient`] — builds it from [`reqwest_client_builder`] so +//! outbound policy (user-agent, timeouts, and future correlation headers) is +//! defined once in the engine instead of per call site. + +use std::time::Duration; + +use super::{default_headers, default_user_agent}; + +/// Connect timeout applied to every client from [`reqwest_client_builder`]. +pub const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(5); + +/// Idle read timeout applied to every client from [`reqwest_client_builder`]. +/// +/// This bounds the wait for each read (first response byte included), not the +/// whole request, so a large streaming download that keeps making progress is +/// never cut off. +pub const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(30); + +/// Returns a [`reqwest::ClientBuilder`] preconfigured with the engine's +/// outbound policy. +/// +/// - `User-Agent` is the process-wide default (see +/// [`set_default_user_agent`](super::set_default_user_agent)), read when this +/// function is called. +/// - The process-wide default headers (see +/// [`set_default_headers`](super::set_default_headers)) are sent on every +/// request, so attribution reaches every client built here. A later +/// `.default_headers(..)` on the returned builder adds to them; on a name +/// clash the later value wins. +/// - [`DEFAULT_CONNECT_TIMEOUT`] and [`DEFAULT_READ_TIMEOUT`] bound hangs. +/// There is deliberately no total-request timeout: callers that want one +/// (for example a short API call) set `.timeout(..)` on the returned builder, +/// and transfer-heavy callers keep the default. +/// +/// The builder is returned unbuilt so callers can layer their own settings on +/// top (default headers, redirect policy, a per-client user-agent override). +/// Later settings win. +pub fn reqwest_client_builder() -> reqwest::ClientBuilder { + reqwest::Client::builder() + .user_agent(default_user_agent()) + .default_headers(process_default_header_map()) + .connect_timeout(DEFAULT_CONNECT_TIMEOUT) + .read_timeout(DEFAULT_READ_TIMEOUT) +} + +/// The process-wide default headers as a `HeaderMap`, skipping any entry that +/// is not a valid header name or value rather than failing client construction. +fn process_default_header_map() -> reqwest::header::HeaderMap { + default_headers() + .iter() + .filter_map(|(name, value)| { + Some(( + reqwest::header::HeaderName::from_bytes(name.as_bytes()).ok()?, + reqwest::header::HeaderValue::from_str(value).ok()?, + )) + }) + .collect() +} + +/// Builds a client from [`reqwest_client_builder`], falling back to a bare +/// client if the TLS backend fails to initialize. +pub(super) fn build_default_client() -> reqwest::Client { + reqwest_client_builder() + .build() + .unwrap_or_else(|_| reqwest::Client::new()) +} + +#[cfg(test)] +mod tests { + use std::{ + io::{Read, Write}, + net::TcpListener, + }; + + use super::*; + use crate::transport::client::{RestoreDefaultUserAgent, UA_TEST_LOCK}; + + /// Serves one request on loopback and returns the raw request head. + fn serve_once() -> (String, std::thread::JoinHandle) { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind loopback"); + let url = format!("http://{}/", listener.local_addr().expect("local addr")); + let handle = std::thread::spawn(move || { + let (mut stream, _) = listener.accept().expect("accept"); + let mut buf = [0_u8; 4096]; + let read = stream.read(&mut buf).expect("read request"); + stream + .write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n") + .expect("write response"); + String::from_utf8_lossy(&buf[..read]).into_owned() + }); + (url, handle) + } + + /// Builds a client while the process-default user-agent is `default_ua`. + /// + /// The user-agent is captured when the builder is created, so the global + /// lock is released before any `.await` in the caller. + fn build_with_default_ua( + default_ua: &str, + configure: impl FnOnce(reqwest::ClientBuilder) -> reqwest::ClientBuilder, + ) -> reqwest::Client { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_user_agent(default_ua); + configure(reqwest_client_builder()) + .build() + .expect("client builds") + } + + /// Builds a client while the process defaults are `ua` and `headers`. + fn build_with_defaults( + ua: &str, + headers: &[(&str, &str)], + configure: impl FnOnce(reqwest::ClientBuilder) -> reqwest::ClientBuilder, + ) -> reqwest::Client { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_user_agent(ua); + super::super::set_default_headers( + headers + .iter() + .map(|(name, value)| ((*name).to_owned(), (*value).to_owned())) + .collect(), + ); + configure(reqwest_client_builder()) + .build() + .expect("client builds") + } + + #[tokio::test] + async fn default_headers_reach_the_wire() { + let client = build_with_defaults("probe/1", &[("x-client-session", "abc123")], |b| b); + let (url, server) = serve_once(); + + client.get(&url).send().await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("x-client-session: abc123"), "{head}"); + } + + /// Generated clients add `Authorization` via `.default_headers(..)` on this + /// builder; that must add to, not replace, the process defaults. + #[tokio::test] + async fn later_default_headers_add_to_process_defaults() { + let client = build_with_defaults("probe/1", &[("x-client-session", "abc123")], |b| { + let mut extra = reqwest::header::HeaderMap::new(); + extra.insert( + reqwest::header::AUTHORIZATION, + reqwest::header::HeaderValue::from_static("Bearer tok"), + ); + b.default_headers(extra) + }); + let (url, server) = serve_once(); + + client.get(&url).send().await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("x-client-session: abc123"), "{head}"); + assert!(head.contains("authorization: bearer tok"), "{head}"); + } + + /// `HttpClient` layers its own headers over the process defaults: both are + /// sent, and the client's value wins on a name clash. + #[tokio::test] + async fn http_client_merges_process_defaults_under_its_own_headers() { + let (url, server) = serve_once(); + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_headers( + [("x-client-session", "process"), ("x-process-only", "1")] + .into_iter() + .map(|(name, value)| (name.to_owned(), value.to_owned())) + .collect(), + ); + crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ) + .default_headers( + [("x-client-session", "client".to_owned())] + .into_iter() + .map(|(name, value)| (name.to_owned(), value)) + .collect(), + ) + .build() + }; + + client.get_bytes("/").await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("x-client-session: client"), "{head}"); + assert!(!head.contains("x-client-session: process"), "{head}"); + assert!(head.contains("x-process-only: 1"), "{head}"); + } + + #[tokio::test] + async fn invalid_default_header_is_skipped_not_fatal() { + let client = build_with_defaults("probe/1", &[("not a header", "x"), ("x-ok", "1")], |b| b); + let (url, server) = serve_once(); + + client.get(&url).send().await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("x-ok: 1"), "{head}"); + } + + #[tokio::test] + async fn builder_sends_the_process_default_user_agent() { + let client = build_with_default_ua("factory-probe/1.2", |builder| builder); + let (url, server) = serve_once(); + + client.get(&url).send().await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("user-agent: factory-probe/1.2"), "{head}"); + } + + #[tokio::test] + async fn caller_can_override_the_user_agent() { + let client = build_with_default_ua("factory-probe/1.2", |builder| { + builder.user_agent("override/9") + }); + let (url, server) = serve_once(); + + client.get(&url).send().await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("user-agent: override/9"), "{head}"); + } + + #[tokio::test] + async fn stalled_server_hits_the_read_timeout_not_a_hang() { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind loopback"); + let url = format!("http://{}/", listener.local_addr().expect("local addr")); + // Accept and never respond; keep the socket open until the client gives up. + let server = std::thread::spawn(move || { + let (stream, _) = listener.accept().expect("accept"); + std::thread::sleep(Duration::from_millis(400)); + drop(stream); + }); + let client = reqwest_client_builder() + .read_timeout(Duration::from_millis(100)) + .build() + .expect("client builds"); + + let error = client.get(&url).send().await.expect_err("must time out"); + + assert!(error.is_timeout(), "{error}"); + server.join().expect("server thread"); + } +} diff --git a/cli-engine/src/transport/client/mod.rs b/cli-engine/src/transport/client/mod.rs index 7d34c41..023a646 100644 --- a/cli-engine/src/transport/client/mod.rs +++ b/cli-engine/src/transport/client/mod.rs @@ -9,12 +9,33 @@ use serde_json::Value; use super::{AuthInjector, Error}; +mod factory; mod methods; +pub use factory::{DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT, reqwest_client_builder}; + const MAX_RETRIES: usize = 3; const BASE_BACKOFF: Duration = Duration::from_millis(500); const BUILTIN_DEFAULT_USER_AGENT: &str = "cli/dev"; -static DEFAULT_USER_AGENT: OnceLock> = OnceLock::new(); + +/// Process-wide outbound identity: the user-agent and any headers every +/// client should carry. One lock covers both so they are published together. +#[derive(Clone, Debug)] +struct ClientIdentity { + user_agent: String, + headers: BTreeMap, +} + +static CLIENT_IDENTITY: OnceLock> = OnceLock::new(); + +fn client_identity() -> &'static RwLock { + CLIENT_IDENTITY.get_or_init(|| { + RwLock::new(ClientIdentity { + user_agent: BUILTIN_DEFAULT_USER_AGENT.to_owned(), + headers: BTreeMap::new(), + }) + }) +} /// Sets the process-wide default user-agent for outbound requests. /// @@ -24,10 +45,8 @@ static DEFAULT_USER_AGENT: OnceLock> = OnceLock::new(); /// token/refresh requests and the client-credentials injector. A per-client /// user-agent still overrides it for that client. pub fn set_default_user_agent(user_agent: impl Into) { - let lock = - DEFAULT_USER_AGENT.get_or_init(|| RwLock::new(BUILTIN_DEFAULT_USER_AGENT.to_owned())); - if let Ok(mut current) = lock.write() { - *current = user_agent.into(); + if let Ok(mut current) = client_identity().write() { + current.user_agent = user_agent.into(); } } @@ -36,14 +55,33 @@ pub fn set_default_user_agent(user_agent: impl Into) { /// /// Used by [`HttpClientBuilder`] and by the engine's OAuth token requests so /// that all outbound traffic carries the same user-agent. -pub(crate) fn default_user_agent() -> String { - DEFAULT_USER_AGENT - .get_or_init(|| RwLock::new(BUILTIN_DEFAULT_USER_AGENT.to_owned())) +#[must_use] +pub fn default_user_agent() -> String { + client_identity().read().map_or_else( + |_| BUILTIN_DEFAULT_USER_AGENT.to_owned(), + |identity| identity.user_agent.clone(), + ) +} + +/// Sets the process-wide default headers sent on requests from +/// [`reqwest_client_builder`] and [`HttpClientBuilder`]. +/// +/// A client's own headers (including auth) win over these on a name clash. +/// Unlike the user-agent, these are not applied to the engine's OAuth token +/// requests. +pub fn set_default_headers(headers: BTreeMap) { + if let Ok(mut current) = client_identity().write() { + current.headers = headers; + } +} + +/// Returns the process-wide default headers set via [`set_default_headers`]. +#[must_use] +pub fn default_headers() -> BTreeMap { + client_identity() .read() - .map_or_else( - |_| BUILTIN_DEFAULT_USER_AGENT.to_owned(), - |value| value.clone(), - ) + .map(|identity| identity.headers.clone()) + .unwrap_or_default() } /// Serializes unit tests that mutate the process-wide default user-agent so @@ -52,10 +90,10 @@ pub(crate) fn default_user_agent() -> String { #[cfg(test)] pub(crate) static UA_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); -/// Restores the process-wide default user-agent to the builtin on drop, so a -/// panicking assertion in a test that mutates it cannot leak the value into -/// later tests in this binary. Declare it after acquiring [`UA_TEST_LOCK`] so -/// the reset runs while the lock is still held. +/// Restores the process-wide default user-agent and headers to the builtins on +/// drop, so a panicking assertion in a test that mutates them cannot leak the +/// values into later tests in this binary. Declare it after acquiring +/// [`UA_TEST_LOCK`] so the reset runs while the lock is still held. #[cfg(test)] pub(crate) struct RestoreDefaultUserAgent; @@ -63,6 +101,7 @@ pub(crate) struct RestoreDefaultUserAgent; impl Drop for RestoreDefaultUserAgent { fn drop(&mut self) { set_default_user_agent(BUILTIN_DEFAULT_USER_AGENT); + set_default_headers(BTreeMap::new()); } } @@ -294,16 +333,24 @@ impl HttpClientBuilder { #[must_use] pub fn build(self) -> HttpClient { HttpClient { - base: reqwest::Client::new(), + base: factory::build_default_client(), base_url: self.base_url, auth: self.auth, user_agent: self.user_agent, - default_headers: self.default_headers, + default_headers: merged_default_headers(self.default_headers), logger: self.logger, } } } +/// Layers the client's own headers over the process-wide defaults so a +/// per-client header wins on a name clash. +fn merged_default_headers(own: BTreeMap) -> BTreeMap { + let mut headers = default_headers(); + headers.extend(own); + headers +} + /// Converts a `reqwest` header map into owned name/value pairs for logging. /// /// Header values that are not valid UTF-8 are rendered as a byte-count diff --git a/cli-engine/src/transport/mod.rs b/cli-engine/src/transport/mod.rs index 7b73d3b..59ab775 100644 --- a/cli-engine/src/transport/mod.rs +++ b/cli-engine/src/transport/mod.rs @@ -12,16 +12,21 @@ use serde::{Deserialize, Serialize}; use crate::DetailedError; +mod attribution; /// HTTP client implementation. pub mod client; mod debug_logger; /// Request auth injectors. pub mod injector; +pub use attribution::AttributionConfig; +pub(crate) use attribution::{Attribution, Signals}; pub use client::{ - HttpClient, HttpClientBuilder, NoopTransportLogger, TransportLogEvent, TransportLogger, - debug_log_reqwest_request, debug_log_reqwest_response, default_transport_logger, - set_default_transport_logger, set_default_user_agent, + DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT, HttpClient, HttpClientBuilder, + NoopTransportLogger, TransportLogEvent, TransportLogger, debug_log_reqwest_request, + debug_log_reqwest_response, default_headers, default_transport_logger, default_user_agent, + reqwest_client_builder, set_default_headers, set_default_transport_logger, + set_default_user_agent, }; pub use debug_logger::StderrTransportLogger; pub use injector::{ From ad846efa806ec794515640f816a9d48a05f39d4d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 8 Oct 2026 17:38:38 -0700 Subject: [PATCH 2/8] fix(transport): publish client identity atomically, validate default headers, honor argv0 personalities Address review feedback on the client attribution change: - The user-agent and default headers were stored under one lock but written and read in separate acquisitions, so a concurrent reader could pair one publish's user-agent with another's headers. Add an atomic `set_client_identity` and a single-lock `client_identity_snapshot`, and use the snapshot in `reqwest_client_builder` and `HttpClientBuilder::new`. - `set_default_headers` accepted arbitrary strings, and `HttpClient` merged them raw into every request, so one invalid entry could fail every request at construction time. Entries with an invalid name or value are now dropped (and their names logged) when published, so every consumer sees only sendable headers. - An argv0 personality runs an independent `Cli` with its own config, but only the dispatcher's identity was published. The execute path now publishes after argv0 resolution and forwards the flag into the personality's run, so the CLI that actually executes publishes its own identity. `Cli::run` still does not publish (tests call it concurrently); this is now documented on `Cli::run` and in docs/attribution.md. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/docs/attribution.md | 2 +- cli-engine/src/cli/argv0.rs | 19 +++- cli-engine/src/cli/mod.rs | 11 ++- cli-engine/src/cli/run.rs | 24 ++++- cli-engine/src/transport/client/factory.rs | 107 +++++++++++++++++++-- cli-engine/src/transport/client/mod.rs | 58 +++++++++-- cli-engine/tests/foundation.rs | 41 ++++++++ 7 files changed, 237 insertions(+), 25 deletions(-) diff --git a/cli-engine/docs/attribution.md b/cli-engine/docs/attribution.md index 971cb33..87e27e5 100644 --- a/cli-engine/docs/attribution.md +++ b/cli-engine/docs/attribution.md @@ -47,6 +47,6 @@ let config = CliConfig::new("my-cli", "Team CLI", "my-cli") ); ``` -The engine resolves attribution once per execution, before any command runs, and publishes it process-wide. It is applied to every `HttpClient` and to every client built from `transport::reqwest_client_builder()` (the entry point for generated or hand-rolled `reqwest` clients). The user-agent tokens also reach the engine's own OAuth token requests; the session header does not. +The engine resolves attribution once per execution, before any command runs, and publishes it process-wide. Publishing happens in the `execute*` entrypoints, after argv0 resolution, so an argv0 personality publishes its own identity and not the dispatcher's. `Cli::run` deliberately does not publish (so running a `Cli` in tests never mutates process-wide state); a harness that drives `Cli::run` and needs outbound requests to carry the identity must publish it itself. The user-agent and default headers are published and read together, so a client never sees one without the other. It is applied to every `HttpClient` and to every client built from `transport::reqwest_client_builder()` (the entry point for generated or hand-rolled `reqwest` clients). The user-agent tokens also reach the engine's own OAuth token requests; the session header does not. Because the headers are process-wide defaults, they are sent to whatever host those clients call. Build clients that talk to third-party hosts from a plain `reqwest::Client` if the session header should not reach them. diff --git a/cli-engine/src/cli/argv0.rs b/cli-engine/src/cli/argv0.rs index ab39327..59fb89d 100644 --- a/cli-engine/src/cli/argv0.rs +++ b/cli-engine/src/cli/argv0.rs @@ -89,8 +89,15 @@ pub(super) enum Argv0Outcome { /// with a fully rendered result when a personality ran or an explicit `argv0` /// invocation was rejected. When no routes are registered this is inert and /// returns the arguments unchanged. `depth` counts chained hand-offs and -/// bounds recursion via [`MAX_ARGV0_DEPTH`]. -pub(super) async fn resolve_argv0(cli: &Cli, text_args: Vec, depth: usize) -> Argv0Outcome { +/// bounds recursion via [`MAX_ARGV0_DEPTH`]. `publish_identity` is forwarded to a +/// personality's own run so the CLI that actually executes publishes its +/// outbound identity, not the dispatcher's. +pub(super) async fn resolve_argv0( + cli: &Cli, + text_args: Vec, + depth: usize, + publish_identity: bool, +) -> Argv0Outcome { if cli.config.argv0_routes.is_empty() { return Argv0Outcome::Proceed(text_args); } @@ -162,7 +169,13 @@ pub(super) async fn resolve_argv0(cli: &Cli, text_args: Vec, depth: usiz alt_args.push(bin); alt_args.extend(rest); Argv0Outcome::Handled( - Box::pin(super::run::run_with_depth(&alt, alt_args, depth + 1)).await, + Box::pin(super::run::run_with_depth( + &alt, + alt_args, + depth + 1, + publish_identity, + )) + .await, ) } None if explicit => Argv0Outcome::Handled(render_argv0_error( diff --git a/cli-engine/src/cli/mod.rs b/cli-engine/src/cli/mod.rs index d7c33e3..627ce36 100644 --- a/cli-engine/src/cli/mod.rs +++ b/cli-engine/src/cli/mod.rs @@ -414,8 +414,7 @@ impl Cli { fn install_client_identity(&self) { let (user_agent, headers) = self.client_identity(&Signals::from_process(&self.config.app_id)); - crate::transport::set_default_user_agent(user_agent); - crate::transport::set_default_headers(headers); + crate::transport::client::set_client_identity(user_agent, headers); } /// Computes the outbound User-Agent and default headers for `signals`: @@ -579,11 +578,17 @@ impl Cli { /// /// Same `--env`/tree-pruning caveat as [`Cli::execute_from`]: see /// [`CliConfig::with_startup_args`]. + /// + /// Unlike the `execute*` entrypoints, this does not publish the process-wide + /// outbound identity (user-agent, default headers, client attribution), so + /// merely running a `Cli` never mutates global state. Use an `execute*` + /// entrypoint, or `transport::set_default_user_agent`, when outbound + /// requests must carry the configured identity. pub async fn run(&self, args: I) -> CliRunOutput where I: IntoIterator, S: Into + Clone, { - run::run_with_depth(self, args, 0).await + run::run_with_depth(self, args, 0, false).await } } diff --git a/cli-engine/src/cli/run.rs b/cli-engine/src/cli/run.rs index b772345..d2e1a9d 100644 --- a/cli-engine/src/cli/run.rs +++ b/cli-engine/src/cli/run.rs @@ -87,8 +87,7 @@ where E: Write, Shutdown: Future, { - cli.install_client_identity(); - let output = run_until_signal(cli.run(args), shutdown).await; + let output = run_until_signal(run_with_depth(cli, args, 0, true), shutdown).await; if output.exit_code == 130 && output.rendered == "command interrupted\n" && let Some(on_shutdown) = &cli.on_shutdown @@ -105,7 +104,17 @@ where /// Runs the CLI like [`Cli::run`](super::Cli::run), threading the `argv0` dispatch recursion /// `depth` so a chain of personality hand-offs is bounded by [`MAX_ARGV0_DEPTH`](super::argv0::MAX_ARGV0_DEPTH). -pub(super) async fn run_with_depth(cli: &Cli, args: I, depth: usize) -> CliRunOutput +/// `publish_identity` installs the outbound identity process-wide once it is +/// known which CLI will actually run (after argv0 resolution), so a personality +/// hand-off publishes the personality's identity. Only the `execute*` +/// entrypoints set it: plain [`Cli::run`](super::Cli::run) must not mutate +/// process globals, since tests call it concurrently. +pub(super) async fn run_with_depth( + cli: &Cli, + args: I, + depth: usize, + publish_identity: bool, +) -> CliRunOutput where I: IntoIterator, S: Into + Clone, @@ -118,10 +127,14 @@ where .iter() .map(|arg| arg.to_string_lossy().into_owned()) .collect::>(); - let text_args = match super::argv0::resolve_argv0(cli, text_args, depth).await { + let text_args = match super::argv0::resolve_argv0(cli, text_args, depth, publish_identity).await + { Argv0Outcome::Handled(output) => return output, Argv0Outcome::Proceed(args) => args, }; + if publish_identity { + cli.install_client_identity(); + } let mut clap_args = normalize_optional_global_flags_before_command(&cli.root, &text_args); if has_root_version_flag(&text_args, &cli.root, &cli.config.name) { return finish_run( @@ -413,7 +426,8 @@ where &bool_flags, &value_flags, ); - return Box::pin(run_with_depth(cli, augmented, depth + 1)).await; + // Same CLI, identity already published above. + return Box::pin(run_with_depth(cli, augmented, depth + 1, false)).await; } return finish_run( cli, diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs index 4c8d8a5..f8d06a7 100644 --- a/cli-engine/src/transport/client/factory.rs +++ b/cli-engine/src/transport/client/factory.rs @@ -8,7 +8,7 @@ use std::time::Duration; -use super::{default_headers, default_user_agent}; +use super::client_identity_snapshot; /// Connect timeout applied to every client from [`reqwest_client_builder`]. pub const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(5); @@ -40,17 +40,19 @@ pub const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(30); /// top (default headers, redirect policy, a per-client user-agent override). /// Later settings win. pub fn reqwest_client_builder() -> reqwest::ClientBuilder { + let (user_agent, headers) = client_identity_snapshot(); reqwest::Client::builder() - .user_agent(default_user_agent()) - .default_headers(process_default_header_map()) + .user_agent(user_agent) + .default_headers(header_map(&headers)) .connect_timeout(DEFAULT_CONNECT_TIMEOUT) .read_timeout(DEFAULT_READ_TIMEOUT) } -/// The process-wide default headers as a `HeaderMap`, skipping any entry that -/// is not a valid header name or value rather than failing client construction. -fn process_default_header_map() -> reqwest::header::HeaderMap { - default_headers() +/// Converts default headers to a `HeaderMap`. Published defaults are already +/// validated; an entry that still fails to convert is skipped rather than +/// failing client construction. +fn header_map(headers: &std::collections::BTreeMap) -> reqwest::header::HeaderMap { + headers .iter() .filter_map(|(name, value)| { Some(( @@ -204,6 +206,97 @@ mod tests { assert!(head.contains("x-process-only: 1"), "{head}"); } + /// The user-agent and default headers are published and read as a pair, so + /// a concurrent reader can never see one publish's user-agent with + /// another's headers. + #[test] + fn identity_pair_is_never_torn_across_concurrent_publishes() { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + let writers: Vec<_> = (0..4) + .map(|id| { + std::thread::spawn(move || { + for _ in 0..2_000 { + let tag = format!("app-{id}"); + super::super::set_client_identity( + tag.clone(), + std::collections::BTreeMap::from([("x-app".to_owned(), tag)]), + ); + } + }) + }) + .collect(); + let reader = std::thread::spawn(|| { + for _ in 0..20_000 { + let (user_agent, headers) = client_identity_snapshot(); + if let Some(app) = headers.get("x-app") { + assert_eq!(&user_agent, app, "torn identity: {user_agent} / {app}"); + } + } + }); + + for writer in writers { + writer.join().expect("writer thread"); + } + reader.join().expect("reader saw only consistent pairs"); + } + + #[test] + fn set_default_headers_drops_invalid_entries_at_publish_time() { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + + super::super::set_default_headers( + [ + ("not a header", "x"), + ("x-bad-value", "line\nbreak"), + ("x-ok", "1"), + ] + .into_iter() + .map(|(name, value)| (name.to_owned(), value.to_owned())) + .collect(), + ); + + let kept = super::super::default_headers(); + assert_eq!(kept.keys().collect::>(), vec!["x-ok"]); + } + + /// An invalid process default must not make `HttpClient` requests fail at + /// request-construction time. + #[tokio::test] + async fn http_client_requests_survive_an_invalid_process_default_header() { + let (url, server) = serve_once(); + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_headers( + [("not a header", "x"), ("x-ok", "1")] + .into_iter() + .map(|(name, value)| (name.to_owned(), value.to_owned())) + .collect(), + ); + crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ) + .build() + }; + + client + .get_bytes("/") + .await + .expect("request is built despite the invalid default"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("x-ok: 1"), "{head}"); + } + #[tokio::test] async fn invalid_default_header_is_skipped_not_fatal() { let client = build_with_defaults("probe/1", &[("not a header", "x"), ("x-ok", "1")], |b| b); diff --git a/cli-engine/src/transport/client/mod.rs b/cli-engine/src/transport/client/mod.rs index 023a646..084d0df 100644 --- a/cli-engine/src/transport/client/mod.rs +++ b/cli-engine/src/transport/client/mod.rs @@ -69,12 +69,52 @@ pub fn default_user_agent() -> String { /// A client's own headers (including auth) win over these on a name clash. /// Unlike the user-agent, these are not applied to the engine's OAuth token /// requests. +/// +/// An entry whose name or value is not a valid HTTP header is dropped here +/// (and its name logged), so every consumer of the defaults sees only headers +/// that can actually be sent. pub fn set_default_headers(headers: BTreeMap) { + let headers = valid_headers(headers); + if let Ok(mut current) = client_identity().write() { + current.headers = headers; + } +} + +/// Replaces the user-agent and default headers under one lock acquisition, so +/// a concurrent reader never observes one app's user-agent with another app's +/// headers. +pub(crate) fn set_client_identity(user_agent: String, headers: BTreeMap) { + let headers = valid_headers(headers); if let Ok(mut current) = client_identity().write() { + current.user_agent = user_agent; current.headers = headers; } } +/// Reads the user-agent and default headers under one lock acquisition. Client +/// constructors use this so the pair they apply is always self-consistent. +pub(crate) fn client_identity_snapshot() -> (String, BTreeMap) { + client_identity().read().map_or_else( + |_| (BUILTIN_DEFAULT_USER_AGENT.to_owned(), BTreeMap::new()), + |identity| (identity.user_agent.clone(), identity.headers.clone()), + ) +} + +/// Keeps only entries that are valid HTTP header names with valid values. +fn valid_headers(headers: BTreeMap) -> BTreeMap { + headers + .into_iter() + .filter(|(name, value)| { + let valid = header::HeaderName::from_bytes(name.as_bytes()).is_ok() + && header::HeaderValue::from_str(value).is_ok(); + if !valid { + tracing::warn!(header = %name, "ignoring invalid default header"); + } + valid + }) + .collect() +} + /// Returns the process-wide default headers set via [`set_default_headers`]. #[must_use] pub fn default_headers() -> BTreeMap { @@ -273,6 +313,8 @@ pub struct HttpClientBuilder { base_url: String, auth: Arc, user_agent: String, + /// Process-wide default headers captured with `user_agent` in one snapshot. + process_headers: BTreeMap, default_headers: BTreeMap, logger: Arc, } @@ -281,10 +323,12 @@ impl HttpClientBuilder { /// Creates a builder with a base URL and auth injector. #[must_use] pub fn new(base_url: impl Into, auth: Arc) -> Self { + let (user_agent, process_headers) = client_identity_snapshot(); Self { base_url: base_url.into(), auth, - user_agent: default_user_agent(), + user_agent, + process_headers, default_headers: BTreeMap::new(), logger: default_transport_logger(), } @@ -337,7 +381,7 @@ impl HttpClientBuilder { base_url: self.base_url, auth: self.auth, user_agent: self.user_agent, - default_headers: merged_default_headers(self.default_headers), + default_headers: merged_default_headers(self.process_headers, self.default_headers), logger: self.logger, } } @@ -345,10 +389,12 @@ impl HttpClientBuilder { /// Layers the client's own headers over the process-wide defaults so a /// per-client header wins on a name clash. -fn merged_default_headers(own: BTreeMap) -> BTreeMap { - let mut headers = default_headers(); - headers.extend(own); - headers +fn merged_default_headers( + mut process: BTreeMap, + own: BTreeMap, +) -> BTreeMap { + process.extend(own); + process } /// Converts a `reqwest` header map into owned name/value pairs for logging. diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index f2e2743..cb6a9d8 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -155,6 +155,7 @@ struct RestoreDefaultUserAgent; impl Drop for RestoreDefaultUserAgent { fn drop(&mut self) { transport::set_default_user_agent("cli/dev"); + transport::set_default_headers(Default::default()); } } @@ -6156,6 +6157,46 @@ async fn http_client_no_content_returns_default_result_preserves_legacy_skips_de assert_eq!(value, Thing::default()); } +/// A personality hand-off runs an independent `Cli` with its own config; the +/// outbound identity published for the process must be that CLI's, not the +/// dispatcher's. +#[tokio::test] +async fn execute_from_publishes_the_personality_identity_not_the_dispatchers() { + let _guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore = RestoreDefaultUserAgent; + let cli = Cli::new( + CliConfig::new("outer", "Outer", "outer") + .with_build(cli_engine::BuildInfo::new("1.0.0")) + .with_argv0_personality("inner", || { + CliConfig::new("inner", "Inner", "inner") + .with_build(cli_engine::BuildInfo::new("9.9.9")) + }), + ); + let (mut stdout, mut stderr) = (Vec::new(), Vec::new()); + + cli.execute_from(["inner", "--version"], &mut stdout, &mut stderr) + .await + .expect("personality run completes"); + + assert_eq!(transport::default_user_agent(), "inner/9.9.9"); +} + +/// `Cli::run` never mutates process-wide state (tests call it concurrently); +/// only the `execute*` entrypoints publish the outbound identity. +#[tokio::test] +async fn run_does_not_publish_the_process_identity() { + let _guard = USER_AGENT_TEST_LOCK.lock().await; + let _restore = RestoreDefaultUserAgent; + transport::set_default_user_agent("sentinel/1"); + let cli = Cli::new( + CliConfig::new("outer", "Outer", "outer").with_build(cli_engine::BuildInfo::new("1.0.0")), + ); + + cli.run(["outer", "--version"]).await; + + assert_eq!(transport::default_user_agent(), "sentinel/1"); +} + #[tokio::test] async fn http_client_common_method_helpers_cover_put_patch_and_delete_variants() { let put_server = TestServer::new(|request| { From 75ea56f79478460bed9cf58ddbefcb699903fab7 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 8 Oct 2026 17:44:47 -0700 Subject: [PATCH 3/8] fix(transport): keep HttpClient on a single identity snapshot; case-insensitive header override `HttpClientBuilder::new` captured the process identity, but `build()` created the base `reqwest::Client` from `reqwest_client_builder()`, which took a second snapshot. If another identity was published in between, the base client could carry the newer snapshot's uniquely named default headers while requests added the older snapshot's, leaking another CLI's correlation header. `HttpClient` applies its user-agent and default headers per request, so its base client now comes from a timeout-only builder that never reads process identity. Header names are case-insensitive, but process defaults were merged with client headers by exact key, so a client's `X-Client-Session` was sent alongside a default `x-client-session` instead of replacing it. Default header names are now lowercased when published and the merge compares them case-insensitively. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/src/transport/client/factory.rs | 79 ++++++++++++++++++++-- cli-engine/src/transport/client/mod.rs | 14 +++- 2 files changed, 86 insertions(+), 7 deletions(-) diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs index f8d06a7..d6a29db 100644 --- a/cli-engine/src/transport/client/factory.rs +++ b/cli-engine/src/transport/client/factory.rs @@ -41,9 +41,18 @@ pub const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(30); /// Later settings win. pub fn reqwest_client_builder() -> reqwest::ClientBuilder { let (user_agent, headers) = client_identity_snapshot(); - reqwest::Client::builder() + timeout_policy_builder() .user_agent(user_agent) .default_headers(header_map(&headers)) +} + +/// A builder carrying only the timeout policy, with no process identity. +/// +/// [`super::HttpClient`] applies its user-agent and default headers per +/// request from the snapshot its builder captured, so its base client must not +/// read the process-wide identity a second time. +fn timeout_policy_builder() -> reqwest::ClientBuilder { + reqwest::Client::builder() .connect_timeout(DEFAULT_CONNECT_TIMEOUT) .read_timeout(DEFAULT_READ_TIMEOUT) } @@ -63,10 +72,10 @@ fn header_map(headers: &std::collections::BTreeMap) -> reqwest:: .collect() } -/// Builds a client from [`reqwest_client_builder`], falling back to a bare -/// client if the TLS backend fails to initialize. +/// Builds the timeout-only base client for [`super::HttpClient`], falling back +/// to a bare client if the TLS backend fails to initialize. pub(super) fn build_default_client() -> reqwest::Client { - reqwest_client_builder() + timeout_policy_builder() .build() .unwrap_or_else(|_| reqwest::Client::new()) } @@ -206,6 +215,68 @@ mod tests { assert!(head.contains("x-process-only: 1"), "{head}"); } + /// A client's `X-Client-Session` replaces a process default `x-client-session` + /// (header names are case-insensitive); only the client's value is sent. + #[tokio::test] + async fn http_client_header_override_is_case_insensitive() { + let (url, server) = serve_once(); + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_headers( + [("x-client-session".to_owned(), "process".to_owned())].into(), + ); + crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ) + .default_headers([("X-Client-Session".to_owned(), "client".to_owned())].into()) + .build() + }; + + client.get_bytes("/").await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert_eq!(head.matches("x-client-session:").count(), 1, "{head}"); + assert!(head.contains("x-client-session: client"), "{head}"); + } + + /// `HttpClient` uses the identity captured when its builder was created; + /// publishing a different identity before `build()` must not leak into it. + #[tokio::test] + async fn http_client_does_not_resnapshot_identity_at_build_time() { + let (url, server) = serve_once(); + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_client_identity( + "app-a/1".to_owned(), + [("x-app".to_owned(), "a".to_owned())].into(), + ); + let builder = crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ); + super::super::set_client_identity( + "app-b/2".to_owned(), + [("x-app-b".to_owned(), "b".to_owned())].into(), + ); + builder.build() + }; + + client.get_bytes("/").await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert!(head.contains("user-agent: app-a/1"), "{head}"); + assert!(head.contains("x-app: a"), "{head}"); + assert!(!head.contains("app-b"), "{head}"); + assert!(!head.contains("x-app-b"), "{head}"); + } + /// The user-agent and default headers are published and read as a pair, so /// a concurrent reader can never see one publish's user-agent with /// another's headers. diff --git a/cli-engine/src/transport/client/mod.rs b/cli-engine/src/transport/client/mod.rs index 084d0df..75e752b 100644 --- a/cli-engine/src/transport/client/mod.rs +++ b/cli-engine/src/transport/client/mod.rs @@ -100,7 +100,9 @@ pub(crate) fn client_identity_snapshot() -> (String, BTreeMap) { ) } -/// Keeps only entries that are valid HTTP header names with valid values. +/// Keeps only entries that are valid HTTP header names with valid values, with +/// names lowercased (header names are case-insensitive, so one canonical form +/// lets later merging detect a clash). fn valid_headers(headers: BTreeMap) -> BTreeMap { headers .into_iter() @@ -112,6 +114,7 @@ fn valid_headers(headers: BTreeMap) -> BTreeMap } valid }) + .map(|(name, value)| (name.to_ascii_lowercase(), value)) .collect() } @@ -388,12 +391,17 @@ impl HttpClientBuilder { } /// Layers the client's own headers over the process-wide defaults so a -/// per-client header wins on a name clash. +/// per-client header wins on a name clash. Header names are case-insensitive, +/// so `X-Foo` in `own` replaces a default `x-foo` instead of being sent +/// alongside it. fn merged_default_headers( mut process: BTreeMap, own: BTreeMap, ) -> BTreeMap { - process.extend(own); + for (name, value) in own { + process.retain(|existing, _| !existing.eq_ignore_ascii_case(&name)); + process.insert(name, value); + } process } From 25f43d3d40dd3d4c94d40484f1ebd0ec9b32938b Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Thu, 8 Oct 2026 17:54:44 -0700 Subject: [PATCH 4/8] fix(transport): stop default headers duplicating request headers; document client-creation timing Process-wide default headers were appended to every HttpClient request after its request-specific headers, and `RequestBuilder::header` appends, so a valid process default such as `content-type` or `user-agent` produced duplicate header lines instead of losing to the request's own value. HttpClient now applies defaults once, on the built request: - the client's own default headers replace a same-named header the request set by default (the legacy way to opt into a vendor `Content-Type`), once rather than alongside it; - process-wide defaults rank lowest and only fill in headers still absent, so they can never duplicate or override anything. Applying them on the built request also keeps them visible in the `--debug transport` trace. Also document that `HttpClientBuilder::new` and `reqwest_client_builder` capture the process identity when called, so clients must be created inside command handlers (after `execute*` publishes), not during module registration. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/docs/attribution.md | 2 +- cli-engine/src/cli/config.rs | 4 +- cli-engine/src/transport/client/factory.rs | 88 +++++++++++++++++++++- cli-engine/src/transport/client/methods.rs | 63 ++++++++++------ cli-engine/src/transport/client/mod.rs | 33 ++++---- 5 files changed, 148 insertions(+), 42 deletions(-) diff --git a/cli-engine/docs/attribution.md b/cli-engine/docs/attribution.md index 87e27e5..3fee7d1 100644 --- a/cli-engine/docs/attribution.md +++ b/cli-engine/docs/attribution.md @@ -47,6 +47,6 @@ let config = CliConfig::new("my-cli", "Team CLI", "my-cli") ); ``` -The engine resolves attribution once per execution, before any command runs, and publishes it process-wide. Publishing happens in the `execute*` entrypoints, after argv0 resolution, so an argv0 personality publishes its own identity and not the dispatcher's. `Cli::run` deliberately does not publish (so running a `Cli` in tests never mutates process-wide state); a harness that drives `Cli::run` and needs outbound requests to carry the identity must publish it itself. The user-agent and default headers are published and read together, so a client never sees one without the other. It is applied to every `HttpClient` and to every client built from `transport::reqwest_client_builder()` (the entry point for generated or hand-rolled `reqwest` clients). The user-agent tokens also reach the engine's own OAuth token requests; the session header does not. +The engine resolves attribution once per execution, before any command runs, and publishes it process-wide. Publishing happens in the `execute*` entrypoints, after argv0 resolution, so an argv0 personality publishes its own identity and not the dispatcher's. `Cli::run` deliberately does not publish (so running a `Cli` in tests never mutates process-wide state); a harness that drives `Cli::run` and needs outbound requests to carry the identity must publish it itself. The user-agent and default headers are published and read together, so a client never sees one without the other. It is applied to every `HttpClient` and to every client built from `transport::reqwest_client_builder()` (the entry point for generated or hand-rolled `reqwest` clients), **provided the client is created after publication**. Both capture the process identity at the moment they are created, so build clients inside command handlers (which run after `execute*` has published), not during module registration or `Cli::new`, which runs earlier. A client created too early keeps the identity that was current then and silently omits attribution. The user-agent tokens also reach the engine's own OAuth token requests; the session header does not. Because the headers are process-wide defaults, they are sent to whatever host those clients call. Build clients that talk to third-party hosts from a plain `reqwest::Client` if the session header should not reach them. diff --git a/cli-engine/src/cli/config.rs b/cli-engine/src/cli/config.rs index d734df3..28eebb2 100644 --- a/cli-engine/src/cli/config.rs +++ b/cli-engine/src/cli/config.rs @@ -398,7 +398,9 @@ impl CliConfig { /// (and `agent/` for a detected AI harness) to the outbound /// User-Agent, and, when the harness exposes a session id, sends a salted /// hash of it in a correlation header. Nothing is sent to any endpoint the - /// CLI was not already calling. See `docs/attribution.md`. + /// CLI was not already calling. The identity is captured when each client is + /// created, so create clients inside command handlers, not during module + /// registration. See `docs/attribution.md`. #[must_use] pub fn with_client_attribution( mut self, diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs index d6a29db..a4e0dde 100644 --- a/cli-engine/src/transport/client/factory.rs +++ b/cli-engine/src/transport/client/factory.rs @@ -24,13 +24,15 @@ pub const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(30); /// outbound policy. /// /// - `User-Agent` is the process-wide default (see -/// [`set_default_user_agent`](super::set_default_user_agent)), read when this -/// function is called. -/// - The process-wide default headers (see +/// [`set_default_user_agent`](super::set_default_user_agent)), and the +/// process-wide default headers (see /// [`set_default_headers`](super::set_default_headers)) are sent on every /// request, so attribution reaches every client built here. A later /// `.default_headers(..)` on the returned builder adds to them; on a name /// clash the later value wins. +/// - Both are captured when this function is called, as one snapshot. Call it +/// from a command handler (after the `execute*` entrypoints have published +/// the identity), not during module registration, which runs earlier. /// - [`DEFAULT_CONNECT_TIMEOUT`] and [`DEFAULT_READ_TIMEOUT`] bound hangs. /// There is deliberately no total-request timeout: callers that want one /// (for example a short API call) set `.timeout(..)` on the returned builder, @@ -215,6 +217,86 @@ mod tests { assert!(head.contains("x-process-only: 1"), "{head}"); } + /// A default header never duplicates or overrides a header the request + /// itself sets: `Content-Type` and `User-Agent` are request-owned. + #[tokio::test] + async fn default_headers_do_not_duplicate_request_owned_headers() { + let (url, server) = serve_once(); + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_headers( + [ + ("content-type".to_owned(), "text/plain".to_owned()), + ("user-agent".to_owned(), "sneaky/0".to_owned()), + ("x-extra".to_owned(), "1".to_owned()), + ] + .into(), + ); + crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ) + .user_agent("real/1") + .build() + }; + + client + .post_without_response("/", &serde_json::json!({"a": 1})) + .await + .expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert_eq!(head.matches("content-type:").count(), 1, "{head}"); + assert!(head.contains("content-type: application/json"), "{head}"); + assert_eq!(head.matches("user-agent:").count(), 1, "{head}"); + assert!(head.contains("user-agent: real/1"), "{head}"); + assert!(head.contains("x-extra: 1"), "{head}"); + } + + /// The client's own default `Content-Type` replaces the JSON one, exactly + /// once (a client opting into a vendor media type), while a process-wide + /// default of the same name does not. + #[tokio::test] + async fn own_default_content_type_replaces_json_once() { + let (url, server) = serve_once(); + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_default_headers( + [("content-type".to_owned(), "text/plain".to_owned())].into(), + ); + crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ) + .default_headers( + [( + "Content-Type".to_owned(), + "application/vnd.test+json".to_owned(), + )] + .into(), + ) + .build() + }; + + client + .post_without_response("/", &serde_json::json!({"a": 1})) + .await + .expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert_eq!(head.matches("content-type:").count(), 1, "{head}"); + assert!( + head.contains("content-type: application/vnd.test+json"), + "{head}" + ); + } + /// A client's `X-Client-Session` replaces a process default `x-client-session` /// (header names are case-insensitive); only the client's value is sent. #[tokio::test] diff --git a/cli-engine/src/transport/client/methods.rs b/cli-engine/src/transport/client/methods.rs index 3714483..90d7ddf 100644 --- a/cli-engine/src/transport/client/methods.rs +++ b/cli-engine/src/transport/client/methods.rs @@ -465,7 +465,7 @@ impl HttpClient { .header(header::IF_MATCH, etag) .build() .map_err(|err| CliCoreError::message(format!("transport: create request: {err}")))?; - self.inject_auth(&mut request).await?; + self.prepare_request(&mut request).await?; self.log_request(&request); self.base .execute(request) @@ -497,18 +497,15 @@ impl HttpClient { form: reqwest::multipart::Form, ) -> Result { let url = format!("{}{}", self.base_url, path); - let mut builder = self + let builder = self .base .post(url) .header(header::USER_AGENT, self.user_agent.clone()) .multipart(form); - for (key, value) in &self.default_headers { - builder = builder.header(key, value); - } let mut request = builder .build() .map_err(|err| CliCoreError::message(format!("transport: create request: {err}")))?; - self.inject_auth(&mut request).await?; + self.prepare_request(&mut request).await?; self.log_request(&request); self.base .execute(request) @@ -547,13 +544,10 @@ impl HttpClient { if !content_type.is_empty() { builder = builder.header(header::CONTENT_TYPE, content_type); } - for (key, value) in &self.default_headers { - builder = builder.header(key, value); - } let mut request = builder .build() .map_err(|err| CliCoreError::message(format!("transport: create request: {err}")))?; - self.inject_auth(&mut request).await?; + self.prepare_request(&mut request).await?; self.log_request(&request); self.base .execute(request) @@ -591,17 +585,14 @@ impl HttpClient { async fn send_get_raw_once(&self, path: &str) -> Result { let url = format!("{}{}", self.base_url, path); - let mut builder = self + let builder = self .base .get(url) .header(header::USER_AGENT, self.user_agent.clone()); - for (key, value) in &self.default_headers { - builder = builder.header(key, value); - } let mut request = builder .build() .map_err(|err| CliCoreError::message(format!("transport: create request: {err}")))?; - self.inject_auth(&mut request).await?; + self.prepare_request(&mut request).await?; self.log_request(&request); self.base .execute(request) @@ -618,7 +609,7 @@ impl HttpClient { .build_request(Method::POST, path, body)? .build() .map_err(|err| CliCoreError::message(format!("transport: create request: {err}")))?; - self.inject_auth(&mut request).await?; + self.prepare_request(&mut request).await?; self.log_request(&request); self.base .execute(request) @@ -711,7 +702,7 @@ impl HttpClient { .build_request(method.clone(), path, body)? .build() .map_err(|err| CliCoreError::message(format!("transport: create request: {err}")))?; - self.inject_auth(&mut request).await?; + self.prepare_request(&mut request).await?; let method_text = method.as_str().to_owned(); self.log_request(&request); self.base @@ -738,9 +729,6 @@ impl HttpClient { .header(header::CONTENT_TYPE, "application/json") .body(body); } - for (key, value) in &self.default_headers { - builder = builder.header(key, value); - } Ok(builder) } @@ -847,13 +835,46 @@ impl HttpClient { Ok((status, body)) } - async fn inject_auth(&self, request: &mut reqwest::Request) -> Result<()> { + /// Finalizes a built request: applies default headers, then injects auth. + /// + /// The client's own defaults replace a same-named header the request set + /// by default (for example `Content-Type`), once. Process-wide defaults + /// rank lowest: they only fill in headers still absent, so they can never + /// duplicate or override anything. Applying both here, on the built + /// request, rather than appending on the builder, is what prevents + /// duplicate header lines and keeps the defaults visible in the + /// `--debug transport` trace. + async fn prepare_request(&self, request: &mut reqwest::Request) -> Result<()> { + self.apply_default_headers(request)?; self.auth .inject(request) .await .map_err(|err| CliCoreError::message(format!("transport: auth inject: {err}"))) } + fn apply_default_headers(&self, request: &mut reqwest::Request) -> Result<()> { + let parse = |name: &str, value: &str| -> Result<_> { + let invalid = |err: &dyn std::fmt::Display| { + CliCoreError::message(format!("transport: invalid default header {name:?}: {err}")) + }; + Ok(( + header::HeaderName::from_bytes(name.as_bytes()).map_err(|e| invalid(&e))?, + header::HeaderValue::from_str(value).map_err(|e| invalid(&e))?, + )) + }; + for (name, value) in &self.default_headers { + let (name, value) = parse(name, value)?; + request.headers_mut().insert(name, value); + } + for (name, value) in &self.process_headers { + let (name, value) = parse(name, value)?; + if !request.headers().contains_key(&name) { + request.headers_mut().insert(name, value); + } + } + Ok(()) + } + async fn decode_json_response( &self, response: reqwest::Response, diff --git a/cli-engine/src/transport/client/mod.rs b/cli-engine/src/transport/client/mod.rs index 75e752b..bbcc184 100644 --- a/cli-engine/src/transport/client/mod.rs +++ b/cli-engine/src/transport/client/mod.rs @@ -306,6 +306,10 @@ pub struct HttpClient { base_url: String, auth: Arc, user_agent: String, + /// Process-wide defaults captured at builder creation; fill in headers a + /// request does not already carry. + process_headers: BTreeMap, + /// The client's own defaults; replace a same-named request header. default_headers: BTreeMap, logger: Arc, } @@ -324,6 +328,11 @@ pub struct HttpClientBuilder { impl HttpClientBuilder { /// Creates a builder with a base URL and auth injector. + /// + /// Captures the process-wide user-agent and default headers now, as one + /// snapshot. Create clients inside command handlers, which run after the + /// `execute*` entrypoints have published the CLI's identity, not during + /// module registration, which runs earlier. #[must_use] pub fn new(base_url: impl Into, auth: Arc) -> Self { let (user_agent, process_headers) = client_identity_snapshot(); @@ -351,6 +360,12 @@ impl HttpClientBuilder { } /// Sets headers sent on every request. + /// + /// A default replaces (never duplicates) a header the request sets by + /// default with the same name, for example `Content-Type`; this is how a + /// client opts into a vendor media type. Header names are case-insensitive. + /// An invalid name or value fails the request. Process-wide defaults rank + /// below these: they only fill in headers that are otherwise absent. #[must_use] pub fn default_headers(mut self, headers: BTreeMap) -> Self { self.default_headers = headers; @@ -384,27 +399,13 @@ impl HttpClientBuilder { base_url: self.base_url, auth: self.auth, user_agent: self.user_agent, - default_headers: merged_default_headers(self.process_headers, self.default_headers), + process_headers: self.process_headers, + default_headers: self.default_headers, logger: self.logger, } } } -/// Layers the client's own headers over the process-wide defaults so a -/// per-client header wins on a name clash. Header names are case-insensitive, -/// so `X-Foo` in `own` replaces a default `x-foo` instead of being sent -/// alongside it. -fn merged_default_headers( - mut process: BTreeMap, - own: BTreeMap, -) -> BTreeMap { - for (name, value) in own { - process.retain(|existing, _| !existing.eq_ignore_ascii_case(&name)); - process.insert(name, value); - } - process -} - /// Converts a `reqwest` header map into owned name/value pairs for logging. /// /// Header values that are not valid UTF-8 are rendered as a byte-count From 3486ef7eaabde95c08d18a20e965a3bae7d44bb0 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 9 Oct 2026 09:16:14 -0700 Subject: [PATCH 5/8] fix(transport): preserve multipart boundary; make process header setters internal - A client's own default `Content-Type` replaced a multipart request's generated `Content-Type`, dropping the boundary the server needs to parse the body. A multipart `Content-Type` is now preserved. - `set_default_headers` / `default_headers` were public, but the engine's client attribution is their only producer, and exposing them let an `execute*` call silently replace caller-set headers and let a `user-agent` entry override the identity user-agent. They are now test-only helpers; production publishes headers together with the user-agent through `set_client_identity`. Nothing public changes for released versions, since this API was never released. - `reqwest_client_builder` now applies the header map before the identity user-agent, so a `user-agent` entry in the map can never replace it, matching how `HttpClient` ranks them. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/src/transport/client/factory.rs | 69 ++++++++++++++++++++-- cli-engine/src/transport/client/methods.rs | 20 +++++++ cli-engine/src/transport/client/mod.rs | 22 +++---- cli-engine/src/transport/mod.rs | 5 +- cli-engine/tests/foundation.rs | 1 - 5 files changed, 96 insertions(+), 21 deletions(-) diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs index a4e0dde..9aaf79c 100644 --- a/cli-engine/src/transport/client/factory.rs +++ b/cli-engine/src/transport/client/factory.rs @@ -25,9 +25,9 @@ pub const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(30); /// /// - `User-Agent` is the process-wide default (see /// [`set_default_user_agent`](super::set_default_user_agent)), and the -/// process-wide default headers (see -/// [`set_default_headers`](super::set_default_headers)) are sent on every -/// request, so attribution reaches every client built here. A later +/// headers published by client attribution (see +/// [`AttributionConfig`](crate::transport::AttributionConfig)) are sent on +/// every request, so attribution reaches every client built here. A later /// `.default_headers(..)` on the returned builder adds to them; on a name /// clash the later value wins. /// - Both are captured when this function is called, as one snapshot. Call it @@ -43,9 +43,12 @@ pub const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(30); /// Later settings win. pub fn reqwest_client_builder() -> reqwest::ClientBuilder { let (user_agent, headers) = client_identity_snapshot(); + // The user-agent goes last: `reqwest` stores it as a default header, so + // applying it after the header map keeps a `user-agent` entry in that map + // from replacing the identity, matching how `HttpClient` ranks them. timeout_policy_builder() - .user_agent(user_agent) .default_headers(header_map(&headers)) + .user_agent(user_agent) } /// A builder carrying only the timeout policy, with no process identity. @@ -256,6 +259,64 @@ mod tests { assert!(head.contains("x-extra: 1"), "{head}"); } + /// A `user-agent` entry among the published headers must not replace the + /// identity user-agent on clients from the factory. + #[tokio::test] + async fn header_map_cannot_override_the_identity_user_agent() { + let client = { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_client_identity( + "identity/1".to_owned(), + [("user-agent".to_owned(), "sneaky/0".to_owned())].into(), + ); + reqwest_client_builder().build().expect("client builds") + }; + let (url, server) = serve_once(); + + client.get(&url).send().await.expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert_eq!(head.matches("user-agent:").count(), 1, "{head}"); + assert!(head.contains("user-agent: identity/1"), "{head}"); + } + + /// A client's own default `Content-Type` must not replace a multipart + /// request's generated `Content-Type`: that header carries the boundary + /// the server needs to parse the body. + #[tokio::test] + async fn own_default_content_type_does_not_strip_the_multipart_boundary() { + let upload = tempfile::NamedTempFile::new().expect("temp file"); + std::fs::write(upload.path(), b"file-bytes").expect("write upload"); + let (url, server) = serve_once(); + let client = crate::transport::HttpClientBuilder::new( + url.trim_end_matches('/'), + std::sync::Arc::new(crate::transport::NoopInjector), + ) + .default_headers( + [( + "Content-Type".to_owned(), + "application/vnd.test+json".to_owned(), + )] + .into(), + ) + .build(); + + client + .post_multipart_without_response("/", "file", upload.path()) + .await + .expect("request succeeds"); + + let head = server.join().expect("server thread").to_lowercase(); + assert_eq!(head.matches("content-type:").count(), 1, "{head}"); + assert!( + head.contains("content-type: multipart/form-data; boundary="), + "{head}" + ); + } + /// The client's own default `Content-Type` replaces the JSON one, exactly /// once (a client opting into a vendor media type), while a process-wide /// default of the same name does not. diff --git a/cli-engine/src/transport/client/methods.rs b/cli-engine/src/transport/client/methods.rs index 90d7ddf..63fcfe0 100644 --- a/cli-engine/src/transport/client/methods.rs +++ b/cli-engine/src/transport/client/methods.rs @@ -864,6 +864,11 @@ impl HttpClient { }; for (name, value) in &self.default_headers { let (name, value) = parse(name, value)?; + // A multipart `Content-Type` carries the boundary the server needs + // to parse the body; replacing it would make the upload unreadable. + if name == header::CONTENT_TYPE && has_multipart_content_type(request) { + continue; + } request.headers_mut().insert(name, value); } for (name, value) in &self.process_headers { @@ -955,3 +960,18 @@ impl HttpClient { } } } + +/// Whether the request's `Content-Type` is a multipart type (its generated +/// value carries the part boundary). +fn has_multipart_content_type(request: &reqwest::Request) -> bool { + request + .headers() + .get(header::CONTENT_TYPE) + .and_then(|value| value.to_str().ok()) + .is_some_and(|value| { + value + .trim_start() + .to_ascii_lowercase() + .starts_with("multipart/") + }) +} diff --git a/cli-engine/src/transport/client/mod.rs b/cli-engine/src/transport/client/mod.rs index bbcc184..cfde15b 100644 --- a/cli-engine/src/transport/client/mod.rs +++ b/cli-engine/src/transport/client/mod.rs @@ -63,17 +63,13 @@ pub fn default_user_agent() -> String { ) } -/// Sets the process-wide default headers sent on requests from -/// [`reqwest_client_builder`] and [`HttpClientBuilder`]. +/// Test helper: replaces only the process-wide default headers. /// -/// A client's own headers (including auth) win over these on a name clash. -/// Unlike the user-agent, these are not applied to the engine's OAuth token -/// requests. -/// -/// An entry whose name or value is not a valid HTTP header is dropped here -/// (and its name logged), so every consumer of the defaults sees only headers -/// that can actually be sent. -pub fn set_default_headers(headers: BTreeMap) { +/// Production code publishes headers together with the user-agent through +/// [`set_client_identity`]; the engine's client attribution is the only +/// producer, which is why this is not public API. +#[cfg(test)] +pub(crate) fn set_default_headers(headers: BTreeMap) { let headers = valid_headers(headers); if let Ok(mut current) = client_identity().write() { current.headers = headers; @@ -118,9 +114,9 @@ fn valid_headers(headers: BTreeMap) -> BTreeMap .collect() } -/// Returns the process-wide default headers set via [`set_default_headers`]. -#[must_use] -pub fn default_headers() -> BTreeMap { +/// Test helper: the process-wide default headers currently published. +#[cfg(test)] +pub(crate) fn default_headers() -> BTreeMap { client_identity() .read() .map(|identity| identity.headers.clone()) diff --git a/cli-engine/src/transport/mod.rs b/cli-engine/src/transport/mod.rs index 59ab775..4dd3b7c 100644 --- a/cli-engine/src/transport/mod.rs +++ b/cli-engine/src/transport/mod.rs @@ -24,9 +24,8 @@ pub(crate) use attribution::{Attribution, Signals}; pub use client::{ DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT, HttpClient, HttpClientBuilder, NoopTransportLogger, TransportLogEvent, TransportLogger, debug_log_reqwest_request, - debug_log_reqwest_response, default_headers, default_transport_logger, default_user_agent, - reqwest_client_builder, set_default_headers, set_default_transport_logger, - set_default_user_agent, + debug_log_reqwest_response, default_transport_logger, default_user_agent, + reqwest_client_builder, set_default_transport_logger, set_default_user_agent, }; pub use debug_logger::StderrTransportLogger; pub use injector::{ diff --git a/cli-engine/tests/foundation.rs b/cli-engine/tests/foundation.rs index cb6a9d8..abb1f91 100644 --- a/cli-engine/tests/foundation.rs +++ b/cli-engine/tests/foundation.rs @@ -155,7 +155,6 @@ struct RestoreDefaultUserAgent; impl Drop for RestoreDefaultUserAgent { fn drop(&mut self) { transport::set_default_user_agent("cli/dev"); - transport::set_default_headers(Default::default()); } } From ac54564dcf6b6757d5aebb2de12a32a5e3932f4d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 9 Oct 2026 09:21:35 -0700 Subject: [PATCH 6/8] fix(transport): setting the default user-agent resets the published identity `set_default_user_agent` replaced only the user-agent, so after an attributed CLI ran, a caller using it paired its new user-agent with the previous execution's session header. Replacing the user-agent invalidates the published pair, so it now clears the headers in the same write. Also document that a blank `CI` value counts as unset, matching `is_truthy`. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/docs/attribution.md | 2 +- cli-engine/src/transport/client/factory.rs | 20 ++++++++++++++++++++ cli-engine/src/transport/client/mod.rs | 7 +++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/cli-engine/docs/attribution.md b/cli-engine/docs/attribution.md index 3fee7d1..57a8228 100644 --- a/cli-engine/docs/attribution.md +++ b/cli-engine/docs/attribution.md @@ -12,7 +12,7 @@ It is off by default. A CLI opts in with `CliConfig::with_client_attribution`. | `User-Agent` | `agent/` (for example `agent/claude-code`) | A known AI harness is detected | | `x-client-session` (name configurable) | 16 hex characters: a salted SHA-256 prefix of the harness session id | The harness exposes a session id, and the user has not opted out | -`` is the first that applies: `agent` (a harness marker is present), `ci` (the `CI` variable is set to anything other than `0`, `false`, `no`, or `off`), `interactive` (stdin and stderr are terminals), or `script` (none of the above). +`` is the first that applies: `agent` (a harness marker is present), `ci` (the `CI` variable is set to a non-blank value other than `0`, `false`, `no`, or `off`, compared case-insensitively), `interactive` (stdin and stderr are terminals), or `script` (none of the above). An example, from a Claude Code session: `User-Agent: gddy/1.4.0 mode/agent agent/claude-code` and `x-gddy-session: 3f9a0c51d27be8a4`. diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs index 9aaf79c..ccf2e86 100644 --- a/cli-engine/src/transport/client/factory.rs +++ b/cli-engine/src/transport/client/factory.rs @@ -259,6 +259,26 @@ mod tests { assert!(head.contains("x-extra: 1"), "{head}"); } + /// Replacing the user-agent invalidates the published pair, so it must not + /// leave a previous execution's attribution headers behind. + #[test] + fn setting_the_user_agent_clears_previously_published_headers() { + let _guard = UA_TEST_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _restore = RestoreDefaultUserAgent; + super::super::set_client_identity( + "cli-a/1".to_owned(), + [("x-client-session".to_owned(), "a-session-hash".to_owned())].into(), + ); + + super::super::set_default_user_agent("cli-b/2"); + + let (user_agent, headers) = client_identity_snapshot(); + assert_eq!(user_agent, "cli-b/2"); + assert!(headers.is_empty(), "stale headers survived: {headers:?}"); + } + /// A `user-agent` entry among the published headers must not replace the /// identity user-agent on clients from the factory. #[tokio::test] diff --git a/cli-engine/src/transport/client/mod.rs b/cli-engine/src/transport/client/mod.rs index cfde15b..145d462 100644 --- a/cli-engine/src/transport/client/mod.rs +++ b/cli-engine/src/transport/client/mod.rs @@ -44,9 +44,16 @@ fn client_identity() -> &'static RwLock { /// outbound token traffic that reads this default — the PKCE provider's /// token/refresh requests and the client-credentials injector. A per-client /// user-agent still overrides it for that client. +/// +/// This replaces the whole process-wide outbound identity: it also drops any +/// headers published by client attribution, so the new user-agent is never +/// paired with a previous execution's session header. pub fn set_default_user_agent(user_agent: impl Into) { if let Ok(mut current) = client_identity().write() { current.user_agent = user_agent.into(); + // The headers belong to the identity this replaces; keeping them would + // pair the new user-agent with a previous execution's session hash. + current.headers.clear(); } } From 23919e7565fbc9341755ee962833b5803f764cb0 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 9 Oct 2026 09:27:59 -0700 Subject: [PATCH 7/8] test(transport): read the full request head in the loopback test server The wire-level tests' `serve_once` helper did a single `read`, which is not guaranteed to contain the whole HTTP header block, so assertions could intermittently miss a header. Read until the blank line that ends the head, drain any request body still in flight so closing cannot reset the client, and return only the head. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/src/transport/client/factory.rs | 27 +++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/cli-engine/src/transport/client/factory.rs b/cli-engine/src/transport/client/factory.rs index ccf2e86..6fdb795 100644 --- a/cli-engine/src/transport/client/factory.rs +++ b/cli-engine/src/transport/client/factory.rs @@ -95,18 +95,39 @@ mod tests { use super::*; use crate::transport::client::{RestoreDefaultUserAgent, UA_TEST_LOCK}; - /// Serves one request on loopback and returns the raw request head. + /// Serves one request on loopback and returns the request head (the bytes + /// up to the blank line that ends the headers). + /// + /// TCP may deliver the head across several reads, so this reads until the + /// terminator arrives. It then drains any request body still in flight + /// before closing, so the client never sees a reset mid-write. fn serve_once() -> (String, std::thread::JoinHandle) { let listener = TcpListener::bind("127.0.0.1:0").expect("bind loopback"); let url = format!("http://{}/", listener.local_addr().expect("local addr")); let handle = std::thread::spawn(move || { + const HEAD_END: &[u8] = b"\r\n\r\n"; let (mut stream, _) = listener.accept().expect("accept"); + stream + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("set read timeout"); + let mut received = Vec::new(); let mut buf = [0_u8; 4096]; - let read = stream.read(&mut buf).expect("read request"); + let head_len = loop { + if let Some(at) = received.windows(HEAD_END.len()).position(|w| w == HEAD_END) { + break at; + } + let read = stream.read(&mut buf).expect("read request head"); + assert!(read > 0, "connection closed before the request head ended"); + received.extend_from_slice(&buf[..read]); + }; stream .write_all(b"HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n") .expect("write response"); - String::from_utf8_lossy(&buf[..read]).into_owned() + stream + .set_read_timeout(Some(Duration::from_millis(50))) + .expect("set drain timeout"); + while matches!(stream.read(&mut buf), Ok(read) if read > 0) {} + String::from_utf8_lossy(&received[..head_len]).into_owned() }); (url, handle) } From d0d3523bd2bd2d918380e2c8eee9bab373ebbb75 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 9 Oct 2026 09:32:22 -0700 Subject: [PATCH 8/8] docs(transport): disclose the filesystem marker check in agent detection The privacy-facing attribution docs said detection reads only environment variables, but the detector is also given a file-existence check and tests a small fixed set of marker paths (currently only `/opt/.devin`). Say so, and state that only existence is tested: no contents are read and no directories are listed. Co-Authored-By: Claude Sonnet 5.5 --- cli-engine/docs/attribution.md | 2 +- cli-engine/src/transport/attribution.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cli-engine/docs/attribution.md b/cli-engine/docs/attribution.md index 57a8228..b066841 100644 --- a/cli-engine/docs/attribution.md +++ b/cli-engine/docs/attribution.md @@ -32,7 +32,7 @@ Run any command with `--debug=transport` to print each outbound request's header ## How detection works, and its limits -Detection reads environment variables that AI harnesses publish to the processes they launch, using the [`is-ai-agent`](https://github.com/sdairs/is-ai-agent) crate. It is cooperative and heuristic. A match does not prove a model issued this particular command (a human can run commands in a terminal a harness opened), and no match does not prove a human did. Treat the result as attribution to a harness, not as proof of intent. Session ids have different scopes per harness (a conversation, a thread, a single run), so the hash correlates calls within one harness only. +Detection reads environment variables that AI harnesses publish to the processes they launch, using the [`is-ai-agent`](https://github.com/sdairs/is-ai-agent) crate. It also checks whether a small fixed set of marker paths defined by that crate exists (currently only `/opt/.devin`, which identifies Devin). Those checks test existence only: no file contents are read and no directories are listed. It is cooperative and heuristic. A match does not prove a model issued this particular command (a human can run commands in a terminal a harness opened), and no match does not prove a human did. Treat the result as attribution to a harness, not as proof of intent. Session ids have different scopes per harness (a conversation, a thread, a single run), so the hash correlates calls within one harness only. ## Enabling it (CLI authors) diff --git a/cli-engine/src/transport/attribution.rs b/cli-engine/src/transport/attribution.rs index d3119c6..a14b8a5 100644 --- a/cli-engine/src/transport/attribution.rs +++ b/cli-engine/src/transport/attribution.rs @@ -9,8 +9,11 @@ //! hash of it (never the raw id) so a service can group one session's calls. //! //! Detection is cooperative and heuristic: it reads environment markers that -//! harnesses publish to their subprocesses. A match does not prove a model -//! issued the command, and no match does not prove a human did. +//! harnesses publish to their subprocesses, and checks whether a small fixed +//! set of marker paths defined by the detector exists (currently only +//! `/opt/.devin`). It only tests existence; it never reads file contents or +//! lists directories. A match does not prove a model issued the command, and no +//! match does not prove a human did. use std::{collections::BTreeMap, path::Path};