fix(startup): avoid permission-sensitive service re-registration - #970
fix(startup): avoid permission-sensitive service re-registration#970stephen-drew wants to merge 4 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Confirmed as real and left open — not folded into the current review stack (#951–#973). Two reasons. It touches a permission-sensitive path (service registration during self-update) across 30 files spanning CLI, GUI, and five docs locales, which is its own review with its own risk profile. And per The direction reads right to me — |
…it (#970) `ocx update` stops the proxy before replacing package files, then brought the service back with `ocx service install`. The Windows scheduler installer always reaches `schtasks /create`, which requires elevation the updater does not have, so an ordinary non-elevated update stopped a working proxy and could not restore its managed service. serviceReinstallArgs() now returns ["service", "repair"], which rewrites the wrapper assets and restarts the EXISTING registration without /create. The export name is kept for out-of-module callers; serviceInstallArgs() is split out for the paths that genuinely need to register. Safety of the substitution: repairService() throws when the service is not installed, and the update path runs after `ocx stop` — but stop never deregisters on any platform. macOS unloads the plist, Windows calls /end, Linux calls systemctl stop; deletion lives only in the uninstall paths. Verified across all three (evidence: devlog 020). Two things a straight argv change would have missed: The Windows GUI worker skipped the refresh entirely (update/job.ts) because its own comment said /create would UAC-fail. That reason does not survive repair, so the skip is narrowed to callers still passing install argv — otherwise the dashboard-triggered update, the most common Windows path, keeps the bug while the CLI gets fixed. bin/ocx.mjs infers 'a service manages this proxy' from service-state.json existing, which can be stale. Repair correctly refuses that case, but its thrown Error is indistinguishable from any other failure there (plain Error, inherited stdio, generic exit status), so message-matching was unimplementable and a blanket install-on-failure would resurrect the elevation prompt. It now reads startup.serviceInstalled from the `status --json` subprocess it already spawns — the file is plain Node ESM and cannot import diagnoseService() directly. Advice strings that fire only for an INSTALLED service now say repair: cli/status, winsw missing-binary, stale baked paths, stale scheduler assets, the launchd older-plist and not-loaded hints. First-install and missing-unit guidance stays install. Red-green: restoring the unconditional Windows skip fails the new guard. Six existing tests pinned the install argv and were updated with reasons. 245 pass / 0 fail across the service, update, winsw, doctor, status, startup and Windows-deploy suites.
|
Superseded by #980 (stack 7/7), commit Your diagnosis is correct and it is now fixed. The safety question your PR did not answer, now verified. Two things the reconstruction adds. The Windows GUI worker never reached the command at all. The stale-marker hole. Also carried: the advice strings that fire only for an already-installed service now say Six existing tests pinned the old install argv and were updated with reasons; a new guard proves the non-elevated Windows worker now receives Thank you — the elevation analysis was the hard part and you got it right. |
…ice repair Two overnight contributor PRs describe real defects the lidge-jun#951-lidge-jun#973 stack does not touch. This unit plans layer 7 as their reconstruction. lidge-jun#964 cannot be carried: five ids in its hand-written text-only list are natively image-capable per NVIDIA's own docs (inkling, minimax-m3, kimi-k2.6, step-3.7-flash, mistral-medium-3.5-128b). A false positive there is silent — the model can read the image, but the proxy substitutes another model's text description. Issue lidge-jun#956's own body carries two of the same errors, so reporter and author shared the premise. 010 inverts the design: maintain the 15 verified vision-capable ids and derive text-only as the complement, so an unclassified new model defaults to sidecar-on rather than to the bug being fixed. lidge-jun#970's premise is right but its diff is oversized: repairService() and 'ocx service repair' already exist here. 020 records the safety proof that matters — repair throws when not installed and the update path runs after 'ocx stop', but stop never deregisters on any of the three platforms. It also closes a hole lidge-jun#970 leaves: bin/ocx.mjs infers service presence from a possibly-stale marker, where repair would throw and lose the managed service. 030 sequences the bottom-up merge and issue closure, including the lidge-jun#954 security-review gate that can legitimately stop the queue.
The A-gate reviewer returned FAIL. Every blocker was reproduced before being accepted; none was rebutted. 001 records the synthesis. B1 killed my own design. I proposed maintaining the 15 vision-capable ids and deriving text-only as their complement, and claimed an unclassified model would default to sidecar-on. It does not — a complement over a static chat-model list leaves an unknown id in neither list, so modelInList returns false and lidge-jun#956 survives verbatim: deepseek-ai/deepseek-v4-flash sidecarWouldRun=true moonshotai/kimi-k2.6 sidecarWouldRun=false brandnew/model-nobody-classified sidecarWouldRun=false I had inverted which list is maintained while keeping the closed world — the same lesson as the three earlier allowlist failures, reproduced while writing the document that cites them. 010 now changes the predicate instead: default-on for the nvidia entry with the vision list as its exception set, so a stale exception list costs one description hop rather than reproducing the bug. B2: removing a native-vision id from noVisionModels is not sufficient. The catalog advertises image input only for list members, so those models would be blocked client-side instead. They need explicit modelInputModalities. B3: src/update/job.ts:775 skips the service refresh entirely on non-elevated Windows — the dashboard path. Its stated reason is that schtasks /create needs UAC, which repair does not call, so the skip must be narrowed or the reporter's own surface stays broken. B4: repairService throws plain Errors and bin/ocx.mjs sees only an exit status, so 'fall back on not-installed' was unimplementable. Re-run diagnoseService() after a failed repair instead of parsing messages. B5: retargeting emits 'edited', which ci.yml does not listen for, so a green check on the same head sha proves nothing about the new merge base. 030 also moves the lidge-jun#964/lidge-jun#970 closure from 'when stack 7 opens' to 'open and green'. The earlier text borrowed a policy from the six carried PRs, which had verified replacement commits already on a branch; this replacement does not exist yet and its first design just failed audit.
2 participants
Summary
ocx service repairduring self-update instead of re-registering it.ocx repairas a documented alias ofocx service repair; reserve Install for a missing, disabled or conflicting registration and explain the Windows administrator requirement.Root cause
On Windows, an existing Task Scheduler registration can be owned by Administrators while the signed-in user is still allowed to run and stop it. Recovery paths that unconditionally suggested
ocx service installinvokedschtasks /create /f, so a repairable service failed with Access denied even thoughschtasks /runand/endwere permitted.User impact
Updates and ordinary recovery no longer overwrite an existing registration. A stopped service can be started, stale generated assets can be repaired without elevation, and a genuine re-registration still follows the explicit administrator-approved install path.
Verification
PATHand updating the obsolete repair assertion.cd docs-site && bun install --frozen-lockfile && bun run buildocx stop, thenocx repair; replacement PID served on127.0.0.1:10100and Startup read backprotected,protection=service,diagnosticStale=false.Source commits before the current
devrebase:52ef5469and70d2066d.Checklist