test(service): block live scheduler mutation under the test guard - #2992
Conversation
|
✅ Deterministic PR hygiene checks passed. |
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change adds guards for armed test processes before live Task Scheduler mutations, native WinSW service removal, and Windows elevation launches. Tests verify rejection before external interaction and rollback before native-service removal. ChangesWindows test-process guards
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The test guard now prevents most live scheduler and elevation mutations, but some native Windows service lifecycle paths can still change machine-global service state when an armed test reaches them. The PR is mergeable with explicit owner awareness and follow-up to enforce the guard at the common service-management boundary. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title clearly identifies the main change: blocking live Windows Task Scheduler mutations when the test guard is active. It is concise and specific, although it does not mention the related Windows elevation safeguard.
✨ Finishing Touches🧪 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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97f499c1bc
ℹ️ 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".
리뷰 · 우선순위 69 / 80설명 지금 저장소 테스트 러너는 지금 HEAD에는 이미 #2975 가 회귀 테스트는 라인 903-917 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
97f499c to
c1b684c
Compare
c1b684c to
1101aba
Compare
1101aba to
1b760b5
Compare
Ingwannu
left a comment
There was a problem hiding this comment.
Approved exact head 1b760b5baa3f33470c085dcc111d0793ba4553f7 for the scoped Windows test-boundary change.
The latest head closes the original scheduler gap and the elevation bypass: armed tests reject machine-global schtasks mutations before the recorder/live launcher, and reject the default live elevation spawn while explicit injected launchers remain testable. Isolated focused verification passed 228/228 plus typecheck; Service lifecycle and React Doctor are green.
The remaining Cross-platform CI failures are both the repository-wide post-release base condition: package.json still says 2.36.0 after tag v2.36.0 moved to the release commit, so release-version-line.test.ts fails on every non-tagged PR head. This approval does not waive required CI. Do not merge until the dedicated dev version-line repair lands, this branch is refreshed if necessary, and exact-head required CI is green.
The repository test runner rewrites HOME and OPENCODEX_HOME, but Windows Task Scheduler is machine-global. A partially-faked repair test fell through the default scheduler runner, replaced the user's real opencodex-proxy task with a temporary test-home launcher, passed, and then deleted that launcher during cleanup. When the explicit test-home guard is armed, allow only read-only /query calls to the live scheduler runner. Every create, delete, run, end, or change operation must be injected. Production is inert because only the repository test preload arms this guard. The regression drives a valid fresh registration through the default create path. Before the fix its fake recorder receives /create /tn opencodex-proxy ... /f; after the fix the call is rejected before even the recorder.
1b760b5 to
83b1316
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b1316ca3
ℹ️ 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".
Summary
The repository test runner isolates
HOME,USERPROFILE, andOPENCODEX_HOME, but Windows Task Scheduler and UAC-launched processes are machine-global.A partially-faked service repair test crossed that boundary and executed the default
schtasks /create /tn opencodex-proxy ... /fpath. It replaced the live task with a launcher inside the temporary test home, the test still passed, and cleanup deleted the launcher. The already-running proxy hid the damaged registration until the next reconnect or restart.When the explicit repository test-home guard is armed, this change:
/queryoperations through the direct Task Scheduler runner; andThis covers direct and elevated registration, create/run/rollback, delete, and other machine-global mutation paths. Production behavior is unchanged because only the repository test preload sets
OCX_TEST_HOME_GUARD=1.Verification
bun test tests/service.test.ts tests/windows-scheduler-install-verification.test.ts tests/test-home-guard.test.ts— 172 pass, 0 fail; 3 POSIX-only permission tests skipped on Windows.bun test tests/windows-elevation-spawn.test.ts— 53 pass, 0 fail; the live-boundary probe contains only a fixed non-RunAsexit 0command if its guard regresses.bun run typecheck,bun run privacy:scan, andgit diff --check— clean.56bd40d242is based on currentdev870a2adb6e; three current-head scheduler, native-SCM, and explicit-injection guard regressions pass 3/3 on Bun 1.4.0. The previously completed full focused suites remain 197/0, with typecheck and privacy scan clean; the liveopencodex-proxyregistration was unchanged afterward and/healthzreturned 200.Checklist
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