fix(lifecycle): consolidate identity-bound restart and service install cleanup - #1206
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces stop-then-start proxy restarts with attested in-place replacement. It adds PID/port-bound authorization, replacement validation, safer service installation cleanup, tray process tracking, lifecycle tests, and updated localized documentation. ChangesRestart lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RunningProxy
participant ReplacementProxy
participant ServiceManager
CLI->>RunningProxy: verify PID, port, and attestation
CLI->>RunningProxy: request in-place restart
RunningProxy->>ReplacementProxy: drain and replace process
CLI->>ReplacementProxy: verify new healthy PID on same port
ReplacementProxy->>ServiceManager: preserve managed supervision
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
7 tasks
4 tasks
github-actions
Bot
added
the
intake: hygiene-blocked
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Wibias
marked this pull request as ready for review
August 7, 2026 09:51
Contributor
✅ READY
|
Collaborator
Author
|
not gui |
Wibias
marked this pull request as ready for review
August 7, 2026 09:57
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@docs-site/src/content/docs/guides/grok-build.md`:
- Around line 130-132: Qualify the managed-block preservation statement in the
service-installed “ocx restart” guidance so it applies only to loopback
auto-registration, while stating that service supervision remains installed
unconditionally. Apply the same wording change in
docs-site/src/content/docs/guides/grok-build.md lines 130-132,
docs-site/src/content/docs/ja/guides/grok-build.md line 81,
docs-site/src/content/docs/ko/guides/grok-build.md line 76, and
docs-site/src/content/docs/zh-cn/guides/grok-build.md line 76; preserve the
documented managed-configuration behavior for service-managed proxy flows.
In `@docs-site/src/content/docs/ja/troubleshooting/windows-memory.md`:
- Line 29: Update the replacement-success descriptions to require a healthy,
identity-verified process replacement on the same port, not merely a different
PID. Apply this consistently in
docs-site/src/content/docs/ja/troubleshooting/windows-memory.md:29-29,
docs-site/src/content/docs/zh-cn/reference/cli/lifecycle.md:31-33, and
docs-site/src/content/docs/zh-cn/troubleshooting/windows-memory.md:26-26,
matching the actual CLI lifecycle behavior before declaring success.
In `@docs-site/src/content/docs/reference/cli/lifecycle.md`:
- Around line 43-45: Document both restart-recovery paths after confirming
listener ownership: standalone or manual-restart setups should run ocx stop
followed by ocx start once, while service-managed setups should run ocx service
start after ocx stop to restore supervision. Apply this clarification in
docs-site/src/content/docs/reference/cli/lifecycle.md lines 43-45,
docs-site/src/content/docs/ko/reference/cli/lifecycle.md lines 43-45, and
docs-site/src/content/docs/ru/reference/cli/lifecycle.md lines 47-49.
In `@docs-site/src/content/docs/troubleshooting/windows-memory.md`:
- Around line 57-58: Update the lifecycle wording to distinguish restart
coordination from replacement execution: in
docs-site/src/content/docs/troubleshooting/windows-memory.md:57-58,
docs-site/src/content/docs/ko/troubleshooting/windows-memory.md:26-26, and
docs-site/src/content/docs/ru/troubleshooting/windows-memory.md:52-54, state
that the proxy coordinates or requests replacement and the service manager
starts it in managed mode; apply the same correction in
docs-site/src/content/docs/ru/guides/grok-build.md:116-118, removing any claim
that the running proxy replaces itself.
In `@src/lib/system-restart-contract.ts`:
- Around line 58-72: Update verifySystemRestartCapability and the restart
request flow to maintain a bounded process-scoped set of consumed nonces,
atomically marking each nonce consumed only after successful HMAC validation and
before scheduling the restart. Reject subsequent uses of the same nonce,
preserve the validated principal for the request so managementPrincipal() does
not revalidate, and add a replay test confirming identical valid headers
authenticate only once.
In `@src/service.ts`:
- Line 2234: Replace the `stopWinswService` cleanup used by
`prepareServiceInstall` with an install-specific WinSW stop operation that
suppresses only confirmed already-stopped or absent-service results and
propagates access-denied, timeout, and other failures. Ensure `install()` does
not proceed when the native stop fails, and add a regression test covering that
failure path.
In `@src/tray/windows-tray.ps1`:
- Around line 272-295: Update the Start Proxy and Stop Proxy click handlers to
invoke Start-OcxCommand with -TrackExit, assign each returned process to
$script:pendingProcess when it is a Process object, and call
Complete-PendingAction $false when tracking fails or the command cannot start.
Match the existing restart handler’s process-tracking behavior and add
regression assertions covering both handlers.
In `@structure/05_gui-and-management-api.md`:
- Line 95: Update the authentication credential-class table near the existing
three classes to add the process-scoped HMAC restart capability. Document that
it authorizes only POST /api/system/restart and grants no general /api/* access,
matching the behavior described in the restart endpoint entry.
In `@tests/system-restart-client.test.ts`:
- Around line 194-215: Add a focused case to the existing “treats an HTTP
rejection…” test covering a `/healthz` fetch that throws, then assert
`requestBoundSystemRestart` returns `accepted: false` with `uncertain: false`.
Keep the existing HTTP-rejection and POST-transport-loss assertions unchanged,
and use the existing `successfulDeps` setup and response helpers.
In `@tests/tray-proxy.test.ts`:
- Around line 234-253: Add a focused test beside the existing
discoverStableProxyForRestart tests that supplies an expired callback and
exercises both deadline guards: verify an expired absence observation returns
the fail-closed uncertain result, and cover expiration after the initial
live-target absence check as well. Assert the callback is invoked and the
discovery does not return absent or proceed toward the fallback start path.
🪄 Autofix
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: 675e49d9-b76d-4b71-a780-9d76b6846572
📒 Files selected for processing (37)
docs-site/src/content/docs/guides/grok-build.mddocs-site/src/content/docs/ja/guides/grok-build.mddocs-site/src/content/docs/ja/reference/cli/lifecycle.mddocs-site/src/content/docs/ja/troubleshooting/windows-memory.mddocs-site/src/content/docs/ko/guides/grok-build.mddocs-site/src/content/docs/ko/reference/cli/lifecycle.mddocs-site/src/content/docs/ko/troubleshooting/windows-memory.mddocs-site/src/content/docs/reference/cli/lifecycle.mddocs-site/src/content/docs/ru/guides/grok-build.mddocs-site/src/content/docs/ru/reference/cli/lifecycle.mddocs-site/src/content/docs/ru/troubleshooting/windows-memory.mddocs-site/src/content/docs/troubleshooting/windows-memory.mddocs-site/src/content/docs/zh-cn/guides/grok-build.mddocs-site/src/content/docs/zh-cn/reference/cli/lifecycle.mddocs-site/src/content/docs/zh-cn/troubleshooting/windows-memory.mdsrc/cli/index.tssrc/cli/system-restart-client.tssrc/cli/tray-proxy.tssrc/lib/system-restart-contract.tssrc/server/index.tssrc/server/management-auth.tssrc/server/management/system-restart.tssrc/server/management/system-routes.tssrc/server/proxy-liveness.tssrc/service.tssrc/tray/windows-tray.ps1structure/05_gui-and-management-api.mdtests/bun-runtime.test.tstests/grok-lifecycle.test.tstests/server-auth.test.tstests/server-live.test.tstests/server-management-auth.test.tstests/service.test.tstests/system-restart-client.test.tstests/system-restart.test.tstests/tray-proxy.test.tstests/windows-tray.test.ts
Comment on lines
+57
to
+58
| asks the attested live process to replace itself, then verifies a different | ||
| PID on the same port without tearing down Codex injection. That is a |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish restart coordination from replacement execution.
The running proxy owns authorization and drain, but the supervised implementation exits with code 1 and relies on the service supervisor to start the replacement. Saying that the proxy replaces itself or manages the replacement is inaccurate for managed installations.
docs-site/src/content/docs/troubleshooting/windows-memory.md#L57-L58: say that the proxy coordinates or requests replacement; the service manager starts it in managed mode.docs-site/src/content/docs/ko/troubleshooting/windows-memory.md#L26-L26: apply the same distinction.docs-site/src/content/docs/ru/troubleshooting/windows-memory.md#L52-L54: apply the same distinction.docs-site/src/content/docs/ru/guides/grok-build.md#L116-L118: replace the claim that the running proxy itself performs the replacement.
As per path instructions, user-facing lifecycle documentation must stay synchronized with actual CLI/API behavior.
📍 Affects 4 files
docs-site/src/content/docs/troubleshooting/windows-memory.md#L57-L58(this comment)docs-site/src/content/docs/ko/troubleshooting/windows-memory.md#L26-L26docs-site/src/content/docs/ru/troubleshooting/windows-memory.md#L52-L54docs-site/src/content/docs/ru/guides/grok-build.md#L116-L118
🤖 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 `@docs-site/src/content/docs/troubleshooting/windows-memory.md` around lines 57
- 58, Update the lifecycle wording to distinguish restart coordination from
replacement execution: in
docs-site/src/content/docs/troubleshooting/windows-memory.md:57-58,
docs-site/src/content/docs/ko/troubleshooting/windows-memory.md:26-26, and
docs-site/src/content/docs/ru/troubleshooting/windows-memory.md:52-54, state
that the proxy coordinates or requests replacement and the service manager
starts it in managed mode; apply the same correction in
docs-site/src/content/docs/ru/guides/grok-build.md:116-118, removing any claim
that the running proxy replaces itself.
Source: Path instructions
Comment on lines
+58
to
+72
| export function verifySystemRestartCapability( | ||
| secret: string, | ||
| nonce: string | null, | ||
| method: string, | ||
| path: string, | ||
| pid: number, | ||
| port: number, | ||
| capability: string | null, | ||
| ): boolean { | ||
| if (!nonce || !capability || !BASE64URL_256.test(capability)) return false; | ||
| const expected = createSystemRestartCapability(secret, nonce, method, path, pid, port); | ||
| if (!expected) return false; | ||
| const expectedBytes = Buffer.from(expected); | ||
| const actualBytes = Buffer.from(capability); | ||
| return expectedBytes.length === actualBytes.length && timingSafeEqual(expectedBytes, actualBytes); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Consume each restart capability after its first successful validation.
Lines 67-72 recompute and verify the HMAC, but they do not record a consumed nonce. The same nonce and capability can authenticate every replay until the current process exits. This does not meet the PR requirement for a one-use process-scoped capability.
Store a bounded per-process set of consumed nonces. Atomically consume the nonce after HMAC validation and before restart scheduling. Preserve the validated principal for the request so managementPrincipal() does not require a second successful validation. Add a replay test that sends the same valid headers twice and verifies that only the first request authenticates.
🤖 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/lib/system-restart-contract.ts` around lines 58 - 72, Update
verifySystemRestartCapability and the restart request flow to maintain a bounded
process-scoped set of consumed nonces, atomically marking each nonce consumed
only after successful HMAC validation and before scheduling the restart. Reject
subsequent uses of the same nonce, preserve the validated principal for the
request so managementPrincipal() does not revalidate, and add a replay test
confirming identical valid headers authenticate only once.
| | Logs & Debug | One sidebar entry (`/#logs`) with two tabs. Logs tab: request/runtime logs for local diagnosis. Debug tab (`/#logs/debug`; legacy `/#debug` deep links redirect there): provider + usage toggles, refresh/follow log viewer. `GET/PUT /api/debug`; `GET /api/debug/logs` and `GET /api/debug/usage-logs` (monotonic `after` cursor, legacy `since` accepted). CLI: `ocx debug provider|usage …` (both streams via running proxy API). | | ||
| | Usage | `GET /api/usage` aggregate read-only summary derived from `~/.opencodex/usage.jsonl`; measured / reported / unreported / unsupported / estimated counts, daily zero-filled grid, model and provider breakdowns. Never exposes prompts. | | ||
| | System | `POST /api/system/restart` restarts the proxy in place. `GET /api/system/memory` — service-process runtime/memory identity (pid, Bun version/revision, optional `bunRuntimeSource` provenance, platform, RSS/heap/external/ArrayBuffers scalars, observed memory = max(RSS, external, ArrayBuffers), `bun:jsc` heap context, streamMode + eager-relay gate decision, watchdog snapshot sliced to the last 60 samples) plus privacy-safe `appOwnedBytes` retained-store totals/counters under static store ids. Scalar-only payload; rides the standard management auth gate and must never move to unauthenticated `/healthz`. Consumed by `ocx doctor`'s Memory/runtime section and the dashboard Memory observability card. | | ||
| | System | `POST /api/system/restart` restarts the proxy in place. Local CLI/tray callers first attest the exact runtime PID and port, then send a process-scoped HMAC capability bound to that method, path, PID, and port; the capability authorizes no other management route and is invalid after replacement. The caller observes one absolute deadline and accepts success only after a different runtime PID is healthy on the same port. `GET /api/system/memory` — service-process runtime/memory identity (pid, Bun version/revision, optional `bunRuntimeSource` provenance, platform, RSS/heap/external/ArrayBuffers scalars, observed memory = max(RSS, external, ArrayBuffers), `bun:jsc` heap context, streamMode + eager-relay gate decision, watchdog snapshot sliced to the last 60 samples) plus privacy-safe `appOwnedBytes` retained-store totals/counters under static store ids. Scalar-only payload; rides the standard management auth gate and must never move to unauthenticated `/healthz`. Consumed by `ocx doctor`'s Memory/runtime section and the dashboard Memory observability card. | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the restart capability as a credential class.
Line 95 describes a process-scoped HMAC capability that authorizes POST /api/system/restart. Lines 15-25 still state that only three mutually exclusive credential classes exist. This creates a contradictory security boundary.
Add the restart capability to the authentication table. State that it authorizes only the exact restart method and path, and does not grant general /api/* access.
🤖 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 `@structure/05_gui-and-management-api.md` at line 95, Update the authentication
credential-class table near the existing three classes to add the process-scoped
HMAC restart capability. Document that it authorizes only POST
/api/system/restart and grants no general /api/* access, matching the behavior
described in the restart endpoint entry.
Comment on lines
+234
to
+253
| test("stable absence requires two empty observations and rejects a reappearing target", async () => { | ||
| let calls = 0; | ||
| const absent = await discoverStableProxyForRestart({ | ||
| findLive: async () => { calls += 1; return null; }, | ||
| waitBetweenChecks: async () => {}, | ||
| }); | ||
| expect(absent).toEqual({ status: "absent" }); | ||
| expect(calls).toBe(2); | ||
|
|
||
| calls = 0; | ||
| const appeared = await discoverStableProxyForRestart({ | ||
| findLive: async () => { | ||
| calls += 1; | ||
| return calls === 1 ? null : { pid: 20, port: 10100, source: "runtime" }; | ||
| }, | ||
| waitBetweenChecks: async () => {}, | ||
| }); | ||
| expect(appeared.status).toBe("uncertain"); | ||
| expect(calls).toBe(2); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case for the expired deadline branch of discoverStableProxyForRestart.
The two discovery tests here pass no expired callback, so the deadline guards in src/cli/tray-proxy.ts at lines 73-75 and lines 90-92 stay uncovered. That branch is the fail-closed gate that stops an out-of-budget absence observation from reaching the fallback start path in runProxyRestart. A regression that drops either guard would keep every current test green.
💚 Proposed extra case
expect(appeared.status).toBe("uncertain");
expect(calls).toBe(2);
+
+ calls = 0;
+ const expired = await discoverStableProxyForRestart({
+ findLive: async () => { calls += 1; return null; },
+ waitBetweenChecks: async () => {},
+ expired: () => true,
+ });
+ expect(expired.status).toBe("uncertain");
+ expect(calls).toBe(1);
});As per path instructions: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
📝 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.
Suggested change
| test("stable absence requires two empty observations and rejects a reappearing target", async () => { | |
| let calls = 0; | |
| const absent = await discoverStableProxyForRestart({ | |
| findLive: async () => { calls += 1; return null; }, | |
| waitBetweenChecks: async () => {}, | |
| }); | |
| expect(absent).toEqual({ status: "absent" }); | |
| expect(calls).toBe(2); | |
| calls = 0; | |
| const appeared = await discoverStableProxyForRestart({ | |
| findLive: async () => { | |
| calls += 1; | |
| return calls === 1 ? null : { pid: 20, port: 10100, source: "runtime" }; | |
| }, | |
| waitBetweenChecks: async () => {}, | |
| }); | |
| expect(appeared.status).toBe("uncertain"); | |
| expect(calls).toBe(2); | |
| }); | |
| test("stable absence requires two empty observations and rejects a reappearing target", async () => { | |
| let calls = 0; | |
| const absent = await discoverStableProxyForRestart({ | |
| findLive: async () => { calls += 1; return null; }, | |
| waitBetweenChecks: async () => {}, | |
| }); | |
| expect(absent).toEqual({ status: "absent" }); | |
| expect(calls).toBe(2); | |
| calls = 0; | |
| const appeared = await discoverStableProxyForRestart({ | |
| findLive: async () => { | |
| calls += 1; | |
| return calls === 1 ? null : { pid: 20, port: 10100, source: "runtime" }; | |
| }, | |
| waitBetweenChecks: async () => {}, | |
| }); | |
| expect(appeared.status).toBe("uncertain"); | |
| expect(calls).toBe(2); | |
| calls = 0; | |
| const expired = await discoverStableProxyForRestart({ | |
| findLive: async () => { calls += 1; return null; }, | |
| waitBetweenChecks: async () => {}, | |
| expired: () => true, | |
| }); | |
| expect(expired.status).toBe("uncertain"); | |
| expect(calls).toBe(1); | |
| }); |
🤖 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/tray-proxy.test.ts` around lines 234 - 253, Add a focused test beside
the existing discoverStableProxyForRestart tests that supplies an expired
callback and exercises both deadline guards: verify an expired absence
observation returns the fail-closed uncertain result, and cover expiration after
the initial live-target absence check as well. Assert the callback is invoked
and the discovery does not return absent or proceed toward the fallback start
path.
Source: Path instructions
![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: ac4258f770
ℹ️ 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".
| if (status === "unknown") throw new Error("Native service status could not be verified."); | ||
| return status === "nonexistent" ? null : status; | ||
| }, | ||
| stop: stopWinswService, |
There was a problem hiding this comment.
When reinstalling or switching from a running WinSW service, this assigns stopWinswService, but that helper catches every stopwait error as though the service were already stopped. An access-denied, timeout, or other real stop failure therefore lets prepareServiceInstall continue into asset writes while the old service can still own and respawn the listener, defeating this cleanup's fail-closed contract and potentially allowing verification against the old process. Use a strict stop operation here that suppresses only a confirmed already-stopped result and propagates all other failures.
Useful? React with 👍 / 👎.
github-actions
Bot
added
intake: hygiene-blocked
Wibias
force-pushed
the
agent/consolidate-restart-supervision
branch
from
August 7, 2026 10:34
31217b1 to
bdd8f64
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/service.ts`:
- Around line 2261-2262: Update the Linux systemd status probe to treat only
loadState === "not-found" as absent; throw when LoadState output is empty so
callers fail closed, while preserving normal state returns. Add a regression
test for empty output covering the probe behavior and ensuring active backends
are not treated as absent.
🪄 Autofix
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: 21c2ae07-0da0-4555-93d0-51706a32ef0f
📒 Files selected for processing (1)
src/service.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/system-restart-contract-security.test.ts`:
- Around line 27-28: Add a positive acceptance test alongside the rejection
cases in the system restart capability security tests, using the original nonce,
method, path, runtime PID, port, and capability() to call
verifySystemRestartCapability and assert it returns true. Preserve the existing
stale-PID rejection test.
🪄 Autofix
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: 4061c76d-85a7-4f26-974b-39399da66d2f
📒 Files selected for processing (3)
src/tray/windows-tray.ps1tests/system-restart-contract-security.test.tstests/windows-tray-restart-hardening.test.ts
Comment on lines
+27
to
+28
| describe("system restart capability security boundary", () => { | ||
| test("rejects a capability when the expected runtime pid is stale", () => { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a valid capability acceptance test.
All current assertions expect false. If verifySystemRestartCapability always returns false, this suite still passes. Add a round-trip assertion with the original nonce, method, path, PID, port, and capability().
Proposed test
describe("system restart capability security boundary", () => {
+ test("accepts a capability for the signed runtime and operation", () => {
+ expect(verifySystemRestartCapability(
+ SECRET,
+ NONCE,
+ SYSTEM_RESTART_METHOD,
+ SYSTEM_RESTART_PATH,
+ PID,
+ PORT,
+ capability(),
+ )).toBe(true);
+ });
+
test("rejects a capability when the expected runtime pid is stale", () => {📝 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.
Suggested change
| describe("system restart capability security boundary", () => { | |
| test("rejects a capability when the expected runtime pid is stale", () => { | |
| describe("system restart capability security boundary", () => { | |
| test("accepts a capability for the signed runtime and operation", () => { | |
| expect(verifySystemRestartCapability( | |
| SECRET, | |
| NONCE, | |
| SYSTEM_RESTART_METHOD, | |
| SYSTEM_RESTART_PATH, | |
| PID, | |
| PORT, | |
| capability(), | |
| )).toBe(true); | |
| }); | |
| test("rejects a capability when the expected runtime pid is stale", () => { |
🤖 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/system-restart-contract-security.test.ts` around lines 27 - 28, Add a
positive acceptance test alongside the rejection cases in the system restart
capability security tests, using the original nonce, method, path, runtime PID,
port, and capability() to call verifySystemRestartCapability and assert it
returns true. Preserve the existing stale-PID rejection test.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tray/windows-tray.ps1 (1)
269-283: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive tracked command failure precedence over state observation.
At Lines 281-282,
$reachedis evaluated before$commandFailed. If another manager reaches the target state while this command exits nonzero, the tray reports success and disposes the failed process handle.Check
$commandFailedfirst. Then evaluate$reached. Add a regression assertion intests/windows-tray-restart-hardening.test.tsthat verifies this failure-first ordering.Proposed fix
- if ($reached) { Complete-PendingAction $true } - elseif ($commandFailed) { Complete-PendingAction $false } + if ($commandFailed) { Complete-PendingAction $false } + elseif ($reached) { Complete-PendingAction $true } elseif ($now -gt $script:pendingDeadline) { Complete-PendingAction $false }🤖 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/tray/windows-tray.ps1` around lines 269 - 283, Update the pending-action completion logic after `$commandFailed` is computed so the `$commandFailed` branch calls `Complete-PendingAction $false` before evaluating `$reached`; retain the deadline fallback afterward. Add a regression assertion in `windows-tray-restart-hardening.test.ts` confirming a nonzero tracked command exit takes precedence over a reached target state.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/tray/windows-tray.ps1`:
- Around line 269-283: Update the pending-action completion logic after
`$commandFailed` is computed so the `$commandFailed` branch calls
`Complete-PendingAction $false` before evaluating `$reached`; retain the
deadline fallback afterward. Add a regression assertion in
`windows-tray-restart-hardening.test.ts` confirming a nonzero tracked command
exit takes precedence over a reached target state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 24094838-1ddf-4a91-b939-942a8c678bc3
📒 Files selected for processing (2)
src/tray/windows-tray.ps1tests/windows-tray-restart-hardening.test.ts
This was referenced Aug 7, 2026
Wibias
added a commit
that referenced
this pull request
Aug 7, 2026
github-actions
Bot
force-pushed
the
agent/consolidate-restart-supervision
branch
from
August 7, 2026 18:45
c8a5368 to
7459eda
Compare
Wibias
force-pushed
the
agent/consolidate-restart-supervision
branch
from
August 7, 2026 18:56
b10ef3b to
fc7a561
Compare
github-actions
Bot
removed
the
intake: hygiene-blocked
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Supersedes #1131 and #1160.
This maintainer-controlled consolidation keeps one restart architecture and carries the non-overlapping service-install hardening from the other PR.
Restart authority — carried from #1131
Service-install cleanup — carried from #1160
Before writing/loading new service assets:
This prevents an old listener from retaining the service port, forcing the newly installed manager into a restart loop, or making the old listener look like a successful install.
Intentionally not carried
#1160's alternate
ocx restartstop/start implementation is not included. #1131's identity-bound in-place restart is the canonical restart path in this consolidation.Credit
Thank you @luvs01 for the identity-bound restart design and implementation in #1131.
Thank you @jonathanli12 for the service-install cleanup and supervision work in #1160.
Verification
devbefore final review.Checklist
devbefore final review.Summary by CodeRabbit
New Features
Documentation