feat(workflow-engine): Introducing a workflow engine to worklows as tools - #111
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new workflow subsystem to core (types, builder, executor, external client, errors, managers), wires an internal tool-caller into ToolContext/server, integrates v4 workflow tools and public typings in the minimal example, and adds UI widgets, styles, and extensive tests for workflow and executor behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Widget as WorkflowWidget
participant Client
participant Executor as WorkflowExecutor
participant ToolCtx as ToolContext
participant External as ExternalToolClient
User->>Widget: Click "Run" with inputs
Widget->>Client: client.tools.callGreet_and_echo(input)
Client->>Executor: executor.execute(input, toolContext)
Executor->>Executor: validate input schema, build WorkflowContext
loop steps
alt internal tool step
Executor->>ToolCtx: callTool(toolName, mappedInput)
ToolCtx->>ToolCtx: _internalToolCaller(toolName, input, ctx)
else external step
Executor->>External: callTool(server, toolName, input)
External->>External: manage connection / invoke MCP
end
Executor->>Executor: validate step output, record result
end
Executor->>Client: return WorkflowExecutionResult
Client->>Widget: deliver result
Widget->>User: render results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Code Review: Workflow Engine FeatureOverviewThis PR introduces a comprehensive workflow engine that allows composing multi-step tools from existing tools and custom logic. The implementation is well-designed with strong type safety and follows the repository's conventions. Positive Highlights1. Excellent Architecture & Design ✅
2. Strong Type Safety ✅
3. Comprehensive Test Coverage ✅
4. Follows Repository Standards ✅
Issues & Concerns🔴 Critical: Resource Leak in External Tool ClientLocation: Issue: Race condition in connection management could lead to resource leaks. Problem: If a connection attempt fails, the pending promise is removed but the connection is never added to the cache. However, callers who awaited the same pending promise will get a client that's not tracked, leading to:
Fix: Ensure error handling propagates correctly and failed connections don't return partial clients. 🟡 Medium: Memory Leak in WorkflowExecutorLocation: Issue: The Problem:
Impact: In production with many workflow executions, this could cause:
Suggested Fix:
🟡 Medium: Missing Validation for Output SchemaLocation: Problem:
Clarification Needed:
🟡 Medium: Unclear Error Handling in
|
Add a comprehensive workflow engine that allows composing multiple tools into reusable workflows with advanced control flow capabilities. Features: - Sequential, parallel, and conditional step execution - Tool steps (call other tools), custom steps (inline logic), and external steps (call MCP servers) - Accumulated context passing data between steps - Configurable error handling (fail, skip, or custom handler) - Retry logic with exponential/linear backoff - Step timeouts and input mapping - Full TypeScript type safety with Zod validation - External tool client with connection caching and LRU eviction API: - Fluent builder: workflow(name).describe().input().output().step().build() - Step helpers: toolStep(), customStep(), externalStep() - Compiles to standard ToolDef for seamless createApp integration Implementation: - Core types and interfaces in workflow/types.ts - Step helpers in workflow/steps.ts - Fluent builder in workflow/builder.ts and workflow-builder-impl.ts - Runtime executor in workflow/executor.ts with retry and error handling - External MCP client in workflow/external-client.ts with caching - Custom error classes in workflow/errors.ts - Comprehensive test suite with 32 tests Exports all workflow APIs from core package index.
Enable workflows to call other tools in the same app by injecting an internal tool caller into the ToolContext. Implementation: - Add _internalToolCaller property to ToolContext (marked @internal) - Create tool caller function during tool registration with closure access to all tools - Inject caller into every tool's context for use by workflows - Workflow executor uses _internalToolCaller to invoke tools directly - Validates input via Zod schemas and preserves context (auth, locale, state) Benefits: - Zero configuration - works automatically for all workflows - Type-safe with proper validation - Context preservation through nested calls - No global registry or circular dependencies - Enables toolStep() to work seamlessly in workflows This allows workflows to compose existing tools without requiring external MCP server setup for internal tool calls.
Add comprehensive workflow examples to the minimal app demonstrating the workflow engine capabilities with rich interactive UI widgets. New Workflows (v4 API): - greet_and_echo: Sequential workflow with tool calls and custom logic * Calls greet_for_workflow tool * Adds excitement transformation * Calls echo_for_workflow with uppercase * Combines results with timestamp - process_greeting: Advanced workflow with parallel and conditional execution * Parallel processing of multiple names * Conditional branching (formal vs casual format) * Demonstrates complex control flow UI Components: - WorkflowWidget: Interactive UI for greet_and_echo workflow * Name input and excitement level slider (1-10) * Step-by-step result display * Excitement meter visualization * Beautiful gradient design - AdvancedWorkflowWidget: Interactive UI for process_greeting workflow * Dynamic name list management (add/remove) * Format selection (casual/formal radio buttons) * Greetings list display * Features checklist visualization Styling: - Workflow-specific gradients and glassmorphism effects - Step result cards with backdrop blur - Range slider with custom styling - Form controls (radio, dynamic lists) - Full dark mode support Both workflows demonstrate proper output handling, nested tool calls, and integration with the UI system via defineReactUI().
Enable workflows to call external MCP servers via HTTP in addition to stdio transport for greater flexibility and remote server support. Implementation: - Import StreamableHTTPClientTransport from MCP SDK - Auto-detect transport based on server identifier format * HTTP/HTTPS URLs → StreamableHTTPClientTransport * Command names → StdioClientTransport (stdio) - Pass URL object to StreamableHTTPClientTransport constructor Transport Selection: - stdio: "weather-server" or "mcp://weather-server" - HTTP: "http://localhost:3000/mcp" or "https://api.example.com/mcp" Documentation: - Updated ExternalToolCaller type documentation with examples - Updated ExternalStepConfig with transport details - Updated externalStep() JSDoc with stdio and HTTP examples Benefits: - Connect to remote MCP servers over HTTP - Unified API for both local and remote tool calls - Automatic protocol detection - Supports all StreamableHTTP features (sessions, auth, reconnection) This allows workflows to seamlessly integrate with both local and remote MCP servers without changing the workflow definition API.
- Introduced new error classes: WorkflowDefinitionError for invalid workflow definitions and ToolResponseValidationError for failed tool response validations. - Updated WorkflowExecutor to support optional response validators for tool calls, improving type safety and error handling. - Enhanced the getOrCreateConnection method in ExternalToolClient to handle race conditions for concurrent connection requests. - Added a close method to WorkflowExecutor for resource cleanup, ensuring proper management of external tool connections. These changes improve the robustness of the workflow engine by providing clearer error reporting and validation mechanisms.
afab4d6 to
68bec98
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@packages/core/src/server/index.ts`:
- Around line 748-771: internalToolCaller currently calls targetToolDef.handler
directly, bypassing middleware, plugin hooks, event emission, and output
validation; instead, route internal calls through the same tool execution
pipeline used for external calls by invoking the shared tool-invocation helper
(or extract one if it doesn't exist) so both registered tool execution and
internalToolCaller use it. Modify internalToolCaller to: 1) perform input
parsing as now but then call the central execution function (e.g.,
executeTool/runTool/invokeTool — create this helper if missing) which runs
middleware, plugin hooks, emits events, executes the handler, and runs output
validation; 2) keep passing ToolContext (including state and
_internalToolCaller) so internal calls retain context; and 3) remove direct
calls to targetToolDef.handler so auth/rate-limit/telemetry and validation
remain consistent.
In `@packages/core/src/workflow/executor.ts`:
- Around line 402-434: The parallel and branch executors bypass per-step
policies by directly calling executeStep; change them to route each child
through a common helper that enforces StepConfig (retry/timeout/onError) such as
a new private method (e.g., executeStepWithPolicy or applyStepConfigAndExecute)
and use that in executeParallelStep and executeBranchStep instead of
executeStep; the helper should read the child step's config, wrap the
executeStep(childStep, context) call with the retry/timeout/onError logic
already used for top-level steps, and return the child result (or propagate
handled errors) so nested steps honor their own policies.
- Around line 197-327: executeNamedStep currently records retries as a
zero-based index and timeouts create StepTimeoutError with a literal "step"
name; update the retry metadata and timeout error naming by (1) changing the
onRetry handler usage so it sets retries = attempt + 1 (i.e., convert the
zero-based attempt index to a 1-based attempt count) in
executeNamedStep/executeStepWithRetry, and (2) propagate the step name into
timeout errors by adding an optional stepName parameter to executeStepWithRetry
and executeStepWithTimeout (or otherwise pass the NamedStep.name from
executeNamedStep into executeStepWithRetry), then use that stepName when
constructing the StepTimeoutError in executeStepWithTimeout instead of the
hardcoded "step".
In `@packages/core/src/workflow/workflow-builder-impl.ts`:
- Around line 189-217: The ToolDef.output cast can remain as-is, but you must
ensure the WorkflowExecutor's resources are cleaned up: either call
executor.close() at the end of the tool handler's execution (after
WorkflowExecutor.run/execute completes and after any async work), or expose a
disposal API on the returned tool definition (e.g., add a close()/dispose()
function property on the ToolDef that calls executor.close()) so external code
can explicitly free resources; update the code that constructs the
WorkflowExecutor (symbol: WorkflowExecutor, local: executor) and the tool
creation block (symbol: ToolDef) to implement one of these two cleanup
approaches.
In `@packages/core/tests/workflow.test.ts`:
- Around line 456-491: In the "should retry with exponential backoff" test,
replace real timers with Vitest fake timers: call vi.useFakeTimers() before
invoking wf.handler and vi.useRealTimers() in a finally block; start the handler
without awaiting, then drive retries deterministically by calling
vi.advanceTimersByTimeAsync() for the expected delays (10ms then 20ms for
exponential backoff) before awaiting the handler result; keep assertions on
attempts, delays, and ordering but compute delays from controlled timer
advancement (or remove Date.now usage and assert the number/order of advances)
so the test is deterministic and not environment-dependent (targets: the test
name string, workflow builder (workflow(...).step(..., { retry: { maxAttempts:
3, delay: 10, backoff: "exponential" } })), customStep, and wf.handler).
🧹 Nitpick comments (6)
packages/core/src/workflow/external-client.ts (2)
229-246: Consider logging transport close failures for debugging.The nested catch blocks silently swallow errors. While the best-effort cleanup approach is reasonable, completely silent failures may make debugging difficult in production.
🔧 Optional: Add debug logging for close failures
private async closeConnection(connection: CachedConnection): Promise<void> { try { await connection.client.close(); } catch { // If Client.close() fails, attempt to close the transport directly try { await connection.transport.close(); - } catch { + } catch (transportError) { // Transport close also failed - for stdio transport, try to kill the process if (connection.transport instanceof StdioClientTransport) { // StdioClientTransport has internal process management // The close() call above should handle it, but if it doesn't, // we've done our best effort } - // Swallow the error - we've attempted cleanup + // Best-effort cleanup - log for debugging if needed + // console.debug("Failed to close transport:", transportError); } } }
251-256:closeAllshould handle individual connection failures gracefully.If one connection fails to close, the loop continues but errors are lost. Consider using
Promise.allSettledfor parallel cleanup or accumulating errors.♻️ Suggested improvement for resilient cleanup
async closeAll(): Promise<void> { - for (const connection of this.connections.values()) { - await this.closeConnection(connection); - } + const closePromises = Array.from(this.connections.values()).map((connection) => + this.closeConnection(connection) + ); + await Promise.allSettled(closePromises); this.connections.clear(); }examples/minimal/src/index.ts (1)
244-267: Type assertions on workflow context could be improved.The repeated
ctx.input as { name: string }andctx.outputs.greet as { message: string }assertions work but reduce type safety. This is acceptable for example code demonstrating the workflow API, but production workflows would benefit from stronger typing.examples/minimal/src/ui/WorkflowWidget.tsx (2)
174-179: Add radix toparseIntfor clarity.While
parseIntdefaults to base 10 for numeric strings, explicitly specifying the radix improves code clarity and avoids potential edge cases.🔧 Minor fix
onChange={(e) => setWorkflowInput({ ...workflowInput, - excitement: parseInt(e.target.value), + excitement: parseInt(e.target.value, 10), }) }
148-202: Modal accessibility could be improved.The modal implementation works but could benefit from focus trapping and ARIA attributes for better accessibility.
♿ Optional accessibility improvements
{isModalOpen && ( - <div className="modal-overlay" onClick={() => setIsModalOpen(false)}> - <div className="modal" onClick={(e) => e.stopPropagation()}> + <div className="modal-overlay" onClick={() => setIsModalOpen(false)} role="dialog" aria-modal="true" aria-labelledby="modal-title"> + <div className="modal" onClick={(e) => e.stopPropagation()}> - <h2>Configure Workflow</h2> + <h2 id="modal-title">Configure Workflow</h2>packages/core/src/workflow/workflow-builder-impl.ts (1)
106-171: Preserve input/output type generics through step/parallel/branch methods.The implementations of
step(),parallel(),branch(), andui()return hard-codedWorkflowBuilderWithSteps<TName, z.ZodType, z.ZodType>(lines 110, 124, 131, 145, 155, 170, 173), which drops the concreteTInputandTOutputgenerics set by priorinput()/output()calls. The interfaces promise to preserve these types (WorkflowBuilderWithOutput.step()returnsWorkflowBuilderWithSteps<TName, TInput, TOutput>;parallel()andbranch()returnthis), but the implementation doesn't deliver.Consider making the class generic over
TInputandTOutputto maintain type safety through the entire fluent API chain, ensuringbuild()retains strong typing.
Code Review: Workflow Engine FeatureSummaryThis PR introduces a comprehensive workflow engine that allows composing multi-step tools from existing tools and custom logic. The implementation is well-structured with strong type safety, good error handling, and thorough test coverage (637 lines of tests). ✅ StrengthsArchitecture & Design
Error Handling
Features
🔍 Issues & Recommendations1. CRITICAL: Race Condition in Connection Management (packages/core/src/workflow/external-client.ts:95-136)The // Check if we have a cached connection
const cached = this.connections.get(server);
if (cached) {
cached.lastUsed = Date.now(); // ✅ Good
return cached.client;
}
// Check if a connection is already being created
const pending = this.pendingConnections.get(server);
if (pending) {
const { client } = await pending; // ⚠️ Returns client but doesn't update cache timestamp
return client;
}Issue: When waiting for a pending connection, the code doesn't update the Fix: After awaiting the pending connection, update the timestamp: if (pending) {
const { client } = await pending;
// Update timestamp for concurrent access
const cached = this.connections.get(server);
if (cached) {
cached.lastUsed = Date.now();
}
return client;
}2. Memory Leak Risk: No Cleanup on Workflow Completion (packages/core/src/workflow/workflow-builder-impl.ts:218-228)The workflow handler creates a handler: async (input: z.infer<TInput>, context: ToolContext) => {
const result = await executor.execute(input, context); // ⚠️ executor.close() never called
return result.output;
}Issue: Each workflow execution may open external MCP connections, but there's no cleanup mechanism. This will accumulate connections over time. Recommendation:
handler: async (input, context) => {
try {
const result = await executor.execute(input, context);
return result.output;
} finally {
await executor.close();
}
}However, Option B would close connections after every workflow invocation, defeating the cache. Consider implementing a TTL-based cleanup or shared executor pattern. 3. Type Safety Gap: Validator Discrimination (packages/core/src/workflow/executor.ts:176-192)if (typeof validator === 'function') {
return validator(result);
} else if (typeof validator === 'object' && 'parse' in validator) {
return validator.parse(result);
}Issue: This runtime type check is fragile. All functions are objects, and the order matters. A Zod schema could theoretically be callable. Better approach: Use type guards or check for Zod-specific properties first: // Check for Zod schema first (more specific)
if (validator && typeof validator === 'object' && 'parse' in validator && typeof validator.parse === 'function') {
return validator.parse(result);
} else if (typeof validator === 'function') {
return validator(result);
}
throw new Error('Invalid validator: must be a Zod schema or validation function');4. Incorrect Error Message (packages/core/src/workflow/executor.ts:314)reject(new StepTimeoutError('step', timeout));Issue: Hardcoded string "step" instead of actual step name. This makes debugging timeouts nearly impossible in multi-step workflows. Fix: Pass the actual step name (requires refactoring to pass step name through the call chain): private async executeStepWithTimeout(
step: Step,
stepName: string, // Add parameter
context: WorkflowContext,
timeout: number
): Promise<unknown> {
// ...
reject(new StepTimeoutError(stepName, timeout));
}5. Inconsistent Error Handling in Close Methods (packages/core/src/workflow/external-client.ts:229-245)try {
await connection.client.close();
} catch {
try {
await connection.transport.close();
} catch {
// Swallow the error
}
}Issues:
Recommendation: At minimum, use the debug logger: import { debugLogger } from '../debug/logger';
try {
await connection.client.close();
} catch (error) {
debugLogger.log('workflow', 'error', 'Failed to close client', { error, server });
try {
await connection.transport.close();
} catch (transportError) {
debugLogger.log('workflow', 'error', 'Failed to close transport', {
error: transportError,
server
});
}
}6. Potential Type Confusion (packages/core/src/types/tools.ts - new additions)The PR adds
7. Test Coverage GapsWhile test coverage is good (637 lines), I noticed potential gaps: Missing test scenarios:
🔧 Code Quality IssuesMinor Issues
const config = step.type === 'parallel' || step.type === 'branch' ? step.config : step.config;This is a no-op - both branches return
private readonly CACHE_TTL = 5 * 60 * 1000; // 5 minutes
private readonly MAX_CONNECTIONS = 10;Consider making these configurable through constructor options.
📝 DocumentationGood
Improvements Needed
🎯 Performance Considerations
🔐 Security Concerns
transport = new StdioClientTransport({
command: serverName, // ⚠️ User input executed as command
args: [],
});While this is expected behavior, ensure workflows are not created from untrusted input. Document this security consideration.
✅ Final VerdictRecommendation: Approve with required changes This is a well-designed feature with excellent type safety and comprehensive testing. However, the critical issues (#1 and #2) should be addressed before merging:
The other issues are important but not blocking for an initial merge. Consider creating follow-up issues for:
Great work overall! This is a valuable addition to the framework. 🎉 Checklist Summary
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@examples/minimal/src/ui/styles.css`:
- Around line 446-495: Add a visible keyboard focus state for the custom range
control by styling the focused thumb and track: update rules for
input[type="range"] to include a focus selector (e.g., input[type="range"]:focus
and input[type="range"]:focus-visible) and add focused pseudo-elements for the
webkit and moz thumbs (input[type="range"]::-webkit-slider-thumb:focus /
:focus-visible and input[type="range"]::-moz-range-thumb:focus / :focus-visible)
and focused track (input[type="range"]::-webkit-slider-runnable-track and
::-moz-range-track when the input is focused) so keyboard users see a clear
ring/outline (contrasting color and sufficient thickness) and ensure the dark
theme (.dark) has a matching focus color; apply these changes near the existing
input[type="range"]::-webkit-slider-thumb and ::-moz-range-thumb blocks.
- Around line 379-408: The .remove-btn and .add-btn classes currently only
define hover styles and lack keyboard focus indicators; update the stylesheet to
add :focus and preferably :focus-visible rules for both .remove-btn and .add-btn
(matching the existing hover visual language) to provide a clear, visible
outline or box-shadow and ensure outline-offset/contrast for keyboard users, and
include :focus { outline: none } only if replaced by an accessible custom focus
style so the focus state remains visible; modify the rules near the .remove-btn
and .add-btn blocks to include these focus selectors.
In `@packages/core/src/workflow/errors.ts`:
- Around line 89-97: The constructor of ToolResponseValidationError currently
spreads details after toolName which allows details.toolName to override the
canonical toolName; change the object passed to super so toolName wins (e.g.,
spread details first then toolName: super(message,
"TOOL_RESPONSE_VALIDATION_ERROR", { ...details, toolName }) or use
Object.assign({}, details, { toolName })) in the ToolResponseValidationError
constructor to ensure the public readonly toolName always takes precedence.
- Around line 54-64: The constructor for ExternalToolError builds metadata with
"{ server, toolName, ...details }" which allows caller-provided details to
override server/toolName; change the merge order so caller details are applied
first and then server/toolName override them (e.g., merge details before
server/toolName) when calling super in ExternalToolError's constructor to ensure
the public readonly server and toolName remain authoritative.
In `@packages/core/src/workflow/external-client.ts`:
- Around line 45-56: In callTool, avoid unsafely casting the unknown input to
Record<string,unknown> before passing to client.callTool; instead validate input
is a plain object (e.g., object literal, not array/null) and only set the
arguments field when that check passes, otherwise call client.callTool without
the arguments property (or throw a validation error). Update the logic around
client.callTool in the callTool method (and any helper used like
getOrCreateConnection) to either perform the object-shape check and pass { name:
toolName, arguments: input as Record<string,unknown> } only when valid, or omit
the arguments key entirely for non-object inputs. Ensure the change preserves
Promise<unknown> return behavior and covers primitives/arrays/null safely.
In `@packages/core/tests/workflow.test.ts`:
- Around line 172-176: Add a Biome ignore comment to each branch configuration
object that uses the then property (e.g., the .branch("branch1", { when: ...,
then: [...], else: [...] }) calls) to suppress the noThenProperty lint rule;
place a line like "// biome-ignore noThenProperty" immediately above each
offending branch config (the ones around the .branch calls containing then/else
arrays) so the tests keep the API intact without lint errors.
♻️ Duplicate comments (1)
packages/core/src/server/index.ts (1)
748-771: Internal tool calls bypass the standard execution pipeline.Line 748–764 directly invokes
targetToolDef.handler, skipping middleware, plugin hooks, event emission, and output validation. This can diverge from normal tool execution and bypass auth/rate-limit/telemetry.Please route internal calls through the same centralized execution path used by normal tool invocations (extract/reuse the existing helper) so internal and external behavior stays consistent.
🧹 Nitpick comments (1)
examples/minimal/src/ui/styles.css (1)
249-259: Consider adding explicit text color and dark mode variant.
.workflow-resultsets only a background gradient without an explicit textcolor. If inherited color changes, readability could be affected. Also, there's no.dark .workflow-resultvariant defined.💡 Suggested improvement
.workflow-result { background: linear-gradient(135deg, `#f093fb` 0%, `#f5576c` 100%); + color: white; } + +.dark .workflow-result { + background: linear-gradient(135deg, `#d063db` 0%, `#d5374c` 100%); +}
- Introduced ExecutorManager and EdgeExecutorManager for improved lifecycle management of workflow executors, optimizing resource usage in both traditional and edge environments. - Added new error classes: WorkflowDefinitionError and ToolResponseValidationError for better error reporting during workflow execution. - Updated WorkflowExecutor to support enhanced validation and error handling, including improved retry logic and step configuration management. - Enhanced ExternalToolClient with configurable connection settings and improved error handling during connection closure. These changes significantly improve the robustness and flexibility of the workflow engine, enabling better performance and error management across different execution environments.
Pull Request Review: Workflow Engine ImplementationSummaryThis PR introduces a comprehensive workflow engine for composing multi-step tool workflows in @mcp-apps-kit/core. The implementation is well-architected with strong type safety, comprehensive error handling, and production-ready lifecycle management. Code Quality: ⭐⭐⭐⭐ (4/5)Strengths1. Architecture & Design
2. Error Handling
3. Production Features
4. Documentation
Issues Found🔴 Critical1. Race Condition in Connection Management (packages/core/src/workflow/external-client.ts:186-214) The // Line 189: Eviction happens BEFORE checking pending connections
if (this.connections.size >= this.MAX_CONNECTIONS) {
await this.evictOldConnections();
}
// Line 194: Create new pending connection
pending = this.createConnectionWithTransport(server);
this.pendingConnections.set(server, pending);Problem: If two concurrent requests for different servers arrive when the cache is full:
Fix: Check if (this.connections.size + this.pendingConnections.size >= this.MAX_CONNECTIONS) {
await this.evictOldConnections();
}2. Memory Leak in Timeout Implementation (packages/core/src/workflow/executor.ts:336-353) The timeout cleanup has a subtle bug: let timer: ReturnType<typeof setTimeout> | undefined;
const timeoutPromise = new Promise<never>((_, reject) => {
timer = setTimeout(() => {
reject(new StepTimeoutError(stepName, timeout));
}, timeout);
});
try {
const result = await Promise.race([this.executeStep(step, context), timeoutPromise]);
return result;
} finally {
if (timer !== undefined) {
clearTimeout(timer);
}
}Problem: If the step completes successfully before timeout, the timer is cleared. However, if the step throws an error that's not a timeout, the Fix: Use try {
const result = await Promise.race([...]);
if (timer !== undefined) clearTimeout(timer);
return result;
} catch (error) {
if (timer !== undefined) clearTimeout(timer);
throw error;
}Or move 🟡 High Priority3. Missing Validation in Workflow Builder (packages/core/src/workflow/workflow-builder-impl.ts) The builder validates duplicate step names and requires at least one step, but doesn't validate:
Recommendation: Add static analysis in 4. Potential Stack Overflow in Nested Workflows (packages/core/src/workflow/executor.ts:358-372) The Recommendation: Consider iterative execution with an explicit stack for very deep nesting, or document maximum nesting depth. 5. Input Validation Inconsistency (packages/core/src/workflow/external-client.ts:22-30) External tool input is validated to be a plain object, but internal tool steps don't have this validation: private async executeToolStep(step: ToolStep, context: WorkflowContext): Promise<unknown> {
const input = step.config?.mapInput ? step.config.mapInput(context) : context.input;
return context.callTool(step.toolName, input); // No validation that input is an object
}Recommendation: Apply consistent validation or document why external tools need stricter validation. 🟢 Medium Priority6. Test Coverage Gaps Based on the test file (workflow.test.ts), missing test coverage for:
Recommendation: Add integration tests for ExternalToolClient and ExecutorManager. 7. Performance Consideration: Sequential Branch Execution (packages/core/src/workflow/executor.ts:458-466) Branch steps execute sequentially: for (const childStep of stepsToExecute) {
const result = await this.executeStep(childStep, context);
results.push(result);
}While this maintains order, it doesn't allow parallel execution within a branch. Consider allowing developers to mark branch steps as parallelizable. 8. Cleanup Interval Memory in Edge Environments (packages/core/src/workflow/external-client.ts:79-89) The code attempts to prevent timers in edge environments: if (typeof setInterval !== "undefined" && typeof process !== "undefined") {
this.cleanupInterval = setInterval(...);
}Issue: This check isn't reliable. Edge runtimes like Deno, Bun, and Cloudflare Workers have Recommendation: Use explicit environment detection via Security Considerations: ✅ GoodStrengths
Recommendations
Performance Considerations: ✅ GoodStrengths
Recommendations
Test Coverage:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/core/src/server/index.ts`:
- Around line 395-399: The stop function currently awaits
ExecutorManager.getInstance().shutdown() and if that throws the HTTP server
isn't closed; wrap the executor shutdown in try/finally so the server close
logic always runs (call the server.close()/closeServerPromise or existing
shutdown helper in the finally block), then if shutdown threw rethrow the error
after closing the server; update the stop implementation that references
ExecutorManager.getInstance().shutdown() to ensure server closure happens in
finally and errors are propagated.
In `@packages/core/src/workflow/executor-manager.ts`:
- Around line 145-148: Current eviction unconditionally calls evictLRU() when
this.executors.size >= this.config.maxExecutors which can close an active
executor; change the logic to only consider idle executors for eviction: inspect
the executor entries in this.executors, filter for idle/not-in-flight instances
(use the executor state flag/property used in Executor class or map values),
pick the least-recently-used idle executor and evict it via evictLRU-like
behavior; if no idle executor exists, do not evict active ones—instead fail fast
by throwing a clear error or return a specific failure so callers can handle
oversubscription (apply the same change to the other eviction site that uses
evictLRU()). Ensure you update evictLRU or add a new helper (e.g.,
evictIdleLRU()) so it only closes idle executors.
In `@packages/core/src/workflow/index.ts`:
- Around line 30-40: The example snippets that call z.string() are missing the
Zod import; update each snippet that uses z (e.g., the workflow example that
uses workflow(), toolStep(), createApp()) to include import { z } from "zod"; at
the top of the snippet so the z.symbols resolve and the examples are copy-paste
ready.
In `@packages/core/src/workflow/workflow-builder-impl.ts`:
- Around line 214-225: The step() implementation currently drops any provided
config for steps with type "parallel" or "branch"; update
WorkflowBuilderImpl.step to validate this by throwing a WorkflowValidationError
when config is passed for step types "parallel" or "branch" (instead of silently
ignoring it). Locate the step() method and the stepWithConfig merge logic, add
an early guard that checks if config is truthy && (step.type === "parallel" ||
step.type === "branch") and throw new WorkflowValidationError with a clear
message identifying the step name and invalid use of config; keep existing
behavior for other step types (merge config as before).
♻️ Duplicate comments (4)
packages/core/src/server/index.ts (1)
753-769: Internal tool calls still bypass middleware/hooks/output validation.This matches a concern already raised on this PR (Line 753-769). Please route internal calls through the same execution pipeline as external calls.
packages/core/src/workflow/executor.ts (1)
431-465: Child steps still bypass per-step retry/timeout/onError policies.
executeParallelStepandexecuteBranchStepinvokeexecuteStepdirectly, so nestedStepConfigis ignored. This can silently drop retries, timeouts, or skip/error-handler behavior for child steps.🔧 Proposed fix (route child steps through policy-aware execution)
@@ - const results = await Promise.all( - step.steps.map((childStep) => this.executeStep(childStep, context)) - ); + const results = await Promise.all( + step.steps.map((childStep, index) => + this.executeChildStep(`parallel[${index}]`, childStep, context) + ) + ); @@ - for (const childStep of stepsToExecute) { - const result = await this.executeStep(childStep, context); + for (const [index, childStep] of stepsToExecute.entries()) { + const result = await this.executeChildStep(`branch[${index}]`, childStep, context); results.push(result); } @@ + private async executeChildStep( + stepName: string, + step: Step, + context: WorkflowContext + ): Promise<unknown> { + try { + return await this.executeStepWithRetry(stepName, step, context, () => {}); + } catch (error) { + const errorHandling = this.getErrorHandling(step); + if (errorHandling === "skip") return undefined; + if (typeof errorHandling === "function") { + return await errorHandling(error as Error, context); + } + throw new WorkflowExecutionError( + `Step "${stepName}" failed: ${(error as Error).message}`, + stepName, + { originalError: error } + ); + } + }packages/core/tests/workflow.test.ts (2)
194-202: Add Biome ignore comments for branchthenproperties.Biome’s
noThenPropertyrule flags these configs; add an ignore comment above each branch config to keep the API intact while passing lint.🛠️ Example fix (apply to each branch config)
.branch("branch1", { when: (ctx) => (ctx.input as { value: string }).value === "test", + // biome-ignore lint/suspicious/noThenProperty: required by workflow branch API then: [customStep(async () => ({ result: "then" }))], else: [customStep(async () => ({ result: "else" }))], })Also applies to: 328-337, 347-355, 365-375
482-517: Make the exponential-backoff retry test deterministic with fake timers.Using real time can be flaky under CI load; drive retries with
vi.useFakeTimers()andvi.advanceTimersByTimeAsync()instead.🧪 Suggested adjustment
it("should retry with exponential backoff", async () => { let attempts = 0; const delays: number[] = []; - let lastTime = Date.now(); + vi.useFakeTimers(); + let lastTime = Date.now(); @@ - await wf.handler({ value: "test" }, mockToolContext); + try { + const run = wf.handler({ value: "test" }, mockToolContext); + await vi.advanceTimersByTimeAsync(10); + await vi.advanceTimersByTimeAsync(20); + await run; + } finally { + vi.useRealTimers(); + }
🧹 Nitpick comments (1)
packages/core/src/workflow/workflow-builder-impl.ts (1)
329-362: PreferdefineToolfor ToolDef constructionThis inline ToolDef creation bypasses the standard helper used for inference and consistency. Please switch to
defineTool(anddefineUIif applicable) in the builder to align with the project’s type‑inference conventions.As per coding guidelines, use
defineTool/defineUIinstead of inline objects.
… eviction - Enhanced the server shutdown process to ensure the HTTP server always closes, even if an error occurs during executor shutdown. - Updated ExecutorManager to evict only idle executors, preventing disruption to active workflows and providing clearer error messages when capacity is exceeded. - Adjusted tests to reflect changes in error handling and eviction logic. These improvements enhance the reliability and robustness of the workflow engine during shutdown scenarios.
Workflow Engine PR ReviewSummaryThis is an excellent, well-architected feature addition that introduces a comprehensive workflow engine to the mcp-apps-kit. The implementation demonstrates strong engineering practices with thorough testing, proper error handling, and production-ready lifecycle management. 🎯 StrengthsArchitecture & Design
Code Quality
Testing
Integration
🔍 Areas for Improvement1. Race Condition in External Client Connection (Minor)Location: The // Current code
let pending = this.pendingConnections.get(server);
if (!pending) {
// Evict old connections if cache is full (before creating new promise)
if (this.connections.size >= this.MAX_CONNECTIONS) {
await this.evictOldConnections(); // ⚠️ Async operation before atomic store
}
pending = this.createConnectionWithTransport(server);
this.pendingConnections.set(server, pending);
}Issue: Between the Suggestion: Set a placeholder in if (!pending) {
const placeholder = Promise.resolve(); // Temporary marker
this.pendingConnections.set(server, placeholder as any);
if (this.connections.size >= this.MAX_CONNECTIONS) {
await this.evictOldConnections();
}
pending = this.createConnectionWithTransport(server);
this.pendingConnections.set(server, pending); // Replace placeholder
}2. Memory Leak Risk in ExecutorManager (Medium)Location: debugLogger.debug(`Evicting LRU idle executor: ${oldestKey}`);
// Fire and forget - don't block on cleanup
this.closeExecutor(oldestKey).catch(() => {
// Ignore errors during eviction
});
return true;Issue: The executor is removed from the map immediately in Suggestion: Only remove from map after successful close: private async closeExecutor(key: string): Promise<void> {
const managed = this.executors.get(key);
if (!managed) return;
try {
await managed.executor.close();
this.executors.delete(key); // ✅ Delete only after successful close
} catch (error) {
debugLogger.error(`Error closing executor: ${key}`, { error });
throw error;
}
}3. Validator Type Guard Could Be More PreciseLocation: function isZodSchema<T>(validator: Validator<T>): validator is z.ZodType<T> {
return (
typeof validator === "object" &&
validator !== null &&
"parse" in validator &&
typeof validator.parse === "function"
);
}Issue: This type guard could false-positive on any object with a Suggestion: Use Zod's built-in check: import { ZodType } from "zod";
function isZodSchema<T>(validator: Validator<T>): validator is z.ZodType<T> {
return validator instanceof ZodType;
}4. Server Shutdown Error Handling Could Be More GracefulLocation: Based on commit message: "Enhanced the server shutdown process to ensure the HTTP server always closes, even if an error occurs during executor shutdown." Observation: While the improvement is good, consider logging which specific executors failed during shutdown for debugging. This would help production troubleshooting. 5. Missing Input Validation DocumentationLocation: The Suggestion: Add to JSDoc: /**
* Create an external step that calls a tool from an external MCP server
*
* @param toolName - Name of the tool to call on the external server
* @param config - Step configuration
* @returns External step definition
*
* @remarks
* Tool input must be a plain object (not null, array, or primitive).
* ...
*/✅ Security Review
📊 Test CoverageThe test suite is comprehensive with ~1170 test cases across the core package. The workflow-specific tests (workflow.test.ts + executor-manager.test.ts) cover:
Suggestion: Consider adding integration tests for:
🎨 Code Style & ConventionsThe code adheres to the repository's standards:
📝 DocumentationThe inline documentation is excellent, particularly:
Minor suggestion: Add a workflow example to the main README.md or create a dedicated workflow guide in the docs/ folder. 🚀 Performance ConsiderationsStrengths:
Potential optimizations (for future consideration):
🎯 RecommendationAPPROVE ✅ This is production-ready code that significantly enhances the framework's capabilities. The minor issues identified above are not blockers and can be addressed in follow-up PRs if desired. The workflow engine is well-architected, thoroughly tested, and properly integrated with the existing codebase. The automatic lifecycle management for different deployment environments shows thoughtful design for real-world usage. Excellent work! 🎉 Priority of Issues:
|
- Introduced a new section in the README to describe the workflow engine, detailing its capabilities for composing multi-step workflows with features like parallel execution, conditional branching, and retry policies. - Added TypeScript code examples demonstrating the creation and registration of workflows, including sequential steps, parallel execution, and error handling configurations. - Updated the core README to include links to the new workflow engine documentation, enhancing the overall clarity and usability of the documentation. These additions provide users with comprehensive guidance on utilizing the workflow engine effectively within their applications.
Comprehensive Code Review - Workflow Engine ImplementationI've thoroughly reviewed this PR introducing a comprehensive workflow engine. This is a substantial and well-architected feature. Below is my detailed feedback: Strengths1. Excellent Architecture
2. Strong Type Safety
3. Robust Error Handling
4. Performance and Resource Management
5. Excellent Test Coverage
CRITICAL Security IssueExternal MCP Server Command InjectionLocation: packages/core/src/workflow/external-client.ts:248-255 The createConnectionWithTransport method accepts arbitrary server identifiers without validation, allowing command injection: Risk: A malicious server string could execute arbitrary commands. Recommendation: Implement allowlist for permitted MCP servers. This MUST be fixed before merge. Other ImprovementsPotential Bugs
Performance
Type SafetyValidator type guard uses duck-typing - could produce false positives Test Coverage Gaps
Documentation
Code Quality Summary
RecommendationsMust Fix (P0):
Should Fix (P1):
Nice to Have (P2):
Overall AssessmentExcellent, production-ready implementation with deep understanding of TypeScript and async patterns. The command injection vulnerability must be addressed before merge. Recommendation: Approve with required changes Great work! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/README.md`:
- Around line 672-695: Update the documented import list and example to include
the missing ToolResponseValidationError type: add ToolResponseValidationError to
the import array alongside WorkflowError, WorkflowExecutionError,
StepTimeoutError, ExternalToolError, WorkflowValidationError, and
WorkflowDefinitionError, and include a brief example catch branch (e.g., if
(error instanceof ToolResponseValidationError) ...) demonstrating how to handle
tool response validation failures; reference the README's error list and the
Workflow error handling snippet where these symbols are shown.
| ### Workflow Errors | ||
|
|
||
| The workflow engine provides specific error types for debugging: | ||
|
|
||
| ```ts | ||
| import { | ||
| WorkflowError, // Base class for all workflow errors | ||
| WorkflowExecutionError, // Step execution failures | ||
| StepTimeoutError, // Step timeout exceeded | ||
| ExternalToolError, // External MCP call failures | ||
| WorkflowValidationError, // Input/output validation failures | ||
| WorkflowDefinitionError, // Invalid workflow configuration | ||
| } from "@mcp-apps-kit/core"; | ||
|
|
||
| try { | ||
| await orderWorkflow.handler(input, context); | ||
| } catch (error) { | ||
| if (error instanceof StepTimeoutError) { | ||
| console.log(`Step timed out: ${error.stepName}`); | ||
| } else if (error instanceof ExternalToolError) { | ||
| console.log(`External call failed: ${error.serverUri}`); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify workflow error types exist and match documentation
echo "=== Searching for all workflow error class definitions ==="
rg -n "class.*Error extends" packages/core/src/workflow/errors.ts
echo -e "\n=== Searching for ToolResponseValidationError ==="
rg -n "ToolResponseValidationError" packages/core/src/
echo -e "\n=== Searching for WorkflowValidationError ==="
rg -n "WorkflowValidationError" packages/core/src/
echo -e "\n=== Check error exports ==="
rg -n "export.*Error" packages/core/src/workflow/errors.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2521
Documentation incomplete: ToolResponseValidationError is missing.
The ToolResponseValidationError error type exists in the implementation (packages/core/src/workflow/errors.ts and is exported from @mcp-apps-kit/core), but it is not included in the documented error types here. Update the import list and consider adding it to the error handling example.
Current documentation (incomplete)
import {
WorkflowError,
WorkflowExecutionError,
StepTimeoutError,
ExternalToolError,
WorkflowValidationError,
WorkflowDefinitionError,
} from "@mcp-apps-kit/core";🤖 Prompt for AI Agents
In `@packages/core/README.md` around lines 672 - 695, Update the documented import
list and example to include the missing ToolResponseValidationError type: add
ToolResponseValidationError to the import array alongside WorkflowError,
WorkflowExecutionError, StepTimeoutError, ExternalToolError,
WorkflowValidationError, and WorkflowDefinitionError, and include a brief
example catch branch (e.g., if (error instanceof ToolResponseValidationError)
...) demonstrating how to handle tool response validation failures; reference
the README's error list and the Workflow error handling snippet where these
symbols are shown.
2 participants
These changes improve the robustness of the workflow engine by providing clearer error reporting and validation mechanisms.