feat(headers, provider): custom header overrides and Moonshot Kimi provider#487
feat(headers, provider): custom header overrides and Moonshot Kimi provider#487weselben wants to merge 5 commits into
Conversation
Document per-provider custom upstream headers and passthrough user headers under docs/features/custom-headers.mdx. Add the Moonshot Kimi provider page and overview entry. Update CLAUDE.md and config/config.example.yaml references.
Introduce HeaderOverridesConfig, ApplyHeaderOverrides, and per-provider factory wiring. Support static custom headers and optional client-header passthrough with skip/allow lists and a security floor. Gate middleware registration on any provider enabling passthrough_user_headers.
Cover config parsing, factory wiring, core header override logic, server middleware capture, and provider-specific application across OpenAI-compatible providers.
Register the Kimi provider, add an OpenAI-compatible adapter, and enable configuration in examples, Helm values, and the default passthrough provider list.
…pi support Add Kimi unit and contract tests, recorded API fixtures, golden files, and recordapi tooling for generating Kimi testdata.
📝 WalkthroughWalkthroughThis PR introduces configurable per-provider HTTP header overrides (static custom upstream headers and curated client header passthrough with skip/allow modes), wires this through provider config, factory, and individual providers (DeepSeek, Ollama, xAI, vLLM, OpenAI-compatible), adds server-side capture middleware, and integrates a new Kimi provider with docs, config examples, and contract tests. ChangesCustom Upstream Headers & Kimi Provider Integration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PassthroughHeaderCapture
participant Authenticator
participant ProviderFactory
participant Provider
Client->>PassthroughHeaderCapture: HTTP request with headers
PassthroughHeaderCapture->>PassthroughHeaderCapture: FilterIncomingHeaders(userPathAlias)
PassthroughHeaderCapture->>Authenticator: request with filtered headers in context
Authenticator->>ProviderFactory: authenticated request
ProviderFactory->>Provider: Create(cfg, opts with HeaderOverrides)
Provider->>Provider: ApplyHeaderOverrides(request, cfg, alias)
Provider-->>Client: upstream response
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (r *staticRewriter) Rewrite(context.Context, ext.Input) (*ext.Result, error) { | ||
| return nil, nil | ||
| func TestAnyProviderHasPassthroughUserHeaders_True(t *testing.T) { |
There was a problem hiding this comment.
staticRewriter.Rewrite is never closed before the new test declaration, so this file will not parse and the package cannot compile. The changed code starts func TestAnyProviderHasPassthroughUserHeaders_True immediately after return nil, nil inside the previous function body.
Artifacts
Repro: focused Go test output showing app_test.go syntax parse failure
- Keeps the command output available without making the summary code-heavy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/providers/openai/compatible_provider.go (2)
56-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the duplicated header-override application logic.
Both
NewCompatibleProviderandNewCompatibleProviderWithHTTPClientbuild an identical request-customization closure (cfg.SetHeaders(...)thenproviders.ApplyHeaderOverrides(...)). A shared helper would prevent divergence if this logic changes again.♻️ Proposed helper extraction
+func applyRequestHeaders(req *http.Request, cfg CompatibleProviderConfig, apiKey string) { + if cfg.SetHeaders != nil { + cfg.SetHeaders(req, apiKey) + } + if cfg.HeaderOverrides != nil { + providers.ApplyHeaderOverrides(req, *cfg.HeaderOverrides, cfg.UserPathAlias) + } +}Then both constructors' closures become
func(req *http.Request) { applyRequestHeaders(req, cfg, apiKey) }.Also applies to: 78-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/openai/compatible_provider.go` around lines 56 - 63, The request header customization logic is duplicated in both compatible provider constructors, so extract it into a shared helper to keep behavior consistent. Add a helper used by NewCompatibleProvider and NewCompatibleProviderWithHTTPClient that applies cfg.SetHeaders and providers.ApplyHeaderOverrides in the same order, then have each llmclient.New closure delegate to that helper with req, cfg, and apiKey.
67-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate
headerOverridesinto the compatible client
NewWithHTTPClientstores these onProvider, butopenai.NewCompatibleProviderWithHTTPClient(...)never receives them, so vLLM/v1/chat/completions-style requests skip custom headers when this constructor is used. PassheaderOverridesanduserPathAliasinto theCompatibleProviderConfighere as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providers/openai/compatible_provider.go` around lines 67 - 87, `NewCompatibleProviderWithHTTPClient` is not forwarding the compatible client’s custom header settings, so requests built through `CompatibleProvider` can miss overrides. Update the `CompatibleProviderConfig` flow in `NewCompatibleProviderWithHTTPClient` so it carries both `HeaderOverrides` and `UserPathAlias`, and ensure the existing request setup in `llmclient.NewWithHTTPClient` continues to apply them via `providers.ApplyHeaderOverrides` alongside `SetHeaders`.internal/app/app.go (1)
173-190: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMove
SetUserPathHeaderbeforeproviders.Init.providers.Initbuilds the long-lived providers throughfactory.Create, andCreatesnapshotsUserPathHeaderduring construction. Leaving this after init leaves customServer.UserPathHeaderdeployments with an empty alias, so the internal identity header can bypass the blocked-header floor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/app/app.go` around lines 173 - 190, Move the cfg.Factory.SetUserPathHeader call to before providers.Init in app initialization so factory.Create snapshots the configured Server.UserPathHeader during provider construction; update the init flow in app startup around providers.Init and anyProviderHasPassthroughUserHeaders so custom user-path aliases are applied before long-lived providers are built.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/app/app_test.go`:
- Around line 647-677: The staticRewriter.Rewrite method is missing its closing
brace before the new test functions, which breaks compilation. Close the Rewrite
function immediately after the return nil, nil statement, then keep the new
anyProviderHasPassthroughUserHeaders tests below it so the existing
staticRewriter and test declarations remain properly separated.
In `@internal/providers/config.go`:
- Around line 424-426: The `overlayProviderEnvVars` handling for
`PassthroughUserHeaders` is currently enable-only, so an env var cannot force
the field back to false once YAML has enabled it. Update the config merge logic
in `overlayProviderEnvVars` (and any related boolean parsing in `parseBool`) so
the env var can explicitly override the YAML value in both directions if that is
the intended behavior, or otherwise document and enforce the one-way semantics
consistently for `PassthroughUserHeaders`.
- Around line 729-737: Validate PassthroughUserHeadersSkipMode before it is
persisted instead of only normalizing empty input in defaultPassthroughSkipMode.
Update the config path in internal/providers/config.go where this value is
assigned so unknown or misspelled modes are rejected or at least warned about,
and ensure shouldForward only receives approved values like skip or only rather
than silently accepting arbitrary strings. Keep the existing empty-value
fallback behavior, but add explicit validation at the
PassthroughUserHeadersSkipMode storage/parse point.
In `@internal/providers/headers.go`:
- Around line 88-97: The ignored custom headers message in the passthrough
branch of applyPassthroughHeaders should be elevated from debug to warn so
operators can see misconfigurations in production. Update the slog call in
internal/providers/headers.go to use a warning level while keeping the same
context message, and adjust the corresponding expectation in headers_test.go,
including TestApplyHeaderOverrides_PassthroughMode_LogsIgnoredCustomHeaders, to
assert the new warning level instead of slog.LevelDebug.
In `@run/lifecycle_test.go`:
- Around line 149-176: The current TestMain_SetUserPathHeaderWiring only checks
that providers.NewProviderFactory().SetUserPathHeader(...) does not panic, which
is a weak assertion because SetUserPathHeader appears to just store the value.
Update the test to verify observable behavior: after calling SetUserPathHeader
on the ProviderFactory, assert the header is actually retained or used through a
getter or downstream effect tied to that factory, or remove the test if no
meaningful assertion is available yet.
In `@tests/contract/kimi_test.go`:
- Around line 27-56: The TestKimiReplayChatCompletion subtests for "params" and
"tools" currently duplicate the "basic" request and only swap fixture names, so
they do not exercise request translation or tool-call handling. Update the test
cases in TestKimiReplayChatCompletion to send distinct core.ChatRequest inputs
for the "params" and "tools" cases by setting provider-specific fields such as
Temperature, TopP, and Tools, and use fixtures that actually reflect those
behaviors (for example, a response with tool_calls) so the replay tests validate
the Kimi provider’s parameter mapping and response parsing.
---
Outside diff comments:
In `@internal/app/app.go`:
- Around line 173-190: Move the cfg.Factory.SetUserPathHeader call to before
providers.Init in app initialization so factory.Create snapshots the configured
Server.UserPathHeader during provider construction; update the init flow in app
startup around providers.Init and anyProviderHasPassthroughUserHeaders so custom
user-path aliases are applied before long-lived providers are built.
In `@internal/providers/openai/compatible_provider.go`:
- Around line 56-63: The request header customization logic is duplicated in
both compatible provider constructors, so extract it into a shared helper to
keep behavior consistent. Add a helper used by NewCompatibleProvider and
NewCompatibleProviderWithHTTPClient that applies cfg.SetHeaders and
providers.ApplyHeaderOverrides in the same order, then have each llmclient.New
closure delegate to that helper with req, cfg, and apiKey.
- Around line 67-87: `NewCompatibleProviderWithHTTPClient` is not forwarding the
compatible client’s custom header settings, so requests built through
`CompatibleProvider` can miss overrides. Update the `CompatibleProviderConfig`
flow in `NewCompatibleProviderWithHTTPClient` so it carries both
`HeaderOverrides` and `UserPathAlias`, and ensure the existing request setup in
`llmclient.NewWithHTTPClient` continues to apply them via
`providers.ApplyHeaderOverrides` alongside `SetHeaders`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2be12b8-ddba-416c-90c0-875d52322990
📒 Files selected for processing (57)
CLAUDE.mdcmd/recordapi/main.gocmd/recordapi/main_test.goconfig/config.example.yamlconfig/config_example_test.goconfig/config_test.goconfig/providers.goconfig/providers_test.goconfig/server.godocs/docs.jsondocs/features/custom-headers.mdxdocs/providers/kimi.mdxdocs/providers/overview.mdxhelm/values.yamlinternal/app/app.gointernal/app/app_test.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/deepseek/deepseek.gointernal/providers/deepseek/deepseek_test.gointernal/providers/factory.gointernal/providers/factory_test.gointernal/providers/groq/groq_test.gointernal/providers/headers.gointernal/providers/headers_test.gointernal/providers/init.gointernal/providers/kimi/kimi.gointernal/providers/kimi/kimi_test.gointernal/providers/ollama/ollama.gointernal/providers/ollama/ollama_test.gointernal/providers/openai/compatible_provider.gointernal/providers/openai/compatible_provider_test.gointernal/providers/vllm/vllm.gointernal/providers/vllm/vllm_test.gointernal/providers/xai/xai.gointernal/providers/xai/xai_test.gointernal/server/http.gointernal/server/http_test.gointernal/server/passthrough_headers.gointernal/server/passthrough_headers_test.gorun/lifecycle_test.gorun/providers.gorun/providers_test.gotests/contract/kimi_test.gotests/contract/testdata/golden/kimi/chat_completion.golden.jsontests/contract/testdata/golden/kimi/chat_completion_stream.golden.jsontests/contract/testdata/golden/kimi/chat_with_params.golden.jsontests/contract/testdata/golden/kimi/chat_with_tools.golden.jsontests/contract/testdata/golden/kimi/embeddings.golden.jsontests/contract/testdata/golden/kimi/models.golden.jsontests/contract/testdata/kimi/chat_completion.jsontests/contract/testdata/kimi/chat_completion_stream.txttests/contract/testdata/kimi/chat_with_params.jsontests/contract/testdata/kimi/chat_with_tools.jsontests/contract/testdata/kimi/embeddings.jsontests/contract/testdata/kimi/models.jsontests/contract/xai_test.go
| func (r *staticRewriter) Rewrite(context.Context, ext.Input) (*ext.Result, error) { | ||
| return nil, nil | ||
| func TestAnyProviderHasPassthroughUserHeaders_True(t *testing.T) { | ||
| resolved := map[string]providers.ProviderConfig{ | ||
| "openai": {Type: "openai", PassthroughUserHeaders: false}, | ||
| "anthropic": {Type: "anthropic", PassthroughUserHeaders: true}, | ||
| "groq": {Type: "groq", PassthroughUserHeaders: false}, | ||
| } | ||
| if !anyProviderHasPassthroughUserHeaders(resolved) { | ||
| t.Fatal("expected true when at least one provider has passthrough_user_headers=true") | ||
| } | ||
| } | ||
|
|
||
| func TestAnyProviderHasPassthroughUserHeaders_False(t *testing.T) { | ||
| resolved := map[string]providers.ProviderConfig{ | ||
| "openai": {Type: "openai", PassthroughUserHeaders: false}, | ||
| "anthropic": {Type: "anthropic", PassthroughUserHeaders: false}, | ||
| } | ||
| if anyProviderHasPassthroughUserHeaders(resolved) { | ||
| t.Fatal("expected false when no provider has passthrough_user_headers=true") | ||
| } | ||
| } | ||
|
|
||
| func TestAnyProviderHasPassthroughUserHeaders_EmptyMap(t *testing.T) { | ||
| if anyProviderHasPassthroughUserHeaders(nil) { | ||
| t.Fatal("expected false for nil resolved provider map") | ||
| } | ||
| if anyProviderHasPassthroughUserHeaders(map[string]providers.ProviderConfig{}) { | ||
| t.Fatal("expected false for empty resolved provider map") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A2 'func \(r \*staticRewriter\) Rewrite' internal/app/app_test.go
rg -n -B2 -A2 'func TestAnyProviderHasPassthroughUserHeaders_True' internal/app/app_test.goRepository: ENTERPILOT/GoModel
Length of output: 704
🏁 Script executed:
#!/bin/bash
sed -n '640,660p' internal/app/app_test.goRepository: ENTERPILOT/GoModel
Length of output: 892
Close staticRewriter.Rewrite before the new tests internal/app/app_test.go:647-649 — add the missing } after return nil, nil; without it, the package won’t compile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/app/app_test.go` around lines 647 - 677, The staticRewriter.Rewrite
method is missing its closing brace before the new test functions, which breaks
compilation. Close the Rewrite function immediately after the return nil, nil
statement, then keep the new anyProviderHasPassthroughUserHeaders tests below it
so the existing staticRewriter and test declarations remain properly separated.
| if values.PassthroughUserHeaders { | ||
| existing.PassthroughUserHeaders = true | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Env override for PassthroughUserHeaders is enable-only.
overlayProviderEnvVars only sets existing.PassthroughUserHeaders = true when the env var is truthy; there's no path for the env var to force it back to false when YAML has already enabled it. Combined with parseBool treating false/unset identically, an operator cannot use the env var alone to disable passthrough for a provider that has it enabled via YAML — they'd need to edit the YAML. This may be intentional (mirrors the "env var alone shouldn't discover a provider" design), but worth confirming this is the desired operational behavior, since passthrough-header forwarding is security-sensitive and ops may want a fast env-only kill switch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/config.go` around lines 424 - 426, The
`overlayProviderEnvVars` handling for `PassthroughUserHeaders` is currently
enable-only, so an env var cannot force the field back to false once YAML has
enabled it. Update the config merge logic in `overlayProviderEnvVars` (and any
related boolean parsing in `parseBool`) so the env var can explicitly override
the YAML value in both directions if that is the intended behavior, or otherwise
document and enforce the one-way semantics consistently for
`PassthroughUserHeaders`.
| // defaultPassthroughSkipMode returns "skip" when the raw value is empty, and | ||
| // otherwise passes through the operator-supplied mode unchanged. | ||
| func defaultPassthroughSkipMode(raw string) string { | ||
| if strings.TrimSpace(raw) == "" { | ||
| return "skip" | ||
| } | ||
| return raw | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file first.
ast-grep outline internal/providers/config.go --view expanded || true
ast-grep outline internal/providers/headers.go --view expanded || true
ast-grep outline internal/providers/config_test.go --view expanded || true
# Locate the fields, defaults, and any exact-mode matching.
rg -n "PassthroughUserHeadersSkipMode|defaultPassthroughSkipMode|skip mode|only|skip" internal/providers/config.go internal/providers/headers.go internal/providers/config_test.goRepository: ENTERPILOT/GoModel
Length of output: 16499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '724,739p' internal/providers/config.go
printf '\n---\n'
sed -n '128,149p' internal/providers/headers.go
printf '\n---\n'
sed -n '90,150p' internal/providers/config_test.goRepository: ENTERPILOT/GoModel
Length of output: 3767
Validate PassthroughUserHeadersSkipMode before storing it. defaultPassthroughSkipMode only fills the empty case; any typo or unsupported value is preserved and later treated as skip by shouldForward, which can silently widen an intended only/allow-list setup. Reject or warn on unknown values instead of accepting them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/config.go` around lines 729 - 737, Validate
PassthroughUserHeadersSkipMode before it is persisted instead of only
normalizing empty input in defaultPassthroughSkipMode. Update the config path in
internal/providers/config.go where this value is assigned so unknown or
misspelled modes are rejected or at least warned about, and ensure shouldForward
only receives approved values like skip or only rather than silently accepting
arbitrary strings. Keep the existing empty-value fallback behavior, but add
explicit validation at the PassthroughUserHeadersSkipMode storage/parse point.
| if cfg.PassthroughUserHeaders { | ||
| // Log if custom headers ignored due to passthrough | ||
| if len(cfg.CustomUpstreamHeaders) > 0 { | ||
| slog.Debug("custom_upstream_headers ignored because passthrough_user_headers is active") | ||
| } | ||
| applyPassthroughHeaders(req, cfg, userPathAlias) | ||
| } else if len(cfg.CustomUpstreamHeaders) > 0 { | ||
| applyStaticHeaders(req, cfg.CustomUpstreamHeaders, userPathAlias) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider slog.Warn instead of slog.Debug for the ignored-config log.
This log is the only signal that an operator's custom_upstream_headers config is being silently dropped because passthrough_user_headers is active. Debug logging is typically disabled in production, so this misconfiguration (which can cause required upstream headers to silently go missing) would go unnoticed without enabling debug logs to investigate.
♻️ Proposed fix
if cfg.PassthroughUserHeaders {
// Log if custom headers ignored due to passthrough
if len(cfg.CustomUpstreamHeaders) > 0 {
- slog.Debug("custom_upstream_headers ignored because passthrough_user_headers is active")
+ slog.Warn("custom_upstream_headers ignored because passthrough_user_headers is active")
}Note: headers_test.go asserts slog.LevelDebug specifically (e.g. TestApplyHeaderOverrides_PassthroughMode_LogsIgnoredCustomHeaders), so that assertion would need updating alongside this change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg.PassthroughUserHeaders { | |
| // Log if custom headers ignored due to passthrough | |
| if len(cfg.CustomUpstreamHeaders) > 0 { | |
| slog.Debug("custom_upstream_headers ignored because passthrough_user_headers is active") | |
| } | |
| applyPassthroughHeaders(req, cfg, userPathAlias) | |
| } else if len(cfg.CustomUpstreamHeaders) > 0 { | |
| applyStaticHeaders(req, cfg.CustomUpstreamHeaders, userPathAlias) | |
| } | |
| } | |
| if cfg.PassthroughUserHeaders { | |
| // Log if custom headers ignored due to passthrough | |
| if len(cfg.CustomUpstreamHeaders) > 0 { | |
| slog.Warn("custom_upstream_headers ignored because passthrough_user_headers is active") | |
| } | |
| applyPassthroughHeaders(req, cfg, userPathAlias) | |
| } else if len(cfg.CustomUpstreamHeaders) > 0 { | |
| applyStaticHeaders(req, cfg.CustomUpstreamHeaders, userPathAlias) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/providers/headers.go` around lines 88 - 97, The ignored custom
headers message in the passthrough branch of applyPassthroughHeaders should be
elevated from debug to warn so operators can see misconfigurations in
production. Update the slog call in internal/providers/headers.go to use a
warning level while keeping the same context message, and adjust the
corresponding expectation in headers_test.go, including
TestApplyHeaderOverrides_PassthroughMode_LogsIgnoredCustomHeaders, to assert the
new warning level instead of slog.LevelDebug.
| func TestMain_SetUserPathHeaderWiring(t *testing.T) { | ||
| // Verify SetUserPathHeader exists and can be called without panic | ||
| factory := providers.NewProviderFactory() | ||
|
|
||
| // Test that SetUserPathHeader can be called with various valid headers | ||
| headers := []string{ | ||
| "X-GoModel-User-Path", | ||
| "X-Tenant-Path", | ||
| "X-Custom-Header", | ||
| "", // empty string should not panic | ||
| } | ||
|
|
||
| for _, header := range headers { | ||
| didPanic := false | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| didPanic = true | ||
| t.Errorf("SetUserPathHeader(%q) panicked: %v", header, r) | ||
| } | ||
| }() | ||
| factory.SetUserPathHeader(header) | ||
| }() | ||
| if didPanic { | ||
| continue | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Weak assertion — test can never fail meaningfully.
SetUserPathHeader (per internal/providers/factory.go) unconditionally stores the header with no validation, so it can never panic; this test only proves that trivial fact and doesn't verify the header is actually retained/used. Consider asserting the stored value is later reflected somewhere observable (e.g., via a getter or downstream effect), or drop it if there's no such assertion possible yet.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@run/lifecycle_test.go` around lines 149 - 176, The current
TestMain_SetUserPathHeaderWiring only checks that
providers.NewProviderFactory().SetUserPathHeader(...) does not panic, which is a
weak assertion because SetUserPathHeader appears to just store the value. Update
the test to verify observable behavior: after calling SetUserPathHeader on the
ProviderFactory, assert the header is actually retained or used through a getter
or downstream effect tied to that factory, or remove the test if no meaningful
assertion is available yet.
| func TestKimiReplayChatCompletion(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| fixturePath string | ||
| }{ | ||
| {name: "basic", fixturePath: "kimi/chat_completion.json"}, | ||
| {name: "params", fixturePath: "kimi/chat_with_params.json"}, | ||
| {name: "tools", fixturePath: "kimi/chat_with_tools.json"}, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| provider := newKimiReplayProvider(t, map[string]replayRoute{ | ||
| replayKey(http.MethodPost, "/chat/completions"): jsonFixtureRoute(t, tc.fixturePath), | ||
| }) | ||
|
|
||
| resp, err := provider.ChatCompletion(context.Background(), &core.ChatRequest{ | ||
| Model: "kimi-k2-0711", | ||
| Messages: []core.Message{{ | ||
| Role: "user", | ||
| Content: "hello", | ||
| }}, | ||
| }) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, resp) | ||
|
|
||
| compareGoldenJSON(t, goldenPathForFixture(tc.fixturePath), resp) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"params"/"tools" subtests don't test params or tools.
All three subtests send an identical core.ChatRequest (same model, same plain message) — only the mocked fixture path differs by name. Their fixtures (chat_with_params.json, chat_with_tools.json) are also byte-identical to chat_completion.json, with no tool_calls, no distinguishing usage/finish_reason, nothing param-specific. As written, the "params" and "tools" cases are duplicates of "basic" in disguise and don't verify request translation or tool-call parsing.
Consider sending requests with actual differentiating fields (e.g. Tools, Temperature/TopP) for the "params"/"tools" cases, and recording distinct upstream fixtures (e.g. a response containing tool_calls) to genuinely exercise that code path.
Based on path instructions: "Tests should cover request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/contract/kimi_test.go` around lines 27 - 56, The
TestKimiReplayChatCompletion subtests for "params" and "tools" currently
duplicate the "basic" request and only swap fixture names, so they do not
exercise request translation or tool-call handling. Update the test cases in
TestKimiReplayChatCompletion to send distinct core.ChatRequest inputs for the
"params" and "tools" cases by setting provider-specific fields such as
Temperature, TopP, and Tools, and use fixtures that actually reflect those
behaviors (for example, a response with tool_calls) so the replay tests validate
the Kimi provider’s parameter mapping and response parsing.
Source: Path instructions
|
After a discussion with Fable 5 — summarized by AI. This needs a round of changes before it can merge. Findings below, ordered by severity. Blockers1. The branch doesn't compile and CI is red. 2. The passthrough engine bypasses the tagging
The engine needs to respect the tagging strip set ( 3. Forwarding Design changes4. The feature applies to far more providers than the PR and docs claim. 5. 6. Default-open forwarding is a risky default. 7. 8. The claimed passthrough defaults don't match the code. Kimi provider: naming, default URL, and docs accuracyWe looked at Kimi's official documentation. There are two distinct products:
The wire dialect is the same OpenAI-compatible format, so a separate provider type isn't justified — but defaulting a type named
Related docs/tooling fixes:
Smaller cleanups
Suggestion: split the PRWith the spoofed headers removed, the Kimi provider doesn't depend on the header-override feature at all. Splitting this into (a) the Kimi provider and (b) the header-override engine would make both much easier to land — (a) is close to ready once naming/docs are settled; (b) needs the security and composition rework above. |
2 participants
This PR adds per-provider HTTP header overrides and a new Moonshot Kimi provider.
Custom upstream / passthrough headers
A new
HeaderOverridesConfigengine lets operators control how headers flow to upstream providers.custom_upstream_headers: staticmap[string]stringheaders attached to every request for a provider.passthrough_user_headers: forwards selected client headers upstream.passthrough_user_headers_skip+passthrough_user_headers_skip_mode: configure a skip or allow list (default:skip).Authorization,Cookie,X-Api-Key, etc.), the internalX-GoModel-User-Pathheader, and any configureduserPathAlias.Example:
The feature is wired into the factory and OpenAI-compatible adapter, and is applied by DeepSeek, Groq, Ollama, vLLM, xAI, and the new Kimi provider. The server middleware that captures passthrough headers is gated so the default hot path is unchanged when no provider opts in.
Moonshot Kimi provider
internal/providers/kimipackage wrapping the OpenAI-compatible client.https://api.kimi.com/coding/v1.custom_upstream_headersandpassthrough_user_headersvia the shared override engine.recordapitooling for generating Kimi test fixtures.Known Limitations
Commits
Future scope
The following native / non-OpenAI-compatible providers are not covered by the header override feature in this PR, but could be wired in later if the same configuration surface is desired:
Each provider uses its own SDK or native request path, so adding support requires a per-provider audit of how headers are constructed before forwarding. OpenRouter was intentionally left out of this PR because it already sets its own attribution headers (
HTTP-Referer,X-OpenRouter-Title, etc.) and changing that behavior was out of scope. OpenAI-compatible providers (including Kimi, OpenAI, DeepSeek, Groq, Ollama, vLLM, xAI, Fireworks AI, and Oracle) already inherit the feature through the shared adapter.Summary by CodeRabbit
New Features
Bug Fixes