fix(service): accept standalone Windows wrappers in ownership probe (carry #6238) - #6258
Conversation
Require one direct Bun launch in a generated wrapper. Cover source and standalone wrappers that contain both launch forms.
Carries #6238 (Windows standalone wrapper ownership probe) with the RT6 security-review hardening commits. Co-authored-by: lcBreathe <165003424+lcBreathe@users.noreply.github.com>
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. |
|
✅ Deterministic PR hygiene checks passed. |
|
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: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe Windows ownership probe now validates generated source and standalone wrappers, registered Task Scheduler actions, and scheduler state evidence. The tests cover accepted and rejected wrapper and task forms. The documentation describes these checks and notes that executable identity is established only by lexical path. ChangesWindows service ownership
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Possibly related PRs
Merge Risk: ⚪ Minimal · up to The change makes the Windows ownership probe stricter and accepts the standalone wrapper form. No unresolved merge-blocking issue was found in the supplied evidence. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new checks reject malformed wrappers, mismatched task actions, and contradictory install state. A remaining limitation is that the recorded executable path does not prove which file occupies that path. Its security impact depends on who can modify the installation location. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 38.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
![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: 3488c24389
ℹ️ 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".
| || valid.length === 0 || evidence.some(e => e.kind === "invalid" || e.kind === "unreadable") | ||
| || valid.some(e => e.state.version !== 2 || e.state.backend !== "scheduler" || e.state.cliPath !== null | ||
| || !e.state.bunPath || normalizeWindowsPath(e.state.bunPath) !== normalizeWindowsPath(executable))) { |
There was a problem hiding this comment.
When a custom OPENCODEX_HOME has a lower-revision compatibility mirror whose refresh previously failed, src/service-manager-probe.ts checks every valid record and rejects the standalone wrapper if that stale mirror retains the prior bunPath, cliPath, or backend. This is a supported state: swapServiceInstallState commits the final authority first and explicitly allows mirror publication to fail, while selectAuthoritativeServiceState treats lower-revision disagreement as repairable. The resulting unknown ownership blocks unattended operations even though the authoritative record and generated wrapper agree; select the authoritative state first and bind the executable to that record, while preserving failure for same/newer unordered conflicts.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 74 / 80Windows 단독 설치(standalone) 서비스 래퍼가 소유권 검사에서 거절되던 버그를 고치는 PR이에요. 예전 검사는 라인 - #6238 은 아직 열린 draft예요. 이 PR이 그 내용을 보안 보강까지 얹어서 옮긴 본선이에요. 같이 두면 같은 수정이 두 갈래로 남아요. 메인테이너의 판단이 필요한 지점 #6238 draft를 이 PR 기준으로 닫을지 정해 주세요. 소스 래퍼에도 단독과 같은 제어 흐름·상태 묶기를 앞으로 올릴지는 제품 판단이에요. 지금 문서의 정션 한계를 이 릴리스에서 더 막을지, 그대로 둘지도 정해 주세요. 생성기 제어 흐름을 지금 HEAD 기준으로 딱 맞춰서, 예전에 깔린 2.71.0 래퍼가 생성기 이후 문구와 조금만 달라도 계속 unknown이 됩니다. 그 경우 너의 추천 이 PR을 본선으로 두고 #6238은 닫으세요. 단독 쪽 엄격한 검사는 풀지 마세요. 정션 한계는 문서대로 남겨도 됩니다. CI 핵심 검사가 초록이면 합쳐도 됩니다. 미리보기 배포 이야기는 이 PR과 무관하니 건너뛰세요. 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Carries #6238 by @lcBreathe onto current
devso it can land in 2.73.0. Fixes #6237: the Windows standalone 2.71.0 service wrapper ("%OCX_BUN%" start) was rejected by the ownership probe, soocx syncrefused an OpenCodex-owned service.The original change accepts the standalone launch form. Three rounds of independent security review then hardened the new acceptance path so that only a wrapper matching the standalone generator can claim ownership:
OCX_BUNassignment and markers, noOCX_CLIin any quoting form, and the generator's control-flow lines in order (insertedgoto,exit,callor labels are rejected);wscript.execommand with the exact launcher arguments.Source-install wrappers keep their existing behavior. Documented residual (
structure/ops/service-and-sidecars.md): install state records a path rather than a file identity, so a junction retargeted by someone with write access to the install location cannot be told apart.The contributor's commits are preserved unchanged; the maintainer commits sit on top.
Co-authored-by: lcBreathe 165003424+lcBreathe@users.noreply.github.com
Verification
bun test tests/codex-integration/codex-service-manager-probe.test.ts tests/codex-integration/codex-service-manager-probe-hardening.test.ts tests/service/standalone-service.test.ts: 108 pass, 0 fail.bun x tsc --noEmit,bun run structure:check,bun run privacy:scan: pass.bun run testis left to this PR's CI because several release lanes share one machine.Checklist
Summary by CodeRabbit