Implement pilot: event-bus based agent #113
Open
Conversation
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
🚀 Preview Deployments Ready!Your changes have been deployed to preview environments: 📦
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 42 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="mcp-studio/README.md">
<violation number="1" location="mcp-studio/README.md:77">
P2: JSON code block contains JavaScript-style comments (`//`) which are invalid in JSON. Users copying these examples will get parsing errors. Consider using `jsonc` (JSON with Comments) as the language identifier, or move explanations outside the code block as markdown text.</violation>
</file>
<file name="pilot/workflows/thread.json">
<violation number="1" location="pilot/workflows/thread.json:10">
P1: Missing `prompt` field in LLM action. All other workflows define a `prompt` field (e.g., `"prompt": "@input.message"`) to specify what the LLM should process. Without it, the LLM step may fail or not receive the user's message.</violation>
</file>
<file name="pilot/server/agent.ts">
<violation number="1" location="pilot/server/agent.ts:119">
P2: The `smartModel` fallback is placed before `...config` spread, so it gets overwritten if the caller passes `smartModel` (including `undefined`). Move it after the spread and use nullish coalescing.</violation>
</file>
<file name="pilot/tsconfig.json">
<violation number="1" location="pilot/tsconfig.json:7">
P2: Strict mode is disabled, which deviates from the project's standard template that uses `"strict": true`. For a new project, especially one handling complex AI workflows and events, enabling strict mode helps catch type errors early. Consider enabling strict mode and the additional linting options used in the project template.</violation>
</file>
<file name="pilot/server/task-manager.ts">
<violation number="1" location="pilot/server/task-manager.ts:67">
P2: Cancelled tasks are not included in the cleanup logic when enforcing `MAX_TASKS`. Since `cancelled` is a terminal state like `completed` and `error`, these tasks should also be eligible for removal to prevent accumulation.</violation>
<violation number="2" location="pilot/server/task-manager.ts:215">
P2: The `getTaskSummary` function doesn't include a count for cancelled tasks, even though `cancelled` is a valid task status. This creates an inconsistency where `total` won't equal the sum of the individual status counts when cancelled tasks exist.</violation>
</file>
<file name="pilot/server/thread-manager.ts">
<violation number="1" location="pilot/server/thread-manager.ts:93">
P2: File is read twice in the same expression. Read the content once and reuse it to avoid redundant I/O operations.</violation>
<violation number="2" location="pilot/server/thread-manager.ts:108">
P1: The `chatId` parameter is accepted but never checked. Different chats from the same source will incorrectly share the same thread, breaking conversation isolation.</violation>
<violation number="3" location="pilot/server/thread-manager.ts:167">
P2: Missing error handling for `JSON.parse`. If the thread file contains invalid JSON, this will throw an unhandled exception. Consider wrapping in try/catch like other functions in this file.</violation>
</file>
<file name="pilot/workflows/execute-multi-step.json">
<violation number="1" location="pilot/workflows/execute-multi-step.json:23">
P2: The step description says it skips when no task is provided, but `skipIf` actually checks for empty tools. Consider updating either the description to match the condition, or changing the condition to check for the task: `empty:@plan.taskForSmartAgent`.</violation>
</file>
<file name="pilot/README.md">
<violation number="1" location="pilot/README.md:285">
P2: Incorrect relative path - `../../mesh-bridge` navigates outside the repository. Should be `../mesh-bridge` to point to a sibling directory of pilot/, consistent with the link on line 85.</violation>
</file>
<file name="pilot/server/core/llm-executor.ts">
<violation number="1" location="pilot/server/core/llm-executor.ts:270">
P1: Using `new Function()` for code execution is a security risk equivalent to `eval()`. The executed code has full Node.js access without sandboxing. Consider using a sandboxed execution environment (like `vm2`, `isolated-vm`, or a worker thread with restricted permissions) or restricting this feature to trusted workflows only.</violation>
</file>
<file name="pilot/server/core/workflow-studio-adapter.ts">
<violation number="1" location="pilot/server/core/workflow-studio-adapter.ts:199">
P1: Missing readonly check before deleting workflow. `saveWorkflow` prevents modifying readonly file-based workflows, but `deleteWorkflow` doesn't have this guard, allowing accidental deletion of protected workflows.</violation>
</file>
<file name="mcp-studio/server/workflow-loader.ts">
<violation number="1" location="mcp-studio/server/workflow-loader.ts:86">
P1: Missing error handling for `readFile`. If the file cannot be read (e.g., deleted, permission denied), the error will propagate and cause all workflow loading to fail. Consider wrapping the `readFile` call in a try-catch to handle file access errors gracefully.</violation>
<violation number="2" location="mcp-studio/server/workflow-loader.ts:257">
P2: Parameter `options` should be optional (`options?: WorkflowLoaderOptions`) to match the fallback pattern used on line 243 and be consistent with `loadWorkflows`. Currently, if called with an empty object, the fallback to environment variables won't work.</violation>
</file>
<file name="pilot/server/tools/system.ts">
<violation number="1" location="pilot/server/tools/system.ts:32">
P1: Path traversal vulnerability: `startsWith` check allows access to sibling directories sharing a prefix (e.g., `/Users/guilherme/Projects-backup` passes check for allowed path `/Users/guilherme/Projects`). Ensure the path is either exactly the allowed path or starts with the allowed path followed by a path separator.</violation>
<violation number="2" location="pilot/server/tools/system.ts:560">
P1: AppleScript injection vulnerability: Backslashes are not escaped before quotes. Input like `\"` becomes `\\"` which in AppleScript is a literal backslash followed by an unescaped quote that closes the string, potentially allowing code injection.</violation>
</file>
<file name="pilot/server/tools/speech.ts">
<violation number="1" location="pilot/server/tools/speech.ts:50">
P2: Race condition: the cleanup code can overwrite a newer process reference. If a concurrent call spawns a new process while this one is awaiting, setting `activeSayProcess = null` in the catch block will orphan the newer process. Store the process reference locally and only clear if it matches.</violation>
</file>
<file name="mcp-studio/server/tools/workflow.ts">
<violation number="1" location="mcp-studio/server/tools/workflow.ts:133">
P1: Potential runtime error: `itemsResult` could be `undefined` if `env.MESH_REQUEST_CONTEXT?.state?.DATABASE` is null/undefined, but the code immediately accesses `itemsResult.result[0]` without a null check. This will throw a TypeError at runtime.</violation>
<violation number="2" location="mcp-studio/server/tools/workflow.ts:146">
P1: Potential null reference error: `countResult` may be `undefined` due to optional chaining. Add null safety: `countResult?.result?.[0]?.results?.[0]`.</violation>
<violation number="3" location="mcp-studio/server/tools/workflow.ts:190">
P1: Potential null reference error: `result` may be `undefined` due to optional chaining. Add null safety: `result?.result?.[0]?.results?.[0]`.</violation>
<violation number="4" location="mcp-studio/server/tools/workflow.ts:297">
P2: Malformed JSON example in tool description. The example step is broken (`true }, "transformCode"...`) and will confuse users. This should be a complete step definition like `{ "name": "fetch_users", "action": { "toolName": "GET_USERS" }, "input": { "all": true }, "transformCode": "export default async (i) => i[0]" }`.</violation>
<violation number="5" location="mcp-studio/server/tools/workflow.ts:403">
P1: Potential null reference error: `result` may be `undefined` due to optional chaining. Add null safety: `result?.result?.[0]?.results?.length`.</violation>
</file>
<file name="mcp-studio/server/main.ts">
<violation number="1" location="mcp-studio/server/main.ts:29">
P2: Debug `console.log` statements should be removed before merging to production. Consider using `console.error` with a proper tag format (e.g., `[MAIN]`) if logging is needed, matching the existing pattern in the error handler.</violation>
</file>
<file name="pilot/server/types/workflow.ts">
<violation number="1" location="pilot/server/types/workflow.ts:146">
P1: Missing dependency detection for several action fields that can contain `@ref` references. This function only traverses `input`, `prompt`, and `tools`, but misses `systemPrompt`, `template`, `code`, `transformCode`, and `config.skipIf` which can all contain step references.</violation>
</file>
<file name="mcp-studio/server/stdio-tools.ts">
<violation number="1" location="mcp-studio/server/stdio-tools.ts:988">
P0: Table name mismatch: query references `workflow_step_result` but the migration creates `workflow_execution_step_result`. Additionally, the ORDER BY uses `created_at` which doesn't exist in this table (should use `started_at_epoch_ms` or similar).</violation>
<violation number="2" location="mcp-studio/server/stdio-tools.ts:1100">
P0: INSERT statement is missing required `instructions` column. The table defines `instructions TEXT NOT NULL` without a default, so this insert will fail with a NOT NULL constraint violation.</violation>
</file>
<file name="pilot/server/core/execution-adapter.ts">
<violation number="1" location="pilot/server/core/execution-adapter.ts:193">
P2: TTL check is bypassed when `completed_at_epoch_ms` is missing. Executions without this timestamp will always pass the TTL check, potentially allowing stale threads to be continued.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
mcp-studio/server/stdio-tools.ts
Outdated
| "https://assets.decocache.com/decocms/fd07a578-6b1c-40f1-bc05-88a3b981695d/f7fc4ffa81aec04e37ae670c3cd4936643a7b269.png"; | ||
|
|
||
| await runSQL( | ||
| `INSERT INTO assistants (id, title, description, avatar, system_prompt, gateway_id, model, created_at, updated_at, created_by, updated_by) |
There was a problem hiding this comment.
P0: INSERT statement is missing required instructions column. The table defines instructions TEXT NOT NULL without a default, so this insert will fail with a NOT NULL constraint violation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/stdio-tools.ts, line 1100:
<comment>INSERT statement is missing required `instructions` column. The table defines `instructions TEXT NOT NULL` without a default, so this insert will fail with a NOT NULL constraint violation.</comment>
<file context>
@@ -0,0 +1,1512 @@
+ "https://assets.decocache.com/decocms/fd07a578-6b1c-40f1-bc05-88a3b981695d/f7fc4ffa81aec04e37ae670c3cd4936643a7b269.png";
+
+ await runSQL(
+ `INSERT INTO assistants (id, title, description, avatar, system_prompt, gateway_id, model, created_at, updated_at, created_by, updated_by)
+ VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`,
+ [
</file context>
✅ Addressed in 70fc89d
mcp-studio/server/stdio-tools.ts
Outdated
| ); | ||
|
|
||
| const stepResults = await runSQL<Record<string, unknown>>( | ||
| "SELECT * FROM workflow_step_result WHERE execution_id = ? ORDER BY created_at ASC", |
There was a problem hiding this comment.
P0: Table name mismatch: query references workflow_step_result but the migration creates workflow_execution_step_result. Additionally, the ORDER BY uses created_at which doesn't exist in this table (should use started_at_epoch_ms or similar).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/stdio-tools.ts, line 988:
<comment>Table name mismatch: query references `workflow_step_result` but the migration creates `workflow_execution_step_result`. Additionally, the ORDER BY uses `created_at` which doesn't exist in this table (should use `started_at_epoch_ms` or similar).</comment>
<file context>
@@ -0,0 +1,1512 @@
+ );
+
+ const stepResults = await runSQL<Record<string, unknown>>(
+ "SELECT * FROM workflow_step_result WHERE execution_id = ? ORDER BY created_at ASC",
+ [args.id],
+ );
</file context>
Suggested change
| "SELECT * FROM workflow_step_result WHERE execution_id = ? ORDER BY created_at ASC", | |
| "SELECT * FROM workflow_execution_step_result WHERE execution_id = ? ORDER BY started_at_epoch_ms ASC", |
✅ Addressed in 70fc89d
Comment on lines
10
to
13
| "action": { | ||
| "type": "llm", | ||
| "model": "fast", |
There was a problem hiding this comment.
P1: Missing prompt field in LLM action. All other workflows define a prompt field (e.g., "prompt": "@input.message") to specify what the LLM should process. Without it, the LLM step may fail or not receive the user's message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/workflows/thread.json, line 10:
<comment>Missing `prompt` field in LLM action. All other workflows define a `prompt` field (e.g., `"prompt": "@input.message"`) to specify what the LLM should process. Without it, the LLM step may fail or not receive the user's message.</comment>
<file context>
@@ -0,0 +1,23 @@
+ {
+ "name": "respond",
+ "description": "Process message with LLM and meta-tools",
+ "action": {
+ "type": "llm",
+ "model": "fast",
</file context>
Suggested change
| "action": { | |
| "type": "llm", | |
| "model": "fast", | |
| "action": { | |
| "type": "llm", | |
| "prompt": "@input.message", | |
| "model": "fast", |
✅ Addressed in 70fc89d
| const thread: Thread = JSON.parse(readFileSync(file.path, "utf-8")); | ||
|
|
||
| // Check if thread matches source/chatId and is open | ||
| if (thread.source !== source) continue; |
There was a problem hiding this comment.
P1: The chatId parameter is accepted but never checked. Different chats from the same source will incorrectly share the same thread, breaking conversation isolation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/thread-manager.ts, line 108:
<comment>The `chatId` parameter is accepted but never checked. Different chats from the same source will incorrectly share the same thread, breaking conversation isolation.</comment>
<file context>
@@ -0,0 +1,273 @@
+ const thread: Thread = JSON.parse(readFileSync(file.path, "utf-8"));
+
+ // Check if thread matches source/chatId and is open
+ if (thread.source !== source) continue;
+ if (thread.status !== "open") continue;
+
</file context>
Suggested change
| if (thread.source !== source) continue; | |
| if (thread.source !== source) continue; | |
| if (thread.chatId !== chatId) continue; |
✅ Addressed in 70fc89d
pilot/server/core/llm-executor.ts
Outdated
| const code = step.action.code; | ||
|
|
||
| try { | ||
| const fn = new Function( |
There was a problem hiding this comment.
P1: Using new Function() for code execution is a security risk equivalent to eval(). The executed code has full Node.js access without sandboxing. Consider using a sandboxed execution environment (like vm2, isolated-vm, or a worker thread with restricted permissions) or restricting this feature to trusted workflows only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/core/llm-executor.ts, line 270:
<comment>Using `new Function()` for code execution is a security risk equivalent to `eval()`. The executed code has full Node.js access without sandboxing. Consider using a sandboxed execution environment (like `vm2`, `isolated-vm`, or a worker thread with restricted permissions) or restricting this feature to trusted workflows only.</comment>
<file context>
@@ -0,0 +1,750 @@
+ const code = step.action.code;
+
+ try {
+ const fn = new Function(
+ "input",
+ `
</file context>
✅ Addressed in 70fc89d
mcp-studio/server/workflow-loader.ts
Outdated
| * Start watching for file changes | ||
| */ | ||
| export async function startWatching( | ||
| options: WorkflowLoaderOptions, |
There was a problem hiding this comment.
P2: Parameter options should be optional (options?: WorkflowLoaderOptions) to match the fallback pattern used on line 243 and be consistent with loadWorkflows. Currently, if called with an empty object, the fallback to environment variables won't work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/workflow-loader.ts, line 257:
<comment>Parameter `options` should be optional (`options?: WorkflowLoaderOptions`) to match the fallback pattern used on line 243 and be consistent with `loadWorkflows`. Currently, if called with an empty object, the fallback to environment variables won't work.</comment>
<file context>
@@ -0,0 +1,315 @@
+ * Start watching for file changes
+ */
+export async function startWatching(
+ options: WorkflowLoaderOptions,
+): Promise<void> {
+ const source = options || getWorkflowSource();
</file context>
Suggested change
| options: WorkflowLoaderOptions, | |
| options?: WorkflowLoaderOptions, |
✅ Addressed in 70fc89d
pilot/server/tools/speech.ts
Outdated
| if (activeSayProcess) { | ||
| try { | ||
| activeSayProcess.kill(); | ||
| activeSayProcess = null; |
There was a problem hiding this comment.
P2: Race condition: the cleanup code can overwrite a newer process reference. If a concurrent call spawns a new process while this one is awaiting, setting activeSayProcess = null in the catch block will orphan the newer process. Store the process reference locally and only clear if it matches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/tools/speech.ts, line 50:
<comment>Race condition: the cleanup code can overwrite a newer process reference. If a concurrent call spawns a new process while this one is awaiting, setting `activeSayProcess = null` in the catch block will orphan the newer process. Store the process reference locally and only clear if it matches.</comment>
<file context>
@@ -0,0 +1,179 @@
+ if (activeSayProcess) {
+ try {
+ activeSayProcess.kill();
+ activeSayProcess = null;
+ return true;
+ } catch {
</file context>
✅ Addressed in 70fc89d
mcp-studio/server/main.ts
Outdated
| events: [...WORKFLOW_EVENTS] as string[], | ||
| handler: async ({ events }, env) => { | ||
| try { | ||
| console.log("handling events", events); |
There was a problem hiding this comment.
P2: Debug console.log statements should be removed before merging to production. Consider using console.error with a proper tag format (e.g., [MAIN]) if logging is needed, matching the existing pattern in the error handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/main.ts, line 29:
<comment>Debug `console.log` statements should be removed before merging to production. Consider using `console.error` with a proper tag format (e.g., `[MAIN]`) if logging is needed, matching the existing pattern in the error handler.</comment>
<file context>
@@ -21,7 +26,9 @@ const runtime = withRuntime<Env, typeof StateSchema, Registry>({
events: [...WORKFLOW_EVENTS] as string[],
handler: async ({ events }, env) => {
try {
+ console.log("handling events", events);
handleWorkflowEvents(events, env as unknown as Env);
+ console.log("events handled");
</file context>
✅ Addressed in 70fc89d
Comment on lines
193
to
196
| if (exec.completed_at_epoch_ms) { | ||
| const age = now - exec.completed_at_epoch_ms; | ||
| if (age > ttlMs) continue; | ||
| } |
There was a problem hiding this comment.
P2: TTL check is bypassed when completed_at_epoch_ms is missing. Executions without this timestamp will always pass the TTL check, potentially allowing stale threads to be continued.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/core/execution-adapter.ts, line 193:
<comment>TTL check is bypassed when `completed_at_epoch_ms` is missing. Executions without this timestamp will always pass the TTL check, potentially allowing stale threads to be continued.</comment>
<file context>
@@ -0,0 +1,333 @@
+ if (exec.status !== "success") continue;
+
+ // Must be within TTL
+ if (exec.completed_at_epoch_ms) {
+ const age = now - exec.completed_at_epoch_ms;
+ if (age > ttlMs) continue;
</file context>
Suggested change
| if (exec.completed_at_epoch_ms) { | |
| const age = now - exec.completed_at_epoch_ms; | |
| if (age > ttlMs) continue; | |
| } | |
| if (!exec.completed_at_epoch_ms) continue; // Skip if no completion timestamp | |
| const age = now - exec.completed_at_epoch_ms; | |
| if (age > ttlMs) continue; |
✅ Addressed in 70fc89d
mcp-studio/server/tools/workflow.ts
Outdated
| { "name": "fetch_user", "action": { "toolName": "GET_USER" }, "input": { "email": "@input.user_email" } }, | ||
| { "name": "fetch_orders", "action": { "toolName": "GET_USER_ORDERS" }, "input": { "user_id": "@fetch_user.user.id" } }, | ||
| { "title": "Get first user and then fetch orders", "steps": [ | ||
| true }, "transformCode": "export default async (i) => i[0]" }, |
There was a problem hiding this comment.
P2: Malformed JSON example in tool description. The example step is broken (true }, "transformCode"...) and will confuse users. This should be a complete step definition like { "name": "fetch_users", "action": { "toolName": "GET_USERS" }, "input": { "all": true }, "transformCode": "export default async (i) => i[0]" }.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/tools/workflow.ts, line 297:
<comment>Malformed JSON example in tool description. The example step is broken (`true }, "transformCode"...`) and will confuse users. This should be a complete step definition like `{ "name": "fetch_users", "action": { "toolName": "GET_USERS" }, "input": { "all": true }, "transformCode": "export default async (i) => i[0]" }`.</comment>
<file context>
@@ -212,83 +279,88 @@ export async function insertWorkflowCollectionItem(
- { "name": "fetch_user", "action": { "toolName": "GET_USER" }, "input": { "email": "@input.user_email" } },
- { "name": "fetch_orders", "action": { "toolName": "GET_USER_ORDERS" }, "input": { "user_id": "@fetch_user.user.id" } },
+{ "title": "Get first user and then fetch orders", "steps": [
+ true }, "transformCode": "export default async (i) => i[0]" },
+ { "name": "fetch_orders", "action": { "toolName": "GET_ORDERS" }, "input": { "user": "@fetch_users.user" } },
]}
</file context>
✅ Addressed in 70fc89d
vibegui
force-pushed
the
feat/pilot-workflow
branch
from
70fc89d to
930c967
Compare
There was a problem hiding this comment.
1 issue found across 23 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="pilot/README.md">
<violation number="1" location="pilot/README.md:285">
P3: The updated Mesh Bridge link now points to `../mesh-bridge`, but that path doesn’t exist in this repo, leaving readers with a broken link. Link directly to the Mesh Bridge project (e.g., its GitHub URL).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ## See Also | ||
|
|
||
| - [Architecture](docs/ARCHITECTURE.md) - Detailed architecture overview | ||
| - [Mesh Bridge](../mesh-bridge) - Browser interface for Pilot |
There was a problem hiding this comment.
P3: The updated Mesh Bridge link now points to ../mesh-bridge, but that path doesn’t exist in this repo, leaving readers with a broken link. Link directly to the Mesh Bridge project (e.g., its GitHub URL).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/README.md, line 285:
<comment>The updated Mesh Bridge link now points to `../mesh-bridge`, but that path doesn’t exist in this repo, leaving readers with a broken link. Link directly to the Mesh Bridge project (e.g., its GitHub URL).</comment>
<file context>
@@ -282,7 +282,7 @@ bun run check
- [Architecture](docs/ARCHITECTURE.md) - Detailed architecture overview
-- [Mesh Bridge](../../mesh-bridge) - Browser interface for Pilot
+- [Mesh Bridge](../mesh-bridge) - Browser interface for Pilot
- [MCP Mesh](https://github.com/decolabs/mesh) - The mesh platform
</file context>
Suggested change
| - [Mesh Bridge](../mesh-bridge) - Browser interface for Pilot | |
| - [Mesh Bridge](https://github.com/decolabs/mesh-bridge) - Browser interface for Pilot |
Contributor
Author
|
@cubic-dev-ai last commit removed a lot of stuff, can you review again? |
@vibegui I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
13 issues found across 35 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="openrouter/server/stdio.ts">
<violation number="1" location="openrouter/server/stdio.ts:157">
P2: The `where` and `orderBy` parameters are defined in the schema but never used in the handler. This misleads API consumers into thinking filtering and sorting are supported when they're actually ignored. Either implement the filtering/sorting logic or remove these parameters from the schema.</violation>
</file>
<file name="mcp-studio/server/tools/workflow.ts">
<violation number="1" location="mcp-studio/server/tools/workflow.ts:538">
P1: Missing optional chaining operator. `DATABASE.DATABASES_RUN_SQL` should be `DATABASE?.DATABASES_RUN_SQL` to match the pattern used elsewhere in this file and prevent TypeError if DATABASE is undefined.</violation>
</file>
<file name="mcp-studio/server/stdio-tools.ts">
<violation number="1" location="mcp-studio/server/stdio-tools.ts:1502">
P1: Schema mismatch: `transformPrompt` references `row.content` and `row.variables` which don't exist in the `prompts` table. The table has `arguments`, `icons`, and `messages` columns instead. This will cause prompt queries to return undefined/empty values instead of actual data.</violation>
</file>
<file name="mcp-studio/server/stdio.ts">
<violation number="1" location="mcp-studio/server/stdio.ts:1">
P2: Incorrect shebang for TypeScript file. Node.js does not natively support TypeScript. Use `#!/usr/bin/env bun` to match the sibling `openrouter/server/stdio.ts` and align with the documented usage (`bun run stdio`).</violation>
</file>
<file name="pilot/README.md">
<violation number="1" location="pilot/README.md:234">
P2: `CONVERSATION_WORKFLOW` is documented as `conversation`, but the repo ships the workflow under the ID `thread`, so the sample config points to a non-existent workflow.</violation>
</file>
<file name="pilot/server/tools/system.ts">
<violation number="1" location="pilot/server/tools/system.ts:296">
P2: Missing input validation for `command` argument. Unlike LIST_FILES and READ_FILE which validate their inputs with type checks, this directly casts without verification. If `command` is undefined or non-string, it will throw a runtime error instead of returning a proper error response.</violation>
<violation number="2" location="pilot/server/tools/system.ts:506">
P2: Missing input validation for `content` argument. Unlike LIST_FILES and READ_FILE which validate their inputs with type checks, this directly casts without verification. If `content` is undefined or non-string, it will cause unexpected behavior.</violation>
</file>
<file name="mcp-studio/README.md">
<violation number="1" location="mcp-studio/README.md:294">
P2: Documentation references non-existent `DATABASE_URL` environment variable. The codebase uses `MESH_URL`, `MESH_TOKEN`, and `MESH_STATE` for database connectivity (via bindings), not `DATABASE_URL`. This example will mislead users.</violation>
</file>
<file name="pilot/server/events.ts">
<violation number="1" location="pilot/server/events.ts:181">
P2: Missing `USER_COMMAND` event type constant for `UserCommandEventSchema`. This breaks the pattern where each event schema has a corresponding constant in `EVENT_TYPES`. If commands are meant to be supported, add the event type; otherwise consider removing the unused schema.</violation>
</file>
<file name="mcp-studio/server/main.ts">
<violation number="1" location="mcp-studio/server/main.ts:43">
P1: Database initialization order is incorrect. `ensureIndexes` is called before `ensureCollections`, but indexes cannot be created on tables that don't exist yet. Move `ensureIndexes` after all table creation calls.</violation>
</file>
<file name="pilot/server/thread-manager.ts">
<violation number="1" location="pilot/server/thread-manager.ts:198">
P2: Missing error handling: `JSON.parse` can throw if the file contains invalid JSON. Other functions in this file handle this case with try-catch.</violation>
</file>
<file name="pilot/server/tools/speech.ts">
<violation number="1" location="pilot/server/tools/speech.ts:53">
P2: Race condition: `activeSayProcess` is unconditionally set to `null` after awaiting, but another call could have started a new process in the meantime. Apply the same guard pattern used in `stopSpeaking()` to prevent clearing another caller's active process.</violation>
</file>
<file name="pilot/docs/ARCHITECTURE.md">
<violation number="1" location="pilot/docs/ARCHITECTURE.md:135">
P2: The documented `agent.response.{source}` payload omits the required `taskId` and `source` fields, so any event built from this spec will be rejected by `AgentResponseEventSchema`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| // Check if the new ID already exists in DB | ||
| const existingResult = | ||
| await env.MESH_REQUEST_CONTEXT?.state?.DATABASE.DATABASES_RUN_SQL({ |
There was a problem hiding this comment.
P1: Missing optional chaining operator. DATABASE.DATABASES_RUN_SQL should be DATABASE?.DATABASES_RUN_SQL to match the pattern used elsewhere in this file and prevent TypeError if DATABASE is undefined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/tools/workflow.ts, line 538:
<comment>Missing optional chaining operator. `DATABASE.DATABASES_RUN_SQL` should be `DATABASE?.DATABASES_RUN_SQL` to match the pattern used elsewhere in this file and prevent TypeError if DATABASE is undefined.</comment>
<file context>
@@ -381,143 +466,95 @@ export const createUpdateTool = (env: Env) =>
- }
+ // Check if the new ID already exists in DB
+ const existingResult =
+ await env.MESH_REQUEST_CONTEXT?.state?.DATABASE.DATABASES_RUN_SQL({
+ sql: "SELECT id FROM workflow_collection WHERE id = ? LIMIT 1",
+ params: [copyId],
</file context>
Suggested change
| await env.MESH_REQUEST_CONTEXT?.state?.DATABASE.DATABASES_RUN_SQL({ | |
| await env.MESH_REQUEST_CONTEXT?.state?.DATABASE?.DATABASES_RUN_SQL({ |
Comment on lines
+1502
to
+1507
| content: row.content, | ||
| variables: row.variables | ||
| ? typeof row.variables === "string" | ||
| ? JSON.parse(row.variables) | ||
| : row.variables | ||
| : [], |
There was a problem hiding this comment.
P1: Schema mismatch: transformPrompt references row.content and row.variables which don't exist in the prompts table. The table has arguments, icons, and messages columns instead. This will cause prompt queries to return undefined/empty values instead of actual data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/stdio-tools.ts, line 1502:
<comment>Schema mismatch: `transformPrompt` references `row.content` and `row.variables` which don't exist in the `prompts` table. The table has `arguments`, `icons`, and `messages` columns instead. This will cause prompt queries to return undefined/empty values instead of actual data.</comment>
<file context>
@@ -0,0 +1,1513 @@
+ id: row.id,
+ title: row.title,
+ description: row.description,
+ content: row.content,
+ variables: row.variables
+ ? typeof row.variables === "string"
</file context>
Suggested change
| content: row.content, | |
| variables: row.variables | |
| ? typeof row.variables === "string" | |
| ? JSON.parse(row.variables) | |
| : row.variables | |
| : [], | |
| arguments: row.arguments | |
| ? typeof row.arguments === "string" | |
| ? JSON.parse(row.arguments) | |
| : row.arguments | |
| : [], | |
| icons: row.icons | |
| ? typeof row.icons === "string" | |
| ? JSON.parse(row.icons) | |
| : row.icons | |
| : [], | |
| messages: row.messages | |
| ? typeof row.messages === "string" | |
| ? JSON.parse(row.messages) | |
| : row.messages | |
| : [], |
Comment on lines
+43
to
72
| await ensureIndexes(env); | ||
| await ensureCollections(env); | ||
| await ensureAssistantsTable(env); | ||
| await ensurePromptsTable(env); |
There was a problem hiding this comment.
P1: Database initialization order is incorrect. ensureIndexes is called before ensureCollections, but indexes cannot be created on tables that don't exist yet. Move ensureIndexes after all table creation calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/main.ts, line 43:
<comment>Database initialization order is incorrect. `ensureIndexes` is called before `ensureCollections`, but indexes cannot be created on tables that don't exist yet. Move `ensureIndexes` after all table creation calls.</comment>
<file context>
@@ -32,10 +37,13 @@ const runtime = withRuntime<Env, typeof StateSchema, Registry>({
+ // Initialize file-based workflows (from WORKFLOWS_DIRS env var)
+ initFileWorkflows();
+
+ await ensureIndexes(env);
await ensureCollections(env);
+ await ensureAssistantsTable(env);
</file context>
Suggested change
| await ensureIndexes(env); | |
| await ensureCollections(env); | |
| await ensureAssistantsTable(env); | |
| await ensurePromptsTable(env); | |
| await ensureCollections(env); | |
| await ensureAssistantsTable(env); | |
| await ensurePromptsTable(env); | |
| await ensureIndexes(env); |
| "COLLECTION_LLM_LIST", | ||
| "List all available models from OpenRouter with filtering and pagination", | ||
| { | ||
| where: z.any().optional().describe("Filter expression"), |
There was a problem hiding this comment.
P2: The where and orderBy parameters are defined in the schema but never used in the handler. This misleads API consumers into thinking filtering and sorting are supported when they're actually ignored. Either implement the filtering/sorting logic or remove these parameters from the schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At openrouter/server/stdio.ts, line 157:
<comment>The `where` and `orderBy` parameters are defined in the schema but never used in the handler. This misleads API consumers into thinking filtering and sorting are supported when they're actually ignored. Either implement the filtering/sorting logic or remove these parameters from the schema.</comment>
<file context>
@@ -0,0 +1,424 @@
+ "COLLECTION_LLM_LIST",
+ "List all available models from OpenRouter with filtering and pagination",
+ {
+ where: z.any().optional().describe("Filter expression"),
+ orderBy: z.any().optional().describe("Sort order"),
+ limit: z.number().optional().default(50).describe("Max results"),
</file context>
| @@ -0,0 +1,78 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
P2: Incorrect shebang for TypeScript file. Node.js does not natively support TypeScript. Use #!/usr/bin/env bun to match the sibling openrouter/server/stdio.ts and align with the documented usage (bun run stdio).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/stdio.ts, line 1:
<comment>Incorrect shebang for TypeScript file. Node.js does not natively support TypeScript. Use `#!/usr/bin/env bun` to match the sibling `openrouter/server/stdio.ts` and align with the documented usage (`bun run stdio`).</comment>
<file context>
@@ -0,0 +1,78 @@
+#!/usr/bin/env node
+/**
+ * MCP Studio - Stdio Entry Point
</file context>
Suggested change
| #!/usr/bin/env node | |
| #!/usr/bin/env bun |
| WORKFLOW_FILES=./workflows/enrich.json,./workflows/notify.json bun run stdio | ||
|
|
||
| # Combine with database (workflows from both sources are merged) | ||
| WORKFLOW_DIR=/path/to/workflows DATABASE_URL=... bun run stdio |
There was a problem hiding this comment.
P2: Documentation references non-existent DATABASE_URL environment variable. The codebase uses MESH_URL, MESH_TOKEN, and MESH_STATE for database connectivity (via bindings), not DATABASE_URL. This example will mislead users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/README.md, line 294:
<comment>Documentation references non-existent `DATABASE_URL` environment variable. The codebase uses `MESH_URL`, `MESH_TOKEN`, and `MESH_STATE` for database connectivity (via bindings), not `DATABASE_URL`. This example will mislead users.</comment>
<file context>
@@ -1,3 +1,519 @@
+WORKFLOW_FILES=./workflows/enrich.json,./workflows/notify.json bun run stdio
+
+# Combine with database (workflows from both sources are merged)
+WORKFLOW_DIR=/path/to/workflows DATABASE_URL=... bun run stdio
+```
+
</file context>
Comment on lines
+181
to
+183
| export const EVENT_TYPES = { | ||
| // Incoming | ||
| USER_MESSAGE: "user.message.received", |
There was a problem hiding this comment.
P2: Missing USER_COMMAND event type constant for UserCommandEventSchema. This breaks the pattern where each event schema has a corresponding constant in EVENT_TYPES. If commands are meant to be supported, add the event type; otherwise consider removing the unused schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/events.ts, line 181:
<comment>Missing `USER_COMMAND` event type constant for `UserCommandEventSchema`. This breaks the pattern where each event schema has a corresponding constant in `EVENT_TYPES`. If commands are meant to be supported, add the event type; otherwise consider removing the unused schema.</comment>
<file context>
@@ -0,0 +1,206 @@
+// Event Type Constants
+// ============================================================================
+
+export const EVENT_TYPES = {
+ // Incoming
+ USER_MESSAGE: "user.message.received",
</file context>
Suggested change
| export const EVENT_TYPES = { | |
| // Incoming | |
| USER_MESSAGE: "user.message.received", | |
| export const EVENT_TYPES = { | |
| // Incoming | |
| USER_MESSAGE: "user.message.received", | |
| USER_COMMAND: "user.command.received", |
| return; | ||
| } | ||
|
|
||
| const thread: Thread = JSON.parse(readFileSync(path, "utf-8")); |
There was a problem hiding this comment.
P2: Missing error handling: JSON.parse can throw if the file contains invalid JSON. Other functions in this file handle this case with try-catch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/thread-manager.ts, line 198:
<comment>Missing error handling: `JSON.parse` can throw if the file contains invalid JSON. Other functions in this file handle this case with try-catch.</comment>
<file context>
@@ -0,0 +1,283 @@
+ return;
+ }
+
+ const thread: Thread = JSON.parse(readFileSync(path, "utf-8"));
+ thread.status = "closed";
+ thread.lastActivityAt = new Date().toISOString();
</file context>
| processToKill.kill(); | ||
| // Only clear if it's still the active process (no race condition) | ||
| if (activeSayProcess === processToKill) { | ||
| activeSayProcess = null; |
There was a problem hiding this comment.
P2: Race condition: activeSayProcess is unconditionally set to null after awaiting, but another call could have started a new process in the meantime. Apply the same guard pattern used in stopSpeaking() to prevent clearing another caller's active process.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/tools/speech.ts, line 53:
<comment>Race condition: `activeSayProcess` is unconditionally set to `null` after awaiting, but another call could have started a new process in the meantime. Apply the same guard pattern used in `stopSpeaking()` to prevent clearing another caller's active process.</comment>
<file context>
@@ -0,0 +1,186 @@
+ processToKill.kill();
+ // Only clear if it's still the active process (no race condition)
+ if (activeSayProcess === processToKill) {
+ activeSayProcess = null;
+ }
+ return true;
</file context>
| ### Published (Outgoing) | ||
|
|
||
| ```typescript | ||
| "agent.response.{source}" { |
There was a problem hiding this comment.
P2: The documented agent.response.{source} payload omits the required taskId and source fields, so any event built from this spec will be rejected by AgentResponseEventSchema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/docs/ARCHITECTURE.md, line 135:
<comment>The documented `agent.response.{source}` payload omits the required `taskId` and `source` fields, so any event built from this spec will be rejected by `AgentResponseEventSchema`.</comment>
<file context>
@@ -0,0 +1,203 @@
+### Published (Outgoing)
+
+```typescript
+"agent.response.{source}" {
+ text: string;
+ chatId?: string;
</file context>
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="pilot/server/main.ts">
<violation number="1" location="pilot/server/main.ts:531">
P1: Debug logging code should be removed before merging. These hardcoded `fetch` calls to `http://127.0.0.1:7242` are development artifacts that will fail silently in production (due to `.catch(() => {})`) and add unnecessary network overhead. Remove all `#region agent log` blocks.</violation>
</file>
<file name="mcp-studio/server/main.ts">
<violation number="1" location="mcp-studio/server/main.ts:56">
P2: Setting `prompts: []` disables the database-backed MCP prompts feature. The `createPrompts` function loaded prompts from the database and exposed them via MCP protocol. If this removal is intentional, consider deleting the now-unused `prompts.ts` file (107 lines of dead code). If unintentional, restore the `createPrompts` import and usage.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const toolsList = json.result?.tools || []; | ||
| // #region agent log | ||
| fetch("http://127.0.0.1:7242/ingest/8397b2ea-9df9-487e-9ffa-b17eb1bfd701", { |
There was a problem hiding this comment.
P1: Debug logging code should be removed before merging. These hardcoded fetch calls to http://127.0.0.1:7242 are development artifacts that will fail silently in production (due to .catch(() => {})) and add unnecessary network overhead. Remove all #region agent log blocks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pilot/server/main.ts, line 531:
<comment>Debug logging code should be removed before merging. These hardcoded `fetch` calls to `http://127.0.0.1:7242` are development artifacts that will fail silently in production (due to `.catch(() => {})`) and add unnecessary network overhead. Remove all `#region agent log` blocks.</comment>
<file context>
@@ -460,12 +506,47 @@ async function getAvailableTools(): Promise<
const toolsList = json.result?.tools || [];
+ // #region agent log
+ fetch("http://127.0.0.1:7242/ingest/8397b2ea-9df9-487e-9ffa-b17eb1bfd701", {
+ method: "POST",
+ headers: { "Content-Type": "application/json" },
</file context>
| }, | ||
| tools, | ||
| prompts: [], // removed because this was making a call to the database for every request to the MCP server | ||
| prompts: [], |
There was a problem hiding this comment.
P2: Setting prompts: [] disables the database-backed MCP prompts feature. The createPrompts function loaded prompts from the database and exposed them via MCP protocol. If this removal is intentional, consider deleting the now-unused prompts.ts file (107 lines of dead code). If unintentional, restore the createPrompts import and usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcp-studio/server/main.ts, line 56:
<comment>Setting `prompts: []` disables the database-backed MCP prompts feature. The `createPrompts` function loaded prompts from the database and exposed them via MCP protocol. If this removal is intentional, consider deleting the now-unused `prompts.ts` file (107 lines of dead code). If unintentional, restore the `createPrompts` import and usage.</comment>
<file context>
@@ -54,7 +53,7 @@ const runtime = withRuntime<Env, typeof StateSchema, Registry>({
},
tools,
- prompts: createPrompts,
+ prompts: [],
});
</file context>
vibegui
force-pushed
the
feat/pilot-workflow
branch
from
c88f5ec to
53d50a5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by cubic
Introduces Pilot, an event-bus–driven agent that listens for user events, runs workflows (tools + LLM), and publishes responses. Adds stdio transport and file-based workflows so Pilot, Studio, and OpenRouter run locally or via Mesh.
New Features
Migration
Written for commit 53d50a5. Summary will update on new commits.