[luvs01]

Summary

• Windows install preparation now requests the existing, installation-scoped wrapper cleanup used by ocx stop after all recorded/conflicting/requested service managers have stopped and before stopping the tracked proxy. This addresses the cleanup gap where a surviving Task Scheduler launcher can respawn the old proxy during reinstall, including when its registration is already absent.
• Manager-stop failures and live Desktop supervision still refuse before wrapper cleanup. Tests inject the cleanup dependency to avoid invoking real process termination. The five new regressions live in their own registered test file, and the service contract documents the ordering and limits.
• Wrapper termination remains best-effort: this layer does not confirm that a surviving wrapper exited or that the replacement serves successfully. This change does not establish the cause of the original installation failure.

Verification

Recorded with Bun 1.4.3; these are separate runs, not additive totals:

• Baseline: bun test tests/service/service.test.ts -t 'install preparation' with the production change removed produced 4 pass / 2 assertion failures (the two positive cleanup cases). Restoring the change made the five new regressions pass; their final moved-file run, bun test tests/service/service-install-wrapper-cleanup.test.ts, produced 5 pass / 0 fail.
• Final layout: bun test tests/service/service-install-wrapper-cleanup.test.ts tests/service/service.test.ts tests/test-layout-tooling.test.ts — 238 pass / 3 skip / 0 fail, 1,268 assertions. The three skipped cases concern Windows definition ACL/UTF-16 fixtures.
• Earlier isolation correction, before moving the five tests: bun test tests/service/service.test.ts tests/service/service-runtime-preflight.test.ts tests/service/service-ownership-handover.test.ts tests/service/service-desktop-runtime-preflight.test.ts — 266 tests / 0 fail. This is historical coverage, not a final-layout total.
• bun node_modules/typescript/bin/tsc --noEmit — exit 0. The direct compiler entry bypassed a junction-related bun x tsc shim-resolution failure.
• bun scripts/file-size-ratchet.ts, bun scripts/structure-ssot.ts, and bun scripts/privacy-scan.ts — passed; git diff --check passed.

The full local suite and a live Windows reinstall E2E were not run. Concurrent worktrees share test resources; validation here targets the changed ordering, refusal controls, mock isolation and layout, with broader coverage left to CI. A manual stop observation on the unpatched runtime is not patched E2E evidence. No GUI changes.

Current-head CI at a7647fb1740ec7a7443c4c2dadcd536d58b80c98: Cross-platform CI 39133137509 and Service lifecycle 39133137536 completed successfully. Windows test shards, Windows Task Scheduler and macOS launchd jobs were skipped; skipped jobs are not passing platform validation. Ingwannu's focused review on this head found no additional correctness blocker, and no new actionable feedback is present. CodeRabbit skipped its review while this PR was Draft; its success status is not review approval. Author-owned readiness is complete; any required maintainer sponsorship/security review remains a before-merge responsibility.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
• Current-head CI is complete and existing actionable review feedback is addressed for review readiness.

Summary by CodeRabbit

• Bug Fixes
  • Windows service-install preparation now attempts to stop relevant wrapper processes after service managers stop and before tracked-proxy cleanup. Wrapper cleanup also runs when no manager stop is needed, but is skipped if stopping a manager fails.
  • Wrapper cleanup is limited to Windows; installations under active Desktop supervision remain protected.
• Tests
  • Added coverage for cleanup ordering, failure handling, and platform-specific behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 6c27282f-9b68-430f-a189-ac1c4e05a0f3

📥 Commits

Reviewing files that changed from the base of the PR and between 8640c2f and a7647fb.

📒 Files selected for processing: 9

• scripts/test-layout/layout.json
• src/service/orchestration.ts
• structure/ops/service-and-sidecars.md
• tests/fixtures/test-layout-expected.json
• tests/service/service-desktop-runtime-preflight.test.ts
• tests/service/service-install-wrapper-cleanup.test.ts
• tests/service/service-ownership-handover.test.ts
• tests/service/service-runtime-preflight.test.ts
• tests/service/service.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Service-install preparation now performs Windows service-wrapper cleanup after successful manager-stop checks and before stopping the tracked proxy. Tests cover cleanup ordering, failure and refusal paths, and non-Windows behavior. The documentation and test-layout mappings were updated.

Changes: Service-install cleanup

Layer / File(s)	Summary
Preparation cleanup src/service/orchestration.ts	ServiceInstallPreparationDeps now supports an optional killServiceWrappers callback. On Windows, preparation invokes the callback or the default wrapper-cleanup function after manager-stop checks and before tracked-proxy cleanup. A manager-stop failure prevents wrapper cleanup.
Cleanup validation and documentation tests/service/*, tests/fixtures/test-layout-expected.json, scripts/test-layout/layout.json, structure/ops/service-and-sidecars.md	Tests check cleanup ordering, cases with no manager stop, manager-stop failures, non-Windows behavior, and Desktop supervision refusal. The documentation describes the cleanup sequence and its limits. The new test is assigned to the service shard.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant prepareServiceInstall
  participant ServiceManagers
  participant WindowsWrapperProcesses
  participant TrackedProxy
  prepareServiceInstall->>ServiceManagers: Stop applicable managers
  prepareServiceInstall->>WindowsWrapperProcesses: Terminate wrappers on Windows
  prepareServiceInstall->>TrackedProxy: Stop tracked proxy

Loading

Merge Risk: ⚪ Minimal · up to a7647

The preparation sequence can continue if wrapper termination fails. No issue identified here requires a change before merge.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (3 skipped: 3…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: stopping scheduler wrappers on Windows before service reinstallation. It matches the implementation and test coverage.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (3 skipped: 3 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ Deterministic PR hygiene checks passed.

[Ingwannu]

Correctness review at a7647fb: the new cleanup is placed after every manager stop and before tracked-proxy shutdown, and preserves the early Desktop-supervision and manager-refusal exits. Reusing the existing install-scoped wrapper helper keeps the change narrow. The structure document accurately describes its best-effort limitation, and the new test is registered in both maps.

Independent Linux/Bun 1.4.3 verification in fresh homes and a capped cgroup: 55 tests passed across service-install-wrapper-cleanup, service-runtime-preflight, service-ownership-handover and service-desktop-runtime-preflight (including all five new ordering/refusal cases). The current-head Cross-platform CI run 39133137509 and Service lifecycle run 39133137536 are successful. The Windows schtasks job was skipped; neither the injected platform tests nor that hosted result proves a real Windows reinstall or wrapper exit.

No additional correctness blocker found in this focused change. Please complete the author-owned current-head readiness checklist when the draft is ready for review. I have kept the draft/readiness state intact; no merge or service operation was performed.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	⚠️ Failed 2026-10-18T14:09:42.225853Z	a7647fb	Draft marked ready

ℹ️ 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" or "@codex security review".

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]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[Ingwannu]

Correctness approval at unchanged head a7647fb, now marked ready (current dev base 8640c2f). The install-scoped wrapper cleanup remains after successful manager stops and before tracked-listener cleanup, with Desktop supervision and manager-refusal exits ahead of it. Both test maps and the structure limitation are documented.

The 55 focused Linux/Bun 1.4.3 tests recorded in my previous review 5563299114 were executed at this exact unchanged head, so they were not rerun just for another receipt. Current-head functional CI, gates/Typecheck and Service lifecycle are successful. The Windows schtasks lane was skipped, so actual Windows reinstall/wrapper exit remains unverified.

The later Codex comment reports review-account usage exhaustion and supplies no source finding; that account-limit message is not a functional CI failure or an instruction to spend credits. This manual correctness review does not waive a real unresolved finding. No account-consent action, workflow dispatch, new scan or merge performed.

[lidge-jun]

Carried into #7397 (squash dbb908f) on dev with a Co-authored-by trailer, as part of the B5 OAuth/accounts/service hardening bundle. It ships in the next release (2.90.0). Thanks!