fix(codex): retry native-main ACL timeout once - #1130
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWindows ACL hardening now supports one bounded timeout retry. Native-main ownership tracks retry state, defers one ChangesACL timeout recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant NativeMainOwner
participant LockFileHardener
participant WindowsACL
NativeMainOwner->>LockFileHardener: request lock-file hardening
LockFileHardener->>WindowsACL: apply required ACL with retry budget
WindowsACL-->>NativeMainOwner: success or coded ETIMEDOUT
NativeMainOwner->>NativeMainOwner: schedule one retry or mark unavailable
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
004a6c1 to
94478e8
Compare
|
Thank you — this is landing as #1135 in the stacked campaign, cherry-picked with your authorship intact. The coded-error taxonomy is what makes this adoptable: it separates "the ACL call timed out" from "the ACL was refused" instead of widening the catch, so the single retry applies only to the recoverable class and a denial still denies. On a path that guards a physical credential that distinction is the whole safety argument, and your permanent-error / timeout / retry-exhaustion cases pin it. Verified on the stack head: One honest limitation I noted in the PR rather than glossing over: my verification ran on macOS, so the Windows ACL boundary is exercised through injection, not natively. The Windows CI leg is what would prove the real behavior. Leaving this PR open for you. |
Summary
ETIMEDOUTfrom permanent permission failures or untrusted message text.EACLRETRYEXHAUSTEDinstead of restalling indefinitely.Verification
devrefresh, both full affected files passed on both runtimes: 166 passed, 0 failed.bun run typecheck— passed.bun run privacy:scan— passed.git diff origin/dev...HEAD --check— passed.bunPATH), so this PR does not claim a green full local suite; public CI remains required.Checklist
No reusable credential, ACL bypass, or weaker default is introduced. Maintainer review is still required before merge.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests