Repository navigation
Conversation
Co-authored-by: Yuichi Uemura <u1@u-kt.com>
b3f22f4 to
88f1499
Compare
📝 WalkthroughWalkthroughThe change adds synchronization of system Codex Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Deleting an imported rule can repeatedly stop Codex launch preparation. Reconcile missing imports against current global rules before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 4 files. (1 skipped: 1 unsupported.)
✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
bf1608b3-5d90-4ef3-9a02-0eff9d2593e6
📒 Files selected for processing (5)
config/tsconfig.cli.jsonsrc/main/codex-accounts/runtime-home-service-per-account-migration.test.tssrc/main/codex/codex-home-paths.tssrc/main/codex/codex-managed-home-rules.test.tssrc/main/codex/codex-managed-home-rules.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| const current = readRegularFile(path) | ||
| if (current !== change.after) { | ||
| if (current !== change.before) { | ||
| fail() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,245p' src/main/codex/codex-managed-home-rules.ts
sed -n '85,210p' src/main/codex/codex-managed-home-rules.test.ts
rg -n 'rule-imports|missing imported|delete.*rules|rmSync|unlinkSync|syncSystemCodexRules' src/main/codexRepository: stablyai/orca
Length of output: 37444
🏁 Script executed:
git diff --no-ext-diff --unified=50 988f87e743ec52aa24ae1f25a308e66d6d8d2e59 a90347730f58a839fc39d10cdc5ab15cb732f9f0 -- src/main/codex/codex-managed-home-rules.ts src/main/codex/codex-managed-home-rules.test.ts src/main/codex/codex-home-paths.ts
printf '\n--- guarded filesystem helpers ---\n'
rg -n -F -- 'function writeFileAtomicallyIfUnchanged' src/main/codex/codex-accounts/fs-utils.ts
sed -n '1,260p' src/main/codex/codex-accounts/fs-utils.ts
printf '\n--- full relevant rule tests ---\n'
sed -n '1,290p' src/main/codex/codex-managed-home-rules.test.ts
printf '\n--- launch binding ---\n'
sed -n '55,105p' src/main/codex/codex-home-paths.ts
rg -n -i 'fail.closed|fail closed|managed.*rule|global.*rule|account.*rule|permission policy' docs src/main/codex -g '*.md' -g '*.ts' --glob '!*.test.ts' || test "$?" -eq 1Repository: stablyai/orca
Length of output: 42157
🏁 Script executed:
printf '%s\n' '--- helper files ---'
rg --files src/main | rg 'codex-accounts/fs-utils|fs-utils'
printf '%s\n' '--- helper definitions and callers ---'
rg -n -F -- 'writeFileAtomicallyIfUnchanged' src/main
rg -n -F -- 'recoverInterruptedGuardedFileOperation' src/main
printf '%s\n' '--- helper source ---'
sed -n '1,280p' src/main/codex-accounts/fs-utils.ts
printf '%s\n' '--- helper tests ---'
rg -n -F -- 'writeFileAtomicallyIfUnchanged' src -g '*.test.ts' -g '*.spec.ts' || test "$?" -eq 1
rg -n -F -- 'recoverInterruptedGuardedFileOperation' src -g '*.test.ts' -g '*.spec.ts' || test "$?" -eq 1Repository: stablyai/orca
Length of output: 13905
🏁 Script executed:
rg -n -F -- 'syncSystemCodexResourcesIntoManagedHome(' src/main src/cliRepository: stablyai/orca
Length of output: 4771
🏁 Script executed:
sed -n '90,130p' src/main/codex-accounts/runtime-home-service-managed-home.ts
sed -n '270,310p' src/main/codex-accounts/runtime-home-service-home-routing.ts
sed -n '55,90p' src/main/codex-accounts/runtime-home-service-launch.tsRepository: stablyai/orca
Length of output: 5084
Reconcile missing imports against the current source rules.
When a managed imported rule is missing, applyChanges() compares it with the settled manifest and stops before reconciling the current source. Launch preparation calls this sync before it returns the managed home, so the same state can block each launch attempt.
Recreate a missing file from the current source snapshot only if that import is still desired. If the source rule was removed, do not write the stale change.after before removing its manifest entry. Keep rejecting present files with unexpected contents. The guarded write with expectedContents: null prevents overwriting a file that appears after the absence check.
Suggested fix
-function applyChanges(rulesPath: string, manifest: RuleManifest): RuleManifest {
+function applyChanges(
+ rulesPath: string,
+ manifest: RuleManifest,
+ desired: Record<string, { sourceName: string; contents: string }>
+): RuleManifest {
...
const current = readRegularFile(path)
+ if (current === null && change.before === change.after) {
+ const wanted = desired[name]
+ if (wanted) {
+ if (!writeFileAtomicallyIfUnchanged(path, null, wanted.contents, { mode: 0o600 })) {
+ fail()
+ }
+ settled.files[name] = {
+ sourceName: wanted.sourceName,
+ before: wanted.contents,
+ after: wanted.contents
+ }
+ }
+ continue
+ }
if (current !== change.after) {
...
- const settled = applyChanges(rulesPath, existing)
+ const settled = applyChanges(rulesPath, existing, desired)
...
const before = settled.files[name]?.after ?? null
- if (readRegularFile(join(rulesPath, name)) !== before) {
+ const present = readRegularFile(join(rulesPath, name))
+ if (present !== null && present !== before) {
fail()
}
pending.files[name] = {
sourceName: desired[name]?.sourceName ?? settled.files[name].sourceName,
- before,
+ before: present,This branch has not been deployed
1 participant
ELI5
Codex launched with an Orca-managed account now reads your global command rules while keeping new approvals in that account. An existing account rules folder no longer hides global forbidden or prompt rules.
What Changed
Orca previously omitted global rules from managed homes. Linking the whole rules folder would make account approvals write back into the global folder, and would still omit global restrictions when an account already had its own folder.
Launch preparation now copies each regular global
.rulesfile to a separate, bounded, non-default filename in the managed rules folder. The account'sdefault.rulesremains writable and untouched. Copies refresh on the next launch, including removal of deleted or renamed global files. Separate files preserve Codex's rule-module behavior and global file order.A small ownership journal records expected contents before guarded atomic writes and removals. Interrupted updates recover from their recorded contents. Conflicting user files, changed imports, unreadable policies, malformed ownership records, or destination symlinks stop launch preparation rather than overwrite account files or quietly omit restrictions.
Why
This keeps global policy effective and preserves account approval scope. A whole-directory link broadens saved approvals; copying over account
default.rulesloses existing approvals. Reusing guarded file publication avoids replacing a competing writer's contents. The host reads its own global configuration; no client rule files or new wire fields are sent.Thanks to Yuichi Uemura for reporting and reproducing the missing rules and proposing the original fix. This replaces #24432 and credits him as co-author.
Linked Issue
Fixes #24431. The issue remains open until this fix merges.
Visual Proof
N/A — launch-time filesystem and permission-policy handling, with no UI changes.
Testing
execpolicy check --rules; it verifies the parser and decisions, not a live agent session. Shipping Codex's loader confirms regular.rulesdiscovery and persistence todefault.rules.Review
Independent reviews challenged recovery after a crash between rename and publication, and the discovery behavior for symlink files. The final code recovers guarded operations before observation and matches Codex's regular-file discovery. Existing launch preparation propagates refusal; it does not fall through to another account.
Agent skill upstream boundary
Notes
Merge risk: low to moderate because this restores permission policy. Filesystem and native policy checks ran on macOS; physical Linux/Windows and paired or SSH execution were not rerun. Shared host preparation applies the host's own rules without changing RPC compatibility. A client update cannot change an older execution host's preparation. WSL's separate home-copy path is unchanged. Folder workspaces and Git worktrees use the same account-home preparation. Existing sessions keep Codex's already-loaded policy; edits take effect on the next launch.
Checklist