Skip to content

feat(agent): proxify ws executor call#1522

Open
arnaud-moncel wants to merge 1 commit intofeat/prd-214-setup-workflow-executor-packagefrom
feat/wf-executor-proxy
Open

feat(agent): proxify ws executor call#1522
arnaud-moncel wants to merge 1 commit intofeat/prd-214-setup-workflow-executor-packagefrom
feat/wf-executor-proxy

Conversation

@arnaud-moncel
Copy link
Copy Markdown
Member

@arnaud-moncel arnaud-moncel commented Apr 1, 2026

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

Note

Add WebSocket executor proxy routes under /_internal/workflow-executions/* in the agent

  • Adds a new WorkflowExecutorProxyRoute that proxies GET, POST, and PATCH requests from /_internal/workflow-executions/:runId/* to an external workflow executor at /runs/*.
  • The proxy is only registered when workflowExecutorUrl is set in AgentOptions; defaults to null otherwise.
  • Adds a multi-stage Dockerfile and main.ts entrypoint for the workflow executor service, which starts on port 4001 and shuts down gracefully on SIGTERM/SIGINT.
  • Risk: network errors from the executor are not caught and will surface directly as request failures to the caller.

Macroscope summarized 1f28b96.

Comment on lines +36 to +39
const executorPath = context.path.replace(
WorkflowExecutorProxyRoute.AGENT_PREFIX,
WorkflowExecutorProxyRoute.EXECUTOR_PREFIX,
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium workflow/workflow-executor-proxy.ts:36

handleProxy uses context.path to construct the target URL, but path contains only the pathname without the query string. Requests with query parameters (e.g., ?foo=bar) lose those parameters when forwarded to the executor. Consider using context.url instead, which includes both the pathname and query string.

-    const executorPath = context.path.replace(
+    const executorPath = context.url.replace(
       WorkflowExecutorProxyRoute.AGENT_PREFIX,
       WorkflowExecutorProxyRoute.EXECUTOR_PREFIX,
     );
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/agent/src/routes/workflow/workflow-executor-proxy.ts around lines 36-39:

`handleProxy` uses `context.path` to construct the target URL, but `path` contains only the pathname without the query string. Requests with query parameters (e.g., `?foo=bar`) lose those parameters when forwarded to the executor. Consider using `context.url` instead, which includes both the pathname and query string.

Evidence trail:
1. packages/agent/src/routes/workflow/workflow-executor-proxy.ts lines 36-40 (REVIEWED_COMMIT): Code uses `context.path.replace()` to construct `executorPath`, then `new URL(executorPath, this.executorUrl)` to create the target URL.
2. Koa documentation at https://koajs.com confirms: `request.path` - "Get request pathname" and `request.querystring` - "Get raw query string void of ?" - these are separate properties, confirming `path` does not include query parameters.

@arnaud-moncel arnaud-moncel force-pushed the feat/wf-executor-proxy branch from c938768 to 8891bf0 Compare April 1, 2026 07:59
@arnaud-moncel arnaud-moncel changed the base branch from main to feat/prd-214-setup-workflow-executor-package April 1, 2026 08:00
@qltysh
Copy link
Copy Markdown

qltysh bot commented Apr 1, 2026

All good ✅

@arnaud-moncel arnaud-moncel force-pushed the feat/wf-executor-proxy branch from 8891bf0 to 1f28b96 Compare April 1, 2026 08:02
Comment on lines +40 to +41
const httpPort = Number(process.env.EXECUTOR_HTTP_PORT ?? '4001');
const pollingIntervalMs = Number(process.env.EXECUTOR_POLLING_INTERVAL_MS ?? '5000');
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low src/main.ts:40

Number(process.env.EXECUTOR_HTTP_PORT) and Number(process.env.EXECUTOR_POLLING_INTERVAL_MS) return NaN for invalid numeric strings (e.g., abc), silently propagating NaN to downstream code that expects a valid number. Unlike required env vars which throw on missing values, these misconfigurations fail silently and cause confusing downstream failures. Consider validating and throwing a clear error when the environment variable is set but not parsable as a number.

-  const httpPort = Number(process.env.EXECUTOR_HTTP_PORT ?? '4001');
-  const pollingIntervalMs = Number(process.env.EXECUTOR_POLLING_INTERVAL_MS ?? '5000');
+  const httpPort = parseIntEnv('EXECUTOR_HTTP_PORT', 4001);
+  const pollingIntervalMs = parseIntEnv('EXECUTOR_POLLING_INTERVAL_MS', 5000);
Also found in 1 other location(s)

packages/agent/src/routes/workflow/workflow-executor-proxy.ts:19

Calling .replace() on options.workflowExecutorUrl at line 19 will throw a TypeError if the value is null. The type AgentOptions.workflowExecutorUrl is string | null, and the test factory in code object 1 defaults it to null. If WorkflowExecutorProxyRoute is instantiated when workflowExecutorUrl is null, the constructor crashes with "Cannot read properties of null (reading 'replace')".

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/main.ts around lines 40-41:

`Number(process.env.EXECUTOR_HTTP_PORT)` and `Number(process.env.EXECUTOR_POLLING_INTERVAL_MS)` return `NaN` for invalid numeric strings (e.g., `abc`), silently propagating `NaN` to downstream code that expects a valid number. Unlike required env vars which throw on missing values, these misconfigurations fail silently and cause confusing downstream failures. Consider validating and throwing a clear error when the environment variable is set but not parsable as a number.

Evidence trail:
packages/workflow-executor/src/main.ts:40 - `const httpPort = Number(process.env.EXECUTOR_HTTP_PORT ?? '4001')` shows Number() parsing without validation
packages/workflow-executor/src/main.ts:41 - `const pollingIntervalMs = Number(process.env.EXECUTOR_POLLING_INTERVAL_MS ?? '5000')` same pattern
packages/workflow-executor/src/main.ts:70 - httpPort passed to Runner constructor
packages/workflow-executor/src/http/executor-http-server.ts:93 - `this.server.listen(this.options.port, resolve)` uses port directly
packages/workflow-executor/src/runner.ts:198 - `setTimeout(() => this.runPollCycle(), this.config.pollingIntervalMs)` uses interval directly
packages/workflow-executor/src/main.ts:24-30 - requireEnv() validates required vars but no equivalent for numeric validation

Also found in 1 other location(s):
- packages/agent/src/routes/workflow/workflow-executor-proxy.ts:19 -- Calling `.replace()` on `options.workflowExecutorUrl` at line 19 will throw a `TypeError` if the value is `null`. The type `AgentOptions.workflowExecutorUrl` is `string | null`, and the test factory in code object 1 defaults it to `null`. If `WorkflowExecutorProxyRoute` is instantiated when `workflowExecutorUrl` is `null`, the constructor crashes with "Cannot read properties of null (reading 'replace')".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant