refactor(ai-proxy): align McpClient API with main branch - #1527
Closed
matthv wants to merge 29 commits into
Closed
Conversation
…premature deps, add smoke test - Rewrite CLAUDE.md with project overview and architecture principles, remove changelog - Remove unused dependencies (ai-proxy, sequelize, zod) per YAGNI - Add smoke test so CI passes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… document system architecture
- Lint now covers src and test directories
- Replace require() with import, use stronger assertion (toHaveLength)
- Add System Architecture section describing Front/Orchestrator/Executor/Agent
- Mark Architecture Principles as planned (not yet implemented)
- Remove redundant test/.gitkeep
- Make index.ts a valid module with export {}
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erver (#1504) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: alban bertolini <albanb@forestadmin.com>
…+ DatabaseStore) (#1506)
…xecutor factories (#1510)
…ain (#1512) Co-authored-by: alban bertolini <albanb@forestadmin.com>
- Remove McpClient.tools property, loadTools() returns local array - Rename closeConnections() → dispose() - Rename testConnections() → checkConnection() - Add McpServers type export - Rename mcpServerConfigs → toolConfigs in create-ai-provider - Update all tests accordingly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
15 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
resolveMcpConfigs accesses args.toolConfigs, but the AiRouter['route'] interface only defines mcpServerConfigs?: unknown. Since toolConfigs doesn't exist on the type, the property will always be undefined at runtime, causing all MCP server configurations to be silently discarded.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/ai-proxy/src/create-ai-provider.ts around line 9:
`resolveMcpConfigs` accesses `args.toolConfigs`, but the `AiRouter['route']` interface only defines `mcpServerConfigs?: unknown`. Since `toolConfigs` doesn't exist on the type, the property will always be `undefined` at runtime, causing all MCP server configurations to be silently discarded.
Evidence trail:
packages/ai-proxy/src/create-ai-provider.ts:15 - accesses `args.toolConfigs`. packages/agent-toolkit/src/interfaces/ai.ts:27 - interface defines `mcpServerConfigs?: unknown`, not `toolConfigs`. packages/agent/src/routes/ai/ai-proxy.ts:36 - real caller passes `mcpServerConfigs`. packages/ai-proxy/test/create-ai-provider.test.ts:68 - tests pass `toolConfigs` directly bypassing interface type checking.
Comment on lines
+440
to
+448
| // validated against the AI response. This guard is the sole runtime enforcement. | ||
| if (recordIndex < 0 || recordIndex > maxIndex) { | ||
| throw new InvalidAIResponseError( | ||
| `AI selected record index ${recordIndex} which is out of range (0-${maxIndex})`, | ||
| ); | ||
| } | ||
|
|
||
| return recordIndex; | ||
| } |
There was a problem hiding this comment.
🟢 Low executors/load-related-record-step-executor.ts:440
The bounds check at lines 442-446 accepts non-integer values like 1.5 because it only tests recordIndex < 0 and recordIndex > maxIndex. Since 1.5 satisfies both conditions, it passes validation, but candidates[1.5] returns undefined and the subsequent relatedData[bestIndex].recordId access throws a TypeError. Consider adding Number.isInteger(recordIndex) to the validation to reject non-integer indices before they cause an out-of-bounds array access.
// NOTE: The Zod schema's .min(0).max(maxIndex) shapes the tool prompt only — it is NOT
// validated against the AI response. This guard is the sole runtime enforcement.
- if (recordIndex < 0 || recordIndex > maxIndex) {
+ if (!Number.isInteger(recordIndex) || recordIndex < 0 || recordIndex > maxIndex) {
throw new InvalidAIResponseError(
- `AI selected record index ${recordIndex} which is out of range (0-${maxIndex})`,
+ `AI selected record index ${recordIndex} which is out of range (0-${maxIndex}) or not an integer`,
);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/executors/load-related-record-step-executor.ts around lines 440-448:
The bounds check at lines 442-446 accepts non-integer values like `1.5` because it only tests `recordIndex < 0` and `recordIndex > maxIndex`. Since `1.5` satisfies both conditions, it passes validation, but `candidates[1.5]` returns `undefined` and the subsequent `relatedData[bestIndex].recordId` access throws a `TypeError`. Consider adding `Number.isInteger(recordIndex)` to the validation to reject non-integer indices before they cause an out-of-bounds array access.
Evidence trail:
packages/workflow-executor/src/executors/load-related-record-step-executor.ts lines 440-446 (bounds check with explicit comment about Zod not validating), line 413 (Zod schema with .int()), lines 223-227 (selectBestRecordIndex called), line 236 (`relatedData[bestIndex]` access), lines 450-455 (toRecordRef accessing properties on data parameter)
Comment on lines
+41
to
+46
| function extractRecordId( | ||
| primaryKeyFields: string[], | ||
| record: Record<string, unknown>, | ||
| ): Array<string | number> { | ||
| return primaryKeyFields.map(field => record[field] as string | number); | ||
| } |
There was a problem hiding this comment.
🟢 Low adapters/agent-client-agent-port.ts:41
When getRelatedData is called with a fields parameter that excludes the primary key fields, extractRecordId returns an array of undefined values cast to string | number. Downstream code expecting valid IDs receives corrupted data because the cast suppresses the type error. Consider validating that all primary key fields exist in the record before mapping, or ensuring primary key fields are always included in the query.
-function extractRecordId(
- primaryKeyFields: string[],
- record: Record<string, unknown>,
-): Array<string | number> {
- return primaryKeyFields.map(field => record[field] as string | number);
+function extractRecordId(
+ primaryKeyFields: string[],
+ record: Record<string, unknown>,
+): Array<string | number> {
+ return primaryKeyFields.map(field => {
+ const value = record[field];
+ if (value === undefined) {
+ throw new Error(`Primary key field '${field}' missing from record`);
+ }
+ return value as string | number;
+ });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/agent-client-agent-port.ts around lines 41-46:
When `getRelatedData` is called with a `fields` parameter that excludes the primary key fields, `extractRecordId` returns an array of `undefined` values cast to `string | number`. Downstream code expecting valid IDs receives corrupted data because the cast suppresses the type error. Consider validating that all primary key fields exist in the record before mapping, or ensuring primary key fields are always included in the query.
Evidence trail:
- packages/workflow-executor/src/adapters/agent-client-agent-port.ts lines 39-43: `extractRecordId` function casts `record[field]` (which can be `undefined`) to `string | number`
- packages/workflow-executor/src/adapters/agent-client-agent-port.ts lines 81-96: `getRelatedData` passes `fields` directly to query without ensuring primary key fields are included
- packages/workflow-executor/src/ports/agent-port.ts lines 14-19: `GetRelatedDataQuery.fields` is optional with no constraints on content
- packages/workflow-executor/src/types/record.ts lines 35-37: `RecordData.recordId` expects `Array<string | number>` but could receive `undefined` values due to the cast
Comment on lines
+12
to
+15
| pendingStepExecutionForRun: (runId: string) => | ||
| `/liana/v1/workflow-step-executions/pending?runId=${encodeURIComponent(runId)}`, | ||
| updateStepExecution: (runId: string) => `/liana/v1/workflow-step-executions/${runId}/complete`, | ||
| collectionSchema: (collectionName: string) => `/liana/v1/collections/${collectionName}`, |
There was a problem hiding this comment.
🟢 Low adapters/forest-server-workflow-port.ts:12
ROUTES.updateStepExecution(runId) and ROUTES.collectionSchema(collectionName) do not encode their path parameters, so values containing /, ?, or # produce malformed URLs that either 404 or route to the wrong endpoint. ROUTES.pendingStepExecutionForRun already uses encodeURIComponent for the same purpose.
- updateStepExecution: (runId: string) => `/liana/v1/workflow-step-executions/${runId}/complete`,
- collectionSchema: (collectionName: string) => `/liana/v1/collections/${collectionName}`,
+ updateStepExecution: (runId: string) => `/liana/v1/workflow-step-executions/${encodeURIComponent(runId)}/complete`,
+ collectionSchema: (collectionName: string) => `/liana/v1/collections/${encodeURIComponent(collectionName)}`,🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/adapters/forest-server-workflow-port.ts around lines 12-15:
`ROUTES.updateStepExecution(runId)` and `ROUTES.collectionSchema(collectionName)` do not encode their path parameters, so values containing `/`, `?`, or `#` produce malformed URLs that either 404 or route to the wrong endpoint. `ROUTES.pendingStepExecutionForRun` already uses `encodeURIComponent` for the same purpose.
Evidence trail:
packages/workflow-executor/src/adapters/forest-server-workflow-port.ts lines 11-16: `pendingStepExecutionForRun` uses `encodeURIComponent(runId)` while `updateStepExecution` and `collectionSchema` directly interpolate their parameters without encoding.
Merge main into feature branch, resolve conflicts by taking main's versions of router.ts, create-ai-provider.ts and their tests. Bump workflow-executor internal deps to match main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| return model; | ||
| } | ||
|
|
||
| async loadRemoteTools( |
There was a problem hiding this comment.
🟢 Low src/ai-client.ts:39
loadRemoteTools is not thread-safe: if called concurrently, two calls can both create McpClient instances, but only the second one is stored in this.mcpClient. The first client is orphaned and never disposed, leaking its connections. Consider adding a lock or check-and-set pattern to ensure only one client creation proceeds at a time.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/ai-proxy/src/ai-client.ts around line 39:
`loadRemoteTools` is not thread-safe: if called concurrently, two calls can both create `McpClient` instances, but only the second one is stored in `this.mcpClient`. The first client is orphaned and never disposed, leaking its connections. Consider adding a lock or check-and-set pattern to ensure only one client creation proceeds at a time.
Evidence trail:
packages/ai-proxy/src/ai-client.ts lines 39-48 (REVIEWED_COMMIT): `loadRemoteTools` method showing the race condition - `await newClient.loadTools()` creates a suspension point between client creation (line 44) and assignment to `this.mcpClient` (line 46), allowing concurrent calls to create orphaned clients
…eability Add sourceId to McpToolRef so that persisted execution data (pendingData, executionParams) tracks which MCP server provided the tool. This fixes tool lookup on re-entry (confirmation flow) when multiple servers expose tools with the same name. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… collection schema - Replace non-null assertion with explicit McpToolNotFoundError when AI selects a tool name that doesn't match any available tool - Resolve related collection name from parent schema before looking up the related schema in cache, fixing cases where relation name differs from target collection name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
Macroscope review — triageReviewed all 7 Macroscope comments. Here's the disposition: Fixed in this PR:
Not applicable / Ignored:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
McpClient.closeConnections()→dispose()McpClient.testConnections()→checkConnection()McpClient.toolsproperty,loadTools()returns a local array insteadMcpServerstype exportmcpServerConfigs→toolConfigsincreate-ai-providerPure renaming refactor, no logic changes.
Test plan
yarn workspace @forestadmin/ai-proxy build— compilesyarn workspace @forestadmin/ai-proxy test— 14/14 suites, 210 tests passyarn workspace @forestadmin/workflow-executor test— 427 tests pass🤖 Generated with Claude Code
Note
Align
McpClientAPI with main branch and introduce theworkflow-executorpackage@forestadmin/workflow-executorpackage with a full step-execution engine:Runner, store adapters (InMemoryStore,DatabaseStore), HTTP server (ExecutorHttpServer), and step executors for Condition, ReadRecord, UpdateRecord, TriggerAction, LoadRelatedRecord, and MCP step types.buildInMemoryExecutorandbuildDatabaseExecutorfactory functions inbuild-workflow-executor.tsthat wire up all dependencies and handle SIGTERM/SIGINT for graceful shutdown.AiClient,createBaseChatModel, andgetAiConfigurationtopackages/ai-proxyand renames thecloseConnectionmethod todisposeonMcpClient.ServerUtilsfrom theforestadmin-clientpackage index and adds theworkflow-executorpackage to CI inbuild.yml.McpClient.closeConnectionis renamed todispose; any existing callers must update to the new method name.Macroscope summarized d43f168.