fix(service): complete the repair migration — docs, startup health, doctor, and update advice - #991
Conversation
…hd hint Follow-up to #980, found by an adversarial audit of the merged dev state. `ocx service repair` ships and is what the update path now runs, but the CLI reference never documented it: the subcommand heading, the table and the example block all listed install/start/stop/status/uninstall only, across all five locales. Worse, the English status example literally read `Repair: ocx service install`. That advice is wrong in a way that costs the user something. `repair` refreshes the installed backend in place; plain `install` re-registers, which needs elevation on Windows and can switch a WinSW install to Task Scheduler (src/service.ts:519, :1760). A user hand-recovering a service after an update would hit a UAC prompt and possibly a backend switch, both avoidable. src/service.ts:2478 had the same stale text in the launchd older-plist hint — the sibling at :1676 was updated in #980 and this one was missed. Both are installed-service recovery paths, so both say repair now. First-install, absent-service and backend-switch messages still say install, which is correct: repair refuses a service that is not installed.
github-actions
Bot
added
the
documentation
📝 WalkthroughWalkthroughThe PR adds ChangesService repair integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StartupHealth
participant StartupRecoverySection
participant User
StartupHealth->>StartupHealth: select repair or install from service state
StartupHealth-->>StartupRecoverySection: provide recovery command
User->>StartupRecoverySection: view or copy command
StartupRecoverySection-->>User: display or copy selected command
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: ba68e9037b
ℹ️ 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".
| const staleLine = stalePlist && stalePlist.loaded && !stalePlist.matchesPlist | ||
| ? " launchd is running an OLDER plist than the one on disk.\n" | ||
| + ` Fix: launchctl bootout gui/$(id -u)/${LABEL} && ocx service install\n` | ||
| + ` Fix: launchctl bootout gui/$(id -u)/${LABEL} && ocx service repair\n` |
There was a problem hiding this comment.
This changes user-facing recovery behavior, but the existing adds the bootout hint when launchd runs an older plist test in tests/service.test.ts checks only for OLDER plist and bootout, so it would still pass if this command regressed to ocx service install. Add a focused assertion for ocx service repair—ideally the complete bootout … && ocx service repair sequence—to preserve the corrected recovery path.
AGENTS.md reference: src/AGENTS.md:L22-L25
Useful? React with 👍 / 👎.
| | none | Create/update and start the service. | | ||
| | `install` | Create and start the service. | | ||
| | `install` | Create and start the service. Registers it, which on Windows needs elevation. | | ||
| | `repair` | Refresh an installed service in place and restart it, without re-registering it. | |
There was a problem hiding this comment.
The claim that repair does not re-register the service is only true for the Windows Task Scheduler path: repairService() delegates macOS repairs to installLaunchd(), which explicitly unloads and reloads the plist, and delegates Linux repairs to the normal install/reload path. As written, this newly added cross-platform table row misdescribes macOS behavior; qualify the no-registration guarantee as Windows-specific or describe the portable guarantee that the installed backend is preserved.
AGENTS.md reference: docs-site/AGENTS.md:L5-L10
Useful? React with 👍 / 👎.
| ## Background service | ||
|
|
||
| ### `ocx service [install|start|stop|status|uninstall|remove]` | ||
| ### `ocx service [install|repair|start|stop|status|uninstall|remove]` |
There was a problem hiding this comment.
The reference now advertises repair, but src/cli/help.ts still renders ocx help service and ocx service --help as ocx service [install|start|stop|status|uninstall|remove]. This leaves the shipped CLI help contradicting the newly synchronized public reference and continues to hide the recovery command from users who consult local help; update the service help entry and its CLI-help coverage alongside this heading.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
…tration Second instance of the same defect the docs commit fixed, found while checking whether that fix was complete. startupHealthSummary() renders 'background service files are stale' and 'installed service is disabled, stopped, or unhealthy' — both cases where a service is already REGISTERED — and then told the user to run 'ocx service install'. The dashboard recovery panel hardcoded the same command in its copy button. install re-registers: it needs elevation on Windows (schtasks /create) and its transactional backend switch can replace a WinSW install with Task Scheduler. So the advice handed a UAC prompt and a possible backend change to someone whose only problem was stale assets. repair rewrites those assets and restarts the existing registration in place. recommendedCommand now picks repair when serviceInstalled and not conflicting; a genuinely absent service still gets install, and a conflict still gets install because repairService() refuses a two-manager conflict outright and the user must uninstall first. The GUI derives its command the same way instead of hardcoding. Red-green: restoring the unconditional installService fails the stale/disabled/ stopped guard. The DTO contract test was updated for the new commands entry, and eslint caught the constant landing in the tray section rather than the recovery section it renders in. 11 pass / 0 fail autostart-health, 583 pass / 0 fail gui.
…troubleshooting docs Third round of the same audit. The reviewer re-checked my first fix and found three more surfaces still advising re-registration for an installed service. update/job.ts: the refresh it runs is 'service repair' (serviceReinstallArgs), but the failure message told Windows users to run 'ocx service install' as administrator. That is the exact post-update path #980 changed — advising install there sends the user to a UAC prompt and a possible WinSW-to-scheduler switch to fix a service that is already registered. It now names the command that actually failed, on every platform, so its output explains why. doctor.ts proxyDownRestartHint(): took only serviceViable, which conflates 'no service at all' with 'registered but stale or stopped'. Only the first wants install. It now takes serviceInstalled/serviceConflict and points an installed service at repair; a conflict still gets install because repairService() refuses a two-manager conflict and the user must uninstall first. docs-site troubleshooting/windows-memory.md in all five locales: the paragraph explicitly discusses 'an already-installed service' and then said to re-run 'ocx service install' to re-bake OPENCODEX_BUN_PATH. repairService() rewrites exactly those scheduler/WinSW assets in place, so repair is both correct and cheaper. The reviewer also flagged autostart-health, which bee1cc7 already fixed — it audited the earlier commit. Its note that the existing stale-service test was vacuous (asserting status but never the resulting command) was accurate, and the guard added in that commit closes it. 196 pass / 0 fail across doctor, autostart-health, update-job, service, and update-stop-first.
lidge-jun
changed the title
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/pages/startup-sections.tsx (1)
228-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the Repair action for every non-viable installed service.
Lines 232-234 select
repairServicefor an installed, non-conflicting disabled or stopped service. However, Line 94 setsserviceNeedsRepaironly whenserviceStaleis true. The details section then shows neither Install nor Repair for disabled or stopped services.Align the direct action with the recovery command.
Proposed fix
- const serviceNeedsRepair = data.serviceSupported && data.serviceInstalled && data.serviceStale && !data.serviceConflict; + const serviceNeedsRepair = data.serviceSupported + && data.serviceInstalled + && !data.serviceConflict + && (data.serviceStale || !data.serviceViable);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/startup-sections.tsx` around lines 228 - 251, The condition selecting repairService in the serviceCommand logic (checking data.serviceInstalled && !data.serviceConflict) does not match the condition for setting serviceNeedsRepair, which currently only checks serviceStale. Update the condition that sets serviceNeedsRepair to align with the serviceCommand selection logic so that disabled or stopped installed services without conflicts are eligible for the Repair action in the details section, matching what the recovery command offers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/autostart-health.ts`:
- Around line 55-59: Add repairService: string to the source
StartupHealth.commands interface alongside installService, matching the existing
COMMANDS entry and returned field so typed consumers can access it.
---
Outside diff comments:
In `@gui/src/pages/startup-sections.tsx`:
- Around line 228-251: The condition selecting repairService in the
serviceCommand logic (checking data.serviceInstalled && !data.serviceConflict)
does not match the condition for setting serviceNeedsRepair, which currently
only checks serviceStale. Update the condition that sets serviceNeedsRepair to
align with the serviceCommand selection logic so that disabled or stopped
installed services without conflicts are eligible for the Repair action in the
details section, matching what the recovery command offers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4e1ad00e-960a-4894-b536-4e8a5aea937a
📒 Files selected for processing (4)
gui/src/pages/startup-sections.tsxgui/src/pages/startup-shared.tssrc/codex/autostart-health.tstests/autostart-health.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/update/job.ts`:
- Around line 826-836: Add focused regression coverage for restartAfterUpdate
when the service refresh returns a non-zero status: verify the installed
non-conflicting, conflicting, and direct-start fallback paths all produce the
expected failed-refresh guidance, including the manual “ocx service repair” and
status commands. Keep the existing proxyDownRestartHint tests unchanged and use
the existing service fixtures/helpers.
- Around line 826-836: Update the installed-service refresh failure path around
restartAfterUpdate so it distinguishes backend conflicts before choosing
recovery guidance. Propagate the conflict state or repair diagnostic from the
service refresh flow, directing conflicting installations to run `ocx service
uninstall` followed by `ocx service install`, while retaining `ocx service
repair` guidance for non-conflicting installed services.
In `@tests/doctor.test.ts`:
- Around line 541-546: Strengthen the assertions in the proxyDownRestartHint
test for both the absent-service and serviceConflict cases so the returned
guidance contains “ocx service install” but does not contain the repair command.
Update the assertions around proxyDownRestartHint and preserve the existing
positive install checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eee85ae1-a889-438f-9c7a-aa080f07a246
📒 Files selected for processing (8)
docs-site/src/content/docs/ja/troubleshooting/windows-memory.mddocs-site/src/content/docs/ko/troubleshooting/windows-memory.mddocs-site/src/content/docs/ru/troubleshooting/windows-memory.mddocs-site/src/content/docs/troubleshooting/windows-memory.mddocs-site/src/content/docs/zh-cn/troubleshooting/windows-memory.mdsrc/cli/doctor.tssrc/update/job.tstests/doctor.test.ts
| // The refresh that just failed was `ocx service repair` (serviceReinstallArgs), | ||
| // which needs no elevation because it never calls `schtasks /create`. Advising | ||
| // `install` here would send the user to re-registration — a UAC prompt on | ||
| // Windows and a possible WinSW-to-scheduler backend switch — to fix a service | ||
| // that is already registered. Point at the same command that failed so its | ||
| // output explains why, on every platform. | ||
| updateJob( | ||
| job, | ||
| {}, | ||
| `Service reinstall failed (exit ${result.status ?? "?"}); falling back to a direct proxy start.` | ||
| + (process.platform === "win32" | ||
| ? " Run 'ocx service install' as administrator to refresh the background service manager." | ||
| : " Run 'ocx service install' by hand to see the reason, then 'ocx service status'."), | ||
| `Service refresh failed (exit ${result.status ?? "?"}); falling back to a direct proxy start.` | ||
| + " Run 'ocx service repair' by hand to see the reason, then 'ocx service status'.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a focused regression test for the failed refresh message.
The supplied test changes cover proxyDownRestartHint, but they do not exercise restartAfterUpdate when the service command returns a non-zero status. Add coverage for installed non-conflicting services, conflicting services, and the direct-start fallback.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/update/job.ts` around lines 826 - 836, Add focused regression coverage
for restartAfterUpdate when the service refresh returns a non-zero status:
verify the installed non-conflicting, conflicting, and direct-start fallback
paths all produce the expected failed-refresh guidance, including the manual
“ocx service repair” and status commands. Keep the existing proxyDownRestartHint
tests unchanged and use the existing service fixtures/helpers.
Source: Path instructions
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the conflict-specific recovery command.
Line 826 always points users to ocx service repair after an installed-service refresh fails. src/service.ts Lines 1772-1777 reject repairService() when both backends are present and require ocx service uninstall followed by ocx service install. This branch does not inspect the conflict state, so a conflicting installation can receive a command that fails for the same reason again. Carry the conflict state into restartAfterUpdate, or propagate the repair diagnostic, so conflicts receive uninstall/install guidance and installed non-conflicting services receive repair guidance.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/update/job.ts` around lines 826 - 836, Update the installed-service
refresh failure path around restartAfterUpdate so it distinguishes backend
conflicts before choosing recovery guidance. Propagate the conflict state or
repair diagnostic from the service refresh flow, directing conflicting
installations to run `ocx service uninstall` followed by `ocx service install`,
while retaining `ocx service repair` guidance for non-conflicting installed
services.
| const absent = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: false }); | ||
| expect(absent).toContain("ocx service install"); | ||
|
|
||
| // A two-manager conflict must be uninstalled first; repairService() refuses it. | ||
| const conflict = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: true, serviceConflict: true }); | ||
| expect(conflict).toContain("ocx service install"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert that conflict guidance excludes repair.
Line 545 checks only for ocx service install. A regression that emits both install and repair would pass while violating the conflict rule. Add negative assertions for the conflict and absent cases.
Suggested assertions
const absent = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: false });
expect(absent).toContain("ocx service install");
+ expect(absent).not.toContain("ocx service repair");
// A two-manager conflict must be uninstalled first; repairService() refuses it.
const conflict = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: true, serviceConflict: true });
expect(conflict).toContain("ocx service install");
+ expect(conflict).not.toContain("ocx service repair");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const absent = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: false }); | |
| expect(absent).toContain("ocx service install"); | |
| // A two-manager conflict must be uninstalled first; repairService() refuses it. | |
| const conflict = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: true, serviceConflict: true }); | |
| expect(conflict).toContain("ocx service install"); | |
| const absent = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: false }); | |
| expect(absent).toContain("ocx service install"); | |
| expect(absent).not.toContain("ocx service repair"); | |
| // A two-manager conflict must be uninstalled first; repairService() refuses it. | |
| const conflict = proxyDownRestartHint({ proxyRunning: false, port: 10100, serviceViable: false, serviceInstalled: true, serviceConflict: true }); | |
| expect(conflict).toContain("ocx service install"); | |
| expect(conflict).not.toContain("ocx service repair"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/doctor.test.ts` around lines 541 - 546, Strengthen the assertions in
the proxyDownRestartHint test for both the absent-service and serviceConflict
cases so the returned guidance contains “ocx service install” but does not
contain the repair command. Update the assertions around proxyDownRestartHint
and preserve the existing positive install checks.
…remaining advice Round 3 of the audit found two RUNTIME bugs I introduced in bee1cc7, plus the last stale advice surfaces. Both runtime bugs were reproduced before fixing. The DTO type never declared repairService, so TypeScript could not catch either: markStartupHealthDiagnosticStale() re-derived recommendedCommand and hardcoded installService, silently undoing the repair choice on every stale-cache read — the exact path the dashboard hits while a probe revalidates: before: ocx service repair -> after stale marking: ocx service install the subprocess DTO rebuild copied installService/installShim/restoreNative and dropped repairService, so after a warm probe the GUI recovery row rendered an empty command and would copy undefined. StartupHealth.commands now declares repairService, which is what makes both sites type-checked rather than convention-checked. Remaining advice surfaces, all reachable with a service already installed: - src/cli/index.ts printed 'install the persistent service' unconditionally while the service summary three lines later told the same user to repair — one status report, two contradictory instructions. - assertServiceAuthEnvironment() and installLaunchd() are both reached THROUGH repair (macOS repair delegates straight to installLaunchd), so they now name the command the caller actually ran. - structure/05 and an update/index comment described the old command set. - Two GUI fixtures omitted repairService, which is what let an incomplete DTO look acceptable in those tests. Red-green: restoring the unconditional installService in the stale-cache path fails the new guard. The pre-existing stale-cache test asserted status and protection but never the resulting command, which is why this regression was invisible to it. 254 pass / 0 fail across service, startup, doctor, update and winsw suites; 583 pass / 0 fail gui.
…fuses Round 4 blocker. My preflight retry hint checked diagnoseService().installed but ignored conflict, unlike the other three sites. Concrete failure: Windows with both Task Scheduler and WinSW registered, non-loopback hostname, no OPENCODEX_API_AUTH_TOKEN. The preflight runs before repairService() (service.ts:2544), so the user was told to set the token and rerun 'ocx service repair' — a command repairService() then refuses outright (:1783), which sends them to uninstall-then-install. Two round trips to reach advice the first message could have given. Install IS the valid recovery for a conflict: installWindows removes the native backend before registering the scheduler one. So the predicate now matches the other three sites exactly: installed && !conflict ? repair : install. The message's first sentence also said the token is required 'before installing' even when reached from repair; it now says refreshing or installing to match. Also fixed while checking the reviewer's second question: installLaunchd() captured its installed-signal AFTER writeFileSync(plistPath()), so existsSync was unconditionally true and every fresh install would have been told to repair. Captured before the write instead.
The round-5 reviewer caught that my guard was vacuous: it defined a local pick() lambda duplicating the predicate and asserted against that, so reverting the production fix to the broken installed-only logic would have left it green. A guard that cannot fail is worse than no guard — it reads as coverage. Extracted serviceRetryCommand() from assertServiceAuthEnvironment() and the test now calls it with the three diagnostic shapes directly. It takes a Pick<ServiceDiagnostic, 'installed' | 'conflict'> defaulting to diagnoseService(), so production behavior is unchanged and the test needs no environment mocking. Red-green: reverting the predicate to 'diag.installed ? repair : install' now fails the guard (97 pass / 1 fail), and restoring it returns 98 pass / 0 fail. This is the third vacuous or wrong-reason test caught in this session — the Windows GUI guard that ran on macOS, the stale-service test that asserted status but never the command, and now this one.
1 participant
Follow-up to #980, produced by a six-round adversarial audit of the merged
devstate (7343f0ba5).What was wrong
ocx service repairshipped in #980 and is what the post-update refresh now runs — but the advice surfaces were never migrated with it. Seven places still told users to runocx service installfor a service that was already registered.That is not cosmetic.
repairrefreshes the installed backend in place;installre-registers, which means a UAC prompt on Windows (schtasks /create) and a transactional backend switch that can replace a WinSW install with Task Scheduler.docs-siteCLI reference, 5 localesrepairundocumented; EN example readRepair: ocx service installdocs-siteWindows-memory guide, 5 localesservice installservice repairservice.ts:2478launchd hintinstallrepair(its sibling at:1676was fixed in #980; this one was missed)autostart-health.tsinstallrepairstartup-health-cache.ts×2repair→install; DTO rebuild dropped the fieldcli/index.tsinstallwhile the summary 3 lines later saidrepairupdate/job.tsrepair, then on failure said "runinstallas administrator"doctor.ts,service.tsauth preflight,installLaunchdinstallTwo of these were runtime bugs I introduced mid-fix
StartupHealth.commandsnever declaredrepairService, so TypeScript could not catch either site:The second meant that after a warm probe the dashboard recovery row rendered an empty command and would copy
undefined. Declaring the field on the interface is what makes both sites type-checked rather than convention-checked.Consistency
All six decision sites now use the identical predicate —
serviceInstalled && !serviceConflict— becauserepairService()refuses a two-manager conflict outright, andinstallis the valid conflict recovery sinceinstallWindowsremoves the native backend first.Still
install, correctly: absent service, missing systemd unit, first-time setup,--nativebackend switch, and rollback.Verification
bun x tsc --noEmitclean ·bun run lint:guiexit 0 ·bun run privacy:scanpassedThree of my own tests were caught as vacuous during this audit and rewritten: a Windows guard that ran on macOS so the branch never executed, a stale-service test that asserted status but never the resulting command, and a preflight test that re-implemented the predicate as a lambda instead of calling it. The last one would have stayed green with the fix reverted.
Residual risk: no destructive live service-lifecycle test on a real Windows or macOS host; platform-manager behavior is covered by seams and focused tests.