-
Notifications
You must be signed in to change notification settings - Fork 785
feat: manage provider custom headers via PATCH and ocx provider edit --headers #961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ both `--adapter` and `--base-url`. | |
| | --- | --- | --- | | ||
| | `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. | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win Document the header scope and authentication restriction. The descriptions say that
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
🤖 Prompt for AI AgentsSource: Path instructions |
||
| | `test <name>` | `--json` | Probe the real upstream model endpoint. | | ||
| | `show <name>` | `--json` | Show config with API keys masked. | | ||
| | `remove <name>` | `--json` | Remove a non-default provider; the last provider cannot be removed. | | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,7 +183,7 @@ keys are not returned to dashboard clients. | |
| | --- | --- | --- | | ||
| | `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 | | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Keep the provider The endpoint does more than merge headers. A non-empty object is shallow-merged;
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, 📍 Affects 5 files
🤖 Prompt for AI AgentsSource: Path instructions |
||
| | `DELETE /api/providers?name=...` | Delete a provider, reassigning the default when possible | 404 unknown provider; 409 `last_provider`; 409 `provider_has_dependent_combos` | | ||
| | `POST /api/providers/test?name=...` | Perform a bounded live provider connectivity/model-discovery probe | 404 unknown provider; failures are normally returned as `ok: false` evidence | | ||
| | `GET /api/provider-quotas` | Read provider quota reports; `refresh=1` forces refresh | — | | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,8 @@ const USAGE = `Usage: | |
| ocx provider edit <name> [--adapter <id>] [--base-url <url>] [--default-model <id|->] | ||
| [--auth-mode <key|forward|oauth|local|->] [--note <text|->] | ||
| [--api-key-transport <x-api-key|bearer|->] | ||
| [--enabled <on|off>] [--live-models <on|off>] [--allow-private-network <on|off>] [--json] | ||
| [--headers <json>] [--enabled <on|off>] [--live-models <on|off>] | ||
| [--allow-private-network <on|off>] [--json] | ||
| ocx provider test <name> [--json] | ||
| ocx provider quota [--refresh] [--json] | ||
| ocx provider presets [--json] | ||
|
|
@@ -39,6 +40,7 @@ async function edit(argv: string[], deps: RuntimeApiDeps): Promise<void> { | |
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user uses the common AGENTS.md reference: AGENTS.md:L189-L195 Useful? React with 👍 / 👎. |
||
| const enabled = takeBooleanOption(args, "--enabled"); | ||
| const liveModels = takeBooleanOption(args, "--live-models"); | ||
| const allowPrivateNetwork = takeBooleanOption(args, "--allow-private-network"); | ||
|
|
@@ -49,6 +51,21 @@ async function edit(argv: string[], deps: RuntimeApiDeps): Promise<void> { | |
| if (authMode !== undefined) patch.authMode = authMode; | ||
| if (note !== undefined) patch.note = note; | ||
| if (apiKeyTransport !== undefined) patch.apiKeyTransport = apiKeyTransport; | ||
| if (headers !== undefined) { | ||
| if (headers === "-") { | ||
| patch.headers = null; | ||
| } else { | ||
| let parsed: unknown; | ||
| try { parsed = JSON.parse(headers); } catch { throw new CliUsageError("--headers must be valid JSON"); } | ||
| if (parsed === null) { | ||
| patch.headers = null; | ||
| } else if (typeof parsed !== "object" || Array.isArray(parsed)) { | ||
| throw new CliUsageError("--headers must be a JSON object like {\"X-Custom\":\"value\"}"); | ||
| } else { | ||
| patch.headers = parsed; | ||
| } | ||
| } | ||
| } | ||
| if (enabled !== undefined) patch.disabled = !enabled; | ||
| if (liveModels !== undefined) patch.liveModels = liveModels; | ||
| if (allowPrivateNetwork !== undefined) patch.allowPrivateNetwork = allowPrivateNetwork; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -286,6 +286,22 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise<Resp | |
| touched = true; | ||
| } | ||
|
|
||
| // 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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a registry provider carries required static metadata in AGENTS.md reference: src/AGENTS.md:L18-L18 Useful? React with 👍 / 👎. |
||
| } 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an existing header is patched using different capitalization, such as replacing Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ 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. 🤖 Prompt for AI AgentsSource: Path instructions |
||
| } | ||
| touched = true; | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ 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.
Re-read and merge the current provider inside a lock-aware mutation before saving. Add a concurrent PATCH regression test in 🤖 Prompt for AI AgentsSource: Path instructions |
||
| if (!touched) return jsonResponse({ error: "no recognized fields to update" }, 400); | ||
|
|
||
| // A disabled-only toggle preserves the v2 fast lane for non-openai providers: it changes | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the missing Markdown table delimiter.
Line 169 lacks the
|separator before400. The error text joins the purpose cell, so the row has two cells instead of the required three.As per path instructions,
docs-site/**changes must preserve readable user-facing documentation.📝 Committable suggestion
🧰 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
Sources: Path instructions, Linters/SAST tools