Repository navigation
fix(codex): never write a Codex config.toml that Codex can't load, and approve both symlink spellings - #25741
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📥 CommitsReviewing files that changed from the base of the PR and between 3c8e2eb2261cf6b6e85ac52c13ae2180f21a39e5 and 76f82d4. 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughReal-home hook trust operations now recognize normalized and explicitly resolved path spellings, propagate approvals across those spellings, and include aliases in trust movement and cleanup. TOML trust reads include entries from Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The previously identified approval-reading and retired-hook cleanup gaps are addressed. No outstanding issue in the supplied evidence prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes preserve hook content identity while adding protection against invalid configuration writes. No introduced authorization bypass was established. Recovery, concurrent edits and disable-state propagation across aliases retain some uncertainty. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
69d4acb8-f40e-4489-9cdf-da44a875572f
📒 Files selected for processing (18)
src/main/codex/codex-hook-legacy-cleanup.tssrc/main/codex/codex-hook-remote-install.tssrc/main/codex/codex-hook-trust-cleanup.test.tssrc/main/codex/codex-hook-trust-cleanup.tssrc/main/codex/codex-hook-user-mirroring.tssrc/main/codex/codex-managed-trust-reconciliation.tssrc/main/codex/codex-real-home-hook-install.tssrc/main/codex/codex-real-home-hook-key-spellings.test.tssrc/main/codex/codex-real-home-hook-sweep.tssrc/main/codex/codex-real-home-hook-withdrawal.tssrc/main/codex/codex-real-home-hooks-json.tssrc/main/codex/codex-trust-identity.tssrc/main/codex/codex-user-hook-trust-moves.test.tssrc/main/codex/codex-user-hook-trust-moves.tssrc/main/codex/config-toml-hook-trust-read.tssrc/main/codex/config-toml-trust-loadability.test.tssrc/main/codex/config-toml-trust.tssrc/main/codex/hook-service-managed-install.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
| const hooksWritePath = resolveHooksJsonWritePath(legacyConfigPath) | ||
| mutateRealHomeHooksPreservingUserTrust({ | ||
| sourcePath: legacyConfigPath, | ||
| sourcePaths: getRealHomeHookKeySourcePaths(), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
ast-grep outline src/main/codex/codex-hook-legacy-cleanup.ts --match sweepLegacySystemManagedHooks --view expanded
rg -n -C 12 'function (collectManagedTrustEntries|removeSelfComputedMatchingTrustEntries)|export function (collectManagedTrustEntries|removeSelfComputedMatchingTrustEntries)' src/main/codexRepository: stablyai/orca
Length of output: 4107
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- cleanup source ---'
nl -ba src/main/codex/codex-hook-legacy-cleanup.ts | sed -n '1,145p'
printf '%s\n' '--- trust cleanup source ---'
nl -ba src/main/codex/codex-hook-trust-cleanup.ts | sed -n '1,115p'
printf '%s\n' '--- definitions and calls ---'
rg -n -C 4 'function (computeTrustKey|computeTrustedHash|readHookTrustEntries)|computeTrustKey\\(|collectManagedTrustEntries\\(|removeSelfComputedMatchingTrustEntries\\(|getRealHomeHookKeySourcePaths\\(' src/main/codex
printf '%s\n' '--- PR diff for reviewed file ---'
git diff --unified=8 9756daeced4eda1dc888f14fefeeed1762aa5822 e51810c750f9a9d50a882534ecb67b265e31c577 -- src/main/codex/codex-hook-legacy-cleanup.tsRepository: stablyai/orca
Length of output: 12188
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- identity and key definitions ---'
rg -n -F 'function createCodexHookTrustEntry' src/main/codex || test "$?" -eq 1
rg -n -F 'export function createCodexHookTrustEntry' src/main/codex || test "$?" -eq 1
rg -n -F 'function computeTrustKey' src/main/codex || test "$?" -eq 1
rg -n -F 'export function computeTrustKey' src/main/codex || test "$?" -eq 1
rg -n -F 'function getRealHomeHookKeySourcePaths' src/main/codex || test "$?" -eq 1
rg -n -F 'export function getRealHomeHookKeySourcePaths' src/main/codex || test "$?" -eq 1
printf '%s\n' '--- related identity and path usages ---'
rg -n -F 'createCodexHookTrustEntry(' src/main/codex
rg -n -F 'getRealHomeHookKeySourcePaths' src/main/codex
printf '%s\n' '--- PR diff ---'
git diff --unified=8 9756daeced4eda1dc888f14fefeeed1762aa5822 e51810c750f9a9d50a882534ecb67b265e31c577 -- src/main/codex/codex-hook-legacy-cleanup.tsRepository: stablyai/orca
Length of output: 6078
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- trust entry identity ---'
nl -ba src/main/codex/codex-hook-identity.ts | sed -n '1,100p'
printf '%s\n' '--- trust key and source path normalization ---'
nl -ba src/main/codex/config-toml-trust.ts | sed -n '45,115p'
rg -n -F 'function normalizeHookTrustKeyForLookup' src/main/codex/config-toml-trust.ts || test "$?" -eq 1
rg -n -F 'export function normalizeHookTrustKeyForLookup' src/main/codex/config-toml-trust.ts || test "$?" -eq 1
rg -n -F 'function normalizeCodexHookSourcePath' src/main/codex || test "$?" -eq 1
rg -n -F 'export function normalizeCodexHookSourcePath' src/main/codex || test "$?" -eq 1
printf '%s\n' '--- real-home source spellings ---'
nl -ba src/main/codex/codex-real-home-hooks-json.ts | sed -n '1,85p'
printf '%s\n' '--- user trust preserving mutation ---'
nl -ba src/main/codex/codex-user-hook-trust-moves.ts | sed -n '1,125p'Repository: stablyai/orca
Length of output: 13598
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- system hooks config path binding ---'
rg -n -F 'function getSystemConfigPath' src/main/codex/codex-hook-definition.ts || test "$?" -eq 1
rg -n -F 'export function getSystemConfigPath' src/main/codex/codex-hook-definition.ts || test "$?" -eq 1
nl -ba src/main/codex/codex-hook-definition.ts | sed -n '1,90p'
printf '%s\n' '--- underlying trust key implementation ---'
sed -n '1,45p' src/main/codex/config-toml-trust.ts | nl -ba
rg -n -F 'export function computeCodexTrustKey' src/main/codex
rg -n -F 'function computeCodexTrustKey' src/main/codexRepository: stablyai/orca
Length of output: 6483
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- getSystemConfigPath implementation ---'
nl -ba src/main/codex/codex-hook-definition.ts | sed -n '112,132p'
printf '%s\n' '--- computeCodexTrustKey implementation and path canonicalization ---'
nl -ba src/main/codex/codex-trust-identity.ts | sed -n '1,112p'Repository: stablyai/orca
Length of output: 5593
🏁 Script executed:
#!/bin/bash
set -eu
nl -ba src/main/codex/codex-trust-identity.ts | sed -n '107,180p'Repository: stablyai/orca
Length of output: 3487
Collect retired-hook trust entries for both home-key paths.
When the real home is symlinked, the hook mutation handles both the spelled and resolved paths, but collectManagedTrustEntries receives only legacyConfigPath. Removal checks only keys computed from the collected entries, so a resolved-key approval can remain after the retired hook is removed. Collect entries for both paths and pass that same path list to the mutation.
🐛 Suggested fix
const nextHooks = { ...config.hooks }
+ const trustSourcePaths = getRealHomeHookKeySourcePaths()
const trustEntries: CodexTrustEntry[] = []
let removedManagedHook = false
for (const [eventName, definitions] of Object.entries(nextHooks)) {
if (!Array.isArray(definitions)) {
continue
}
- const eventTrustEntries = collectManagedTrustEntries(
- legacyConfigPath,
- eventName,
- definitions,
- isRetiredCodexHookCommand
+ const eventTrustEntries = trustSourcePaths.flatMap((sourcePath) =>
+ collectManagedTrustEntries(
+ sourcePath,
+ eventName,
+ definitions,
+ isRetiredCodexHookCommand
+ )
)
...
- sourcePaths: getRealHomeHookKeySourcePaths(),
+ sourcePaths: trustSourcePaths,| if (!result.has(key)) { | ||
| result.set(key, state) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '25,160p' src/main/codex/config-toml-hook-trust-read.ts
sed -n '180,205p' src/main/codex/config-toml-trust-loadability.test.ts
sed -n '1,55p' src/main/codex/codex-hook-trust-cleanup.test.ts
rg -n '"smol-toml"|smol-toml' package.json pnpm-lock.yamlRepository: stablyai/orca
Length of output: 6517
🏁 Script executed:
sed -n '1,90p' src/main/codex/config-toml-hook-trust-read.ts
rg -n -F -- "trusted_hash = '" src/main/codex || test "$?" -eq 1
rg -n -F -- 'conflictingTrustedHashKeys' src/main/codex
rg -n -F -- 'trusted_hash' src/main/codex/config-toml-trust-loadability.test.ts src/main/codex/config-toml-hook-trust-read.ts
rg -n -F -- 'function normalizeCodexHookTrustLookupKey' src/main/codex
rg -n -F -- 'normalizeCodexHookTrustLookupKey' src/main/codex
sed -n '190,203p' package.json
test -d node_modules/smol-toml && { echo '--- package source available'; rg -n -F -- 'function parse' node_modules/smol-toml; } || echo 'smol-toml package source unavailable in node_modules'Repository: stablyai/orca
Length of output: 7209
🌐 Web query:
smol-toml 1.8.0 parse TOML literal strings duplicate table implementation documentation
💡 Result:
For **smol-toml 1.8.0**:
- **Literal strings:** The TOML spec defines single-quoted literal strings as having no escape processing, so backslashes are preserved; triple-single-quoted strings can span lines, with the first newline after the opener trimmed. The available smol-toml documentation describes the package as a TOML 1.1 parser, but I couldn’t verify these details against the specific **1.8.0** source. ([github.com](https://github.com/toml-lang/toml/blob/main/toml.md?plain=1&utm_source=openai))
- **Duplicate tables:** TOML says defining a table more than once is invalid. That establishes the expected behavior for a compliant parser, but the sources I found don’t confirm smol-toml 1.8.0’s exact implementation or error message. ([github.com](https://github.com/toml-lang/toml/blob/main/toml.md?plain=1&utm_source=openai))
The project’s current documentation reflects a later version, so treat these as TOML-spec expectations—not verified 1.8.0 implementation documentation. ([npmjs.com](https://www.npmjs.com/package/smol-toml?utm_source=openai))
Citations:
- 1: https://github.com/toml-lang/toml/blob/main/toml.md?plain=1&utm_source=openai
- 2: https://github.com/toml-lang/toml/blob/main/toml.md?plain=1&utm_source=openai
- 3: https://www.npmjs.com/package/smol-toml?utm_source=openai
🏁 Script executed:
sed -n '220,275p' src/main/codex/codex-trust-identity.ts
sed -n '88,145p' src/main/codex/config-toml-trust-loadability.test.ts
sed -n '35,60p' src/main/codex/codex-hook-trust-cleanup.test.ts
rg -n -F -- 'smol-toml@1.8.0' pnpm-lock.yamlRepository: stablyai/orca
Length of output: 3858
🌐 Web query:
github un-ts smol-toml tag v1.8.0 parser single quoted literal string parse function source
💡 Result:
The repository is **`squirrelchat/smol-toml`** (not `un-ts/smol-toml`). At tag **`v1.8.0`**, the single- and multi-line string parser is **`parseString(ctx)`** in [`src/primitive.ts`](https://github.com/squirrelchat/smol-toml/blob/v1.8.0/src/primitive.ts). It detects a single-quoted literal string with `c === 0x27` (`'`), then disables backslash escape processing for literal strings; it still checks for a closing quote and rejects prohibited control characters. ([github.com](https://github.com/squirrelchat/smol-toml/blob/v1.8.0/src/primitive.ts))
The parser is shared with basic strings; `parseString` is the relevant function, rather than a separate `parseLiteralString`.
Citations:
- 1: https://github.com/squirrelchat/smol-toml/blob/v1.8.0/src/primitive.ts
Read single-quoted hashes without overriding conflicts.
For a valid [hooks.state."key"] table with trusted_hash = 'sha256:…', smol-toml 1.8.0 parses the value as a string, but the header scanner only matches double-quoted values. The scanner records the key without a hash, so readHookTrustContent skips the parsed entry. readHookTrustEntries can then report no approval, allowing stale-trust cleanup to remove it.
Do not fill every undefined hash from parsed state: the scanner also uses undefined to represent conflicting hashes. Extend the header scanner to read literal strings so that conflict handling remains intact.
🐛 Suggested fix
- const hashMatch = /^[ \t]*trusted_hash[ \t]*=[ \t]*"((?:[^"\\]|\\.)*)"[ \t]*(?:#.*)?$/.exec(
- line
- )
+ const hashMatch =
+ /^[ \t]*trusted_hash[ \t]*=[ \t]*(?:"((?:[^"\\]|\\.)*)"|'([^']*)')[ \t]*(?:#.*)?$/.exec(
+ line
+ )
if (hashMatch) {
- trustedHashes.add(unescapeTomlBasicString(hashMatch[1]!))
+ trustedHashes.add(
+ hashMatch[1] === undefined ? hashMatch[2]! : unescapeTomlBasicString(hashMatch[1])
+ )
}![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
- Refuse to write an unloadable
config.toml.writeLoadableHookTrustConfig/assertLoadableHookTrustConfiginconfig-toml-trust.tsparse withsmol-tomland throwCodexConfigTomlRefusedErrorwhen a previously loadable config would become unloadable (an appended[hooks.state."k"]table cannot extend an inlinehooks.state = {...}or a dotted key). Everyhooks.statewriter now routes through it, and the SSH installer asserts before the remote write; the managed install surfacesstate: 'error'instead of corrupting the file. - Read approvals that have no table header.
readHookTrustContentmerges dotted-key / inline-tablehooks.stateentries (table blocks win on collision), so cleanup and mirroring see user- or Codex-written approvals in either form. - Stale runtime-trust cleanup fix.
removeStaleRuntimeHookTrustEntriesnow only skips a key whenexpectedHash !== undefined && expectedHash === state.trustedHash, so an unexpected hashless key (conflicting duplicate tables read as no-hash) is correctly removed. - Both spellings of a symlinked
~/.codex.getRealHomeHookKeySourcePathsreturns the as-spelled and realpath-resolved paths;getExplicitHomeCodexHookSourcePathresolves through the nearest existing ancestor;approveOtherRealHomeKeySpellingscopies Codex's verbatim hash to the resolved key, alias keys prove ownership through the ledger under the primary key, and opt-out removes under both.mutateRealHomeHooksPreservingUserTrustnow takessourcePaths, with all callers updated.
I verified the two load-bearing external claims against the openai/codex source: the hook trust hash is path-independent (hook_hash hashes only event + matcher group/handler), and find_codex_home canonicalizes only the CODEX_HOME branch, so a symlinked home genuinely has two distinct trust keys. The changed tests (plus the trust-API and real-home regression suites) all pass locally.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
…e both symlink spellings - Refuse any hooks.state write into a Codex config.toml (upsert, move, remove, mirrored enabled state, SSH installer) that would turn a loadable file into one Codex cannot load; write nothing and surface the reason. - Read approvals written as dotted keys or inline tables. - Approve and remove Orca's ~/.codex hook under both the spelled and the resolved key when ~/.codex or HOME is a symlink; move user approvals under both keys. - Stale runtime trust cleanup no longer keeps an unexpected key whose conflicting duplicate tables read as no hash.
…ad literal-string trusted_hash
3c8e2eb to
76f82d4
Compare
ELI5
Orca writes small "this hook is approved" records into Codex's settings file (
config.toml). In a few uncommon setups that write could leave the file in a shape Codex refuses to start with. Orca could also miss approvals that already exist, or put them under only one of two names Codex uses for the same folder. This PR makes Orca refuse any write that would break the file, read approvals in every form Codex accepts, and approve under both names.What Changed
Before: if your
~/.codex/config.tomlkeeps hook approvals inline (hooks.state = { … }) or as dotted keys (hooks.state."…".trusted_hash = …), Orca appended a[hooks.state."…"]table anyway. The result is invalid TOML, and Codex would not start until you fixed the file by hand. This could happen on every Codex launch from Orca, and over SSH too.After: every place Orca writes these approvals checks the result first:
If the file loaded before and would not load after, Orca writes nothing. The hook status says why ("…defines hook approvals in a form Orca cannot add to without breaking it").
Reading. Orca now reads approvals written as dotted keys or inline tables. Before, it only saw
[hooks.state."…"]tables, so it treated those approvals as missing.Symlinked folders. Codex names
~/.codex/hooks.jsontwo ways when~/.codexor your home folder is a symlink: as typed, and resolved. Codex's own approval only covered the first. Orca now copies that approval to the second, moves your own hooks' approvals under both, and removes both when you turn hooks off.Cleanup fix. An unexpected approval whose duplicate tables disagree is now cleaned up instead of surviving.
Why
Linked Issue
Fixes #
Visual Proof
N/A: no UI change. The only visible difference is the hook status text in the rare refused case.
Testing
~/.codex, where Codex's approval is copied and the opt-out removes both keys;~/.codexexists;src/main/codex,src/main/codex-accountsandsrc/main/agent-hookssuites pass on macOS.Notes