refactor(workflow): move orchestration to the CLI - #167
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR scopes workflow commands to the active workspace, rejects unknown CLI options, hardens agent store and prompt-file cleanup, enables agents for workflows, and removes server-side workflow tool registration. ChangesCLI hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant WorkflowCLI
participant WorkspaceScopeResolver
participant WorkflowStore
Operator->>WorkflowCLI: run or inspect workflow
WorkflowCLI->>WorkspaceScopeResolver: resolve workspace scope
WorkflowCLI->>WorkflowStore: retrieve workflow run
WorkflowCLI->>WorkflowStore: validate workspace ownership
WorkflowStore-->>WorkflowCLI: return scoped result
WorkflowCLI-->>Operator: display status or operation result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Waishnav
changed the title
Greptile SummaryThis PR tightens workspace isolation and argument validation for workflow CLI operations, improves local-agent store and prompt-file cleanup, and removes workflow tools from the MCP server.
Confidence Score: 4/5The workflow MCP surface must be restored or its remaining advertised contracts removed before merging; the temporary-directory leak is non-blocking. The server no longer invokes the only workflow-tool registrar, so workflow-enabled MCP clients cannot execute or monitor workflows, while each agent run also leaves an empty generated prompt directory behind. Files Needing Attention: src/server.ts, src/cli.ts
|
| Filename | Overview |
|---|---|
| src/server.ts | Removes the sole registration path for the documented workflow MCP and UI tools. |
| src/workflow-cli.ts | Adds workspace scoping, path containment, stale-run reaping, and unknown-option validation to workflow CLI operations. |
| src/cli.ts | Allows agent tooling for workflow-enabled configurations and closes stores reliably, but leaves generated temporary directories behind. |
| src/local-agent-targets.ts | Rejects unknown double-dash options instead of silently incorporating them into agent prompts. |
| src/local-agent-targets.test.ts | Adds coverage for rejecting unknown local-agent run options. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Workflow-enabled MCP client] --> B[createMcpServer]
B --> C[MCP server returned]
C -. registration removed .-> D[run_workflow / status / cancel absent]
D --> E[Unknown-tool failure]
F[devspace workflow CLI] --> G[Resolve current workspace scope]
G --> H[Launch or inspect scoped workflow run]
Reviews (1): Last reviewed commit: "refactor(mcp): remove workflow execution..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -47,7 +47,6 @@ import { formatPathForPrompt } from "./skills.js"; | |||
| import { createWorkspaceStore } from "./workspace-store.js"; | |||
| import { formatAgentsPath, WorkspaceRegistry } from "./workspaces.js"; | |||
| import { buildLocalAgentCatalog } from "./local-agent-catalog.js"; | |||
There was a problem hiding this comment.
When Dynamic Workflows are enabled, createMcpServer no longer calls the sole registerWorkflowTools registration function, so MCP calls to workflow execution, status, cancellation, and UI tools fail as unknown tools.
| }); | ||
| } | ||
| } finally { |
There was a problem hiding this comment.
The worker unlinks prompt.txt but leaves the unique directory created by mkdtempSync, so repeated agent runs permanently accumulate empty devspace-agent-prompt-* directories in the system temporary directory.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
7b38089 to
e604f10
Compare
e604f10 to
3020b71
Compare
|
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workflow-cli.ts (1)
109-115: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unused positional arguments.
These commands parse the documented arguments but ignore trailing positional tokens. Validate exact positional arity so invalid invocations do not run a different command than the user requested.
src/workflow-cli.ts#L109-L115: reject all positional tokens forworkflow run.src/workflow-cli.ts#L195-L198: reject positionals after<runId>.src/workflow-cli.ts#L228-L230: reject positionals after<runId>.src/workflow-cli.ts#L266-L268: reject positionals after<runId>.src/workflow-cli.ts#L292-L295: reject positionals after<runId> <callIndex>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-cli.ts` around lines 109 - 115, Validate exact positional argument arity in src/workflow-cli.ts: for workflow run at lines 109-115 reject all positional tokens; at lines 195-198, 228-230, and 266-268 reject any tokens after <runId>; and at lines 292-295 reject any tokens after <runId> <callIndex>. Apply the checks in the corresponding command handlers before execution so trailing positionals cannot be ignored.
🧹 Nitpick comments (1)
src/local-agent-targets.test.ts (1)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the usage guidance in the error contract.
The parser appends
USAGEto unknown-option errors, but this test checks onlyUnknown option: --unknown. Extend the assertion to verifyUsage: devspace agents runor assert the complete error message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-targets.test.ts` around lines 84 - 88, Update the assert.throws expectation for parseLocalAgentRunArgs to include the appended usage guidance, specifically matching “Usage: devspace agents run” or the complete expected error message alongside the unknown-option text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli.ts`:
- Around line 573-576: Update the cleanup block after isGeneratedPromptFile to
remove the temporary directory created by writeAgentPromptFile after unlinking
promptFile. Preserve the existing best-effort cleanup behavior and ensure the
generated directory is removed only after the prompt file cleanup completes.
- Around line 536-540: Update runAgentsWorker after retrieving the record and
before store.update to call assertAgentInScope with the record and
resolveCurrentWorkspaceScope(config). Preserve workspace scoping for all
operations, using the workspaceId returned by open_workspace as the opaque scope
handle.
- Around line 78-84: Wrap the `agents` switch case body containing the `config`
declaration and `runAgentsCommand(args)` in braces, ensuring `config` is scoped
only to that case while preserving the existing validation and command
execution.
In `@src/local-agent-targets.ts`:
- Around line 67-69: Update the option-value parsing branches in the local-agent
argument parser so model, effort, and thinking values beginning with “--” are
rejected before assignment, for both separated and equals forms. Preserve valid
value handling, and add regression tests covering each affected form, including
parseLocalAgentRunArgs with separated model and effort arguments.
In `@src/workflow-cli.ts`:
- Around line 485-493: Update resolveWorkflowFilePath to canonicalize both the
resolved workflow path and workspaceRoot before the containment check, while
retaining the existing lexical isPathInsideRoot validation. Enforce canonical
containment before returning the path or launching the workflow, and throw
InvalidWorkflowInputError with code "invalid_path" when either check fails.
- Around line 496-512: Update src/workflow-cli.ts lines 496-512 in
assertWorkflowInScope to require an exact workspaceId match whenever the active
scope provides one, rejecting runs with missing or different IDs. Update
src/workflow-cli.ts lines 253-254 to filter the store query by workspaceId
before applying the result limit, and define an explicit migration or
compatibility path for ID-less records; all workflow operations must remain
scoped by the opaque ID returned from open_workspace.
---
Outside diff comments:
In `@src/workflow-cli.ts`:
- Around line 109-115: Validate exact positional argument arity in
src/workflow-cli.ts: for workflow run at lines 109-115 reject all positional
tokens; at lines 195-198, 228-230, and 266-268 reject any tokens after <runId>;
and at lines 292-295 reject any tokens after <runId> <callIndex>. Apply the
checks in the corresponding command handlers before execution so trailing
positionals cannot be ignored.
---
Nitpick comments:
In `@src/local-agent-targets.test.ts`:
- Around line 84-88: Update the assert.throws expectation for
parseLocalAgentRunArgs to include the appended usage guidance, specifically
matching “Usage: devspace agents run” or the complete expected error message
alongside the unknown-option text.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7e237d2-039d-4865-9d5b-022f30f59c00
📒 Files selected for processing (5)
src/cli.tssrc/local-agent-targets.test.tssrc/local-agent-targets.tssrc/server.tssrc/workflow-cli.ts
💤 Files with no reviewable changes (1)
- src/server.ts
| const config = loadConfig(); | ||
| if (!config.subagents && !config.workflows) { | ||
| throw new Error( | ||
| "Subagents are disabled. Set DEVSPACE_SUBAGENTS=1 to enable the experimental feature.", | ||
| "Subagents and Dynamic Workflows are disabled. Set DEVSPACE_SUBAGENTS=1 or DEVSPACE_WORKFLOWS=1 to enable agent tooling.", | ||
| ); | ||
| } | ||
| await runAgentsCommand(args); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the agents case declaration with braces.
Biome reports noSwitchDeclarations for config. Wrap this case body in braces so its declaration cannot be visible to other switch clauses.
Proposed fix
- case "agents":
+ case "agents": {
const config = loadConfig();
if (!config.subagents && !config.workflows) {
throw new Error(
"Subagents and Dynamic Workflows are disabled. Set DEVSPACE_SUBAGENTS=1 or DEVSPACE_WORKFLOWS=1 to enable agent tooling.",
);
}
await runAgentsCommand(args);
return;
+ }📝 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.
| const config = loadConfig(); | |
| if (!config.subagents && !config.workflows) { | |
| throw new Error( | |
| "Subagents are disabled. Set DEVSPACE_SUBAGENTS=1 to enable the experimental feature.", | |
| "Subagents and Dynamic Workflows are disabled. Set DEVSPACE_SUBAGENTS=1 or DEVSPACE_WORKFLOWS=1 to enable agent tooling.", | |
| ); | |
| } | |
| await runAgentsCommand(args); | |
| case "agents": { | |
| const config = loadConfig(); | |
| if (!config.subagents && !config.workflows) { | |
| throw new Error( | |
| "Subagents and Dynamic Workflows are disabled. Set DEVSPACE_SUBAGENTS=1 or DEVSPACE_WORKFLOWS=1 to enable agent tooling.", | |
| ); | |
| } | |
| await runAgentsCommand(args); | |
| return; | |
| } |
🧰 Tools
🪛 Biome (2.5.6)
[error] 78-78: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli.ts` around lines 78 - 84, Wrap the `agents` switch case body
containing the `config` declaration and `runAgentsCommand(args)` in braces,
ensuring `config` is scoped only to that case while preserving the existing
validation and command execution.
Source: Linters/SAST tools
| const record = store.get(id); | ||
| if (!record) throw new Error(`Unknown subagent id: ${id}`); | ||
|
|
||
| store.update(record.id, { status: "running", error: undefined }); | ||
| const profiles = await loadLocalAgentProfiles(config, record.workspaceRoot); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce workspace scope before starting the worker.
runAgentsWorker can load and execute any stored agent ID. A direct agents __worker invocation can therefore mutate and run an agent record from another workspace. Call assertAgentInScope(record, resolveCurrentWorkspaceScope(config)) before changing its status.
As per coding guidelines: treat every operation as workspace-scoped and use workspaceId as the opaque handle returned by open_workspace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli.ts` around lines 536 - 540, Update runAgentsWorker after retrieving
the record and before store.update to call assertAgentInScope with the record
and resolveCurrentWorkspaceScope(config). Preserve workspace scoping for all
operations, using the workspaceId returned by open_workspace as the opaque scope
handle.
Source: Coding guidelines
| if (isGeneratedPromptFile(promptFile)) { | ||
| await unlink(promptFile).catch(() => undefined); | ||
| } | ||
| store.close(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the generated prompt directory after cleanup.
writeAgentPromptFile creates a unique temporary directory. This block removes only prompt.txt, so every worker leaves an empty devspace-agent-prompt-* directory behind. Remove the empty generated directory after unlinking the prompt file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli.ts` around lines 573 - 576, Update the cleanup block after
isGeneratedPromptFile to remove the temporary directory created by
writeAgentPromptFile after unlinking promptFile. Preserve the existing
best-effort cleanup behavior and ensure the generated directory is removed only
after the prompt file cleanup completes.
| if (part?.startsWith("--")) { | ||
| throw new Error(`Unknown option: ${part}\n${USAGE}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject option-like tokens before consuming option values.
The new check runs after known options parse their values. Therefore, parseLocalAgentRunArgs(["codex", "--model", "--unknown", "hello"]) stores "--unknown" as model, and --effort=--unknown stores it as effort.
If model and effort values cannot start with --, reject such values in every model, effort, and thinking value branch. Add regression tests for separated and = forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/local-agent-targets.ts` around lines 67 - 69, Update the option-value
parsing branches in the local-agent argument parser so model, effort, and
thinking values beginning with “--” are rejected before assignment, for both
separated and equals forms. Preserve valid value handling, and add regression
tests covering each affected form, including parseLocalAgentRunArgs with
separated model and effort arguments.
| function resolveWorkflowFilePath(path: string, workspaceRoot: string): string { | ||
| const resolvedPath = resolve(workspaceRoot, path); | ||
| if (!isPathInsideRoot(resolvedPath, workspaceRoot)) { | ||
| throw new InvalidWorkflowInputError({ | ||
| code: "invalid_path", | ||
| message: `Workflow file must be inside the workspace: ${workspaceRoot}`, | ||
| }); | ||
| } | ||
| return resolvedPath; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Resolve symlinks before accepting a workflow file.
isPathInsideRoot only checks lexical paths. A symlink inside the workspace can point outside the workspace and still pass this check. Canonicalize both paths and enforce containment after canonicalization before launch.
Based on learnings: enforce lexical and canonical containment for workflow script paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflow-cli.ts` around lines 485 - 493, Update resolveWorkflowFilePath
to canonicalize both the resolved workflow path and workspaceRoot before the
containment check, while retaining the existing lexical isPathInsideRoot
validation. Enforce canonical containment before returning the path or launching
the workflow, and throw InvalidWorkflowInputError with code "invalid_path" when
either check fails.
Source: Learnings
| function assertWorkflowInScope( | ||
| run: Pick<WorkflowRunRecord, "workspaceRoot" | "workspaceId">, | ||
| scope: { workspaceRoot: string; workspaceId?: string }, | ||
| ): void { | ||
| if (resolve(run.workspaceRoot) !== resolve(scope.workspaceRoot)) { | ||
| throw new InvalidWorkflowInputError({ | ||
| code: "invalid_argument", | ||
| message: `Workflow run belongs to a different workspace: ${scope.workspaceRoot}`, | ||
| }); | ||
| } | ||
| if (scope.workspaceId && run.workspaceId && run.workspaceId !== scope.workspaceId) { | ||
| throw new InvalidWorkflowInputError({ | ||
| code: "invalid_argument", | ||
| message: `Workflow run belongs to a different workspaceId: ${scope.workspaceId}`, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Apply workspaceId consistently to workflow ownership.
When the active scope has a workspaceId, Line 506 permits a run with no workspaceId. Line 254 also lists every run for the root without an ID filter. This exposes legacy or other-scope records within the same root.
src/workflow-cli.ts#L496-L512: reject a run unless itsworkspaceIdexactly matches the activeworkspaceIdwhen one is present.src/workflow-cli.ts#L253-L254: extend the store query to filter byworkspaceIdbefore applying the result limit. Define an explicit migration or compatibility path for ID-less records.
As per coding guidelines: treat every operation as workspace-scoped and use workspaceId as the opaque handle returned by open_workspace.
📍 Affects 1 file
src/workflow-cli.ts#L496-L512(this comment)src/workflow-cli.ts#L253-L254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflow-cli.ts` around lines 496 - 512, Update src/workflow-cli.ts lines
496-512 in assertWorkflowInScope to require an exact workspaceId match whenever
the active scope provides one, rejecting runs with missing or different IDs.
Update src/workflow-cli.ts lines 253-254 to filter the store query by
workspaceId before applying the result limit, and define an explicit migration
or compatibility path for ID-less records; all workflow operations must remain
scoped by the opaque ID returned from open_workspace.
Source: Coding guidelines
|
Closing in favor of #142. The Sol implementation follows the CLI-only architecture through to completion: it removes the obsolete MCP workflow execution surface and its dependent UI/tooling instead of leaving the old contract partially alive. It also avoids the cross-workspace resume and secondary-entry-point issues found during review. |
1 participant
Long-running orchestration should not depend on an MCP request timeout or session lifetime. Workflow run, status, cancel, list, and call inspection now use validated CLI options and workspace-scoped state; agent commands clean up their stores and detached prompt handoffs. MCP no longer registers separate workflow execution tools, so every host uses the same CLI lifecycle.
Summary by CodeRabbit