feat(providers): take extra request headers on the Anthropic and OpenAI builders - #27
Conversation
…AI builders AnthropicBuilder and OpenAiBuilder gain the extra_headers(HashMap) chain method the compatible and Responses builders already have. The pairs are validated once at build time (reserved credential/framing names and invalid entries dropped with a WARN) and stamped per request on the providers' pooled runtime-proxy client rather than as default headers on a dedicated client, so a host that builds one provider per turn with a per-turn correlation header does not grow the client cache by one client per turn.
…version, forward config headers Review of the previous commit: the two provider tests hand-composed the request and never drove chat(), so a dropped apply at a request site stayed green - they now capture the request on a loopback server. anthropic-version is single-valued and joins the reserved list. The factory forwards the config-level extra_headers to the two new builders, closing the gap where an operator's [providers.models.anthropic.*] extra_headers reached nothing. Doc wording on the builders now describes the actual drop rule.
sheperdh
left a comment
There was a problem hiding this comment.
Review summary
Reviewed head 61f464c.
The implementation delivers the MemBox dependency described in this PR: ModelProviderRuntimeOptions.extra_headers is forwarded through the Anthropic and OpenAI chat-completions factories, validated once when each provider is built, and applied at the outbound request sites. The linked MemBox zeroclaw-labs#3621 correlation headers therefore reach the Anthropic-native and cache-aware OpenAI calls required by MemMachine-Platform zeroclaw-labs#1072.
I did not find a functional regression in the existing empty-header path. The added per-request work is a short iteration over already-validated header pairs, while both providers continue using the shared runtime-proxy clients; the Anthropic streaming task clones only that small typed vector. This avoids per-turn client/cache growth and preserves connection pooling.
The provider-level wire tests cover successful propagation and credential-header protection. I am leaving two inline notes: one API-consistency warning about sharing Anthropic-only reserved names with OpenAI, and one non-blocking request for factory-boundary regression coverage.
🔴 Blocking — Complete the repository-required PR metadata
The live PR body does not satisfy the current .github/pull_request_template.md: it omits the required Security & Privacy Impact, Compatibility, rollback, label snapshot, and concrete CI/output evidence. Those are substantive here because this patch changes caller-controlled outbound network headers next to provider credentials. The body and both commit tails also contain Claude attribution, which the repository's AGENTS.md and template explicitly prohibit.
Please update the public artifacts before approval: document that header values are caller/config supplied, values are not logged, credential/protocol headers are protected, the builder API is additive, an empty map preserves prior behavior, and rollback is a revert; include the validation evidence actually run; remove the AI-attribution footer/session link and prohibited AI co-author trailers. The memorybox base also differs from the template's master rule, so the body should explicitly explain the fork-specific base if that is intentional.
Decision
Request changes for the repository-policy/public-artifact blocker above. The MemBox correlation-header behavior itself is implemented correctly; the two inline code notes are non-blocking.
…ire handoff The reserved list was shared, so the OpenAI chat-completions provider dropped x-api-key and anthropic-version, which it never sets itself, while the Responses provider forwards them; switching wire_api changed what a custom gateway received. ReservedHeaders is now per provider (Anthropic: credential, API version and the OAuth browser-access flag; OpenAI: the bearer credential only), each plus framing, and the drop WARN names the provider. Two loopback tests build through FamilyProviderFactory with ModelProviderRuntimeOptions.extra_headers and assert on the wire, sharing one capture server with the Responses test.
61f464c to
8ec3ce4
Compare
|
Review round 1 — head Totals: 3 findings — 3 applied, 0 declined, 0 deferred.
Not done here: the compatible / Responses / new-module validate-and-warn loops stay three copies (named follow-up). CI note: the fork's Quality Gate workflow triggers on Since your review: the three items above, plus a fresh-context review of the fix diff (the OAuth path's |
sheperdh
left a comment
There was a problem hiding this comment.
Re-review summary
Reviewed the updated PR at head 8ec3ce457c41df217b58c4c06f084db6e7ec80f2.
The issues raised in the previous review have been resolved:
- The PR description now follows the repository template and documents the scope,
memoryboxbase-branch rationale, linked requirements, security/privacy considerations, compatibility impact, validation evidence, and rollback plan. The prohibited AI attribution/session metadata has also been removed from the PR and commit metadata. - Reserved-header filtering is now provider-specific. Anthropic still protects its credential and protocol headers, while OpenAI preserves legitimate custom headers such as
x-api-keyandanthropic-version; both providers protect authorization and HTTP framing headers as appropriate. - Factory-to-wire regression tests now construct each provider through
FamilyProviderFactorywithModelProviderRuntimeOptions.extra_headersand verify the resulting HTTP request, covering the configuration path that MemBox actually uses.
Correctness and consistency
- Runtime extra headers are applied to the intended Anthropic and OpenAI chat request paths, including streaming requests and warm-up requests.
- Header names and values are parsed when the provider is created, so malformed configuration fails early rather than during each request.
- User-supplied headers cannot override protected credentials or framing headers.
- The OpenAI behavior is now consistent with the existing Responses provider for legitimate non-credential headers.
- The implementation satisfies the linked MemBox correlation-header requirement without changing the public provider interfaces.
Side effects and performance
- An empty
extra_headersmap preserves the previous request behavior. - Existing shared HTTP clients, proxy handling, and client caching remain intact.
- The added request-time cost is limited to iterating over a small pre-validated header collection; the streaming path clones that small collection when constructing the stream. I found no material performance regression.
- No secret header values are logged.
Validation
- The PR documents successful formatting, clippy, and crate-test validation (
1,248tests passed) on Windows with Rust1.96.1. - The repository's current PR workflows do not run the full code test suite for the
memoryboxbase branch, so the supplied local evidence is relevant and transparent. - There is no dedicated end-to-end wire test for the streaming branch, but it invokes the same tested header-application helper at the request-construction point. I consider that a non-blocking coverage gap, not a correctness defect.
Decision
Approved. I found no remaining blocking correctness, consistency, compatibility, security, or performance issue in the updated patch.
Summary
memorybox— this is the MemVerge fork's integration branch, which MemBox pins by commit (agent/Cargo.toml, the threezeroclaw*crates). The template'smasterrule is the upstream project's; fork changes land onmemoryboxand are carried upstream separately, so the base here is deliberate.AnthropicBuilderandOpenAiBuilder(chat-completions) gain theextra_headers(HashMap<String, String>)chain method the compatible and Responses builders already have, so a host can stamp per-request correlation headers on the Anthropic-native and cache-aware OpenAI routes too. MemBox needs this for MemVerge/MemBox#3621: itsx-membox-turn-id/x-membox-call-kindtags reached the platform gateway on the compatible and Responses branches but not on the two branches that had no hook, so a turn on a Claude model or the cache-aware route wrote tagged sub-call rows with no tagged main row (MemMachine-Platform#1072's turn cost and main-vs-sub split).extra_headers.rs, crate-private) into typed(HeaderName, HeaderValue)pairs and stamped on eachRequestBuilder, not baked into a dedicated client's default headers. Both providers take their client from the shared runtime-proxy cache, and a host legitimately builds one provider per turn with a per-turn header value; keying a cached client on that value would add a client toRUNTIME_PROXY_CLIENT_CACHEper turn. Pooling is unchanged.Authorization/x-api-key/anthropic-version/anthropic-dangerous-direct-browser-access(the credential, API version and OAuth-path flag it sets itself); OpenAI chat-completions dropsAuthorizationonly, sox-api-key/anthropic-versionreach a custom gateway on both OpenAI wires alike. Framing (content-type,content-length,host) and invalid entries are dropped for every provider, with a WARN, the same tolerance the compatible provider has.anthropic-betastays appendable.factory.rsforwards the config-level[providers.models.<alias>].extra_headersto both new builders, which it previously did only for the compatible and Responses branches.docs/book/src/providers/configuration.mddoes not yet list the reserved names per family. One follow-up covers all three: a shared helper, compat's filter, and the doc sentence.zeroclaw-providersonly. Hosts that call the two builders (MemBox) opt in explicitly; an operator config that already carriedextra_headersunder an Anthropic or OpenAI chat-completions alias now has those headers sent, where before they were silently ignored.memoryboxmerge commit), Related MemVerge/MemMachine-Platform#1072.Testing (required)
How you can test (when useful)
N/A— the behavior is a request header on the wire, and the loopback tests below capture exactly that; there is no user-visible surface to click through.How I tested
All checks on the repo's
rust-versiontoolchain (cargo +1.96.1), on Windows.ci.yml: fmt, clippy, tests, comment-hygiene) triggers onpull_request: branches: [master]only, so on amemoryboxPR only "Validate PR title" and the path labeler run.rust-versiontoolchain is the evidence. (The comment-hygiene gate was run through its Python filter on the changed files; its shell wrapper needsrg, which this machine lacks.)cargo +1.96.1 fmt --all -- --check cargo +1.96.1 clippy -p zeroclaw-providers --tests -- -D warnings cargo +1.96.1 test -p zeroclaw-providerschatagainst a loopback server: the tags arrive; a reserved entry in the map (x-api-key/anthropic-versionon Anthropic,Authorizationon OpenAI) neither displaces nor duplicates the provider's own header; on OpenAI chat-completions a caller'sx-api-keyis forwarded, matching the Responses wire; a builder without the map stamps nothing.FamilyProviderFactory::create_providerwithModelProviderRuntimeOptions.extra_headers, against the same loopback: the config → factory → request boundary MemBox uses.apply_extra_headerscall at the request site each provider test drives fails that test; deleting either factory forwarding line fails exactly its factory test.anthropic-dangerous-direct-browser-accessto the Anthropic reserved set, an abort-on-drop guard for the loopback server, and routed the existing Responses factory test through the same capture helper.Security & Privacy Impact (required)
NoNo— the same requests to the same endpoints, with additional headers.No— the provider's own credential header stays authoritative: a caller-suppliedAuthorization/x-api-keycopy is dropped at build time, never appended beside the real one. Header values are caller/config supplied and are not logged (the WARN on a dropped or invalid entry records the header name only).NoNoYes, describe the risk and mitigation: n/a. Residual note: a header value that a host sets from untrusted input is that host's concern; this crate validates it as an HTTP header value and forwards it.Compatibility (required)
Yes— the builder methods are additive; an empty map is byte-for-byte the previous behavior.Yes— no new key, but the existingextra_headersconfig key now takes effect for Anthropic and OpenAI chat-completions aliases, where it was previously accepted and ignored. An operator who set it there and relied on it doing nothing should remove it; no other step.NoNoor either surface/floor question isYes: exact upgrade steps for existing users: none required; see the config note above.Rollback (required for medium/high-risk PRs)
Low-risk:
git revert <merge sha>. There is no flag; the feature is off whenever the map is empty, which is the default for every existing caller. Observable symptom if a header were wrong: the gateway's request log would show the unexpected name, and thereserved_header_owned_by_provider/Skipping invalid extra headerWARNs name any dropped entry.