fix(codex): honor tri-state fastMode in Codex config injection - #1022
Merged
lidge-jun merged 2 commits intoAug 5, 2026
Conversation
harryzhou2000
requested review from
Ingwannu,
Wibias and
lidge-jun
as code owners
August 4, 2026 18:51
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughCodex configuration injection now treats ChangesCodex fast mode injection
Estimated code review effort: 2 (Simple) | ~10 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)
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: 501642f7da
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/inject.ts`:
- Around line 406-417: Update ensureFastModeFeature to identify the features
table and fast_mode key using TOML-aware parsing, including inline table
comments, quoted table names such as ["features"], and quoted keys such as
"fast_mode". When fastMode is explicit, update the existing matching table/key
rather than appending duplicates, and add regression tests covering each
accepted spelling.
In `@tests/codex-inject-integration.test.ts`:
- Around line 120-154: Extend the fastMode tests around runInject to inspect the
generated opencodex.config.toml profile as well as config.toml. For the explicit
true case, assert the profile contains fast_mode = true; for the unset case,
assert it does not force fast_mode = true. Keep the existing config.toml
assertions unchanged.
🪄 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: 4bb1143f-54f6-4bfb-abb3-b5456cfacfdf
📒 Files selected for processing (8)
docs-site/src/content/docs/guides/codex-integration.mddocs-site/src/content/docs/ja/guides/codex-integration.mddocs-site/src/content/docs/ko/guides/codex-integration.mddocs-site/src/content/docs/ru/guides/codex-integration.mddocs-site/src/content/docs/zh-cn/guides/codex-integration.mdsrc/codex/inject.tstests/codex-inject-integration.test.tstests/codex-inject.test.ts
injectCodexConfig unconditionally wrote [features] fast_mode = true, even when fastMode: false strips service_tier on the wire and when the user's config opted out. Thread the tri-state fastMode through ensureFastModeFeature and buildProfileFile: - true -> fast_mode = true (unchanged) - false -> fast_mode = false (consistent with the wire strip) - unset -> preserve the user's fast_mode; do not add a [features] table Adds unit and integration regression tests for all three states and updates the integration guide (en/ja/ko/ru/zh-cn).
…L spellings - buildProfileFile no longer defaults an unset fastMode to true: the fallback profile omits [features]/fast_mode when fastMode is unset, mirroring the config.toml passthrough instead of re-enabling priority mode. - ensureFastModeFeature recognizes valid TOML spellings ([features] # comment, ["features"]/['features'], quoted "fast_mode" keys) so an explicit fastMode updates the existing table/key rather than appending duplicates. - Integration tests assert the generated profile for all three states and cover the TOML spellings.
lidge-jun
force-pushed
the
fix/codex-fast-mode-tri-state
branch
from
August 5, 2026 05:09
26cebc1 to
f742a3f
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
This was referenced Aug 5, 2026
lidge-jun
added a commit
that referenced
this pull request
Aug 5, 2026
PR #1022 made fast_mode opt-in and documented that unset adds no [features] table. The config example two lines above still showed that table unconditionally, so a reader following the example expected output they will not get. One comment line in each of the five locales, marking the block as conditional.
lidge-jun
added a commit
that referenced
this pull request
Aug 5, 2026
A second audit round caught a sentence claiming nothing inside the span may be non-deterministic or IO-bearing. The span IS the IO - a journal write, two atomic file replacements, and the marking that follows them. The real constraint is narrower: nothing there may perform IO the journal does not account for, because restore replays only what the journal recorded. Also corrects the compatibility claim about PR #1022, now landed. Its two changed call sites sit INSIDE the span, not outside as originally written. The ordering decision survives with a different reason: land it first so the lock is written against the function's final shape. Line citations refreshed against dev at 4685876, with a note that the anchors are the call names rather than the numbers.
lidge-jun
added a commit
that referenced
this pull request
Aug 5, 2026
Six conflicts, fifteen hunks, resolved once. A rebase was tried earlier and stopped at commit 74 of 129 with the same files recurring; the branch is longer now, and merging resolves each hunk a single time while preserving commit SHAs and review anchors. inject.ts took dev in all seven hunks. Every one of ours there was a formatter reflow of the base while dev carried the real tri-state fastMode change from #1022, so taking dev IS the semantic composition rather than a textual keep-both. The write-lock pipeline lives elsewhere in the file and is untouched - ensureFastModeFeature still runs before the candidate bytes are final, and the witness still hashes those final bytes. catalog/sync.ts is the one that needed judgement. Dev still wrote the catalog with a raw atomicWriteFile; keeping that would bypass the catalog write lock this branch added. The permit writer owns the write, and dev's account-bound row count and its restoration helpers survive around it. The rest compose: our permit-guarded cache invalidation beside dev's cooldown worker, our tri-state service evidence beside dev's reworded accessor comment, clientIntegrations beside dev's account-qualified subagentModels contract, and our ownership-aware provider map beside dev's routed-slug replacement map. Verified in a scratch worktree before landing: typecheck clean, 111 focused tests passing, and the lock call edge still reachable from inject.ts.
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
injectCodexConfig()unconditionally forced[features] fast_mode = trueinto~/.codex/config.tomlon everyocx init/ocx start/ocx sync, even when the user'sfastModesetting said otherwise. This made the client-side injection contradict the wire path (which already honors the tri-statefastMode:trueinjectsservice_tier: priority,falsestrips it, unset is passthrough).The injection now follows the same tri-state semantics:
fastMode: true→ writesfast_mode = true(unchanged behavior)fastMode: false→ writesfast_mode = false, consistent with the wire stripfastModeunset → preserves the user's existingfast_modeand no longer creates a[features]table in configs that never had one (fast becomes opt-in)The fallback profile (
opencodex.config.toml) mirrors the flag, and omitsfast_modeentirely when unset.Verification
bun run typecheck— cleanbun test --isolate tests/codex-inject.test.ts tests/codex-inject-integration.test.ts tests/service-tier-capability.test.ts tests/codex-v2-gate.test.ts— 143 pass, 0 failfastModetrue / false / unset), including configs with and without an existing[features]table.Note: the full local suite has unrelated environment-dependent failures in this workspace (server port binding under the sandbox, load flakes); the touched files and all injection tests pass.
Checklist
Closes #1021
Summary by CodeRabbit
New Features
Documentation
Tests