Skip to content

feat(providers): take extra request headers on the Anthropic and OpenAI builders - #27

Merged
sheperdh merged 3 commits into
memoryboxfrom
feat/anthropic-openai-extra-headers
Sep 14, 2026
Merged

sheperdh merged 3 commits into
memoryboxfrom
feat/anthropic-openai-extra-headers

Conversation

@marvinyu-memverge

@marvinyu-memverge marvinyu-memverge commented Sep 11, 2026

Copy link
Copy Markdown

Summary

  • Base branch: memorybox — this is the MemVerge fork's integration branch, which MemBox pins by commit (agent/Cargo.toml, the three zeroclaw* crates). The template's master rule is the upstream project's; fork changes land on memorybox and are carried upstream separately, so the base here is deliberate.
  • What changed and why:
    • AnthropicBuilder and OpenAiBuilder (chat-completions) gain the extra_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: its x-membox-turn-id / x-membox-call-kind tags 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).
    • The map is validated once at build time (extra_headers.rs, crate-private) into typed (HeaderName, HeaderValue) pairs and stamped on each RequestBuilder, 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 to RUNTIME_PROXY_CLIENT_CACHE per turn. Pooling is unchanged.
    • Reserved names are per provider: Anthropic drops 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 drops Authorization only, so x-api-key / anthropic-version reach 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-beta stays appendable.
    • factory.rs forwards the config-level [providers.models.<alias>].extra_headers to both new builders, which it previously did only for the compatible and Responses branches.
  • Scope boundary: no change to the compatible / Responses providers or to what any provider sends when the map is empty (the default). The three validate-and-warn loops (compatible, Responses, this module) remain separate copies with their own reserved sets — the compatible provider applies no reserved filter at all today, and docs/book/src/providers/configuration.md does not yet list the reserved names per family. One follow-up covers all three: a shared helper, compat's filter, and the doc sentence.
  • Blast radius: zeroclaw-providers only. Hosts that call the two builders (MemBox) opt in explicitly; an operator config that already carried extra_headers under an Anthropic or OpenAI chat-completions alias now has those headers sent, where before they were silently ignored.
  • Linked issue(s): Related MemVerge/MemBox#3621 (consumer; merged, pinned to this branch and to be re-pinned to the memorybox merge commit), Related MemVerge/MemMachine-Platform#1072.
  • Labels: none applied at the time of writing (the fork has no label automation beyond the path labeler, which added none).

Testing (required)

How you can test (when useful)

  • Reviewer testing requested? 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-version toolchain (cargo +1.96.1), on Windows.

  • CI checks relied on and why they cover this change: none cover the code. The Quality Gate workflow (ci.yml: fmt, clippy, tests, comment-hygiene) triggers on pull_request: branches: [master] only, so on a memorybox PR only "Validate PR title" and the path labeler run.
  • Known CI coverage gap, if any: the whole code gate, for the reason above; the local run below on the repo's rust-version toolchain is the evidence. (The comment-hygiene gate was run through its Python filter on the changed files; its shell wrapper needs rg, which this machine lacks.)
  • Commands run and tail output:
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-providers
$ cargo +1.96.1 fmt --all -- --check          # exit 0
$ cargo +1.96.1 clippy -p zeroclaw-providers --tests -- -D warnings
    Checking zeroclaw-providers v0.8.4
    Finished `dev` profile [unoptimized + debuginfo] target(s)   # exit 0
$ cargo +1.96.1 test -p zeroclaw-providers
test result: ok. 1248 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 16.46s
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
$ python3 scripts/ci/comment_hygiene_gate.py <changed files>
Comment hygiene gate passed.
  • Beyond CI, what did you manually verify?
    • On the wire, through each provider's own chat against a loopback server: the tags arrive; a reserved entry in the map (x-api-key / anthropic-version on Anthropic, Authorization on OpenAI) neither displaces nor duplicates the provider's own header; on OpenAI chat-completions a caller's x-api-key is forwarded, matching the Responses wire; a builder without the map stamps nothing.
    • Through FamilyProviderFactory::create_provider with ModelProviderRuntimeOptions.extra_headers, against the same loopback: the config → factory → request boundary MemBox uses.
    • Mutation-checked: deleting the apply_extra_headers call at the request site each provider test drives fails that test; deleting either factory forwarding line fails exactly its factory test.
    • A fresh-context review of the round-1 fix diff added the OAuth path's anthropic-dangerous-direct-browser-access to 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.
    • Not verified: the streaming SSE path end-to-end (the stamp on the stream spawn is the same helper call as the four unary sites, but no loopback test drives a stream).
  • If any command was intentionally skipped, why: none skipped.

Security & Privacy Impact (required)

  • New permissions, capabilities, or file system access scope? No
  • New external network calls? No — the same requests to the same endpoints, with additional headers.
  • Secrets / tokens / credentials handling changed? No — the provider's own credential header stays authoritative: a caller-supplied Authorization / x-api-key copy 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).
  • PII, real identities, or personal data in diff, tests, fixtures, or docs? No
  • Prompt injection or untrusted model-visible text introduced/changed? No
  • If any Yes, 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)

  • Backward compatible? Yes — the builder methods are additive; an empty map is byte-for-byte the previous behavior.
  • Config / env / CLI surface changed? Yes — no new key, but the existing extra_headers config 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.
  • Rust/MSRV/toolchain floor changed? No
  • If backward compatibility is No or either surface/floor question is Yes: 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 the reserved_header_owned_by_provider / Skipping invalid extra header WARNs name any dropped entry.

…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 sheperdh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/zeroclaw-providers/src/extra_headers.rs Outdated
Comment thread crates/zeroclaw-providers/src/factory.rs
…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.
@marvinyu-memverge
marvinyu-memverge force-pushed the feat/anthropic-openai-extra-headers branch from 61f464c to 8ec3ce4 Compare September 14, 2026 18:34
@marvinyu-memverge

Copy link
Copy Markdown
Author

Review round 1 — head 8ec3ce45

Totals: 3 findings — 3 applied, 0 declined, 0 deferred.

  • 🔴 PR metadata: body rewritten to the full template (base-branch note for memorybox, scope boundary, blast radius, Security & Privacy, Compatibility, rollback, label snapshot, local-gate tails).
  • 🟡 Reserved headers are per provider (ReservedHeaders::ANTHROPIC / ::OPENAI + framing); OpenAI chat-completions now forwards x-api-key / anthropic-version like the Responses wire.
  • 🔵 Factory → wire loopback tests for both providers, mutation-checked against the forwarding lines.

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 master only, so a memorybox PR runs just PR-title validation and the path labeler; the fmt / clippy / test evidence is the local run in the body.

Since your review: the three items above, plus a fresh-context review of the fix diff (the OAuth path's anthropic-dangerous-direct-browser-access joined the Anthropic reserved set; the drop WARN names the provider; the Responses factory test shares the new capture helper).

@sheperdh sheperdh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, memorybox base-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-key and anthropic-version; both providers protect authorization and HTTP framing headers as appropriate.
  • Factory-to-wire regression tests now construct each provider through FamilyProviderFactory with ModelProviderRuntimeOptions.extra_headers and 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_headers map 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,248 tests passed) on Windows with Rust 1.96.1.
  • The repository's current PR workflows do not run the full code test suite for the memorybox base 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.

@sheperdh
sheperdh merged commit a8b3b37 into memorybox Sep 14, 2026
3 checks passed
@sheperdh
sheperdh deleted the feat/anthropic-openai-extra-headers branch September 14, 2026 22:12
@yaleMemVerge
yaleMemVerge restored the feat/anthropic-openai-extra-headers branch September 14, 2026 22:44
@yaleMemVerge
yaleMemVerge deleted the feat/anthropic-openai-extra-headers branch September 14, 2026 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants