Repository navigation
fix(catalog): explain a suppressed Luna Reserve row in the sync log - #4958
Conversation
The Luna Reserve projection is account-qualified, so it cannot be written without a selector that targets the main Codex account. A fresh authless install has an empty codexAccountNamespaces map, so no selector exists, the projection returns undefined, and the operator gets no row and no reason. An omission has no row to carry an explanation, so catalog sync now emits one warn-once line naming the cause and the action that restores it, in the same place and the same shape as the account-gated native suppression warning. Scoped to an install where authless Codex Desktop routing is effective: a default install wants no Reserve row and is never told about one. Also documents codexAccountPickerEnabled and codexAccountNamespaces, which were absent from the configuration reference entirely.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds reason classification and warn-once diagnostics for omitted Luna Reserve catalog rows. Catalog sync invokes the diagnostics, exports the new API, adds coverage for each suppression case, and documents the related configuration fields and behavior. ChangesLuna Reserve suppression diagnostics
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant CatalogSync
participant ReserveWarn
participant Console
CatalogSync->>ReserveWarn: classify omitted Reserve row
ReserveWarn-->>CatalogSync: suppression reason
CatalogSync->>ReserveWarn: warn once for reason
ReserveWarn->>Console: write cause and restoration action
Merge Risk: ⚪ Minimal · up to The diagnostic is wired to the retained catalog sync path and uses the same state that controls Reserve-row generation. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
리뷰 · 우선순위 74 / 80이 PR는 #4811을 닫기 위한 진단 로그입니다. 지금 selector는 새 파일 설정 참고에 없던 라인 48 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
![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: c6dde0cb44
ℹ️ 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".
| config: ReserveSuppressionConfig, | ||
| input: ReserveSuppressionInput, | ||
| ): ReserveCatalogSuppressionReason | undefined { | ||
| if (!isEffectiveCodexDesktopAuthless(config) || input.mainSelectors.length > 0) return undefined; |
There was a problem hiding this comment.
When disabledModels contains the bare gpt-reserve slug, the final catalog policy intentionally hides every account-qualified Reserve row, but this predicate ignores that setting. An authless installation without a main selector therefore logs instructions to restore a row the operator explicitly disabled, and following the instructions only creates a hidden row. Pass the disabled-model state into this check and omit the warning when bare Reserve is disabled.
Useful? React with 👍 / 👎.
The facade re-export pushed src/codex/catalog/sync.ts from 52 to 58 lines, past its file-size ratchet cap in tests/fixtures/file-size-baseline.json. The ratchet only moves downward by design, so the cap cannot be raised. The re-export was never load-bearing: retained-sync.ts imports the warning directly from ./reserve-warn, and the reserve tests already import the projection helpers from ./reserve rather than through the facade.
|
Merging with macOS legs outstanding, and recording why rather than leaving it implicit. At this exact head the full Linux suite (test 1/4 through 4/4), This change is platform-neutral, so waiting on a queue that is both saturated and known-unreliable would delay the work without adding information. The evidence that governs the release is not per-PR macOS legs; it is the full-platform Stating the boundary plainly: this is merged on Linux, gates and cross-platform smoke evidence at its exact head, with macOS coverage deferred to the candidate run rather than claimed here. |
1 participant
Summary
Closes #4811.
With
codexDesktopAuthless: trueon a fresh loopback install, no Luna Reserve row is written andnothing says why. The Reserve projection is account-qualified — its slug is
<selector>/gpt-reserve— socreateReserveCatalogProjectionrefuses whenmainSelectorsisempty (
src/codex/catalog/reserve.ts:25). Selectors come fromvisibleCodexAccountSelectors,which returns
[]whilecodexAccountPickerEnabledis false, and that predicate requires anon-empty
codexAccountNamespacesmap (src/codex/account-namespaces.ts:190-195). A freshinstall has no such map, so the feature the operator just turned on produces nothing.
An omitted row has nowhere to carry a reason, which is the same shape as #4212 for account-gated
natives, so this takes the same answer: catalog sync emits one warn-once line where the selector
inputs are still in scope, naming the cause and the action that restores it. The four causes are
distinguished rather than collapsed — no enabled canonical OpenAI provider, an explicitly
disabled picker, an empty selector map, or a map whose selectors never target the main account —
because they need different fixes.
The diagnostic is scoped to an install where authless Codex Desktop routing is effective. A
default install wants no Reserve row and must never be told about one; an install that stores the
flag where it cannot take effect is a different silence, already reported as
inertReasonbydescribeCodexDesktopSwitches, and is not repeated here.This deliberately does not decouple the projection from the selector map. Emitting a bare
gpt-reserverow outside the account-qualified path is the capability change requested in #3844,which was closed as not planned; the issue text asks for the diagnostic and the documentation
instead, and that boundary is respected.
codexAccountPickerEnabledandcodexAccountNamespaceswere absent from the configurationreference entirely, which is why the dependency was undiscoverable. Both are now documented, with
the Reserve consequence stated on each.
Files
src/codex/catalog/reserve-warn.ts(new) — pure reason resolution plus a warn-once emitter,mirroring
gated-native-warn.ts. TakesincludeAccountBoundNativeOpenAias an argument so itstays off the catalog metadata import graph.
src/codex/catalog/retained-sync.ts— calls it wherereserveMainSelectorsis computed.src/codex/catalog/sync.ts— facade re-export, matching howgated-native-warnis exposed.structure/catalog.md— records the contract under "Accounts, namespaces, and pool rotation";src/codex/is owned by this doc perstructure/manifest.json.docs-site/.../reference/configuration/server.md— the two missing keys.tests/codex-integration/reserve-catalog.test.ts— regression coverage.Verification
Local verification was not run: this lane forbids running the local suite, focused tests,
typecheck, build, install, or the
ocxbinary. Hosted CI is the executable verification for thischange.
Correctness is argued statically from the source: the suppression predicate reproduces the exact
gate chain in
reserve.ts→retained-sync.ts→account-models.ts→account-namespaces.ts,and returns
undefinedwhenevercreateReserveCatalogProjectionwould have produced a row, sothe warning cannot fire on a working install.
Hosted CI jobs that exercise this change:
tests/codex-integration/reserve-catalog.test.ts— seven new cases covering each reason, theno-suppression case, the never-opted-in case, and warn-once behaviour.
tests/ci-workflows/structure-ssot.test.ts/bun run structure:check—structure/catalog.mddoc-map and ownership gate for the touched
src/codex/area.tests/test-layout.test.ts— unchanged; no new test file was added, so nolayout.jsonorfixture bookkeeping is required.
bun run typecheckand the full suite on Linux, Windows and macOS.bun run privacy:scan— the new log line contains no account id, credential, or request body;it names configuration keys only.
Checklist
Summary by CodeRabbit
New Features
Documentation