feat(transport): shared reqwest client factory and opt-in client attribution

[jpage-godaddy]

Summary

• Adds transport::reqwest_client_builder(): a preconfigured reqwest::ClientBuilder for code that needs a plain reqwest::Client and cannot go through HttpClient (progenitor-generated clients, hand-rolled streaming or multipart uploads). 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.
• Adds opt-in client attribution (CliConfig::with_client_attribution(AttributionConfig::new())) so a CLI can tell the services it calls what kind of caller it is, without any telemetry channel:
  • User-Agent tokens: mode/<agent|ci|interactive|script>, plus agent/<slug> when an AI harness is detected.
  • A correlation header (default x-client-session, configurable) carrying a salted SHA-256 prefix of the harness session id. The raw id never leaves the process, and nothing is sent when no harness session id exists (no persistent identifier, nothing written to disk).
  • <APP_ID>_NO_SESSION_ID=1 drops the header; AttributionConfig::without_session_id() disables it for a CLI.
  • Detection uses the is-ai-agent crate (zero dependencies, MIT/Apache). It is cooperative and heuristic, and docs/attribution.md says so.
• The user-agent and the new default headers share one lock (ClientIdentity) rather than adding a third process-global.
• Off by default, so existing consumers' user-agents are unchanged. Full description of what is and is not sent: cli-engine/docs/attribution.md.

New public API

transport::{reqwest_client_builder, default_user_agent, set_default_headers, default_headers, DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT, AttributionConfig}, CliConfig::with_client_attribution, CliConfig::attribution.

Identity publishing semantics (refined during review)

• The user-agent and default headers are written (set_client_identity) and read (client_identity_snapshot) under a single lock acquisition, so a client never pairs one publish's user-agent with another's headers.
• set_default_headers drops entries with an invalid header name or value at publish time (logging the name), so the factory and HttpClient only ever see sendable headers.
• Header precedence on an HttpClient request is applied once, on the built request: the client's own default headers replace a same-named header the request set by default (e.g. a vendor Content-Type), exactly once; process-wide defaults rank lowest and only fill in headers still absent, so they can never duplicate or override anything. Names are case-insensitive. Defaults are also visible in the --debug transport trace.
• HttpClient captures the process identity once, when its builder is created; its base reqwest::Client is built from a timeout-only builder and never re-reads process identity.
• Because identity is captured when a client (or reqwest_client_builder()) is created, create clients inside command handlers, which run after execute* has published, not during module registration. Documented in docs/attribution.md and on the relevant APIs.
• Identity is published by the execute* entrypoints after argv0 resolution, so an argv0 personality (an independent Cli with its own config) publishes its own identity rather than the dispatcher's.
• Cli::run intentionally does not publish, so running a Cli (as tests do, concurrently) never mutates process-wide state. This is documented on Cli::run and in docs/attribution.md.

Behavior changes to review

• HttpClient previously had no timeouts. It now has a 5s connect timeout and a 30s idle read timeout (per read, not per request). No total-request timeout is applied, so large streaming transfers that keep making progress are unaffected, but a server that takes more than 30s to send the first response byte now fails where it previously waited.
• sha2 is no longer an optional dependency (attribution needs it unconditionally); it is removed from the pkce-auth feature list.
• New dependency: is-ai-agent 0.6.

Out of scope / follow-ups

• The default session header name (x-client-session) is a placeholder. It is trivially changed in one constant; I'd like reviewer opinion before this ships.
• traceparent forwarding is intentionally not included.
• Attribution headers are not applied to the engine's OAuth token requests (the user-agent tokens are).

Test plan

• cargo fmt --all --check
• cargo clippy --all-targets -- -D warnings (default features and --features pkce-auth)
• cargo test --all-targets (default features and --features pkce-auth)
• cargo test --doc, RUSTDOCFLAGS='-D warnings' cargo doc --no-deps, ./cli-engine/scripts/check-module-size.sh, cargo rustdoc --lib -- -W missing-docs (0)
• New tests:
  • factory: UA reaches the wire, caller override wins, a stalled server hits the read timeout instead of hanging, default headers reach the wire, a caller's later .default_headers(..) adds to (does not replace) the process defaults, an invalid default header is skipped rather than fatal, and HttpClient merges process defaults under its own headers (client wins on a clash).
  • attribution: harness detection, hashed (never raw) session id, hash stability and per-app salting, mode precedence, CI=false/0/off handling, the opt-out env var and its name derivation, configurable/invalid header names.
  • Cli::client_identity composes the base user-agent (including an explicit override) with attribution.
  • identity pair is never torn under concurrent publishes; invalid default headers are dropped at publish time and do not fail HttpClient requests; an argv0 personality publishes its own identity on the execute path; Cli::run leaves the identity untouched.

Manual verification

This was validated end to end against a downstream consumer CLI (gddy) by pointing it at this branch with a local [patch.crates-io] override and running the real binary against a local capture server.

Setup:

# In a consumer CLI that opts in via `.with_client_attribution(AttributionConfig::new())`:
# Cargo.toml
#   [patch.crates-io]
#   cli-engine = { path = "../cli-engine/cli-engine" }
cargo build
# Point the CLI at a local HTTP server that logs `user-agent` and `x-client-session`.

Test WITHOUT the change (baseline):

# On main: any request carries only `<name>/<version>`; no mode/agent tokens, no session header.

Test WITH the change:

CLAUDECODE=1 CLAUDE_CODE_SESSION_ID=sess-secret-123 <cli> <any command that makes a request>
# Expected: user-agent `<name>/<version> mode/agent agent/claude-code`
#           x-client-session: 16 hex chars (the raw id does not appear anywhere)

<cli> <same command>                       # no harness env, non-TTY
# Expected: user-agent `<name>/<version> mode/script`, no session header

CLAUDECODE=1 CLAUDE_CODE_SESSION_ID=sess-secret-123 <APP_ID>_NO_SESSION_ID=1 <cli> <same command>
# Expected: agent tokens present, no session header

Cleanup:

# Remove the [patch.crates-io] block from the consumer's Cargo.toml.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Identity lifecycle, concurrency, and malformed-header handling can produce missing, mixed, or failed outbound attribution.

3 open findings

• Atomically publish and snapshot client identity headers · New
• Install identity in the common CLI run path · New
• Validate process default headers before merging · New

What changed in this PR

Adds shared HTTP client policy and opt-in caller attribution across cli-engine transports.

Changes:

• Introduces a preconfigured reqwest client factory with connection/read timeouts.
• Adds configurable client-mode and hashed-session attribution.
• Updates tests, documentation, and dependencies.

File	Description
cli-engine/​src/​transport/​mod.rs	Exports transport factory and attribution APIs.
cli-engine/​src/​transport/​client/​mod.rs	Adds shared identity state and default-header merging.
cli-engine/​src/​transport/​client/​factory.rs	Implements the preconfigured reqwest factory and tests.
cli-engine/​src/​transport/​attribution.rs	Implements attribution detection and hashing.
cli-engine/​src/​transport/​attribution/​tests.rs	Tests attribution behavior and opt-outs.
cli-engine/​src/​cli/​run.rs	Installs client identity during execution.
cli-engine/​src/​cli/​mod.rs	Resolves and publishes configured attribution.
cli-engine/​src/​cli/​flags_apply.rs	Tests CLI identity composition.
cli-engine/​src/​cli/​config.rs	Adds attribution configuration.
cli-engine/​docs/​concepts.md	Documents shared transport policy.
cli-engine/​docs/​attribution.md	Documents attribution and privacy behavior.
cli-engine/​Cargo.toml	Adds attribution dependencies.
Cargo.lock	Locks the new dependency.

🧠 Review effort: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

HttpClient can mix identity snapshots, and case-sensitive header merging can violate override semantics.

1 open finding

• Re-snapshotting process identity breaks atomic client headers · New

3 resolved since last review

• Atomically publish and snapshot client identity headers
• Validate process default headers before merging
• Install identity in the common CLI run path

🧠 Review effort: Balanced

[Copilot]

🔵 Needs a closer look

Process defaults can duplicate request-specific headers, and the attribution lifecycle documentation overstates client coverage.

0 open findings

1 resolved since last review

• Re-snapshotting process identity breaks atomic client headers

Previously missed (2)

In code that hasn't changed since last review

Process default headers duplicate request-specific headers

cli-engine/​src/​transport/​client/​mod.rs:387

Process defaults are merged into the map that each request adds after its request-specific headers (for example, Content-Type is set before the loop in methods.rs:548-551). RequestBuilder::header appends, so a valid process default such as content-type or user-agent produces duplicate values rather than being overridden by the client/request, contrary to this API's “client wins” contract. Apply the captured process map as actual reqwest client defaults, or skip defaults for names already present on the request.

Clients created before attribution publication retain stale process identity

cli-engine/​docs/​attribution.md:50

This says attribution is applied to every HttpClient, but HttpClientBuilder::new snapshots identity immediately, while Cli::new invokes module registration before any execute* entrypoint publishes attribution. A client created or captured during module registration therefore permanently retains the previous process identity. Document that both client builders must be called after publication (or change the lifecycle) so consumers do not silently miss attribution.

🧠 Review effort: Balanced

[jpage-godaddy]

Addressing the two "Previously missed" findings from the latest Copilot review (they were bundled in the review body, so there were no threads to resolve). Fixed in 25f43d3.

1. Process default headers duplicated request-specific headers (mod.rs). Confirmed: I reproduced it first, and a process default content-type/user-agent put two lines of each on the wire. Defaults are now applied once, on the built request, instead of being appended on the builder. The reviewer's suggested rule ("request always wins") had to be refined, though: an existing test, http_client_default_headers_can_override_json_content_type_preserves_legacy, documents that a client's own default Content-Type is meant to override the JSON one. So the hierarchy is: the client's own default headers replace a same-named request header (once, not alongside it), while process-wide defaults rank lowest and only fill in headers that are absent. New tests pin both halves, plus the Content-Type override exactly once.

2. Clients created before attribution publication keep stale identity (attribution.md). Correct, and a documentation gap rather than a code defect: HttpClientBuilder::new and reqwest_client_builder have always captured the identity when called (the same was true of the user-agent before this PR). Documented that clients must be created inside command handlers, not during module registration, in docs/attribution.md, on HttpClientBuilder::new, reqwest_client_builder, and with_client_attribution.

[Copilot]

🟡 Changes recommended

Header precedence currently breaks multipart boundaries, can override the factory user-agent, and discards explicitly configured process defaults.

1 open finding

• Preserve multipart Content-Type boundary · New

Previously missed (2)

In code that hasn't changed since last review

Preserve default headers when adding attribution

cli-engine/​src/​cli/​mod.rs:417

Every execute* call replaces the entire header map with attribution headers (or an empty map), so headers configured through the new public set_default_headers API during normal startup are silently discarded before command handlers create clients. Preserve caller-configured defaults as a separate layer and merge the per-execution attribution headers into them, while still removing attribution left by a previous execution.

Apply process defaults before identity user-agent

cli-engine/​src/​transport/​client/​factory.rs:48

The factory installs the user-agent before the process default header map, so a user-agent entry accepted by set_default_headers replaces the identity user-agent. This contradicts HttpClient, where process defaults rank below the request-owned user-agent, and makes default_user_agent() differ from what goes over the wire. Apply the process header map first so the identity user-agent wins; callers can still override it later on the returned builder.

🧠 Review effort: Balanced