Add dynamic workflow orchestration for local coding agents#83
Add dynamic workflow orchestration for local coding agents#83Waishnav wants to merge 25 commits into
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds durable workflow execution with SQLite persistence, DAG scheduling, supervisor leases, isolated worktrees, streamed local-agent adapters, a restricted JavaScript runtime, CLI/MCP interfaces, workspace policies, replay semantics, graceful process termination, and integration tests. ChangesDurable workflow execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkflowCLI
participant WorkflowOrchestrator
participant WorkflowStore
participant WorkflowSupervisor
participant LocalAgentAdapter
Client->>WorkflowCLI: submit workflow
WorkflowCLI->>WorkflowOrchestrator: create and submit request
WorkflowOrchestrator->>WorkflowStore: persist snapshot and events
WorkflowCLI->>WorkflowSupervisor: ensure supervisor
WorkflowSupervisor->>WorkflowStore: claim ready node
WorkflowSupervisor->>LocalAgentAdapter: start provider run
LocalAgentAdapter-->>WorkflowSupervisor: stream events and result
WorkflowSupervisor->>WorkflowStore: persist completion
Client->>WorkflowCLI: wait or read events
WorkflowCLI->>WorkflowOrchestrator: query durable state
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 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/db/schema.ts`:
- Line 117: Scope workflow idempotency keys to workspaces instead of enforcing
global uniqueness: in src/db/schema.ts lines 117-117 replace the column-level
unique constraint with workspace-scoped unique indexes; in src/db/migrations.ts
lines 206-206 create matching constraints, including explicit handling for
unscoped workflows; and in src/workflows/store.ts lines 224-257 include the
normalized workspace ID/root in replay and conflict lookups.
In `@src/local-agent-adapters.ts`:
- Around line 311-326: Update the final response extraction in the OpenCode
session flow to use the assistant message returned by session.messages() after
session.wait(), rather than promptResult, which is only a SessionInputAdmitted
acknowledgement. Pass the appropriate messages-derived assistant response to
extractOpenCodeFinalResponse while preserving the existing requireFinalResponse
handling.
In `@src/local-agent-runtime.ts`:
- Around line 458-476: Bound provider result aggregation in both sites: in
src/local-agent-runtime.ts lines 458-476, replace the unbounded items collection
used by the providerEvents loop with a bounded collection while preserving
finalResponse and session emission behavior; in src/local-agent-adapters.ts
lines 385-386, replace unbounded ACP textParts accumulation with a bounded
character buffer. Use the existing aggregation symbols and enforce limits
throughout each run.
In `@src/process-platform.test.ts`:
- Around line 77-80: Update the test process mock’s kill method to stop
assigning signalCode synchronously; retain recording the signal and
asynchronously emitting "close" so the wait helper must rely on its close-event
listener.
In `@src/process-platform.ts`:
- Around line 105-108: Update the termination flow around terminateProcessTree
and waitForProcessTreeExit so it checks the final wait result after SIGKILL. If
the child or detached process group is still alive, propagate/report disposal
failure instead of returning success; preserve the existing graceful SIGTERM
path.
In `@src/workflows/cli.ts`:
- Around line 174-179: Keep durable workflow acceptance separate from fallible
supervisor startup: in src/workflows/cli.ts lines 174-179, update the submission
flow around orchestrator.submitDetailed and ensureSupervisor to return the
accepted workflow ID while exposing supervisor startup failure separately; in
src/workflows/mcp.ts lines 151-152, return the accepted workflow after commit
instead of throwing on startup failure, or require an idempotency key before
allowing retries.
In `@src/workflows/dag-scheduler.test.ts`:
- Line 163: Replace the Unix-specific execFileSync("mkdir", ["-p", repo]) setup
in the DAG scheduler test with the platform-independent filesystem API, using
recursive directory creation for repo and preserving the existing test setup
behavior.
In `@src/workflows/mcp.ts`:
- Around line 105-109: Update the workflow_run tool’s annotations to set
idempotentHint to false, while preserving the other hints. This ensures the tool
accurately reflects that repeated invocations without an idempotencyKey create
separate workflows.
In `@src/workflows/runtime-journal.ts`:
- Around line 108-150: Update the state-transition methods shown in the diff,
including the run creation flow around submit and the additional transition
ranges, so each state mutation and its corresponding event-row insert execute
within the same immediate SQLite transaction. Move event persistence into the
transaction callback after the mutation, and return the resulting state only
after both writes succeed; remove separate post-commit appendEvent calls while
preserving existing event payloads and transition behavior.
In `@src/workflows/runtime.ts`:
- Around line 159-175: Move the journal.submit(), successful-replay return, and
journal.resume() flow into the resource-owning try/finally immediately after
constructing journal and orchestrator. Ensure both journal and an internally
created orchestrator are closed for submission, idempotency, or resume errors,
while preserving the existing replay return behavior.
- Around line 176-210: Update the runtime workflow cleanup around scheduleAgent,
the timeout handler, and the catch/finally paths to be abort-aware: when the
configured timeout fires, propagate cancellation to active operations and avoid
awaiting the normal waitForWorkspace loop indefinitely. Use an abort signal or a
separately bounded cancellation grace period so cleanup completes promptly while
preserving normal workflow behavior before timeout.
In `@src/workflows/store.ts`:
- Around line 683-749: The dispatch ordering in the claim transaction is not
strictly monotonic because last_dispatched_at can tie at millisecond precision.
Update the workflow-run dispatch bookkeeping around the last_dispatched_at
update and candidate ORDER BY to persist and use a strictly increasing dispatch
sequence or equivalent total-order value, ensuring tied timestamps cannot
repeatedly favor older runs while preserving the existing fairness ordering.
- Around line 803-894: Update markNodeDispatching, markNodeRunning,
markNodeCancelling, recordNodeProviderSession, and appendNodeExecutionEvent so
every attempt mutation is conditionally authorized by the current active node
lease and supervisor lease. Atomically validate the attempt identity, node claim
token and expiry, and supervisor owner/epoch/lease before phase, session, or
provider-event writes; return the existing failure result when validation fails
and preserve idempotent event handling.
In `@src/workflows/submission.ts`:
- Around line 71-73: Update the hasWriteNode calculation in the submission
workflow to account for the workflow-level read_only clamp before resolving
baseSha. Only treat nodes as workspace_write when the effective workflow access
permits it; preserve baseSha as undefined for fully read-only workflows so they
remain valid outside Git repositories.
In `@src/workflows/supervisor.test.ts`:
- Around line 79-84: Increase the startup timeout passed to ensureSupervisor in
the supervisor test to a larger platform-independent bound that accommodates
macOS CI before durable lease acquisition. Keep the existing supervisor setup
and readiness behavior unchanged.
In `@src/workflows/supervisor.ts`:
- Around line 55-75: The supervisor setup currently creates the WorkflowStore
before validation and performs acquisition outside the try/finally cleanup
scope. Move validateSupervisorTiming before constructing WorkflowStore, then
include acquireSupervisor and the failed-acquisition close path within the
protected try/finally lifetime so setup exceptions always close the store.
- Around line 183-255: Update the supervisor execution flow around tick,
handleFactory, and completion to make cancellation and timeout authoritative
during startup: create an AbortController, arm the timeout before invoking
handleFactory, and pass its signal through LocalAgentRunInput. Skip dispatch
when tick has already observed cancellation, and ensure completion cannot be
recorded as successful when timeoutObserved is true; preserve cancellation
status and cleanup behavior.
- Around line 269-278: Update the completedSuccessfully branch around
cleanupWorkflowWorktree to catch cleanup failures, call
preserveWorkflowWorktree(store, identity), and record the caught error through
the existing workflow logging mechanism. Do not silently discard the rejection;
retain the current cleanup behavior when it succeeds and the existing
preservation path for unsuccessful workflows.
In `@src/workflows/worktrees.ts`:
- Line 1: Update safeSegment and the worktree allocation path to append a
deterministic digest derived from the original identity after sanitization.
Preserve readable sanitized text while ensuring distinct identities such as
build/test and build-test cannot resolve to the same path within a workflow.
- Line 3: Update assertContained to reject any result from relative() that is
absolute before accepting the path as contained, including cross-drive Windows
paths such as D:\outside. Import and use isAbsolute alongside the existing path
utilities, while preserving the current containment checks for safe relative
results.
🪄 Autofix (Beta)
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: db48264d-286a-4442-b674-2147e388076b
📒 Files selected for processing (37)
docs/workflow-cli.mddocs/workflow-mcp.mddocs/workflow-runtime.mdpackage.jsonsrc/cli.tssrc/db/client.tssrc/db/migrations.tssrc/db/schema.tssrc/local-agent-adapters.test.tssrc/local-agent-adapters.tssrc/local-agent-profiles.tssrc/local-agent-runtime.test.tssrc/local-agent-runtime.tssrc/oauth-store.test.tssrc/process-platform.test.tssrc/process-platform.tssrc/server.tssrc/workflows/cli.test.tssrc/workflows/cli.tssrc/workflows/dag-scheduler.test.tssrc/workflows/mcp.test.tssrc/workflows/mcp.tssrc/workflows/migrations.test.tssrc/workflows/orchestrator.tssrc/workflows/policy.test.tssrc/workflows/policy.tssrc/workflows/runtime-journal.tssrc/workflows/runtime.test.tssrc/workflows/runtime.tssrc/workflows/store.tssrc/workflows/submission.tssrc/workflows/supervisor-launch.tssrc/workflows/supervisor.test.tssrc/workflows/supervisor.tssrc/workflows/types.tssrc/workflows/workflows.test.tssrc/workflows/worktrees.ts
| const promptResult = await session.prompt({ | ||
| sessionID: sessionId, | ||
| prompt: { text: input.prompt }, | ||
| delivery: "queue", | ||
| resume: true, | ||
| }, { throwOnError: true }); | ||
| pushBounded(items, promptResult); | ||
| await session.wait({ sessionID: sessionId }, { throwOnError: true }); | ||
| const messages = await session.messages( | ||
| { sessionID: sessionId, order: "desc", limit: 100 }, | ||
| { throwOnError: true }, | ||
| ); | ||
| pushBounded(items, messages); | ||
| abortController.abort(); | ||
| await eventPump.catch(() => undefined); | ||
| const finalResponse = requireFinalResponse("OpenCode", extractOpenCodeFinalResponse(promptResult)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
url="$(curl -fsSL 'https://registry.npmjs.org/@opencode-ai%2Fsdk/1.17.13' | jq -r '.dist.tarball')"
curl -fsSL "$url" | tar -xz -C "$tmp"
rg -n -C4 'delivery|session\.prompt|session\.wait' "$tmp/package" \
-g '*.d.ts' -g '*.ts' -g '*.json' | head -n 200Repository: Waishnav/devspace
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant region.
wc -l src/local-agent-adapters.ts
sed -n '260,360p' src/local-agent-adapters.ts
# Find OpenCode-related helpers and message extraction logic.
rg -n "extractOpenCodeFinalResponse|OpenCode|session\.prompt|session\.messages|session\.wait" src/local-agent-adapters.tsRepository: Waishnav/devspace
Length of output: 5254
🌐 Web query:
@opencode-ai/sdk session.prompt session.wait session.messages final response queued prompt return value
💡 Result:
In the @opencode-ai/sdk, the session interaction methods are designed for asynchronous communication with AI coding sessions [1][2][3]. Key methods and behaviors: session.prompt({ path, body }) This method sends a prompt message to an existing session [1][4]. By default, it returns an AssistantMessage containing the AI's response [1][2]. If you set the body property noReply: true, it returns a UserMessage, which provides context without triggering an immediate assistant response [1][2]. The method also supports defining an outputFormat for structured JSON responses [5][3]. session.wait({ sessionID }) For deterministic monitoring, especially in V2 of the SDK, you should use client.v2.session.wait({ sessionID }) [6]. This method blocks execution and returns only after the session agent loop has reached an idle state, providing a reliable way to ensure a task has completed before proceeding [6]. Do not rely on session.idle status alone; use session.wait for completion monitoring [6]. session.messages({ path }) This method retrieves the current list of messages within a specific session [1][4]. It returns an array of objects, where each object contains message metadata (info) and the associated content parts (parts) [1][2]. Note that this provides a snapshot of the messages; if the agent is still streaming a response, the TextPart might contain partial data [7]. To monitor progress or receive updates in real-time, you can subscribe to the event stream using client.event.subscribe [6][7]. This allows you to handle specific events such as message.part.updated or session.idle, which is useful for updating your application state as the AI generates content [7].
Citations:
- 1: https://dev.opencode.ai/docs/sdk/
- 2: https://opencode.ai/docs/sdk/
- 3: https://open-code.ai/en/docs/sdk
- 4: https://opencode.ai/docs/sdk.md
- 5: https://anomalyco-opencode.mintlify.app/sdk/overview
- 6: What is the deterministic way to monitor a session is completed anomalyco/opencode#3815
- 7: https://forums.basehub.com/anomalyco/opencode/19
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect OpenCode helper implementations and surrounding adapter code.
rg -n -C5 "function extractOpenCodeFinalResponse|const extractOpenCodeFinalResponse|extractOpenCodeFinalResponse\\(" src/local-agent-adapters.ts
sed -n '1,220p' src/local-agent-adapters.tsRepository: Waishnav/devspace
Length of output: 9175
🌐 Web query:
"@opencode-ai/sdk" "session.prompt" "delivery: \"queue\"" "session.messages"
💡 Result:
The @opencode-ai/sdk provides programmatic control over OpenCode sessions, but it does not contain a native "queue" feature for session messages [1][2][3]. The concept of a "queue" in the OpenCode ecosystem is implemented via third-party community plugins rather than the core SDK [4][5][6]. Specifically, packages such as opencode-queue or @0xsero/opencode-queue are used to manage message flow [4][5][6]. These plugins typically introduce a /queue slash command or similar interface to allow users to hold, pause, or batch messages while a session is busy, preventing interruptions to active operations [4][6]. To interact with session messages using the core SDK, you should use the following methods: - session.messages({ path }): Lists all messages within a specific session [1][7][3]. - session.prompt({ path, body }): Sends a prompt message to the session. By setting body.noReply: true, you can inject context (e.g., system instructions) into the session without triggering an immediate AI response [1][8][3].
Citations:
- 1: https://dev.opencode.ai/docs/sdk/
- 2: https://opencode.ai/docs/sdk/
- 3: https://open-code.ai/en/docs/sdk
- 4: https://github.com/Simpidbit/opencode-queue
- 5: https://www.npmjs.com/package/opencode-queue
- 6: https://registry.npmjs.org/@0xsero/opencode-queue
- 7: https://frank.dev.opencode.ai/docs/sdk/
- 8: https://github.com/sst/opencode/blob/9ad6588f/packages/web/src/content/docs/sdk.mdx
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the OpenCode dependency version and inspect the response extractor.
rg -n '"`@opencode-ai/sdk`|`@opencode-ai/sdk/v2`|opencode' package.json pnpm-lock.yaml package-lock.json yarn.lock src/local-agent-adapters.ts
sed -n '935,1015p' src/local-agent-adapters.tsRepository: Waishnav/devspace
Length of output: 844
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the package metadata for the SDK version used by the repo.
node - <<'JS'
const fs = require('fs');
for (const file of ['package.json', 'pnpm-lock.yaml', 'package-lock.json', 'yarn.lock']) {
if (fs.existsSync(file)) {
console.log('FILE', file);
const txt = fs.readFileSync(file, 'utf8');
const m = txt.match(/@opencode-ai\/sdk(?:\/v2)?["':\s=@]*([^\s"']+)/g);
if (m) console.log(m.slice(0, 20).join('\n'));
}
}
JSRepository: Waishnav/devspace
Length of output: 310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
meta="$(curl -fsSL 'https://registry.npmjs.org/@opencode-ai%2Fsdk')"
version="$(printf '%s' "$meta" | python3 - <<'PY'
import json,sys
meta=json.load(sys.stdin)
print(meta["dist-tags"]["latest"])
PY
)"
echo "LATEST=$version"
tarball="$(printf '%s' "$meta" | python3 - <<'PY'
import json,sys
meta=json.load(sys.stdin)
v=meta["dist-tags"]["latest"]
print(meta["versions"][v]["dist"]["tarball"])
PY
)"
echo "TARBALL=$tarball"
curl -fsSL "$tarball" | tar -xz -C "$tmp"
# Inspect the public typings/readme for prompt/wait/messages semantics.
rg -n -C4 'session\.prompt|session\.wait|session\.messages|delivery|noReply|AssistantMessage|UserMessage' "$tmp/package" \
-g '*.d.ts' -g '*.md' -g '*.json' | head -n 200Repository: Waishnav/devspace
Length of output: 876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== package-lock excerpt ==="
sed -n '2776,2815p' package-lock.json
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
tarball="https://registry.npmjs.org/@opencode-ai/sdk/-/sdk-1.17.13.tgz"
echo "=== fetching $tarball ==="
curl -fsSL "$tarball" | tar -xz -C "$tmp"
echo "=== sdk package files ==="
find "$tmp/package" -maxdepth 2 -type f \( -name '*.d.ts' -o -name '*.md' -o -name 'package.json' \) | sort | sed -n '1,120p'
echo "=== prompt/wait/messages references ==="
rg -n -C3 'session\.prompt|session\.wait|session\.messages|delivery|noReply|AssistantMessage|UserMessage|queue' "$tmp/package" \
-g '*.d.ts' -g '*.md' -g '*.json' | sed -n '1,220p'Repository: Waishnav/devspace
Length of output: 18394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'extractOpenCodeFinalResponse|OpenCode' test tests src --glob '*.{ts,tsx,js,jsx,mjs,cjs}' | sed -n '1,220p'Repository: Waishnav/devspace
Length of output: 11129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '935,1010p' src/local-agent-adapters.tsRepository: Waishnav/devspace
Length of output: 2891
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL 'https://registry.npmjs.org/@opencode-ai/sdk/-/sdk-1.17.13.tgz' | tar -xz -C "$tmp"
echo "=== V2SessionPromptResponses and related types ==="
rg -n -C5 'type V2SessionPromptResponses|type V2SessionPrompt|type AssistantMessage|type Message =|type UserMessage =' "$tmp/package/dist/v2/gen/types.gen.d.ts" "$tmp/package/dist/v2/gen/sdk.gen.d.ts"
echo "=== session.prompt response definition ==="
rg -n -C8 'session\.prompt|V2SessionPromptResponses|V2SessionPromptErrors' "$tmp/package/dist/v2/gen/sdk.gen.d.ts" "$tmp/package/dist/v2/gen/types.gen.d.ts"
echo "=== docs mentions for prompt return values ==="
rg -n -C4 'prompt\(|wait\(|messages\(' "$tmp/package" -g '*.md' -g '*.d.ts' | sed -n '1,220p'Repository: Waishnav/devspace
Length of output: 22493
Extract the final OpenCode response from messages. session.prompt() returns SessionInputAdmitted here, so promptResult is only the admission ack; use the assistant message from session.messages() after session.wait().
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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-adapters.ts` around lines 311 - 326, Update the final
response extraction in the OpenCode session flow to use the assistant message
returned by session.messages() after session.wait(), rather than promptResult,
which is only a SessionInputAdmitted acknowledgement. Pass the appropriate
messages-derived assistant response to extractOpenCodeFinalResponse while
preserving the existing requireFinalResponse handling.
| const items: unknown[] = []; | ||
| let finalResponse = ""; | ||
| let providerSessionId = thread.id; | ||
| let emittedSessionId: string | null = null; | ||
| if (providerSessionId) { | ||
| emittedSessionId = providerSessionId; | ||
| controller.emit({ type: "session", providerSessionId, resumed }); | ||
| } | ||
| for await (const event of providerEvents) { | ||
| if (event.type === "thread.started") { | ||
| providerSessionId = event.thread_id; | ||
| if (providerSessionId !== emittedSessionId) { | ||
| emittedSessionId = providerSessionId; | ||
| controller.emit({ type: "session", providerSessionId, resumed }); | ||
| } | ||
| } else if (event.type === "item.completed") { | ||
| items.push(event.item); | ||
| if (event.item.type === "agent_message") { | ||
| finalResponse = event.item.text; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound provider result aggregation as well as streamed events. Both implementations retain provider data for the entire run, allowing long executions to exhaust memory.
src/local-agent-runtime.ts#L458-L476: cap completed Codex items using a bounded collection.src/local-agent-adapters.ts#L385-L386: replace unbounded ACPtextPartsaccumulation with a bounded character buffer.
📍 Affects 2 files
src/local-agent-runtime.ts#L458-L476(this comment)src/local-agent-adapters.ts#L385-L386
🤖 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-runtime.ts` around lines 458 - 476, Bound provider result
aggregation in both sites: in src/local-agent-runtime.ts lines 458-476, replace
the unbounded items collection used by the providerEvents loop with a bounded
collection while preserving finalResponse and session emission behavior; in
src/local-agent-adapters.ts lines 385-386, replace unbounded ACP textParts
accumulation with a bounded character buffer. Use the existing aggregation
symbols and enforce limits throughout each run.
| kill(signal: NodeJS.Signals = "SIGTERM"): boolean { | ||
| this.signals.push(signal); | ||
| this.signalCode = signal; | ||
| queueMicrotask(() => this.emit("close")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not mark the test process exited before emitting "close".
Line 79 sets signalCode synchronously, allowing the wait helper to detect termination without exercising its close-event listener. Remove that assignment so this test actually verifies asynchronous close handling.
Proposed fix
kill(signal: NodeJS.Signals = "SIGTERM"): boolean {
this.signals.push(signal);
- this.signalCode = signal;
queueMicrotask(() => this.emit("close"));
return true;
}📝 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.
| kill(signal: NodeJS.Signals = "SIGTERM"): boolean { | |
| this.signals.push(signal); | |
| this.signalCode = signal; | |
| queueMicrotask(() => this.emit("close")); | |
| kill(signal: NodeJS.Signals = "SIGTERM"): boolean { | |
| this.signals.push(signal); | |
| queueMicrotask(() => this.emit("close")); |
🤖 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/process-platform.test.ts` around lines 77 - 80, Update the test process
mock’s kill method to stop assigning signalCode synchronously; retain recording
the signal and asynchronously emitting "close" so the wait helper must rely on
its close-event listener.
| terminateProcessTree(child, "SIGTERM", detached, runtime); | ||
| if (await waitForProcessTreeExit(child, detached, graceMs, runtime)) return; | ||
| terminateProcessTree(child, "SIGKILL", detached, runtime); | ||
| await waitForProcessTreeExit(child, detached, graceMs, runtime); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report failure when the process survives SIGKILL.
The final wait result is ignored, so disposal can report success while the child or detached process group remains alive.
Proposed fix
terminateProcessTree(child, "SIGKILL", detached, runtime);
-await waitForProcessTreeExit(child, detached, graceMs, runtime);
+if (!await waitForProcessTreeExit(child, detached, graceMs, runtime)) {
+ throw new Error("Process tree remained alive after SIGKILL.");
+}📝 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.
| terminateProcessTree(child, "SIGTERM", detached, runtime); | |
| if (await waitForProcessTreeExit(child, detached, graceMs, runtime)) return; | |
| terminateProcessTree(child, "SIGKILL", detached, runtime); | |
| await waitForProcessTreeExit(child, detached, graceMs, runtime); | |
| terminateProcessTree(child, "SIGTERM", detached, runtime); | |
| if (await waitForProcessTreeExit(child, detached, graceMs, runtime)) return; | |
| terminateProcessTree(child, "SIGKILL", detached, runtime); | |
| if (!await waitForProcessTreeExit(child, detached, graceMs, runtime)) { | |
| throw new Error("Process tree remained alive after SIGKILL."); | |
| } |
🤖 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/process-platform.ts` around lines 105 - 108, Update the termination flow
around terminateProcessTree and waitForProcessTreeExit so it checks the final
wait result after SIGKILL. If the child or detached process group is still
alive, propagate/report disposal failure instead of returning success; preserve
the existing graceful SIGTERM path.
| const store = new WorkflowStore(stateDir); | ||
| const supervisorLeaseMs = options.supervisorLeaseMs ?? DEFAULT_SUPERVISOR_LEASE_MS; | ||
| const nodeLeaseMs = options.nodeLeaseMs ?? DEFAULT_NODE_LEASE_MS; | ||
| const heartbeatMs = options.heartbeatMs ?? DEFAULT_HEARTBEAT_MS; | ||
| const idleMs = options.idleMs ?? DEFAULT_IDLE_MS; | ||
| const globalConcurrency = options.globalConcurrency ?? DEFAULT_GLOBAL_CONCURRENCY; | ||
| validateSupervisorTiming(supervisorLeaseMs, nodeLeaseMs, heartbeatMs, idleMs, globalConcurrency); | ||
| const acquired = store.acquireSupervisor({ | ||
| ownerToken: options.ownerToken ?? randomUUID(), | ||
| ownerPid: options.ownerPid ?? process.pid, | ||
| leaseMs: supervisorLeaseMs, | ||
| }); | ||
| if (!acquired) { | ||
| store.close(); | ||
| return false; | ||
| } | ||
|
|
||
| const identity: WorkflowSupervisorIdentity = acquired; | ||
| const active = new Set<Promise<void>>(); | ||
| let lastWorkAt = Date.now(); | ||
| try { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Protect the store from setup-time exceptions.
validateSupervisorTiming() and acquireSupervisor() execute before the try/finally. Invalid options or acquisition errors therefore leave the SQLite store open. Validate before constructing the store and include acquisition in its protected lifetime.
🤖 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/workflows/supervisor.ts` around lines 55 - 75, The supervisor setup
currently creates the WorkflowStore before validation and performs acquisition
outside the try/finally cleanup scope. Move validateSupervisorTiming before
constructing WorkflowStore, then include acquireSupervisor and the
failed-acquisition close path within the protected try/finally lifetime so setup
exceptions always close the store.
| const tick = async () => { | ||
| if (tickRunning || leaseFailure) return; | ||
| tickRunning = true; | ||
| try { | ||
| if (!store.heartbeatSupervisor(supervisor, options.supervisorLeaseMs)) { | ||
| leaseFailure = new Error("Workflow supervisor lease lost."); | ||
| await handle?.cancel(leaseFailure); | ||
| return; | ||
| } | ||
| if (!store.heartbeatNode({ ...identity, leaseMs: options.nodeLeaseMs })) { | ||
| leaseFailure = new Error("Workflow node claim lost."); | ||
| await handle?.cancel(leaseFailure); | ||
| return; | ||
| } | ||
| if (!cancellationObserved && store.isCancellationRequested(identity.workflowId)) { | ||
| cancellationObserved = true; | ||
| store.markNodeCancelling(identity); | ||
| await handle?.cancel(new Error("Workflow cancellation requested.")); | ||
| } | ||
| } finally { | ||
| tickRunning = false; | ||
| } | ||
| }; | ||
| await tick(); | ||
| if (leaseFailure) throw leaseFailure; | ||
| heartbeat = setInterval(() => void tick().catch(() => undefined), options.heartbeatMs); | ||
|
|
||
| handle = await options.handleFactory(execution.provider, { | ||
| prompt: fullPrompt, | ||
| workspace: execution.workspaceRoot, | ||
| model: execution.model, | ||
| thinking: execution.thinking, | ||
| policy: execution.effectivePolicy, | ||
| }); | ||
| if (leaseFailure) { | ||
| await handle.cancel(leaseFailure); | ||
| throw leaseFailure; | ||
| } | ||
| if (cancellationObserved) { | ||
| await handle.cancel(new Error("Workflow cancellation requested.")); | ||
| } | ||
| if (!store.markNodeRunning(identity)) { | ||
| throw new Error("Workflow node attempt was lost during dispatch."); | ||
| } | ||
|
|
||
| if (execution.timeoutMs !== undefined) { | ||
| timeout = setTimeout(() => { | ||
| timeoutObserved = true; | ||
| void handle?.cancel(new Error("Workflow node timed out.")); | ||
| }, execution.timeoutMs); | ||
| } | ||
|
|
||
| const drain = drainEvents(store, identity, handle); | ||
| let result: LocalAgentRunResult; | ||
| try { | ||
| result = await handle.result(); | ||
| } finally { | ||
| await drain; | ||
| } | ||
| if (result.providerSessionId) { | ||
| store.recordNodeProviderSession(identity, result.providerSessionId); | ||
| } | ||
| const cancelled = cancellationObserved || store.isCancellationRequested(identity.workflowId); | ||
| const completed = store.completeAgentNode({ | ||
| ...identity, | ||
| status: cancelled ? "cancelled" : "succeeded", | ||
| result: { | ||
| provider: result.provider, | ||
| providerSessionId: result.providerSessionId, | ||
| finalResponse: result.finalResponse, | ||
| }, | ||
| }); | ||
| completedSuccessfully = Boolean(completed && !cancelled); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make cancellation and timeout authoritative across adapter startup.
A cancellation detected by tick() still launches handleFactory; meanwhile, the timeout is armed only after startup resolves. A hung startup is therefore unbounded, and a result resolving after the timer fires can still be recorded as successful.
Create an AbortController, arm the timeout before startup, pass its signal to LocalAgentRunInput, skip dispatch when already cancelled, and reject completion when timeoutObserved is set.
🤖 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/workflows/supervisor.ts` around lines 183 - 255, Update the supervisor
execution flow around tick, handleFactory, and completion to make cancellation
and timeout authoritative during startup: create an AbortController, arm the
timeout before invoking handleFactory, and pass its signal through
LocalAgentRunInput. Skip dispatch when tick has already observed cancellation,
and ensure completion cannot be recorded as successful when timeoutObserved is
true; preserve cancellation status and cleanup behavior.
| if (worktreeAllocated) { | ||
| if (completedSuccessfully && allocatedWorktreeRoot) { | ||
| await cleanupWorkflowWorktree({ | ||
| store, | ||
| identity, | ||
| worktreeRoot: allocatedWorktreeRoot, | ||
| }).catch(() => undefined); | ||
| } else { | ||
| preserveWorkflowWorktree(store, identity); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve and report worktrees when cleanup fails.
The cleanup rejection is silently discarded, bypassing the preservation path and potentially leaving an untracked managed worktree. Catch the failure, mark the worktree preserved, and record the cleanup error.
Proposed control flow
- await cleanupWorkflowWorktree({
- store,
- identity,
- worktreeRoot: allocatedWorktreeRoot,
- }).catch(() => undefined);
+ try {
+ await cleanupWorkflowWorktree({
+ store,
+ identity,
+ worktreeRoot: allocatedWorktreeRoot,
+ });
+ } catch {
+ preserveWorkflowWorktree(store, identity);
+ }📝 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.
| if (worktreeAllocated) { | |
| if (completedSuccessfully && allocatedWorktreeRoot) { | |
| await cleanupWorkflowWorktree({ | |
| store, | |
| identity, | |
| worktreeRoot: allocatedWorktreeRoot, | |
| }).catch(() => undefined); | |
| } else { | |
| preserveWorkflowWorktree(store, identity); | |
| } | |
| if (worktreeAllocated) { | |
| if (completedSuccessfully && allocatedWorktreeRoot) { | |
| try { | |
| await cleanupWorkflowWorktree({ | |
| store, | |
| identity, | |
| worktreeRoot: allocatedWorktreeRoot, | |
| }); | |
| } catch { | |
| preserveWorkflowWorktree(store, identity); | |
| } | |
| } else { | |
| preserveWorkflowWorktree(store, identity); | |
| } |
🤖 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/workflows/supervisor.ts` around lines 269 - 278, Update the
completedSuccessfully branch around cleanupWorkflowWorktree to catch cleanup
failures, call preserveWorkflowWorktree(store, identity), and record the caught
error through the existing workflow logging mechanism. Do not silently discard
the rejection; retain the current cleanup behavior when it succeeds and the
existing preservation path for unsuccessful workflows.
| @@ -0,0 +1,176 @@ | |||
| import { execFile } from "node:child_process"; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make allocation paths collision-resistant.
safeSegment is lossy, so distinct valid keys such as build/test and build-test produce the same worktree path within one workflow. Append a digest of the original identity rather than relying solely on sanitized text.
Proposed fix
+import { createHash } from "node:crypto";
import { execFile } from "node:child_process";+ const allocationKey = createHash("sha256")
+ .update(JSON.stringify([
+ input.identity.workflowId,
+ input.identity.nodeKey,
+ input.identity.attempt,
+ ]))
+ .digest("hex")
+ .slice(0, 16);
const path = join(
worktreeRoot,
- `${safeSegment(basename(sourceRoot))}-${safeSegment(input.identity.workflowId)}-${safeSegment(input.identity.nodeKey)}-a${input.identity.attempt}`,
+ `${safeSegment(basename(sourceRoot))}-${safeSegment(input.identity.nodeKey)}-${allocationKey}`,
);Also applies to: 42-45
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/worktrees.ts` at line 1, Update safeSegment and the worktree
allocation path to append a deterministic digest derived from the original
identity after sanitization. Preserve readable sanitized text while ensuring
distinct identities such as build/test and build-test cannot resolve to the same
path within a workflow.
| @@ -0,0 +1,176 @@ | |||
| import { execFile } from "node:child_process"; | |||
| import { mkdir, realpath } from "node:fs/promises"; | |||
| import { basename, join, relative, resolve } from "node:path"; | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const { win32 } = require("node:path");
const result = win32.relative("C:\\managed", "D:\\outside");
console.log({ result, absolute: win32.isAbsolute(result) });
NODERepository: Waishnav/devspace
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/workflows/worktrees.ts | cat -nRepository: Waishnav/devspace
Length of output: 8081
Reject absolute results from relative() in assertContained (src/workflows/worktrees.ts:153-156). On Windows, cross-drive paths return an absolute value like D:\outside, so this guard can accept an escaped path. Add an isAbsolute() check before treating the relationship as contained.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/worktrees.ts` at line 3, Update assertContained to reject any
result from relative() that is absolute before accepting the path as contained,
including cross-drive Windows paths such as D:\outside. Import and use
isAbsolute alongside the existing path utilities, while preserving the current
containment checks for safe relative results.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
e751d60 to
2fb2a5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/workflows/supervisor.test.ts (1)
236-249: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the late-success path actually outlive cancellation.
LocalAgentRunController.cancel()immediately rejects the result, and Line 248 clears the 60 ms timer during disposal. The scheduled success never runs, so this only tests ordinary timeout cancellation. Use a customLocalAgentRunHandlewhosecancel()is a no-op and whoseresult()resolves after the timeout.🤖 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/workflows/supervisor.test.ts` around lines 236 - 249, Update the handleFactory setup around LocalAgentRunController so the late-success scenario outlives cancellation: replace the controller-backed handle with a custom LocalAgentRunHandle whose cancel() is a no-op and whose result() resolves only after the delayed success interval. Preserve the existing delayed success payload and ensure disposal does not clear the timer before it resolves.
🤖 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/process-sessions.ts`:
- Line 53: Update terminate and shutdown to handle the value returned by
session.process?.kill("SIGTERM"): preserve synchronous behavior while attaching
rejection handling when kill returns a promise, preventing unhandled promise
rejections in both methods.
In `@src/workflows/mcp.test.ts`:
- Line 132: Remove the duplicated declarations in src/workflows/mcp.test.ts:
lines 132-132 should retain only one dagWorkflowId declaration, lines 137-140
should retain only one workflow property declaration, and lines 191-194 should
retain only one supervisor and workflow property declaration. Ensure the
affected test structures compile without duplicate identifiers or properties.
In `@src/workflows/runtime.ts`:
- Around line 261-272: Update executeAgentCall to handle failures from
wakeSupervisor or later orchestration after the agent request is submitted:
cancel the child workflow and complete its journal call before propagating the
error. Ensure the existing agent_result failure path receives the resulting
error, while preserving normal success behavior. Apply the same durable-state
cleanup to the corresponding flow around the later scheduleAgent invocation.
In `@src/workflows/supervisor-launch.ts`:
- Line 2: Normalize the relative stateDir and cliEntrypoint paths before the
supervisor changes cwd, ensuring they retain the parent process’s original
working-directory semantics. Update the supervisor launch flow around the store
initialization and child spawn to use these resolved paths consistently,
including the child-visible CLI entrypoint and the SQLite store path.
In `@src/workflows/worktrees.ts`:
- Around line 91-103: Update the cleanup flow around cleanupWorkflowWorktree so
worktreeRoot comes from the authoritative configured or persisted
managed-worktree root rather than dirname(record.path). Pass that root
independently of each expired record, while preserving the existing identity and
cleanup iteration behavior.
- Around line 133-139: Remove the ancestry check using `git(["merge-base",
"--is-ancestor", ...])` from the cleanup flow in `worktrees.ts`. Keep validating
ownership by matching `registeredWorktrees(record.sourceRoot)` entries against
`record.path`, then proceed with forced worktree removal regardless of rewritten
history.
---
Nitpick comments:
In `@src/workflows/supervisor.test.ts`:
- Around line 236-249: Update the handleFactory setup around
LocalAgentRunController so the late-success scenario outlives cancellation:
replace the controller-backed handle with a custom LocalAgentRunHandle whose
cancel() is a no-op and whose result() resolves only after the delayed success
interval. Preserve the existing delayed success payload and ensure disposal does
not clear the timer before it resolves.
🪄 Autofix (Beta)
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: dcf232e6-bc85-4789-bc3a-bfc254661d75
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
docs/workflow-runtime.mdpackage.jsonsrc/db/client.tssrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/process-sessions.tssrc/server-shutdown.test.tssrc/server.tssrc/workflows/cli.test.tssrc/workflows/cli.tssrc/workflows/dag-scheduler.test.tssrc/workflows/mcp.test.tssrc/workflows/mcp.tssrc/workflows/migrations.test.tssrc/workflows/orchestrator.tssrc/workflows/runtime-journal.tssrc/workflows/runtime.test.tssrc/workflows/runtime.tssrc/workflows/store.tssrc/workflows/supervisor-launch.tssrc/workflows/supervisor.test.tssrc/workflows/supervisor.tssrc/workflows/workflows.test.tssrc/workflows/worktrees.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- docs/workflow-runtime.md
- src/oauth-store.test.ts
- package.json
- src/workflows/orchestrator.ts
- src/server.ts
- src/workflows/cli.test.ts
- src/workflows/supervisor.ts
- src/workflows/mcp.ts
- src/workflows/runtime-journal.ts
- src/workflows/store.ts
- src/workflows/workflows.test.ts
- src/workflows/cli.ts
| interface ManagedProcess { | ||
| write(data: string): void; | ||
| kill(signal?: NodeJS.Signals): void; | ||
| kill(signal?: NodeJS.Signals): void | Promise<void>; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle potential unhandled promises in terminate and shutdown.
The kill method signature now permits returning a Promise<void>, and the Windows-specific implementation for non-SIGKILL signals now returns a promise via terminateProcessTreeGracefully. However, the existing terminate() and shutdown() methods call kill("SIGTERM") synchronously without handling the returned promise.
This can result in unhandled promise rejections if the graceful termination encounters an error, and will likely trigger floating promise lint rules. Consider explicitly handling the promise in those methods.
terminate(workspaceId: string, sessionId: number): void {
const session = this.getOwnedSession(workspaceId, sessionId);
if (session.running) {
const result = session.process?.kill("SIGTERM");
if (result instanceof Promise) result.catch(console.error);
}
}
shutdown(): void {
for (const session of this.sessions.values()) {
if (session.cleanupTimer) clearTimeout(session.cleanupTimer);
if (session.running) {
const result = session.process?.kill("SIGTERM");
if (result instanceof Promise) result.catch(console.error);
}
}
this.sessions.clear();
}🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/process-sessions.ts` at line 53, Update terminate and shutdown to handle
the value returned by session.process?.kill("SIGTERM"): preserve synchronous
behavior while attaching rejection handling when kill returns a promise,
preventing unhandled promise rejections in both methods.
| }, | ||
| }); | ||
| assert.equal(dagRun.isError, undefined); | ||
| const dagWorkflowId = (dagRun.structuredContent as { workflow: { id: string } }).workflow.id; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove accidentally duplicated lines.
Several lines in this file have been duplicated, likely due to a copy-paste mistake or an unresolved merge conflict. This causes SyntaxError and TypeScript compilation failures.
src/workflows/mcp.test.ts#L132-L132: remove the duplicatedagWorkflowIdvariable declaration.src/workflows/mcp.test.ts#L137-L140: remove the redundantworkflowproperty declarations.src/workflows/mcp.test.ts#L191-L194: remove the redundantsupervisorandworkflowproperty declarations.
📍 Affects 1 file
src/workflows/mcp.test.ts#L132-L132(this comment)src/workflows/mcp.test.ts#L137-L140src/workflows/mcp.test.ts#L191-L194
🤖 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/workflows/mcp.test.ts` at line 132, Remove the duplicated declarations in
src/workflows/mcp.test.ts: lines 132-132 should retain only one dagWorkflowId
declaration, lines 137-140 should retain only one workflow property declaration,
and lines 191-194 should retain only one supervisor and workflow property
declaration. Ensure the affected test structures compile without duplicate
identifiers or properties.
| scheduleAgent(async () => { | ||
| try { | ||
| const result = await executeAgentCall(message.callIndex, message.request); | ||
| sendChild(child, { type: "agent_result", requestId: message.requestId, ok: true, result }); | ||
| } catch (error) { | ||
| sendChild(child, { | ||
| type: "agent_result", | ||
| requestId: message.requestId, | ||
| ok: false, | ||
| error: normalizeError(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Settle durable child state before reporting an agent failure.
If wakeSupervisor() or subsequent orchestration fails, the script receives a rejected agent() call and can still succeed, while the child workflow remains runnable and its journal call stays running. Catch post-submission failures inside executeAgentCall(), cancel the child, and complete the journal call before returning the error.
Also applies to: 326-340
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/runtime.ts` around lines 261 - 272, Update executeAgentCall to
handle failures from wakeSupervisor or later orchestration after the agent
request is submitted: cancel the child workflow and complete its journal call
before propagating the error. Ensure the existing agent_result failure path
receives the resulting error, while preserving normal success behavior. Apply
the same durable-state cleanup to the corresponding flow around the later
scheduleAgent invocation.
| @@ -0,0 +1,118 @@ | |||
| import { spawn } from "node:child_process"; | |||
| import { dirname } from "node:path"; | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize child-visible paths before changing cwd.
Line 60 makes relative stateDir and cliEntrypoint values resolve from the CLI directory, while the parent store used its original working directory. This can launch the supervisor against a different SQLite database. Resolve both paths before opening the store and spawning the child, or avoid changing cwd.
Also applies to: 19-19, 35-40, 49-60
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/supervisor-launch.ts` at line 2, Normalize the relative
stateDir and cliEntrypoint paths before the supervisor changes cwd, ensuring
they retain the parent process’s original working-directory semantics. Update
the supervisor launch flow around the store initialization and child spawn to
use these resolved paths consistently, including the child-visible CLI
entrypoint and the SQLite store path.
| for (const record of input.store.listExpiredWorktrees(input.now)) { | ||
| if (input.beforeCleanup && !input.beforeCleanup()) break; | ||
| try { | ||
| await cleanupWorkflowWorktree({ | ||
| store: input.store, | ||
| identity: { | ||
| workflowId: record.workflowId, | ||
| nodeKey: record.nodeKey, | ||
| attempt: record.attempt, | ||
| claimToken: "retention-cleanup", | ||
| }, | ||
| worktreeRoot: dirname(record.path), | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not derive the trusted managed root from record.path.
Passing dirname(record.path) makes containment prove only that a persisted path is beneath its own parent. A corrupted record can therefore authorize cleanup outside the configured managed-worktree root. Persist or pass the authoritative root independently.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/worktrees.ts` around lines 91 - 103, Update the cleanup flow
around cleanupWorkflowWorktree so worktreeRoot comes from the authoritative
configured or persisted managed-worktree root rather than dirname(record.path).
Pass that root independently of each expired record, while preserving the
existing identity and cleanup iteration behavior.
| const registered = await registeredWorktrees(record.sourceRoot); | ||
| const match = registered.find((entry) => resolve(entry.path) === resolve(record.path)); | ||
| if (!match) { | ||
| throw new WorkflowValidationError("Workflow worktree Git registration does not match persisted ownership"); | ||
| } | ||
| await git(["merge-base", "--is-ancestor", record.baseSha, match.head], record.sourceRoot); | ||
| await git(["worktree", "remove", "--force", record.path], record.sourceRoot); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Allow cleanup after worktree history rewrites.
Rebases, resets, or orphan commits make record.baseSha cease being an ancestor of HEAD, permanently leaving the worktree in cleanup_failed. Validate repository/worktree ownership independently instead of requiring ancestry before removal.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/worktrees.ts` around lines 133 - 139, Remove the ancestry check
using `git(["merge-base", "--is-ancestor", ...])` from the cleanup flow in
`worktrees.ts`. Keep validating ownership by matching
`registeredWorktrees(record.sourceRoot)` entries against `record.path`, then
proceed with forced worktree removal regardless of rewritten history.
Co-Authored-By: Claude <noreply@anthropic.com>
4f497b6 to
97398c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/runtime.ts`:
- Around line 276-283: Bound cumulative runtime-event persistence in the runtime
event-processing flow around journal.appendEvent: track each run’s emitted event
count and total serialized payload bytes, enforce explicit per-run limits before
appending, and terminate the runtime when either budget is exceeded. Ensure
accounting includes every accepted event and remains scoped to run.id.
🪄 Autofix (Beta)
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: 3ebfd9a1-0595-4672-a8b4-c65f78f6b89b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
docs/workflow-runtime.mdpackage.jsonsrc/db/client.tssrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/workflows/cli.test.tssrc/workflows/cli.tssrc/workflows/migrations.test.tssrc/workflows/runtime-journal.tssrc/workflows/runtime.test.tssrc/workflows/runtime.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/oauth-store.test.ts
- package.json
- docs/workflow-runtime.md
- src/db/migrations.ts
- src/workflows/cli.test.ts
- src/workflows/migrations.test.ts
- src/db/schema.ts
- src/workflows/runtime.test.ts
- src/workflows/cli.ts
- src/workflows/runtime-journal.ts
| if (message.type === "event") { | ||
| const eventType = requiredBoundedString(message.eventType, "Runtime event type", 64); | ||
| if (eventType !== "phase.started" && eventType !== "phase.completed" && eventType !== "log") { | ||
| throw new WorkflowValidationError(`Unsupported workflow runtime event: ${eventType}`); | ||
| } | ||
| const payload = normalizeEventPayload(message.payload); | ||
| journal.appendEvent(run.id, eventType, payload); | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound cumulative runtime-event persistence.
The per-event size checks do not limit event count or total bytes. A workflow can emit events continuously for up to 24 hours, growing the durable journal and disk usage without bound. Add a per-run event count/byte budget and terminate the runtime when exceeded.
Also applies to: 624-670
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/runtime.ts` around lines 276 - 283, Bound cumulative
runtime-event persistence in the runtime event-processing flow around
journal.appendEvent: track each run’s emitted event count and total serialized
payload bytes, enforce explicit per-run limits before appending, and terminate
the runtime when either budget is exceeded. Ensure accounting includes every
accepted event and remains scoped to run.id.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
97398c1 to
0052715
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/runtime.ts`:
- Around line 215-216: Replace the one-time error listeners on the spawned
runtime child with persistent listeners in the child setup and corresponding
shutdown path, including the handlers around spawnRuntimeChild and the later
child error handling near sendChild. Use on("error") instead of once("error") so
every IPC failure remains handled throughout the child’s lifetime.
🪄 Autofix (Beta)
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: a0fca9f5-7023-4d7a-ae33-fb9c10c61656
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
docs/workflow-runtime.mdpackage.jsonsrc/db/migrations.tssrc/db/schema.tssrc/oauth-store.test.tssrc/workflows/cli.test.tssrc/workflows/cli.tssrc/workflows/migrations.test.tssrc/workflows/runtime-journal.tssrc/workflows/runtime.test.tssrc/workflows/runtime.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/workflow-runtime.md
- src/oauth-store.test.ts
- src/workflows/runtime.test.ts
- src/workflows/migrations.test.ts
- package.json
- src/workflows/cli.test.ts
- src/db/schema.ts
- src/workflows/runtime-journal.ts
- src/workflows/cli.ts
| child = spawnRuntimeChild(); | ||
| child.once("error", (error) => settle(() => reject(error))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\.(once|on)\("error"|\.send\(' src/workflows/runtime.ts
rg -n -C2 '"engines"|"node"' package.jsonRepository: Waishnav/devspace
Length of output: 1811
🏁 Script executed:
#!/bin/bash
sed -n '180,260p' src/workflows/runtime.ts
printf '\n---\n'
sed -n '430,470p' src/workflows/runtime.ts
printf '\n---\n'
sed -n '600,710p' src/workflows/runtime.ts
printf '\n---\n'
rg -n 'spawnRuntimeChild|sendChild\(|process\.send\(' src/workflows/runtime.tsRepository: Waishnav/devspace
Length of output: 9396
🏁 Script executed:
#!/bin/bash
rg -n -C2 'sendChild\(' src/workflows/runtime.tsRepository: Waishnav/devspace
Length of output: 918
Keep the child error listener attached for the runtime lifetime (src/workflows/runtime.ts:215-216, 454-456). sendChild() uses callback-less child.send(), so a later IPC failure after the first once("error") fires can become unhandled. on("error", ...) avoids that race when multiple agent replies overlap child shutdown.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/workflows/runtime.ts` around lines 215 - 216, Replace the one-time error
listeners on the spawned runtime child with persistent listeners in the child
setup and corresponding shutdown path, including the handlers around
spawnRuntimeChild and the later child error handling near sendChild. Use
on("error") instead of once("error") so every IPC failure remains handled
throughout the child’s lifetime.
1 participant
Summary
DevSpace becomes a durable dynamic-workflow harness for configured local coding agents while preserving CLI compatibility for parent harnesses that only expose shell execution.
The implementation uses one authoritative orchestration service across CLI, MCP, DAG scheduling, managed worktrees, and the restricted JavaScript runtime. Provider adapters remain authoritative; this does not introduce a separate Anthropic API integration.
Stacked PRs
Merge in order:
Resulting surfaces
Validation
Known non-failing warnings are unchanged: Node DEP0190 in an existing shell path, the intentional invalid-profile fixture warning, and Vite large-chunk warnings.
🤖 Generated with Claude Code
Summary by CodeRabbit
devspace workflowsCLI for durable workflow execution (run,status,wait,events,cancel, plusscriptwith JSON-enveloped output).devspace workflows script) with SES sandboxing and durable replay.