Repository navigation
fix(codex): edit Codex config.toml through one parser-checked writer (1/2) - #23930
brennanb2025 wants to merge 28 commits into
Conversation
Fixes #22592. After Orca started a Codex agent, every `codex` command (including the Codex app and Codex outside Orca) failed with "duplicate key" in ~/.codex/config.toml. Codex had written the project as `["projects"."<path>"]`; Orca's text matchers only knew the bare `[projects."<path>"]` spelling, so Orca appended a second table, added a second `trust_level` beside a quoted `"trust_level"`, and its mirror into Orca-managed Codex homes kept both spellings (and a second `[hooks.state]`). TOML forbids all of these. Now every Orca edit of a Codex config.toml finds tables and keys by their decoded key path (quoted, bare, literal, spaced, dotted or inline), decides with a real TOML parse (smol-toml, 64-bit integers included), and keeps its text-level edit so the user's comments and formatting survive. Each result is checked before it is written: it must parse, hold the intended value, and leave every other value unchanged. A file that does not parse is repaired only where the damage is the exact shape earlier Orca builds wrote (never a table under an array of tables, and never if a removed value would be lost); otherwise the edit is refused with the file and line, and the launch continues. The same check covers the local atomic writer, the managed-home mirror, settings promotion, the legacy profile cleanup, the SSH relay trust write and the SFTP hook-trust write. While ~/.codex does not parse, a managed Codex home whose config parses is left untouched (logged once and recorded), so state only it holds survives an invalid save; a fresh or equally unreadable managed home gets an unchanged copy so Orca-launched Codex shows Codex's own error, like `codex` typed by hand. Settings are never promoted from, or merged out of, a managed config that does not parse, and one that is reseeded is kept as `.bak`. smol-toml is loaded with createRequire (its declarations are ESM-only, and the CLI builds as CommonJS) and shipped as a packaged runtime dependency so the packaged app and CLI can find it.
…ever taken for managed state A ~/.codex broken only by a duplicate project or hook-trust table was copied verbatim into a fresh managed Codex home, and the mirror's own dedupe then made that copy look like readable managed state: later passes kept it, merged its sections and promoted from it. After the user fixed and trimmed ~/.codex, Orca wrote a deleted marketplace back into the real ~/.codex, and the managed home kept a deleted MCP server and a trusted folder. The copy now ends with a marker comment (`# orca: verbatim copy of <source>; replaced when the source parses`), appended so Codex reports the same error lines as in the user's file. A marked managed config is always reseeded, never kept, merged or promoted from; the marker disappears with the reseed, so it cannot strand.
…e parsed markCodexProjectTrusted wrote ~/.codex first; a refused write there (the file does not parse) threw before the managed-home write ran, so Orca-launched Codex asked for trust again. Each home is now attempted independently and the failure(s) surface afterwards.
Trust-write refusals were dropped by empty catch blocks or warned on every launch. They now go through the once-per-file-and-message reporter; other failures keep their existing handling, and nothing throws into launch.
…tied - A marked copy of a broken ~/.codex is replaced with a fresh config when the user fixes it by deleting or emptying config.toml (managed, account and legacy shared homes). - A file is the copy only if it ends in the marker and does not parse, so a valid user config ending in that text is no longer reseeded. - A broken managed config is kept as .bak before the copy replaces it, and the copy itself is never backed up. - Drop the refusal map and TTL getter that nothing in production read.
Trust-write, promotion, and mirror reports about one config file shared a single last-message slot, so they displaced each other and every one was logged again on every launch. Remember the last message per file and kind; a mirror pass that succeeds forgets only mirror reports, and a trust write that succeeds forgets the trust refusal so a later one is reported again.
The mirror parses every table header through the full key-path parser to decide whether it is a projects or hooks.state table. Reject a header that contains neither the table names nor a backslash (an escape is the only way to spell a key without its letters), and read the canonical [projects."path"] form with one anchored regex. Mirror pass on a 5,000-project config: ~165 ms -> ~116 ms.
…; broken-config handling moves to a follow-up This PR fixes #22592 (Orca's own duplicate tables). It had also grown a separate feature for a ~/.codex the user broke by hand: a marked verbatim copy in the managed home, keeping a working managed copy untouched, a .bak on reseed, a no-source path in the accounts mirror, and skipping settings promotion from the copy. That feature moves to a later PR. For a ~/.codex that does not parse even after Orca's duplicate repair, the managed-home, legacy shared-home and accounts mirrors now do exactly what they did before this PR. Kept: the checked writer, the repair applied to ~/.codex and to the managed config before mirroring, and the rule that a mirror never writes a result Codex cannot parse when every input parses. Settings promotion still never edits a ~/.codex Codex cannot parse. When it has nothing to write back, the mirror proceeds as before; when an in-Codex change is pending, the pass stalls (as for any failed write-back) so the mirror cannot erase that change, and the refusal is logged once. codex-managed-config-validity.ts is gone; the two helpers still used moved beside their callers. A mirror pass now parses whole files 4 times instead of 10.
…write once
When ~/.codex does not parse, the managed home mirrors it as before this PR,
so the managed config.toml does not parse either. Two hook paths then went
wrong on every Codex launch:
- A hook trust edit with nothing to write (no entries to upsert, or no
existing hooks.state table to toggle) still ran the checked writer, which
refuses a file it cannot parse. With status hooks off, the user-hook
refresh returned 'error', warned on every launch, and skipped the trust
provenance snapshot and legacy cleanup that follow. Before this PR it was
a silent no-op. It is again.
- A hook trust write with real entries is correctly refused, but launch prep
logged that refusal on every launch. It is now logged once per file, like
a refused workspace trust write, and launch prep does not repeat it. The
hook status is unchanged ('error' with the same detail).
A trust refusal report now ends only when a trust write actually lands; a
no-op edit proves nothing about the file, and otherwise the no-op above
would re-arm the workspace trust refusal log on every launch.
…write lands A refused Codex trust write is logged once per config file. The local writer forgets that report when a later trust write lands, so a new refusal is logged again; the SSH writer never did, so after one refusal every later refusal of that remote file stayed silent for the session. The remote writer now clears it the same way.
…ort promotion stalls - A project that ~/.codex defines only through a sub-table, such as [projects."/a".extra], was treated as defined inline, so the mirror dropped the managed home's [projects."/a"] trust on every pass (Codex then asked to trust the folder again). Any header under projects."/a" now counts as a table definition, as before this PR. - A settings promotion stall was logged once and never forgotten, so a second stall after a successful promotion was silent. A promotion pass that does not stall now ends the report.
…rsing The sync status is derived from source-side checks only, so it can read as synced while a pass stalls on content (a refused settings promotion or an unparseable merge result); the comment claimed the two can never disagree. The line scanner comment still said Orca has no TOML dependency.
…otted key also sets it A project that ~/.codex defines through a sub-table such as [projects."/a".extra] and also through dotted keys ([projects] with "/a".trust_level = ..., or root projects."/a".trust_level = ...) was counted as table-defined, so the mirror kept the managed home's [projects."/a"] table, the merged file redefined /a, and every mirror pass was refused. A project now counts as table-defined only when it has its own [projects."/a"] header or every key under it comes from a sub-table header.
…ever clear The refusal logging grew per-file, per-kind bookkeeping with clear calls in promotion, the mirror, local and remote trust writes, and a WeakSet of hook statuses in launch prep; three review-loop bugs came from its clearing rules. Replace it with one rule: each distinct (config path, message) logs once per app session, in a bounded set. Launch prep logs a hook status error through the same call instead of checking a WeakSet; hook status values are unchanged. Refusals are still contained exactly as before.
Main (#23744, #23937) moved every agent trust write into one dispatcher, applyWorkspaceTrustOnThisHost, run on the host that starts the agent: main for local and WSL launches, the SSH relay for remote ones. The desktop SFTP trust writer, the trust IPC and the per-call-site launch-prep writes are gone. Re-applied on top: - markCodexProjectTrusted attempts every config.toml it is given, so a file Orca refuses to edit never leaves another Codex home untrusted. - The dispatcher reports refused Codex config edits once per file and message. - smol-toml is loaded with a literal require so the relay's esbuild bundle inlines it; relay hosts have no node_modules, and createRequire made the relay fail to load. - Remote trust tests ported to the relay writer; hook-status log-once test moved out of the test file main deleted.
… every value under it
A deeper header such as [projects."/a".extra.deep] counted `extra` as a
header-defined sub-table even when a dotted key ("/a".extra.q = 1 under
[projects], or root projects."/a".extra.q = 1) created `extra`. Orca then
kept its managed [projects."/a"] table, the merged file redefined /a, and
the mirror was refused on every pass. A project now counts as table-defined
only when every value under it sits under some [projects."/a"...] header,
checked recursively. Also drops a stale "loaded via createRequire" note:
smol-toml is now loaded with a literal require.
The relay runs the checked writer on the remote host, and SSH hosts may run Node 18, which has no Array.prototype.toReversed. The repair and the hook enable/disable edit iterated with it, so repairing a config an older Orca broke threw a TypeError there and left the file broken.
Review status: ready for reviewThe problem: after Orca trusted a folder, every What changes for users:
What the review changed (6 rounds including narrow re-checks, plus an architecture challenge and a final readiness pass):
Verified:
Not verified:
CI: all required checks green on head |
…ership #23930 merged current main, whose #23744/#23937 moved trust writing into applyWorkspaceTrustOnThisHost (local) and the SSH relay. Resolved by taking that design and dropping what it already covers: the per-home trust writes, the launch-prep trust writes and their shared budget, and the worker-start trust diagnosis (moved to a follow-up; #23847 is now referenced, not fixed). Kept on top of it: - Local Codex trust also reaches the per-account home the launch reads: the launch env's CODEX_HOME joins getLocalCodexTrustConfigFiles, as the relay already does. - An OS error that stops a trust write names config.toml, not the atomic writer's temp file; config refusals pass through unchanged. - The WSL hook-trust relocation now logs through warnCodexConfigOnce and writes through withConfigPath, the base's single refusal and write rules.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add TOML parsing, duplicate repair, and checked-edit utilities for Codex configuration. Project and hook trust updates now use checked edits and validated writes. Managed and legacy runtime configuration mirroring includes additional preparation, merge, and validation behavior. Workspace-trust operations continue across configured files and report refusals. Tests cover TOML variants, malformed configurations, mirroring, relay bundling, and warning deduplication. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to In uncommon cases, settings promotion can write a TOML date Codex rejects, and a refused mirror can leave settings unapplied while reporting them as synced. These are bounded configuration and sync failures; merge with owner awareness and prioritize the fixes. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ).filter( | ||
| // Why: ~/.codex defines this project inline or with dotted keys, so a | ||
| // runtime `[projects."…"]` table beside it would redefine it. | ||
| (section) => | ||
| !isRuntimeProjectTomlSection(section.header) || | ||
| !systemInlineProjects.has(getTomlSectionHeaderKey(section.header)) |
There was a problem hiding this comment.
~/.codex defines a project with inline or dotted keys but no trust_level, this filter drops the managed home's trusted project table. For example, a system project containing only "/repo".extra.q = 1 causes an existing managed [projects."/repo"] trust entry to disappear on the next mirror pass. Codex then loses that trust even though the user never revoked it.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the whole PR: one parser-checked writer now owns every Orca edit of a Codex config.toml, plus a duplicate-repair pass, replacing the previous per-spelling text matchers.
- New parser/atomic-edit layer —
codex-config-toml-document(smol-toml@1.8.0via literalrequire, BOM/CRLF aware,integersAsBigInt: 'asNeeded'),codex-config-toml-structure(decoded key paths, table blocks, Node-18-safe), andcodex-config-toml-checked-edit(applyCheckedCodexConfigTomlEditrefuses an edit unless the result parses, holds the intended value, and leaves every unowned value unchanged). - Repair —
codex-config-toml-repairremoves only the exact Orca-appended duplicate shapes (keeps the user's first definition, re-parses to prove no removed value was lost) so an already-broken~/.codexself-heals on the next trust write. - Every write routed through it — project trust (
config-toml-project-trustmatches inline/dotted/sub-table/escaped spellings), hook trust upsert/remove/enabled (config-toml-hook-trust-edit), settings promotion, the managed-home mirror, and the legacy shared home split intocodex-config-mirror-{merge,source}/codex-config-legacy-shared-home-mirror. - Refusal handling —
CodexConfigTomlEditRefusedError(reason/line/configPath),warnCodexConfigOncebounds the once-per-file+message log at 500 entries;markCodexProjectTrustednow attempts every home and rethrows;reportCodexTrustWriteRefusalslogs refusals once and returns other failures. - Relay/SSH — the same writer runs on the remote host;
relay-codex-config-toml-bundle.test.mjsbundles the real esbuild externals and proves smol-toml is inlined and Node-18 array methods suffice.
The design's safety nets are the strongest part: a wrong merge result degrades to a refusal that keeps the last good copy and logs once, not a corrupt file. I read every TOC region and ran the new/targeted suites locally (checked-edit, mirror-duplicate-repair, syntax, config-toml project trust, stale writes, agent-trust-presets, launch-hook warning, hook-service user-hook mirroring, relay bundle, relay trust spawn) — all pass. The one acknowledged gap (managed-home duplicate path exercised in units only, not live) is disclosed in the PR body.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Adopts #23958's rules for Codex's quoted config.toml spellings and keeps one implementation of each piece: - Repair: #23958's Orca-trust duplicate rules (the user's table wins in either order) run first inside the checked writer's repair; the checked writer then drops only duplicates that add no value. Its own Orca-block and trust-line rules, and #23958's separate refusal logger, are removed. - Trust writes stay on the checked writer; an existing quoted trust_level with a different value is rewritten bare, as #23958 does. - Header matchers, the hooks.state parent check and the section scanner use the checked writer's helpers with #23958's rules; #23958's duplicates are removed. getProjectTrustLevel uses #23958's trust_level reader. - Plugin and project hook trust carry is wired into the split mirror files.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review (60dcc2e), the branch merged origin/main, which brought main's own fix for the same #22592 duplicate-key bug (#23958, same author) and reconciled it with this branch's parser-checked writer. The two implementations are wired coherently: this branch's duplicate repair now delegates to main's, and the targeted suites, both typecheck projects, and the relay bundle test all pass locally.
- Merged
origin/main— imported main's quoted-spelling recognition and Orca-duplicate repair, then reconciled it with this branch's checked writer. - Adopted main's repair core — added
config-toml-project-duplicate-repair.tsandconfig-toml-project-trust-level.ts;codex-config-toml-repair.tsnow runsrepairOrcaDuplicateTrustTablesfirst, then collapses any remaining value-neutral duplicates, with refusal logging left to the checked writer. - Carried plain-Codex plugin/project hook trust into managed homes —
classifyHookTrustKey/getHookTrustTomlSectionKeys, asharedHookTrustoption onstripRuntimeOwnedTomlSections, and a newsystemConfigDirargument onmergeSystemCodexConfigIntoRuntime. - Broadened header matching —
ensureHooksStateParentTablenow treats anyhooks.state.*depth as a child, and a quotedtrust_levelkey is rewritten bare in place. - Added coverage — hook-trust carry and spellings, project/hook duplicate repair, and the Node-18 relay bundle.
ℹ️ Two implementations for #22592 now coexist
Main's #23958 already fixes the duplicate-key bug this PR targets. The branch keeps that fix and layers its parser-checked writer on top, which is coherent and tested — but the PR description still presents this as the first and only fix and never mentions #23958. A one-line note on how the two are meant to relate (supersede, absorb, or stay layered) would help reviewers judge the intended end state.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Codex can write "trusted_hash" and "enabled" quoted; main's reader only matched the bare names, so a quoted "enabled" = false read as enabled.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review (ad682c1), the delta is a single test-only commit.
- Pinned quoted hook-trust field names —
config-toml-hook-trust-read.test.tsassertsreadHookTrustContentreads"trusted_hash"/"enabled"when the field names are quoted, both on a file that parses and on a duplicate-key file that falls back to the structural scan. Both cases exercise the production reader end-to-end and read real values (trustedHash: 'sha256:abc',enabled: false), so they would fail against a bare-name-only matcher. The new file passes locally (2/2).
No production code changed in this delta, so no behavioral surface to re-check; the config-toml-hook-trust-read.ts parser refactor the test exercises is pre-ad682c1 branch code, already covered by the prior review.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Brings in #23552 (cfa43e7), which moves a shifted user hook's trust block to its new key (moveHookTrustContent / moveHookTrustEntries) instead of rebasing it through a Codex session. Conflicts and resolutions: - config-toml-hook-trust-edit.ts: ported main's moveHookTrustContent into the checked writer. One applyCheckedCodexConfigTomlEdit call now strips the old and new keys' blocks (every spelling, matched by decoded key) and appends the moved bodies verbatim. Owned paths are the old/new keys plus their spellings; each new key must hold the old block's parsed values; an invalid result is refused. Main's guards stay: nothing to move or a stored key of unknown shape returns the file unchanged without parsing. Dropped main's stale repairOrcaDuplicateTrustTables import (#23930 moved repair into codex-config-toml-repair). appendTrustBlocks became appendTomlBlock, shared by the upsert and the move. - config-toml-syntax.ts: kept main's exported CODEX_HOOK_TRUST_KEY and #23930's decoded-key header parsing. - config-toml-runtime-owned-sections.ts: #23930's import block plus main's CODEX_HOOK_TRUST_KEY; main's local copy of the regex stays removed. - config-toml-trust.ts: union of imports (move, remove, setEnabled, upsert). moveHookTrustEntries keeps main's name and signature but now wraps the edit in withConfigPath and writes with writeTomlConfigAtomically, like its sibling hook-trust writers. Outside the conflicts: codex-user-hook-trust-moves.ts reports a refused move through reportCodexConfigTomlEditRefusal (once per file and message) instead of console.warn on every hooks write. Added tests: a quoted-spelling stale table at the new key leaves one parseable table, and a hand-broken config is left untouched with the refusal logged once.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review (01030ef0), the only new commit is the merge of origin/main (3ab3c92) into the branch. It adopts main's own user-hook-trust rework (#23552) and re-integrates this branch's parser-checked writer with it. I read the net delta on every PR file that changed since 01030ef0, completed the full-diff TOC read, and ran the touched suites plus both typecheck projects; all pass.
- Merged
origin/main, adopting #23552's user-hook-trust rework —codex-user-hook-trust-rebase{,-client}.tsare gone;codex-user-hook-trust-moves.tsreplaces them with a synchronousmutateRealHomeHooksPreservingUserTrust(noruntimeHomePath/restoreHooks).codex-hook-legacy-cleanup.ts,codex-real-home-hook-install.ts,codex-real-home-hook-sweep.ts, andstartup/codex-launch-preparation.tsnow follow main's newer flow. - Routed the shifted-hook trust move through the checked writer — new
moveHookTrustContent(config-toml-hook-trust-edit.ts) runs the move underapplyCheckedCodexConfigTomlEdit, asserts each moved block still holds the source value at its new key, and refuses (once) a config Codex cannot parse; newmoveHookTrustEntries(config-toml-trust.ts) wraps it with the mutation-lane path and atomic write. The move'scatchincodex-user-hook-trust-moves.tsreports a refusal once and leaves other failures to a plain warn. - Deduplicated
CODEX_HOOK_TRUST_KEYintoconfig-toml-syntax.ts(imported byconfig-toml-runtime-owned-sectionsand hook-trust-edit), and generalizedappendTrustBlockstoappendTomlBlock. - Added coverage —
codex-user-hook-trust-moves.test.tsnow pins that a stale table at the new key is replaced with exact bytes and that an unparseableconfig.tomlis left untouched with the refusal logged once.
The merge leaves the branch's writer layer coherent on top of main's move logic: the moved block is the only thing that changes, its value is verified at the new key, and any result that would not parse is refused rather than written. No prior pullfrog threads were open, and no new issue surfaced.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ite is refused When ~/.codex/config.toml can't be parsed, the managed mirror copies it, so the managed-home trust write is refused first. Its catch returned before the legacy sweep, so a retired Orca entry stayed in ~/.codex/hooks.json and the user hook's trust move was never attempted (found in Windows QA). The sweep now runs on that error path too; its move is refused and logged once, as on any unreadable file. The status detail no longer ends "parse.. Run /hooks in Codex to approve." for an unreadable file, where /hooks can't help.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the prior pullfrog review (b6afcb5), the only new commit is 0262518, a Windows-QA follow-up to the refused-trust-write path.
- Legacy sweep now runs on a refused trust write —
installCodexHooksExclusivelyandrefreshCodexRuntimeUserHooksExclusivelycallcleanupLegacyManagedHookRepresentations()in theircatchbefore returning. Previously the refused managed-home trust write returned early and left a retired Orca entry in~/.codex/hooks.json, and never attempted the user hook's trust move. The sweep was already on each function's success path; it swallows its own errors and re-enters the system-config lane through the AsyncLocalStorage pass-through. - Centralized the hook-trust failure detail — new
describeHookTrustWriteFailure(summary, error, approveAdvice)returns${summary}: ${error.message}without the/hooksadvice for areason === 'input-invalid'refusal (where/hookscannot help), and otherwise strips trailing periods before appending the advice. Replaces the inline templates in the local install, local maintenance, remote install, and WSL runtime status details. - Added coverage —
hook-service-refused-trust-legacy-sweep.test.ts(hooks on and off) hand-breaks~/.codex/config.tomlso the mirror copies it and the managed trust write is refused, then asserts the retired entry is removed while the user hook survives, the broken TOML is byte-identical, the detail carries no..or/hooks, and the trust-move refusal logs once across two launches.
The delta is narrow and coherent: the sweep is idempotent and self-error-swallowing, and the new test targets exactly the pre-fix behavior (without the catch sweep the retired entry would remain and no move refusal would be logged). Verified locally: the new test passes 2/2, the related hook-service-user-hook-mirroring and hook-service-legacy-cleanup suites pass (21), the relay bundle test passes, and pnpm tc:node / pnpm tc:cli are clean. There were no open pullfrog review threads to retire.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Removing a retired Orca entry from ~/.codex/hooks.json shifts the user hooks after it, so their approvals must move in config.toml in the same step. When config.toml can't be parsed that move is refused, and sweeping anyway left the user's approval at the old key and the retired entry's trust at the new one: after the user fixed the file, Codex asked them to review their own hook again. The retired-entry sweep now plans the config.toml edit first and, if the file can't be read, leaves hooks.json untouched and logs once. The next launch re-derives the sweep from hooks.json and moves the approval cleanly. A refusal no longer escapes as a stack trace, and the legacy profile cleanup no longer depends on the sweep succeeding. The real-home lanes are unchanged.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes — since the prior pullfrog review (0262518), two commits replace "sweep ~/.codex/hooks.json even when the trust write is refused" with an explicit wait.
- Waited instead of sweeping while
config.tomlis unreadable — newplanRetiredHookSweepTrustcomputes theconfig.tomlside (the shifted user-hook trust moves and the retired entries' own trust removal) beforehooks.jsonis touched; aninput-invalidrefusal now leaves both files byte-identical, logs once, and re-derives the sweep on the next launch. - Extracted the planner into
codex-retired-hook-sweep-trust.ts, and listed it in the CLI project socodex-hook-legacy-cleanup.tstype-checks undertc:cli. - Separated the legacy profile cleanup into its own
try/catchso it still runs when the system sweep throws. - Rewrote the coverage in
hook-service-refused-trust-legacy-sweep.test.tsto prove the wait (both files byte-identical, one log) and the eventual sweep and approval move once the file is fixed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| collectManagedTrustEntries, | ||
| removeSelfComputedMatchingTrustEntries | ||
| } from './codex-hook-trust-cleanup' | ||
| import { collectManagedTrustEntries } from './codex-hook-trust-cleanup' |
There was a problem hiding this comment.
removeSelfComputedMatchingTrustEntries lost its only caller in this commit: planRetiredHookSweepTrust inlines the same self-computed-hash match (trust.get(computeTrustKey(entry))?.trustedHash === computeTrustedHash(entry)) instead of reusing it. If step 2 (#23931) does not reintroduce a caller, delete the now-unused export in codex-hook-trust-cleanup.ts (or have the planner reuse it) so the logic does not live in two places.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 1cb9ea59-7ee4-4d02-b3cf-57f84e273a99
📒 Files selected for processing (4)
config/tsconfig.cli.jsonsrc/main/codex/codex-hook-legacy-cleanup.tssrc/main/codex/codex-retired-hook-sweep-trust.tssrc/main/codex/hook-service-refused-trust-legacy-sweep.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Reject non-Codex TOML values before promotion. · codex-config-toml-document.ts:17-39
src/main/codex/codex-config-toml-document.ts:17-39
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject non-Codex TOML values before promotion.
smol-toml1.8.0 normalizes impossible dates such as2023-02-30instead of rejecting them. During settings promotion, raw values from the runtime config pass throughupsertPromotedSettingsInContentandapplyCheckedCodexConfigTomlEdit. The checked edit validates only withparseCodexConfigToml, then the promotion writes the result. Therefore, a runtime value such asmodel = 2023-02-30can replace a valid system value and produce a file that the Codex TOML reader cannot load, while Orca reports a successful write. Use Codex-compatible validation for promoted values before writing.
🟡 Minor · Report a stall after refusing invalid output. · codex-config-mirror.ts:75-79
src/main/codex/codex-config-mirror.ts:75-79
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport a stall after refusing invalid output.
The new
refused-invalidbranch returns before it reports a sync outcome. The Accounts/config-sync handler recomputes status from the selected home and can returnsyncedwhen the source and existing managed config are readable. The mirror can therefore keep the previous settings while the status reports that the settings were applied. Record an explicit stalled status for this branch, using the same status channel asrefused-indeterminate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 76449d0b-0630-4557-8046-c55cbc0165ba
📒 Files selected for processing (1)
src/main/codex/hook-service-refused-trust-legacy-sweep.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
ELI5
Orca edits Codex's settings file (
~/.codex/config.toml) to mark your folders as trusted and to install its status hook. It did this with text patterns that only recognised one way of writing each setting. Codex writes some of them differently, so Orca sometimes added a second copy of a section. A settings file with the same section twice is invalid, so everycodexcommand stopped working, even outside Orca. Now every Orca edit goes through one checked writer. It reads the file with a real parser, never leaves it invalid, and keeps your comments. It also removes the duplicates older Orca versions already wrote.What Changed
The problem users see (#22592): after Orca trusts a folder,
codex,codex execandcodex login statusall fail withError loading config.toml: duplicate key, including when you runcodexoutside Orca.["projects"."/path"], but Orca looked only for[projects."/path"]. Not finding it, Orca appended a second section for the same folder.trust_levelkey, for Orca's hook-trust sections, and in Orca's own copy of your settings, the "managed Codex home" that Orca-launched Codex uses.Scope:
main: a~/.codex/config.tomlyou broke yourself (for example a line with no value). Codex shows its own error, and Orca never edits a file it can't read.Before:
After:
config.toml. It covers:~/.codexand Orca's managed Codex homes;~/.codex, the previous version is kept asconfig.toml.bak, as for every Orca write there; the next write replaces it. Orca's own Codex home is rebuilt from~/.codexand keeps no backup, as onmain.~/.codexcan't be edited, Orca still trusts the folder in its managed Codex home, and the reverse.enabledline: when Orca carries a disabled hook's state onto a hook-trust section that has noenabledline, it now writesenabled = false. Before, that section stayed enabled.~/.codexis broken: say you change a setting inside Orca-launched Codex (for example/model), then break~/.codexby hand before Orca copies the change back. Orca now holds the change and skips copying settings from~/.codexinto its managed home until~/.codexparses again. It logs this once and then copies the change back, and Orca-launched Codex keeps working meanwhile. Before, Orca wrote into the broken file and copied the broken file into its managed home.hooks.jsonchange happens. Ifconfig.tomlcan't be read, the approval move is refused and logged once, and Codex lists the moved hook for review. Onmainthe move edited the unreadable file.~/.codex/hooks.json, it plans theconfig.tomledit first. If the file can't be read, it leaves both files alone and logs once ("Waiting to remove retired Orca hook entries until config.toml can be read"). The first launch after you fix the file removes the entry and moves your approval cleanly, so Codex doesn't ask you to review your own hook again. Removing the entry anyway would strand your approval at the old position."/x" = { ... }), and hook trust written as dotted keys or inline tables. Codex itself always writes table headers.smol-toml, pinned to 1.8.0. It was already in the lockfile through another package, so the lockfile change is three lines. It's loaded with a plainrequire:node_modules.Why
[projects."..."]section Codex writes. Both throw on a file with duplicates, so the repair step would still be needed.-coption: trust must work however Codex is launched (hand-typed, scripts, cmd.exe, company-specificcodexwrappers), and the settings file is the one thing every launch reads.trust_levelkey, hook trust, the managed home, and already-broken files.This PR is step 1 of 3. The plan fixes how Orca edits Codex's config, trust and hooks:
codexstartup #22592.~/.codexby hand. Today, as onmain, Orca copies the broken file into its managed Codex home, so Orca-launched Codex shows the same error ascodexin a terminal. The follow-up decides whether Orca should keep its working copy instead. It was split out of this PR during review so the bug fix could be judged on its own.Linked Issue
Fixes #22592
Visual Proof
Live on an M4Air Mac, with real Codex (
codex login status) and an isolated test home.main.main.Before: Orca appends a second section and Codex fails with
duplicate key.After: one section, the comment kept, and Codex loads ("Not logged in" is expected in the isolated home).
Scenario 2 is a file an older Orca already broke. Before: it stays broken.
After: the duplicate is repaired, and Codex loads.
Scenario 5 launches Codex in a linked worktree that Codex had already trusted. After: one section, and Codex loads.
Testing
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Live QA (M4Air, before/after as above):
~/.codexdirectly. Unit tests cover it.Live QA (Windows high spec, Codex 0.159.2, an isolated copy of Orca built from this branch; the machine's real
~/.codexwas hash-checked and never changed):[projects.'c:\users\...']), isn't added a second time. Codex opens there with no trust prompt.codexstartup #22592 duplicate-key file is repaired both in~/.codexand in Orca's own Codex home, and Codex loads both. The Mac QA couldn't reach Orca's own home; Windows always uses it."enabled" = falsehook stays off inside Orca.config.toml, the retired-entry cleanup waits, and after the file is fixed it moves the approval cleanly. QA found the first version of this wrong; it's fixed here, with tests.smol-tomlfrom the packaged runtime modules, and CI's Windows package check confirms it ships. A full packaged launch on Windows wasn't run.New tests cover:
~/.codexmirrored exactly as onmain;node_modules, and repairing on Node 18 (Node 20-only array methods removed).The key tests fail with their fix removed.
A mirror comparison with
mainover thousands of generated config shapes: the managed Codex home is never written unparseable, and a hand-broken~/.codexgives the same bytes as onmain.Typecheck is clean for the node and CLI projects. oxlint, oxfmt and the changed-code quality gate are clean. The targeted suites pass.
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
main, trust on an SSH host is written by Orca's relay on that host (feat(agents): pre-trust the folder wherever Orca starts an agent #23744). This PR routes that write through the same checked writer, withsmol-tomlinlined into the relay bundle and no Node 20-only APIs. The remote hook installer, which writes over SFTP, also goes through the checked writer. There's no protocol change, so mixed desktop and relay versions are safe; each desktop deploys its own relay.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)