feat(workflow): engine API with fakes#96
Conversation
agent/parallel/pipeline/phase/log/workflow primitives, semaphore, ALS phase, isolation worktree hook, nest depth 1, and executeWorkflow against injectable runProvider for unit tests. Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
| "isolation: 'worktree' requires createWorktree host support", | ||
| ); | ||
| } | ||
| worktree = await deps.createWorktree({ |
There was a problem hiding this comment.
[P0] Worktree creation happens before beginAgentCall, but the catch path always calls failAgentCall. If setup fails (non-Git workspace, missing Git, bad base SHA, collision), the original error is masked by Unknown workflow agent call. Begin a preparing row before fallible setup, or guard failure journaling with a callStarted flag.
There was a problem hiding this comment.
Addressed by stacked PR #101 (d5b00f5). The agent path now tracks whether beginAgentCall actually succeeded and only calls failAgentCall for an existing row. A worktree setup failure preserves the original error and still emits a typed agent_call_failed event. A regression test makes createWorktree throw and verifies that the error is not replaced by Unknown workflow agent call. This thread becomes obsolete once #101 is merged.
| runId: deps.runId, | ||
| callIndex: index, | ||
| responseText: truncate(result.finalResponse, WORKFLOW_LIMITS.responseTextBytes), | ||
| structuredJson: structuredJson |
There was a problem hiding this comment.
[P1] Please do not truncate structured JSON. A truncated payload is invalid JSON; replay then silently falls back to response text and changes the return type from object to string. Reject oversized structured results or persist them as an artifact/reference. Only free-form response text should be truncated.
| // Child surface: reuse parent agent/parallel/pipeline/phase/log/budget/workflow | ||
| // so callIndex + semaphore stay shared. Override args + meta for the child body. | ||
| const childApi: WorkflowApi = { | ||
| agent: input.parentApi.agent, |
There was a problem hiding this comment.
[P1] Reusing the parent's agent closure also reuses the parent's meta and phase context, so a child's defaultProvider and phase semantics are not actually honored. Share only runtime state (semaphore, call counter, replay, signal, journal), then construct a child API with the child's own meta and args.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/workflow-engine.test.ts (4)
401-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
void createStubBudget;is a lint workaround, not a test.Drop the import at Line 15 rather than keeping a no-op reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-engine.test.ts` at line 401, Remove the unused createStubBudget import and delete the no-op void createStubBudget reference; no test logic should remain solely to satisfy linting.
50-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
orderis populated but never asserted.The block header claims barrier semantics, yet nothing checks the recorded start/end interleaving. Either assert on
order(e.g. all threestart:entries precede the firstend:at concurrency 4) or drop the variable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-engine.test.ts` around lines 50 - 78, Update the parallel workflow test using the order variable in the createWorkflowApi test setup to assert barrier semantics: verify all three start entries occur before the first end entry when concurrency is 4. Keep the existing result and call-count assertions, or remove order only if the interleaving behavior is no longer being tested.
337-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDepth-2 case doesn't test what the comment describes.
The
.then()chained onworkflow(...)at Line 343 never executes because the promise rejects first, so the assertion passes for the right reason but via a misleading construct. Simplify to a parent that nests amidscript which itself nests, and drop the.then().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-engine.test.ts` around lines 337 - 360, Simplify the depth-2 test in the executeWorkflow case so the parent directly awaits workflow({ scriptPath: ... }) without chaining .then(). Keep resolveNestedSource returning the mid script that performs the second nesting attempt, ensuring the nest_depth rejection is exercised by the intended nested workflow path.
389-396: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCancellation test only covers the pre-flight path.
ac.abort()at Line 395 fires beforeapi.agent, sorunProvideris never invoked and theac.abort()inside it at Line 390 is unreachable. Mid-flight cancellation — abort while the provider is running, and the post-providerthrowIfCancelledinsrc/workflow-api.ts— is untested, as is theisCancelRequested(runId)journal branch. Consider adding a case that aborts during the provider call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-engine.test.ts` around lines 389 - 396, Add a separate cancellation test that allows api.agent to enter runProvider, aborts the controller from within or during that provider call, then resolves the provider and asserts api.agent rejects with WorkflowEngineError. Cover the post-provider cancellation check and ensure the runId journal is marked as cancelled via the isCancelRequested(runId) path, while keeping the existing pre-flight test unchanged.src/workflow-engine.ts (2)
116-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead try/catch — both branches rethrow the same error.
♻️ Simplify
- try { - const result = await runWorkflowSandbox({ - parsed, - api, - timeoutMs: options.timeoutMs ?? WORKFLOW_HOST_TIMEOUT_MS, - }); - return { - result, - meta: parsed.meta, - callCount: api.getCallCount(), - }; - } catch (error) { - if (error instanceof WorkflowEngineError) { - throw error; - } - throw error; - } + const result = await runWorkflowSandbox({ + parsed, + api, + timeoutMs: options.timeoutMs ?? WORKFLOW_HOST_TIMEOUT_MS, + }); + return { + result, + meta: parsed.meta, + callCount: api.getCallCount(), + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-engine.ts` around lines 116 - 132, Remove the redundant try/catch surrounding runWorkflowSandbox in the workflow execution method; let the await call and its existing return object propagate errors naturally, preserving the special-case behavior only if it performs distinct handling elsewhere.
149-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo sources of truth for the nesting limit.
WORKFLOW_MAX_NEST_DEPTH_LOCALduplicatesWORKFLOW_MAX_NEST_DEPTHfrom./workflow-types.js, whichsrc/workflow-api.tsalready enforces inworkflow(). If the shared constant ever changes, the API guard and the engine guard disagree. Import the shared constant instead.Also applies to: 188-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow-engine.ts` around lines 149 - 153, Replace the local WORKFLOW_MAX_NEST_DEPTH_LOCAL usage in the workflow engine’s nesting checks with the shared WORKFLOW_MAX_NEST_DEPTH imported from ./workflow-types.js, including the additional check around the second referenced location. Remove the duplicate local constant and keep the existing WorkflowEngineError behavior unchanged.
🤖 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/workflow-api.ts`:
- Around line 601-611: Update the option parsing around record.model,
record.effort, and record.provider to reject values when those properties are
present but not strings, matching the validation behavior of schema, isolation,
and writeMode. Preserve assignment for valid strings and ensure invalid values
throw a WorkflowEngineError instead of being silently omitted and falling back
to defaults.
- Around line 520-532: The nested workflow() path in the workflow function must
validate nameOrRef before calling deps.resolveNestedSource: accept only a string
or an object with a valid scriptPath string, and reject invalid or
workspace-escaping paths with WorkflowEngineError code "path". Document in the
resolveNestedSource contract that host resolvers must confine scriptPath to the
configured workspace root, while preserving valid nested workflow resolution.
---
Nitpick comments:
In `@src/workflow-engine.test.ts`:
- Line 401: Remove the unused createStubBudget import and delete the no-op void
createStubBudget reference; no test logic should remain solely to satisfy
linting.
- Around line 50-78: Update the parallel workflow test using the order variable
in the createWorkflowApi test setup to assert barrier semantics: verify all
three start entries occur before the first end entry when concurrency is 4. Keep
the existing result and call-count assertions, or remove order only if the
interleaving behavior is no longer being tested.
- Around line 337-360: Simplify the depth-2 test in the executeWorkflow case so
the parent directly awaits workflow({ scriptPath: ... }) without chaining
.then(). Keep resolveNestedSource returning the mid script that performs the
second nesting attempt, ensuring the nest_depth rejection is exercised by the
intended nested workflow path.
- Around line 389-396: Add a separate cancellation test that allows api.agent to
enter runProvider, aborts the controller from within or during that provider
call, then resolves the provider and asserts api.agent rejects with
WorkflowEngineError. Cover the post-provider cancellation check and ensure the
runId journal is marked as cancelled via the isCancelRequested(runId) path,
while keeping the existing pre-flight test unchanged.
In `@src/workflow-engine.ts`:
- Around line 116-132: Remove the redundant try/catch surrounding
runWorkflowSandbox in the workflow execution method; let the await call and its
existing return object propagate errors naturally, preserving the special-case
behavior only if it performs distinct handling elsewhere.
- Around line 149-153: Replace the local WORKFLOW_MAX_NEST_DEPTH_LOCAL usage in
the workflow engine’s nesting checks with the shared WORKFLOW_MAX_NEST_DEPTH
imported from ./workflow-types.js, including the additional check around the
second referenced location. Remove the duplicate local constant and keep the
existing WorkflowEngineError behavior unchanged.
🪄 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: e825b213-63b5-48c4-9a8c-a3a3b5b97f65
📒 Files selected for processing (4)
package.jsonsrc/workflow-api.tssrc/workflow-engine.test.tssrc/workflow-engine.ts
| const workflow = async (...args: unknown[]): Promise<unknown> => { | ||
| if (nestDepth >= WORKFLOW_MAX_NEST_DEPTH) { | ||
| throw new WorkflowEngineError( | ||
| "nest_depth", | ||
| `workflow() nesting limited to ${WORKFLOW_MAX_NEST_DEPTH} level`, | ||
| ); | ||
| } | ||
| if (!deps.resolveNestedSource || !deps.executeNested) { | ||
| throw new WorkflowEngineError("internal", "nested workflow() is not configured on this host"); | ||
| } | ||
| const nameOrRef = args[0] as string | { scriptPath: string }; | ||
| const childArgs = args[1]; | ||
| const source = await deps.resolveNestedSource(nameOrRef); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any host-side resolveNestedSource implementation confines scriptPath to the workspace root.
rg -nP -C6 'resolveNestedSource' --type=tsRepository: Waishnav/devspace
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tracked ts files count:"
git ls-files '*.ts' | wc -l
echo "search workflow-api references:"
fd -a 'workflow-api\.ts$' . || true
echo "search resolveNestedSource all files:"
rg -n -C 4 'resolveNestedSource|WORKFLOW_MAX_NEST_DEPTH|executeNested|WorkflowEngineError' . || true
echo "list relevant files:"
git ls-files | rg 'workflow|engine|test|src/workflow-api.ts' | head -200Repository: Waishnav/devspace
Length of output: 24696
Constrain scriptPath before resolving nested workflows.
args[0] is passed directly to resolveNestedSource, and the test resolver reads ref.scriptPath without containment; this API does not enforce that host resolvers limit paths to the workspace. Add runtime validation for the string/ref shape and document that resolveNestedSource must confine scriptPath to the configured workspace root, using "path" errors when it does not.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflow-api.ts` around lines 520 - 532, The nested workflow() path in
the workflow function must validate nameOrRef before calling
deps.resolveNestedSource: accept only a string or an object with a valid
scriptPath string, and reject invalid or workspace-escaping paths with
WorkflowEngineError code "path". Document in the resolveNestedSource contract
that host resolvers must confine scriptPath to the configured workspace root,
while preserving valid nested workflow resolution.
| if (typeof record.label === "string") out.label = record.label; | ||
| if (typeof record.phase === "string") out.phase = record.phase; | ||
| if (record.schema !== undefined) { | ||
| if (!record.schema || typeof record.schema !== "object" || Array.isArray(record.schema)) { | ||
| throw new WorkflowEngineError("schema", "agent opts.schema must be an object"); | ||
| } | ||
| out.schema = record.schema as object; | ||
| } | ||
| if (typeof record.model === "string") out.model = record.model; | ||
| if (typeof record.effort === "string") out.effort = record.effort; | ||
| if (typeof record.provider === "string") out.provider = record.provider; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-string option values are silently dropped.
schema, isolation, and writeMode throw on bad input, but a non-string provider (or model/effort) is silently ignored — agent("x", { provider: 123 }) quietly falls back to meta.defaultProvider instead of failing. Reject wrong-typed values that are present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/workflow-api.ts` around lines 601 - 611, Update the option parsing around
record.model, record.effort, and record.provider to reject values when those
properties are present but not strings, matching the validation behavior of
schema, isolation, and writeMode. Preserve assignment for valid strings and
ensure invalid values throw a WorkflowEngineError instead of being silently
omitted and falling back to defaults.
1 participant
Summary
createWorkflowApi: agent/parallel/pipeline/phase/log/workflow + semaphore + ALS phaseexecuteWorkflowhost entry; nest depth 1; isolation worktree hookrunProvider(no real LLM)Stack
PR3 of 5 · base:
pr/dw-2-store-sandboxTest plan
tsx src/workflow-engine.test.tsnpm run typecheckSummary by CodeRabbit
New Features
Tests