Windows turnkey release preparation#97
Merged
Merged
Conversation
added 9 commits
June 7, 2026 14:38
…pipeline Implements three parallel slices from the Windows Turnkey Release roadmap: Slice 1 (#74): NSIS installer config for per-user Windows install - perMachine: false, oneClick: false, runAfterFinish: true - Start Menu shortcut, clean uninstall with deleteAppDataOnUninstall Slice 2 (#73): Credential-first provider scanning for all 7 providers - Detects env var credentials and config directory presence - Scans binary PATH for installed provider CLIs - Returns structured per-provider status (ready/needs-config/not-installed) Slice 4 (#72): Managed download/verify pipeline for OpenCode runtime - Fetches latest release from GitHub releases API - Platform-specific asset matching (win-x64, linux-x64, darwin-arm64/x64) - SHA-256 verification with atomic download-replace pattern - chmod on non-Windows platforms Also includes ADR 0005 (backend-owned managed runtime sidecar) and ADR 0006 (provider-agnostic first-run wizard). Contracts: providerScan.ts, managedRuntime.ts Implementation: providerCredentialScan.ts, managedRuntimeDownload.ts Tests: 32 new tests, all passing; snip 7 existing tests unaffected
…inget manifests Slice 5 (#83): Managed sidecar lifecycle - contracts: ManagedSidecarSnapshot schema with idle/downloading/starting/ready/stopping/error states - server: ManagedSidecarLifecycle service using Effect ServiceMap + Ref state machine - Fresh password per start (crypto.randomBytes, base64url), stored in snapshot only - 16 tests covering state transitions, error handling, password uniqueness Slice 3 (#84): Provider-agnostic first-run wizard - contracts: FirstRunState, FirstRunWizardData, CompleteFirstRunWizardInput schemas - server: detectFirstRunState, getFirstRunWizardData, completeFirstRunWizard, skipFirstRunWizard - Full RPC pipeline: contracts → WS methods → IPC → wsNativeApi → wsRpc handlers - web: FirstRunWizard component placeholder - 5 tests using ServerSettingsService.layerTest() Slice 8 (#78): Scoop + Winget package manifests - Scoop manifest template (packages/scoop/jcode.json) - Winget YAML manifest template (packages/winget/manifests/j/JCode/JCode.yaml) - generate-winget-manifest.ts: YAML generators with validation (hash, version, repo slug) - update-package-manifests.ts: Downloads installer, computes SHA-256, writes manifests - CI: update_package_manifests job in release.yml - 12 tests covering manifest generation, SHA-256 computation, error handling Closes #83, #84, #78
…car health/diagnostics Implements Slice 6 (#81) and Slice 7 (#85) of the Windows Turnkey Release. Slice 6 — Managed Runtime Profile Auto-Creation: - Contract schemas: ManagedRuntimeProfileAutoCreateRequest/Result - Auto-creates OpenCodeRuntimeProfile from sidecar snapshot - Clean install defaults: mode=managed, configMode=generated - Existing config defaults: mode=external, configMode=inherit - Profile deduplication: returns existing profile if ID matches - applyAutoCreatedProfile() returns ServerSettingsPatch - 7/7 tests passing Slice 7 — Sidecar Health Check, Repair, and Diagnostics: - Contract schemas: ManagedSidecarHealthCheck, RepairRequest/Result, Diagnostics - Health status mapping: ready+valid→healthy, ready+missing→unhealthy, error→unhealthy, idle/stopping→not_running, downloading→degraded - Repair: stop + optional forceRedownload + restart + health recheck - Diagnostics: health + platform info + sidecar snapshot - 18/18 tests passing Phase 2 files restored after Slice 7 agent regression. Total: 46/46 tests passing across all Phase 2+3 slices.
…-release-prd # Conflicts: # apps/server/src/wsRpc.ts # apps/web/src/wsNativeApi.ts # docs/adr/README.md # packages/contracts/src/ipc.ts # packages/contracts/src/settings.ts
|
Warning Review limit reached
More reviews will be available in 32 minutes and 10 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds managed OpenCode sidecar lifecycle and first-run wizard flows, WS RPC/API and web UI wiring for those flows, updated ADR/context docs, and release automation that generates Scoop and Winget manifests. ChangesManaged sidecar and first-run onboarding
Release manifest generation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
github-actions
Bot
added
size:XXL
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/adr/README.md (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate ADR index metadata after adding new records.
Last reviewedshould be refreshed when adding ADR entries (0009/0010) so index metadata remains accurate.As per coding guidelines, “Keep metadata tables current when adding or substantially changing a document.”
Also applies to: 27-29
🤖 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/adr/README.md` at line 11, The Last reviewed date in the ADR README index file needs to be updated to reflect when new ADR entries (0009 and 0010) were added. Update the Last reviewed field from its current value to today's date to maintain accurate metadata as per the coding guideline that metadata tables should be kept current when adding or substantially changing documents. This same update pattern should also be applied to the affected lines noted in the comment (lines 27-29) to ensure all relevant metadata timestamps are consistent and current.Source: Coding guidelines
🤖 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 @.github/workflows/release.yml:
- Around line 358-360: The finalize job needs to be updated to depend on the
newly added update_package_manifests job to prevent finalize from running if
manifest generation fails. Add update_package_manifests to the needs array of
the finalize job so that it waits for successful manifest generation before
proceeding to push release-version commits.
In `@apps/server/src/provider/firstRunWizard.ts`:
- Around line 109-124: The managedRuntimeDir is being resolved unconditionally
at the start of completeFirstRunWizard, which causes the function to fail
entirely if the resolution fails, even for users who should take the
external-profile path. Move the yield* resolveManagedRuntimeDir resolution
inside the conditional block where it is actually needed, specifically only when
the managed profile branch is being taken. This way, resolution failures only
affect users taking the managed-profile path and not those who should use the
external-profile path.
In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts`:
- Around line 3784-3789: The condition checking for managed sidecar lifecycle
disable currently requires both serverUrl AND serverPassword to be present
together to disable the managed sidecar mode. This means if a caller provides
only a serverUrl without a password, the managed sidecar is still used, which is
incorrect. In the needsManagedSidecarLifecycle assignment, change the logic from
checking `!(providerServerUrl && providerServerPassword)` to
`!(providerServerUrl || providerServerPassword)` and similarly change
`!(profileServerUrl && profileServerPassword)` to `!(profileServerUrl ||
profileServerPassword)` so that providing either a serverUrl OR a serverPassword
(independently) will disable managed sidecar mode. Additionally, add a
regression test case that verifies managed profile mode with only
providerOptions.serverUrl (without password) correctly disables the managed
sidecar lifecycle.
In `@apps/server/src/provider/managedRuntimeDownload.ts`:
- Around line 321-330: The verifyManagedRuntimeBinary function unconditionally
yields resolveManagedRuntimeDir even when binaryPath is explicitly provided,
causing unnecessary side effects and potential failures. Refactor the function
to only yield resolveManagedRuntimeDir when binaryPath is not provided, making
the runtimeDir resolution and path.join operation conditional. When binaryPath
is provided, skip the runtime directory resolution entirely and use the provided
path directly.
- Around line 106-116: The GitHubReleaseApiResponse schema is missing the digest
field that GitHub's API provides for all release assets, preventing digest
verification from working. Add an optional digest field
(Schema.optional(Schema.String)) to the asset Struct within the
GitHubReleaseApiResponse schema. Then update the asset mapping logic to include
the digest value when constructing the asset object. Finally, update the
verification logic that compares digests to strip the sha256: prefix from the
parsed digest before comparing it with the computed hash value.
In `@apps/server/src/provider/managedRuntimeHealth.ts`:
- Around line 209-210: The forceRedownload parameter defined in the function
signature is being ignored because the code hardcodes { forceDownload: true }
when invoking the download operation. Replace the hardcoded true value with the
forceRedownload parameter to honor the caller's intent. Additionally, update the
test expectations in the managedRuntimeHealth.test.ts file to verify both
scenarios: when forceRedownload is true (forcing download) and when it is false
or omitted (allowing restart-only repair without forcing a download).
In `@apps/server/src/provider/managedRuntimeLifecycle.test.ts`:
- Around line 529-557: The test "does not reuse password after a failed start"
only asserts that the second password is truthy but does not actually verify
that the password value differs between the first failed attempt and the second
successful attempt. Capture the serverPassword from both calls to
lifecycle.startManagedRuntime() (or inspect the mock function arguments passed
to startOpenCodeServerProcess in each call) and add an explicit assertion
comparing the two password values to ensure they are distinct, so the test
properly enforces that passwords are not reused after a failure.
In `@apps/server/src/provider/providerCredentialScan.test.ts`:
- Around line 64-111: The test suite for resolveBinaryPath only covers the Linux
platform, but the feature explicitly supports Windows. Add new test cases within
the "resolveBinaryPath" describe block to validate Windows platform behavior.
Specifically, add tests that verify the function correctly handles the Windows
PATH separator (semicolon instead of colon), resolves executable candidates with
Windows extensions (.exe, .cmd, .bat), and properly handles quoted PATH entries
on win32. Create test cases similar to the existing ones but with platform set
to "win32" and PATH environment variables containing Windows-style paths and
separators to ensure cross-platform regression coverage.
In `@apps/server/src/provider/providerCredentialScan.ts`:
- Around line 137-146: In the resolveBinaryPath function, the fullPath
construction uses string concatenation instead of platform-safe path joining,
and the pathEntries are not dequoted which can cause issues on Windows with
quoted PATH values. Replace the string concatenation approach for building
fullPath with a proper path joining method that handles platform-specific
separators, and add a dequoting step to the pathEntries after splitting to
remove surrounding quotes before iterating through them in the loop.
In `@apps/server/src/wsRpc.ts`:
- Around line 116-123: The resolveLocalLegacyWsAuthSession function currently
grants LOCAL_LEGACY_OWNER_AUTH_SESSION when authToken is missing, creating a
security bypass. The condition should be hardened to only return the owner
session when both authToken and legacyToken are explicitly provided and match
each other, not when authToken is undefined. Remove the !input.authToken portion
from the logical OR condition and require explicit token matching for the legacy
owner session to be granted.
- Around line 287-297: The forceRedownload intent passed from the request is
being correctly forwarded to repairManagedSidecar in this function, but the
downstream repair implementation is not respecting the false value. Review the
repairManagedSidecar function implementation (in
apps/server/src/provider/managedRuntimeHealth.ts) and ensure that when
forceDownload is passed as false, the repair flow does not unconditionally start
with a redownload. Update the implementation to use the forceDownload parameter
value as intended rather than always defaulting to true.
In `@apps/web/src/components/FirstRunWizard.browser.tsx`:
- Around line 304-311: The test assertion for the skip-path is too loose and
allows false positives where completeFirstRunWizard could still be called with a
provider payload while the test passes. Tighten the assertion by replacing the
current check that completeFirstRunWizard was not called with an empty object to
instead verify that completeFirstRunWizard was never called at all when testing
the skip path behavior. This ensures the test properly validates that only
skipFirstRun is invoked and prevents regressions where completeFirstRunWizard
might be called with non-empty payloads.
In `@docs/adr/0009-backend-owned-managed-runtime-sidecar.md`:
- Line 24: Remove brittle source code references from the ADR that include
specific line counts and line numbers. In the section describing
openCodeRuntimeHealth.ts, remove the "459 lines" reference and any specific line
number citations like "line 805", keeping only the canonical file path and the
invariant descriptions of what the code does (such as the health check states
and exported functions). Apply the same fix to line 67 where similar brittle
references exist. Keep the architectural content and invariant statements while
replacing line-specific references with canonical file paths, ensuring the ADR
remains authoritative as the codebase evolves.
In `@docs/adr/0010-provider-agnostic-first-run-wizard.md`:
- Line 14: The ADR's Verification section inaccurately documents the provider
count as 7 and describes the provider readiness logic as "credentials and/or
binary," but the actual implementation covers 9 providers with a state machine
using ready, needs-config, and not-installed states where ready requires both
credentials and binary present. Update the Verification statement and all
related descriptions throughout the document (including the sections referenced
at lines 18-18 and 30-31) to accurately reflect the actual ProviderDiscoveryKind
count of 9 providers and the correct readiness contract where ready status
requires both credentials and binary to be available, not either-or.
In `@packages/contracts/src/firstRunWizard.ts`:
- Line 22: The `completedAt` field in the firstRunWizard.ts file at line 22 uses
a generic Schema.String which allows any string value and weakens the contract
for a date field. Replace Schema.String with an appropriate timestamp-specific
schema type (such as Schema.timestamp or similar timestamp schema provided by
the Schema library) while keeping it wrapped in Schema.optional to maintain the
optional nature of the field.
In `@packages/contracts/src/settings.ts`:
- Around line 120-122: The firstRun property uses a hardcoded default literal ({
completed: false, skipped: false }) that duplicates the DEFAULT_FIRST_RUN_STATE
constant. This creates maintenance risk where future changes to FirstRunState
required fields could cause the hardcoded literal and the constant to diverge.
Replace the inline default literal in the Schema.withDecodingDefault call with a
reference to DEFAULT_FIRST_RUN_STATE to ensure the defaults stay synchronized.
In `@packages/contracts/src/ws.test.ts`:
- Around line 170-224: The test block is missing decode tests for two new server
WebSocket methods that were added in this PR: serverGetFirstRunWizardData and
serverCompleteFirstRunWizard. Add two new it.effect test cases after the
serverSkipFirstRun test, following the same pattern as the existing tests in
this block. Each test should decode a WebSocketRequest with the respective _tag,
verify the decoded body contains the correct _tag using assert.strictEqual, and
include appropriate test IDs like "req-get-wizard-data-1" and
"req-complete-wizard-1".
In `@scripts/generate-winget-manifest.test.ts`:
- Around line 86-95: Add a direct regression test for the repositoryToWingetId
function with invalid input. Currently, invalid repository slug validation is
tested indirectly through createWingetInstallerManifest. Create a new test case
that directly calls repositoryToWingetId with the value "bad slug" and asserts
it throws an error matching the pattern /Invalid GitHub repository slug/. This
will lock in the deterministic validation behavior for this exported helper
function independently.
In `@scripts/generate-winget-manifest.ts`:
- Around line 121-125: The repositoryToWingetId function lacks input validation
and will throw a raw TypeError when repository is malformed (doesn't contain a
"/" separator), causing repo to be undefined and subsequently failing when
calling repo.slice(). Add validation at the beginning of the
repositoryToWingetId function to check that the repository string contains
exactly one "/" and splits into both owner and repo components; throw an
appropriate validation error with a descriptive message if the format is
invalid, before attempting to call slice on the repo variable.
In `@scripts/update-package-manifests.ts`:
- Around line 63-69: The fetchFn call that downloads the Windows installer from
the buildWindowsInstallerUrl does not have a timeout configured, which can cause
the release automation to hang indefinitely if the connection stalls. Add a
timeout parameter to the fetchFn call to ensure the operation fails fast if the
download takes too long. Configure the timeout value to a reasonable duration
that allows normal downloads to complete while preventing indefinite waits.
---
Outside diff comments:
In `@docs/adr/README.md`:
- Line 11: The Last reviewed date in the ADR README index file needs to be
updated to reflect when new ADR entries (0009 and 0010) were added. Update the
Last reviewed field from its current value to today's date to maintain accurate
metadata as per the coding guideline that metadata tables should be kept current
when adding or substantially changing documents. This same update pattern should
also be applied to the affected lines noted in the comment (lines 27-29) to
ensure all relevant metadata timestamps are consistent and current.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 644980cd-3ae2-4b20-95d8-d96b7542ab61
📒 Files selected for processing (63)
.github/workflows/release.ymlCONTEXT.mdapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/firstRunWizard.test.tsapps/server/src/provider/firstRunWizard.tsapps/server/src/provider/managedRuntimeDownload.test.tsapps/server/src/provider/managedRuntimeDownload.tsapps/server/src/provider/managedRuntimeHealth.test.tsapps/server/src/provider/managedRuntimeHealth.tsapps/server/src/provider/managedRuntimeLifecycle.test.tsapps/server/src/provider/managedRuntimeLifecycle.tsapps/server/src/provider/managedRuntimeProfile.test.tsapps/server/src/provider/managedRuntimeProfile.tsapps/server/src/provider/openCodeRuntimeHealth.test.tsapps/server/src/provider/openCodeRuntimeHealth.tsapps/server/src/provider/openCodeRuntimeProfiles.tsapps/server/src/provider/opencodeRuntime.test.tsapps/server/src/provider/opencodeRuntime.tsapps/server/src/provider/providerCredentialScan.test.tsapps/server/src/provider/providerCredentialScan.tsapps/server/src/provider/runtimeLayer.tsapps/server/src/wsRpc.test.tsapps/server/src/wsRpc.tsapps/web/src/components/EventRouter.browser.tsxapps/web/src/components/FirstRunWizard.browser.tsxapps/web/src/components/FirstRunWizard.tsxapps/web/src/components/OpenCodeRuntimeSettingsPanel.browser.tsxapps/web/src/components/OpenCodeRuntimeSettingsPanel.tsxapps/web/src/routes/__root.tsxapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsdocs/adr/0006-remote-client-runtime-ws-rpc-scope-wiring.mddocs/adr/0007-parallel-windows-wsl-backend-routing.mddocs/adr/0008-scoped-remote-client-capability-tokens.mddocs/adr/0009-backend-owned-managed-runtime-sidecar.mddocs/adr/0010-provider-agnostic-first-run-wizard.mddocs/adr/README.mdpackage.jsonpackages/contracts/src/firstRunWizard.tspackages/contracts/src/index.tspackages/contracts/src/ipc.tspackages/contracts/src/ipc.typecheck.tspackages/contracts/src/managedRuntime.tspackages/contracts/src/managedRuntimeHealth.test.tspackages/contracts/src/managedRuntimeHealth.tspackages/contracts/src/managedRuntimeLifecycle.tspackages/contracts/src/managedRuntimeProfile.tspackages/contracts/src/providerDiscovery.tspackages/contracts/src/providerScan.tspackages/contracts/src/rpc.test.tspackages/contracts/src/rpc.tspackages/contracts/src/settings.tspackages/contracts/src/ws.test.tspackages/contracts/src/ws.tspackages/scoop/jcode.jsonpackages/winget/manifests/j/JCode/JCode.yamlscripts/build-desktop-artifact-mac-config.test.tsscripts/generate-winget-manifest.test.tsscripts/generate-winget-manifest.tsscripts/lib/desktop-platform-build-config.tsscripts/update-package-manifests.test.tsscripts/update-package-manifests.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/server/src/wsRpc.ts`:
- Around line 1713-1717: Guard providerGetRuntimeBootstrapStatus with
withCurrentSession like the other owner-only runtime RPCs in wsRpc.ts. The issue
is that this handler currently calls getOpenCodeRuntimeBootstrapStatus(input)
directly, so scoped or non-owner sessions can read
ProviderRuntimeBootstrapSnapshot fields. Update the
WS_METHODS.providerGetRuntimeBootstrapStatus entry to wrap the rpcEffect call in
withCurrentSession, matching the authorization pattern used by the adjacent
bootstrap/repair endpoints and the access-checked providerGetRuntimeHealth path.
In `@apps/web/src/wsNativeApi.ts`:
- Around line 711-735: The provider API methods are still capturing the initial
transport instance, so they can call a disposed client after session/auth
resets. Update the provider method bindings in wsNativeApi.ts to route through
requestWs instead of closing over transport, using the existing requestWs helper
and the related provider method symbols like getComposerCapabilities,
getRuntimeHealth, and listAgents so they always use the lazily recreated
transport.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7790d3ab-7b3c-4cab-ab71-fe375bbf1195
📒 Files selected for processing (11)
apps/server/src/wsRpc.tsapps/web/src/components/OpenCodeRuntimeSettingsPanel.browser.tsxapps/web/src/components/OpenCodeRuntimeSettingsPanel.tsxapps/web/src/wsNativeApi.tsdocs/adr/0007-parallel-windows-wsl-backend-routing.mdpackages/contracts/src/ipc.tspackages/contracts/src/providerDiscovery.tspackages/contracts/src/rpc.test.tspackages/contracts/src/rpc.tspackages/contracts/src/ws.test.tspackages/contracts/src/ws.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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 `@apps/server/src/wsRpc.ts`:
- Around line 1713-1717: Guard providerGetRuntimeBootstrapStatus with
withCurrentSession like the other owner-only runtime RPCs in wsRpc.ts. The issue
is that this handler currently calls getOpenCodeRuntimeBootstrapStatus(input)
directly, so scoped or non-owner sessions can read
ProviderRuntimeBootstrapSnapshot fields. Update the
WS_METHODS.providerGetRuntimeBootstrapStatus entry to wrap the rpcEffect call in
withCurrentSession, matching the authorization pattern used by the adjacent
bootstrap/repair endpoints and the access-checked providerGetRuntimeHealth path.
In `@apps/web/src/wsNativeApi.ts`:
- Around line 711-735: The provider API methods are still capturing the initial
transport instance, so they can call a disposed client after session/auth
resets. Update the provider method bindings in wsNativeApi.ts to route through
requestWs instead of closing over transport, using the existing requestWs helper
and the related provider method symbols like getComposerCapabilities,
getRuntimeHealth, and listAgents so they always use the lazily recreated
transport.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7790d3ab-7b3c-4cab-ab71-fe375bbf1195
📒 Files selected for processing (11)
apps/server/src/wsRpc.tsapps/web/src/components/OpenCodeRuntimeSettingsPanel.browser.tsxapps/web/src/components/OpenCodeRuntimeSettingsPanel.tsxapps/web/src/wsNativeApi.tsdocs/adr/0007-parallel-windows-wsl-backend-routing.mdpackages/contracts/src/ipc.tspackages/contracts/src/providerDiscovery.tspackages/contracts/src/rpc.test.tspackages/contracts/src/rpc.tspackages/contracts/src/ws.test.tspackages/contracts/src/ws.ts
🛑 Comments failed to post (2)
apps/server/src/wsRpc.ts (1)
1713-1717: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Guard
providerGetRuntimeBootstrapStatuslike the other runtime endpoints.Line 1713 skips
withCurrentSession(...), so scoped/non-owner sessions can readProviderRuntimeBootstrapSnapshotdata such asbinaryPath,serverUrl, andprofileId. The adjacent bootstrap/repair RPCs are owner-gated, and evenproviderGetRuntimeHealthis access-checked, so this new read endpoint currently falls outside the intended WS authorization model.Suggested fix
- [WS_METHODS.providerGetRuntimeBootstrapStatus]: (input) => - rpcEffect( - getOpenCodeRuntimeBootstrapStatus(input), - "Failed to get runtime bootstrap status", - ), + [WS_METHODS.providerGetRuntimeBootstrapStatus]: (input) => + withCurrentSession( + requireOwnerWsRpcAccess, + rpcEffect( + getOpenCodeRuntimeBootstrapStatus(input), + "Failed to get runtime bootstrap status", + ), + ),📝 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.[WS_METHODS.providerGetRuntimeBootstrapStatus]: (input) => withCurrentSession( requireOwnerWsRpcAccess, rpcEffect( getOpenCodeRuntimeBootstrapStatus(input), "Failed to get runtime bootstrap status", ), ),🧰 Tools
🪛 ast-grep (0.44.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 { execFile } 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 `@apps/server/src/wsRpc.ts` around lines 1713 - 1717, Guard providerGetRuntimeBootstrapStatus with withCurrentSession like the other owner-only runtime RPCs in wsRpc.ts. The issue is that this handler currently calls getOpenCodeRuntimeBootstrapStatus(input) directly, so scoped or non-owner sessions can read ProviderRuntimeBootstrapSnapshot fields. Update the WS_METHODS.providerGetRuntimeBootstrapStatus entry to wrap the rpcEffect call in withCurrentSession, matching the authorization pattern used by the adjacent bootstrap/repair endpoints and the access-checked providerGetRuntimeHealth path.apps/web/src/wsNativeApi.ts (1)
711-735: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Route provider calls through
requestWstoo.Line 648 can dispose/reset the cached transport, but these provider methods still close over the original
transport. After browser-session auth, provider actions can keep using a disposed transport instead of the lazily recreated one.Proposed fix
provider: { getComposerCapabilities: (input) => - transport.request(WS_METHODS.providerGetComposerCapabilities, input), - getRuntimeHealth: (input) => transport.request(WS_METHODS.providerGetRuntimeHealth, input), + requestWs(WS_METHODS.providerGetComposerCapabilities, input), + getRuntimeHealth: (input) => requestWs(WS_METHODS.providerGetRuntimeHealth, input), getManagedSidecarHealth: () => - transport.request(WS_METHODS.providerGetManagedSidecarHealth, undefined), + requestWs(WS_METHODS.providerGetManagedSidecarHealth), repairManagedSidecar: (input) => - transport.request(WS_METHODS.providerRepairManagedSidecar, input), + requestWs(WS_METHODS.providerRepairManagedSidecar, input), exportManagedSidecarDiagnostics: () => - transport.request(WS_METHODS.providerExportManagedSidecarDiagnostics, undefined), + requestWs(WS_METHODS.providerExportManagedSidecarDiagnostics), getRuntimeBootstrapStatus: (input) => - transport.request(WS_METHODS.providerGetRuntimeBootstrapStatus, input), - bootstrapRuntime: (input) => transport.request(WS_METHODS.providerBootstrapRuntime, input), - repairRuntime: (input) => transport.request(WS_METHODS.providerRepairRuntime, input), - compactThread: (input) => transport.request(WS_METHODS.providerCompactThread, input), - listCommands: (input) => transport.request(WS_METHODS.providerListCommands, input), - listSkills: (input) => transport.request(WS_METHODS.providerListSkills, input), - installSkill: (input) => transport.request(WS_METHODS.providerInstallSkill, input), - uninstallSkill: (input) => transport.request(WS_METHODS.providerUninstallSkill, input), - setSkillEnabled: (input) => transport.request(WS_METHODS.providerSetSkillEnabled, input), - searchSkillsCatalog: (input) => - transport.request(WS_METHODS.providerSearchSkillsCatalog, input), - listPlugins: (input) => transport.request(WS_METHODS.providerListPlugins, input), - readPlugin: (input) => transport.request(WS_METHODS.providerReadPlugin, input), - listModels: (input) => transport.request(WS_METHODS.providerListModels, input), - listAgents: (input) => transport.request(WS_METHODS.providerListAgents, input), + requestWs(WS_METHODS.providerGetRuntimeBootstrapStatus, input), + bootstrapRuntime: (input) => requestWs(WS_METHODS.providerBootstrapRuntime, input), + repairRuntime: (input) => requestWs(WS_METHODS.providerRepairRuntime, input), + compactThread: (input) => requestWs(WS_METHODS.providerCompactThread, input), + listCommands: (input) => requestWs(WS_METHODS.providerListCommands, input), + listSkills: (input) => requestWs(WS_METHODS.providerListSkills, input), + installSkill: (input) => requestWs(WS_METHODS.providerInstallSkill, input), + uninstallSkill: (input) => requestWs(WS_METHODS.providerUninstallSkill, input), + setSkillEnabled: (input) => requestWs(WS_METHODS.providerSetSkillEnabled, input), + searchSkillsCatalog: (input) => requestWs(WS_METHODS.providerSearchSkillsCatalog, input), + listPlugins: (input) => requestWs(WS_METHODS.providerListPlugins, input), + readPlugin: (input) => requestWs(WS_METHODS.providerReadPlugin, input), + listModels: (input) => requestWs(WS_METHODS.providerListModels, input), + listAgents: (input) => requestWs(WS_METHODS.providerListAgents, input), },📝 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.getComposerCapabilities: (input) => requestWs(WS_METHODS.providerGetComposerCapabilities, input), getRuntimeHealth: (input) => requestWs(WS_METHODS.providerGetRuntimeHealth, input), getManagedSidecarHealth: () => requestWs(WS_METHODS.providerGetManagedSidecarHealth), repairManagedSidecar: (input) => requestWs(WS_METHODS.providerRepairManagedSidecar, input), exportManagedSidecarDiagnostics: () => requestWs(WS_METHODS.providerExportManagedSidecarDiagnostics), getRuntimeBootstrapStatus: (input) => requestWs(WS_METHODS.providerGetRuntimeBootstrapStatus, input), bootstrapRuntime: (input) => requestWs(WS_METHODS.providerBootstrapRuntime, input), repairRuntime: (input) => requestWs(WS_METHODS.providerRepairRuntime, input), compactThread: (input) => requestWs(WS_METHODS.providerCompactThread, input), listCommands: (input) => requestWs(WS_METHODS.providerListCommands, input), listSkills: (input) => requestWs(WS_METHODS.providerListSkills, input), installSkill: (input) => requestWs(WS_METHODS.providerInstallSkill, input), uninstallSkill: (input) => requestWs(WS_METHODS.providerUninstallSkill, input), setSkillEnabled: (input) => requestWs(WS_METHODS.providerSetSkillEnabled, input), searchSkillsCatalog: (input) => requestWs(WS_METHODS.providerSearchSkillsCatalog, input), listPlugins: (input) => requestWs(WS_METHODS.providerListPlugins, input), readPlugin: (input) => requestWs(WS_METHODS.providerReadPlugin, input), listModels: (input) => requestWs(WS_METHODS.providerListModels, input), listAgents: (input) => requestWs(WS_METHODS.providerListAgents, input),🤖 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 `@apps/web/src/wsNativeApi.ts` around lines 711 - 735, The provider API methods are still capturing the initial transport instance, so they can call a disposed client after session/auth resets. Update the provider method bindings in wsNativeApi.ts to route through requestWs instead of closing over transport, using the existing requestWs helper and the related provider method symbols like getComposerCapabilities, getRuntimeHealth, and listAgents so they always use the lazily recreated transport.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/adr/0010-provider-agnostic-first-run-wizard.md (1)
26-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten the readiness-state wording.
The prose says the strategy can tell whether a provider is missing credentials, a PATH binary, or both, but the three states only encode
ready,needs-config, andnot-installed. As written,not-installedcollapses “missing binary only” and “missing both,” so please reword this or add a separate state. As per coding guidelines, “Keep docs source-grounded; do not invent commands, scripts, environment variables, or runtime behavior.”♻️ Suggested wording
-Provider detection uses a credential-first, binary-second strategy. This is architecturally important because it can explain whether a provider is missing credentials, a PATH binary, or both: +Provider detection uses a credential-first, binary-second strategy. This is architecturally important because it can distinguish providers that are ready, need configuration, or need installation:🤖 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/adr/0010-provider-agnostic-first-run-wizard.md` around lines 26 - 30, The readiness-state description in the provider detection section is inconsistent with the stated credential-first, binary-second strategy because the existing states in the doc collapse missing-binary-only and missing-both into not-installed. Update the wording around the provider state model in this ADR so it only claims what the implementation and providerMaintenance.ts binary discovery actually support, or add a distinct state if that distinction is meant to be represented; keep the prose source-grounded and aligned with the three documented states ready, needs-config, and not-installed.Source: Coding guidelines
🤖 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 `@apps/web/src/components/ChatView.browser.tsx`:
- Around line 577-580: The helper addThreadToSnapshot currently accepts
Partial<OrchestrationThread>, but it only applies envMode, branch, and
worktreePath. Narrow the overrides parameter to a type that includes only those
fields, and update any callers or test fixtures accordingly so the signature
matches the actual behavior of addThreadToSnapshot.
In `@docs/adr/0009-backend-owned-managed-runtime-sidecar.md`:
- Around line 13-14: Broaden the ADR’s source-of-truth table so it reflects the
managed download and lifecycle pipeline, not just the runtime/health/discovery
pieces. Update the relevant row in the document to cite the additional download
and lifecycle modules alongside opencodeRuntime.ts, openCodeRuntimeHealth.ts,
and providerDiscovery.ts, keeping the wording strictly source-grounded and
aligned with the existing Source of truth / Verification entries.
---
Outside diff comments:
In `@docs/adr/0010-provider-agnostic-first-run-wizard.md`:
- Around line 26-30: The readiness-state description in the provider detection
section is inconsistent with the stated credential-first, binary-second strategy
because the existing states in the doc collapse missing-binary-only and
missing-both into not-installed. Update the wording around the provider state
model in this ADR so it only claims what the implementation and
providerMaintenance.ts binary discovery actually support, or add a distinct
state if that distinction is meant to be represented; keep the prose
source-grounded and aligned with the three documented states ready,
needs-config, and not-installed.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c26712f1-82fc-44e7-82db-6c93d190bc80
📒 Files selected for processing (6)
apps/server/src/wsRpc.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/wsNativeApi.tsdocs/adr/0009-backend-owned-managed-runtime-sidecar.mddocs/adr/0010-provider-agnostic-first-run-wizard.md
Comment on lines
+13
to
+14
| | Source of truth | `apps/server/src/provider/opencodeRuntime.ts`, `apps/server/src/provider/openCodeRuntimeHealth.ts`, `packages/contracts/src/providerDiscovery.ts` | | ||
| | Verification | Managed runtime spawns via server-side Effect ChildProcess; health checks gate desktop readiness; managed profiles are OpenCode-specific in contracts schema | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Broaden the source-of-truth table.
The ADR now depends on the managed download and lifecycle pipeline, but the source-of-truth row only cites opencodeRuntime.ts, openCodeRuntimeHealth.ts, and providerDiscovery.ts. Add the download/lifecycle modules so the document stays source-grounded as the implementation evolves. As per coding guidelines, “Keep docs source-grounded; do not invent commands, scripts, environment variables, or runtime behavior.”
♻️ Suggested update
-| Source of truth | `apps/server/src/provider/opencodeRuntime.ts`, `apps/server/src/provider/openCodeRuntimeHealth.ts`, `packages/contracts/src/providerDiscovery.ts` |
+| Source of truth | `apps/server/src/provider/managedRuntimeDownload.ts`, `apps/server/src/provider/managedRuntimeLifecycle.ts`, `apps/server/src/provider/opencodeRuntime.ts`, `apps/server/src/provider/openCodeRuntimeHealth.ts`, `packages/contracts/src/providerDiscovery.ts` |📝 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
| | Source of truth | `apps/server/src/provider/opencodeRuntime.ts`, `apps/server/src/provider/openCodeRuntimeHealth.ts`, `packages/contracts/src/providerDiscovery.ts` | | |
| | Verification | Managed runtime spawns via server-side Effect ChildProcess; health checks gate desktop readiness; managed profiles are OpenCode-specific in contracts schema | | |
| | Source of truth | `apps/server/src/provider/managedRuntimeDownload.ts`, `apps/server/src/provider/managedRuntimeLifecycle.ts`, `apps/server/src/provider/opencodeRuntime.ts`, `apps/server/src/provider/openCodeRuntimeHealth.ts`, `packages/contracts/src/providerDiscovery.ts` | | |
| | Verification | Managed runtime spawns via server-side Effect ChildProcess; health checks gate desktop readiness; managed profiles are OpenCode-specific in contracts schema | |
🤖 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/adr/0009-backend-owned-managed-runtime-sidecar.md` around lines 13 - 14,
Broaden the ADR’s source-of-truth table so it reflects the managed download and
lifecycle pipeline, not just the runtime/health/discovery pieces. Update the
relevant row in the document to cite the additional download and lifecycle
modules alongside opencodeRuntime.ts, openCodeRuntimeHealth.ts, and
providerDiscovery.ts, keeping the wording strictly source-grounded and aligned
with the existing Source of truth / Verification entries.
Source: Coding guidelines
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
Verification
Issue Evidence
Summary by CodeRabbit