Repository navigation
Conversation
CI on windows-latest/node18 and windows-latest/node22 was deterministically failing 23 assertions across two files in PR #33's suite -- all one root cause: Windows has no POSIX permission bits, so fchmodSync/chmod are no-ops there, and a freshly created file just keeps whatever raw default mode Windows reports (never the exact 0640/0750/0644 this codebase enforces on POSIX). These assertions were never platform-guarded. Fixed by adding a checkPosixMode(name, actualMode, expectedMode, reason) helper to each file, following the SKIP convention already proven working elsewhere in this suite family (tests/gateway/recovery/run-tests.js, and the two existing win32-guarded functions already in tests/gateway/startup-permissions/run-tests.js): on win32 it records SKIP instead of comparing modes, otherwise it delegates to the existing check(). Guarding was done per-assertion rather than per-function, since several affected functions mix POSIX-mode assertions with other assertions (throws, content-correctness, changed-file reporting) that must keep running on Windows. - tests/state-store/atomic-primitives/run-tests.js: 5 assertions (atomicCreateExclusive/atomicOverwriteFile default-mode and umask checks). - tests/gateway/startup-permissions/run-tests.js: 18 assertions (mostly in tightenStateDirPermissionsRetrofitsEveryRealSensitivePathToTheAccessModel). Verified (Linux, reconstructed dependency tree, both suites run to completion): - Unpatched behavior unchanged: 45/45 and 39/39 passed, 0 skipped, 0 failed -- confirms zero regression on POSIX. - With process.platform simulated as win32: 40/40 and 34/34 non-skipped assertions still pass, 0 failed; the 5 and 18 guarded assertions (plus 2 pre-existing win32-guarded functions in the startup-permissions file) correctly emit SKIP instead of FAIL. Branched off PR #33 (feature/gateway-crash-recovery-idempotency) at its current tip per Paul's decision to fix this in parallel rather than block on PR #33 merging first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNWPgMQ5chbPrTw9Kc6B85
|
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: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe gateway startup and state-store atomic-primitives tests now skip exact POSIX permission-mode checks on Windows. The atomic-primitives test output labels skipped checks and reports the number skipped. ChangesPlatform-aware permission tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The changes appear to preserve the relevant checks while skipping unsupported POSIX mode assertions on Windows. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f035a9929
ℹ️ 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".
…(round 2) Codex review on PR #34 (comment 4100372808) flagged that record() in tests/state-store/atomic-primitives/run-tests.js only special-cased the "PASS" status when printing console output -- every other status, including a deliberate "SKIP" (used by the 5 Windows POSIX-mode-skip guards added in round 1), was printed with a generic "FAIL ${name}+..." prefix. The `failures` counter and SUMMARY line already counted these correctly as skipped, not failed, so the process still exited 0 -- but Windows CI logs showed misleading "FAIL ..." lines for assertions that actually passed by skipping. Fix: record() now emits a dedicated "SKIP" prefix for skip status, leaving PASS/FAIL behavior unchanged. Verified locally: - POSIX run: SUMMARY passed=45 failed=0 skipped=0 (unchanged) - Simulated win32 run: SUMMARY passed=40 failed=0 skipped=5 (unchanged counts; the 5 lines now correctly print SKIP instead of FAIL) Scope: only tests/state-store/atomic-primitives/run-tests.js touched. The identical pre-existing bug in tests/gateway/recovery/run-tests.js's own record() is out of scope for this PR and intentionally left as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNWPgMQ5chbPrTw9Kc6B85
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Delightful! 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". |
This branch has not been deployed
1 participant
What
Fixes the Windows-only CI failures on PR #33 (windows-latest/node18 and windows-latest/node22), by making 23 exact-POSIX-mode-bit assertions skip on win32 instead of failing.
Root cause
Windows has no POSIX permission bits.
fchmodSync/chmodare effectively no-ops there, so a freshly created file just keeps whatever raw default mode Windows reports -- never the exact0640/0750/0644this codebase's access model enforces on POSIX. These 23 assertions were never platform-guarded, unlike the rest of this test-suite family.Confirmed deterministic (not flaky) and byte-identical across both Windows CI jobs before starting this fix.
Fix
Added a
checkPosixMode(name, actualMode, expectedMode, reason)helper to each affected file. Onwin32it recordsSKIP(matching the convention already proven working intests/gateway/recovery/run-tests.jsand the two existing win32-guarded functions already intests/gateway/startup-permissions/run-tests.js); otherwise it delegates to the file's existingcheck().Guarding is per-assertion, not per-function, because several affected functions mix POSIX-mode assertions with other assertions (throws, content-correctness, changed-file reporting) that must keep running on Windows.
tests/state-store/atomic-primitives/run-tests.js-- 5 assertions (atomicCreateExclusive/atomicOverwriteFiledefault-mode and umask checks).tests/gateway/startup-permissions/run-tests.js-- 18 assertions (mostly insidetightenStateDirPermissionsRetrofitsEveryRealSensitivePathToTheAccessModel).Verification
Reconstructed the full dependency chain for both test files (this repo has zero production npm dependencies) and ran them directly with
nodeon Linux:SUMMARY passed=45 failed=0 skipped=0andSUMMARY passed=39 failed=0 skipped=0-- confirms the starting point.45/45and39/39, 0 skipped, 0 failed -- zero regression on Linux/POSIX.process.platformsimulated aswin32:passed=40 failed=0 skipped=5andpassed=14 failed=0 skipped=20(the 18-file's 20 skips = the 18 newly-guarded assertions here + the 2 pre-existing win32-guarded functions already in that file, which collapse several POSIX sub-checks into one SKIP each). 0 FAILs in either file under the win32 simulation.Scope
Branched off PR #33 (
feature/gateway-crash-recovery-idempotency) at its tip (cfbee414f) rather than waiting for it to merge first, per Paul's call to fix this in parallel. This PR targets that branch, notrelease/v0.5.0-candidate-- it should merge into PR #33's branch, not around it.No other files touched. No behavior changes on POSIX (Linux/macOS CI, and Windows production behavior which already never relied on these exact modes).
🤖 Generated with Claude Code
https://claude.ai/code/session_01CNWPgMQ5chbPrTw9Kc6B85
Summary by CodeRabbit