feat(providers): harden supported CLI updates - #57
Merged
Conversation
…-clis # Conflicts: # apps/server/src/provider/Layers/ProviderService.ts # apps/server/src/provider/Services/ProviderService.ts
…-clis # Conflicts: # apps/server/src/serverLayers.ts
|
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:
📝 WalkthroughWalkthroughThis PR introduces conservative provider CLI update maintenance (gates, cross-process locks, owned-resource draining, target identity verification), Windows Job Object process containment for ACP/provider subprocesses with cooperative teardown, provider adapter ownership integration, verified packaged CLI publishing, and web-side update presentation with an accompanying runbook. ChangesProvider Maintenance Coordination
Estimated code review effort: 5 (Critical) | ~150 minutes ACP Process Ownership and Windows Job Containment
Estimated code review effort: 5 (Critical) | ~180 minutes Provider Adapter Ownership Integration
Estimated code review effort: 4 (Complex) | ~90 minutes Packaged CLI Verification and Provider Update UI
Estimated code review effort: 3 (Moderate) | ~40 minutes Supporting Fixes and Regression Tests
Estimated code review effort: 2 (Simple) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderHealth
participant ProviderMaintenanceGate
participant ProviderMaintenanceOwnedResources
participant ProviderProcessOwnerTracker
participant WindowsJobProcessSupervisor
ProviderHealth->>ProviderMaintenanceGate: withExclusiveMaintenance(provider)
ProviderMaintenanceGate->>ProviderMaintenanceOwnedResources: drainProviderResources
ProviderMaintenanceOwnedResources->>ProviderProcessOwnerTracker: teardown owned processes
ProviderProcessOwnerTracker->>WindowsJobProcessSupervisor: proveExit / requestStop
WindowsJobProcessSupervisor-->>ProviderProcessOwnerTracker: drain acknowledgement
ProviderProcessOwnerTracker-->>ProviderMaintenanceOwnedResources: teardown proven
ProviderMaintenanceOwnedResources-->>ProviderMaintenanceGate: drained
ProviderMaintenanceGate-->>ProviderHealth: run update command
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Owner
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/server/src/provider/providerMaintenance.ts (1)
438-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider collapsing the near-identical npm/bun/pnpm global builders.
makeNpmGlobalProviderMaintenanceCapabilities,makeBunGlobalProviderMaintenanceCapabilities, andmakePnpmGlobalProviderMaintenanceCapabilitiesshare the samenpmPackageNameguard,canonicalInstallRootguard,updatePathPrepend, andupdateTargetwiring, differing only inupdateArgsand thelockKeyprefix. A single helper parameterized by(updateArgs, lockKeyPrefix)would remove the triplicated control flow and keep future changes in one place.As per coding guidelines: "avoid duplicating logic across files and prefer reusable modules over local shortcuts."
🤖 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/provider/providerMaintenance.ts` around lines 438 - 532, The three global provider capability builders duplicate the same validation and capability wiring. Extract a shared helper parameterized by the manager-specific updateArgs and lock-key prefix, then have makeNpmGlobalProviderMaintenanceCapabilities, makeBunGlobalProviderMaintenanceCapabilities, and makePnpmGlobalProviderMaintenanceCapabilities delegate to it while preserving their distinct arguments and existing fallback behavior.Source: Coding guidelines
apps/server/src/provider/Layers/ProviderService.ts (1)
307-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared maintenance-busy wrapper into a reusable helper. All three sites wrap
maintenanceGate.withOperation(...)andcatchTag("ProviderMaintenanceBusyError", ...)to convert into a domain error — two of them produce an identicalProviderValidationError, and the third differs only in the target error type. Consider a single helper (e.g. exported fromproviderMaintenanceGate.ts) that takes abusy → errormapper, so the gating/translation logic lives in one place.
apps/server/src/provider/Layers/ProviderService.ts#L307-L319: replacewithProviderOperation's inlinecatchTagbody with the shared helper mapping toProviderValidationError.apps/server/src/provider/Layers/ProviderDiscoveryService.ts#L81-L91: replacewithProviderDiscovery's duplicatecatchTagbody with the same shared helper mapping toProviderValidationError.apps/server/src/git/Layers/ProviderTextGeneration.ts#L55-L71: use the shared helper inrunWithProviderMaintenance, passing a mapper that returnsTextGenerationError.As per coding guidelines: "Before adding functionality, check for shared logic that can be extracted; avoid duplicating logic across files and prefer reusable modules over local shortcuts."
🤖 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/provider/Layers/ProviderService.ts` around lines 307 - 319, Extract the shared maintenance-busy handling around maintenanceGate.withOperation into a reusable helper, preferably in providerMaintenanceGate.ts, accepting a busy-error mapper. Update apps/server/src/provider/Layers/ProviderService.ts lines 307-319 and apps/server/src/provider/Layers/ProviderDiscoveryService.ts lines 81-91 to use it with ProviderValidationError mapping; update apps/server/src/git/Layers/ProviderTextGeneration.ts lines 55-71 so runWithProviderMaintenance uses the same helper with a TextGenerationError mapper.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/server/src/keybindings.test.ts`:
- Around line 871-875: Extend the failure test around acquireUseRelease to read
keybindingsConfigPath after the operation completes, then assert the restored
configuration still contains the original terminal.toggle rule. Keep the
existing Result.isFailure and failure-message assertions, and use the test’s
established config-reading/parsing helpers.
In `@apps/server/src/provider/opencodeRuntime.ts`:
- Around line 1225-1235: Update the OpenCodeRuntimeError detail in the
Scope.close failure branch after Scope.close(pooledServer.scope, Exit.void) to
describe local server scope finalization failure, rather than process-tree exit
proof failure. Keep the existing cause handling and closeFailure assignment
unchanged.
In `@apps/server/src/terminal/processTreeKiller.ts`:
- Around line 267-278: Update the Windows teardown validation around
readCurrentProcesses and ProcessTreeKiller.signal so it does not call the
synchronous captureProcessSnapshotSync path from normal cleanup. Propagate an
async signal/validation flow that reuses captureProcessSnapshotAsync, preserving
the existing PID filtering and snapshot-selection behavior while ensuring
SIGKILL validation does not block the event loop.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 307-319: Extract the shared maintenance-busy handling around
maintenanceGate.withOperation into a reusable helper, preferably in
providerMaintenanceGate.ts, accepting a busy-error mapper. Update
apps/server/src/provider/Layers/ProviderService.ts lines 307-319 and
apps/server/src/provider/Layers/ProviderDiscoveryService.ts lines 81-91 to use
it with ProviderValidationError mapping; update
apps/server/src/git/Layers/ProviderTextGeneration.ts lines 55-71 so
runWithProviderMaintenance uses the same helper with a TextGenerationError
mapper.
In `@apps/server/src/provider/providerMaintenance.ts`:
- Around line 438-532: The three global provider capability builders duplicate
the same validation and capability wiring. Extract a shared helper parameterized
by the manager-specific updateArgs and lock-key prefix, then have
makeNpmGlobalProviderMaintenanceCapabilities,
makeBunGlobalProviderMaintenanceCapabilities, and
makePnpmGlobalProviderMaintenanceCapabilities delegate to it while preserving
their distinct arguments and existing fallback behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0744089e-649c-4cbd-b305-0c1c7fb80a79
📒 Files selected for processing (101)
.github/workflows/ci.ymlREADME.mdapps/desktop/scripts/smoke-test-windows-job.windows.integration.test.mjsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/scripts/acp-conformance-agent.tsapps/server/scripts/acp-mock-agent.tsapps/server/scripts/acp-windows-job-fixture.mjsapps/server/scripts/acp-windows-job-native.csapps/server/scripts/acp-windows-job.ps1apps/server/scripts/cli.tsapps/server/scripts/cliPublishContract.test.tsapps/server/scripts/cliPublishContract.tsapps/server/src/editorAppIcons.test.tsapps/server/src/git/Layers/CursorTextGeneration.test.tsapps/server/src/git/Layers/OpenCodeTextGeneration.test.tsapps/server/src/git/Layers/OpenCodeTextGeneration.tsapps/server/src/git/Layers/ProviderTextGeneration.test.tsapps/server/src/git/Layers/ProviderTextGeneration.tsapps/server/src/git/runtimeLayer.test.tsapps/server/src/git/runtimeLayer.tsapps/server/src/keybindings.test.tsapps/server/src/main.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/persistence/DatabaseLifecycleLock.test.tsapps/server/src/persistence/DatabaseLifecycleLock.tsapps/server/src/project/Layers/ProjectFaviconResolver.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/DroidAdapter.test.tsapps/server/src/provider/Layers/DroidAdapter.tsapps/server/src/provider/Layers/GrokAdapter.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.tsapps/server/src/provider/Layers/ProviderDiscoveryService.test.tsapps/server/src/provider/Layers/ProviderDiscoveryService.tsapps/server/src/provider/Layers/ProviderHealth.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Services/ProviderService.tsapps/server/src/provider/acp/AcpAdapterSessionSupport.test.tsapps/server/src/provider/acp/AcpJsonRpcConnection.test.tsapps/server/src/provider/acp/AcpSdkConformance.test.tsapps/server/src/provider/acp/AcpSessionRuntime.test.tsapps/server/src/provider/acp/AcpSessionRuntime.tsapps/server/src/provider/acp/AcpSessionRuntimeTestSupport.tsapps/server/src/provider/acp/AcpSessionTeardown.test.tsapps/server/src/provider/acp/AcpSessionTeardown.tsapps/server/src/provider/acp/AcpWindowsJob.test.tsapps/server/src/provider/acp/AcpWindowsJob.tsapps/server/src/provider/acp/AcpWindowsJob.windows.integration.test.tsapps/server/src/provider/acp/AcpWindowsJobTestSupport.tsapps/server/src/provider/acp/CursorAcpCommand.test.tsapps/server/src/provider/acp/DroidSessionTeardownGate.test.tsapps/server/src/provider/acp/DroidSessionTeardownGate.tsapps/server/src/provider/claudeCredentialKeepalive.tsapps/server/src/provider/claudeProcessEnv.test.tsapps/server/src/provider/cursorSkillsDiscovery.test.tsapps/server/src/provider/cursorSkillsDiscovery.tsapps/server/src/provider/opencodeRuntime.test.tsapps/server/src/provider/opencodeRuntime.tsapps/server/src/provider/providerMaintenance.test.tsapps/server/src/provider/providerMaintenance.tsapps/server/src/provider/providerMaintenanceCommandCoordinator.test.tsapps/server/src/provider/providerMaintenanceCommandCoordinator.tsapps/server/src/provider/providerMaintenanceCrossProcessLock.test.tsapps/server/src/provider/providerMaintenanceCrossProcessLock.tsapps/server/src/provider/providerMaintenanceGate.test.tsapps/server/src/provider/providerMaintenanceGate.tsapps/server/src/provider/providerMaintenanceOwnedResources.test.tsapps/server/src/provider/providerMaintenanceOwnedResources.tsapps/server/src/provider/providerUpdateOutcome.test.tsapps/server/src/provider/providerUpdateOutcome.tsapps/server/src/provider/providerUpdateQuiescence.test.tsapps/server/src/provider/providerUpdateQuiescence.tsapps/server/src/provider/runtimeLayer.tsapps/server/src/provider/skillsCatalog.test.tsapps/server/src/provider/skillsCatalog.tsapps/server/src/provider/supervisedProcessTeardown.test.tsapps/server/src/provider/supervisedProcessTeardown.tsapps/server/src/serverLayers.test.tsapps/server/src/serverLayers.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/processTreeKiller.test.tsapps/server/src/terminal/processTreeKiller.tsapps/server/tsdown.config.tsapps/web/src/components/settings/ProvidersSettingsPanel.tsxapps/web/src/providerUpdates.test.tsapps/web/src/providerUpdates.tsapps/web/src/routes/__root.tsxdocs/provider-cli-updates.mdpackages/contracts/src/server.tspackages/shared/src/commandCodeCliExecutable.test.tspackages/shared/src/commandCodeCliExecutable.tspackages/shared/src/windowsProcess.test.tspackages/shared/src/windowsProcess.tspatches/@effect%2Fplatform-node-shared@8881a9b.patchscripts/lib/dependency-security-pins.test.tsscripts/lib/workflow-contracts.test.tsscripts/lib/workflow-contracts.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Too many files changed for review. ( Bypass the limit by tagging |
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 (3)
apps/server/src/provider/Layers/ProviderService.ts (3)
1307-1319: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftProtect the provider-switch transaction before stopping the old session.
On the provider-switch path,
previousAdapter.stopSessionruns at Line [1368] before replacement startup obtains maintenance admission. If the gate is busy or latched, replacement startup fails and the restoration attempt at Lines [1381-1400] is gated and can fail again, leaving the previous runtime stopped while its binding still points to it. Acquire admission for the complete switch/rollback transaction, or provide a guaranteed restoration path before performing the destructive stop.Also applies to: 1381-1399
🤖 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/provider/Layers/ProviderService.ts` around lines 1307 - 1319, Update the provider-switch flow around ProviderService.startSession so maintenance admission is acquired before previousAdapter.stopSession and held across replacement startup and rollback. Ensure the same admission covers restoration in the failure path around the existing 1381-1399 logic, preventing a busy or latched gate from leaving the old session stopped while its binding still references it.
1463-1488: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not route maintenance failures through native-fork fallback.
The
Effect.catchis outsidewithProviderOperation, so a maintenance-busy error converted toProviderValidationErroris treated like an ordinary native fork failure and converted tonull. Move the fallback catch inside the adapter’sruneffect, or explicitly rethrow gate-derived errors, so maintenance admission failures reach the caller.🤖 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/provider/Layers/ProviderService.ts` around lines 1463 - 1488, Keep maintenance admission failures from being converted into native-fork fallback by moving the fallback catch inside the adapter.forkThread run effect, before withProviderOperation handles the operation. Alternatively, explicitly rethrow gate-derived ProviderValidationError errors while catching only native fork failures, ensuring maintenance-busy errors reach the caller.
2369-2373: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRe-arm idle teardown after temporary maintenance rejection.
When the gate is busy,
withProviderOperationfails;stopIdleRuntimeSessiononly logs that failure andrunRegisteredRuntimeIdleStopretires the generation without scheduling another timer. The idle runtime can therefore remain open indefinitely after maintenance ends. Treat temporary gate-busy failures as retryable/cancelled idle stops and preserve fail-closed behavior for latched failures.🤖 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/provider/Layers/ProviderService.ts` around lines 2369 - 2373, Update the idle-stop flow around stopIdleRuntimeSession and runRegisteredRuntimeIdleStop so temporary gate-busy failures from withProviderOperation are classified as retryable/cancelled and cause idle teardown to be re-armed after maintenance ends. Preserve the existing fail-closed behavior for latched failures, and ensure the ProviderService.stopRuntimeSessionIfIdle operation reports the distinction needed by the scheduler.
🤖 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 `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 1307-1319: Update the provider-switch flow around
ProviderService.startSession so maintenance admission is acquired before
previousAdapter.stopSession and held across replacement startup and rollback.
Ensure the same admission covers restoration in the failure path around the
existing 1381-1399 logic, preventing a busy or latched gate from leaving the old
session stopped while its binding still references it.
- Around line 1463-1488: Keep maintenance admission failures from being
converted into native-fork fallback by moving the fallback catch inside the
adapter.forkThread run effect, before withProviderOperation handles the
operation. Alternatively, explicitly rethrow gate-derived
ProviderValidationError errors while catching only native fork failures,
ensuring maintenance-busy errors reach the caller.
- Around line 2369-2373: Update the idle-stop flow around stopIdleRuntimeSession
and runRegisteredRuntimeIdleStop so temporary gate-busy failures from
withProviderOperation are classified as retryable/cancelled and cause idle
teardown to be re-armed after maintenance ends. Preserve the existing
fail-closed behavior for latched failures, and ensure the
ProviderService.stopRuntimeSessionIfIdle operation reports the distinction
needed by the scheduler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82779cc1-918f-4575-a604-1717d2295c4e
📒 Files selected for processing (4)
apps/server/src/provider/Layers/ProviderDiscoveryService.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/opencodeRuntime.test.tsapps/server/src/provider/opencodeRuntime.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/server/src/provider/opencodeRuntime.test.ts
- apps/server/src/provider/Layers/ProviderDiscoveryService.ts
- apps/server/src/provider/opencodeRuntime.ts
…-clis # Conflicts: # apps/server/scripts/cli.ts # apps/server/src/provider/Layers/CursorAdapter.test.ts # apps/server/src/provider/Layers/ProviderHealth.test.ts # apps/server/src/provider/Layers/ProviderHealth.ts # apps/server/src/provider/acp/AcpSessionRuntime.ts # apps/server/src/provider/opencodeRuntime.test.ts # apps/server/src/provider/opencodeRuntime.ts # apps/server/src/provider/supervisedProcessTeardown.test.ts # apps/server/src/provider/supervisedProcessTeardown.ts # apps/server/src/terminal/Layers/Manager.test.ts # apps/server/src/terminal/Layers/Manager.ts # apps/server/src/terminal/processTreeKiller.test.ts # apps/server/src/terminal/processTreeKiller.ts # patches/@effect%2Fplatform-node-shared@8881a9b.patch
…-clis # Conflicts: # scripts/lib/workflow-contracts.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/ci.yml (1)
92-99: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse a glob for
report_path.github/workflows/ci.yml:92-99The folded scalar turns these six paths into one space-separated string, butgha-mergify-ciexpects a glob-stylereport_path. Replace this with a glob that matches all six reports so every JUnit file is picked up.🤖 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 @.github/workflows/ci.yml around lines 92 - 99, Update the report_path configuration in the CI workflow to use a glob pattern matching all test-report.junit.xml files across the relevant apps, packages, and scripts directories, instead of listing paths as a folded space-separated scalar. Keep test_step_outcome unchanged.apps/server/native/windows-job-launcher/launcher.cpp (1)
128-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrailing CR/LF trim only strips one character.
message_lengthisconst, so the loop body nullssystem_message[message_length - 1]but the condition keeps re-checking that same (now\0) index and exits after one pass. System messages typically end with"\r\n", so only the\nis stripped and a stray\rsurvives into themessage=field — whichformatCodexCliVersionCheckFailurereads and can surface to users.🐛 Proposed fix
- if (message_length > 0 && system_message != nullptr) { - while (message_length > 0 && - (system_message[message_length - 1] == L'\r' || - system_message[message_length - 1] == L'\n')) { - system_message[message_length - 1] = L'\0'; - } - std::fwprintf(stderr, L" message=%ls", system_message); + if (message_length > 0 && system_message != nullptr) { + DWORD trimmed_length = message_length; + while (trimmed_length > 0 && + (system_message[trimmed_length - 1] == L'\r' || + system_message[trimmed_length - 1] == L'\n')) { + system_message[--trimmed_length] = L'\0'; + } + std::fwprintf(stderr, L" message=%ls", system_message);🤖 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/native/windows-job-launcher/launcher.cpp` around lines 128 - 136, Update the trailing CR/LF trimming logic in the system-message handling block to decrement the effective message length after each removed character, allowing consecutive newline characters such as "\r\n" to be stripped. Preserve the existing null termination and message output behavior in the surrounding launcher flow.
🧹 Nitpick comments (1)
apps/server/scripts/cli.ts (1)
368-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "read dist bundles + verify patched spawner" block.
This read-directory → filter
.mjs/.cjs→readFileString→Effect.try(assertPatchedEffectProcessSpawnerIsBundled)sequence is duplicated inverifyIsolatedPackageInstall(Lines 172-187). Extracting a small helper (e.g.verifyRuntimeBundlesIn(distDir)) would keep the two verification paths in sync and, as a side benefit, let the isolated-install path share the explicit empty-bundle guard (Line 371) that only exists here.As per coding guidelines: "Before adding functionality, check for shared logic that can be extracted; avoid duplicating logic across files and prefer reusable modules over local shortcuts."
🤖 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/scripts/cli.ts` around lines 368 - 387, Extract the duplicated runtime-bundle verification sequence from verifyIsolatedPackageInstall and the shown CLI flow into a shared helper such as verifyRuntimeBundlesIn(distDir). Have the helper read and filter .mjs/.cjs bundles, reject an empty bundle set with the existing CliError, load their contents, and invoke assertPatchedEffectProcessSpawnerIsBundled while preserving the current error handling; update both callers to use it.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/server/src/provider/Layers/GrokAdapter.ts`:
- Around line 2350-2354: Update the cleanup effect in the Effect.addFinalizer
callback to use Effect.ensuring around stopAll, ensuring
PubSub.shutdown(runtimeEventPubSub) and managedNativeEventLogger?.close()
execute even when stopAll fails; preserve the existing teardown operations and
ordering.
- Around line 2342-2344: Update stopAll to reuse
stopCursorSessionsBestEffort(...) so every session is attempted even when one
stopSessionInternal call fails. Replace the finalizer cleanup’s Effect.tap with
Effect.ensuring, preserving PubSub and logger shutdown regardless of teardown
failure.
In `@apps/server/src/provider/Layers/ProviderHealth.test.ts`:
- Around line 117-126: Update makeCheckClaudeProviderStatus so
TEST_PROVIDER_PROCESS_OPTIONS is spread before options, allowing caller-supplied
process overrides to take precedence and matching
makeCheckCommandCodeProviderStatus.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 319-330: Update withProviderOperation so
withProviderMaintenanceOperation receives input.run directly instead of wrapping
it with Effect.result, allowing typed provider failures to reach the maintenance
gate’s onError latch; preserve the existing toValidationError mapping for busy
errors and the declared ProviderValidationError type.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 92-99: Update the report_path configuration in the CI workflow to
use a glob pattern matching all test-report.junit.xml files across the relevant
apps, packages, and scripts directories, instead of listing paths as a folded
space-separated scalar. Keep test_step_outcome unchanged.
In `@apps/server/native/windows-job-launcher/launcher.cpp`:
- Around line 128-136: Update the trailing CR/LF trimming logic in the
system-message handling block to decrement the effective message length after
each removed character, allowing consecutive newline characters such as "\r\n"
to be stripped. Preserve the existing null termination and message output
behavior in the surrounding launcher flow.
---
Nitpick comments:
In `@apps/server/scripts/cli.ts`:
- Around line 368-387: Extract the duplicated runtime-bundle verification
sequence from verifyIsolatedPackageInstall and the shown CLI flow into a shared
helper such as verifyRuntimeBundlesIn(distDir). Have the helper read and filter
.mjs/.cjs bundles, reject an empty bundle set with the existing CliError, load
their contents, and invoke assertPatchedEffectProcessSpawnerIsBundled while
preserving the current error handling; update both callers to use it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a81ce01-08b3-460f-936f-0012821e95fb
📒 Files selected for processing (70)
.github/workflows/ci.ymlapps/server/native/windows-job-launcher/launcher.config.jsonapps/server/native/windows-job-launcher/launcher.cppapps/server/scripts/acp-conformance-agent.tsapps/server/scripts/cli.tsapps/server/scripts/cliPublishContract.test.tsapps/server/scripts/cliPublishContract.tsapps/server/src/codexAppServerManager.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/git/Layers/CodexTextGeneration.ownership.test.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/ProviderTextGeneration.tsapps/server/src/main.tsapps/server/src/provider/Layers/AntigravityAdapter.test.tsapps/server/src/provider/Layers/AntigravityAdapter.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CommandCodeAdapter.test.tsapps/server/src/provider/Layers/CommandCodeAdapter.tsapps/server/src/provider/Layers/CursorAdapter.test.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/GrokAdapter.test.tsapps/server/src/provider/Layers/GrokAdapter.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/PiAdapter.test.tsapps/server/src/provider/Layers/PiAdapter.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Layers/ProviderDiscoveryService.test.tsapps/server/src/provider/Layers/ProviderDiscoveryService.tsapps/server/src/provider/Layers/ProviderHealth.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Services/ProviderService.tsapps/server/src/provider/acp/AcpSdkConformance.test.tsapps/server/src/provider/acp/AcpSessionRuntime.test.tsapps/server/src/provider/acp/AcpSessionRuntime.tsapps/server/src/provider/acp/AcpSessionTeardown.test.tsapps/server/src/provider/acp/AcpSessionTeardown.tsapps/server/src/provider/containedClaudeSdkProcess.test.tsapps/server/src/provider/containedClaudeSdkProcess.tsapps/server/src/provider/opencodeRuntime.test.tsapps/server/src/provider/opencodeRuntime.tsapps/server/src/provider/providerMaintenance.test.tsapps/server/src/provider/providerMaintenance.tsapps/server/src/provider/providerMaintenanceCrossProcessLock.test.tsapps/server/src/provider/providerMaintenanceCrossProcessLock.tsapps/server/src/provider/providerMaintenanceGate.test.tsapps/server/src/provider/providerMaintenanceGate.tsapps/server/src/provider/providerProcessOwnerTracker.test.tsapps/server/src/provider/providerProcessOwnerTracker.tsapps/server/src/provider/runtimeLayer.test.tsapps/server/src/provider/runtimeLayer.tsapps/server/src/provider/supervisedProcessTeardown.test.tsapps/server/src/provider/supervisedProcessTeardown.tsapps/server/src/provider/windowsJobLauncherSourceContract.test.tsapps/server/src/provider/windowsJobProcessSupervisor.test.tsapps/server/src/provider/windowsJobProcessSupervisor.tsapps/server/src/provider/windowsProviderProcess.test.tsapps/server/src/provider/windowsProviderProcess.tsapps/server/src/provider/windowsProviderProcess.windows.test.tsapps/server/src/provider/windowsProviderProcessOwnershipContract.test.tsapps/server/src/serverLayers.test.tsapps/server/src/serverLayers.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/terminal/processTreeKiller.test.tsapps/server/src/terminal/processTreeKiller.tsapps/web/src/routes/__root.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/server/src/provider/Services/ProviderService.ts
- apps/server/src/provider/providerMaintenanceCrossProcessLock.test.ts
- apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts
- apps/web/src/routes/__root.tsx
- apps/server/src/git/Layers/ProviderTextGeneration.ts
- apps/server/src/provider/acp/AcpSessionTeardown.ts
- apps/server/src/main.ts
- apps/server/src/serverLayers.ts
- apps/server/src/provider/providerMaintenanceCrossProcessLock.ts
- apps/server/src/provider/Layers/ProviderDiscoveryService.ts
- apps/server/src/terminal/processTreeKiller.test.ts
- apps/server/src/provider/acp/AcpSessionRuntime.ts
- apps/server/src/provider/Layers/OpenCodeAdapter.ts
- apps/server/src/provider/supervisedProcessTeardown.ts
- apps/server/src/provider/providerMaintenance.test.ts
- apps/server/src/provider/providerMaintenance.ts
- apps/server/src/provider/opencodeRuntime.ts
- apps/server/src/provider/Layers/ProviderHealth.ts
This was referenced Jul 22, 2026
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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 `@apps/server/src/provider/providerMaintenanceCrossProcessLock.ts`:
- Around line 417-460: Make the acquisition sequence in the provider maintenance
lock flow uninterruptible, including acquireDatabaseLifecycleLock and its
verification/release handling, so cancellation cannot leave a
DatabaseLifecycleLock held without cleanup. Prefer composing the child effect
directly or wrapping the relevant section in Effect.uninterruptible while
preserving the existing rejection-release behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9eb5bd1b-81fc-471e-a5f2-02728021ca53
📒 Files selected for processing (31)
apps/server/scripts/cliPublishContract.test.tsapps/server/scripts/cliPublishContract.tsapps/server/src/codexAppServerManager.test.tsapps/server/src/provider/Layers/AntigravityAdapter.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CommandCodeAdapter.tsapps/server/src/provider/Layers/CursorAdapter.tsapps/server/src/provider/Layers/GrokAdapter.tsapps/server/src/provider/Layers/PiAdapter.test.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Layers/ProviderHealth.test.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/containedClaudeSdkProcess.test.tsapps/server/src/provider/providerMaintenance.test.tsapps/server/src/provider/providerMaintenanceCommandCoordinator.test.tsapps/server/src/provider/providerMaintenanceCrossProcessLock.test.tsapps/server/src/provider/providerMaintenanceCrossProcessLock.tsapps/server/src/provider/providerMaintenanceGate.tsapps/server/src/provider/providerProcessOwnerTracker.test.tsapps/server/src/provider/stopSessionsBestEffort.test.tsapps/server/src/provider/stopSessionsBestEffort.tsapps/server/src/provider/supervisedProcessTeardown.test.tsapps/server/src/provider/supervisedProcessTeardown.tsapps/server/src/provider/windowsJobProcessSupervisor.test.tsapps/server/src/provider/windowsJobProcessSupervisor.tsapps/server/src/terminal/Layers/Manager.test.tsapps/server/src/terminal/processTreeKiller.test.tsscripts/lib/super-synara-workflow-contract.test.tsscripts/lib/super-synara-workflow-contract.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- apps/server/src/provider/Layers/ProviderAdapterRegistry.test.ts
- apps/server/src/codexAppServerManager.test.ts
- apps/server/scripts/cliPublishContract.ts
- apps/server/scripts/cliPublishContract.test.ts
- apps/server/src/provider/providerMaintenanceCommandCoordinator.test.ts
- apps/server/src/provider/containedClaudeSdkProcess.test.ts
- apps/server/src/provider/providerMaintenance.test.ts
- apps/server/src/provider/providerProcessOwnerTracker.test.ts
- apps/server/src/provider/Layers/PiAdapter.test.ts
- apps/server/src/provider/Layers/ClaudeAdapter.test.ts
- apps/server/src/provider/Layers/CommandCodeAdapter.ts
- apps/server/src/provider/windowsJobProcessSupervisor.ts
- apps/server/src/provider/windowsJobProcessSupervisor.test.ts
- apps/server/src/terminal/Layers/Manager.test.ts
- apps/server/src/provider/Layers/ClaudeAdapter.ts
- apps/server/src/provider/providerMaintenanceGate.ts
- apps/server/src/provider/Layers/AntigravityAdapter.ts
- apps/server/src/provider/Layers/ProviderHealth.test.ts
- apps/server/src/provider/supervisedProcessTeardown.ts
- apps/server/src/terminal/processTreeKiller.test.ts
- apps/server/src/provider/Layers/CursorAdapter.ts
- apps/server/src/provider/Layers/ProviderService.ts
|
Tick the box to add this pull request to the merge queue (same as
|
This was referenced Jul 22, 2026
This was referenced Jul 23, 2026
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
Local verification
The local Visual Studio installation does not include the ARM64 C++ component, so the arm64 launcher build and packaged publish dry-run remain required exact-head Windows CI gates.
Per repository instructions, bun fmt, bun lint, and bun typecheck were not run locally without explicit user authorization; the exact-head quality check remains a merge gate.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores / Tests