You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 reads environment variables and tests whether one fixed marker path exists (/opt/.devin, existence only; no contents read, no directories listed). 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, DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT, AttributionConfig}, CliConfig::with_client_attribution, CliConfig::attribution. (Process-wide default headers are deliberately not public API: client attribution is their only producer, and they are published together with the user-agent through an internal atomic setter.)
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.
Published headers are validated at publish time: entries with an invalid header name or value are dropped (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. A multipart request's generated Content-Type (it carries the boundary) is never replaced. In reqwest_client_builder() the identity user-agent is applied last, so a header map can never override it.
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.
set_default_user_agent replaces the whole published identity: it also clears any headers published by attribution, so a new user-agent is never paired with a previous execution's session header.
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)
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.
…ibution
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/<agent|ci|interactive|script>` and `agent/<slug>`
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. `<APP_ID>_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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…nsensitive 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 <noreply@anthropic.com>
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.
…ument 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 <noreply@anthropic.com>
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.
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.
…ers 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 <noreply@anthropic.com>
Addressing the two "Previously missed" findings from the latest Copilot review (prose-only, so no threads). Fixed in 3486ef7. Both traced to the same root cause: the public transport::set_default_headers / default_headers API, which this PR added and which invited more edge cases each round (invalid values, merge order, clobbering, ordering). After discussing with the author, the fix is to remove the root cause rather than layer more rules on it.
1. execute* replaced caller-configured default headers (cli/mod.rs).set_default_headers and default_headers are now pub(crate) (test-only helpers); the engine's client attribution is the only producer of process-wide default headers, and production publishes them together with the user-agent through the atomic set_client_identity. There is therefore no caller-configured layer to discard, and replacing the previous execution's attribution headers on each execute* is exactly the intended behavior. This API was never released, so nothing public changes for existing versions.
2. Factory installed the user-agent before the header map (factory.rs). Confirmed (I mutation-checked: with the old order, a user-agent: sneaky/0 entry replaced the identity on the wire). The header map is now applied first and the identity user-agent last, so it always wins, matching HttpClient. Test added.
Document that blank CI values are treated as false
cli-engine/docs/attribution.md:15
This description conflicts with is_truthy and the new explicit_off_spellings_do_not_count_as_ci test: an empty or whitespace-only CI value is also treated as false, although it is not one of the four values listed here. Document the non-blank requirement so users can predict the emitted mode.
…dentity
`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 <noreply@anthropic.com>
Addressing the "Previously missed" finding from the latest Copilot review (prose-only, no thread). Fixed in ac54564: docs/attribution.md now says the CI variable must be a non-blank value other than 0/false/no/off (case-insensitive), matching is_truthy and the explicit_off_spellings_do_not_count_as_ci test.
This single read is not guaranteed to contain the complete HTTP header block: TCP may split even this small request across reads. The wire assertions can therefore intermittently miss a header, and the server may respond before the request is complete. Read until \r\n\r\n before returning the captured head.
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 <noreply@anthropic.com>
Addressing the "Previously missed" finding from the latest Copilot review (prose-only, no thread). Fixed in 23919e7: the loopback test server's serve_once did a single read, which isn't guaranteed to contain the whole header block. It now reads until the blank line that ends the head, drains any in-flight request body before closing (so the client never sees a reset mid-write), and returns only the head. Ran the wire-level tests 15 times with no failures.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Privacy-facing documentation omits the filesystem marker checks used during agent detection.
0 open findings
Previously missed (2)
In code that hasn't changed since last review
Document filesystem marker checks in AI agent detection privacy explanation
cli-engine/docs/attribution.md:35
The detector is not environment-only: Signals::from_process also supplies Path::exists to is_ai_agent::detect_with, so detection may inspect filesystem markers. This privacy-facing explanation should disclose that existence check rather than saying detection only reads environment variables.
This module documentation says detection reads only environment markers, but from_process also gives the detector a real filesystem-existence lookup. Mentioning filesystem markers here keeps the implementation's privacy behavior accurately documented.
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 <noreply@anthropic.com>
Addressing the two "Previously missed" findings from the latest Copilot review (both prose-only, same point). Fixed in d0d3523. Correct and worth fixing precisely, since this is privacy-facing documentation: detection is not environment-only. I checked the detector's source: it tests exactly one filesystem marker, /opt/.devin (which identifies Devin), and only whether the path exists. docs/attribution.md and the attribution.rs module docs now disclose this, state that no file contents are read and no directories are listed, and note the marker set is defined by the detector crate.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
transport::reqwest_client_builder(): a preconfiguredreqwest::ClientBuilderfor code that needs a plainreqwest::Clientand cannot go throughHttpClient(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.HttpClientnow builds on the same factory.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-Agenttokens:mode/<agent|ci|interactive|script>, plusagent/<slug>when an AI harness is detected.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=1drops the header;AttributionConfig::without_session_id()disables it for a CLI.is-ai-agentcrate (zero dependencies, MIT/Apache). It reads environment variables and tests whether one fixed marker path exists (/opt/.devin, existence only; no contents read, no directories listed). It is cooperative and heuristic, anddocs/attribution.mdsays so.ClientIdentity) rather than adding a third process-global.cli-engine/docs/attribution.md.New public API
transport::{reqwest_client_builder, default_user_agent, DEFAULT_CONNECT_TIMEOUT, DEFAULT_READ_TIMEOUT, AttributionConfig},CliConfig::with_client_attribution,CliConfig::attribution. (Process-wide default headers are deliberately not public API: client attribution is their only producer, and they are published together with the user-agent through an internal atomic setter.)Identity publishing semantics (refined during review)
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.HttpClientonly ever see sendable headers.HttpClientrequest 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 vendorContent-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 transporttrace. A multipart request's generatedContent-Type(it carries the boundary) is never replaced. Inreqwest_client_builder()the identity user-agent is applied last, so a header map can never override it.HttpClientcaptures the process identity once, when its builder is created; its basereqwest::Clientis built from a timeout-only builder and never re-reads process identity.reqwest_client_builder()) is created, create clients inside command handlers, which run afterexecute*has published, not during module registration. Documented indocs/attribution.mdand on the relevant APIs.execute*entrypoints after argv0 resolution, so an argv0 personality (an independentCliwith its own config) publishes its own identity rather than the dispatcher's.set_default_user_agentreplaces the whole published identity: it also clears any headers published by attribution, so a new user-agent is never paired with a previous execution's session header.Cli::runintentionally does not publish, so running aCli(as tests do, concurrently) never mutates process-wide state. This is documented onCli::runand indocs/attribution.md.Behavior changes to review
HttpClientpreviously 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.sha2is no longer an optional dependency (attribution needs it unconditionally); it is removed from thepkce-authfeature list.is-ai-agent0.6.Out of scope / follow-ups
x-client-session) is a placeholder. It is trivially changed in one constant; I'd like reviewer opinion before this ships.traceparentforwarding is intentionally not included.Test plan
cargo fmt --all --checkcargo 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).default_headers(..)adds to (does not replace) the process defaults, an invalid default header is skipped rather than fatal, andHttpClientmerges process defaults under its own headers (client wins on a clash).CI=false/0/offhandling, the opt-out env var and its name derivation, configurable/invalid header names.Cli::client_identitycomposes the base user-agent (including an explicit override) with attribution.HttpClientrequests; an argv0 personality publishes its own identity on the execute path;Cli::runleaves 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:
Test WITHOUT the change (baseline):
# On main: any request carries only `<name>/<version>`; no mode/agent tokens, no session header.Test WITH the change:
Cleanup:
# Remove the [patch.crates-io] block from the consumer's Cargo.toml.🤖 Generated with Claude Code