feat: migrate from VSCode extension to standalone web app#620
Open
ABAPPLO wants to merge 3 commits intobreaking-brake:mainfrom
Open
feat: migrate from VSCode extension to standalone web app#620ABAPPLO wants to merge 3 commits intobreaking-brake:mainfrom
ABAPPLO wants to merge 3 commits intobreaking-brake:mainfrom
Conversation
…ebview bridge - Create packages/core with platform-agnostic interfaces (IFileSystem, IMessageTransport, IWorkflowProvider, ILogger, IDialogService) - Move shared types and validation utils to @cc-wf-studio/core - Refactor McpServerManager to support both UI mode (transport) and headless mode (workflow provider) - Add platform-agnostic FileService, NodeFileSystem, ConsoleLogger, and FileSystemWorkflowProvider - Create webview bridge abstraction (IHostBridge) with VSCode and Electron adapters - Refactor main.tsx to auto-detect VSCode/Electron/Dev environment - Add Electron desktop app shell with IPC handlers, theme variables, and preload script - Add standalone MCP server CLI entry point (cc-wf-mcp-server) - Create VSCode adapters (VSCodeFileSystem, VSCodeMessageTransport, VSCodeLogger) - Add npm workspaces configuration and tsconfig.base.json
- Add standalone-theme.css with 75 --vscode-* CSS variable defaults (dark/light) - Import theme in main.tsx as fallback for Web/Dev mode - Fix scrollbar track using same color as thumb, add body color transition - Add Multi-Platform Support, Web Mode, and Project Structure to README
- Add Hono web server with WebSocket message routing (packages/web-server/) - Add web bridge adapter for browser-to-server communication via WebSocket - Port all 65+ message handlers from VSCode extension to web server - Add encrypted file-based secret store replacing VSCode secrets API - Add shell-exec service replacing VSCode terminal API - Remove src/extension/ (21 command handlers, 48 services) - Remove packages/electron/ and packages/vscode/ - Update build scripts, tsconfig, and CLAUDE.md for web architecture
📝 WalkthroughWalkthroughThe PR migrates from a VSCode extension architecture to a standalone web application with a Node.js Hono web server backend and shared core package. It removes all extension-specific code, establishes a monorepo workspace structure with Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (React)
participant WS as WebSocket Handler
participant Handler as Domain Handler
participant Service as Service (File/MCP/Slack)
participant FS as File System / External API
Browser->>WS: Message (e.g., SAVE_WORKFLOW)
WS->>Handler: Route to handler
Handler->>Service: Execute operation
alt Success Path
Service->>FS: Read/Write/Query
FS-->>Service: Result
Service-->>Handler: Success result
Handler-->>WS: Reply with data
WS-->>Browser: SAVE_SUCCESS / operation response
else Error Path
Service->>FS: Operation fails
FS-->>Service: Error
Service-->>Handler: Error
Handler-->>WS: Error reply
WS-->>Browser: ERROR with code/message
end
sequenceDiagram
participant CLI as CLI / MCP Client
participant CoreService as Core Service (MCP Manager)
participant Transport as Transport (IMessageTransport)
participant Provider as Workflow Provider
CLI->>CoreService: Initialize with transport or provider
alt UI Mode (WebSocket Transport)
CoreService->>Transport: setTransport(IMessageTransport)
CoreService->>Transport: postMessage (request workflow/apply)
Transport-->>CoreService: Handle response
else Headless Mode (File-based Provider)
CoreService->>Provider: setWorkflowProvider(IWorkflowProvider)
CoreService->>Provider: getCurrentWorkflow()
Provider-->>CoreService: Return workflow
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate-editing-flow.ts (1)
217-218:⚠️ Potential issue | 🟡 MinorEnsure the output directory exists before writing.
The script writes to
packages/core/src/generated/without first ensuring the directory exists. Sincefs.writeFiledoes not create parent directories automatically, the build will fail with an ENOENT error if this directory doesn't exist.📁 Proposed fix to create directory before writing
+ // Ensure output directory exists + await fs.mkdir(path.dirname(OUTPUT_PATH), { recursive: true }); + // Write output file await fs.writeFile(OUTPUT_PATH, tsContent, 'utf-8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-editing-flow.ts` around lines 217 - 218, Before calling fs.writeFile with OUTPUT_PATH, ensure its parent directory exists by creating it recursively: compute the directory via path.dirname(OUTPUT_PATH) and call fs.promises.mkdir(..., { recursive: true }) (or await fs.mkdir with recursive true) and only then write tsContent with fs.writeFile; update the same function that currently uses OUTPUT_PATH and tsContent so the mkdir call precedes the write to avoid ENOENT.
🟠 Major comments (17)
README.md-63-72 (1)
63-72:⚠️ Potential issue | 🟠 MajorUpdate docs to reflect the actual post-migration architecture.
Line 63 onward still documents a 3-mode model (VSCode/Electron/Web), and Lines 89-96 list
packages/electron/,packages/vscode/, andsrc/extension/. This conflicts with the PR’s standalone web-app migration and removed VSCode/Electron paths, so onboarding instructions are now misleading.Suggested README correction
-## Multi-Platform Support - -CC Workflow Studio runs in three modes: +## Runtime Model + +CC Workflow Studio runs as a standalone web application: | Mode | Description | Use Case | |------|-------------|----------| -| **VSCode Extension** | Install from [VS Marketplace](https://marketplace.visualstudio.com/items?itemName=breaking-brake.cc-wf-studio) or [OpenVSX](https://open-vsx.org/extension/breaking-brake/cc-wf-studio) | Primary usage — full integration with editor | -| **Electron Desktop App** | Standalone desktop application via `packages/electron/` | Offline use without VSCode | -| **Web Browser (Dev)** | Run with Vite dev server | Development and quick preview | +| **Web App** | React web UI + Node.js/Hono backend over WebSocket | Primary usage | +| **Web Browser (Dev)** | Vite dev server for UI iteration | Local development | ## Project Structure -``` +```text cc-wf-studio/ ├── packages/ │ ├── core/ # Shared backend logic (platform-agnostic) -│ ├── electron/ # Electron desktop app -│ └── vscode/ # VSCode extension adapter +│ └── web-server/ # Hono HTTP/WebSocket backend ├── src/ -│ ├── extension/ # VSCode Extension Host -│ └── webview/ # React UI (shared across all platforms) +│ └── webview/ # React UI └── resources/ # Icons, schemas</details> Also applies to: 87-97 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 63 - 72, Update the "Multi-Platform Support" section
and any other occurrences (e.g., the block around lines 87–97) to reflect the
post-migration standalone web-app architecture: replace the three-mode table
(VSCode Extension / Electron Desktop App / Web Browser (Dev)) with a single
web-app description, remove references to packages/electron/ and
packages/vscode/ and the src/extension/ entry, and replace the repository tree
with the suggested structure (packages/core, packages/web-server, src/webview,
resources) and a short note that the app is a Hono HTTP/WebSocket backend +
React webview; ensure the README headings "Multi-Platform Support" and the
package listing are consistent with these new names and paths.</details> </blockquote></details> <details> <summary>packages/web-server/src/adapters/ws-message-transport.ts-21-23 (1)</summary><blockquote> `21-23`: _⚠️ Potential issue_ | _🟠 Major_ **`postMessage` silently drops messages when no sender is attached.** At Line 21–Line 23, missing sender leads to data loss with no signal to callers; this can break request/response flows during reconnect windows. <details> <summary>Suggested minimal fail-fast fix</summary> ```diff postMessage(message: { type: string; requestId?: string; payload?: unknown }): void { - this.sender?.(message); + if (!this.sender) { + throw new Error('WebSocket sender is not attached'); + } + this.sender(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/adapters/ws-message-transport.ts` around lines 21 - 23, postMessage currently drops messages when this.sender is undefined; change postMessage in ws-message-transport.ts (the postMessage method that uses this.sender) to fail-fast: if this.sender is not set, throw a descriptive Error (include context like message.type and message.requestId) instead of silently returning, otherwise call this.sender(message); this ensures callers are signaled about missing sender/connection during reconnect windows.packages/web-server/package.json-21-21 (1)
21-21:⚠️ Potential issue | 🟠 MajorUse
@types/node@^22to match the runtime baseline targeted in CI.At Line 21,
@types/node@^25.0.3is misaligned with the project's runtime targets. The@types/nodemajor version tracks the Node.js major version for which it provides type definitions—@types/node@25.xis designed for Node.js 25.x, not Node 18.x. This creates a type-safety gap: developers can write code using Node 25 APIs that will pass type-checking but fail at runtime in your Node 18+ and CI Node 22 environments.Change to
@types/node@^22to match CI's Node 22 target, or use@types/node@^18if strictly adhering to the minimumengines.noderequirement. Verify alignment across all packages in this monorepo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/package.json` at line 21, Update the dev dependency entry for the TypeScript Node definitions in package.json: replace the current "@types/node": "^25.0.3" entry with "@types/node": "^22" so the types align with the CI/runtime baseline (Node 22); ensure the change is made in packages/web-server's package.json and, after updating the version string, run your normal install/check workflow and verify other packages in the monorepo use a compatible `@types/node` major version.packages/web-server/src/services/secret-store.ts-26-30 (1)
26-30:⚠️ Potential issue | 🟠 MajorWeak key derivation undermines encryption security and decrypt lacks validation.
The encryption key is derived solely from
hostname+usernamewith a single SHA-256 hash and no salt. This means:
- Any process running as the same user can derive the identical key
- If
secrets.jsonis copied, the key can be reconstructed from public machine infoAdditionally, the
decrypt()method (lines 42-51) doesn't validate the ciphertext format before splitting. If a ciphertext is malformed,split(':')may return fewer than 3 elements, causingBuffer.from()to receive undefined values.While the file permissions (0o600) provide OS-level access control, proper secrets encryption requires:
- A key derivation function with salt and high iteration count (e.g., PBKDF2 with 600K+ iterations + random salt)
- Or, use OS-level secure storage via
keytar(integrates with native keystores: Keychain on macOS, Credential Manager on Windows, Secret Service on Linux)- Or, require a user-provided passphrase for key derivation
Validate the
decrypt()input format before processing to prevent silent failures from malformed data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/services/secret-store.ts` around lines 26 - 30, Replace the weak single-hash key derivation in getKey() with a proper KDF or OS keystore: either derive the key using crypto.pbkdf2Sync with a random per-installation salt (store salt alongside secrets.json) and a high iteration count (e.g., 600k+) and a secure length, or switch to using keytar to retrieve/store the encryption key from the OS secure store; ensure getKey() returns the derived key from that secure source. In decrypt(), validate the ciphertext format before splitting (ensure it contains the expected two ':' separators and non-empty iv/tag/ciphertext parts), handle malformed input by throwing a clear error, and avoid passing undefined to Buffer.from(); reference the getKey() and decrypt() functions when implementing these fixes.packages/core/src/utils/path-utils.ts-49-51 (1)
49-51:⚠️ Potential issue | 🟠 MajorAntigravity user skill directory is inconsistent with the shared contract.
Line [50] points to
~/.gemini/antigravity/skills, while the contract comments inpackages/core/src/types/messages.tsdocument Antigravity user skills under~/.agent/skills. This mismatch can break discovery/export parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/path-utils.ts` around lines 49 - 51, The function getAntigravityUserSkillsDir currently returns "~/.gemini/antigravity/skills" which conflicts with the documented contract in packages/core/src/types/messages.ts; update getAntigravityUserSkillsDir to return the path under "~/.agent/skills" (e.g. path.join(os.homedir(), '.agent', 'skills')) and run/update any callers/tests that rely on the previous path to maintain discovery/export parity.packages/core/src/utils/validate-workflow.ts-150-153 (1)
150-153:⚠️ Potential issue | 🟠 MajorHooks are validated from the wrong property path.
Lines [150]-[153] validate
rawWf.hooks, but the model places hooks atslashCommandOptions.hooks. Current logic can skip validation for real hook payloads.🛠️ Suggested fix
- const rawWf = workflow as Record<string, unknown>; - if (rawWf.hooks) { - const hooksErrors = validateHooks(rawWf.hooks as WorkflowHooks); + const slashCommandOptions = (workflow as Partial<Workflow>).slashCommandOptions; + if (slashCommandOptions?.hooks) { + const hooksErrors = validateHooks(slashCommandOptions.hooks as WorkflowHooks); errors.push(...hooksErrors); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/validate-workflow.ts` around lines 150 - 153, The code validates hooks from rawWf.hooks but the model stores them under slashCommandOptions.hooks, so update the validation to read hooks from rawWf.slashCommandOptions?.hooks (or cast rawWf.slashCommandOptions as WorkflowSlashCommandOptions) and pass that into validateHooks; specifically, replace the rawWf.hooks check with a guard that checks rawWf.slashCommandOptions?.hooks and call validateHooks(rawWf.slashCommandOptions.hooks as WorkflowHooks) so real hook payloads are validated.packages/core/src/types/messages.ts-1913-1928 (1)
1913-1928:⚠️ Potential issue | 🟠 MajorType guards lack directional discrimination.
Lines [1913]-[1928] implement identical checks that only verify
{ type: string }, accepting any object regardless of direction.ExtensionMessageandWebviewMessagehave distinct literal type values that can distinguish them.Update each guard to verify against its respective allowed types:
isExtensionMessageshould check ifmessage.typeis one of: 'LOAD_WORKFLOW', 'SAVE_SUCCESS', 'EXPORT_SUCCESS', 'ERROR', 'WORKFLOW_LIST_LOADED', 'INITIAL_STATE', 'PREVIEW_MODE_INIT', 'PREVIEW_UPDATE', 'PREVIEW_PARSE_ERROR', 'SAVE_CANCELLED', 'EXPORT_CANCELLED', 'SKILL_LIST_LOADED', 'SKILL_CREATION_SUCCESS', 'SKILL_CREATION_FAILED', 'SKILL_VALIDATION_SUCCESS'isWebviewMessageshould check ifmessage.typeis one of: 'SAVE_WORKFLOW', 'EXPORT_WORKFLOW', 'CONFIRM_OVERWRITE', 'LOAD_WORKFLOW_LIST', 'LOAD_WORKFLOW', 'STATE_UPDATE', 'ACCEPT_TERMS', 'CANCEL_TERMS', 'BROWSE_SKILLS', 'CREATE_SKILL', 'VALIDATE_SKILL_FILE', 'REFINE_WORKFLOW', 'CANCEL_REFINEMENT', 'CLEAR_CONVERSATION', 'LIST_MCP_SERVERS'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/types/messages.ts` around lines 1913 - 1928, The current type guards isExtensionMessage and isWebviewMessage only check for a { type: string } shape and therefore accept messages of the wrong direction; update each guard to keep the existing object/null/typeof checks but additionally verify message.type is one of the allowed literal strings for that direction: for isExtensionMessage ensure message.type is one of 'LOAD_WORKFLOW','SAVE_SUCCESS','EXPORT_SUCCESS','ERROR','WORKFLOW_LIST_LOADED','INITIAL_STATE','PREVIEW_MODE_INIT','PREVIEW_UPDATE','PREVIEW_PARSE_ERROR','SAVE_CANCELLED','EXPORT_CANCELLED','SKILL_LIST_LOADED','SKILL_CREATION_SUCCESS','SKILL_CREATION_FAILED','SKILL_VALIDATION_SUCCESS'; for isWebviewMessage ensure message.type is one of 'SAVE_WORKFLOW','EXPORT_WORKFLOW','CONFIRM_OVERWRITE','LOAD_WORKFLOW_LIST','LOAD_WORKFLOW','STATE_UPDATE','ACCEPT_TERMS','CANCEL_TERMS','BROWSE_SKILLS','CREATE_SKILL','VALIDATE_SKILL_FILE','REFINE_WORKFLOW','CANCEL_REFINEMENT','CLEAR_CONVERSATION','LIST_MCP_SERVERS'; use a set/array inclusion check against message.type to perform the discrimination while preserving the existing type predicate signatures.packages/core/src/utils/workflow-validator.ts-97-100 (1)
97-100:⚠️ Potential issue | 🟠 MajorRemove double-cast to unsafe
Workflowtype without full validation.Line 99 uses
workflow as unknown as Workflowto bypass type safety. The function only validates 5 required fields (id, name, version, nodes, connections) but returnsvalid: truefor objects missing createdAt and updatedAt—both required non-optional Date fields in the Workflow interface. This allows malformed workflow objects to propagate as valid.Either validate all required Workflow fields before casting, or return a narrower type (e.g.,
Record<string, unknown>) that matches what the function actually checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/workflow-validator.ts` around lines 97 - 100, The function in workflow-validator.ts currently returns workflow as unknown as Workflow (removing type safety) while only checking id, name, version, nodes, connections; remove the unsafe double-cast and either (a) extend the validator to assert createdAt and updatedAt exist and are valid Dates (or parseable ISO strings) and normalize them before returning a Workflow, or (b) change the return type to a narrower shape (e.g., Record<string, unknown> or a ValidatedPartialWorkflow interface) that matches the actual checks; update the return to either construct and return a fully validated Workflow object or return the narrower type so callers cannot assume createdAt/updatedAt are present (refer to the workflow variable and the Workflow type in the file and adjust the function signature accordingly).packages/core/src/types/workflow-definition.ts-77-83 (1)
77-83:⚠️ Potential issue | 🟠 MajorMake
HookActiona discriminated union to properly constraincommandfield.The current interface requires
command: stringfor all actions, but your validator only checkscommandwhentype === 'command'. This creates a type contract mismatch with runtime behavior and contradicts the documented intent in your comments.Suggested fix
-export interface HookAction { - /** Hook type: 'command' for shell commands, 'prompt' for LLM-based (Stop only) */ - type: 'command' | 'prompt'; - /** Shell command to execute (required for type: 'command') */ - command: string; - /** Run hook only once per session (optional) */ - once?: boolean; -} +export type HookAction = + | { + type: 'command'; + command: string; + once?: boolean; + } + | { + type: 'prompt'; + once?: boolean; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/types/workflow-definition.ts` around lines 77 - 83, The HookAction interface is not a discriminated union so it incorrectly requires command for all types; change it to a proper discriminated union (e.g., CommandHook { type: 'command'; command: string; once?: boolean } and PromptHook { type: 'prompt'; once?: boolean }) and export HookAction = CommandHook | PromptHook; update any code that constructs or narrows HookAction (validator, type guards, or switch on action.type) to expect the command field only on the CommandHook variant and adjust usages that assumed command always exists (e.g., places referencing action.command or destructuring action) to first check action.type or use a type guard.packages/web-server/src/handlers/workflow-handlers.ts-19-20 (1)
19-20:⚠️ Potential issue | 🟠 Major
overwriteExistingis currently ignored, so exports always overwrite files.Line 19 exposes
overwriteExisting?: boolean, but no write path checks it beforewriteFile.Also applies to: 39-41, 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/handlers/workflow-handlers.ts` around lines 19 - 20, The handler currently ignores payload.overwriteExisting so exports always overwrite files; update the export/write paths in workflow-handlers.ts (where payload: { workflow: Record<string, unknown>; overwriteExisting?: boolean } is used and writeFile is called) to check payload.overwriteExisting before writing: if overwriteExisting is false or undefined, detect existing target files (using fs.stat/exists or equivalent) and either skip the write and return a clear error/response or throw a controlled error; if overwriteExisting is true, proceed with writeFile as before. Apply this same check to the other export/write sites noted (the uses around the payload at the other occurrences) so all writeFile calls honor overwriteExisting.packages/web-server/src/handlers/workflow-handlers.ts-28-29 (1)
28-29:⚠️ Potential issue | 🟠 MajorValidate untyped payloads before array/object access.
The casts on Line 28 and Line 50 do not runtime-validate data. Malformed payloads can throw on
nodes.filter(...)ordata.label.Suggested fix
-const nodes = (workflow.nodes as Array<Record<string, unknown>>) || []; +const nodesRaw = workflow.nodes; +const nodes = Array.isArray(nodesRaw) ? (nodesRaw as Array<Record<string, unknown>>) : []; @@ -const data = node.data as Record<string, unknown>; +const data = + node.data && typeof node.data === 'object' + ? (node.data as Record<string, unknown>) + : {};Also applies to: 44-45, 50-52, 88-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/handlers/workflow-handlers.ts` around lines 28 - 29, Validate untyped payloads from workflow before accessing properties: ensure workflow.nodes is an Array (e.g., guard before assigning nodes) and only call nodes.filter/map after that check; validate each node/data is a plain object and that expected fields (e.g., data.label) exist and are of the correct type (string) before using them; apply the same runtime checks around the code referencing nodes, data, exportedFiles and any places noted (around the usages at the earlier nodes assignment and the blocks corresponding to lines 44-45, 50-52, 88-90) so malformed payloads are handled gracefully (returning/throwing a clear error or skipping invalid entries).packages/core/src/cli/mcp-server-cli.ts-44-51 (1)
44-51:⚠️ Potential issue | 🟠 MajorMake shutdown idempotent and failure-safe.
On Line 44–Line 51, repeated signals can trigger concurrent
manager.stop()calls, and failures in shutdown are not handled.Suggested fix
- const shutdown = async (): Promise<void> => { - logger.info('Shutting down MCP server...'); - await manager.stop(); - process.exit(0); - }; - - process.on('SIGINT', shutdown); - process.on('SIGTERM', shutdown); + let shuttingDown = false; + const shutdown = async (signal: 'SIGINT' | 'SIGTERM'): Promise<void> => { + if (shuttingDown) return; + shuttingDown = true; + logger.info(`Received ${signal}. Shutting down MCP server...`); + try { + await manager.stop(); + process.exit(0); + } catch (error) { + logger.error('Failed to stop MCP server cleanly', error); + process.exit(1); + } + }; + + process.once('SIGINT', () => { + void shutdown('SIGINT'); + }); + process.once('SIGTERM', () => { + void shutdown('SIGTERM'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/cli/mcp-server-cli.ts` around lines 44 - 51, The shutdown handler can be invoked multiple times and currently allows concurrent manager.stop() calls and uncaught failures; make shutdown idempotent by adding a shared flag (e.g., isShuttingDown) checked at the top of the shutdown function, wrap the await manager.stop() in try/catch to handle/retry or log errors, ensure process.exit is only called once with a non-zero exit code on failure, and keep the same signal registrations (process.on('SIGINT', shutdown) and process.on('SIGTERM', shutdown)) so repeated signals no-op while a shutdown is in progress.packages/core/src/adapters/node-file-system.ts-23-29 (1)
23-29:⚠️ Potential issue | 🟠 MajorDistinguish ENOENT from other filesystem errors in
fileExists.The bare
catchblock on line 27 masks permission and I/O errors as "file not found." This causesws-handler.tsline 221 to report permission failures asLOAD_FAILEDwith message "Workflow not found," which is misleading and prevents proper error diagnosis.Only ENOENT errors indicate the file doesn't exist; other errors (EACCES, EIO, etc.) should be re-thrown to allow proper error handling upstream.
Suggested fix
async fileExists(filePath: string): Promise<boolean> { try { await fs.access(filePath); return true; - } catch { - return false; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return false; + } + throw error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/adapters/node-file-system.ts` around lines 23 - 29, In fileExists, don't swallow all filesystem errors; when calling fs.access inside the fileExists(filePath: string) method, catch the error and only return false if the error.code === 'ENOENT', otherwise re-throw the error so permission (EACCES) and I/O errors bubble up to callers (e.g., ws-handler.ts) for proper handling; use the fs.access call and the caught error's code field to distinguish ENOENT from other errors.packages/core/src/services/file-service.ts-56-58 (1)
56-58:⚠️ Potential issue | 🟠 MajorPath traversal risk in
getWorkflowFilePath.The
workflowNameparameter is used directly in path construction without sanitization. A malicious name like../../etc/passwdcould escape the workflows directory.🛡️ Proposed fix to sanitize workflow name
getWorkflowFilePath(workflowName: string): string { + // Sanitize workflow name to prevent path traversal + const sanitized = workflowName.replace(/[\/\\:*?"<>|]/g, '-').replace(/\.\./g, ''); - return path.join(this.workflowsDirectory, `${workflowName}.json`); + return path.join(this.workflowsDirectory, `${sanitized}.json`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/file-service.ts` around lines 56 - 58, getWorkflowFilePath uses workflowName directly which allows path traversal; sanitize and validate the input before joining: strip or reject path separators (e.g., use path.basename(workflowName) or validate against a safe regex like /^[a-zA-Z0-9_-]+$/), then build the path and canonicalize it (path.resolve) and verify the resolved path starts with the workflowsDirectory resolved path to prevent escaping; update getWorkflowFilePath to perform these checks and throw an error for invalid names, referencing getWorkflowFilePath and this.workflowsDirectory.packages/web-server/src/routes/ws-handler.ts-59-70 (1)
59-70:⚠️ Potential issue | 🟠 MajorState is shared across all WebSocket connections.
createState()is called once insetupWebSocketHandler, meaning all WebSocket connections share the samefileService,dialogService,transport, andmcpManager. This may cause issues if multiple clients connect simultaneously, as they would share dialog state and transport sender.Consider whether this is intentional (single-user web app) or if state should be per-connection. If single-user is intended, document this limitation. Otherwise, move state creation inside the returned function:
♻️ Per-connection state alternative
export function setupWebSocketHandler(): (c: unknown) => WSEvents { - const state = createState(); - return (_c: unknown) => { + const state = createState(); let ws: WSContext | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/routes/ws-handler.ts` around lines 59 - 70, The current implementation calls createState() once at module-level inside setupWebSocketHandler(), causing a single shared state (fileService, dialogService, transport, mcpManager) to be reused across all WebSocket connections; to fix, move the createState() invocation inside the returned connection factory (the inner function that currently defines ws) so each call to the returned function gets its own state instance, ensuring per-connection fileService/dialogService/transport/mcpManager isolation; update any references in the inner function to use the new local state variable and, if shared state was intentional, instead add a clear comment in setupWebSocketHandler documenting the single-client limitation.packages/web-server/src/routes/ws-handler.ts-156-169 (1)
156-169:⚠️ Potential issue | 🟠 MajorMissing validation before using
workflow.namein file path.The workflow name is cast directly without validation. Combined with the path traversal issue in
FileService.getWorkflowFilePath, this could allow writing to arbitrary paths.🛡️ Proposed validation
case 'SAVE_WORKFLOW': { if (!fileService || !payload?.workflow) { reply('ERROR', { code: 'VALIDATION_ERROR', message: 'Workflow is required' }); break; } const workflow = payload.workflow as Record<string, unknown>; + const workflowName = workflow.name; + if (typeof workflowName !== 'string' || !workflowName.trim()) { + reply('ERROR', { code: 'VALIDATION_ERROR', message: 'Workflow name is required' }); + break; + } await fileService.ensureWorkflowsDirectory(); - const filePath = fileService.getWorkflowFilePath(workflow.name as string); + const filePath = fileService.getWorkflowFilePath(workflowName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/routes/ws-handler.ts` around lines 156 - 169, The SAVE_WORKFLOW handler uses workflow.name without validation when calling fileService.getWorkflowFilePath, which allows path traversal; validate and sanitize before use: ensure payload.workflow exists and workflow.name is a non-empty string, reject or reply with a VALIDATION_ERROR if missing or invalid, enforce a whitelist (e.g., alphanumeric, dashes/underscores) or normalize and compare to path.basename to prevent ../ segments, and only then call fileService.getWorkflowFilePath/get/write; update the SAVE_WORKFLOW case to perform these checks and return an error reply when the name fails validation.packages/web-server/src/handlers/mcp-handlers.ts-159-162 (1)
159-162:⚠️ Potential issue | 🟠 MajorIncompatible Node.js version requirement with
import.meta.dirnamefallback.The project requires Node.js >= 18.0.0, but
import.meta.dirnameis only available in Node.js 20.11.0+. On Node.js 18.x, 19.x, or 20.0-20.10, the fallbackpath.dirname(new URL(import.meta.url).pathname)will be used. This fallback may fail on Windows becauseimport.meta.urlcontains afile://URL with pathname/C:/path/...(leading slash), which can cause path resolution issues when passed topath.resolve().Either:
- Update package.json to require
"node": ">=20.11.0", or- Fix the fallback to handle Windows file paths correctly (strip leading slash on Windows)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/handlers/mcp-handlers.ts` around lines 159 - 162, The current corePath calculation uses import.meta.dirname with a fallback path.dirname(new URL(import.meta.url).pathname) which breaks on Windows; replace the fallback with a safe conversion using fileURLToPath(import.meta.url) (from the 'url' module) so corePath = path.resolve(import.meta.dirname ?? path.dirname(fileURLToPath(import.meta.url)), '../../../core'); alternatively if you prefer to require newer Node, update package.json "engines.node" to ">=20.11.0" and keep import.meta.dirname; reference symbols: corePath, import.meta.dirname, import.meta.url, and path.dirname.
🟡 Minor comments (7)
README.md-87-87 (1)
87-87:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced code block.
Line 87 uses a bare fenced block; markdownlint MD040 expects a language identifier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 87, The README contains a bare fenced code block (```); to satisfy markdownlint MD040, edit that fenced code block by adding an appropriate language identifier immediately after the opening backticks (for example, bash, js, or markdown depending on the snippet) so the block becomes ```<language>; locate the bare ``` block in the README and update it accordingly.packages/web-server/src/services/web-dialog-service.ts-43-65 (1)
43-65:⚠️ Potential issue | 🟡 MinorTimeout is not cleared when dialog resolves normally, leading to potential resource leak.
When
handleDialogResponseresolves the promise before the 60-second timeout, thesetTimeoutcontinues running and will attempt to delete an already-deleted entry and callresolve(false)on an already-resolved promise.While not immediately breaking (calling resolve twice is a no-op), it's cleaner to clear the timeout.
🛡️ Proposed fix to clear timeout on response
async showConfirmDialog(message: string, confirmLabel: string): Promise<boolean> { if (!this.sender) return false; const requestId = `dialog-${Date.now()}-${Math.random().toString(36).slice(2)}`; return new Promise<boolean>((resolve) => { - this.pendingDialogs.set(requestId, { resolve, reject: () => resolve(false) }); + const timeoutId = setTimeout(() => { + if (this.pendingDialogs.has(requestId)) { + this.pendingDialogs.delete(requestId); + resolve(false); + } + }, 60000); + + this.pendingDialogs.set(requestId, { + resolve: (value: boolean) => { + clearTimeout(timeoutId); + resolve(value); + }, + reject: () => resolve(false), + }); this.sender?.({ type: 'SHOW_CONFIRM_DIALOG', requestId, payload: { message, confirmLabel }, }); - - // Auto-resolve after 60 seconds if no response - setTimeout(() => { - if (this.pendingDialogs.has(requestId)) { - this.pendingDialogs.delete(requestId); - resolve(false); - } - }, 60000); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/services/web-dialog-service.ts` around lines 43 - 65, The showConfirmDialog function creates a 60s setTimeout but never clears it when the dialog is resolved; update showConfirmDialog to capture the timeout ID (e.g., const timeoutId = setTimeout(...)) and store it in the pendingDialogs entry alongside resolve/reject, and then modify handleDialogResponse (the method that resolves/rejects pending dialogs) to clearTimeout(timeoutId) before deleting the pendingDialogs entry and calling resolve/reject; also ensure the timeout handler clears the entry and does not try to clear an already-cleared timeout. This change touches showConfirmDialog, the pendingDialogs map shape, and handleDialogResponse to properly clear timeouts and avoid resource leaks.packages/web-server/src/routes/ws-handler.ts-506-511 (1)
506-511:⚠️ Potential issue | 🟡 MinorNo type validation for
SET_REVIEW_BEFORE_APPLYpayload.The value is cast directly to boolean without checking its actual type. A non-boolean value would be coerced, potentially causing unexpected behavior.
🛡️ Add type check
case 'SET_REVIEW_BEFORE_APPLY': { - if (payload != null && state.mcpManager) { - state.mcpManager.setReviewBeforeApply(payload.value as boolean); + if (payload != null && state.mcpManager && typeof payload.value === 'boolean') { + state.mcpManager.setReviewBeforeApply(payload.value); } break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/routes/ws-handler.ts` around lines 506 - 511, The SET_REVIEW_BEFORE_APPLY switch branch currently casts payload.value to boolean and calls state.mcpManager.setReviewBeforeApply without validating the type; add a guard to ensure payload is not null and typeof payload.value === 'boolean' before invoking setReviewBeforeApply (and otherwise skip the call or log a warning/error), referencing the SET_REVIEW_BEFORE_APPLY case, payload.value, and state.mcpManager.setReviewBeforeApply to locate where to add the check.packages/web-server/src/handlers/slack-handlers.ts-184-189 (1)
184-189:⚠️ Potential issue | 🟡 MinorMissing error handling for file download and JSON parsing.
The fetch response isn't checked for success status before parsing. A non-2xx response would cause
JSON.parseto throw a confusing error. Also,JSON.parsecan throw on invalid JSON.🐛 Proposed fix for robust error handling
// Download file content const response = await fetch(file.url_private, { headers: { Authorization: `Bearer ${token}` }, }); +if (!response.ok) { + throw new Error(`Failed to download file: ${response.status} ${response.statusText}`); +} const content = await response.text(); -const workflow = JSON.parse(content); +let workflow; +try { + workflow = JSON.parse(content); +} catch (parseError) { + throw new Error('Downloaded file is not valid JSON'); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/handlers/slack-handlers.ts` around lines 184 - 189, Add robust error handling around the file download and JSON parsing in slack-handlers.ts: after calling fetch(file.url_private, { headers: { Authorization: `Bearer ${token}` } }) check response.ok and handle non-2xx responses (log and throw or return an error) before calling response.text(); then wrap JSON.parse(content) in a try/catch to catch and log parse errors (include the raw content and the error) and propagate a clear error instead of letting a cryptic exception bubble up; update any callers to handle the thrown error accordingly.packages/web-server/src/handlers/skill-handlers.ts-155-158 (1)
155-158:⚠️ Potential issue | 🟡 MinorScope detection logic doesn't correctly identify user scope.
The condition
normalizedPath.includes('/.claude/skills')matches project paths but doesn't distinguish user home directory. A path like/home/user/.claude/skills/foo.mdwould also match but should be'user'scope.🐛 Proposed fix for scope detection
const normalizedPath = skillPath.replace(/\\/g, '/'); -const scope: 'user' | 'project' | 'local' = normalizedPath.includes('/.claude/skills') - ? 'project' - : 'user'; +const homedir = os.homedir().replace(/\\/g, '/'); +let scope: 'user' | 'project' | 'local'; +if (normalizedPath.startsWith(homedir + '/.claude/skills')) { + scope = 'user'; +} else if (normalizedPath.includes('/.claude/skills')) { + scope = 'project'; +} else { + scope = 'local'; +}You'll need to add
import os from 'node:os';at the top if not already present (it is imported at line 10).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/handlers/skill-handlers.ts` around lines 155 - 158, The current scope detection using normalizedPath.includes('/.claude/skills') misclassifies user-home skills as project; update the logic around the scope variable so that after computing normalizedPath from skillPath you check if it contains '/.claude/skills' and then differentiate user vs project by comparing against the user's home directory (use os.homedir(), normalizing slashes the same way as normalizedPath) — if normalizedPath startsWith the normalized homeDir + '/.claude/skills' set scope = 'user', otherwise 'project'; ensure node:os is imported (import os from 'node:os') if not already present and keep references to normalizedPath, skillPath, and scope to locate the change.packages/web-server/src/handlers/ai-handlers.ts-395-403 (1)
395-403:⚠️ Potential issue | 🟡 MinorCLI errors resolve instead of rejecting, hiding failures.
When
child.on('error')fires, the promise resolves with an empty output rather than rejecting. This masks errors like "command not found" and makes debugging difficult.🐛 Proposed fix to propagate errors
-child.on('error', (_error) => { +child.on('error', (error) => { if (requestId) activeProcesses.delete(requestId); - resolve({ - output: '', - cancelled: false, - executionTimeMs: Date.now() - startTime, - }); + // Reject to propagate the error to caller + // Note: This requires changing the caller to handle rejection + log('ERROR', `CLI spawn error: ${error.message}`); + resolve({ + output: '', + cancelled: false, + executionTimeMs: Date.now() - startTime, + error: error.message, // Add error field to return type + }); });Then handle the error field in the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web-server/src/handlers/ai-handlers.ts` around lines 395 - 403, The child process error handler currently calls resolve with an empty result which hides failures; update the child.on('error', ...) handler to (1) remove the activeProcesses entry (activeProcesses.delete(requestId)) as now, (2) call the Promise reject callback with the received error object instead of resolve so the caller can detect and handle failures, and (3) ensure the surrounding Promise signature includes a reject parameter and any callers of the function that await this Promise handle/rethrow the error appropriately.packages/web-server/src/handlers/export-handlers.ts-309-316 (1)
309-316:⚠️ Potential issue | 🟡 MinorCLI spawn errors are logged but not reported to client.
When
child.on('error')fires (e.g., command not found), the error is only logged. The client receives no notification that the CLI failed to start, leaving the operation in an ambiguous state.🐛 Proposed fix to notify client of spawn errors
child.on('error', (error) => { log('ERROR', `CLI command failed: ${command}`, { error: error.message }); + send({ + type: 'CLI_ERROR', + requestId, + payload: { error: error.message }, + }); });
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (139)
.gitignore.vscodeignoreCLAUDE.mdREADME.mdpackage.jsonpackages/core/package.jsonpackages/core/src/adapters/console-logger.tspackages/core/src/adapters/node-file-system.tspackages/core/src/cli/mcp-server-cli.tspackages/core/src/i18n/i18n-service.tspackages/core/src/i18n/translation-keys.tspackages/core/src/i18n/translations/en.tspackages/core/src/i18n/translations/ja.tspackages/core/src/i18n/translations/ko.tspackages/core/src/i18n/translations/zh-CN.tspackages/core/src/i18n/translations/zh-TW.tspackages/core/src/index.tspackages/core/src/interfaces/dialog-service.tspackages/core/src/interfaces/file-system.tspackages/core/src/interfaces/logger.tspackages/core/src/interfaces/message-transport.tspackages/core/src/interfaces/workflow-provider.tspackages/core/src/services/file-service.tspackages/core/src/services/fs-workflow-provider.tspackages/core/src/services/logger-holder.tspackages/core/src/services/mcp-server-service.tspackages/core/src/services/mcp-server-tools.tspackages/core/src/services/schema-loader-service.tspackages/core/src/types/ai-metrics.tspackages/core/src/types/mcp-node.tspackages/core/src/types/messages.tspackages/core/src/types/slack-integration-types.tspackages/core/src/types/workflow-definition.tspackages/core/src/utils/migrate-workflow.tspackages/core/src/utils/path-utils.tspackages/core/src/utils/schema-parser.tspackages/core/src/utils/sensitive-data-detector.tspackages/core/src/utils/slack-error-handler.tspackages/core/src/utils/slack-message-builder.tspackages/core/src/utils/validate-workflow.tspackages/core/src/utils/workflow-validator.tspackages/core/tsconfig.jsonpackages/web-server/package.jsonpackages/web-server/src/adapters/ws-message-transport.tspackages/web-server/src/handlers/ai-handlers.tspackages/web-server/src/handlers/export-handlers.tspackages/web-server/src/handlers/mcp-handlers.tspackages/web-server/src/handlers/skill-handlers.tspackages/web-server/src/handlers/slack-handlers.tspackages/web-server/src/handlers/workflow-handlers.tspackages/web-server/src/routes/ws-handler.tspackages/web-server/src/server.tspackages/web-server/src/services/secret-store.tspackages/web-server/src/services/shell-exec.tspackages/web-server/src/services/web-dialog-service.tspackages/web-server/tsconfig.jsonscripts/generate-editing-flow.tssrc/extension/commands/antigravity-handlers.tssrc/extension/commands/codex-handlers.tssrc/extension/commands/copilot-handlers.tssrc/extension/commands/cursor-handlers.tssrc/extension/commands/export-workflow.tssrc/extension/commands/gemini-handlers.tssrc/extension/commands/load-workflow-list.tssrc/extension/commands/load-workflow.tssrc/extension/commands/mcp-handlers.tssrc/extension/commands/open-editor.tssrc/extension/commands/roo-code-handlers.tssrc/extension/commands/save-workflow.tssrc/extension/commands/skill-operations.tssrc/extension/commands/slack-connect-manual.tssrc/extension/commands/slack-connect-oauth.tssrc/extension/commands/slack-description-generation.tssrc/extension/commands/slack-import-workflow.tssrc/extension/commands/slack-share-workflow.tssrc/extension/commands/text-editor.tssrc/extension/commands/workflow-name-generation.tssrc/extension/commands/workflow-refinement.tssrc/extension/editors/workflow-preview-editor-provider.tssrc/extension/extension.tssrc/extension/services/ai-editing-skill-service.tssrc/extension/services/ai-metrics-service.tssrc/extension/services/ai-provider.tssrc/extension/services/antigravity-extension-service.tssrc/extension/services/antigravity-skill-export-service.tssrc/extension/services/claude-cli-path.tssrc/extension/services/claude-code-service.tssrc/extension/services/cli-path-detector.tssrc/extension/services/codex-cli-path.tssrc/extension/services/codex-cli-service.tssrc/extension/services/codex-mcp-sync-service.tssrc/extension/services/codex-skill-export-service.tssrc/extension/services/copilot-cli-mcp-sync-service.tssrc/extension/services/copilot-export-service.tssrc/extension/services/copilot-skill-export-service.tssrc/extension/services/cursor-extension-service.tssrc/extension/services/cursor-skill-export-service.tssrc/extension/services/export-service.tssrc/extension/services/file-service.tssrc/extension/services/gemini-cli-path.tssrc/extension/services/gemini-mcp-sync-service.tssrc/extension/services/gemini-skill-export-service.tssrc/extension/services/mcp-cache-service.tssrc/extension/services/mcp-cli-service.tssrc/extension/services/mcp-config-reader.tssrc/extension/services/mcp-sdk-client.tssrc/extension/services/mcp-server-config-writer.tssrc/extension/services/refinement-prompt-builder.tssrc/extension/services/refinement-service.tssrc/extension/services/roo-code-extension-service.tssrc/extension/services/roo-code-mcp-sync-service.tssrc/extension/services/roo-code-skill-export-service.tssrc/extension/services/skill-file-generator.tssrc/extension/services/skill-normalization-service.tssrc/extension/services/skill-relevance-matcher.tssrc/extension/services/skill-service.tssrc/extension/services/slack-api-service.tssrc/extension/services/slack-description-prompt-builder.tssrc/extension/services/slack-oauth-service.tssrc/extension/services/terminal-execution-service.tssrc/extension/services/vscode-lm-service.tssrc/extension/services/workflow-name-prompt-builder.tssrc/extension/services/workflow-prompt-generator.tssrc/extension/services/yaml-parser.tssrc/extension/types/slack-messages.tssrc/extension/utils/path-utils.tssrc/extension/utils/slack-token-manager.tssrc/extension/webview-content.tssrc/webview/src/main.tsxsrc/webview/src/services/bridge.tssrc/webview/src/services/electron-bridge-adapter.tssrc/webview/src/services/vscode-bridge-adapter.tssrc/webview/src/services/web-bridge-adapter.tssrc/webview/src/styles/main.csssrc/webview/src/styles/standalone-theme.csssrc/webview/vite.config.tstsconfig.base.jsontsconfig.jsonvite.extension.config.ts
💤 Files with no reviewable changes (44)
- src/extension/commands/load-workflow-list.ts
- src/extension/commands/cursor-handlers.ts
- src/extension/services/cursor-extension-service.ts
- src/extension/services/antigravity-skill-export-service.ts
- src/extension/commands/slack-connect-manual.ts
- src/extension/services/claude-code-service.ts
- src/extension/commands/codex-handlers.ts
- src/extension/services/gemini-cli-path.ts
- src/extension/services/copilot-export-service.ts
- src/extension/extension.ts
- src/extension/commands/skill-operations.ts
- src/extension/services/codex-cli-path.ts
- src/extension/commands/slack-connect-oauth.ts
- src/extension/commands/gemini-handlers.ts
- src/extension/services/codex-skill-export-service.ts
- src/extension/services/file-service.ts
- src/extension/commands/mcp-handlers.ts
- src/extension/commands/copilot-handlers.ts
- src/extension/services/codex-mcp-sync-service.ts
- src/extension/commands/antigravity-handlers.ts
- src/extension/services/ai-metrics-service.ts
- src/extension/commands/text-editor.ts
- src/extension/commands/load-workflow.ts
- src/extension/services/export-service.ts
- src/extension/services/antigravity-extension-service.ts
- src/extension/services/codex-cli-service.ts
- src/extension/commands/roo-code-handlers.ts
- src/extension/commands/export-workflow.ts
- .vscodeignore
- src/extension/services/claude-cli-path.ts
- src/extension/commands/workflow-name-generation.ts
- src/extension/commands/open-editor.ts
- src/extension/services/ai-provider.ts
- src/extension/editors/workflow-preview-editor-provider.ts
- src/extension/commands/slack-share-workflow.ts
- src/extension/services/copilot-skill-export-service.ts
- src/extension/services/ai-editing-skill-service.ts
- src/extension/services/cli-path-detector.ts
- src/extension/commands/save-workflow.ts
- src/extension/commands/slack-import-workflow.ts
- src/extension/commands/slack-description-generation.ts
- src/extension/commands/workflow-refinement.ts
- src/extension/services/copilot-cli-mcp-sync-service.ts
- src/extension/services/cursor-skill-export-service.ts
Comment on lines
+111
to
+126
| export function resolveSkillPathWithRoot( | ||
| skillPath: string, | ||
| scope: 'user' | 'project' | 'local', | ||
| workspaceRoot?: string | ||
| ): string { | ||
| if (scope === 'user' || scope === 'local') { | ||
| return skillPath; | ||
| } | ||
| if (!workspaceRoot) { | ||
| throw new Error('No workspace folder found for project Skill resolution'); | ||
| } | ||
| if (path.isAbsolute(skillPath)) { | ||
| return skillPath; | ||
| } | ||
| return path.resolve(workspaceRoot, skillPath); | ||
| } |
There was a problem hiding this comment.
Project-scope path resolution can escape the workspace root.
Lines [122]-[125] and [139] allow absolute paths and ../ traversal without enforcing containment. In server mode, this can expose arbitrary filesystem paths.
🛠️ Suggested fix
+function assertWithinWorkspace(candidatePath: string, workspaceRoot: string): void {
+ const rel = path.relative(workspaceRoot, candidatePath);
+ if (rel.startsWith('..') || path.isAbsolute(rel)) {
+ throw new Error('Path must stay within workspace root for project scope');
+ }
+}
+
export function resolveSkillPathWithRoot(
skillPath: string,
scope: 'user' | 'project' | 'local',
workspaceRoot?: string
): string {
@@
- if (path.isAbsolute(skillPath)) {
- return skillPath;
- }
- return path.resolve(workspaceRoot, skillPath);
+ const resolved = path.isAbsolute(skillPath)
+ ? path.normalize(skillPath)
+ : path.resolve(workspaceRoot, skillPath);
+ assertWithinWorkspace(resolved, workspaceRoot);
+ return resolved;
}
@@
export function toRelativePathWithRoot(
absolutePath: string,
scope: 'user' | 'project' | 'local',
workspaceRoot?: string
): string {
@@
- return path.relative(workspaceRoot, absolutePath);
+ const normalized = path.normalize(absolutePath);
+ assertWithinWorkspace(normalized, workspaceRoot);
+ return path.relative(workspaceRoot, normalized);
}Also applies to: 128-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/utils/path-utils.ts` around lines 111 - 126, The
project-scope path resolution in resolveSkillPathWithRoot currently returns
absolute paths or allows ../ traversal which can escape the workspace root;
update resolveSkillPathWithRoot so when scope === 'project' it resolves the
candidate via path.resolve(workspaceRoot, skillPath) (or fs.realpathSync for
symlink safety), then verify containment by comparing
path.relative(workspaceRoot, resolvedPath) (or ensuring
resolvedPath.startsWith(realWorkspaceRoot + path.sep)), and throw an error if
the resolved path is outside the workspaceRoot; continue to return the original
skillPath for 'user'/'local' scopes.
Comment on lines
+357
to
+361
| const child = spawn(command, ['--print', prompt], { | ||
| cwd, | ||
| shell: true, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
There was a problem hiding this comment.
Command injection risk via prompt content passed to CLI.
The prompt is passed directly as a CLI argument with shell: true. If the prompt contains shell metacharacters (backticks, $(), semicolons), they could be executed. User instructions flow into this prompt via buildRefinementPrompt.
🔒 Proposed fix - escape or avoid shell
Option 1: Remove shell: true (preferred, if CLI supports it)
-const child = spawn(command, ['--print', prompt], {
+const child = spawn(command, ['--print', prompt], {
cwd,
- shell: true,
+ shell: false,
stdio: ['pipe', 'pipe', 'pipe'],
});Option 2: Pass prompt via stdin instead of argument
const child = spawn(command, ['--print', '-'], {
cwd,
shell: false,
stdio: ['pipe', 'pipe', 'pipe'],
});
child.stdin.write(prompt);
child.stdin.end();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const child = spawn(command, ['--print', prompt], { | |
| cwd, | |
| shell: true, | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }); | |
| const child = spawn(command, ['--print', prompt], { | |
| cwd, | |
| shell: false, | |
| stdio: ['pipe', 'pipe', 'pipe'], | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-server/src/handlers/ai-handlers.ts` around lines 357 - 361, The
spawn call that runs the CLI with user-controlled prompt (see spawn(command,
['--print', prompt], { shell: true, ... }) in ai-handlers.ts and where
buildRefinementPrompt feeds into prompt) is vulnerable to shell injection; fix
by removing shell: true (use shell: false) and avoid passing the prompt as a
shell argument—either pass the prompt via stdin to the child process
(spawn(command, ['--print', '-'], stdio configured, write prompt to child.stdin
and end) or ensure the CLI supports safely receiving the prompt as a single argv
element without a shell; update the spawn invocation and surrounding logic that
builds/writes the prompt accordingly.
Comment on lines
+271
to
+284
| function executeCliCommand( | ||
| command: string, | ||
| args: string[], | ||
| cwd: string, | ||
| send: Send, | ||
| requestId?: string | ||
| ): void { | ||
| try { | ||
| const child = spawn(command, args, { | ||
| cwd, | ||
| shell: true, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
|
|
There was a problem hiding this comment.
Command injection vulnerability with shell: true.
Using spawn with shell: true and unsanitized arguments (args) allows command injection. A workflow name like foo; rm -rf / would execute arbitrary commands.
🔒 Proposed fix - avoid shell or sanitize inputs
Option 1: Remove shell: true (preferred)
const child = spawn(command, args, {
cwd,
- shell: true,
+ shell: false,
stdio: ['pipe', 'pipe', 'pipe'],
});Option 2: If shell is required, sanitize workflow name at the source (in handleRunAsSlashCommandWeb, handleAgentRunWeb):
const sanitizedName = workflowName.replace(/[^a-zA-Z0-9_-]/g, '-');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-server/src/handlers/export-handlers.ts` around lines 271 - 284,
The spawn call in executeCliCommand uses shell: true with unsanitized args,
enabling command injection; remove shell: true so arguments are passed directly
to child processes (update executeCliCommand invocation and ensure callers like
handleRunAsSlashCommandWeb and handleAgentRunWeb provide separate args rather
than a single shell string), or if shell semantics are absolutely required,
sanitize user-supplied values at the source (e.g., workflowName in
handleRunAsSlashCommandWeb and handleAgentRunWeb) by stripping/whitelisting
characters (allow only alphanumerics, dash, underscore) before passing into
args; update tests and call sites to ensure args are safe and no longer rely on
shell expansion.
Comment on lines
+36
to
+40
| const workflowName = workflow.name as string; | ||
| // Simple export: create a command file with workflow instructions | ||
| const commandContent = generateCommandContent(workflow, nodes); | ||
| const commandPath = path.join(commandsDir, `${workflowName}.md`); | ||
| await fileService.writeFile(commandPath, commandContent); |
There was a problem hiding this comment.
Sanitize workflow/agent names before building output paths.
On Line 39 and Line 53, workflowName / agentName are used directly in filenames. Path separators or traversal tokens can escape .claude/commands or .claude/agents.
Suggested fix
+function toSafeFileName(input: string, fallback: string): string {
+ const base = input.trim() || fallback;
+ return base.replace(/[<>:"/\\|?*\x00-\x1F]/g, '-').replace(/\s+/g, '-');
+}
+
// Generate command file content
const workflowName = workflow.name as string;
// Simple export: create a command file with workflow instructions
const commandContent = generateCommandContent(workflow, nodes);
-const commandPath = path.join(commandsDir, `${workflowName}.md`);
+const commandPath = path.join(commandsDir, `${toSafeFileName(workflowName, 'workflow')}.md`);
await fileService.writeFile(commandPath, commandContent);
exportedFiles.push(commandPath);
@@
const data = node.data as Record<string, unknown>;
const agentName = (data.label as string) || `agent-${node.id}`;
const agentContent = generateAgentContent(data);
-const agentPath = path.join(agentsDir, `${agentName}.md`);
+const agentPath = path.join(agentsDir, `${toSafeFileName(agentName, 'agent')}.md`);
await fileService.writeFile(agentPath, agentContent);
exportedFiles.push(agentPath);Also applies to: 51-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web-server/src/handlers/workflow-handlers.ts` around lines 36 - 40,
The code currently uses workflowName and agentName directly to build file paths
for commands and agents (see variables workflowName/agentName,
commandsDir/agentsDir and calls to fileService.writeFile), allowing path
separators or traversal tokens to escape target directories; fix by validating
and sanitizing these names before joining paths—implement or reuse a
safe-filename utility (e.g. strip path separators, remove "..", null bytes, and
other unsafe characters or slugify/URI-encode the name) and use the sanitized
value when creating commandPath/agentPath so all writes
(generateCommandContent/generateAgentContent -> fileService.writeFile) are
confined to the intended .claude/commands and .claude/agents directories.
Owner
|
Thanks for this PR! Before reviewing, I'd like to clarify a few things: 1. Vision for the web app 2. Breaking change: VSCode extension support is removed As a result, merging this would make CC Workflow Studio no longer function as a VSCode extension. This is a complete replacement, not a coexistence of both platforms. If the goal is to support web mode, could we consider an approach that preserves VSCode extension support alongside the web app (similar to the foundation laid in #616)? |
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
2 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
packages/web-server/with full message routing for all 65+ message types, replacingsrc/extension/(21 command handlers, 48 services)IHostBridgeabstraction so the React UI works unchangedArchitecture
Changes
Added
packages/web-server/— Hono HTTP server with WebSocket endpointsrc/server.ts— Static file serving + WebSocket setupsrc/routes/ws-handler.ts— Message router (~700 lines, mirrors all cases fromopen-editor.ts)src/handlers/— Feature handlers: workflow CRUD, skills, export/run (7 agents), MCP, Slack, AIsrc/services/secret-store.ts— Encrypted file-based token storage (replacesvscode.ExtensionContext.secrets)src/services/shell-exec.ts—child_process.spawn()with streaming (replacesvscode.window.createTerminal())src/services/web-dialog-service.ts—IDialogServiceimpl via WebSocketsrc/adapters/ws-message-transport.ts—IMessageTransportimpl via WebSocketsrc/webview/src/services/web-bridge-adapter.ts—IHostBridgeimpl using WebSocket with auto-reconnectsrc/webview/vite.config.ts(VITE_WEB_MODE, WS proxy)src/webview/src/main.tsxnpm run dev:web,npm run build:web,npm run start:webscriptsRemoved
src/extension/— 21 command handlers + 48 services (~28,000 lines)packages/electron/— Electron shellpackages/vscode/— VSCode-specific adaptersvite.extension.config.ts,.vscodeignorepackage.json(engines.vscode,contributes,activationEvents,main)@types/vscode,@vscode/test-cli,@vscode/test-electrondevDependenciesModified
package.json— Updated scripts, removed VSCode fields,engines.node >= 18tsconfig.json— Removedvscodetype, switched to ESNext modulescripts/generate-editing-flow.ts— Output path updated fromsrc/extension/topackages/core/CLAUDE.md— Updated architecture diagrams, project structure, commandsTest plan
npm run buildsucceeds with zero errorsnpm run lintpasses with zero warningsnpm run dev:webstarts Hono server + Vite dev serverhttp://localhost:3001— webview renders correctlySummary by CodeRabbit
New Features
Refactor
Documentation