Repository navigation
fix(codex): recognize quoted projects keys in config.toml trust headers - #22605
leilei3167 wants to merge 3 commits into
Conversation
Codex writes ["projects"."/path"] tables; the matcher only accepted bare projects., so upsert appended a duplicate and broke Codex startup.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe project header parser now recognizes bare, double-quoted, and single-quoted Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change recognizes quoted Codex project and trust keys, preventing duplicate TOML entries for the covered forms. No actionable merge risk remains in the reviewed change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The header matcher change itself is correct, but it may not fully resolve #22592. The reported Codex block quotes the trust key too ("trust_level" = "trusted"), and the trust-line matchers still require a bare trust_level. If that form is real, Orca now finds the header and then inserts a second trust_level, trading a duplicate table for a duplicate key. Details and the open question are inline.
Reviewed changes
- Widened project-header matcher —
parseProjectTomlHeaderPathnow acceptsprojects,"projects", and'projects'before the path segment, reusing the existing TOML string decoder for the path. - Regression tests — basic-quoted key, literal-quoted key, and basic-quoted key whose path needs
\"unescaping; each asserts the existing block is updated in place rather than duplicated. - Reach of the parser — the same matcher feeds
isRuntimeProjectTomlSection/getTomlSectionHeaderKeyinconfig-toml-runtime-owned-sections.ts, so quoted project headers are now recognized in the mirror path as well.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Codex can write "trust_level" under a quoted projects header (stablyai#22592). Widen the trust key matcher (and getProjectTrustLevel) so upsert replaces the existing line instead of inserting a duplicate bare key.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior pullfrog review (be61fa8): compiling the quoted-key matchers for the trust value and their regression coverage.
- Quoted
trust_levelin the upsert matcher —trustLevelPatternnow acceptstrust_level,"trust_level", and'trust_level', so a block found via the widened header matcher has its existing key replaced instead of a second bare line inserted. - Quoted
trust_levelin mirror reads —getProjectTrustLevelwas widened identically, so dedupe and revocation-preference read a quoted level rather thannull. - Regression coverage — five new cases: quoted project key (basic + literal), quoted key whose path needs
\"unescaping, and quotedtrust_level(basic, matching the #22592 bytes, + literal). Each asserts a single trust key and no appended table/key.
Both production matchers that read trust_level are covered; the new files pass locally (config-toml-trust-project.test.ts 31/31, codex-config-mirror.test.ts 34/34), and the quoted-trust_level case genuinely fails against the header-only change.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![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: 39916d75-6810-4364-a5b9-2f7ddbcdeb56
📒 Files selected for processing (3)
src/main/codex/config-toml-project-trust.tssrc/main/codex/config-toml-runtime-owned-sections.tssrc/main/codex/config-toml-trust-project.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| // Why: same quoted-key forms as upsert (#22592); otherwise mirror/dedupe reads null. | ||
| const match = | ||
| /^[ \t]*trust_level[ \t]*=[ \t]*(?:"(trusted|untrusted)"|'(trusted|untrusted)')[ \t\r]*(?:#.*)?$/m.exec( | ||
| /^[ \t]*(?:trust_level|"trust_level"|'trust_level')[ \t]*=[ \t]*(?:"(trusted|untrusted)"|'(trusted|untrusted)')[ \t\r]*(?:#.*)?$/m.exec( |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
git diff b4d732685cc9722ce9519c5031da87a4f18a4dc4 be61fa8beaa110b3720fe1e3898463be95d92abd -- src/main/codex/config-toml-runtime-owned-sections.ts src/main/codex/config-toml-project-trust.ts src/main/codex/config-toml-trust-project.test.ts
sed -n '110,165p' src/main/codex/config-toml-runtime-owned-sections.ts
rg -n 'getProjectTrustLevel|trust_level.*trusted|trusted.*trust_level' src/main/codex --glob '*test*' --glob '*.ts'Repository: stablyai/orca
Length of output: 20790
🏁 Script executed:
cat -n src/main/codex/config-toml-runtime-owned-sections.ts | sed -n '1,155p'
cat -n src/main/codex/codex-config-mirror.ts | sed -n '225,285p'
cat -n src/main/codex/codex-config-mirror.test.ts | sed -n '290,455p'
cat -n src/main/codex/codex-config-mirror.test.ts | sed -n '590,675p'
cat -n src/main/codex/config-toml-trust-project.test.ts | sed -n '175,225p'
rg -n 'extractOrdinaryCodexSettings|deduplicateProjectTomlSections|mirror.*Codex|sync.*Codex|runtimeConfig' src/main/codex/codex-config-mirror.ts src/main/codex/codex-config-mirror.test.ts src/main/codex/config-toml-runtime-owned-sections.tsRepository: stablyai/orca
Length of output: 41698
Cover quoted keys in the mirror revocation test.
The quoted-key cases in config-toml-trust-project.test.ts exercise the upsert writer, not getProjectTrustLevel through the mirror. The mirror revocation test uses a bare key. If the reader loses its quoted-key alternatives, the system revocation can be dropped when a matching runtime project exists, leaving the runtime’s trusted entry in place. Parameterize the mirror test over both quoted key forms.
Suggested fix
- it('applies a CRLF system revocation to an LF runtime project', () => {
- mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true })
- writeFileSync(
- getRuntimeConfigPath(),
- ["[projects.'c:\\repo']", 'trust_level = "trusted"', ''].join('\n'),
- 'utf-8'
- )
- writeFileSync(
- getSystemConfigPath(),
- ['[projects."C:/repo"]', 'trust_level = "untrusted"', ''].join('\r\n'),
- 'utf-8'
- )
-
- syncSystemConfigIntoManagedCodexHome()
-
- const runtimeConfig = readFileSync(getRuntimeConfigPath(), 'utf-8')
- expect(runtimeConfig.match(/\[projects\./g)).toHaveLength(1)
- expect(runtimeConfig).toContain('trust_level = "untrusted"')
- expect(runtimeConfig).not.toContain('trust_level = "trusted"')
- })
+ it.each(['"trust_level"', "'trust_level'"])(
+ 'applies a CRLF system revocation with key %s to an LF runtime project',
+ (trustKey) => {
+ mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true })
+ writeFileSync(
+ getRuntimeConfigPath(),
+ ["[projects.'c:\\repo']", 'trust_level = "trusted"', ''].join('\n'),
+ 'utf-8'
+ )
+ writeFileSync(
+ getSystemConfigPath(),
+ ['[projects."C:/repo"]', `${trustKey} = "untrusted"`, ''].join('\r\n'),
+ 'utf-8'
+ )
+
+ syncSystemConfigIntoManagedCodexHome()
+
+ const runtimeConfig = readFileSync(getRuntimeConfigPath(), 'utf-8')
+ expect(runtimeConfig.match(/\[projects\./g)).toHaveLength(1)
+ expect(runtimeConfig).toContain('trust_level = "untrusted"')
+ expect(runtimeConfig).not.toContain('trust_level = "trusted"')
+ }
+ )🧰 Tools
🪛 OpenGrep (1.30.0)
[ERROR] 142-144: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
|
Thanks for looking into this @leilei3167 Could you add regression coverage in codex-config-mirror.test.ts too? The mirror uses the same parser, so this fix covers that path, but the new tests only exercise the trust updater. A system config with Looping in @nwparker on recovery for users who already hit this. The updated mirror can deduplicate Orca’s managed config, but existing duplicates in |
…2592) Add coverage in codex-config-mirror.test.ts for a system config with ["projects"."…"] and a runtime config with [projects."…"] for the same path, asserting mirror leaves exactly one project table.
|
Added |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (be61fa8): one regression test added to the Codex config mirror suite. No production code changed in this commit.
- Quoted-key mirror dedup test —
codex-config-mirror.test.tsnow mirrors a basic-quoted system project header (["projects"."…"]) against a bare runtime header and asserts exactly one project table survives. Verified it fails against the pre-fix header matcher (expected 1, received 2), so it exercises the bug rather than passing vacuously.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Closing as superseded by #23958, which is merged and present on I checked this PR's scope and discussion against the merged implementation. The replacement explicitly supersedes this PR and covers its requested behavior. Thanks for the contribution; the branch is preserved. |
ELI5
When Orca marks a worktree as trusted for Codex, it updates
~/.codex/config.toml. Codex sometimes writes the project table with quotes around the wordprojects(["projects"."/path"]). Orca only looked for the unquoted form, thought the project was missing, and appended a second table. TOML forbids that, so every latercodexstart failed with a duplicate-key error. This change teaches the header matcher to recognize both spellings so Orca updates the existing block instead of appending.What Changed
parseProjectTomlHeaderPathnow accepts bareprojects, basic-quoted"projects", and literal-quoted'projects'table keys before the path segment.Why
The locator returning “not found” was indistinguishable from “matcher too narrow,” so a formatting difference that Codex already emits turned into a broken config. Accepting the quoted key forms (same approach as recognizing both quote styles on the path) is the smallest fix that stops the append. Alternatives considered: rewriting every quoted header to the bare form on read (more invasive, rewrites user files unnecessarily), or only detecting duplicates after write without fixing the miss (still leaves trust updates broken until a second pass).
Linked Issue
Fixes #22592
Visual Proof
N/A — this is TOML header matching in the Codex trust upsert path; there is no UI or interaction change.
Testing
Regression:
pnpm exec vitest run --config config/vitest.config.ts src/main/codex/config-toml-trust-project.test.ts["projects"."…"]/['projects'.'…']blocks keptuntrustedand a second[projects."…"]block was appended).AI Disclosure
Review
Self-review notes:
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
No security, cross-platform, Remote SSH, mobile, backwards-compat, or performance concerns beyond the notes above. Bare
[projects."…"]headers continue to match.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)