refactor(mcp): move workflow orchestration to the CLI - #142
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe change replaces workflow tools and detailed workflow views with active workflow summaries. The server exposes simplified agent and workflow data. The workspace UI removes workflow polling and renders summary information directly. ChangesWorkflow summary integration
Workspace UI simplification
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 makes dynamic workflow orchestration CLI-only, simplifies the workspace application payload, and replaces the detailed workflow UI with compact active-run summaries.
Confidence Score: 4/5The workflow summary scoping defect should be fixed before merging because active CLI-created workflows can disappear from open_workspace results. open_workspace always supplies a fresh workspace ID, causing the store to ignore the matching project root and exclude active runs created by ordinary CLI sessions; the bundled skill also still advertises the removed MCP tools. Files Needing Attention: src/server.ts and src/workflow-summary.ts
|
| Filename | Overview |
|---|---|
| src/server.ts | Removes MCP workflow registration and reduces open_workspace output, but the new workspace-ID scope misses root-only CLI workflow runs. |
| src/workflow-summary.ts | Introduces compact active-workflow summaries with consistent call aggregation, while inheriting workspace-ID-preferred filtering from the store. |
| src/workflow-tools.ts | Deletes the MCP orchestration and workflow-dashboard tool surface as part of the intended CLI-only contract. |
| src/ui/workspace-app.tsx | Removes workflow polling and detailed workflow-card rendering consistently with the deleted MCP UI tools. |
| src/ui/workflow-dashboard.ts | Simplifies the workspace dashboard to render static compact workflow summaries and reduced provider/profile metadata. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[open_workspace] --> B[Generate workspace ID]
B --> C[loadActiveWorkflowSummaries]
C --> D{workspaceId present?}
D -->|Yes| E[Query runs by workspace_id only]
D -->|No| F[Query runs by workspace_root]
G[CLI workflow run without injected ID] --> H[Persist run with project root only]
H -. missed by .-> E
Reviews (1): Last reviewed commit: "fix(mcp): scope workflow summaries by wo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2c242a6 to
f421b2c
Compare
f421b2c to
869b57f
Compare
869b57f to
8766c96
Compare
8766c96 to
7d602ea
Compare
[gpt-5.4] RESPONDING ON BEHALF OF WAISHNAVFixed the workflow visibility issue across The stale MCP skill guidance is also valid, but it belongs to the intentionally separate skill layer immediately above this PR. PR #143 removes those MCP instructions and provider internals in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/server.ts (1)
276-281: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the workflow status enum from the shared constant.
src/workflow-summary.tsline 4 definesACTIVE_WORKFLOW_STATUSESas the single source of active statuses. Line 279 repeats the same literals. If a status is added toACTIVE_WORKFLOW_STATUSES, this schema rejects the new value during output validation, andopen_workspacefails at runtime instead of at compile time.Export the constant and build the enum from it, so the two stay in sync.
♻️ Proposed change to remove the duplicated literal set
In
src/workflow-summary.ts, export the constant:-const ACTIVE_WORKFLOW_STATUSES = ["starting", "running"] as const satisfies readonly WorkflowRunStatus[]; +export const ACTIVE_WORKFLOW_STATUSES = ["starting", "running"] as const satisfies readonly WorkflowRunStatus[];In
src/server.ts, import it and derive the enum:-import { loadActiveWorkflowSummaries } from "./workflow-summary.js"; +import { ACTIVE_WORKFLOW_STATUSES, loadActiveWorkflowSummaries } from "./workflow-summary.js";const workflowRunSummaryOutputSchema = z.object({ id: z.string(), name: z.string(), - status: z.enum(["starting", "running"]), + status: z.enum(ACTIVE_WORKFLOW_STATUSES), calls: workflowCallCountsOutputSchema, });Confirm that
z.enum()accepts a readonly tuple in zod 4.4.3 before you apply this change.🤖 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/server.ts` around lines 276 - 281, Export ACTIVE_WORKFLOW_STATUSES from workflow-summary.ts, then import it in server.ts and derive workflowRunSummaryOutputSchema.status from that shared constant instead of duplicating the literals. Confirm the installed Zod version accepts the constant’s readonly tuple in z.enum(); preserve the existing validation behavior while keeping future status additions synchronized.src/workflow-summary.test.ts (1)
51-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a run owned by the queried workspace ID.
The fixture contains one root-only run and one run owned by
workspace-2. The assertion proves that root-only runs are included and foreign runs are excluded. It does not prove that a run created withworkspaceId: "workspace-1"is returned. A regression that dropped matching-ID runs would still pass.Also consider covering
failedcalls and thefrom_cachebranch inloadActiveWorkflowSummaries, because both aggregation paths are currently unexercised.💚 Proposed additional fixture and assertion
+ const owned = store.createRun({ + name: "Owned", + source: "named", + scriptPath: join(root, "owned.js"), + scriptHash: "owned", + workspaceRoot, + workspaceId: "workspace-1", + }); store.claimRun(run.id, process.pid);assert.deepEqual(loadActiveWorkflowSummaries(store, { workspaceId: "workspace-1", workspaceRoot, }), [ + { + id: owned.id, + name: "Owned", + status: "starting", + calls: { running: 0, completed: 0, failed: 0 }, + }, { id: run.id, name: "Review", status: "running", calls: { running: 1, completed: 1, failed: 0 }, }, ]);Confirm the ordering returned by
listRunsForScopebefore you fix the expected array order.🤖 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-summary.test.ts` around lines 51 - 61, Add test coverage in the workflow summary fixture for a run explicitly owned by workspace-1, then assert it is returned by loadActiveWorkflowSummaries alongside the root-owned run while workspace-2 remains excluded. Confirm listRunsForScope ordering before setting the expected array order, and extend the assertions to exercise failed call aggregation and the from_cache branch.
🤖 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/open-workspace-capabilities.test.ts`:
- Around line 48-73: Update the `skillDiagnostics` assertion in the
`enabledSchema` test to inspect the schema shape via the existing `fields()`
helper, asserting that `skillDiagnostics` is absent from the declared keys. Do
not rely solely on `"skillDiagnostics" in parsed`, since the parsed input does
not include that field; optionally add a parse-level stripping check only if
supported by the project’s Zod version.
In `@src/ui/card-types.ts`:
- Around line 59-64: Use a loose card-side shape for activeWorkflows in
src/ui/card-types.ts lines 59-64, with optional id, name, status, and Partial
calls, while preserving ActiveWorkflowSummary as the server contract. In
src/ui/workflow-dashboard.ts lines 127-141, add nullish fallbacks when reading
run.name and run.status and update summaryCounts to accept optional partial
calls.
---
Nitpick comments:
In `@src/server.ts`:
- Around line 276-281: Export ACTIVE_WORKFLOW_STATUSES from workflow-summary.ts,
then import it in server.ts and derive workflowRunSummaryOutputSchema.status
from that shared constant instead of duplicating the literals. Confirm the
installed Zod version accepts the constant’s readonly tuple in z.enum();
preserve the existing validation behavior while keeping future status additions
synchronized.
In `@src/workflow-summary.test.ts`:
- Around line 51-61: Add test coverage in the workflow summary fixture for a run
explicitly owned by workspace-1, then assert it is returned by
loadActiveWorkflowSummaries alongside the root-owned run while workspace-2
remains excluded. Confirm listRunsForScope ordering before setting the expected
array order, and extend the assertions to exercise failed call aggregation and
the from_cache branch.
🪄 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: 126a3642-992e-49a2-930c-282a0057e49b
📒 Files selected for processing (15)
package.jsonsrc/open-workspace-capabilities.test.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/icons.tssrc/ui/tool-display.test.tssrc/ui/tool-display.tssrc/ui/workflow-dashboard.tssrc/ui/workspace-app.tsxsrc/workflow-summary.test.tssrc/workflow-summary.tssrc/workflow-tools.tssrc/workflow-ui.test.tssrc/workflow-ui.ts
💤 Files with no reviewable changes (7)
- src/ui/icons.ts
- src/ui/tool-display.ts
- src/workflow-tools.ts
- src/ui/tool-display.test.ts
- src/workflow-ui.ts
- src/workflow-ui.test.ts
- src/ui/card-types.test.ts
| const parsed = enabledSchema.parse({ | ||
| workspaceId: "workspace-1", | ||
| root: process.cwd(), | ||
| mode: "checkout", | ||
| agentsFiles: [], | ||
| availableAgentsFiles: [], | ||
| skills: [], | ||
| agentProviders: ["codex"], | ||
| agents: [{ name: "reviewer", description: "Review changes." }], | ||
| activeWorkflows: [{ | ||
| id: "wfr_1", | ||
| name: "Review", | ||
| status: "running", | ||
| calls: { running: 1, completed: 2, failed: 0 }, | ||
| }], | ||
| instruction: "Reuse this workspace.", | ||
| }); | ||
| assert.deepEqual(parsed.agentProviders, ["codex"]); | ||
| assert.deepEqual(parsed.agents, [{ name: "reviewer", description: "Review changes." }]); | ||
| assert.deepEqual(parsed.activeWorkflows, [{ | ||
| id: "wfr_1", | ||
| name: "Review", | ||
| status: "running", | ||
| calls: { running: 1, completed: 2, failed: 0 }, | ||
| }]); | ||
| assert.equal("skillDiagnostics" in parsed, false); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Line 73 does not test what it intends to test.
The parsed input at lines 48-64 never contains skillDiagnostics. "skillDiagnostics" in parsed is therefore false no matter what the schema declares. The assertion passes even if openWorkspaceOutputSchema reintroduces the field.
Assert against the schema shape instead. The file already has a fields() helper at lines 16-18 that returns the shape keys.
💚 Proposed stronger assertion
-assert.equal("skillDiagnostics" in parsed, false);
+assert.equal(
+ fields({ ...baseEnv, DEVSPACE_SUBAGENTS: "1", DEVSPACE_WORKFLOWS: "1" }).has("skillDiagnostics"),
+ false,
+);If you want to keep a parse-level check as well, pass the field in the input and confirm that z.object strips it:
+const stripped = enabledSchema.parse({
+ ...validInput,
+ skillDiagnostics: [{ path: "a", message: "b" }],
+});
+assert.equal("skillDiagnostics" in stripped, false);Confirm the strip-by-default behavior of z.object in zod 4.4.3 before you rely on the second form.
📝 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 parsed = enabledSchema.parse({ | |
| workspaceId: "workspace-1", | |
| root: process.cwd(), | |
| mode: "checkout", | |
| agentsFiles: [], | |
| availableAgentsFiles: [], | |
| skills: [], | |
| agentProviders: ["codex"], | |
| agents: [{ name: "reviewer", description: "Review changes." }], | |
| activeWorkflows: [{ | |
| id: "wfr_1", | |
| name: "Review", | |
| status: "running", | |
| calls: { running: 1, completed: 2, failed: 0 }, | |
| }], | |
| instruction: "Reuse this workspace.", | |
| }); | |
| assert.deepEqual(parsed.agentProviders, ["codex"]); | |
| assert.deepEqual(parsed.agents, [{ name: "reviewer", description: "Review changes." }]); | |
| assert.deepEqual(parsed.activeWorkflows, [{ | |
| id: "wfr_1", | |
| name: "Review", | |
| status: "running", | |
| calls: { running: 1, completed: 2, failed: 0 }, | |
| }]); | |
| assert.equal("skillDiagnostics" in parsed, false); | |
| const parsed = enabledSchema.parse({ | |
| workspaceId: "workspace-1", | |
| root: process.cwd(), | |
| mode: "checkout", | |
| agentsFiles: [], | |
| availableAgentsFiles: [], | |
| skills: [], | |
| agentProviders: ["codex"], | |
| agents: [{ name: "reviewer", description: "Review changes." }], | |
| activeWorkflows: [{ | |
| id: "wfr_1", | |
| name: "Review", | |
| status: "running", | |
| calls: { running: 1, completed: 2, failed: 0 }, | |
| }], | |
| instruction: "Reuse this workspace.", | |
| }); | |
| assert.deepEqual(parsed.agentProviders, ["codex"]); | |
| assert.deepEqual(parsed.agents, [{ name: "reviewer", description: "Review changes." }]); | |
| assert.deepEqual(parsed.activeWorkflows, [{ | |
| id: "wfr_1", | |
| name: "Review", | |
| status: "running", | |
| calls: { running: 1, completed: 2, failed: 0 }, | |
| }]); | |
| assert.equal( | |
| fields({ ...baseEnv, DEVSPACE_SUBAGENTS: "1", DEVSPACE_WORKFLOWS: "1" }).has("skillDiagnostics"), | |
| false, | |
| ); |
🤖 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/open-workspace-capabilities.test.ts` around lines 48 - 73, Update the
`skillDiagnostics` assertion in the `enabledSchema` test to inspect the schema
shape via the existing `fields()` helper, asserting that `skillDiagnostics` is
absent from the declared keys. Do not rely solely on `"skillDiagnostics" in
parsed`, since the parsed input does not include that field; optionally add a
parse-level stripping check only if supported by the project’s Zod version.
| activeWorkflows?: ActiveWorkflowSummary[]; | ||
| agentProviders?: string[]; | ||
| agents?: Array<{ | ||
| name?: string; | ||
| description?: string; | ||
| provider?: string; | ||
| model?: string; | ||
| effort?: string; | ||
| }>; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Strict ActiveWorkflowSummary type applied to an unvalidated host payload. The card adopts the server-side contract type, which requires id, name, status, and calls. The value arrives through structuredContent from the host, and isToolResultCard only checks that the value is an object. The dashboard then dereferences the required fields, so a malformed entry throws a TypeError before container.replaceChildren(root) runs and the whole workspace dashboard fails to render.
src/ui/card-types.ts#L59-L64: declare a loose card-side shape foractiveWorkflowswith optionalid,name,status, and aPartialcalls; keepActiveWorkflowSummaryas the server-side contract.src/ui/workflow-dashboard.ts#L127-L141: apply??fallbacks forrun.nameandrun.status, and changesummaryCountsto accept an optional partialcallsobject.
📍 Affects 2 files
src/ui/card-types.ts#L59-L64(this comment)src/ui/workflow-dashboard.ts#L127-L141
🤖 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/ui/card-types.ts` around lines 59 - 64, Use a loose card-side shape for
activeWorkflows in src/ui/card-types.ts lines 59-64, with optional id, name,
status, and Partial calls, while preserving ActiveWorkflowSummary as the server
contract. In src/ui/workflow-dashboard.ts lines 127-141, add nullish fallbacks
when reading run.name and run.status and update summaryCounts to accept optional
partial calls.
1 participant
Dedicated MCP workflow tools duplicated the CLI contract and tied long-running orchestration to a request-response transport. This layer removes those execution tools and their live workflow dashboard while keeping the host as the visible orchestrator through ordinary shell or process tools.
open_workspaceremains model-useful but small: usable provider names, profile name and description, and active workflowid,name,status, plus running, completed, and failed call counts. Active runs are scoped by workspace identity so separate workspaces on the same checkout do not leak into each other.Verified with
npm run typecheck, focused workspace-summary tests, and the full test suite.Summary by CodeRabbit
Improvements
Changes