feat(web): add orchestrator multi-stage pipeline engine (THE-206)#442
Open
StanGirard wants to merge 3 commits intomainfrom
Open
feat(web): add orchestrator multi-stage pipeline engine (THE-206)#442StanGirard wants to merge 3 commits intomainfrom
StanGirard wants to merge 3 commits intomainfrom
Conversation
Adds a complete multi-stage workflow execution engine for running ordered AI coding stages inside Docker containers. This is the runtime layer for the Orchestrator feature (THE-206). Backend: - OrchestratorConfig/Run types, file-based persistence store - OrchestratorExecutor: sequential stage execution with shared/per-stage container modes, cancellation, cost tracking - REST API routes (CRUD + run management) - WsBridge result listener for stage completion detection Frontend: - OrchestratorPage: list/edit views, stages builder, run dialog - OrchestratorRunView: real-time stage timeline with status, duration, cost - Sidebar navigation link, hash routing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR adds a comprehensive orchestrator system that chains multiple Claude/Codex sessions into sequential pipelines running in Docker containers. The implementation includes backend execution engine, file-based persistence, REST API, WebSocket integration for stage completion tracking, and polished frontend UI with real-time progress views. Major changes:
Issues found:
Positives:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Browser UI
participant API as Hono API
participant Exec as OrchestratorExecutor
participant Store as OrchestratorStore
participant Docker as ContainerManager
participant CLI as Claude CLI
participant WS as WsBridge
UI->>API: POST /orchestrators/:id/run
API->>Store: getOrchestrator(id)
Store-->>API: OrchestratorConfig
API->>Exec: startRun(id, input)
Exec->>Store: createRun(run)
Store-->>Exec: OrchestratorRun (pending)
Note over Exec: Async execution begins
Exec->>Docker: createContainer(runId, cwd, config)
Docker-->>Exec: containerId, containerName
Exec->>Docker: copyWorkspaceToContainer()
loop For each stage
Exec->>CLI: launch({model, cwd, containerId})
CLI-->>Exec: sessionId
Exec->>WS: injectUserMessage(sessionId, prompt)
Exec->>WS: onResultMessage(sessionId, callback)
WS-->>Exec: result (on completion)
Exec->>Store: updateRun({stages[i]: completed})
end
Exec->>Docker: removeContainer(runId)
Exec->>Store: updateRun({status: completed})
UI->>API: GET /orchestrator-runs/:runId (polling)
API->>Store: getRun(runId)
Store-->>API: OrchestratorRun (updated)
API-->>UI: run status + stages
Last reviewed commit: 1aefde6 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+266
to
+268
| } finally { | ||
| this.activeRuns.delete(runId); | ||
| } |
Contributor
There was a problem hiding this comment.
shared mode container is never cleaned up after the run completes, fails, or is cancelled
Suggested change
| } finally { | |
| this.activeRuns.delete(runId); | |
| } | |
| } finally { | |
| // Clean up shared container if it exists | |
| if (containerMode === "shared" && sharedContainerId) { | |
| try { | |
| containerManager.removeContainer(runId); | |
| } catch (err) { | |
| console.error(`[orchestrator-executor] Failed to remove shared container for run ${runId}:`, err); | |
| } | |
| } | |
| this.activeRuns.delete(runId); | |
| } |
Comment on lines
+140
to
+147
| // Clean up container if it exists | ||
| if (run.containerId) { | ||
| try { | ||
| containerManager.removeContainer(runId); | ||
| } catch { | ||
| // Best-effort cleanup | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
per-stage containers not cleaned up when deleting a run (only shared container is handled)
Suggested change
| // Clean up container if it exists | |
| if (run.containerId) { | |
| try { | |
| containerManager.removeContainer(runId); | |
| } catch { | |
| // Best-effort cleanup | |
| } | |
| } | |
| // Clean up container(s) if they exist | |
| if (run.containerId) { | |
| // Shared mode: single container with runId as key | |
| try { | |
| containerManager.removeContainer(runId); | |
| } catch { | |
| // Best-effort cleanup | |
| } | |
| } else { | |
| // Per-stage mode: multiple containers with runId-stage-N pattern | |
| // We don't know how many stages ran, so try to clean up based on config | |
| const orch = orchestratorStore.getOrchestrator(run.orchestratorId); | |
| if (orch) { | |
| for (let i = 0; i < orch.stages.length; i++) { | |
| try { | |
| containerManager.removeContainer(`${runId}-stage-${i}`); | |
| } catch { | |
| // Stage may not have run or already cleaned up | |
| } | |
| } | |
| } | |
| } |
Contributor
Additional Comments (2)
|
- Add shared container cleanup in executor's finally block - Handle per-stage container cleanup on run deletion - Fix flaky timing assertion in store test (toBeGreaterThanOrEqual) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
@greptile review |
Comment on lines
+279
to
+321
| private async setupContainer( | ||
| trackingKey: string, | ||
| config: OrchestratorConfig, | ||
| image: string, | ||
| env: ReturnType<typeof envManager.getEnv> & {}, | ||
| ): Promise<{ containerId: string; containerName: string }> { | ||
| console.log(`[orchestrator-executor] Creating container for ${trackingKey} with image ${image}`); | ||
|
|
||
| // Merge env variables from environment profile + orchestrator config | ||
| const envVars: Record<string, string> = { | ||
| ...(env.variables || {}), | ||
| ...(config.env || {}), | ||
| }; | ||
|
|
||
| const containerInfo = containerManager.createContainer(trackingKey, config.cwd, { | ||
| image, | ||
| ports: env.ports || [], | ||
| volumes: env.volumes, | ||
| env: Object.keys(envVars).length > 0 ? envVars : undefined, | ||
| }); | ||
|
|
||
| // Copy workspace files into the container | ||
| await containerManager.copyWorkspaceToContainer(containerInfo.containerId, config.cwd); | ||
| containerManager.reseedGitAuth(containerInfo.containerId); | ||
|
|
||
| // Run init script if configured | ||
| if (env.initScript?.trim()) { | ||
| console.log(`[orchestrator-executor] Running init script in container for ${trackingKey}`); | ||
| const result = await containerManager.execInContainerAsync( | ||
| containerInfo.containerId, | ||
| ["bash", "-lc", env.initScript], | ||
| { timeout: 120_000 }, | ||
| ); | ||
| if (result.exitCode !== 0) { | ||
| console.warn(`[orchestrator-executor] Init script exited with code ${result.exitCode}: ${result.output.slice(0, 200)}`); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| containerId: containerInfo.containerId, | ||
| containerName: containerInfo.name, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
container leaks if setupContainer fails after createContainer succeeds
If containerManager.createContainer() succeeds but copyWorkspaceToContainer() or the init script fails, the container is created but never cleaned up because the exception bubbles up before the container ID is saved to sharedContainerId or before per-stage cleanup can happen.
Wrap container creation in try/catch to ensure cleanup on partial failures:
private async setupContainer(...): Promise<{ containerId: string; containerName: string }> {
let containerId: string | undefined;
try {
const containerInfo = containerManager.createContainer(trackingKey, config.cwd, {...});
containerId = containerInfo.containerId;
await containerManager.copyWorkspaceToContainer(containerId, config.cwd);
containerManager.reseedGitAuth(containerId);
if (env.initScript?.trim()) {
// ... init script
}
return { containerId, containerName: containerInfo.name };
} catch (err) {
if (containerId) {
try {
containerManager.removeContainer(trackingKey);
} catch {}
}
throw err;
}
}
Comment on lines
+148
to
+160
| } else { | ||
| // Per-stage mode: try to clean up per-stage containers | ||
| const orch = orchestratorStore.getOrchestrator(run.orchestratorId); | ||
| if (orch) { | ||
| for (let i = 0; i < orch.stages.length; i++) { | ||
| try { | ||
| containerManager.removeContainer(`${runId}-stage-${i}`); | ||
| } catch { | ||
| // Stage may not have run or already cleaned up | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
per-stage container cleanup uses wrong stage count and fails if orchestrator was deleted
Two issues:
- Uses
orch.stages.lengthinstead ofrun.stages.length- if the orchestrator config was modified after the run, this will miss containers - If
orchis null (orchestrator was deleted), no cleanup happens at all
Suggested change
| } else { | |
| // Per-stage mode: try to clean up per-stage containers | |
| const orch = orchestratorStore.getOrchestrator(run.orchestratorId); | |
| if (orch) { | |
| for (let i = 0; i < orch.stages.length; i++) { | |
| try { | |
| containerManager.removeContainer(`${runId}-stage-${i}`); | |
| } catch { | |
| // Stage may not have run or already cleaned up | |
| } | |
| } | |
| } | |
| } | |
| } else { | |
| // Per-stage mode: try to clean up per-stage containers | |
| // Use run.stages.length since orchestrator config may have changed or been deleted | |
| for (let i = 0; i < run.stages.length; i++) { | |
| try { | |
| containerManager.removeContainer(`${runId}-stage-${i}`); | |
| } catch { | |
| // Stage may not have run or already cleaned up | |
| } | |
| } | |
| } |
- orchestrator-executor.ts: 74% → 97% (29 new tests covering per-stage mode, cancellation, error paths, container cleanup) - OrchestratorPage.tsx: 61% → 100% (43 new tests covering editor form, stage builder, save/delete, run modal, status colors) - orchestrator-api.ts: 0% → covered (21 new tests for all API functions) - orchestrator-types.ts (frontend + backend): import tests for type-only files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="web/src/components/OrchestratorRunView.tsx">
<violation number="1" location="web/src/components/OrchestratorRunView.tsx:170">
P2: Stale data flash when `runId` changes: `loading`, `run`, and `error` are not reset at the start of the effect. When navigating between runs, the previous run's data renders until the new fetch completes.</violation>
<violation number="2" location="web/src/components/OrchestratorRunView.tsx:181">
P2: Polling stops permanently on a transient network error. If `getRun` throws while the run is still active, no retry is scheduled. Consider adding a retry with backoff in the `catch` block (e.g., `timer = setTimeout(fetchRun, 5000)`) so that a single failed request doesn't kill the real-time view.</violation>
</file>
<file name="web/src/components/Sidebar.tsx">
<violation number="1" location="web/src/components/Sidebar.tsx:70">
P2: The orchestrator icon's connecting lines (e.g. `M8 1v3`, `M3.5 5.5L1.5 4`) are open stroke-only paths that will be invisible because the sidebar renders icons with `fill="currentColor"` and no `stroke`. Only the filled circles will display. Either redesign the icon to use filled shapes (e.g., thin rectangles for lines), or add stroke rendering support to the icon component.</violation>
</file>
<file name="web/src/components/OrchestratorPage.tsx">
<violation number="1" location="web/src/components/OrchestratorPage.tsx:278">
P2: The "View Runs" button will never scroll to the runs section because no element has `id="recent-runs"`. Add the missing `id` to the runs container.</violation>
</file>
<file name="web/server/orchestrator-store.ts">
<violation number="1" location="web/server/orchestrator-store.ts:31">
P1: Path traversal vulnerability: `orchestratorPath` and `runPath` accept unsanitized IDs from URL parameters. An ID containing `../` sequences can escape the intended directory, enabling reads, writes, or deletes of arbitrary `.json` files. Add path validation to reject IDs containing path separators or `..` segments.</violation>
<violation number="2" location="web/server/orchestrator-store.ts:123">
P2: Renaming an orchestrator changes its ID (slug), but existing runs still reference the old `orchestratorId`. After a rename, `listRuns(newId)` won't find them — those runs become orphaned. Either update all associated runs when the ID changes, or keep the ID immutable and separate from the slug.</violation>
</file>
<file name="web/src/orchestrator-api.ts">
<violation number="1" location="web/src/orchestrator-api.ts:16">
P1: These HTTP helpers (`get`, `post`, `put`, `del`, `getAuthHeaders`) duplicate the ones in `api.ts` but drop 401 handling and analytics tracking. When the server returns 401 on any orchestrator endpoint, the user won't be logged out or have their stale token cleared — they'll just see a generic error. Export the shared helpers from `api.ts` (or extract to a common module) instead of re-implementing them.</violation>
</file>
<file name="web/src/components/OrchestratorRunView.test.tsx">
<violation number="1" location="web/src/components/OrchestratorRunView.test.tsx:134">
P2: This test doesn't actually verify its claim that completed runs hide the Cancel button. The first render (running) is never unmounted, and the second render (completed) is unmounted without any assertion that Cancel is absent. Add an explicit assertion like `expect(screen.queryAllByText('Cancel')).toHaveLength(1)` before `unmount()`, and unmount the first render to properly isolate the two scenarios.</violation>
</file>
<file name="web/server/orchestrator-executor.ts">
<violation number="1" location="web/server/orchestrator-executor.ts:116">
P2: Silent no-op when a run is active in the store but missing from the in-memory `activeRuns` map. The method returns successfully without cancelling anything, leaving the run stuck in `running`/`pending` state. At minimum, update the store status to `cancelled` so the run doesn't appear permanently stuck.</violation>
<violation number="2" location="web/server/orchestrator-executor.ts:193">
P2: Per-stage Docker containers leak if `setupContainer` throws after creating the container (e.g., during workspace copy or init script). The `finally` block only handles `shared` mode cleanup. Consider wrapping the per-stage container setup and stage execution in a try/finally that removes the per-stage container on failure.</violation>
</file>
<file name="web/server/routes/orchestrator-routes.ts">
<violation number="1" location="web/server/routes/orchestrator-routes.ts:150">
P1: Per-stage container cleanup uses the current orchestrator config (`orch.stages.length`) instead of the run's own `stages` array. If the orchestrator was modified or deleted after the run, containers will leak. Use `run.stages.length` directly — it already snapshots the stages at run time and is always available.</violation>
</file>
<file name="web/server/orchestrator-executor.test.ts">
<violation number="1" location="web/server/orchestrator-executor.test.ts:662">
P2: This test is named "should handle waitForCLIConnection timeout" but it never exercises the timeout path. It transitions the session to `"exited"` state, hitting the same `"CLI process exited before connecting"` branch already covered by the test at line 507. The actual timeout error (`"CLI process did not connect within 30s"` at executor line 430) remains untested.
Consider using `vi.useFakeTimers()` to advance past `CLI_CONNECT_TIMEOUT_MS` without a real 30s wait, or rename this test to clarify what it actually covers.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| mkdirSync(RUNS_DIR, { recursive: true }); | ||
| } | ||
|
|
||
| function orchestratorPath(id: string): string { |
There was a problem hiding this comment.
P1: Path traversal vulnerability: orchestratorPath and runPath accept unsanitized IDs from URL parameters. An ID containing ../ sequences can escape the intended directory, enabling reads, writes, or deletes of arbitrary .json files. Add path validation to reject IDs containing path separators or .. segments.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/orchestrator-store.ts, line 31:
<comment>Path traversal vulnerability: `orchestratorPath` and `runPath` accept unsanitized IDs from URL parameters. An ID containing `../` sequences can escape the intended directory, enabling reads, writes, or deletes of arbitrary `.json` files. Add path validation to reject IDs containing path separators or `..` segments.</comment>
<file context>
@@ -0,0 +1,232 @@
+ mkdirSync(RUNS_DIR, { recursive: true });
+}
+
+function orchestratorPath(id: string): string {
+ return join(ORCHESTRATORS_DIR, `${id}.json`);
+}
</file context>
| @@ -0,0 +1,99 @@ | |||
| import type { | |||
There was a problem hiding this comment.
P1: These HTTP helpers (get, post, put, del, getAuthHeaders) duplicate the ones in api.ts but drop 401 handling and analytics tracking. When the server returns 401 on any orchestrator endpoint, the user won't be logged out or have their stale token cleared — they'll just see a generic error. Export the shared helpers from api.ts (or extract to a common module) instead of re-implementing them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/orchestrator-api.ts, line 16:
<comment>These HTTP helpers (`get`, `post`, `put`, `del`, `getAuthHeaders`) duplicate the ones in `api.ts` but drop 401 handling and analytics tracking. When the server returns 401 on any orchestrator endpoint, the user won't be logged out or have their stale token cleared — they'll just see a generic error. Export the shared helpers from `api.ts` (or extract to a common module) instead of re-implementing them.</comment>
<file context>
@@ -0,0 +1,99 @@
+ return { Authorization: `Bearer ${token}` };
+}
+
+async function get<T = unknown>(path: string): Promise<T> {
+ const res = await fetch(`${BASE}${path}`, {
+ headers: { ...getAuthHeaders() },
</file context>
| } | ||
| } else { | ||
| // Per-stage mode: try to clean up per-stage containers | ||
| const orch = orchestratorStore.getOrchestrator(run.orchestratorId); |
There was a problem hiding this comment.
P1: Per-stage container cleanup uses the current orchestrator config (orch.stages.length) instead of the run's own stages array. If the orchestrator was modified or deleted after the run, containers will leak. Use run.stages.length directly — it already snapshots the stages at run time and is always available.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/routes/orchestrator-routes.ts, line 150:
<comment>Per-stage container cleanup uses the current orchestrator config (`orch.stages.length`) instead of the run's own `stages` array. If the orchestrator was modified or deleted after the run, containers will leak. Use `run.stages.length` directly — it already snapshots the stages at run time and is always available.</comment>
<file context>
@@ -0,0 +1,166 @@
+ }
+ } else {
+ // Per-stage mode: try to clean up per-stage containers
+ const orch = orchestratorStore.getOrchestrator(run.orchestratorId);
+ if (orch) {
+ for (let i = 0; i < orch.stages.length; i++) {
</file context>
| if (data.status === "running" || data.status === "pending") { | ||
| timer = setTimeout(fetchRun, 3000); | ||
| } | ||
| } catch (e: unknown) { |
There was a problem hiding this comment.
P2: Polling stops permanently on a transient network error. If getRun throws while the run is still active, no retry is scheduled. Consider adding a retry with backoff in the catch block (e.g., timer = setTimeout(fetchRun, 5000)) so that a single failed request doesn't kill the real-time view.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/components/OrchestratorRunView.tsx, line 181:
<comment>Polling stops permanently on a transient network error. If `getRun` throws while the run is still active, no retry is scheduled. Consider adding a retry with backoff in the `catch` block (e.g., `timer = setTimeout(fetchRun, 5000)`) so that a single failed request doesn't kill the real-time view.</comment>
<file context>
@@ -0,0 +1,364 @@
+ if (data.status === "running" || data.status === "pending") {
+ timer = setTimeout(fetchRun, 3000);
+ }
+ } catch (e: unknown) {
+ if (cancelled) return;
+ setError(e instanceof Error ? e.message : String(e));
</file context>
| let cancelled = false; | ||
| let timer: ReturnType<typeof setTimeout> | null = null; | ||
|
|
||
| async function fetchRun() { |
There was a problem hiding this comment.
P2: Stale data flash when runId changes: loading, run, and error are not reset at the start of the effect. When navigating between runs, the previous run's data renders until the new fetch completes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/components/OrchestratorRunView.tsx, line 170:
<comment>Stale data flash when `runId` changes: `loading`, `run`, and `error` are not reset at the start of the effect. When navigating between runs, the previous run's data renders until the new fetch completes.</comment>
<file context>
@@ -0,0 +1,364 @@
+ let cancelled = false;
+ let timer: ReturnType<typeof setTimeout> | null = null;
+
+ async function fetchRun() {
+ try {
+ const data = await orchestratorApi.getRun(runId);
</file context>
| if (!newId) throw new Error("Orchestrator name must contain alphanumeric characters"); | ||
|
|
||
| // If name changed, check for slug collision with a different orchestrator | ||
| if (newId !== id && existsSync(orchestratorPath(newId))) { |
There was a problem hiding this comment.
P2: Renaming an orchestrator changes its ID (slug), but existing runs still reference the old orchestratorId. After a rename, listRuns(newId) won't find them — those runs become orphaned. Either update all associated runs when the ID changes, or keep the ID immutable and separate from the slug.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/orchestrator-store.ts, line 123:
<comment>Renaming an orchestrator changes its ID (slug), but existing runs still reference the old `orchestratorId`. After a rename, `listRuns(newId)` won't find them — those runs become orphaned. Either update all associated runs when the ID changes, or keep the ID immutable and separate from the slug.</comment>
<file context>
@@ -0,0 +1,232 @@
+ if (!newId) throw new Error("Orchestrator name must contain alphanumeric characters");
+
+ // If name changed, check for slug collision with a different orchestrator
+ if (newId !== id && existsSync(orchestratorPath(newId))) {
+ throw new Error(`An orchestrator with a similar name already exists ("${newId}")`);
+ }
</file context>
| // "completed" appears as the status badge | ||
| expect(screen.getAllByText("completed").length).toBeGreaterThanOrEqual(1); | ||
| }); | ||
| // Cancel button should NOT appear for the completed run (only 1 Cancel total from the running run above) |
There was a problem hiding this comment.
P2: This test doesn't actually verify its claim that completed runs hide the Cancel button. The first render (running) is never unmounted, and the second render (completed) is unmounted without any assertion that Cancel is absent. Add an explicit assertion like expect(screen.queryAllByText('Cancel')).toHaveLength(1) before unmount(), and unmount the first render to properly isolate the two scenarios.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/src/components/OrchestratorRunView.test.tsx, line 134:
<comment>This test doesn't actually verify its claim that completed runs hide the Cancel button. The first render (running) is never unmounted, and the second render (completed) is unmounted without any assertion that Cancel is absent. Add an explicit assertion like `expect(screen.queryAllByText('Cancel')).toHaveLength(1)` before `unmount()`, and unmount the first render to properly isolate the two scenarios.</comment>
<file context>
@@ -0,0 +1,344 @@
+ // "completed" appears as the status badge
+ expect(screen.getAllByText("completed").length).toBeGreaterThanOrEqual(1);
+ });
+ // Cancel button should NOT appear for the completed run (only 1 Cancel total from the running run above)
+ unmount();
+ });
</file context>
| let stageContainerName = sharedContainerName; | ||
|
|
||
| // Per-stage container mode: create a fresh container for each stage | ||
| if (containerMode === "per-stage") { |
There was a problem hiding this comment.
P2: Per-stage Docker containers leak if setupContainer throws after creating the container (e.g., during workspace copy or init script). The finally block only handles shared mode cleanup. Consider wrapping the per-stage container setup and stage execution in a try/finally that removes the per-stage container on failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/orchestrator-executor.ts, line 193:
<comment>Per-stage Docker containers leak if `setupContainer` throws after creating the container (e.g., during workspace copy or init script). The `finally` block only handles `shared` mode cleanup. Consider wrapping the per-stage container setup and stage execution in a try/finally that removes the per-stage container on failure.</comment>
<file context>
@@ -0,0 +1,488 @@
+ let stageContainerName = sharedContainerName;
+
+ // Per-stage container mode: create a fresh container for each stage
+ if (containerMode === "per-stage") {
+ const containerResult = await this.setupContainer(
+ `${runId}-stage-${i}`,
</file context>
| if (run.status !== "running" && run.status !== "pending") { | ||
| throw new Error(`Run "${runId}" is not active (status: ${run.status})`); | ||
| } | ||
| return; |
There was a problem hiding this comment.
P2: Silent no-op when a run is active in the store but missing from the in-memory activeRuns map. The method returns successfully without cancelling anything, leaving the run stuck in running/pending state. At minimum, update the store status to cancelled so the run doesn't appear permanently stuck.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/orchestrator-executor.ts, line 116:
<comment>Silent no-op when a run is active in the store but missing from the in-memory `activeRuns` map. The method returns successfully without cancelling anything, leaving the run stuck in `running`/`pending` state. At minimum, update the store status to `cancelled` so the run doesn't appear permanently stuck.</comment>
<file context>
@@ -0,0 +1,488 @@
+ if (run.status !== "running" && run.status !== "pending") {
+ throw new Error(`Run "${runId}" is not active (status: ${run.status})`);
+ }
+ return;
+ }
+
</file context>
| expect(failedRun.error).toContain("CLI process exited before connecting"); | ||
| }); | ||
|
|
||
| it("should handle waitForCLIConnection timeout", async () => { |
There was a problem hiding this comment.
P2: This test is named "should handle waitForCLIConnection timeout" but it never exercises the timeout path. It transitions the session to "exited" state, hitting the same "CLI process exited before connecting" branch already covered by the test at line 507. The actual timeout error ("CLI process did not connect within 30s" at executor line 430) remains untested.
Consider using vi.useFakeTimers() to advance past CLI_CONNECT_TIMEOUT_MS without a real 30s wait, or rename this test to clarify what it actually covers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At web/server/orchestrator-executor.test.ts, line 662:
<comment>This test is named "should handle waitForCLIConnection timeout" but it never exercises the timeout path. It transitions the session to `"exited"` state, hitting the same `"CLI process exited before connecting"` branch already covered by the test at line 507. The actual timeout error (`"CLI process did not connect within 30s"` at executor line 430) remains untested.
Consider using `vi.useFakeTimers()` to advance past `CLI_CONNECT_TIMEOUT_MS` without a real 30s wait, or rename this test to clarify what it actually covers.</comment>
<file context>
@@ -0,0 +1,1318 @@
+ expect(failedRun.error).toContain("CLI process exited before connecting");
+ });
+
+ it("should handle waitForCLIConnection timeout", async () => {
+ // Covers lines 429-432: CLI never connects within timeout
+ // We need to temporarily override the timeout to make this test fast.
</file context>
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
What it does
Users can define multi-stage pipelines (e.g. "implement → test → review") that chain multiple Claude/Codex sessions sequentially inside a shared Docker container. Each stage gets its own child session, and the timeline view shows real-time progress with status, duration, costs, and links to individual sessions.
Files changed
Testing
Review provenance