Repository navigation
feat(vscode): Usability enhancements - #268
Merged
Merged
Conversation
added 4 commits
March 16, 2026 13:24
Local mode no longer uses a hardcoded port (previously defaulted to 5565).
The engine now starts with --port=0 so the OS assigns a free port, and
the extension parses the actual port from Uvicorn's startup output.
Why: Multiple VS Code windows running local engines would collide on the
same port. Dynamic allocation lets each window get its own engine process
without conflicts.
Affected: engine-manager.ts, connection.ts, config.ts
Engine installs are now placed in versioned directories
(<enginesRoot>/engines/<tag>--<hash>/) instead of a single shared folder.
A cross-process lockfile (proper-lockfile) coordinates installs so multiple
VS Code windows don't race. Channel pointer files (current-stable.json,
current-pre.json) track which version is active. PID files allow safe
cleanup of old version directories that aren't in use.
Why: The previous single-directory model meant that upgrading the engine
while another window was running it would clobber the in-use binary.
On Windows this caused EBUSY errors; on all platforms it was a race
condition between download/extract and running processes.
Also moves the engine storage root from context.extensionPath to
context.globalStorageUri.fsPath so engines persist across extension
updates instead of being re-downloaded each time.
Affected: engine-installer.ts, engine-manager.ts, extension.ts,
connection.ts, package.json (added proper-lockfile)
Removed debugger registration from package.json (activation events,
breakpoints contribution, debuggers contribution) and commented out
the registerDebugger() call in extension.ts. The adapter.ts and
session.ts files are still compiled but not wired up; adapter.ts now
contains a restoration guide in its header comment.
Why: The debugger is being reworked and was adding activation overhead
and user confusion while non-functional.
Affected: package.json, extension.ts, adapter.ts
- Moved API key input from a top-level "RocketRide account" section into
the On-prem connection mode panel (it's only relevant there).
- Cloud mode now shows a "Coming Soon" placeholder in both Settings and
Welcome pages.
- Mode-specific fields are wrapped in a bordered config box for clarity.
- Replaced the array-based engine arguments editor with a single text
input (engineArgs changed from string[] to string).
- Added a "Full debug output" checkbox (local.debugOutput) that passes
--trace=servicePython, shown instead of the manual args field when
enabled.
- Removed the local port input — port is now dynamic and not user-facing.
- Renamed "Engine Version" to "Server Version" throughout the UI.
- Hidden internal-only commands from the command palette (file ops,
status, setup credentials).
- Added two new commands: rocketride.provider.connection.focus and
rocketride.provider.files.focus.
Added an IntegrationSettings component with toggle checkboxes for
GitHub Copilot, Claude Code, Cursor, and Windsurf. Backed by new
VS Code configuration keys (rocketride.integrations.*).
Why: Preparing for AI assistant integrations; the toggles are
persisted but not yet wired to functionality.
Affected: IntegrationSettings.tsx (new), PageSettings.tsx,
PageSettingsProvider.ts, package.json
When the parent engine was started with --trace=..., child pipeline
processes now inherit that flag automatically (unless the user explicitly
set one). Uses the new startup_args() import from rocketlib.
Why: Without this, debugging child processes required manually adding
trace args to every pipeline launch configuration.
- Welcome page text colors changed from VS Code theme variables to
explicit rgba(255,255,255,...) values for consistent appearance on
the dark branded background.
- Mode description moved inside a config box with border.
- MUI InputLabel style overrides added for smaller italic labels.
- Removed padding from MuiInputBase-input.
- Placeholder text styled with italic/light/reduced opacity.
- Reconnection backoff cap reduced from 15s to 5s.
…efinements Add a "Full debug output" checkbox to Settings (local + on-prem modes) that injects --trace=servicePython into engine args. This gives users a single toggle for detailed trace logging without manually editing server arguments. The trace flag is sent both to the local engine command line and via DAP execute/launch requests, so task subprocesses receive it too. The trace flag respects user overrides: if the user types their own --trace= in the Server Arguments field, the checkbox's value is skipped to avoid duplicates — the engine's C++ argparser rejects duplicate --trace entries. On the server side, task_engine.py now inherits the parent engine's --trace setting from sys.argv when the DAP args don't include one, ensuring subprocesses always get trace config regardless of how the engine was started. User-provided args containing spaces are safely split using shlex.split() to handle quoted paths like --path='C:\Program Files'. DAP request/response logging added to ConnectionManager so all commands (execute, launch, etc.) and their full payloads are visible in the RocketRide: Extension output panel — previously these were completely silent, making debugging difficult. Other refinements: - Server Arguments field now only shown in local mode (not on-prem) - "when VS Code starts" → "when extension starts" for accuracy - Suppress Python 3.12 frozen modules debugger warning in node.py - Reduce reconnection backoff cap from 15s to 5s
Add the ability to install, update, start, stop, and remove the RocketRide engine as a native OS service directly from the Deploy page. This supplements the existing Cloud and Docker deployment options with a third "Local Service" path that runs the engine as a background service that starts automatically on boot. Key changes: - New `deploy/` module with platform-specific service managers: - Windows: uses NSSM (Non-Sucking Service Manager) - Linux: uses systemd unit files - macOS: uses launchd plist files - Common `ServiceManager` abstract base class with port-check utility - PageDeployProvider: orchestrates service lifecycle by combining EngineInstaller (download/version management) with ServiceManager (OS service registration). Tracks installed version in a config file at ProgramData/RocketRide/config.json. Adds status polling (3s interval) and a waitForServiceRunning loop with 60s timeout. - PageDeploy.tsx: new service status display (state indicator, version, install path), split-button UI with version dropdown for install and update actions, start/stop/remove controls, progress and error feedback via messages from the provider. - styles.css: service status indicators (color-coded states), split button with dropdown, progress/error message styles. - engine-installer.ts: extract `getLatestPointer()` to deduplicate the channel-fallback logic across getExecutablePath, getInstalledDir, getInstalledVersion, and getInstalledPublishedAt. - engine-manager.ts: respect configured engineVersion setting when reading version info (prerelease vs stable channel). - onprem.svg: replaced chip icon with a server/gear icon that better represents local service deployment.
…r lifecycle management The previous Docker deployment was a fire-and-forget approach that opened a terminal, ran `docker pull` and `docker create`, and gave the user no feedback or control afterward. This meant users had no way to start, stop, update, or remove the container from within VS Code — they had to drop to the CLI for everything after initial setup. This commit replaces that with a proper Docker container manager that mirrors the existing OS service (NSSM/systemd/launchd) deployment panel: - Add `DockerManager` class (`docker-manager.ts`) using the dockerode SDK to provide install, start, stop, remove, update, and status operations. Handles Windows named-pipe detection (Docker Desktop vs standalone) and maps Docker API errors to user-friendly messages. - Add `ssh2` stub (`src/stubs/ssh2.js`) and esbuild alias so that docker-modem (a dockerode dependency) doesn't pull in native .node binaries — we only use local socket connections, never SSH tunnels. - Extend `PageDeployProvider` backend with Docker lifecycle commands (dockerInstall, dockerRemove, dockerUpdate, dockerStart, dockerStop), Docker status polling, and GHCR tag fetching via the Docker Registry V2 anonymous token flow to populate the version dropdown. - Add Docker config persistence (`docker-config.json` in ProgramData / /Library/Application Support / /etc) to track which version was installed, since Docker container inspect doesn't carry that metadata. - Rework the React `PageDeploy` UI to give Docker its own status display, progress/error feedback, and split-button version selector — the same UX pattern already used by the OS service panel. Refactor shared UI elements (renderSplitButton, renderInstalledActions, renderStatusIndicator) to be reusable across both panels instead of duplicated. - Update progress bar styling to use monospace font with text-overflow ellipsis, better suited for streaming Docker pull output. - Add `dockerode` and `@types/dockerode` as dependencies; sort existing deps alphabetically in package.json.
Rod-Christensen
requested review from
asclearuc,
dsapandora and
ryan-t-christensen
March 17, 2026 07:48
Rod-Christensen
requested review from
jmaionchi and
stepmikhaylov
as code owners
March 17, 2026 07:48
github-actions
Bot
added
module:vscode
Contributor
📝 WalkthroughWalkthroughIntroduces platform service and Docker managers, versioned engine installer, dynamic local engine ports and PID tracking, switches engineArgs to a single string with effective-args injection, updates VS Code manifest/settings (local.debugOutput, integrations), rewrites Deploy/Settings UIs, and plugs Docker/SSH stubbing and build aliasing. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VSCode
participant ConfigManager
participant EngineInstaller
participant EngineManager
participant Engine
VSCode->>ConfigManager: getEffectiveEngineArgs()
ConfigManager-->>VSCode: args (may include --trace=servicePython)
VSCode->>EngineInstaller: install(versionSpec)
EngineInstaller->>EngineInstaller: acquire install.lock
EngineInstaller->>GitHub: fetch release asset
GitHub-->>EngineInstaller: asset
EngineInstaller->>Filesystem: extract to versioned dir
EngineInstaller->>EngineInstaller: update pointers, release lock
EngineInstaller-->>VSCode: executablePath
VSCode->>EngineManager: start(executablePath, port=0)
EngineManager->>Engine: launch (port=0)
Engine-->>EngineManager: ready on dynamic port
EngineManager-->>VSCode: actualPort
sequenceDiagram
actor User
participant VSCode
participant DockerManager
participant GitHubCR
participant DockerDaemon
participant Container
User->>VSCode: install docker-based runtime
VSCode->>DockerManager: install(imageTag)
DockerManager->>GitHubCR: pull image
GitHubCR-->>DockerDaemon: stream layers
DockerManager->>DockerDaemon: create & start container (bind 5565)
DockerDaemon->>Container: launch
Container-->>DockerDaemon: running
DockerManager-->>VSCode: status/progress
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/vscode/src/connection/engine-installer.ts (1)
548-559:⚠️ Potential issue | 🟠 MajorFilter the prerelease channel to actual prerelease releases.
This
find()does not checkr.prerelease, so the<Prerelease>option will usually resolve to the newest stable release whenever stable has been published more recently than the latest prerelease.🐛 Minimal fix
- const pre = data.find(r => r.tag_name.startsWith('server-') && r.assets && r.assets.length > 0); + const pre = data.find(r => r.tag_name.startsWith('server-') && r.prerelease && r.assets && r.assets.length > 0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/engine-installer.ts` around lines 548 - 559, The prerelease branch that handles versionSpec === 'prerelease' currently selects a release via data.find(...) but fails to require r.prerelease, causing stable releases to be chosen; update the predicate used when searching releases (the code around octokit.repos.listReleases and the local variable pre) to also check r.prerelease === true in addition to tag_name startingWith('server-') and assets.length > 0, then continue returning this.toReleaseInfo(pre) as before.
🧹 Nitpick comments (11)
apps/vscode/src/providers/views/PageWelcome/styles.css (1)
95-158: Consolidate repeated left-panel tint values into local CSS variables.The new colors are fine, but the same
rgba(255,255,255,...)literals now repeat across multiple selectors. Centralizing them in.welcome-leftwill simplify future theme tuning.♻️ Suggested refactor
.welcome-left { width: 280px; min-width: 280px; background: linear-gradient(180deg, `#1a1a3a` 0%, `#252545` 100%); + --welcome-left-text-subtle: rgba(255, 255, 255, 0.6); + --welcome-left-text-muted: rgba(255, 255, 255, 0.75); + --welcome-left-text-strong: rgba(255, 255, 255, 0.9); + --welcome-left-divider: rgba(255, 255, 255, 0.2); padding: 40px 30px; display: flex; flex-direction: column; align-items: center; border-right: 1px solid var(--vscode-widget-border); } @@ .welcome-brand-sub { font-size: 12px; - color: rgba(255, 255, 255, 0.6); + color: var(--welcome-left-text-subtle); @@ .welcome-tagline { @@ - color: rgba(255, 255, 255, 0.75); + color: var(--welcome-left-text-muted); @@ .welcome-features li { @@ - color: rgba(255, 255, 255, 0.9); + color: var(--welcome-left-text-strong); @@ .welcome-feature-icon { - color: rgba(255, 255, 255, 0.9); + color: var(--welcome-left-text-strong); @@ .welcome-divider { @@ - background: rgba(255, 255, 255, 0.2); + background: var(--welcome-left-divider); @@ .welcome-links a { - color: rgba(255, 255, 255, 0.75); + color: var(--welcome-left-text-muted); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageWelcome/styles.css` around lines 95 - 158, Multiple selectors (.welcome-brand-sub, .welcome-tagline, .welcome-features li, .welcome-feature-icon, .welcome-links a, .welcome-links a:hover, .welcome-divider) repeat rgba(255,255,255,...) colors; define local CSS custom properties on .welcome-left (e.g. --left-fg, --left-fg-strong, --left-fg-muted, --left-divider) and replace the repeated rgba literals in those classes with the new variables so future theme tweaks only change .welcome-left's variable values.packages/shared-ui/src/theme.ts (1)
327-338: Scope and normalizeMuiInputLabelVSCode styling to avoid low-readability defaults.This override affects all input labels in VSCode and forces a very low-emphasis style (italic + light + low opacity). Consider keeping global labels readable and size them from
baseFontSize, then apply compact/italic styling only where explicitly needed.Suggested adjustment
MuiInputLabel: { styleOverrides: { root: inVSCode ? { - fontSize: '0.75rem', - fontStyle: 'italic', - fontWeight: 300, - opacity: 0.7, + fontSize: `${Math.max(baseFontSize - 1, 11)}px`, + fontWeight: 400, } : {}, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-ui/src/theme.ts` around lines 327 - 338, The current MuiInputLabel override applies an italic, light-weight, low-opacity style globally when inVSCode, reducing readability; change the global override in the MuiInputLabel -> styleOverrides -> root block to preserve readable defaults (use fontSize derived from baseFontSize, no forced italic or reduced opacity), and move the compact/italic/light/low-opacity presentation into a scoped alternative (e.g., a new CSS class or theme variant such as "compactInputLabel" or a component override for specific components) so only labelled compact inputs receive fontStyle: 'italic', fontWeight: 300 and opacity: 0.7; reference the inVSCode flag to adjust only sizing (e.g., fontSize: `${baseFontSize * 0.75}rem`) while removing global fontStyle/fontWeight/opacity changes from MuiInputLabel.root.packages/ai/src/ai/modules/task/task_engine.py (1)
46-46: Unused import detected.Static analysis indicates
debugis imported but not used in this file. If it's no longer needed after recent changes, consider removing it.♻️ Proposed fix
-from rocketlib import debug, args as startup_args +from rocketlib import args as startup_args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai/src/ai/modules/task/task_engine.py` at line 46, The import list includes an unused symbol `debug` from `rocketlib` in the statement that also imports `args as startup_args`; remove `debug` from that import (leaving `args as startup_args`) to eliminate the unused import, or if `debug` was intended to be used, add the necessary usage in this module (e.g., replace existing logging calls to use `debug`)—locate the import line with `from rocketlib import debug, args as startup_args` and update it accordingly.apps/vscode/src/providers/views/PageSettings/ConnectionSettings.tsx (1)
228-241: Consider semantic grouping for version dropdown.The disabled separator option (
────────────────) may cause accessibility issues as screen readers could announce it as a selectable option. Using<optgroup>provides proper semantic grouping.♻️ Accessible alternative using optgroup
<select id="serverVersion" value={settings.localEngineVersion} onChange={handleVersionChange} disabled={engineVersionsLoading} > - <option value="latest"><Latest></option> - <option value="prerelease"><Prerelease></option> - {engineVersions.length > 0 && ( - <option disabled>{'────────────────'}</option> - )} + <optgroup label="Quick Select"> + <option value="latest"><Latest></option> + <option value="prerelease"><Prerelease></option> + </optgroup> {engineVersionsLoading && ( <option disabled>Loading versions...</option> )} - {engineVersions.map(v => ( - <option key={v.tag_name} value={v.tag_name}> - {displayVersion(v.tag_name)} - </option> - ))} + {engineVersions.length > 0 && ( + <optgroup label="Specific Versions"> + {engineVersions.map(v => ( + <option key={v.tag_name} value={v.tag_name}> + {displayVersion(v.tag_name)} + </option> + ))} + </optgroup> + )} </select>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageSettings/ConnectionSettings.tsx` around lines 228 - 241, Replace the visual disabled separator option with semantic optgroup(s) for the versions dropdown: wrap the static options ("latest", "prerelease") in one optgroup (e.g., label "Recommended") and the dynamic engineVersions map in a separate optgroup (e.g., label "All versions"), keeping the existing option elements created by engineVersions.map(v => <option key={v.tag_name} value={v.tag_name}>{displayVersion(v.tag_name)}</option>), and move the loading state option into the dynamic optgroup (or replace it with a disabled optgroup label) so screen readers get proper grouping instead of a disabled pseudo-separator.apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx (1)
32-53: Consider narrowing the type for better type safety.The
keytype iskeyof SettingsDatawhich includes non-boolean fields. While the current implementation works becauseINTEGRATIONSonly contains boolean keys, the type could be more precise.♻️ Type-safe alternative
-const INTEGRATIONS: { key: keyof SettingsData; label: string; description: string }[] = [ +type IntegrationKey = 'integrationCopilot' | 'integrationClaudeCode' | 'integrationCursor' | 'integrationWindsurf'; + +const INTEGRATIONS: { key: IntegrationKey; label: string; description: string }[] = [This ensures only boolean integration keys can be used and provides compile-time safety if
SettingsDatachanges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx` around lines 32 - 53, INTEGRATIONS uses key: keyof SettingsData which may include non-boolean fields; tighten the type by declaring a helper mapped type (e.g., BooleanKeys<T> = { [K in keyof T]: T[K] extends boolean ? K : never }[keyof T]) and change INTEGRATIONS to use key: BooleanKeys<SettingsData> so only boolean integration keys (like integrationCopilot, integrationClaudeCode, integrationCursor, integrationWindsurf) are allowed; update any related references expecting keyof SettingsData to accept the narrowed BooleanKeys<SettingsData> where needed.apps/vscode/src/deploy/service-manager.ts (1)
22-22: Remove unused import.The
iconsimport is not used in this file. The static analysis tool correctly flagged this.🧹 Proposed fix
import * as net from 'net'; import { getLogger } from '../shared/util/output'; -import { icons } from '../shared/util/icons';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/deploy/service-manager.ts` at line 22, The file imports the unused symbol `icons` from '../shared/util/icons'; remove that unused import to clean up the code and satisfy static analysis. Locate the import statement `import { icons } from '../shared/util/icons';` in service-manager.ts and delete it (or remove `icons` from the import list if multiple symbols are imported), then run lint/typecheck to confirm the warning is resolved.apps/vscode/src/deploy/service-windows.ts (1)
248-271: Consider adding redirect depth limit.The
downloadFilemethod recursively follows HTTP redirects without a depth limit. While rare, a malicious or misconfigured server could cause a stack overflow with redirect loops.🛡️ Proposed fix
- private downloadFile(url: string, destPath: string): Promise<void> { + private downloadFile(url: string, destPath: string, maxRedirects: number = 5): Promise<void> { return new Promise((resolve, reject) => { const protocol = url.startsWith('https') ? https : http; const req = protocol.get(url, { headers: { 'User-Agent': 'RocketRide-VSCode' } }, (response) => { if (response.statusCode && response.statusCode >= 300 && response.statusCode < 400 && response.headers.location) { + if (maxRedirects <= 0) { + response.destroy(); + reject(new Error('Too many redirects')); + return; + } response.destroy(); - this.downloadFile(response.headers.location, destPath).then(resolve, reject); + this.downloadFile(response.headers.location, destPath, maxRedirects - 1).then(resolve, reject); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/deploy/service-windows.ts` around lines 248 - 271, The downloadFile function follows redirects recursively with no depth limit; modify downloadFile(url: string, destPath: string, maxRedirects = 10) to track remaining redirects and decrement on recursive calls, and when response.headers.location is present call this.downloadFile(newLocation, destPath, maxRedirects - 1) and reject with an explicit error if maxRedirects <= 0; ensure the existing request/response cleanup (response.destroy(), file.close()) and req.on('error') behavior remain unchanged.apps/vscode/src/providers/PageDeployProvider.ts (2)
365-365: ReplaceFunctiontype with explicit signature.Using
Functionloses type safety. Define the expected method signatures explicitly.♻️ Proposed fix
- https.get(url, options, (res: { statusCode?: number; on: Function; setEncoding: Function }) => { + https.get(url, options, (res: { statusCode?: number; on: (event: string, listener: (data?: string) => void) => void; setEncoding: (encoding: BufferEncoding) => void }) => {Alternatively, import and use the
IncomingMessagetype from Node'shttpmodule for full type safety:+import type { IncomingMessage } from 'http'; ... - https.get(url, options, (res: { statusCode?: number; on: Function; setEncoding: Function }) => { + https.get(url, options, (res: IncomingMessage) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/PageDeployProvider.ts` at line 365, The inline callback in PageDeployProvider.ts currently types the response parameter as { statusCode?: number; on: Function; setEncoding: Function }, which uses the unsafe Function type; change it to a proper shape or use Node's IncomingMessage type from 'http' (or import IncomingMessage from 'http' and type the callback param as res: IncomingMessage) and replace on: Function and setEncoding: Function with their actual signatures (e.g., on(event: string, listener: (...args: any[]) => void): this and setEncoding(encoding?: string): void) so the https.get callback has explicit, type-safe method signatures.
558-561: Remove unusedgetApiKeymethod.This method is defined but never called anywhere in the file.
🧹 Proposed fix
- private async getApiKey(): Promise<string> { - const config = ConfigManager.getInstance().getConfig(); - return config.apiKey || ''; - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/PageDeployProvider.ts` around lines 558 - 561, The private method getApiKey in PageDeployProvider is unused and should be removed to clean up dead code; delete the entire getApiKey() { const config = ConfigManager.getInstance().getConfig(); return config.apiKey || ''; } method from the PageDeployProvider class (ensure no references remain to getApiKey elsewhere and run a quick project-wide search for getApiKey to confirm).apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx (2)
13-25: Type duplication from backend files.
ServiceStatusandDockerStatusinterfaces duplicate types fromservice-manager.tsanddocker-manager.ts. This is acceptable for webview isolation (webview code runs in a separate context), but consider adding a comment noting the intentional duplication to prevent future drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx` around lines 13 - 25, The ServiceStatus and DockerStatus interfaces in PageDeploy.tsx intentionally duplicate types defined in service-manager.ts and docker-manager.ts for webview isolation; add a short clear comment above both interface declarations stating that this duplication is deliberate (webview runs in a separate context) and should be kept in sync with the backend types to prevent accidental drift, referencing the backend files by name (service-manager.ts, docker-manager.ts) to guide future maintainers.
97-100: Remove or useversionsLoadingstate.The
versionsLoadingstate is set but never read. If you intended to show a loading indicator while fetching versions, you should add the UI for it; otherwise, remove the unused state.🧹 Option 1: Remove if not needed
// Service version state const [versions, setVersions] = useState<VersionItem[]>([]); - const [versionsLoading, setVersionsLoading] = useState(false); const [selectedVersion, setSelectedVersion] = useState('latest');And remove the setter calls at lines 165 and 154.
🔧 Option 2: Use for loading indicator
Add a loading indicator in the version dropdown or split button when
versionsLoadingis true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx` around lines 97 - 100, The versionsLoading state is declared but never read; either remove it and all calls to its setter (remove the useState hook for versionsLoading and any calls to setVersionsLoading) or keep it and wire it into the UI by showing a loading indicator in the version selector (e.g., disable the version dropdown/split button and show a spinner when versionsLoading is true). Specifically update the PageDeploy component: delete the const [versionsLoading, setVersionsLoading] = useState(false) and remove setVersionsLoading(...) calls if opting for removal, or if opting to use it, read versionsLoading where the version dropdown or split button is rendered (referencing the versions, selectedVersion, and setSelectedVersion state) to conditionally render a spinner/disabled state while versions are loading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/config.ts`:
- Around line 465-483: getEffectiveEngineArgs currently pushes the entire
engineArgs string as one element, causing spawn() to receive a single combined
arg; update getEffectiveEngineArgs to split argsStr into individual tokens
before adding to result (e.g. const argsArray =
argsStr.trim().split(/\s+/).filter(Boolean); result.push(...argsArray);), keep
the existing hasTrace check and still push '--trace=servicePython' as a separate
element when needed, and ensure the function returns an array of separate
argument strings for EngineManager.start() to spread into spawn().
In `@apps/vscode/src/connection/engine-installer.ts`:
- Around line 350-387: The cleanupOldVersions routine currently runs without the
installer lock and can remove dirs another process is extracting or that are
still pointed-to by channel pointers; update callers to invoke
cleanupOldVersions while holding the same installer lock used by the installer
(i.e., ensure engine-manager calls cleanupOldVersions under the existing
install/lock mechanism) and modify cleanupOldVersions to first load and preserve
pointer targets from current-stable.json and current-pre.json (skip deleting any
directory referenced there) before removing others; retain the existing
hasLiveProcesses(dirPath) check and error handling but ensure the overall method
is executed while holding the installer lock so deletions are cross-process
safe.
In `@apps/vscode/src/connection/engine-manager.ts`:
- Around line 206-214: The PID file is stored in mutable instance state
(this.pidFilePath) and removed by multiple async callbacks (stopProcess timeout
and child 'exit'), which can race and delete a PID file belonging to a newly
started child; fix by scoping the PID file to the specific child that created
it—capture the child.pid (or attach a pidFilePath property to the ChildProcess)
when creating the file in the block that writes it and update
removePidFile()/exit/timeout handlers to only remove the file if the PID (or
pidFilePath) matches that captured value (or the same child instance), leaving
this.pidFilePath as a derived/current-only property; update references in
removePidFile, stopProcess, and the child 'exit' handler (symbols:
this.pidFilePath, removePidFile(), stopProcess(), this.child, the child's 'exit'
listener) to perform the check before unlinking.
- Around line 244-255: tryResolveReady currently marks the server ready even
when portRegex doesn't match, leaving this.actualPort undefined; change
tryResolveReady so it only sets this.actualPort, processReady, this.started,
logs readiness, and calls resolve() after a successful regex match (i.e., when
match is truthy); leave other branches untouched (do not mark ready or resolve
when match is falsy) and reference tryResolveReady, portRegex, this.actualPort,
processReady, processErrored, this.started, logger.output, and resolve() when
making the change.
- Around line 144-151: The ConfigManager.getEffectiveEngineArgs() currently
returns the whole engineArgs string as one element; update
getEffectiveEngineArgs() (in apps/vscode/src/config.ts) to tokenize the
engineArgs string into individual arguments before returning (e.g., split on
whitespace while preserving quoted groups or use a simple shell-like parse) so
callers like engine-manager.ts (uses effectiveArgs spread),
PageStatusProvider.ts and session.ts receive separate flag tokens (e.g.,
"--foo=1" and "--bar=2") instead of a single combined token.
In `@apps/vscode/src/deploy/docker-manager.ts`:
- Around line 200-206: The update() flow currently calls this.remove(true)
before await this.pullImage(fullImage,...), which risks deleting the running
deployment if the image pull fails; change update() to call await
this.pullImage(fullImage, onProgress) first (and authenticate as needed), then
create/start the new container (swap in the new image), verify it is
running/healthy, and only after successful start call this.remove(true) to
remove the old container/image; reference the update(), pullImage(), and
remove(true) calls to locate and reorder these steps and add basic health/start
verification before cleanup.
- Around line 121-127: The PortBindings currently expose `${CONTAINER_PORT}/tcp`
on all interfaces; update the container creation in docker-manager.ts so each
PortBindings entry for `${CONTAINER_PORT}/tcp` includes HostIp: '127.0.0.1'
(alongside HostPort: String(CONTAINER_PORT)) to bind the published port to
loopback. Locate the block using ExposedPorts, HostConfig and PortBindings
(references: CONTAINER_PORT, ExposedPorts, HostConfig, PortBindings) and apply
the same change to the second createContainer call elsewhere in the file (the
other occurrence around the lines noted).
In `@apps/vscode/src/deploy/service-linux.ts`:
- Around line 69-82: The update() method currently writes the temporary unit
file into INSTALL_ROOT which may be unwritable; change it to create the temp
file in the OS temp directory (use os.tmpdir()) instead of INSTALL_ROOT—build
the unitContent via buildUnitFile(executablePath, engineDir), write the tmp file
to path.join(os.tmpdir(), 'rocketride.service.tmp'), then use runSudo('cp',
[tmpUnit, UNIT_PATH]) and fs.unlinkSync(tmpUnit) to copy and clean up; ensure
the same cleanup behavior and systemctl calls (daemon-reload/start) remain
unchanged.
- Around line 36-50: The install method can fail creating /opt/rocketride and
writing the temp unit due to lack of permissions; replace the direct
fs.mkdirSync(INSTALL_ROOT, ...) with an elevated directory creation (e.g., await
this.runSudo('mkdir', ['-p', INSTALL_ROOT]) or await this.runSudo('install',
['-d', INSTALL_ROOT])) before touching files, and write the temporary unit file
to a user-writable temp directory (os.tmpdir()) as tmpUnit instead of under
INSTALL_ROOT so fs.writeFileSync(tmpUnit, ...) won’t require root; keep the sudo
cp to UNIT_PATH and the subsequent runSudo calls for systemctl (UNIT_NAME)
as-is.
In `@apps/vscode/src/deploy/service-mac.ts`:
- Around line 66-78: The update method currently writes the temporary plist to
INSTALL_ROOT which can trigger permission issues; change the temp file creation
in update() to use the system temp directory (os.tmpdir()) instead of
INSTALL_ROOT (e.g., build tmpPlist with path.join(os.tmpdir(),
'rocketride.plist.tmp')), keep the rest of the flow (writeFileSync, runSudo cp
to PLIST_PATH, unlinkSync) the same, and ensure you still call
buildPlist(executablePath, engineDir) to generate contents before copying.
- Around line 139-150: The runSudo function builds the AppleScript command by
naively wrapping args with single quotes which fails for arguments that contain
single quotes; fix it by adding a proper escaping step used when constructing
the script string (the script variable in runSudo) — implement a small escape
function (used inside runSudo) that replaces each single quote in an argument
with the safe shell-quoted sequence (so the argument can be safely wrapped in
single quotes), then use that escaped value instead of `'${a}'` when joining
args; keep the rest of runSudo (execFile('osascript', …)) unchanged.
- Around line 36-48: The install method currently creates LOGS_DIR and writes a
temporary plist under INSTALL_ROOT without elevation, which will fail on
/Library paths; change it to create the logs directory with elevated privileges
(call this.runSudo('mkdir', ['-p', LOGS_DIR']) or equivalent) and avoid writing
the temp plist directly to INSTALL_ROOT — instead write the tmpPlist to a
writable location like os.tmpdir() (use a variable tmpPlist constructed from
os.tmpdir()) and then use this.runSudo('cp', [tmpPlist, PLIST_PATH]) to move it
into place; keep using buildPlist to generate the content and remove the tmp
file after copying, and optionally set ownership/permissions on PLIST_PATH via
runSudo if needed.
In `@apps/vscode/src/providers/PageDeployProvider.ts`:
- Around line 385-387: CONFIG_PATH is Windows-only; update the static readonly
CONFIG_PATH in PageDeployProvider so it picks a platform-appropriate default
instead of falling back to 'C:\\ProgramData'. Replace the current single join
with a conditional using process.platform (e.g., if process.platform === 'win32'
use process.env.PROGRAMDATA || 'C:\\ProgramData', else use a Unix/macOS-friendly
path such as process.env.XDG_CONFIG_HOME || path.join(os.homedir(), '.config')
and then path.join(..., 'RocketRide', 'config.json')); mirror the approach used
by DOCKER_CONFIG_PATH (same file) to ensure consistent cross-platform behavior
and keep the identifier CONFIG_PATH unchanged. Ensure path and os (if used) are
imported and the value remains a static readonly field.
In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx`:
- Around line 211-218: The inline ref callback on the "split-button" div is
adding a document click listener without a proper cleanup (causing a leak);
replace that pattern by creating a stable ref (e.g., splitButtonRef) and move
the add/remove logic into a useEffect which registers a single handler that uses
splitButtonRef.current and calls setDropdownOpen(null) when clicks occur outside
the element, and ensure the effect returns a cleanup that removes the listener;
update the div to use ref={splitButtonRef} and keep the handler and
setDropdownOpen references stable or include them in the effect dependencies as
appropriate.
In `@apps/vscode/src/providers/views/PageSettings/PageSettings.tsx`:
- Around line 135-141: SettingsData declares localEngineArgs as a string but the
PageSettings initial state sets it to an empty array; change the initial value
of localEngineArgs in the PageSettings component state from [] to "" so the
initial state type matches the SettingsData interface (look for the SettingsData
interface and the localEngineArgs entry in the initial state object in
PageSettings.tsx).
In `@apps/vscode/src/stubs/ssh2.js`:
- Around line 1-5: Add the Node ESLint pragma to the top of the ssh2 stub so
ESLint knows Node globals are allowed: insert the comment /* eslint-env node */
as the first line of the file that defines module.exports = { Client: function()
{} }; to avoid no-undef errors for module and other Node globals while keeping
the existing stub implementation intact.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 1454-1458: The loop that expands user_args into child_args uses
shlex.split(arg) but does not guard against ValueError from malformed quotes,
which can crash task startup; update the loop in task_engine.py (the section
handling user_args -> child_args) to wrap shlex.split(arg) in a try/except for
ValueError, log or surface a clear error message (including the offending arg
and the exception) and fall back to a safe behavior (e.g., append the original
arg or reject the task with a clear error) so task startup won't fail with an
unclear traceback.
In `@packages/ai/src/ai/node.py`:
- Around line 103-104: The current except block silently swallows all exceptions
during debugger initialization (the except Exception as e: pass), which hides
real errors; update the exception handling in the debugger initialization code
to log the caught exception using the imported debug helper (e.g., debug.error
or debug.exception) and/or restrict the except to expected exceptions only
(e.g., OSError, PermissionError) so unexpected errors still surface; locate the
try/except around the debugger start in packages/ai/src/ai/node.py and replace
the pass with a logged message that includes the exception object (e) and
context.
---
Outside diff comments:
In `@apps/vscode/src/connection/engine-installer.ts`:
- Around line 548-559: The prerelease branch that handles versionSpec ===
'prerelease' currently selects a release via data.find(...) but fails to require
r.prerelease, causing stable releases to be chosen; update the predicate used
when searching releases (the code around octokit.repos.listReleases and the
local variable pre) to also check r.prerelease === true in addition to tag_name
startingWith('server-') and assets.length > 0, then continue returning
this.toReleaseInfo(pre) as before.
---
Nitpick comments:
In `@apps/vscode/src/deploy/service-manager.ts`:
- Line 22: The file imports the unused symbol `icons` from
'../shared/util/icons'; remove that unused import to clean up the code and
satisfy static analysis. Locate the import statement `import { icons } from
'../shared/util/icons';` in service-manager.ts and delete it (or remove `icons`
from the import list if multiple symbols are imported), then run lint/typecheck
to confirm the warning is resolved.
In `@apps/vscode/src/deploy/service-windows.ts`:
- Around line 248-271: The downloadFile function follows redirects recursively
with no depth limit; modify downloadFile(url: string, destPath: string,
maxRedirects = 10) to track remaining redirects and decrement on recursive
calls, and when response.headers.location is present call
this.downloadFile(newLocation, destPath, maxRedirects - 1) and reject with an
explicit error if maxRedirects <= 0; ensure the existing request/response
cleanup (response.destroy(), file.close()) and req.on('error') behavior remain
unchanged.
In `@apps/vscode/src/providers/PageDeployProvider.ts`:
- Line 365: The inline callback in PageDeployProvider.ts currently types the
response parameter as { statusCode?: number; on: Function; setEncoding: Function
}, which uses the unsafe Function type; change it to a proper shape or use
Node's IncomingMessage type from 'http' (or import IncomingMessage from 'http'
and type the callback param as res: IncomingMessage) and replace on: Function
and setEncoding: Function with their actual signatures (e.g., on(event: string,
listener: (...args: any[]) => void): this and setEncoding(encoding?: string):
void) so the https.get callback has explicit, type-safe method signatures.
- Around line 558-561: The private method getApiKey in PageDeployProvider is
unused and should be removed to clean up dead code; delete the entire
getApiKey() { const config = ConfigManager.getInstance().getConfig(); return
config.apiKey || ''; } method from the PageDeployProvider class (ensure no
references remain to getApiKey elsewhere and run a quick project-wide search for
getApiKey to confirm).
In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx`:
- Around line 13-25: The ServiceStatus and DockerStatus interfaces in
PageDeploy.tsx intentionally duplicate types defined in service-manager.ts and
docker-manager.ts for webview isolation; add a short clear comment above both
interface declarations stating that this duplication is deliberate (webview runs
in a separate context) and should be kept in sync with the backend types to
prevent accidental drift, referencing the backend files by name
(service-manager.ts, docker-manager.ts) to guide future maintainers.
- Around line 97-100: The versionsLoading state is declared but never read;
either remove it and all calls to its setter (remove the useState hook for
versionsLoading and any calls to setVersionsLoading) or keep it and wire it into
the UI by showing a loading indicator in the version selector (e.g., disable the
version dropdown/split button and show a spinner when versionsLoading is true).
Specifically update the PageDeploy component: delete the const [versionsLoading,
setVersionsLoading] = useState(false) and remove setVersionsLoading(...) calls
if opting for removal, or if opting to use it, read versionsLoading where the
version dropdown or split button is rendered (referencing the versions,
selectedVersion, and setSelectedVersion state) to conditionally render a
spinner/disabled state while versions are loading.
In `@apps/vscode/src/providers/views/PageSettings/ConnectionSettings.tsx`:
- Around line 228-241: Replace the visual disabled separator option with
semantic optgroup(s) for the versions dropdown: wrap the static options
("latest", "prerelease") in one optgroup (e.g., label "Recommended") and the
dynamic engineVersions map in a separate optgroup (e.g., label "All versions"),
keeping the existing option elements created by engineVersions.map(v => <option
key={v.tag_name} value={v.tag_name}>{displayVersion(v.tag_name)}</option>), and
move the loading state option into the dynamic optgroup (or replace it with a
disabled optgroup label) so screen readers get proper grouping instead of a
disabled pseudo-separator.
In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx`:
- Around line 32-53: INTEGRATIONS uses key: keyof SettingsData which may include
non-boolean fields; tighten the type by declaring a helper mapped type (e.g.,
BooleanKeys<T> = { [K in keyof T]: T[K] extends boolean ? K : never }[keyof T])
and change INTEGRATIONS to use key: BooleanKeys<SettingsData> so only boolean
integration keys (like integrationCopilot, integrationClaudeCode,
integrationCursor, integrationWindsurf) are allowed; update any related
references expecting keyof SettingsData to accept the narrowed
BooleanKeys<SettingsData> where needed.
In `@apps/vscode/src/providers/views/PageWelcome/styles.css`:
- Around line 95-158: Multiple selectors (.welcome-brand-sub, .welcome-tagline,
.welcome-features li, .welcome-feature-icon, .welcome-links a, .welcome-links
a:hover, .welcome-divider) repeat rgba(255,255,255,...) colors; define local CSS
custom properties on .welcome-left (e.g. --left-fg, --left-fg-strong,
--left-fg-muted, --left-divider) and replace the repeated rgba literals in those
classes with the new variables so future theme tweaks only change
.welcome-left's variable values.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Line 46: The import list includes an unused symbol `debug` from `rocketlib` in
the statement that also imports `args as startup_args`; remove `debug` from that
import (leaving `args as startup_args`) to eliminate the unused import, or if
`debug` was intended to be used, add the necessary usage in this module (e.g.,
replace existing logging calls to use `debug`)—locate the import line with `from
rocketlib import debug, args as startup_args` and update it accordingly.
In `@packages/shared-ui/src/theme.ts`:
- Around line 327-338: The current MuiInputLabel override applies an italic,
light-weight, low-opacity style globally when inVSCode, reducing readability;
change the global override in the MuiInputLabel -> styleOverrides -> root block
to preserve readable defaults (use fontSize derived from baseFontSize, no forced
italic or reduced opacity), and move the compact/italic/light/low-opacity
presentation into a scoped alternative (e.g., a new CSS class or theme variant
such as "compactInputLabel" or a component override for specific components) so
only labelled compact inputs receive fontStyle: 'italic', fontWeight: 300 and
opacity: 0.7; reference the inVSCode flag to adjust only sizing (e.g., fontSize:
`${baseFontSize * 0.75}rem`) while removing global fontStyle/fontWeight/opacity
changes from MuiInputLabel.root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b451f8c7-cd2b-4095-b409-78e3017f1488
⛔ Files ignored due to path filters (2)
apps/vscode/onprem.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.vscode/launch.jsonapps/vscode/esbuild.jsapps/vscode/package.jsonapps/vscode/src/config.tsapps/vscode/src/connection/connection.tsapps/vscode/src/connection/engine-installer.tsapps/vscode/src/connection/engine-manager.tsapps/vscode/src/debugger/adapter.tsapps/vscode/src/debugger/session.tsapps/vscode/src/deploy/docker-manager.tsapps/vscode/src/deploy/service-linux.tsapps/vscode/src/deploy/service-mac.tsapps/vscode/src/deploy/service-manager.tsapps/vscode/src/deploy/service-windows.tsapps/vscode/src/extension.tsapps/vscode/src/providers/PageDeployProvider.tsapps/vscode/src/providers/PageSettingsProvider.tsapps/vscode/src/providers/PageStatusProvider.tsapps/vscode/src/providers/SidebarFilesProvider.tsapps/vscode/src/providers/styles/vscode.cssapps/vscode/src/providers/views/PageDeploy/PageDeploy.tsxapps/vscode/src/providers/views/PageDeploy/styles.cssapps/vscode/src/providers/views/PageSettings/ConnectionSettings.tsxapps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsxapps/vscode/src/providers/views/PageSettings/PageSettings.tsxapps/vscode/src/providers/views/PageSettings/styles.cssapps/vscode/src/providers/views/PageWelcome/PageWelcome.tsxapps/vscode/src/providers/views/PageWelcome/styles.cssapps/vscode/src/stubs/ssh2.jspackage.jsonpackages/ai/src/ai/modules/task/task_engine.pypackages/ai/src/ai/node.pypackages/shared-ui/src/theme.ts
stepmikhaylov
marked this pull request as draft
March 17, 2026 09:49
stepmikhaylov
marked this pull request as draft
March 17, 2026 09:49
asclearuc
reviewed
Mar 17, 2026
…t, function or class Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…t, function or class Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Rod-Christensen
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/vscode/src/providers/views/PageSettings/PageSettings.tsx (1)
127-142:⚠️ Potential issue | 🔴 CriticalInitial
SettingsDatastate is missing requiredlocalDebugOutput.
SettingsDatarequireslocalDebugOutput, but the initializer omits it. This breaks type correctness and can surface as undefined UI state before settings load.💡 Proposed fix
const [settings, setSettings] = useState<SettingsData>({ hostUrl: 'http://localhost:5565', connectionMode: 'local', hasApiKey: false, apiKey: '', // Initialize empty - will be loaded from secure storage autoConnect: true, defaultPipelinePath: 'pipelines', // Initialize with default value localEngineVersion: 'latest', localEngineArgs: '', + localDebugOutput: false, pipelineRestartBehavior: 'prompt', envVars: {}, integrationCopilot: false, integrationClaudeCode: false, integrationCursor: false, integrationWindsurf: false });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageSettings/PageSettings.tsx` around lines 127 - 142, The initial useState initializer for the settings state (const [settings, setSettings] = useState<SettingsData>(...)) is missing the required localDebugOutput property from the SettingsData type; update the initializer object used in PageSettings to include a sensible default for localDebugOutput (e.g., false or the appropriate default type/value) so the settings state is fully typed and UI doesn't see undefined before loaded.apps/vscode/src/connection/engine-manager.ts (2)
198-248:⚠️ Potential issue | 🔴 CriticalGuard child-event handlers so stale process callbacks cannot clobber the active engine.
Current callbacks use shared instance state without verifying event ownership. If an older child emits
error/exitafter a new child starts, it can clearthis.child, flipstarted, or kill the new process viacleanupProcess().🐛 Minimal fix
- this.child = spawn(executablePath, args, { + const child = spawn(executablePath, args, { cwd: path.dirname(executablePath), stdio: 'pipe', }); + this.child = child; @@ - const myPidFile = this.child.pid - ? path.join(path.dirname(executablePath), `engine-${this.child.pid}.pid`) + const myPidFile = child.pid + ? path.join(path.dirname(executablePath), `engine-${child.pid}.pid`) : undefined; @@ - fs.writeFileSync(myPidFile, String(this.child.pid)); + fs.writeFileSync(myPidFile, String(child.pid)); } catch { @@ - this.child.on('error', (err) => { + child.on('error', (err) => { + if (this.child !== child) return; if (!processReady && !processErrored) { processErrored = true; this.logger.output(`${icons.error} DAP server failed to launch: ${err.message}`); this.cleanupProcess(myPidFile); reject(err); } }); - this.child.on('exit', (code, signal) => { + child.on('exit', (code, signal) => { // Clean up PID file on exit — only if it's still ours this.removePidFile(myPidFile); + if (this.child !== child) return; if (!processReady && !processErrored) { processErrored = true; this.logger.output(`${icons.error} DAP server exited during startup (code=${code}, signal=${signal})`); this.cleanupProcess(); reject(new Error(`Process exited during startup: code=${code}, signal=${signal}`)); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/engine-manager.ts` around lines 198 - 248, The child process event handlers currently act on shared instance state and can be triggered by stale children; fix by capturing the spawned child and its pid-file into local constants (e.g. const launched = this.child and const myPidFile as already set) and at the top of each handler (the functions attached to child.on('error') and child.on('exit')) verify the event belongs to that launched instance (if (this.child !== launched) return) before performing any cleanup, writing logs, flipping started, clearing this.child, calling cleanupProcess()/removePidFile(), or emitting 'terminated' so stale callbacks cannot clobber a new engine.
266-291:⚠️ Potential issue | 🟠 MajorMake readiness parsing line-buffered across stream chunks.
stdout/stderrchunks are arbitrary; parsing each chunk independently can split theUvicorn running...line and never resolve startup.🔧 Suggested hardening
+ let stdoutBuffer = ''; + let stderrBuffer = ''; + + const processOutputChunk = (chunk: string, stream: 'stdout' | 'stderr'): void => { + let buffer = (stream === 'stdout' ? stdoutBuffer : stderrBuffer) + chunk; + const lines = buffer.split(/\r?\n/); + buffer = lines.pop() ?? ''; + + for (const raw of lines) { + const msg = raw.trim(); + if (!msg) continue; + this.logger.console(msg); + if (msg.includes('Uvicorn running')) { + tryResolveReady(msg); + } + } + + if (stream === 'stdout') stdoutBuffer = buffer; + else stderrBuffer = buffer; + }; + // Monitor stdout for readiness this.child.stdout?.on('data', (data) => { - const output = data.toString(); - for (const message of output.split('\n')) { - const msg = message.trim(); - if (msg) { - this.logger.console(msg); - if (msg.includes('Uvicorn running')) { - tryResolveReady(msg); - } - } - } + processOutputChunk(data.toString(), 'stdout'); }); // Monitor stderr this.child.stderr?.on('data', (data) => { - const output = data.toString(); - for (const message of output.split('\n')) { - const msg = message.trim(); - if (msg) { - this.logger.console(msg); - if (msg.includes('Uvicorn running')) { - tryResolveReady(msg); - } - } - } + processOutputChunk(data.toString(), 'stderr'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/engine-manager.ts` around lines 266 - 291, The stdout/stderr handlers currently parse each incoming chunk independently which can split the "Uvicorn running" line across chunks; change the handlers on this.child.stdout?.on('data', ...) and this.child.stderr?.on('data', ...) to use a line-buffered approach: keep a per-stream accumulator string, append each chunk, split on '\n' and process only the complete lines (trim and call this.logger.console and tryResolveReady when a line includes 'Uvicorn running'), and preserve the final partial segment back into the accumulator for the next chunk so partial lines are not lost.
🧹 Nitpick comments (3)
apps/vscode/src/providers/views/PageWelcome/styles.css (1)
156-165: Minor inconsistency: hover color is hardcoded while base color uses variable.The base link color uses
var(--left-fg), but the hover state hardcodes#ffffff. For consistency, consider adding a--left-fg-hovercustom property or reusing--left-fg-strong.♻️ Suggested consistency improvement
.welcome-links a:hover { - color: `#ffffff`; + color: var(--left-fg-strong); text-decoration: underline; }Or define a dedicated hover variable with the other custom properties:
--left-fg-hover: `#ffffff`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageWelcome/styles.css` around lines 156 - 165, The hover rule for .welcome-links a hardcodes `#ffffff` while the base uses var(--left-fg); change the hover to use a CSS variable (e.g., var(--left-fg-hover) or var(--left-fg-strong)) and add that custom property alongside the other theme vars so hover styling stays consistent and themable; update the .welcome-links a:hover selector to reference the chosen variable instead of the literal color.apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx (1)
502-503: Move module constant to top of file for clarity.
IMAGE_BASEis defined at the end of the file but used in the component JSX (line 398). While this works due to module-level hoisting, placing constants at the top improves readability and follows common conventions.♻️ Suggested refactor
Move line 503 to after the imports (around line 12):
import './styles.css'; + +const IMAGE_BASE = 'ghcr.io/rocketride-org/rocketride-engine'; // These interfaces are intentionally duplicated...And remove lines 502-503.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx` around lines 502 - 503, IMAGE_BASE is declared near the end of the module but is referenced earlier in the PageDeploy component JSX; move the module constant IMAGE_BASE to the top of the file just after the import block (so it appears before the PageDeploy component and its JSX), removing the original trailing declaration, to improve readability and follow convention.apps/vscode/src/deploy/service-windows.ts (1)
204-246: Consider adding integrity verification for NSSM download.The NSSM binary is downloaded over HTTPS, which provides transport security, but there's no verification of the downloaded archive's integrity. A compromised or man-in-the-middle attack could potentially inject malicious code.
Consider adding a SHA-256 hash verification after download:
🛡️ Optional: Add hash verification
+import * as crypto from 'crypto'; + +const NSSM_SHA256 = 'expected_sha256_hash_here'; + private async downloadNssm(): Promise<void> { // ... download logic ... await this.downloadFile(NSSM_DOWNLOAD_URL, zipPath); + + // Verify integrity + const hash = crypto.createHash('sha256'); + hash.update(fs.readFileSync(zipPath)); + const actualHash = hash.digest('hex'); + if (actualHash !== NSSM_SHA256) { + fs.unlinkSync(zipPath); + throw new Error('NSSM integrity check failed'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/deploy/service-windows.ts` around lines 204 - 246, Add SHA-256 integrity verification in downloadNssm: after the retry loop where zipPath is written and before extracting with AdmZip, compute the SHA-256 of the downloaded file (using Node's crypto on fs.readFileSync(zipPath)) and compare it to an expected constant (e.g., NSSM_ZIP_SHA256) that you add near NSSM_DOWNLOAD_URL; if the digest does not match, delete the zip and throw an Error indicating checksum mismatch. Keep all existing retry/cleanup behavior (use downloadFile, NSSM_DIR, NSSM_PATH) and fail fast before zip.extractEntryTo if the hash check fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/config.ts`:
- Around line 499-501: getApiHost currently returns config.hostUrl even in local
mode which can write a stale remote URI into .env; update getApiHost to detect
local mode (the runtime mode used by ConnectionManager/dynamic port) and, when
local, ignore config.hostUrl and return the local fallback
'http://localhost:5565' instead, otherwise keep the existing behavior using
config.hostUrl || 'http://localhost:5565'. Ensure the check is placed in
getApiHost so .env sync only persists the real remote URL when not in local
mode.
In `@apps/vscode/src/connection/engine-installer.ts`:
- Around line 214-219: The uninstall() method deletes enginesRoot without using
the cross-process installer lock; update uninstall() to acquire the same
installer lock used elsewhere in the EngineInstaller (or engine installer
module) before checking/deleting this.enginesRoot and to release the lock when
finished (including on errors), so deletion is serialized with concurrent
installs/updates—locate and reuse the existing lock acquisition/release
functions (the same lock helpers used by install/update paths) around the
fs.existsSync / fs.rmSync operations inside uninstall().
In `@apps/vscode/src/providers/PageDeployProvider.ts`:
- Around line 524-553: DOCKER_CONFIG_PATH currently points to a global location
that on Linux may be unwritable by a normal user; update the definition of
DOCKER_CONFIG_PATH and any reads/writes (referencing DOCKER_CONFIG_PATH,
writeDockerConfig, readDockerConfig, deleteDockerConfig) to use a per-user,
writable config location instead (for example prefer process.env.XDG_CONFIG_HOME
if set, otherwise fall back to the user's home config directory such as
path.join(os.homedir(), '.config', 'RocketRide', 'docker-config.json')), then
keep the existing mkdirSync/writeFileSync/readFileSync/unlinkSync logic but
operating on that user-writable path so no root permissions are required. Ensure
the new path construction uses path.join and preserves the RocketRide
subdirectory naming.
- Around line 385-397: The CONFIG_PATH currently resolves to /etc/... on Linux
which is not writable by normal users; update PageDeployProvider.CONFIG_PATH to
pick a user-writable location on Linux (prefer XDG_CONFIG_HOME or fallback to
path.join(os.homedir(), '.config', 'RocketRide', 'config.json')) and keep the
existing Windows/macOS behavior for other platforms, then ensure
writeServiceConfig still uses that constant (PageDeployProvider.CONFIG_PATH) and
creates the directory with fs.mkdirSync(..., { recursive: true }) before
writing; alternatively, if you intend system-wide config, document that
installation must create /etc/RocketRide with correct permissions and only use
that path during an elevated installer path resolution.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Line 1459: The warning path calls logger.warning(...) but no module-level
logger is defined, causing a NameError; fix by ensuring a logger exists and is
used—either import logging and call logging.getLogger(__name__) once at module
scope (e.g., define logger = logging.getLogger(__name__)) and then keep
logger.warning(f"Failed to parse engine arg {arg!r}: {e}, using as-is"), or
replace the call with logging.warning(...) so the malformed-arg handling doesn't
crash; update the reference to the symbol logger.warning in task_engine.py
accordingly.
---
Outside diff comments:
In `@apps/vscode/src/connection/engine-manager.ts`:
- Around line 198-248: The child process event handlers currently act on shared
instance state and can be triggered by stale children; fix by capturing the
spawned child and its pid-file into local constants (e.g. const launched =
this.child and const myPidFile as already set) and at the top of each handler
(the functions attached to child.on('error') and child.on('exit')) verify the
event belongs to that launched instance (if (this.child !== launched) return)
before performing any cleanup, writing logs, flipping started, clearing
this.child, calling cleanupProcess()/removePidFile(), or emitting 'terminated'
so stale callbacks cannot clobber a new engine.
- Around line 266-291: The stdout/stderr handlers currently parse each incoming
chunk independently which can split the "Uvicorn running" line across chunks;
change the handlers on this.child.stdout?.on('data', ...) and
this.child.stderr?.on('data', ...) to use a line-buffered approach: keep a
per-stream accumulator string, append each chunk, split on '\n' and process only
the complete lines (trim and call this.logger.console and tryResolveReady when a
line includes 'Uvicorn running'), and preserve the final partial segment back
into the accumulator for the next chunk so partial lines are not lost.
In `@apps/vscode/src/providers/views/PageSettings/PageSettings.tsx`:
- Around line 127-142: The initial useState initializer for the settings state
(const [settings, setSettings] = useState<SettingsData>(...)) is missing the
required localDebugOutput property from the SettingsData type; update the
initializer object used in PageSettings to include a sensible default for
localDebugOutput (e.g., false or the appropriate default type/value) so the
settings state is fully typed and UI doesn't see undefined before loaded.
---
Nitpick comments:
In `@apps/vscode/src/deploy/service-windows.ts`:
- Around line 204-246: Add SHA-256 integrity verification in downloadNssm: after
the retry loop where zipPath is written and before extracting with AdmZip,
compute the SHA-256 of the downloaded file (using Node's crypto on
fs.readFileSync(zipPath)) and compare it to an expected constant (e.g.,
NSSM_ZIP_SHA256) that you add near NSSM_DOWNLOAD_URL; if the digest does not
match, delete the zip and throw an Error indicating checksum mismatch. Keep all
existing retry/cleanup behavior (use downloadFile, NSSM_DIR, NSSM_PATH) and fail
fast before zip.extractEntryTo if the hash check fails.
In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx`:
- Around line 502-503: IMAGE_BASE is declared near the end of the module but is
referenced earlier in the PageDeploy component JSX; move the module constant
IMAGE_BASE to the top of the file just after the import block (so it appears
before the PageDeploy component and its JSX), removing the original trailing
declaration, to improve readability and follow convention.
In `@apps/vscode/src/providers/views/PageWelcome/styles.css`:
- Around line 156-165: The hover rule for .welcome-links a hardcodes `#ffffff`
while the base uses var(--left-fg); change the hover to use a CSS variable
(e.g., var(--left-fg-hover) or var(--left-fg-strong)) and add that custom
property alongside the other theme vars so hover styling stays consistent and
themable; update the .welcome-links a:hover selector to reference the chosen
variable instead of the literal color.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dda088b-03bf-4c3a-8778-532ca76725c8
📒 Files selected for processing (18)
apps/vscode/src/config.tsapps/vscode/src/connection/engine-installer.tsapps/vscode/src/connection/engine-manager.tsapps/vscode/src/deploy/docker-manager.tsapps/vscode/src/deploy/service-linux.tsapps/vscode/src/deploy/service-mac.tsapps/vscode/src/deploy/service-manager.tsapps/vscode/src/deploy/service-windows.tsapps/vscode/src/providers/PageDeployProvider.tsapps/vscode/src/providers/views/PageDeploy/PageDeploy.tsxapps/vscode/src/providers/views/PageSettings/ConnectionSettings.tsxapps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsxapps/vscode/src/providers/views/PageSettings/PageSettings.tsxapps/vscode/src/providers/views/PageWelcome/styles.cssapps/vscode/src/stubs/ssh2.jspackages/ai/src/ai/modules/task/task_engine.pypackages/ai/src/ai/node.pypackages/shared-ui/src/theme.ts
✅ Files skipped from review due to trivial changes (1)
- apps/vscode/src/deploy/service-manager.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared-ui/src/theme.ts
- apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx
- packages/ai/src/ai/node.py
* fix mcp client sse http-stream, adjust styling * move _is_mapping into IGlobal as a staticmethod
github-actions
Bot
added
module:vscode
Rod-Christensen
marked this pull request as ready for review
March 18, 2026 05:16
ryan-t-christensen
previously approved these changes
Mar 18, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (4)
packages/ai/src/ai/modules/task/task_engine.py (1)
1459-1462:⚠️ Potential issue | 🔴 CriticalUndefined
loggerwill crash on malformed engine arguments.
loggeris not defined in this module. Whenshlex.split()raisesValueErroron malformed input (e.g., unclosed quotes), this line will raiseNameError, masking the original parsing issue. The class already providesself.debug_message()for logging.🐛 Proposed fix
except ValueError as e: - logger.warning(f"Failed to parse engine arg {arg!r}: {e}, using as-is") + self.debug_message(f"Failed to parse engine arg {arg!r}: {e}, using as-is") child_args.append(arg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai/src/ai/modules/task/task_engine.py` around lines 1459 - 1462, The except block catching ValueError after child_args.extend(shlex.split(arg)) uses an undefined logger and will raise NameError; replace the logger.warning call with the class logger helper by calling self.debug_message(...) (or the appropriate instance logging method provided) and include the arg and exception details in the message, then append the original arg to child_args as before so malformed args are used as-is; update the except block around shlex.split(arg) in the code path that builds child_args accordingly.apps/vscode/src/config.ts (1)
499-501:⚠️ Potential issue | 🟡 MinorLocal mode may sync a stale remote URL to
.env.In local mode,
getApiHost()still returnsconfig.hostUrlwhen present. If a user previously used on-prem mode, the.envsync could persist that remote URI instead of a consistent local default. Since the actual URL is determined at runtime by ConnectionManager, consider always returning the local fallback in local mode for.envconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/config.ts` around lines 499 - 501, getApiHost currently returns config.hostUrl when present, which can persist a stale remote URI into .env; update getApiHost so that when the extension is running in local mode (use the existing local-mode flag/utility used elsewhere in this module — e.g., the local mode boolean or isLocalMode()/isLocal flag), it always returns the local fallback string 'http://localhost:5565' (ignore config.hostUrl) so .env sync is consistent; keep ConnectionManager behavior unchanged for runtime resolution.apps/vscode/src/connection/engine-installer.ts (1)
214-219:⚠️ Potential issue | 🟠 Major
uninstall()lacks cross-process lock coordination.Unlike
install()andcleanupOldVersions(),uninstall()deletes the entireenginesRootwithout acquiring the cross-process lock. A concurrent VS Code window could be installing or updating pointers while this runs.🔒 Proposed fix
- public uninstall(): void { + public async uninstall(): Promise<void> { + fs.mkdirSync(this.enginesRoot, { recursive: true }); + this.ensureLockFileExists(); + let release: (() => Promise<void>) | undefined; + try { + release = await lockfile.lock(this.lockFilePath(), { + stale: 120000, + retries: { retries: 10, minTimeout: 500, maxTimeout: 3000 }, + }); + } catch { + this.logger.output(`${icons.warning} Could not acquire lock for uninstall`); + return; + } + try { if (fs.existsSync(this.enginesRoot)) { fs.rmSync(this.enginesRoot, { recursive: true, force: true }); this.logger.output(`${icons.info} All engines uninstalled from ${this.enginesRoot}`); } + } finally { + if (release) await release(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/engine-installer.ts` around lines 214 - 219, The uninstall() method deletes enginesRoot without using the same cross-process lock used by install() and cleanupOldVersions(), risking races; modify uninstall() to acquire the same cross-process lock (the lock mechanism used by install()/cleanupOldVersions()), perform the exists check and rmSync(...) while holding the lock, and always release the lock in a finally block (with a reasonable timeout/try-acquire failure path) so concurrent VS Code windows cannot install/update pointers while enginesRoot is removed.apps/vscode/src/deploy/service-linux.ts (1)
69-73:⚠️ Potential issue | 🔴 CriticalUse secure temp files for privileged unit updates.
Line 69 and Line 98 use a fixed filename in
/tmp(rocketride.service.tmp) beforesudo cp. This is vulnerable to symlink/race attacks and can let another local user swap unit contents before root installs them.🔒 Proposed hardening diff
+ private writeSecureTempUnit(unitContent: string): { tmpDir: string; tmpUnit: string } { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'rocketride-')); + const tmpUnit = path.join(tmpDir, 'rocketride.service'); + fs.writeFileSync(tmpUnit, unitContent, { encoding: 'utf8', mode: 0o600, flag: 'wx' }); + return { tmpDir, tmpUnit }; + } + public async install(executablePath: string, engineDir: string): Promise<void> { // Create install root with elevation (may not be user-writable) await this.runSudo('mkdir', ['-p', INSTALL_ROOT]); const unitContent = this.buildUnitFile(executablePath, engineDir); - const tmpUnit = path.join(os.tmpdir(), 'rocketride.service.tmp'); - fs.writeFileSync(tmpUnit, unitContent, 'utf8'); - - await this.runSudo('cp', [tmpUnit, UNIT_PATH]); - fs.unlinkSync(tmpUnit); + const { tmpDir, tmpUnit } = this.writeSecureTempUnit(unitContent); + try { + await this.runSudo('cp', [tmpUnit, UNIT_PATH]); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } await this.runSudo('systemctl', ['daemon-reload']); await this.runSudo('systemctl', ['enable', UNIT_NAME]); await this.runSudo('systemctl', ['start', UNIT_NAME]); @@ public async update(executablePath: string, engineDir: string): Promise<void> { await this.runSudo('systemctl', ['stop', UNIT_NAME]); const unitContent = this.buildUnitFile(executablePath, engineDir); - const tmpUnit = path.join(os.tmpdir(), 'rocketride.service.tmp'); - fs.writeFileSync(tmpUnit, unitContent, 'utf8'); - await this.runSudo('cp', [tmpUnit, UNIT_PATH]); - fs.unlinkSync(tmpUnit); + const { tmpDir, tmpUnit } = this.writeSecureTempUnit(unitContent); + try { + await this.runSudo('cp', [tmpUnit, UNIT_PATH]); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } await this.runSudo('systemctl', ['daemon-reload']); await this.runSudo('systemctl', ['start', UNIT_NAME]);#!/bin/bash # Verify predictable temp unit usage in privileged copy paths. rg -n -C2 "rocketride\\.service\\.tmp|writeFileSync\\(tmpUnit|runSudo\\('cp', \\[tmpUnit, UNIT_PATH\\]\\)"Also applies to: 98-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/deploy/service-linux.ts` around lines 69 - 73, The code uses a predictable temp file name (tmpUnit = path.join(os.tmpdir(), 'rocketride.service.tmp')) and then writes with fs.writeFileSync and later runs runSudo('cp', [tmpUnit, UNIT_PATH]) which is susceptible to symlink/race attacks; change to create a secure, unique temp file (e.g., use fs.mkdtemp or a UUID-based filename or tmp.file from a secure temp library) and write unitContent there, verify the file is created with safe permissions (0600) before invoking runSudo('cp', [secureTmpPath, UNIT_PATH]), and ensure you unlink the same secure path after copy; update all occurrences (tmpUnit, fs.writeFileSync, fs.unlinkSync, runSudo('cp', [tmpUnit, UNIT_PATH])) accordingly.
🧹 Nitpick comments (4)
apps/vscode/src/providers/views/PageWelcome/styles.css (1)
162-165: Add keyboard-visible focus style for left-panel links.Hover styling was updated, but keyboard users should get an equivalent visual cue on focus.
♿ Suggested accessibility refinement
-.welcome-links a:hover { +.welcome-links a:hover, +.welcome-links a:focus-visible { color: `#ffffff`; text-decoration: underline; + outline: 1px solid var(--left-fg-strong); + outline-offset: 2px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageWelcome/styles.css` around lines 162 - 165, The hover-only rule for left-panel links (.welcome-links a:hover) lacks a keyboard-visible focus style, so add matching focus styles for keyboard users by extending the selector to include :focus and preferably :focus-visible (e.g., .welcome-links a:focus, .welcome-links a:focus-visible) and apply the same color and underline (or an accessible alternative) to ensure keyboard users receive an equivalent visual cue; update the existing .welcome-links a:hover declaration to include these focus selectors and ensure it meets contrast and visibility requirements for the left-panel links.apps/vscode/src/deploy/service-mac.ts (1)
132-164: Consider escaping XML special characters in interpolated values.The template interpolates
executablePath,workingDir, etc. directly into XML. While these are typically system paths controlled by the extension, paths containing&,<, or>would produce malformed plist XML.A simple helper would make this more robust:
♻️ Suggested helper
+private escapeXml(s: string): string { + return s.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); +} + private buildPlist(executablePath: string, workingDir: string): string { + const exe = this.escapeXml(executablePath); + const wd = this.escapeXml(workingDir); + const logs = this.escapeXml(LOGS_DIR); return `<?xml version="1.0" encoding="UTF-8"?> ... - <string>${executablePath}</string> + <string>${exe}</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/deploy/service-mac.ts` around lines 132 - 164, The buildPlist function interpolates values like PLIST_LABEL, executablePath, workingDir, SERVICE_PORT and path.join(LOGS_DIR, ...) directly into XML which can break the plist if any value contains XML-special characters; add a small helper (e.g., escapeXml) to replace &, <, >, " and ' with their entities and call it for every interpolated value used in buildPlist (PLIST_LABEL, executablePath, workingDir, SERVICE_PORT string, and the stdout/stderr paths produced via path.join) before embedding them into the template.apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx (1)
71-79: Associate help text with each checkbox for assistive tech.At Line 79, the description is not programmatically linked to the checkbox, so screen readers may miss it during focus navigation.
♿ Suggested accessibility patch
- {INTEGRATIONS.map(({ key, label, description }) => ( - <React.Fragment key={key}> + {INTEGRATIONS.map(({ key, label, description }) => { + const helpTextId = `${key}-help-text`; + return ( + <React.Fragment key={key}> <label> <input type="checkbox" checked={!!settings[key]} + aria-describedby={helpTextId} onChange={(e) => onSettingsChange({ [key]: e.target.checked })} /> <span>{label}</span> </label> - <div className="help-text">{description}</div> + <div id={helpTextId} className="help-text">{description}</div> </React.Fragment> - ))} + ); + })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx` around lines 71 - 79, The checkbox input in IntegrationSettings (the input using checked={!!settings[key]} and onChange calling onSettingsChange) is not programmatically linked to its help text (description), so add an accessible relationship by giving the help text element a unique id (e.g. `${key}-help`) and adding aria-describedby referring to that id on the input; ensure the id is stable/unique per key and keep the help text element (the div with className="help-text") targeted by aria-describedby so screen readers announce the description when the checkbox receives focus.apps/vscode/package.json (1)
94-98: Syncapps/vscode/docs/api/with the new public manifest surface before release.This PR introduces public settings/commands (
rocketride.local.debugOutput,rocketride.integrations.*, focus commands, and command discoverability changes). Please update the API docs set so extension behavior and supported config are documented as a stable contract.Based on learnings: Reference and quote relevant documentation from
apps/vscode/docs/api/in code review responses when discussing Rocket Ride code.Also applies to: 130-149, 162-171, 284-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/package.json` around lines 94 - 98, Update the API docs in apps/vscode/docs/api to reflect the new public manifest surface: add entries for the new settings (rocketride.local.debugOutput with its boolean/default/description, all rocketride.integrations.* keys), the new focus commands and any command discoverability changes, and ensure descriptions, defaults and visibility match the package.json manifest; also update example usage and a stability/compatibility note so this is presented as a stable contract, and after editing, reference/quote the updated docs when discussing Rocket Ride code in future reviews.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/deploy/service-linux.ts`:
- Line 56: The child process created by runSudo is leaving stdout undrained,
which can block on verbose commands like apt-get install (invoked with
LinuxServiceManager.ENGINE_DEPS); update the runSudo implementation used by
service-linux.ts (and similarly service-mac.ts) to attach a 'data' listener on
child.stdout (e.g., child.stdout.on('data', ()=>{})) to continuously drain
stdout, and ensure the listener is added before awaiting process completion so
the stdout buffer cannot fill and stall the deployment flow.
In `@apps/vscode/src/deploy/service-windows.ts`:
- Around line 204-245: The downloadNssm method currently downloads and extracts
nssm.exe without validating its integrity or signature; update it to verify the
payload before extraction/execution by either (a) pinning a known SHA-256 digest
and computing the downloaded zip/file hash via the downloadFile result and
comparing to the constant, or (b) validating the Authenticode signature of the
extracted nssm.exe using a Windows signature check library, and fail the
operation if validation fails; locate the logic in downloadNssm (and where
downloadFile is used) and add the hash/signature verification step against a
stored constant (e.g. NSSM_SHA256 or an Authenticode check) before
zip.extractEntryTo and before any elevated execution of NSSM_PATH, logging and
throwing a descriptive error on mismatch.
- Around line 72-83: Update the NSSM install/start command arguments so the
engine is bound to loopback instead of all interfaces: in the runElevatedScript
call that builds psCmd(...) for NSSM (references: runElevatedScript, psCmd,
NSSM_PATH, SERVICE_NAME, SERVICE_PORT) replace the `--host=0.0.0.0` argument
with `--host=127.0.0.1`; make the same change in the other similar NSSM psCmd
block later in the file (the second install/start sequence).
- Around line 114-121: The current catch on the runElevatedScript('remove.ps1',
script) call swallows all errors; change it to inspect the thrown error from
runElevatedScript (or its message) and only ignore the explicit "service not
installed" condition, while rethrowing or logging and aborting for other errors
(e.g., UAC cancellation, missing nssm.exe, removal failure). Only proceed to
remove INSTALL_ROOT and call this.logger.output(`${icons.success} Service
removed`) when the elevated script actually succeeded; if the script fails with
any non-"not installed" error, surface that error (reject/throw or log and
return) instead of continuing. Locate this logic around
runElevatedScript/remove.ps1, INSTALL_ROOT cleanup, and the success log to
implement the conditional error handling.
- Around line 289-299: The PowerShell wrapper (psCommand) uses Start-Process
-Wait which doesn't propagate the elevated process exit code back to the caller,
so execFile sees success even when the elevated script fails; change psCommand
to start the elevated process with -PassThru, capture the returned process
object, and explicitly exit with its ExitCode (e.g. $p = Start-Process ... -Wait
-PassThru; exit $p.ExitCode) so execFile('powershell.exe', ...) receives the
real non-zero exit code and the existing error handling in the execFile callback
will reject appropriately.
In `@apps/vscode/src/providers/PageDeployProvider.ts`:
- Around line 324-340: The wait loops (waitForServiceRunning and the similar
loop at lines 568-583) currently only log a timeout and allow flows to continue;
change them to fail the action by throwing an Error (or rejecting) when the
timeout elapses so upstream handlers like connectToService, serviceComplete, and
dockerComplete do not run on failed startups. Locate the methods
waitForServiceRunning and the analogous wait loop, replace the final logger-only
path with a thrown Error containing clear context (e.g., include timeoutSec and
service id/name), and ensure callers either await/propagate the thrown Error or
handle it to abort the deployment flow.
- Around line 483-491: getDockerImageTag currently returns the literal 'latest',
which causes dockerInstall to persist 'latest' and dockerUpdate to skip pulling;
change the behavior so 'latest' is resolved to a concrete image identifier (tag
or digest) before persisting or ensure update logic always pulls for the latest
channel. Specifically, in getDockerImageTag and the calling flow
(dockerInstall/dockerUpdate) detect when versionSpec === 'latest' and (a) query
GHCR or the ghcrTags list to find the actual latest stable tag or digest and
return that concrete value, or (b) if resolution is not done here, update
dockerUpdate to treat the saved value specially when the selected channel is
'latest' by forcing a pull regardless of equality. Apply the same change to the
other occurrences noted around lines 493-500 and 517-524 so 'latest' is never
stored/treated as already up-to-date.
In `@apps/vscode/src/providers/views/PageDeploy/PageDeploy.tsx`:
- Around line 242-259: The split-button toggle and options are not
keyboard/screen-reader accessible; update the toggle button used in PageDeploy
to include an accessible name (aria-label), aria-expanded bound to isOpen, and
aria-haspopup/aria-controls referencing the dropdown id, and convert each option
rendered in the options.map (currently a clickable div) into keyboard-focusable,
semantic controls (either <button> or elements with role="menuitem" and
tabIndex={0}) and add onKeyDown handlers to those items to trigger
onVersionChange on Enter/Space and close the menu via setDropdownOpen(null) on
selection or Escape; also ensure the dropdown container (split-button-dropdown)
has an appropriate role (menu) and that focus is moved into the menu when
opening (e.g., focus the selected item) so keyboard users can navigate.
In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx`:
- Around line 27-55: The docs are missing entries for the new boolean settings;
update the Extension Settings documentation table in README-vscode.md to add
rows for integrationCopilot, integrationClaudeCode, integrationCursor, and
integrationWindsurf (use the exact key names), each with a short description
like "Enable RocketRide integration with GitHub Copilot" (and analogous
descriptions for Claude Code, Cursor, and Windsurf) and the appropriate type
(boolean) and default value; ensure the new rows follow the existing table
column order/format so they render consistently.
---
Duplicate comments:
In `@apps/vscode/src/config.ts`:
- Around line 499-501: getApiHost currently returns config.hostUrl when present,
which can persist a stale remote URI into .env; update getApiHost so that when
the extension is running in local mode (use the existing local-mode flag/utility
used elsewhere in this module — e.g., the local mode boolean or
isLocalMode()/isLocal flag), it always returns the local fallback string
'http://localhost:5565' (ignore config.hostUrl) so .env sync is consistent; keep
ConnectionManager behavior unchanged for runtime resolution.
In `@apps/vscode/src/connection/engine-installer.ts`:
- Around line 214-219: The uninstall() method deletes enginesRoot without using
the same cross-process lock used by install() and cleanupOldVersions(), risking
races; modify uninstall() to acquire the same cross-process lock (the lock
mechanism used by install()/cleanupOldVersions()), perform the exists check and
rmSync(...) while holding the lock, and always release the lock in a finally
block (with a reasonable timeout/try-acquire failure path) so concurrent VS Code
windows cannot install/update pointers while enginesRoot is removed.
In `@apps/vscode/src/deploy/service-linux.ts`:
- Around line 69-73: The code uses a predictable temp file name (tmpUnit =
path.join(os.tmpdir(), 'rocketride.service.tmp')) and then writes with
fs.writeFileSync and later runs runSudo('cp', [tmpUnit, UNIT_PATH]) which is
susceptible to symlink/race attacks; change to create a secure, unique temp file
(e.g., use fs.mkdtemp or a UUID-based filename or tmp.file from a secure temp
library) and write unitContent there, verify the file is created with safe
permissions (0600) before invoking runSudo('cp', [secureTmpPath, UNIT_PATH]),
and ensure you unlink the same secure path after copy; update all occurrences
(tmpUnit, fs.writeFileSync, fs.unlinkSync, runSudo('cp', [tmpUnit, UNIT_PATH]))
accordingly.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 1459-1462: The except block catching ValueError after
child_args.extend(shlex.split(arg)) uses an undefined logger and will raise
NameError; replace the logger.warning call with the class logger helper by
calling self.debug_message(...) (or the appropriate instance logging method
provided) and include the arg and exception details in the message, then append
the original arg to child_args as before so malformed args are used as-is;
update the except block around shlex.split(arg) in the code path that builds
child_args accordingly.
---
Nitpick comments:
In `@apps/vscode/package.json`:
- Around line 94-98: Update the API docs in apps/vscode/docs/api to reflect the
new public manifest surface: add entries for the new settings
(rocketride.local.debugOutput with its boolean/default/description, all
rocketride.integrations.* keys), the new focus commands and any command
discoverability changes, and ensure descriptions, defaults and visibility match
the package.json manifest; also update example usage and a
stability/compatibility note so this is presented as a stable contract, and
after editing, reference/quote the updated docs when discussing Rocket Ride code
in future reviews.
In `@apps/vscode/src/deploy/service-mac.ts`:
- Around line 132-164: The buildPlist function interpolates values like
PLIST_LABEL, executablePath, workingDir, SERVICE_PORT and path.join(LOGS_DIR,
...) directly into XML which can break the plist if any value contains
XML-special characters; add a small helper (e.g., escapeXml) to replace &, <, >,
" and ' with their entities and call it for every interpolated value used in
buildPlist (PLIST_LABEL, executablePath, workingDir, SERVICE_PORT string, and
the stdout/stderr paths produced via path.join) before embedding them into the
template.
In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx`:
- Around line 71-79: The checkbox input in IntegrationSettings (the input using
checked={!!settings[key]} and onChange calling onSettingsChange) is not
programmatically linked to its help text (description), so add an accessible
relationship by giving the help text element a unique id (e.g. `${key}-help`)
and adding aria-describedby referring to that id on the input; ensure the id is
stable/unique per key and keep the help text element (the div with
className="help-text") targeted by aria-describedby so screen readers announce
the description when the checkbox receives focus.
In `@apps/vscode/src/providers/views/PageWelcome/styles.css`:
- Around line 162-165: The hover-only rule for left-panel links (.welcome-links
a:hover) lacks a keyboard-visible focus style, so add matching focus styles for
keyboard users by extending the selector to include :focus and preferably
:focus-visible (e.g., .welcome-links a:focus, .welcome-links a:focus-visible)
and apply the same color and underline (or an accessible alternative) to ensure
keyboard users receive an equivalent visual cue; update the existing
.welcome-links a:hover declaration to include these focus selectors and ensure
it meets contrast and visibility requirements for the left-panel links.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57ee1e93-8fef-42e1-ab62-537162d91268
📒 Files selected for processing (20)
apps/vscode/package.jsonapps/vscode/src/config.tsapps/vscode/src/connection/engine-installer.tsapps/vscode/src/connection/engine-manager.tsapps/vscode/src/deploy/docker-manager.tsapps/vscode/src/deploy/service-linux.tsapps/vscode/src/deploy/service-mac.tsapps/vscode/src/deploy/service-manager.tsapps/vscode/src/deploy/service-windows.tsapps/vscode/src/providers/PageDeployProvider.tsapps/vscode/src/providers/views/PageDeploy/PageDeploy.tsxapps/vscode/src/providers/views/PageDeploy/styles.cssapps/vscode/src/providers/views/PageSettings/ConnectionSettings.tsxapps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsxapps/vscode/src/providers/views/PageSettings/PageSettings.tsxapps/vscode/src/providers/views/PageWelcome/styles.cssapps/vscode/src/stubs/ssh2.jspackages/ai/src/ai/modules/task/task_engine.pypackages/ai/src/ai/node.pypackages/shared-ui/src/theme.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared-ui/src/theme.ts
- apps/vscode/src/providers/views/PageSettings/PageSettings.tsx
- apps/vscode/src/deploy/service-manager.ts
stepmikhaylov
previously approved these changes
Mar 18, 2026
stepmikhaylov
enabled auto-merge (squash)
March 18, 2026 15:25
Rod-Christensen
dismissed stale reviews from stepmikhaylov and ryan-t-christensen
via
March 18, 2026 18:37
419a37e
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/vscode/src/connection/engine-installer.ts (1)
843-855:⚠️ Potential issue | 🟠 Major
httpStream()needs a timeout to prevent indefinite hangs.If the HTTP request stalls during DNS lookup, TLS handshake, or socket connection, the promise remains pending forever, blocking
downloadAsset()and preventing retries from triggering. The cancellation token only provides external timeout control; add an explicit timeout to theprotocol.get()call (e.g.,req.setTimeout()) so that stalled requests fail fast and allow the retry loop to proceed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/connection/engine-installer.ts` around lines 843 - 855, The httpStream() promise can hang on stalled network phases; add an explicit socket timeout on the outgoing request (use req.setTimeout(timeoutMs, ...) or equivalent) inside httpStream so stalled requests are aborted and the promise is rejected; on timeout call req.destroy() (or req.abort()) and reject with a clear Error so downloadAsset()'s retry logic can proceed, and ensure you remove or handle the timeout/error listeners to avoid leaks; keep the timeout value configurable or use a reasonable default and apply the same behavior when following redirects in httpStream().
🧹 Nitpick comments (3)
apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx (2)
32-55: Narrow integration key typing to integration-prefixed settings only.
BooleanKeys<SettingsData>also includes unrelated booleans (hasApiKey,autoConnect, etc.). Constraining keys tointegration*avoids accidental misconfiguration later.♻️ Proposed change
-type BooleanKeys<T> = { [K in keyof T]: T[K] extends boolean ? K : never }[keyof T]; +type IntegrationKey = Extract<keyof SettingsData, `integration${string}`>; -const INTEGRATIONS: { key: BooleanKeys<SettingsData>; label: string; description: string }[] = [ +const INTEGRATIONS: { key: IntegrationKey; label: string; description: string }[] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx` around lines 32 - 55, The INTEGRATIONS array currently types keys with BooleanKeys<SettingsData>, which can include non-integration booleans; create a narrower template-lit type (e.g., IntegrationKeys<SettingsData>) that only yields keys that both extend boolean and match the `integration${string}` pattern, and replace BooleanKeys<SettingsData> with this new IntegrationKeys type in the INTEGRATIONS declaration so INTEGRATIONS.key can only be an integration-prefixed setting (referencing BooleanKeys and INTEGRATIONS to locate where to change).
25-25: Use a type-only import forSettingsDatato avoid circular dependency issues.
SettingsDatais used only as a type in this file (for the interface and type constraints), creating an unnecessary runtime dependency. A circular import exists between this file andPageSettings.tsx, whichimport typewill safely resolve.♻️ Proposed change
-import { SettingsData } from './PageSettings'; +import type { SettingsData } from './PageSettings';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx` at line 25, Change the runtime import of SettingsData to a type-only import to break the circular dependency: replace the current import of SettingsData with a type import (i.e., use "import type { SettingsData } ...") wherever IntegrationSettings.tsx references the SettingsData type (including the interface and any type constraints) so the module is erased at compile time and no runtime dependency on PageSettings remains.apps/vscode/src/providers/views/PageWelcome/styles.css (1)
156-165: Keep the bottom links clearly link-like at rest.After the switch to
--left-fg, these anchors read very close to the surrounding copy until hover/focus. I’d keep a stronger default treatment here so they stay discoverable without interaction.♻️ Possible tweak
.welcome-links a { - color: var(--left-fg); - text-decoration: none; + color: var(--left-fg-strong); + text-decoration: underline; + text-underline-offset: 2px; font-size: 12px; } .welcome-links a:hover, .welcome-links a:focus, .welcome-links a:focus-visible { color: `#ffffff`; - text-decoration: underline; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode/src/providers/views/PageWelcome/styles.css` around lines 156 - 165, The .welcome-links a selector currently uses the muted variable (--left-fg) making links hard to distinguish; update the default styling for .welcome-links a to a stronger, more link-like appearance (e.g., a higher-contrast color and/or underline) while keeping the existing hover/focus rules (.welcome-links a:hover, .welcome-links a:focus, .welcome-links a:focus-visible) intact; modify the .welcome-links a rule to use a clearly distinguishable color (or add text-decoration: underline) so links are discoverable at rest without changing the hover behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/config.ts`:
- Around line 499-501: getApiHost() now returns different values based on
connectionMode, but the .env sync logic only watches hostUrl so ROCKETRIDE_URI
can become stale; update the sync trigger to detect changes to the effective API
host (call getApiHost()) or to listen for connectionMode changes and invoke the
same env-sync routine used when hostUrl changes. Specifically, modify the code
that currently compares hostUrl (the existing .env sync path) to instead compute
currentApiHost = getApiHost() (or add a ConnectionManager.on('modeChange', ...)
handler) and call the existing env sync function so ROCKETRIDE_URI is updated
whenever getApiHost() or connectionMode changes.
- Around line 478-485: The current trace detection uses
argsStr.includes('--trace=') (hasTrace) and misses the space-separated form like
"--trace servicePython", causing an extra flag to be appended; update the
detection logic used by hasTrace to check for either "--trace=" or "--trace "
(for example via a regex such as /--trace(?:=|\s+)/) so that when inspecting
argsStr before pushing '--trace=servicePython' (in the block referencing
config.local.debugOutput and result.push('--trace=servicePython')), the code
correctly recognizes both forms and avoids adding a duplicate trace flag.
In `@apps/vscode/src/connection/engine-installer.ts`:
- Around line 157-161: ensureLockFileExists currently writes the lock file
without ensuring the parent directory exists, causing ENOENT when called from
uninstall() or cleanupOldVersions(); update ensureLockFileExists (used alongside
lockFilePath()) to first ensure this.enginesRoot (or the directory returned by
lockFilePath()) exists by creating the directory if missing (e.g., mkdirSync
with { recursive: true }) before calling writeFileSync, so the lock file
creation becomes a no-op on fresh profiles and mirrors install() behavior.
In `@apps/vscode/src/deploy/service-linux.ts`:
- Around line 86-95: The remove() method currently swallows all errors from
runSudo calls and always prints success; change it to only ignore "not
installed"/ENOENT cases and otherwise propagate or surface failures so the
success message is not shown on partial/total failure. Specifically, in remove()
(references: runSudo, UNIT_NAME, UNIT_PATH, INSTALL_ROOT, logger.output,
icons.success) wrap each runSudo call to check the thrown error: if it indicates
"not found"/"Unit ... not found" or filesystem ENOENT for rm, treat as non-fatal
and continue, but for any other error record it (or rethrow) and abort printing
the success message; alternatively aggregate non-ignored errors and if the
aggregate is non-empty log the error(s) and throw/return before calling
logger.output(`${icons.success} Service removed`). Ensure systemctl
daemon-reload still runs on success path and that only the legitimate "already
absent" conditions are suppressed.
- Around line 55-62: prepareInstallRoot currently chowns INSTALL_ROOT to the
interactive user which enables privilege escalation; change ownership and
permissions so the service does not run as root and cannot execute user-modified
payloads: in prepareInstallRoot replace the runSudo('chown', ['-R', username,
INSTALL_ROOT]) with ownership root:root and restrictive perms (e.g.,
runSudo('chown', ['-R', 'root:root', INSTALL_ROOT]) and runSudo('chmod', ['-R',
'0755', INSTALL_ROOT]) or make only specific runtime subdirs writable). In
parallel, update buildUnitFile() to set a dedicated unprivileged service account
(add User=<serviceUser> and Group=<serviceUser> in the generated unit; create or
ensure that <serviceUser> exists during install) so the unit never executes as
root; reference functions buildUnitFile(), prepareInstallRoot(), INSTALL_ROOT
and LinuxServiceManager.ENGINE_DEPS when making these edits.
- Around line 164-189: The runSudo function must handle spawn() errors and avoid
unguarded stdio dereferences: immediately attach a child.on('error', ...)
handler after calling spawn('sudo', ...) to reject the Promise with the emitted
error, and change accesses to child.stdin/child.stdout/child.stderr to use
optional chaining (e.g., child.stdin?.write, child.stdin?.end, child.stdout?.on,
child.stderr?.on) so a missing stdio won't throw; keep the existing close
handler to resolve/reject on exit but ensure the error handler short-circuits
and rejects the same Promise when spawn fails (refer to runSudo, spawn,
this.sudoPassword, and the child.on('close', ...) logic).
In `@apps/vscode/src/deploy/service-mac.ts`:
- Around line 79-87: The remove() method currently swallows all errors from the
privileged commands so it always logs success; change it to surface failures
instead: for each runSudo call (unload/remove PLIST_PATH and rm -rf
INSTALL_ROOT/CONFIG_DIR) catch and collect errors (or rethrow) instead of
ignoring, log a descriptive failure via this.logger.error including the command
and error, and return/throw a non-success result so the caller/ UI can show
cleanup failed; keep references to remove(), runSudo(), PLIST_PATH,
INSTALL_ROOT, CONFIG_DIR and the existing this.logger.output/this.logger.error
to locate and update the code paths.
- Around line 53-60: prepareInstallRoot currently makes INSTALL_ROOT (e.g.,
/Library/RocketRide) user-owned via runSudo('chown', ['-R', ...]) which is
dangerous because the LaunchDaemon plist lacks a UserName key and will run as
root; either stop changing ownership (keep INSTALL_ROOT root-owned) or ensure
the daemon will run as the interactive user by adding UserName to the
LaunchDaemon plist. To fix: remove or narrow the chown call in
prepareInstallRoot (refer to prepareInstallRoot, runSudo, chown, INSTALL_ROOT,
CONFIG_DIR) so system-owned files remain root-owned, or alternatively add a
UserName entry to the LaunchDaemon plist so the service runs as the interactive
user; pick one approach and update the corresponding code/path and plist
accordingly.
- Around line 184-207: The runSudo function currently spawns a child process
with spawn('sudo', ...) but doesn't listen for the child's 'error' event and
assumes stdio streams exist; add a child.on('error', ...) handler that rejects
the Promise with the spawn error to avoid hanging when spawn fails (e.g., sudo
not found), and guard access to child.stdin, child.stdout and child.stderr
(check for existence before calling write(), end(), or attaching 'data'
listeners) so the promise always resolves or rejects cleanly even if streams are
null; reference runSudo, spawn, child, this.sudoPassword, child.stdin,
child.stdout, child.stderr and ensure the existing child.on('close', ...) logic
still resolves/rejects as before.
In `@apps/vscode/src/deploy/service-windows.ts`:
- Around line 63-85: The install() method currently relies on NSSM defaults
(LocalSystem); update the NSSM configuration sequence passed to
runElevatedScript to explicitly set the service account via an ObjectName entry
(using psCmd(NSSM_PATH, 'set', SERVICE_NAME, 'ObjectName', '<DOMAIN\\User>' or
'.\\<username>') placed alongside the other psCmd(...) calls before starting the
service), and either create/configure a dedicated low-privilege account
beforehand or add a comment documenting why LocalSystem is required if you
decide not to change it; reference install(), runElevatedScript, psCmd,
NSSM_PATH, and SERVICE_NAME when making the change.
- Around line 266-292: The downloadFile method currently has no request/response
timeouts; add a bounded timeout (e.g., configurable constant) that aborts the
HTTP request and destroys the response/stream if the download stalls, cleans up
the file write stream, clears timers and listeners, and rejects with a clear
timeout error; implement this by starting a timer when the request is made (and
reset/refresh it on response or socket 'data' events), calling req.abort() or
req.destroy() and response.destroy() on timeout, ensuring file.close() and
unlink(destPath) as needed on error, and removing the timer on success/failure;
apply the same pattern to httpStream in engine-installer.ts so both downloadFile
and httpStream use request + response timeouts and proper cleanup.
In `@apps/vscode/src/providers/PageDeployProvider.ts`:
- Around line 267-287: The short-circuit currently only compares
currentConfig.version to newVersion which misses tag moves; change the check in
PageDeployProvider so it also verifies the installed publishedAt matches the
config before returning early: call installer.getInstalledPublishedAt(channel)
and require currentConfig.publishedAt === that value (in addition to the
version/tag check, or replace with comparing versionSpec+publishedAt) so the
branch only short-circuits when both version identity and publishedAt match;
keep existing calls to installer.getInstalledVersion and writeServiceConfig
as-is but use the publishedAt comparison when deciding "Already up to date".
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 1466-1471: The current check only looks for tokens starting with
'--trace=' and can miss a separate '--trace' token (and its value) in
child_args; update the detection to treat either a token that starts with
'--trace=' or a standalone '--trace' followed by a value as “already present”
(e.g., check any(a.startswith('--trace=') or a == '--trace' for a in child_args)
and also consider the case where '--trace' appears at index i and
child_args[i+1] is its value). In the startup_args() inheritance loop, when
picking up the parent's trace option, handle both forms: if you find '--trace='
append as before; if you find a standalone '--trace' in startup_args, append
both the '--trace' token and its value (the next token) to child_args, and
ensure you don't append duplicates when child_args already contains either form.
---
Outside diff comments:
In `@apps/vscode/src/connection/engine-installer.ts`:
- Around line 843-855: The httpStream() promise can hang on stalled network
phases; add an explicit socket timeout on the outgoing request (use
req.setTimeout(timeoutMs, ...) or equivalent) inside httpStream so stalled
requests are aborted and the promise is rejected; on timeout call req.destroy()
(or req.abort()) and reject with a clear Error so downloadAsset()'s retry logic
can proceed, and ensure you remove or handle the timeout/error listeners to
avoid leaks; keep the timeout value configurable or use a reasonable default and
apply the same behavior when following redirects in httpStream().
---
Nitpick comments:
In `@apps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsx`:
- Around line 32-55: The INTEGRATIONS array currently types keys with
BooleanKeys<SettingsData>, which can include non-integration booleans; create a
narrower template-lit type (e.g., IntegrationKeys<SettingsData>) that only
yields keys that both extend boolean and match the `integration${string}`
pattern, and replace BooleanKeys<SettingsData> with this new IntegrationKeys
type in the INTEGRATIONS declaration so INTEGRATIONS.key can only be an
integration-prefixed setting (referencing BooleanKeys and INTEGRATIONS to locate
where to change).
- Line 25: Change the runtime import of SettingsData to a type-only import to
break the circular dependency: replace the current import of SettingsData with a
type import (i.e., use "import type { SettingsData } ...") wherever
IntegrationSettings.tsx references the SettingsData type (including the
interface and any type constraints) so the module is erased at compile time and
no runtime dependency on PageSettings remains.
In `@apps/vscode/src/providers/views/PageWelcome/styles.css`:
- Around line 156-165: The .welcome-links a selector currently uses the muted
variable (--left-fg) making links hard to distinguish; update the default
styling for .welcome-links a to a stronger, more link-like appearance (e.g., a
higher-contrast color and/or underline) while keeping the existing hover/focus
rules (.welcome-links a:hover, .welcome-links a:focus, .welcome-links
a:focus-visible) intact; modify the .welcome-links a rule to use a clearly
distinguishable color (or add text-decoration: underline) so links are
discoverable at rest without changing the hover behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1863c9e6-09a8-450a-8c89-1e6351b227bb
📒 Files selected for processing (12)
apps/vscode/src/config.tsapps/vscode/src/connection/engine-installer.tsapps/vscode/src/deploy/service-linux.tsapps/vscode/src/deploy/service-mac.tsapps/vscode/src/deploy/service-windows.tsapps/vscode/src/providers/PageDeployProvider.tsapps/vscode/src/providers/views/PageDeploy/PageDeploy.tsxapps/vscode/src/providers/views/PageDeploy/styles.cssapps/vscode/src/providers/views/PageSettings/IntegrationSettings.tsxapps/vscode/src/providers/views/PageWelcome/styles.cssdocs/README-vscode.mdpackages/ai/src/ai/modules/task/task_engine.py
✅ Files skipped from review due to trivial changes (1)
- docs/README-vscode.md
Rod-Christensen
marked this pull request as draft
March 18, 2026 19:23
auto-merge was automatically disabled
March 18, 2026 19:23
Pull request was converted to draft
Rod-Christensen
marked this pull request as ready for review
March 18, 2026 19:24
2 of 7 tasks
This was referenced Apr 1, 2026
10 tasks
20 tasks
This was referenced Aug 5, 2026
asclearuc
added a commit
that referenced
this pull request
Aug 8, 2026
With on_launch and on_terminate relocated, nothing in cmd_debug.py is reachable: the extension's debugger has been disabled since #268 and no live client sends initialize, attach, pause, continue, configurationDone, threads or disconnect. Delete the file, unregister DebugCommands from TaskConn, and drop TaskConn.request() / on_command() — the pair that forwarded unhandled DAP commands to debugpy. on_disconnect is deleted rather than relocated: stripped of its debugger parts it returns build_response(request), which is byte-for-byte what dap_conn's unhandled-command fallback already produces. Removing request()/on_command() also drops their rrext_ guard. No privilege change — all 36 rrext_ commands dispatch by name via on_{command} and never reached it; only unknown or misspelled ones did, and those now get the same empty-success fallback as any other unknown command. A new test pins the 36 handlers in place. Refs #1844
asclearuc
added a commit
that referenced
this pull request
Aug 11, 2026
With on_launch and on_terminate relocated, nothing in cmd_debug.py is reachable: the extension's debugger has been disabled since #268 and no live client sends initialize, attach, pause, continue, configurationDone, threads or disconnect. Delete the file, unregister DebugCommands from TaskConn, and drop TaskConn.request() / on_command() — the pair that forwarded unhandled DAP commands to debugpy. on_disconnect is deleted rather than relocated: stripped of its debugger parts it returns build_response(request), which is byte-for-byte what dap_conn's unhandled-command fallback already produces. Removing request()/on_command() also drops their rrext_ guard. No privilege change — all 36 rrext_ commands dispatch by name via on_{command} and never reached it; only unknown or misspelled ones did, and those now get the same empty-success fallback as any other unknown command. A new test pins the 36 handlers in place. Refs #1844
asclearuc
added a commit
that referenced
this pull request
Sep 11, 2026
With on_launch and on_terminate relocated, nothing in cmd_debug.py is reachable: the extension's debugger has been disabled since #268 and no live client sends initialize, attach, pause, continue, configurationDone, threads or disconnect. Delete the file, unregister DebugCommands from TaskConn, and drop TaskConn.request() / on_command() — the pair that forwarded unhandled DAP commands to debugpy. on_disconnect is deleted rather than relocated: stripped of its debugger parts it returns build_response(request), which is byte-for-byte what dap_conn's unhandled-command fallback already produces. Removing request()/on_command() also drops their rrext_ guard. No privilege change — all 36 rrext_ commands dispatch by name via on_{command} and never reached it; only unknown or misspelled ones did, and those now get the same empty-success fallback as any other unknown command. A new test pins the 36 handlers in place. Refs #1844
asclearuc
added a commit
that referenced
this pull request
Sep 14, 2026
With on_launch and on_terminate relocated, nothing in cmd_debug.py is reachable: the extension's debugger has been disabled since #268 and no live client sends initialize, attach, pause, continue, configurationDone, threads or disconnect. Delete the file, unregister DebugCommands from TaskConn, and drop TaskConn.request() / on_command() — the pair that forwarded unhandled DAP commands to debugpy. on_disconnect is deleted rather than relocated: stripped of its debugger parts it returns build_response(request), which is byte-for-byte what dap_conn's unhandled-command fallback already produces. Removing request()/on_command() also drops their rrext_ guard. No privilege change — all 36 rrext_ commands dispatch by name via on_{command} and never reached it; only unknown or misspelled ones did, and those now get the same empty-success fallback as any other unknown command. A new test pins the 36 handlers in place. Refs #1844
asclearuc
added a commit
that referenced
this pull request
Sep 14, 2026
* refactor(ai): relocate on_launch and on_terminate to TaskCommands on_launch is the cloud pipeline-launch path (the SaaS ALB's PUT /task dispatches here) and on_terminate is the SDK's pipeline stop. Neither is debugging; both sat on DebugCommands for historical reasons. Move them ahead of the debugger removal so deleting cmd_debug.py cannot take live production paths with it. Relocation only — task.debug and attach_debugger=True are left as they are, each changed in its own follow-up rather than riding along inside a code move. The debugger-session bookkeeping cannot follow the methods, since TaskCommands holds no _debug_* state: on_launch drops its "already active" guard, stops recording _debug_id/_debug_token, and returns None instead of an 'initialized' event (the real reply already went out via send_response, and its only consumer suppressed it). on_terminate no longer falls back to _debug_token — both live callers pass the token explicitly. The stray-teamId error now matches on_execute's wording. Tests: 5 cases move to test_cmd_task.py rewritten against TaskCommands, 1 is dropped with the guard it covered, 1 is added for the new return contract — no net change to the packages/ai count. Refs #1844 * refactor(ai): delete cmd_debug.py and the debugpy forwarding path With on_launch and on_terminate relocated, nothing in cmd_debug.py is reachable: the extension's debugger has been disabled since #268 and no live client sends initialize, attach, pause, continue, configurationDone, threads or disconnect. Delete the file, unregister DebugCommands from TaskConn, and drop TaskConn.request() / on_command() — the pair that forwarded unhandled DAP commands to debugpy. on_disconnect is deleted rather than relocated: stripped of its debugger parts it returns build_response(request), which is byte-for-byte what dap_conn's unhandled-command fallback already produces. Removing request()/on_command() also drops their rrext_ guard. No privilege change — all 36 rrext_ commands dispatch by name via on_{command} and never reached it; only unknown or misspelled ones did, and those now get the same empty-success fallback as any other unknown command. A new test pins the 36 handlers in place. Refs #1844 * refactor(ai): remove debugpy port passing, node bootstrap and the DAP-over-TCP client Completes the pipeline-debugger removal: the command handlers went in the previous commit, this drops the machinery they drove. task_engine.py no longer assigns a debug port or passes --debug_port / --debug_host / --wait_for_client to the subprocess; node.py loses the argparse block and the debugpy.listen() / wait_for_client() bootstrap that consumed them; dbg_debugpy.py and transport_tcpip.py are deleted, having existed solely to reach the listener node.py no longer starts; and requirements.txt goes with its depends() call, being the repo's only debugpy declaration. Deleting DbgDebugpy forces the rest: TaskDbgDebugpy, _debug_python and its cleanup, Task.attach_task() which existed only to construct it, and in turn TaskServer.attach_task(), the attach gate and the unused attach_debugger parameter. _debug_port, is_debug_available() and _noDebug lose their last writer or reader along with the port passing. Deliberately untouched: the engine->python shim and the C++ setupDebug() thread registration, which serve debugging our own Python under the IDE rather than the pipeline debugger; also task.debug, debuggerAttached in the SDKs, the extension files and the docs. Tests: test_transport_tcpip.py deleted with the transport (23 cases), plus the two tests whose subjects no longer exist. packages/ai 1870 -> 1845. Refs #1844 * fix(ai): bind token before the try in on_terminate The failure log in the except block references `token`, but its assignment lives inside the try — so when get_task_token() itself raises, the handler dies with UnboundLocalError and the real error never surfaces. Pre-existing, carried over verbatim by the relocation in 39a6d85 and caught by review on #1886. Refs #1844 * test(ai): cover the authorization refusal in on_terminate on_terminate gates stopping a task behind get_task's task.control check, but only the allowed path was covered. Adds the negative case: when the lookup raises PermissionError the handler must propagate it and stop_task must never be awaited. Raised by review on #1886. Refs #1844 * test(ai): assert on_terminate checks task.control specifically The mock raised on any get_task call, so the refusal test would have passed even if on_terminate requested the wrong permission — or none. Assert the exact call instead. Raised by review on #1886. Refs #1844 * fix(ai): refuse unhandled DAP commands instead of returning success Removing on_command left TaskConn falling through to DAPConn's default, which builds a SUCCESS response for a command nobody handled. That is a behaviour change the removal did not intend: a misspelled rrext_* in a script, or a stale client still sending the debugger commands, would read success: true and conclude the command worked. Restores a minimal on_command that only refuses. The old error strings are deliberately not restored — 'Invalid command' applied solely to rrext_*, while everything else surfaced 'Task token is required' from the debugpy path, which was an artifact rather than an answer. Nothing in either repo matches on those strings. The test now pins the property rather than the absence of the method. Also corrects three stale lines in eaas.py's docstring: attach and disconnect went with cmd_debug.py, and the proxying claim described the forwarding that was removed. Raised by review on #1886. Refs #1844 * fix(ai): adopt the devTeam rename in the relocated on_launch While this branch was out, #1993 renamed AccountInfo.defaultTeam to devTeam. The relocated on_launch kept the old name, and git merged cmd_task.py without a conflict, so nothing flagged it — every launch would have raised AttributeError. That same commit added a guard beside the rename, in on_execute and in cmd_debug.on_launch, the very function this branch relocates. Carrying it over is not a new rule: leaving it out would have deleted a guard develop has today and let an empty team reach verify_team_permission and the org resolution. The wording is the on_execute variant, since after this branch the path is no longer the debugger. Test maintenance forced by the same drift. Two call sites still passed the helper's old snake_case default_team. And test_rrext_handlers_still_dispatch_by_name pinned an exact handler count that upstream invalidated by consolidating six app handlers into on_rrext_app (37 -> 35); the branch's handler set is byte-identical to develop's, so the count becomes a floor rather than a census. The new test pins that an empty devTeam refuses before the permission check and before start_task — coverage develop has on neither copy of the guard. Gate: 2894 passed, 122 skipped. Refs #1844
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
7 participants
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
This branch is a comprehensive overhaul of how the VS Code extension manages, deploys, and connects to the RocketRide engine. The core theme is giving users full control over the engine lifecycle without leaving VS Code — previously, users had to manually manage processes, services, and Docker containers from the terminal.
1. Dynamic port allocation for local engine
The extension previously hardcoded port 5565 for local engine connections. This meant opening a second VS Code window would fail because the port was already taken. The engine now starts with
--port=0so the OS assigns a free port, and the extension parses the actual port from Uvicorn's startup log output. Each VS Code window gets its own isolated engine process.2. Versioned engine installs with cross-process safety
Engine binaries were previously extracted into a single shared directory. Upgrading while another window was running the engine would overwrite the in-use binary — causing
EBUSYon Windows and race conditions everywhere. Engines are now installed into versioned directories (<root>/engines/<tag>--<hash>/) with:proper-lockfile) to prevent install races between multiple VS Code windowscurrent-stable.json,current-pre.json) to track which version is active per channelcontext.extensionPathtocontext.globalStorageUri.fsPathso engines survive extension updates3. Local OS service deployment — install, start, stop, update, remove
Added a complete "Local Service" deployment path alongside Cloud and Docker. Users can install the RocketRide engine as a native background service that auto-starts on boot, managed entirely from the Deploy page:
The
PageDeployProviderorchestrates the lifecycle by combiningEngineInstaller(download/version management) withServiceManager(OS service registration). Installed version is tracked in a config file (ProgramData/RocketRide/config.json). Status polling runs every 3 seconds with a 60-second timeout when waiting for the service to reach "running" state.The Deploy page UI gained a service status display (color-coded state indicator, version, install path), a split-button with version dropdown for install/update, start/stop/remove controls, and progress/error feedback streamed from the provider.
4. Docker container lifecycle management
The previous Docker deployment was fire-and-forget: it opened a terminal, ran
docker pullanddocker create, and the user had no further control from VS Code. This replaces it with a proper container manager mirroring the OS service panel:DockerManagerclass using thedockerodeSDK for install, start, stop, remove, update, and status operations. Handles Windows named-pipe detection (Docker Desktop vs standalone Docker) and maps Docker API errors to user-friendly messages.ssh2stub (src/stubs/ssh2.js) with an esbuild alias so thatdocker-modem(a dockerode transitive dependency) doesn't pull in native.nodebinaries. We only connect via local socket/named pipe, never SSH — the stub satisfies therequire('ssh2').Clientreference at zero cost.docker-config.json) to track which version was installed, since Docker container inspect doesn't carry that metadata.renderSplitButton,renderInstalledActions,renderStatusIndicator) were refactored to be reusable across both panels.5. Debug trace propagation and DAP logging
--trace=servicePythoninto engine args. One toggle replaces manual arg editing. The checkbox respects user overrides: if the user already typed--trace=in the Server Arguments field, the checkbox value is skipped to avoid duplicate-flag rejections from the C++ argparser.task_engine.pynow inherits the parent engine's--tracesetting fromsys.argvwhen DAP args don't include one, so subprocess pipelines always get trace config. User args with spaces are safely split viashlex.split().ConnectionManager— all commands (execute,launch, etc.) and their full payloads are now visible in the "RocketRide: Extension" output panel. Previously these were completely silent, making protocol-level debugging impossible.node.py.6. Settings and UI refinements
adapter.tscontains a restoration guide. The debugger was adding activation overhead and user confusion while non-functional during rework.string[]array editor to a singlestringtext input.openStatus,openFile,runPipeline,stopPipeline,openPipelineAsText,page.status.open,setupCredentials).rocketride.provider.connection.focusandrocketride.provider.files.focus.rgba(255,255,255,...)values for consistent appearance on the dark branded background.onprem.svgreplaced with a server/gear icon that better represents local service deployment.text-overflow: ellipsis, better suited for streaming Docker pull output.Test plan
--trace=servicePythonappears in engine output and child processes inherit itSummary by CodeRabbit
New Features
UI / Pages
Settings
Chores