fix(codex): preserve account-scoped native model ids - #1515
Conversation
|
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 change preserves valid, previously unknown native OpenAI model IDs from Codex account catalogs. It emits them only as selector-qualified rows and propagates them through synchronization, convergence, discovery, management, and model visibility handling. ChangesAccount-Scoped Native Model Discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
1/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
f5ef4cc to
f9bc6c5
Compare
|
Rebased the fork branch onto current |
f9bc6c5 to
b7c51c7
Compare
|
Rebased and force-with-lease pushed onto current dev@cbbfdd8773e68a5dc2391ddeb32f33a225373c1a; new head is b7c51c7. Fresh local proof: focused catalog/convergence/cache/discovery/toggle suite 83/83, 610 assertions; typecheck, privacy scan, and diff check passed. The PR description and issue linkage are updated; keeping Draft pending independent review. |
|
Initial review: the issue is real and this is directionally valuable, but the preservation boundary is broad enough that I am keeping it draft pending a full exact-head review. The safe part of the design is that observed account-scoped IDs are not promoted into the global static native allowlist and are exposed only through the configured ChatGPT/Codex forward-account path. Before this becomes ready, please obtain executed exact-head CI and verify these negative cases explicitly across every read surface:
The current 83-test focused proof is useful but the PR changes catalog sync, convergence, server discovery, management rows, and cache replacement. Keep the readiness boxes open until the maintained matrix executes and an independent review confirms the provenance and isolation boundaries. I am not approving or merging this head yet. |
|
Updated exact head: This follow-up closes the review gaps around #1480:
Fresh local proof on this exact head: 84 focused tests passed, 616 assertions; typecheck, privacy scan, diff check, and the 265-page docs build passed. The PR remains Draft for exact-head CI and maintainer review. |
212e705 to
84b2616
Compare
|
Superseded by the final current-dev update below. The authoritative candidate is now based on |
84b2616 to
e47a368
Compare
|
Final current-dev rebase completed after correcting the upstream base pointer. Base: |
e47a368 to
f1126c0
Compare
|
Current exact-head handoff: rebased onto upstream Post-rebase proof: 84 focused tests passed, 0 failed, 616 assertions; typecheck, privacy scan, and diff check passed. Please run exact-head CI and review the provenance/selector isolation matrix; keeping Draft. |
f1126c0 to
a402498
Compare
|
Final exact-head update: upstream The final rebase proof is 84/84 focused tests, 616 assertions, plus typecheck, privacy scan, and diff check. Please review and run exact-head CI; Draft remains intentional. |
|
Final exact-head gate status: |
|
@lidge-jun Reviewer-request API is unavailable to the fork author (403), so this is the explicit handoff instead. Final head |
a402498 to
ddc192c
Compare
|
Current exact-head handoff after the latest upstream rebase:
The PR remains Draft because independent maintainer review and readiness confirmation are still pending. The linked #1480 evidence now covers the concrete ChatGPT/Codex account surface, exact |
|
Fresh exact-head verification for
The PR remains Draft pending maintainer review and the repository readiness gate. |
|
@Ingwannu The exact-head candidate evidence is now consolidated in the PR body: 84 account/catalog tests passed, clean current-dev comparison reproduced the unrelated baseline failures, and #1480 has concrete ChatGPT/Codex account evidence for |
ddc192c to
57dccdd
Compare
|
Reviewed against current The functional change is right and stays. Preserving an exact account-scoped id like What I changed is the claim, not the behavior. So the name overstates what the check proves, and the docs repeated the claim. To be fair about severity, because the first review round overstated it too. This grants nothing new. The follow-up therefore renames the predicate to Verification at |
![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: 57dccddf7f
ℹ️ 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".
| if (!isAccountBoundOpenAiNativeSlug(slug) | ||
| || entry.supported_in_api !== true | ||
| || !hasNativeCatalogRowShape(entry) | ||
| || (entry.visibility !== "list" && entry[ACCOUNT_BOUND_OBSERVED_NATIVE_MARKER] !== true)) { |
There was a problem hiding this comment.
When a user disables an unknown account-scoped model, convergence writes its generated selector-qualified row with visibility: "hide" but without opencodex_account_observed_native. This condition then rejects that row on the next catalog read, so accountBoundNativeOpenAiSlugsBySelector() forgets the model and /api/model-visibility no longer includes it in supportedNative; an attempt to re-enable the same model therefore returns 400 until Codex happens to observe it upstream again. Treat trusted generated account-bound rows as observations regardless of their current visibility, while still applying the current selector filter, and add a disable/re-enable regression test.
AGENTS.md reference: src/AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
| const accountNativeEntries = selectorNativeSlugs.map(slug => ( | ||
| nativeEntriesBySlug.get(slug) | ||
| ?? deriveEntry(template, slug, "OpenAI native model (Codex OAuth passthrough).", 9, undefined, new Set(), openaiContextCap) |
There was a problem hiding this comment.
When an observed unknown model has capabilities that differ from the selected catalog template, this path keeps only its slug and synthesizes a new entry from that unrelated template. The resulting account-qualified row can advertise the wrong supported_reasoning_levels, context/tool flags, instructions, and comp_hash; for example, the new tests observe a medium-only row but the generated catalog expands it to the template's ladder, allowing Codex to send an effort the model may reject. Carry the admitted observed RawEntry per selector and clone that entry before qualification instead of deriving it solely from the slug, with a regression asserting the observed capability fields survive.
AGENTS.md reference: src/AGENTS.md:L24-L26
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: 4
🤖 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 `@src/codex/catalog/metadata.ts`:
- Around line 370-399: Reuse a single catalog/cache observed-entry snapshot per
discovery request instead of triggering repeated synchronous reads. After
fetchAllModels, read both sources once and pass the snapshot into
accountBoundNativeOpenAiSlugsBySelector and desktopVisibleNativeSlugs, caching
the computed desktop list for both Anthropic checks; apply the same snapshot
propagation in model-rows and model-routes.
In `@src/codex/catalog/sync.ts`:
- Around line 1642-1646: Export the existing mainAccountSelectors helper from
metadata.ts and use it in the sync flow instead of duplicating the
visibleCodexAccountSelectors filter, removing the now-unused direct imports.
Hoist loadConfig() into one local value before shouldSyncCodexOnStart and reuse
that same configuration for selector computation, ensuring both decisions
observe one config generation.
In `@src/codex/convergence.ts`:
- Around line 238-246: Extract the shared account-native slug derivation into
projectAccountNativeSlugs in metadata.ts, returning accountNativeSlugs,
accountNativeSlugsBySelector, and an always-empty observedNativeSlugs with the
existing no-selector behavior. Replace the duplicated derivation in the
convergence flow and sync flow, while keeping their observed-entry sources
separate and preserving the existing consumers’ spreadable values.
In `@structure/03_catalog-and-subagents.md`:
- Around line 35-40: Update the catalog synchronization description to replace
the native catalog provenance claim with a native-row plausibility check. State
that complete hand-written cache rows are accepted, the check does not verify
upstream or account provenance, and selector mapping falls back to eligible main
selectors when no account marker exists; preserve the documented handling of
unknown ids and selector-qualified rows.
🪄 Autofix
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: 6eb7b3f2-926b-468e-ae0b-55b0c98a6824
📒 Files selected for processing (16)
docs-site/src/content/docs/guides/codex-app-models.mddocs-site/src/content/docs/reference/configuration/routing.mdsrc/codex/catalog.tssrc/codex/catalog/bundled.tssrc/codex/catalog/metadata.tssrc/codex/catalog/sync.tssrc/codex/convergence.tssrc/server/index.tssrc/server/management/model-routes.tssrc/server/management/model-rows.tsstructure/03_catalog-and-subagents.mdtests/claude-models-discovery.test.tstests/codex-catalog-sync-hardening.test.tstests/codex-convergence-account-selectors.test.tstests/codex-models-cache-invalidate.test.tstests/native-model-toggle.test.ts
| export function accountBoundNativeOpenAiSlugs( | ||
| observedEntries: readonly RawEntry[] = [ | ||
| ...(readCurrentCodexModelsCache()?.models ?? []), | ||
| // Existing generated rows are also safe to reuse after a process starts without a cache | ||
| // invalidation pass; bare user-authored catalog rows are intentionally not trusted here. | ||
| ...(readCurrentCodexCatalog()?.models ?? []).filter(entry => | ||
| trustedAccountBoundNativeCatalogSlug(entry) !== undefined), | ||
| ], | ||
| ): string[] { | ||
| const observed = observedEntries.flatMap(entry => { | ||
| const slug = observedAccountBoundNativeSlug(entry); | ||
| return slug === undefined ? [] : [slug]; | ||
| }); | ||
| return unique([...NATIVE_OPENAI_MODELS, ...observed]); | ||
| } | ||
|
|
||
| /** | ||
| * Resolve account-native ids per public selector. Bare observations come from Codex's main | ||
| * catalog/cache, so they are eligible only for selectors that target the main account. A | ||
| * generated qualified row carries its own selector and never gets copied to an unrelated pool | ||
| * account. An explicit observation marker is public selector metadata only; private account ids | ||
| * never enter the catalog or cache. | ||
| */ | ||
| export function accountBoundNativeOpenAiSlugsBySelector( | ||
| config: AccountSelectorConfig, | ||
| observedEntries: readonly RawEntry[] = [ | ||
| ...(readCurrentCodexModelsCache()?.models ?? []), | ||
| ...(readCurrentCodexCatalog()?.models ?? []).filter(entry => | ||
| trustedAccountBoundNativeCatalogSlug(entry) !== undefined), | ||
| ], |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers that rely on the disk-reading default parameters.
set -euo pipefail
echo "== accountBoundNativeOpenAiSlugsBySelector call sites =="
rg -nP --type=ts -C4 '\baccountBoundNativeOpenAiSlugsBySelector\s*\('
echo "== accountBoundNativeOpenAiSlugs call sites =="
rg -nP --type=ts -C4 '\baccountBoundNativeOpenAiSlugs\s*\('
echo "== observedAccountBoundNativeOpenAiSlugs call sites =="
rg -nP --type=ts -C4 '\bobservedAccountBoundNativeOpenAiSlugs\s*\('
echo "== desktopVisibleNativeSlugs call sites =="
rg -nP --type=ts -C4 '\bdesktopVisibleNativeSlugs\s*\('
echo "== confirm the /v1/models handler encloses the src/server/index.ts calls =="
rg -nP --type=ts -C25 "url\.pathname === \"/v1/models\"" src/server/index.tsRepository: lidge-jun/opencodex
Length of output: 214
🏁 Script executed:
set -euo pipefail
echo "== matching files =="
fd -t f -e ts . | rg '(^|/)(metadata|parsing|sync|convergence|index|model-rows|model-routes)\.ts$'
echo "== symbol definitions and call sites =="
rg -n -F -e 'accountBoundNativeOpenAiSlugsBySelector' \
-e 'accountBoundNativeOpenAiSlugs' \
-e 'observedAccountBoundNativeOpenAiSlugs' \
-e 'desktopVisibleNativeSlugs' \
-e 'readCurrentCodexModelsCache' \
-e 'readCurrentCodexCatalog' \
--glob '*.ts' .
echo "== metadata structure =="
metadata="$(fd -t f -a 'metadata.ts' . | head -n1)"
test -n "$metadata"
wc -l "$metadata"
sed -n '330,455p' "$metadata"
echo "== server model-list context =="
index="$(fd -t f -a 'index.ts' . | rg '(^|/)src/server/index\.ts$' | head -n1)"
test -n "$index"
rg -n -C35 'v1/models|accountBoundNativeOpenAiSlugsBySelector|desktopVisibleNativeSlugs|observedAccountBoundNativeOpenAiSlugs' "$index"
echo "== management callers =="
for name in model-rows.ts model-routes.ts; do
file="$(fd -t f -a "$name" . | head -n1)"
if test -n "$file"; then
echo "--- $file"
rg -n -C12 'accountBoundNativeOpenAiSlugs|accountBoundNativeOpenAiSlugsBySelector|observedAccountBoundNativeOpenAiSlugs' "$file" || true
fi
doneRepository: lidge-jun/opencodex
Length of output: 28406
🏁 Script executed:
set -euo pipefail
echo "== metadata implementation =="
sed -n '190,240p' src/codex/catalog/metadata.ts
sed -n '350,445p' src/codex/catalog/metadata.ts
echo "== bundled readers and parsing =="
sed -n '500,555p' src/codex/catalog/bundled.ts
rg -n -C12 'function readCatalog|export .*readCatalog|readCatalog\(' src/codex/catalog/parsing.ts src/codex/catalog/bundled.ts
echo "== complete /v1/models response branches =="
sed -n '869,1015p' src/server/index.ts
echo "== all relevant direct callers with function context =="
for file in src/codex/catalog/metadata.ts src/server/index.ts src/server/management/model-rows.ts src/server/management/model-routes.ts src/cli/claude-desktop.ts src/server/management/native-integration-routes.ts src/server/management/agent-settings-routes.ts src/server/management/shared.ts; do
echo "--- $file"
rg -n -C8 'accountBoundNativeOpenAiSlugsBySelector|accountBoundNativeOpenAiSlugs\(|observedAccountBoundNativeOpenAiSlugs|desktopVisibleNativeSlugs\(' "$file" || true
done
echo "== catalog/cache file size and reader caching =="
rg -n -C8 'CODEX.*CATALOG|MODELS.*CACHE|catalogPath|modelsCache|readFileSync|existsSync|JSON.parse' src/codex/catalog/bundled.ts src/codex/catalog/parsing.ts
find . -type f \( -name 'model_catalog.json' -o -name '*models*cache*.json' \) -printf '%s %p\n' 2>/dev/null | head -20Repository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
set -euo pipefail
echo "== account-bound inclusion condition =="
rg -n -C12 'function shouldIncludeAccountBoundNativeOpenAi|export function shouldIncludeAccountBoundNativeOpenAi|shouldIncludeAccountBoundNativeOpenAi' src/codex/catalog/metadata.ts
echo "== concise line-numbered target regions =="
nl -ba src/codex/catalog/metadata.ts | sed -n '208,228p;366,438p'
nl -ba src/server/index.ts | sed -n '869,947p'
nl -ba src/server/management/model-rows.ts | sed -n '45,68p'
nl -ba src/server/management/model-routes.ts | sed -n '232,245p'
echo "== read path resolution =="
rg -n -C10 'function readCodexCatalogPath|export function readCodexCatalogPath|function activeCodexModelsCachePath|export function activeCodexModelsCachePath' src/codex
echo "== deterministic static verifier for default-reader call paths =="
python3 - <<'PY'
from pathlib import Path
import re
metadata = Path("src/codex/catalog/metadata.ts").read_text()
for name in ("accountBoundNativeOpenAiSlugs", "accountBoundNativeOpenAiSlugsBySelector"):
m = re.search(rf"export function {name}\s*\\([^)]*\\)\\s*(?::[^{{]+)?\\{{", metadata, re.S)
assert m, name
body = metadata[m.start():]
body = body[:body.find("\nexport function ", 1)] if "\nexport function " in body[1:] else body
print(name, "default disk-reader expressions:", body.count("readCurrentCodexModelsCache()"), body.count("readCurrentCodexCatalog()"))
index = Path("src/server/index.ts").read_text()
for pattern in (
r"accountBoundNativeOpenAiSlugsBySelector\\(config\\)",
r"desktopVisibleNativeSlugs\\(config\\)",
):
print(pattern, "matches:", len(re.findall(pattern, index)))
print("desktop call is inside Anthropic branch:",
index.index("desktopVisibleNativeSlugs(config)") > index.index("if (wantsAnthropicList")
and index.index("desktopVisibleNativeSlugs(config)") < index.index("if (url.searchParams.has(\"client_version\""))
)
PYRepository: lidge-jun/opencodex
Length of output: 3512
🏁 Script executed:
set -euo pipefail
echo "== concise line-numbered target regions =="
sed -n '208,228p' src/codex/catalog/metadata.ts | awk '{printf "%d %s\n", NR+207, $0}'
sed -n '366,438p' src/codex/catalog/metadata.ts | awk '{printf "%d %s\n", NR+365, $0}'
sed -n '869,947p' src/server/index.ts | awk '{printf "%d %s\n", NR+868, $0}'
sed -n '45,68p' src/server/management/model-rows.ts | awk '{printf "%d %s\n", NR+44, $0}'
sed -n '232,245p' src/server/management/model-routes.ts | awk '{printf "%d %s\n", NR+231, $0}'
echo "== read path resolution =="
rg -n -C10 'function readCodexCatalogPath|export function readCodexCatalogPath|function activeCodexModelsCachePath|export function activeCodexModelsCachePath' src/codex || true
echo "== deterministic static verifier for default-reader call paths =="
python3 - <<'PY'
from pathlib import Path
import re
metadata = Path("src/codex/catalog/metadata.ts").read_text()
for name in ("accountBoundNativeOpenAiSlugs", "accountBoundNativeOpenAiSlugsBySelector"):
m = re.search(rf"export function {name}\s*\([^)]*\)\s*(?::[^{]+)?\{{", metadata, re.S)
assert m, name
tail = metadata[m.start():]
next_export = tail.find("\nexport function ", 1)
body = tail if next_export < 0 else tail[:next_export]
print(name, "readCurrentCodexModelsCache calls:", body.count("readCurrentCodexModelsCache()"),
"readCurrentCodexCatalog calls:", body.count("readCurrentCodexCatalog()"))
index = Path("src/server/index.ts").read_text()
for expression in (
"accountBoundNativeOpenAiSlugsBySelector(config)",
"desktopVisibleNativeSlugs(config)",
):
print(expression, "matches:", index.count(expression))
start = index.index("if (wantsAnthropicList")
end = index.index('if (url.searchParams.has("client_version")', start)
print("desktop call is inside Anthropic branch:", "desktopVisibleNativeSlugs(config)" in index[start:end])
PYRepository: lidge-jun/opencodex
Length of output: 16779
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
metadata = Path("src/codex/catalog/metadata.ts").read_text()
for name in ("accountBoundNativeOpenAiSlugs", "accountBoundNativeOpenAiSlugsBySelector"):
pattern = r"export function " + name + r"\s*\([^)]*\)\s*(?::[^{]+)?\{"
m = re.search(pattern, metadata, re.S)
assert m, name
tail = metadata[m.start():]
next_export = tail.find("\nexport function ", 1)
body = tail if next_export < 0 else tail[:next_export]
print(name, "readCurrentCodexModelsCache calls:", body.count("readCurrentCodexModelsCache()"),
"readCurrentCodexCatalog calls:", body.count("readCurrentCodexCatalog()"))
index = Path("src/server/index.ts").read_text()
print("accountBoundNativeOpenAiSlugsBySelector(config) matches:",
index.count("accountBoundNativeOpenAiSlugsBySelector(config)"))
print("desktopVisibleNativeSlugs(config) matches:",
index.count("desktopVisibleNativeSlugs(config)"))
start = index.index("if (wantsAnthropicList")
end = index.index('if (url.searchParams.has("client_version")', start)
print("desktop call is inside Anthropic branch:",
"desktopVisibleNativeSlugs(config)" in index[start:end])
PYRepository: lidge-jun/opencodex
Length of output: 276
Reuse one observed-entry snapshot per discovery request. accountBoundNativeOpenAiSlugsBySelector defaults to synchronous catalog and cache reads at src/codex/catalog/metadata.ts:393-400. The /v1/models handler invokes this default at src/server/index.ts:904. Anthropic discovery invokes desktopVisibleNativeSlugs(config) twice at lines 925 and 942, and that function invokes the same default at metadata.ts:217. When both files exist, one Anthropic discovery request can therefore perform six JSON parses and block Bun's event loop. Read both sources once after fetchAllModels, pass the snapshot through accountBoundNativeOpenAiSlugsBySelector and desktopVisibleNativeSlugs, and reuse the computed desktop list. Apply the same snapshot pattern to src/server/management/model-rows.ts:57 and src/server/management/model-routes.ts:238.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/codex/catalog/metadata.ts` around lines 370 - 399, Reuse a single
catalog/cache observed-entry snapshot per discovery request instead of
triggering repeated synchronous reads. After fetchAllModels, read both sources
once and pass the snapshot into accountBoundNativeOpenAiSlugsBySelector and
desktopVisibleNativeSlugs, caching the computed desktop list for both Anthropic
checks; apply the same snapshot propagation in model-rows and model-routes.
Source: Path instructions
| const currentConfig = loadConfig(); | ||
| const mainSelectors = visibleCodexAccountSelectors(currentConfig).filter(selector => { | ||
| const target = new Map(codexAccountNamespaceEntries(currentConfig)).get(selector); | ||
| return isMainCodexAccountTarget(target ?? ""); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the main-selector rule instead of duplicating it, and load the configuration once.
Two problems in this block.
First, Lines 1643-1646 reimplement mainAccountSelectors from src/codex/catalog/metadata.ts:203-207. Both build a Map from codexAccountNamespaceEntries, then filter visibleCodexAccountSelectors by isMainCodexAccountTarget. The metadata helper is not exported, so this file copied it.
The two copies form a producer-consumer pair that must agree. This function writes the result into opencodex_account_observed_selectors at Line 1659. accountBoundNativeOpenAiSlugsBySelector reads that marker at metadata.ts:413-419 and keeps only selectors that satisfy its own mainSelectors set. If the eligibility rule changes in metadata.ts and this copy is not updated, the persisted marker stops matching the consumer filter. The observed model then disappears from discovery with no error, which is the exact symptom issue #1480 reports. Export mainAccountSelectors and call it here.
Second, loadConfig() runs twice: once inside shouldSyncCodexOnStart(loadConfig()) at Line 1634, and again at Line 1642. Each call reads config.json and runs the Zod parse in src/config.ts:1983. Beyond the duplicated cost, the two reads can observe different generations of the file, so the sync gate and the selector list can come from different configurations. Hoist one value.
♻️ Proposed fix
In src/codex/catalog/metadata.ts, export the existing helper:
-function mainAccountSelectors(config: AccountSelectorConfig): string[] {
+export function mainAccountSelectors(config: AccountSelectorConfig): string[] {
const targets = new Map(codexAccountNamespaceEntries(config));
return visibleCodexAccountSelectors(config).filter(selector =>
isMainCodexAccountTarget(targets.get(selector) ?? ""));
}Re-export it from the facade in src/codex/catalog.ts if other modules need it, then in this file:
- if (!shouldSyncCodexOnStart(loadConfig())) return false;
+ const currentConfig = loadConfig();
+ if (!shouldSyncCodexOnStart(currentConfig)) return false;
const catalogPath = readCodexCatalogPath();
if (!existsSync(catalogPath)) return false;
const catalog = JSON.parse(readFileSync(catalogPath, "utf8"));
const models = catalog.models ?? catalog;
const currentCache = readCatalog(activeCodexModelsCachePath());
const existingSlugs = new Set(models.flatMap((entry: RawEntry) =>
typeof entry.slug === "string" ? [entry.slug] : []));
- const currentConfig = loadConfig();
- const mainSelectors = visibleCodexAccountSelectors(currentConfig).filter(selector => {
- const target = new Map(codexAccountNamespaceEntries(currentConfig)).get(selector);
- return isMainCodexAccountTarget(target ?? "");
- });
+ const mainSelectors = mainAccountSelectors(currentConfig);The import of codexAccountNamespaceEntries and isMainCodexAccountTarget at Line 33 then becomes unnecessary in this file.
Note that the current copy also rebuilds the Map inside the filter callback, so it constructs one Map per selector. The shared helper builds it once.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/codex/catalog/sync.ts` around lines 1642 - 1646, Export the existing
mainAccountSelectors helper from metadata.ts and use it in the sync flow instead
of duplicating the visibleCodexAccountSelectors filter, removing the now-unused
direct imports. Hoist loadConfig() into one local value before
shouldSyncCodexOnStart and reuse that same configuration for selector
computation, ensuring both decisions observe one config generation.
| const accountNativeSlugs = accountSelectors.length > 0 | ||
| ? accountBoundNativeOpenAiSlugs(observedAccountNativeEntries) | ||
| : []; | ||
| const accountNativeSlugsBySelector = accountSelectors.length > 0 | ||
| ? accountBoundNativeOpenAiSlugsBySelector(config, observedAccountNativeEntries) | ||
| : new Map<string, readonly string[]>(); | ||
| // Unknown account-native ids have no safe bare/global identity. They are only projected through | ||
| // selector-qualified rows when a live selector is configured. | ||
| const observedNativeSlugs: string[] = []; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract this derivation so the convergence catalog cannot drift from the retained-sync catalog.
Lines 238-246 duplicate src/codex/catalog/sync.ts:1319-1327 exactly: the same accountSelectors.length > 0 gate, the same two metadata calls, the same always-empty observedNativeSlugs, and the same explanatory comment. The two blocks then feed the same two functions, buildCatalogEntriesFromObservedState and mergeCatalogEntriesFromObservedState.
Both paths must produce the same catalog for the same configuration. writeRetainedCatalogSync and prepareCatalog are the two writers of model_catalog.json. If a later change updates one gate and not the other, the two writers emit different account-qualified row sets for identical input, and the difference appears only as a catalog that flips content depending on which path ran last. That failure mode is silent, because both paths validate their own output with trustedAccountBoundNativeCatalogSlug.
The observed-entry sources correctly differ and should stay separate: this file reads through the evidence session (cacheBytes, activeBytes), while sync.ts reads read.modelsCache and onDiskCatalog. Only the derivation below those reads is common.
♻️ Proposed shared helper
Add to src/codex/catalog/metadata.ts:
export interface AccountNativeSlugProjection {
readonly accountNativeSlugs: readonly string[];
readonly accountNativeSlugsBySelector: ReadonlyMap<string, readonly string[]>;
/**
* Always empty. Unknown account-native ids have no safe bare/global identity, so they are
* projected only through the selector map. The no-selector catalog remains the static
* native/API-key surface.
*/
readonly observedNativeSlugs: readonly string[];
}
export function projectAccountNativeSlugs(
config: AccountSelectorConfig,
accountSelectors: readonly string[],
observedEntries: readonly RawEntry[],
): AccountNativeSlugProjection {
if (accountSelectors.length === 0) {
return {
accountNativeSlugs: [],
accountNativeSlugsBySelector: new Map(),
observedNativeSlugs: [],
};
}
return {
accountNativeSlugs: accountBoundNativeOpenAiSlugs(observedEntries),
accountNativeSlugsBySelector: accountBoundNativeOpenAiSlugsBySelector(config, observedEntries),
observedNativeSlugs: [],
};
}Then in this file:
- const accountNativeSlugs = accountSelectors.length > 0
- ? accountBoundNativeOpenAiSlugs(observedAccountNativeEntries)
- : [];
- const accountNativeSlugsBySelector = accountSelectors.length > 0
- ? accountBoundNativeOpenAiSlugsBySelector(config, observedAccountNativeEntries)
- : new Map<string, readonly string[]>();
- // Unknown account-native ids have no safe bare/global identity. They are only projected through
- // selector-qualified rows when a live selector is configured.
- const observedNativeSlugs: string[] = [];
+ const { accountNativeSlugs, accountNativeSlugsBySelector, observedNativeSlugs } =
+ projectAccountNativeSlugs(config, accountSelectors, observedAccountNativeEntries);Apply the same replacement at src/codex/catalog/sync.ts:1319-1327. Both nativeBackfillSlugs spreads keep working, because observedNativeSlugs stays a spreadable iterable.
🤖 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/codex/convergence.ts` around lines 238 - 246, Extract the shared
account-native slug derivation into projectAccountNativeSlugs in metadata.ts,
returning accountNativeSlugs, accountNativeSlugsBySelector, and an always-empty
observedNativeSlugs with the existing no-selector behavior. Replace the
duplicated derivation in the convergence flow and sync flow, while keeping their
observed-entry sources separate and preserving the existing consumers’
spreadable values.
| When account selectors are enabled, the sync path may also observe exact, visible, API-supported | ||
| OpenAI-family ids from Codex's user-owned catalog/cache. Only rows with native catalog provenance | ||
| are trusted; unknown ids are carried through startup cache invalidation as hidden observations and | ||
| are emitted only as selector-qualified rows whose account provenance matches. They never expand | ||
| the bare native or API-key model list. This keeps account-scoped upstream ids such as | ||
| `gpt-daybreak-blue-latest` callable without treating them as a static release allowlist. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the provenance claim.
“Only rows with native catalog provenance are trusted” is incorrect. A complete hand-written cache row is accepted by design. The selector mapping also falls back to eligible main selectors when no account marker exists.
Describe this as a native-row plausibility check. State that it does not verify upstream or account provenance. This document otherwise contradicts docs-site/src/content/docs/guides/codex-app-models.md Lines 22-24 and docs-site/src/content/docs/reference/configuration/routing.md Lines 45-48.
🤖 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 `@structure/03_catalog-and-subagents.md` around lines 35 - 40, Update the
catalog synchronization description to replace the native catalog provenance
claim with a native-row plausibility check. State that complete hand-written
cache rows are accepted, the check does not verify upstream or account
provenance, and selector mapping falls back to eligible main selectors when no
account marker exists; preserve the documented handling of unknown ids and
selector-qualified rows.
|
Full-suite result at |
Review follow-up on top of @Yuxin-Qiao's commits. The behavior is kept; what changes is the claim made about it. hasNativeCatalogProvenance checked only field shape — base_instructions non-empty, comp_hash string-or-null, shell_type, a reasoning-levels array, model_messages an object. There is no signature, source identity, or server attestation, and models_cache.json is a user-owned file, so a complete hand-written row passes. I reproduced that directly before changing anything: a forged gpt-* row with those fields is accepted. Calling that provenance is the problem, not the acceptance itself. It grants nothing new — router.ts already routes any bare gpt-* id under an account selector regardless of the catalog, so the effect is that a poisoned row gets ADVERTISED through discovery, not that a new route or credential becomes reachable. And anyone who can rewrite that cache can already edit config.json or run ocx directly. So the predicate is renamed to say what it does, the two docs pages drop the provenance language, and a test pins the accepting behavior with the reasoning attached. A future reader who wants rejection needs a real provenance signal, not a longer list of fields to match. The existing minimal-row test is kept and retitled: filtering malformed rows is what this check is actually for.
57dccdd to
54c21ed
Compare
|
Rebased onto current
Verification at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54c21ed161
ℹ️ 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 eligible = generatedSelector !== undefined | ||
| ? (mainSelectors.has(generatedSelector) ? [generatedSelector] : []) | ||
| : markedSelectors.length > 0 | ||
| ? markedSelectors.filter(selector => mainSelectors.has(selector)) | ||
| : [...mainSelectors]; |
There was a problem hiding this comment.
When a user renames a selector that maps to @main, previously preserved unknown models disappear: after the first convergence the only evidence is either a generated row carrying the old selector or a hidden observation whose marker lists the old selector, and both branches here reject the model because the old name is no longer in mainSelectors. Subsequent catalog and /v1/models generation therefore omit the model permanently even though the new selector targets the same account. Associate observations with the account target rather than the transient public label, or make every current main-account selector eligible for main-account observations.
Useful? React with 👍 / 👎.
3 participants
Summary
gpt-daybreak-blue-latestthat are not yet in opencodex's static native set.dev@567a6d4ffe6a7d7b341b5cc11e736081e1bb4522; current head:ddc192c668cf36d13ff189b71363341c4f01d81b.gpt-daybreak-blue-latest, successful direct OpenCodex invocation, and the native/API-key alias mismatch this PR addresses.Closes #1480
Verification
tests/native-model-toggle.test.ts,tests/codex-convergence-account-selectors.test.ts,tests/claude-models-discovery.test.ts,tests/codex-catalog-sync-hardening.test.ts, andtests/codex-models-cache-invalidate.test.ts.gpt-daybreak-blue-latestpath.origin/devreproduced the same baseline/environment failures in the broader focused set; those failures are outside this catalog diff. The repository-wide suite is therefore not claimed green.bun run typecheck— passed.bun run privacy:scan— passed.git diff --check origin/dev...HEAD— passed.Checklist
dev.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit
New Features
Bug Fixes