fix: gate root skip-permissions bypass and ship the auto-review model override (#1688 #1225) - #2631
Conversation
…iew_model override (#1225)
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughChangesClaude root launch handling
Codex auto-review model configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant cmdClaude
participant Claude
User->>cmdClaude: Invoke root Claude with skip-permissions
cmdClaude->>cmdClaude: Check UID and flag
cmdClaude->>Claude: Launch with warning and IS_SANDBOX environment
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9910c4ef9b
ℹ️ 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".
| if (isRoutedCatalogEntry(entry) | ||
| || (globalStamp && typeof current === "string" && configuredValues.has(current))) { | ||
| entry.auto_review_model_override = null; |
There was a problem hiding this comment.
Restore native overrides when removing the global override
When a valid auto_review_model is applied, every native row's original auto_review_model_override is replaced by the configured value; if the setting is then removed or becomes unresolved, sourceModels contains only that global value, so globalStamp is true and this branch changes the native rows to null rather than recovering their original upstream overrides. Consequently, an apply-then-clear sequence permanently changes native auto-review selection across later syncs, contrary to the documented preservation behavior in providers.md; retain the original value from a pristine/upstream source or persist ownership metadata for OpenCodex stamps.
AGENTS.md reference: AGENTS.md:L279-L280
Useful? React with 👍 / 👎.
Summary
Two community issues, verified and falsified independently.
#1688 —
ocx claude --dangerously-skip-permissionsaborts as rootClaude Code 2.1.205+ exits as root unless it believes it is sandboxed. OpenCodex launches the session, so it can mark the environment — but doing that unconditionally would tell Claude Code a safety guard is satisfied when it is not.
So the bypass is gated on both conditions: the user explicitly passed
--dangerously-skip-permissions(they have already accepted that risk) anduid == 0. An ordinary launch is untouched, and an explicitIS_SANDBOX=0from the user still wins —setDefaultnever overwrites a user value, and that case is tested.The launch says what happened, both ways:
A silent safety-guard bypass would have been the wrong shape here even if it fixed the symptom. No bubblewrap sandbox was added — that is a Linux-only isolation feature needing its own threat model, not something to bundle into a launcher flag fix.
#1225 — configurable auto-review model
Upstream Codex picks the reviewer model from
auto_review_model_overrideon the catalog entry of the running model, so OpenCodex's lever is the catalog it writes. This carries #2363's approach: a rootauto_review_modelsetting, trimmed and validated against the final catalog, stamped across entries by both writers (retained sync and convergence), with a diagnostic when the target does not resolve.One thing corrected before merging: #2363's original head accidentally broke permit-bound Codex-home cache invalidation — an unrelated regression that would have shipped with the feature. Fixed in
src/codex/catalog/sync.ts:1960and kept green by the composed-acceptance and convergence suites, which is how it was caught.Verification
Falsified separately: removing the
IS_SANDBOXassignment reddens the root-launch case (Expected "1", received undefined); removing the override stamping reddens the catalog assertion (Unable to find property auto_review_model_override).Checklist
devdocs-site/updated for the new settingCloses #1688. Closes #1225.
Summary by CodeRabbit
New Features
Documentation
Tests