feat: manage provider custom headers via PATCH and ocx provider edit --headers - #961
feat: manage provider custom headers via PATCH and ocx provider edit --headers#961Yuxin-Qiao wants to merge 2 commits into
Conversation
PATCH /api/providers now accepts a headers field, shallow-merged onto the existing block (null or {} clears it), and ocx provider edit gains --headers <json>. Custom providers such as Agent Router can restore required fingerprint headers through the management plane instead of hand-editing config.json. POST /api/providers already round-trips headers; validation reuses providerHeadersConfigError.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe provider edit CLI now accepts ChangesProvider header management
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as ocx provider edit
participant Runtime as provider-runtime.ts
participant API as PATCH /api/providers
participant Route as provider-routes.ts
participant State as Provider state and catalog
CLI->>Runtime: Read --headers <json>
Runtime->>Runtime: Parse or reject header value
Runtime->>API: Send headers patch
API->>Route: Validate and apply update
Route->>State: Persist headers and refresh catalog
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f2eb0f53d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const authMode = cleared(takeOption(args, "--auth-mode")); | ||
| const note = cleared(takeOption(args, "--note")); | ||
| const apiKeyTransport = cleared(takeOption(args, "--api-key-transport")); | ||
| const headers = takeOption(args, "--headers"); |
There was a problem hiding this comment.
When a user uses the common --headers={...} spelling or repeats --headers, takeOption leaves the unconsumed token in args, and rejectArgs prints the complete JSON verbatim to stderr. Custom header values can contain tokens or account identifiers, so a syntax error can expose them in terminal or CI logs before server-side validation runs; parse this option with a secret-aware helper or explicitly redact its value from usage errors.
AGENTS.md reference: AGENTS.md:L189-L195
Useful? React with 👍 / 👎.
| if (!isPlainRecord(headersValue)) return jsonResponse({ error: "headers must be an object" }, 400); | ||
| const headersError = providerHeadersConfigError(headersValue); | ||
| if (headersError) return jsonResponse({ error: headersError }, 400); | ||
| next.headers = { ...next.headers, ...headersValue } as Record<string, string>; |
There was a problem hiding this comment.
When an existing header is patched using different capitalization, such as replacing X-Custom with x-custom, this object spread retains both keys instead of replacing the HTTP header. Bun's Headers normalization then emits a combined value such as x-custom: v1, v2, so the PATCH succeeds while the upstream receives an invalid fingerprint; remove any existing case-insensitive match before adding the patched entry.
Useful? React with 👍 / 👎.
| if (headersValue === null || (isPlainRecord(headersValue) && Object.keys(headersValue).length === 0)) { | ||
| delete next.headers; |
There was a problem hiding this comment.
When a registry provider carries required static metadata in headers, clearing custom headers also deletes that metadata. For example, ocx provider add opencode-free seeds x-opencode-client: desktop, but a later --headers - or --headers '{}' removes the entire block, so subsequent upstream requests lose the provider's client marker; clear only user-managed entries or rederive the registry's static headers before persisting.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs-site/src/content/docs/ja/reference/management-api.md`:
- Line 169: Update the Markdown table row for PATCH /api/providers in the
management API reference by inserting the missing cell delimiter before the
“400” response description, restoring three properly separated cells while
preserving the existing Japanese documentation text.
In `@docs-site/src/content/docs/reference/cli/providers-accounts.md`:
- Line 19: Update the `edit <name>` provider-field description to state that
`--headers` applies only to custom/OpenAI-compatible upstream endpoints and
cannot set or override sensitive authentication headers; retain the existing
merge and clear behavior. Apply the same restriction wording in English at
docs-site/src/content/docs/reference/cli/providers-accounts.md:19-19, and
translate it for
docs-site/src/content/docs/ja/reference/cli/providers-accounts.md:18-18,
docs-site/src/content/docs/ko/reference/cli/providers-accounts.md:18-18, and
docs-site/src/content/docs/ru/reference/cli/providers-accounts.md:20-20.
In `@docs-site/src/content/docs/reference/management-api.md`:
- Line 186: Update the PATCH /api/providers endpoint row in
docs-site/src/content/docs/reference/management-api.md:186-186 to document
shallow merging of non-empty headers, clearing with null or {}, rejection of
sensitive authentication headers, invalid names, non-string values, and CRLF
with HTTP 400, and the distinction between configurable custom-adapter headers
and forwarded caller credentials. Translate and apply the same semantics in
docs-site/src/content/docs/ja/reference/management-api.md:169-169,
docs-site/src/content/docs/ko/reference/management-api.md:169-169,
docs-site/src/content/docs/ru/reference/management-api.md:188-188, and
docs-site/src/content/docs/zh-cn/reference/management-api.md:169-169.
In `@src/server/management/provider-routes.ts`:
- Around line 289-300: The headers PATCH merge in the provider route must be
case-insensitive. Update the merge logic in the headers handling block to remove
existing keys whose lowercase names match any incoming patch key before adding
the patch, so casing-only updates cannot preserve duplicates or conflicts. Keep
validation and null/empty clearing behavior unchanged, and add a regression test
covering an existing X-Foo header followed by a x-foo patch.
- Around line 289-304: Update handleProviderRoutes so PATCH mutations re-read
the latest provider and merge changes inside the configuration mutation lock
immediately before saving, rather than persisting the stale snapshot captured
before awaited validation; preserve existing validation and PATCH semantics. Add
a regression test in management provider validation tests that performs
concurrent PATCH requests for different headers and asserts both updates remain.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aefa9390-56be-44f9-a7c0-aca0dc6a935d
📒 Files selected for processing (14)
docs-site/src/content/docs/ja/reference/cli/providers-accounts.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/cli/providers-accounts.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/cli/providers-accounts.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/reference/cli/providers-accounts.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/cli/providers-accounts.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdsrc/cli/provider-runtime.tssrc/server/management/provider-routes.tstests/cli-headless-parity.test.tstests/management-provider-validation.test.ts
| | `GET /api/providers` |編集されたプロバイダー設定と検出状態をリストする | — | | ||
| | `POST /api/providers` |検証済みプロバイダーを 1 つ追加または置換し、必要に応じてそれをデフォルトにします。 400 無効または危険な宛先または構成。 409 名前空間の衝突 | | ||
| | `PATCH /api/providers?name=...` |許可されたプロバイダー フィールド、有効/デフォルト状態、または OpenAI アカウント モードを更新します。 400 無効なフィールドまたは遷移。 404 不明なプロバイダ | | ||
| | `PATCH /api/providers?name=...` |許可されたプロバイダー フィールド（マージされる `headers` ブロックを含む）、有効/デフォルト状態、または OpenAI アカウント モードを更新します。 400 無効なフィールドまたは遷移。 404 不明なプロバイダ | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the missing Markdown table delimiter.
Line 169 lacks the | separator before 400. The error text joins the purpose cell, so the row has two cells instead of the required three.
-| `PATCH /api/providers?name=...` | 許可されたプロバイダー フィールド（マージされる `headers` ブロックを含む）、有効/デフォルト状態、または OpenAI アカウント モードを更新します。 400 無効なフィールドまたは遷移。 404 不明なプロバイダ |
+| `PATCH /api/providers?name=...` | 許可されたプロバイダー フィールド（マージされる `headers` ブロックを含む）、有効/デフォルト状態、または OpenAI アカウント モードを更新します。 | 400 無効なフィールドまたは遷移。 404 不明なプロバイダ |As per path instructions, docs-site/** changes must preserve readable user-facing documentation.
📝 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.
| | `PATCH /api/providers?name=...` |許可されたプロバイダー フィールド（マージされる `headers` ブロックを含む）、有効/デフォルト状態、または OpenAI アカウント モードを更新します。 400 無効なフィールドまたは遷移。 404 不明なプロバイダ | | |
| | `PATCH /api/providers?name=...` |許可されたプロバイダー フィールド（マージされる `headers` ブロックを含む）、有効/デフォルト状態、または OpenAI アカウント モードを更新します。 | 400 無効なフィールドまたは遷移。 404 不明なプロバイダ | |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 169-169: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 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 `@docs-site/src/content/docs/ja/reference/management-api.md` at line 169,
Update the Markdown table row for PATCH /api/providers in the management API
reference by inserting the missing cell delimiter before the “400” response
description, restoring three properly separated cells while preserving the
existing Japanese documentation text.
Sources: Path instructions, Linters/SAST tools
| | `list` | `--json` | List configured providers and the remaining registry entries. | | ||
| | `add <name>` | `--adapter <adapter>`, `--base-url <url>`, `--api-key <key>`, `--default-model <model>`, `--set-default`, `--force`, `--json`, `--sync` | Add a registry/custom provider. `--force` overwrites; `--sync` refreshes a running proxy in human-output mode. | | ||
| | `edit <name>` | provider field flags, `--json` | Edit validated live provider fields without replacing key pools. | | ||
| | `edit <name>` | provider field flags, `--headers <json>`, `--json` | Edit validated live provider fields without replacing key pools. `--headers` merges custom request headers; pass `{}` or `-` to clear them. | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Document the header scope and authentication restriction.
The descriptions say that --headers merges custom request headers. They do not state that these headers apply only to custom/OpenAI-compatible upstream endpoints. They also do not state that sensitive authentication headers cannot be set or overridden.
docs-site/src/content/docs/reference/cli/providers-accounts.md#L19-L19: Add both restrictions to the English description.docs-site/src/content/docs/ja/reference/cli/providers-accounts.md#L18-L18: Translate the same restrictions.docs-site/src/content/docs/ko/reference/cli/providers-accounts.md#L18-L18: Translate the same restrictions.docs-site/src/content/docs/ru/reference/cli/providers-accounts.md#L20-L20: Translate the same restrictions.
As per path instructions, custom-header documentation must distinguish custom upstream headers from forwarded credentials and must not imply that sensitive authentication headers are overridable.
📍 Affects 4 files
docs-site/src/content/docs/reference/cli/providers-accounts.md#L19-L19(this comment)docs-site/src/content/docs/ja/reference/cli/providers-accounts.md#L18-L18docs-site/src/content/docs/ko/reference/cli/providers-accounts.md#L18-L18docs-site/src/content/docs/ru/reference/cli/providers-accounts.md#L20-L20
🤖 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 `@docs-site/src/content/docs/reference/cli/providers-accounts.md` at line 19,
Update the `edit <name>` provider-field description to state that `--headers`
applies only to custom/OpenAI-compatible upstream endpoints and cannot set or
override sensitive authentication headers; retain the existing merge and clear
behavior. Apply the same restriction wording in English at
docs-site/src/content/docs/reference/cli/providers-accounts.md:19-19, and
translate it for
docs-site/src/content/docs/ja/reference/cli/providers-accounts.md:18-18,
docs-site/src/content/docs/ko/reference/cli/providers-accounts.md:18-18, and
docs-site/src/content/docs/ru/reference/cli/providers-accounts.md:20-20.
Source: Path instructions
| | `GET /api/providers` | List redacted provider configuration and discovery state | — | | ||
| | `POST /api/providers` | Add or replace one validated provider and optionally make it default | 400 invalid/dangerous destination or config; 409 namespace collision | | ||
| | `PATCH /api/providers?name=...` | Update allowed provider fields, enabled/default state, or OpenAI account mode | 400 invalid field or transition; 404 unknown provider | | ||
| | `PATCH /api/providers?name=...` | Update allowed provider fields (including a merged `headers` block), enabled/default state, or OpenAI account mode | 400 invalid field or transition; 404 unknown provider | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the provider headers contract complete in English and every locale.
The endpoint does more than merge headers. A non-empty object is shallow-merged; null or {} clears the block; sensitive authentication headers, invalid names, non-string values, and CRLF are rejected with HTTP 400. The documentation must also distinguish configurable custom adapter headers from forwarded caller credentials.
docs-site/src/content/docs/reference/management-api.md#L186-L186: update the English endpoint row with the complete clear, validation, and header-scope semantics.docs-site/src/content/docs/ja/reference/management-api.md#L169-L169: translate the same semantics.docs-site/src/content/docs/ko/reference/management-api.md#L169-L169: translate the same semantics.docs-site/src/content/docs/ru/reference/management-api.md#L188-L188: translate the same semantics.docs-site/src/content/docs/zh-cn/reference/management-api.md#L169-L169: translate the same semantics.
Suggested English row wording
-| `PATCH /api/providers?name=...` | Update allowed provider fields (including a merged `headers` block), enabled/default state, or OpenAI account mode | 400 invalid field or transition; 404 unknown provider |
+| `PATCH /api/providers?name=...` | Update allowed provider fields. A non-empty `headers` object is shallow-merged; `null` or `{}` clears it. These are custom adapter headers, not forwarded caller credentials. | 400 invalid field, header name/value, or transition; 404 unknown provider |As per path instructions, docs-site/** documentation must stay in sync with actual CLI/API behavior and translated locale pages must not contradict the English source.
📍 Affects 5 files
docs-site/src/content/docs/reference/management-api.md#L186-L186(this comment)docs-site/src/content/docs/ja/reference/management-api.md#L169-L169docs-site/src/content/docs/ko/reference/management-api.md#L169-L169docs-site/src/content/docs/ru/reference/management-api.md#L188-L188docs-site/src/content/docs/zh-cn/reference/management-api.md#L169-L169
🤖 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 `@docs-site/src/content/docs/reference/management-api.md` at line 186, Update
the PATCH /api/providers endpoint row in
docs-site/src/content/docs/reference/management-api.md:186-186 to document
shallow merging of non-empty headers, clearing with null or {}, rejection of
sensitive authentication headers, invalid names, non-string values, and CRLF
with HTTP 400, and the distinction between configurable custom-adapter headers
and forwarded caller credentials. Translate and apply the same semantics in
docs-site/src/content/docs/ja/reference/management-api.md:169-169,
docs-site/src/content/docs/ko/reference/management-api.md:169-169,
docs-site/src/content/docs/ru/reference/management-api.md:188-188, and
docs-site/src/content/docs/zh-cn/reference/management-api.md:169-169.
Source: Path instructions
| // headers is the one object-valued field in the mask. PATCH semantics merge it | ||
| // shallowly into the existing block so a single fingerprint header can be added | ||
| // without wiping the rest; null or an empty object clears the whole block. | ||
| if (Object.hasOwn(rawBody, "headers")) { | ||
| const headersValue = rawBody.headers; | ||
| if (headersValue === null || (isPlainRecord(headersValue) && Object.keys(headersValue).length === 0)) { | ||
| delete next.headers; | ||
| } else { | ||
| if (!isPlainRecord(headersValue)) return jsonResponse({ error: "headers must be an object" }, 400); | ||
| const headersError = providerHeadersConfigError(headersValue); | ||
| if (headersError) return jsonResponse({ error: headersError }, 400); | ||
| next.headers = { ...next.headers, ...headersValue } as Record<string, string>; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: adapters either canonicalize header names or tests cover casing-only updates.
rg -n -C 8 -e 'new Headers' -e 'providerOutbound' -e 'build.*Request' -e 'headers' src tests || trueRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== candidate adapter files =='
fd -t f -e ts -e tsx src | rg -i 'adapter|provider|route|request' | head -200
printf '%s\n' '== adapter contracts and header configuration =='
rg -n -C 5 'interface ProviderAdapter|type ProviderAdapter|buildRequest|runTurn|providerHeadersConfigError|headers:' src/server src/lib src/providers src/adapters 2>/dev/null | head -1200
printf '%s\n' '== header forwarding and normalization operations =='
rg -n -C 4 'new Headers|Object\.entries\(.*headers|Object\.keys\(.*headers|headers\s*:' src --glob '*.ts' | rg -i 'adapter|provider|buildRequest|fetch|headers' | head -1200Repository: lidge-jun/opencodex
Length of output: 187
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '== tracked source files containing adapter/provider names =='
git ls-files | grep -E '(^|/)([^/]*(adapter|provider|route|request)[^/]*)\.(ts|tsx)$' | head -300
printf '%s\n' '== relevant symbols in tracked TypeScript files =='
git grep -n -E -C 5 'ProviderAdapter|buildRequest|runTurn|providerHeadersConfigError' -- 'src/**/*.ts' 'src/**/*.tsx' 2>/dev/null | head -1600
printf '%s\n' '== provider route and header-related symbols =='
git grep -n -E -C 8 'headers|isPlainRecord|providerHeadersConfigError' -- 'src/server/management/provider-routes.ts' 'tests/**/*.ts' 2>/dev/null | head -1600Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== provider header consumption in adapters =='
git grep -n -E -C 6 'provider\.headers|\.headers\b' -- 'src/adapters/*.ts' 'src/adapters/**/*.ts' | head -2400
printf '%s\n' '== request construction sites =='
git grep -n -E -C 10 'headers:|new Headers|fetch\(' -- 'src/adapters/*.ts' 'src/adapters/**/*.ts' | head -3000
printf '%s\n' '== exact provider header validator and config types =='
git grep -n -E -C 12 'function providerHeadersConfigError|providerHeadersConfigError|headers\??:.*Record|headers:.*Record' -- 'src/config.ts' 'src/types.ts' 'src/config/**/*.ts' 2>/dev/null | head -1400
printf '%s\n' '== existing management tests for header PATCH behavior =='
git grep -n -E -C 10 'headers|PATCH|patch' -- 'tests/management-provider-validation.test.ts' 'tests/management-*.test.ts' | head -1800Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== all provider.headers consumers =='
git grep -n 'provider\.headers' -- 'src' | sort
printf '%s\n' '== header validator implementation =='
git grep -n -E 'providerHeadersConfigError|function isPlainRecord|const.*HEADER|sensitive.*header' -- 'src/config.ts' 'src/config' 'src/lib' | head -300
printf '%s\n' '== provider header tests =='
git grep -n -E -C 8 'case|casing|headers|X-Foo|x-foo|duplicate' -- 'tests/management-provider-validation.test.ts' 'tests/provider-*.test.ts' | head -1600
printf '%s\n' '== relevant adapter request construction slices =='
sed -n '645,665p' src/adapters/openai-chat.ts
sed -n '970,1010p' src/adapters/openai-responses.ts
sed -n '810,840p' src/adapters/anthropic.ts
sed -n '320,345p' src/adapters/google.tsRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== validator implementation =='
sed -n '560,630p' src/config.ts
printf '%s\n' '== provider PATCH tests near the existing patch coverage =='
sed -n '710,760p' tests/management-provider-validation.test.ts
sed -n '840,900p' tests/management-provider-validation.test.ts
printf '%s\n' '== deterministic casing probe =='
python3 - <<'PY'
existing = {"X-Foo": "old", "X-Bar": "keep"}
patch = {"x-foo": "new"}
merged = {**existing, **patch}
print("merged_record:", merged)
print("case_insensitive_foo_keys:", [k for k in merged if k.lower() == "x-foo"])
print("duplicate_case_insensitive_name:", len([k for k in merged if k.lower() == "x-foo"]) > 1)
PY
node - <<'JS'
const headers = new Headers({ "X-Foo": "old", "x-foo": "new" });
console.log("Headers.get(x-foo):", headers.get("x-foo"));
console.log("Headers.entries:", [...headers.entries()]);
JS
printf '%s\n' '== static check for case-insensitive cleanup =='
python3 - <<'PY'
from pathlib import Path
paths = list(Path("src").rglob("*.ts"))
hits = []
for p in paths:
text = p.read_text()
if "provider.headers" in text:
hits.append((str(p), "hasLowerCaseComparison" if "toLowerCase()" in text and "provider.headers" in text else "noLocalCaseNormalization"))
print(*hits, sep="\n")
PYRepository: lidge-jun/opencodex
Length of output: 8934
Normalize header names before merging. src/server/management/provider-routes.ts:300 preserves both X-Foo and x-foo. providerHeadersConfigError does not reject case-only duplicates. Adapters such as src/adapters/openai-chat.ts:660, src/adapters/openai-responses.ts:981, src/adapters/anthropic.ts:835, and src/adapters/google.ts:330 apply Object.assign without case-insensitive cleanup. A casing-only PATCH can therefore send combined or conflicting values. Remove existing keys whose lowercase names match the patch before adding it, and add a regression test for X-Foo followed by x-foo.
🤖 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 `@src/server/management/provider-routes.ts` around lines 289 - 300, The headers
PATCH merge in the provider route must be case-insensitive. Update the merge
logic in the headers handling block to remove existing keys whose lowercase
names match any incoming patch key before adding the patch, so casing-only
updates cannot preserve duplicates or conflicts. Keep validation and null/empty
clearing behavior unchanged, and add a regression test covering an existing
X-Foo header followed by a x-foo patch.
Source: Path instructions
| // headers is the one object-valued field in the mask. PATCH semantics merge it | ||
| // shallowly into the existing block so a single fingerprint header can be added | ||
| // without wiping the rest; null or an empty object clears the whole block. | ||
| if (Object.hasOwn(rawBody, "headers")) { | ||
| const headersValue = rawBody.headers; | ||
| if (headersValue === null || (isPlainRecord(headersValue) && Object.keys(headersValue).length === 0)) { | ||
| delete next.headers; | ||
| } else { | ||
| if (!isPlainRecord(headersValue)) return jsonResponse({ error: "headers must be an object" }, 400); | ||
| const headersError = providerHeadersConfigError(headersValue); | ||
| if (headersError) return jsonResponse({ error: headersError }, 400); | ||
| next.headers = { ...next.headers, ...headersValue } as Record<string, string>; | ||
| } | ||
| touched = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: a lock covers provider reads, validation, and persistence.
rg -n -C 10 -e 'handleProviderRoutes' -e 'saveConfigPreservingClaudeCode' -e 'CONFIG_MUTATION_LOCK' -e 'mutation.*lock' src tests || trueRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- provider-routes outline ---'
ast-grep outline src/server/management/provider-routes.ts
printf '%s\n' '--- management dispatcher references ---'
rg -n -C 8 'handleProviderRoutes|provider-routes|handleManagementAPI|management.*lock|withConfigMutationLock|mutatePersistedConfig' src/server src tests -g '*.ts' -g '*.tsx' | head -n 500
printf '%s\n' '--- route section ---'
sed -n '220,340p' src/server/management/provider-routes.ts
printf '%s\n' '--- config lock/save sections ---'
sed -n '1580,1785p' src/config.ts
sed -n '1980,2075p' src/config.tsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- provider PATCH setup and persistence ---'
sed -n '73,220p' src/server/management/provider-routes.ts
sed -n '330,475p' src/server/management/provider-routes.ts
printf '%s\n' '--- exact save implementation ---'
sed -n '2018,2075p' src/config.ts
printf '%s\n' '--- provider PATCH tests and concurrency coverage ---'
rg -n -C 6 'PATCH|headers|concurrent|Promise\.all|provider-routes|/api/providers' tests -g '*.ts' | head -n 700
printf '%s\n' '--- management context and dependency seams ---'
cat -n src/server/management/context.tsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
route = Path("src/server/management/provider-routes.ts").read_text()
api = Path("src/server/management-api.ts").read_text()
config = Path("src/config.ts").read_text()
def line_of(text, needle):
pos = text.index(needle)
return text.count("\n", 0, pos) + 1
checks = {
"PATCH route is async": "export async function handleProviderRoutes" in route,
"PATCH snapshots provider before validation": route.index("const next: OcxProviderConfig") < route.index("await providerDestinationResolvedError", route.index("const next: OcxProviderConfig")),
"PATCH saves after validation": route.index("await providerDestinationResolvedError", route.index("const next: OcxProviderConfig")) < route.index("save(config);", route.index("const next: OcxProviderConfig")),
"dispatcher has no provider mutation lock": not bool(re.search(r"withConfigMutationLockSync|mutatePersistedConfig", api[api.index("export async function handleManagementAPI"):api.index("export async function handleManagementAPI") + 12000])),
"save lock is synchronous": "export function withConfigMutationLockSync" in config and "never return a Promise from `fn`" in config,
"save does not re-read providers": "Scope residual: only `claudeCode` is reconciled." in config and "const persistedConfig: OcxConfig = { ...config, port: persistedBinding.port }" in config,
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
for label, text, needle in [
("next snapshot", route, "const next: OcxProviderConfig"),
("destination validation", route, "const resolvedError = await providerDestinationResolvedError(name, next)"),
("provider save", route, "config.providers[name] = stripRegistryOnlyStaticHeaders(name, next)"),
("dispatcher", api, "routed = (await handleConfigRoutes(ctx))"),
("save lock", config, "export function saveConfigPreservingClaudeCode"),
]:
print(f"{label}: line {line_of(text, needle)}")
PY
printf '%s\n' '--- focused provider-management test files ---'
rg -l '(/api/providers|provider patch|provider.*PATCH|headers)' tests -g '*.ts' | sort
printf '%s\n' '--- concurrent provider test markers ---'
rg -n -i -C 2 'concurrent|Promise\.all|headers' tests -g '*provider*' -g '*management*' -g '*.ts' | rg -i 'provider|headers|Promise\.all|concurrent' | head -n 250Repository: lidge-jun/opencodex
Length of output: 33833
Rebase concurrent provider PATCH updates under the config mutation lock.
handleProviderRoutes copies config.providers[name] at line 216, awaits destination validation at line 319, then saves the stale snapshot at line 347. The dispatcher does not hold a lock across this sequence, and saveConfigPreservingClaudeCode only locks the final synchronous write. Concurrent PATCH requests can therefore erase each other’s header updates.
Re-read and merge the current provider inside a lock-aware mutation before saving. Add a concurrent PATCH regression test in tests/management-provider-validation.test.ts that updates different headers and asserts both persist.
🤖 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 `@src/server/management/provider-routes.ts` around lines 289 - 304, Update
handleProviderRoutes so PATCH mutations re-read the latest provider and merge
changes inside the configuration mutation lock immediately before saving, rather
than persisting the stale snapshot captured before awaited validation; preserve
existing validation and PATCH semantics. Add a regression test in management
provider validation tests that performs concurrent PATCH requests for different
headers and asserts both updates remain.
Source: Path instructions
|
Please put your Pull-Request on Ready for Review, once you are finished. |
|
Left open and out of the current bug stack (#951–#973), because this is a feature rather than a defect: there is no way to set That is not a rejection. The motivating case is concrete (providers needing client-fingerprint headers, with hand-editing |
- redact --headers values from CLI usage errors so tokens never hit stderr (P1) - merge header blocks case-insensitively so a casing-only PATCH replaces the existing key instead of leaving a combined duplicate for Headers (P2) - restore registry static headers when clearing custom headers so transports like opencode-free keep their client marker (P2) - re-apply the field mask under the config mutation lock immediately before saving so concurrent PATCHes merge instead of clobbering each other (major) - regression tests for redaction, casing merge, static-header clear, and concurrent PATCH persistence
3 participants
Summary
The provider management plane has no way to set or restore the
headersfield, so custom providers that require client-fingerprint headers (e.g. Agent Router via theopenai-chat/anthropicadapters) fail with upstream401 unauthorized client detectedafter the block is lost — the only recovery was hand-editing~/.opencodex/config.json.PATCH /api/providers?name=...now accepts aheadersobject. It is shallow-merged onto the provider's existing block (adding one fingerprint header never wipes the rest);nullor{}clears the whole block. Validation reusesproviderHeadersConfigError, so sensitive headers (Authorizationetc.), invalid names, non-string values, and CRLF injection are rejected with 400.ocx provider editgains--headers <json>(and--headers -/--headers '{}'to clear). The value is parsed client-side and sent through the same PATCH route, so CLI and API validation stay identical.POST /api/providersalready round-tripsheadersfor custom providers, so a re-save keeps them; the issue's third ask is covered by the existing path.Example:
ocx provider edit AGR-OAI --headers '{"x-app":"cli","anthropic-version":"2023-06-01"}'Tests
bun run typecheck— passbun test tests/cli-headless-parity.test.ts tests/cli-provider.test.ts tests/config.test.ts tests/repo-hygiene.test.ts— 168 passbun run privacy:scan— passtests/management-provider-validation.test.ts; CLI--headersforwarding and malformed-JSON rejection intests/cli-headless-parity.test.ts.Environment note:
tests/management-provider-validation.test.tscannot fully run on this machine because local Clash DNS maps*.example.testto 198.18.x.x benchmark addresses, which the destination policy deliberately rejects. The same pre-existing tests fail identically on cleandev; CI's unresolvable.testhosts take the allowed path.Fixes #959
Summary by CodeRabbit
New Features
ocx provider edit.{},-, ornull.headersconfiguration.Bug Fixes
Documentation