fix(workflow): set durations to seconds

[EnkiP]

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 @forestadmin/workflow-executor package with HTTP server, step executors, and CLI

• Introduces a new @forestadmin/workflow-executor package that polls Forest server for available workflow runs, executes steps (condition, read-record, update-record, trigger-action, load-related-record, mcp, guidance) via LLM-assisted or manual flows, and persists state via a Sequelize-backed or in-memory RunStore.
• Adds a BaseStepExecutor with idempotency short-circuiting, per-step timeout, and uniform error-to-outcome mapping; concrete executors extend this for each step type.
• Exposes an authenticated Koa HTTP server (ExecutorHttpServer) with /health, GET /runs/:runId, and POST /runs/:runId/trigger endpoints; JWT is read from Authorization header or forest_session_token cookie.
• Adds a WorkflowExecutorProxyRoute to the agent that forwards /_internal/workflow-executions/* requests to the executor when workflowExecutorUrl is configured.
• Adds an AiClient to @forestadmin/ai-proxy with model caching, remote tool lifecycle management, and mcpServerId propagation through all RemoteTool derivatives.
• Fixes QuerySerializer.formatFields in agent-client to emit comma-separated fields[collection]=a,b instead of repeated query parameters.
• Risk: QuerySerializer field serialization change is a breaking wire-format change for any agent-client consumers relying on repeated fields[] parameters.

📊 Macroscope summarized f3b3102. 54 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted

🗂️ Filtered Issues

No issues evaluated.

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (69)

Rating	File	% Diff	Uncovered Line #s
	packages/agent/src/routes/workflow/workflow-executor-proxy.ts	97.4%	80
	packages/ai-proxy/src/integrations/snowflake/tools.ts	100.0%
	packages/ai-proxy/src/get-ai-configuration.ts	100.0%
	packages/ai-proxy/src/remote-tool.ts	100.0%
	packages/forestadmin-client/src/index.ts	100.0%
	packages/ai-proxy/src/validate-ai-configurations.ts	100.0%
	packages/agent-client/src/domains/collection.ts	0.0%	175
	packages/ai-proxy/src/integrations/zendesk/tools.ts	100.0%
	packages/ai-proxy/src/forest-integration-client.ts	100.0%
	packages/ai-proxy/src/mcp-client.ts	100.0%
	packages/ai-proxy/src/create-base-chat-model.ts	100.0%
	packages/ai-proxy/src/ai-client.ts	100.0%
	packages/ai-proxy/src/integrations/kolar/tools.ts	100.0%
	packages/agent/src/utils/options-validator.ts	100.0%
	packages/agent/src/routes/index.ts	66.7%	179
	packages/agent-client/src/query-serializer.ts	100.0%
	packages/ai-proxy/src/index.ts	100.0%
	...r/src/adapters/forestadmin-client-activity-log-port-factory.ts	100.0%
	packages/workflow-executor/src/adapters/console-logger.ts	100.0%
	packages/workflow-executor/src/cli.ts	0.0%	4-14
	packages/workflow-executor/src/executors/step-executor-factory.ts	100.0%
	...ow-executor/src/executors/load-related-record-step-executor.ts	99.0%	226, 377
	...orkflow-executor/src/executors/summary/step-summary-builder.ts	100.0%
	packages/workflow-executor/src/build-workflow-executor.ts	100.0%
	packages/workflow-executor/src/adapters/pretty-logger.ts	100.0%
	packages/workflow-executor/src/adapters/server-types.ts	100.0%
	...-executor/src/adapters/forestadmin-client-activity-log-port.ts	100.0%
	...s/workflow-executor/src/executors/read-record-step-executor.ts	100.0%
	packages/workflow-executor/src/executors/record-step-executor.ts	100.0%
	...ges/workflow-executor/src/executors/condition-step-executor.ts	100.0%
	packages/workflow-executor/src/http/pending-data-validators.ts	100.0%
	packages/workflow-executor/src/adapters/activity-log-drainer.ts	100.0%
	packages/workflow-executor/src/executors/base-step-executor.ts	100.0%
	packages/workflow-executor/src/http/executor-http-server.ts	99.0%	121
	.../workflow-executor/src/adapters/forest-server-workflow-port.ts	100.0%
	packages/workflow-executor/src/adapters/record-id-serializer.ts	100.0%
	packages/workflow-executor/src/adapters/server-ai-adapter.ts	80.0%	26-27, 70-71
	packages/workflow-executor/src/in-flight-run-registry.ts	100.0%
	...workflow-executor/src/executors/update-record-step-executor.ts	98.9%	94
	packages/workflow-executor/src/executors/activity-log.ts	100.0%
	packages/workflow-executor/src/adapters/ai-client-adapter.ts	73.3%	20-21, 37-38
	...-executor/src/executors/trigger-record-action-step-executor.ts	100.0%
	packages/workflow-executor/src/adapters/with-retry.ts	94.1%	45
	...ages/workflow-executor/src/adapters/agent-client-agent-port.ts	100.0%
	...workflow-executor/src/adapters/run-to-available-step-mapper.ts	100.0%
	packages/workflow-executor/src/errors.ts	99.2%	267
	packages/workflow-executor/src/executors/mcp-step-executor.ts	98.4%	181
	...ow-executor/src/adapters/step-outcome-to-update-step-mapper.ts	100.0%
	...s/workflow-executor/src/adapters/always-error-ai-model-port.ts	100.0%
	packages/workflow-executor/src/defaults.ts	100.0%
	...ow-executor/src/executors/summary/step-execution-formatters.ts	100.0%
	packages/workflow-executor/src/adapters/step-definition-mapper.ts	100.0%
	...ages/workflow-executor/src/executors/guidance-step-executor.ts	86.7%	20-21
	packages/workflow-executor/src/executors/agent-with-log.ts	100.0%
	packages/workflow-executor/src/cli-core.ts	100.0%
	packages/workflow-executor/src/http/step-serializer.ts	100.0%
	packages/workflow-executor/src/types/validated/execution.ts	100.0%
	packages/workflow-executor/src/runner.ts	100.0%
	packages/workflow-executor/src/remote-tool-fetcher.ts	100.0%
	packages/workflow-executor/src/stores/in-memory-store.ts	100.0%
	packages/workflow-executor/src/stores/build-run-store.ts	100.0%
	packages/workflow-executor/src/schema-cache.ts	100.0%
	packages/workflow-executor/src/types/validated/step-outcome.ts	100.0%
	packages/workflow-executor/src/index.ts	100.0%
	packages/workflow-executor/src/types/validated/step-definition.ts	100.0%
	packages/workflow-executor/src/stores/database-store.ts	97.1%	71
	packages/workflow-executor/src/types/validated/collection.ts	100.0%
	packages/workflow-executor/src/validate-secrets.ts	100.0%
	packages/workflow-executor/src/schema-resolver.ts	100.0%
	Total	98.6%

🤖 Increase coverage with AI coding...

In the `fix/set-duration-to-seconds` branch, add test coverage for this new code:

- `packages/agent-client/src/domains/collection.ts` -- Line 175
- `packages/agent/src/routes/index.ts` -- Line 179
- `packages/agent/src/routes/workflow/workflow-executor-proxy.ts` -- Line 80
- `packages/workflow-executor/src/adapters/ai-client-adapter.ts` -- Lines 20-21 and 37-38
- `packages/workflow-executor/src/adapters/server-ai-adapter.ts` -- Lines 26-27 and 70-71
- `packages/workflow-executor/src/adapters/with-retry.ts` -- Line 45
- `packages/workflow-executor/src/cli.ts` -- Line 4-14
- `packages/workflow-executor/src/errors.ts` -- Line 267
- `packages/workflow-executor/src/executors/guidance-step-executor.ts` -- Line 20-21
- `packages/workflow-executor/src/executors/load-related-record-step-executor.ts` -- Lines 226 and 377
- `packages/workflow-executor/src/executors/mcp-step-executor.ts` -- Line 181
- `packages/workflow-executor/src/executors/update-record-step-executor.ts` -- Line 94
- `packages/workflow-executor/src/http/executor-http-server.ts` -- Line 121
- `packages/workflow-executor/src/stores/database-store.ts` -- Line 71

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

[EnkiP]

wrong branch, new PR: #1646

[macroscopeapp]

🟡 Medium src/ai-client.ts:41

If Promise.all(providers.map(p => p.loadTools())) throws, this.toolProviders is never updated with the new providers array. Created ToolProvider instances will leak since closeConnections() and subsequent loadRemoteTools() calls have no reference to them. Consider assigning this.toolProviders = providers before awaiting the promises, or using try/finally to ensure disposal if loading fails.

  async loadRemoteTools(configs: Record<string, ToolConfig>): Promise<RemoteTool[]> {
     await this.disposeToolProviders('Error closing previous remote tool connection');
 
     const providers = createToolProviders(configs, this.logger);
-    const toolsByProvider = await Promise.all(providers.map(p => p.loadTools()));
     this.toolProviders = providers;
+    const toolsByProvider = await Promise.all(providers.map(p => p.loadTools()));
 
-    return toolsByProvider.flat();
+    return toolsByProvider.flat();
   }

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/ai-proxy/src/ai-client.ts around lines 41-49:

If `Promise.all(providers.map(p => p.loadTools()))` throws, `this.toolProviders` is never updated with the new providers array. Created `ToolProvider` instances will leak since `closeConnections()` and subsequent `loadRemoteTools()` calls have no reference to them. Consider assigning `this.toolProviders = providers` before awaiting the promises, or using `try/finally` to ensure disposal if loading fails.

Evidence trail:
packages/ai-proxy/src/ai-client.ts lines 41-49: `loadRemoteTools` creates providers (line 44), awaits loadTools (line 45), and only assigns to `this.toolProviders` on line 46 after success. Lines 55-70: `disposeToolProviders` sets `this.toolProviders = []` (line 59) and calls `dispose()` on providers (line 61), confirming these are disposable resources. On line 45 rejection, providers created on line 44 become unreachable and are never disposed.

[macroscopeapp]

🟡 Medium executors/trigger-record-action-step-executor.ts:167

saveFrontendResult does not set idempotencyPhase: 'done' when saving the execution result, unlike executeOnExecutor which does (line 161). This causes checkIdempotency to return null instead of recognizing the step as complete on re-entry, potentially triggering incorrect re-processing of the confirmation flow.

  private async saveFrontendResult(
    target: ActionTarget,
    actionResult: unknown,
    existingExecution: TriggerRecordActionStepExecutionData,
  ): Promise<StepExecutionResult> {
    const { selectedRecordRef, displayName, name } = target;

    await this.context.runStore.saveStepExecution(this.context.runId, {
      ...existingExecution,
      type: 'trigger-action',
      stepIndex: this.context.stepIndex,
      executionParams: { displayName, name },
      executionResult: { success: true, actionResult },
      selectedRecordRef,
+      idempotencyPhase: 'done',
    });

    return this.buildOutcomeResult({ status: 'success' });
  }

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts around lines 167-185:

`saveFrontendResult` does not set `idempotencyPhase: 'done'` when saving the execution result, unlike `executeOnExecutor` which does (line 161). This causes `checkIdempotency` to return `null` instead of recognizing the step as complete on re-entry, potentially triggering incorrect re-processing of the confirmation flow.

Evidence trail:
- packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts lines 174-181: `saveFrontendResult` saves without `idempotencyPhase: 'done'`
- packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts line 160: `executeOnExecutor` saves with `idempotencyPhase: 'done'`
- packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts lines 31-45: `checkIdempotency` checks for `idempotencyPhase === 'done'` and returns null otherwise
- packages/workflow-executor/src/executors/trigger-record-action-step-executor.ts lines 123-128: Branch C saves initial pending data without `idempotencyPhase`, so `existingExecution` spread won't provide it
- packages/workflow-executor/src/executors/update-record-step-executor.ts line 280: consistent pattern uses `idempotencyPhase: 'done'`
- packages/workflow-executor/src/executors/mcp-step-executor.ts line 152: consistent pattern uses `idempotencyPhase: 'done'`

[macroscopeapp]

🟢 Low adapters/run-to-available-step-mapper.ts:64

The guidance branch in toStepOutcome ignores 'awaiting-input' status and returns 'success' instead. When ctx.status is 'awaiting-input', this causes the orchestrator to treat a paused guidance step as completed. Consider using toRecordStatus(ctx.status) to match the other branches.

  if (outcomeType === 'guidance') {
-    const status: GuidanceStepOutcome['status'] = ctx.status === 'error' ? 'error' : 'success';
+    const status: GuidanceStepOutcome['status'] = toRecordStatus(ctx.status);

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts around lines 64-66:

The `guidance` branch in `toStepOutcome` ignores `'awaiting-input'` status and returns `'success'` instead. When `ctx.status` is `'awaiting-input'`, this causes the orchestrator to treat a paused guidance step as completed. Consider using `toRecordStatus(ctx.status)` to match the other branches.

Evidence trail:
packages/workflow-executor/src/adapters/run-to-available-step-mapper.ts:31-36 (`toRecordStatus` handles 'awaiting-input'), :64-68 (guidance branch only checks for 'error', maps everything else to 'success'), :52-61 (condition branch uses `toRecordStatus`), :70-76 (mcp and record branches use `toRecordStatus`). packages/workflow-executor/src/types/validated/step-outcome.ts:9 (`RecordStepStatusSchema` = `['success', 'error', 'awaiting-input']`), :55-62 (`GuidanceStepOutcomeSchema` uses `RecordStepStatusSchema` for status).

[macroscopeapp]

🟢 Low adapters/console-logger.ts:4

The spread order { message, timestamp, ...context } allows properties in context to overwrite message and timestamp. If a caller passes context with a message or timestamp key, the log entry will show the caller's values instead of the actual message and timestamp. Change the spread to { ...context, message, timestamp } so the logger's fields take precedence.

-    console.error(JSON.stringify({ message, timestamp: new Date().toISOString(), ...context }));
+    console.error(JSON.stringify({ ...context, message, timestamp: new Date().toISOString() }));

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/adapters/console-logger.ts around lines 4-14:

The spread order `{ message, timestamp, ...context }` allows properties in `context` to overwrite `message` and `timestamp`. If a caller passes `context` with a `message` or `timestamp` key, the log entry will show the caller's values instead of the actual message and timestamp. Change the spread to `{ ...context, message, timestamp }` so the logger's fields take precedence.

Evidence trail:
packages/workflow-executor/src/adapters/console-logger.ts lines 5, 9, 13 at REVIEWED_COMMIT — all three methods use `{ message, timestamp: ..., ...context }` with `...context` last, allowing context properties to overwrite `message` and `timestamp`.

[macroscopeapp]

src/build-workflow-executor.ts:188

After stop() is called, shutdownPromise retains its resolved value. If start() is called again on the same instance, a subsequent SIGTERM/SIGINT will see shutdownPromise is still truthy, skip calling shutdown(), and exit with code 0 without actually stopping the server or runner. Consider resetting shutdownPromise to null after stop() completes so the next signal triggers a fresh shutdown.

     async stop() {
       process.removeListener('SIGTERM', onSignal);
       process.removeListener('SIGINT', onSignal);
 
       if (!shutdownPromise) shutdownPromise = shutdown();
       await shutdownPromise;
+      shutdownPromise = null;
     },

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/build-workflow-executor.ts around lines 188-195:

After `stop()` is called, `shutdownPromise` retains its resolved value. If `start()` is called again on the same instance, a subsequent SIGTERM/SIGINT will see `shutdownPromise` is still truthy, skip calling `shutdown()`, and exit with code 0 without actually stopping the server or runner. Consider resetting `shutdownPromise` to `null` after `stop()` completes so the next signal triggers a fresh shutdown.

Evidence trail:
packages/workflow-executor/src/build-workflow-executor.ts lines 134-196 at REVIEWED_COMMIT. Key lines: 139 (`shutdownPromise` init to null), 157 (guard `if (!shutdownPromise)`), 180-185 (`start()` re-registers signal handlers without resetting `shutdownPromise`), 188-194 (`stop()` sets `shutdownPromise` but never resets to null).

[macroscopeapp]

🟠 High src/build-workflow-executor.ts:124

pollingIntervalS uses ?? instead of positiveOrDefault, so passing pollingIntervalS: 0 creates a 0-second polling interval that hammers the server instead of falling back to DEFAULT_POLLING_INTERVAL_S. This contradicts the comment at lines 60-61 which states that bad timeout configs must fall back to defaults. Consider using positiveOrDefault for pollingIntervalS to match stepTimeoutS, aiInvokeTimeoutS, and schemaCacheTtlS.

-    pollingIntervalS: options.pollingIntervalS ?? DEFAULT_POLLING_INTERVAL_S,
+    pollingIntervalS: positiveOrDefault(options.pollingIntervalS, DEFAULT_POLLING_INTERVAL_S),

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @packages/workflow-executor/src/build-workflow-executor.ts around line 124:

`pollingIntervalS` uses `??` instead of `positiveOrDefault`, so passing `pollingIntervalS: 0` creates a 0-second polling interval that hammers the server instead of falling back to `DEFAULT_POLLING_INTERVAL_S`. This contradicts the comment at lines 60-61 which states that bad timeout configs must fall back to defaults. Consider using `positiveOrDefault` for `pollingIntervalS` to match `stepTimeoutS`, `aiInvokeTimeoutS`, and `schemaCacheTtlS`.

Evidence trail:
packages/workflow-executor/src/build-workflow-executor.ts lines 60-64 (comment + positiveOrDefault helper), line 99 (schemaCacheTtlS uses positiveOrDefault), line 124 (pollingIntervalS uses ?? instead), line 128 (stepTimeoutS uses positiveOrDefault), line 129 (aiInvokeTimeoutS uses positiveOrDefault) at REVIEWED_COMMIT.

[macroscopeapp]

🟢 Low workflow/workflow-executor-proxy.ts:33

runId from route parameters is interpolated directly into executorRelativeUrl without validation. When runId contains path traversal sequences like ../, new URL(executorRelativeUrl, this.executorUrl) normalizes the path, allowing requests to arbitrary endpoints on the executor. For example, runId='../admin' resolves /runs/../admin to /admin on the target server. Consider validating that runId matches a safe pattern (e.g., UUID or alphanumeric) before constructing the target URL.

-    const { runId } = context.params;
+    const { runId } = context.params;
+
+    // Validate runId to prevent path traversal
+    if (!/^[a-zA-Z0-9_-]+$/.test(runId)) {
+      context.throw(400, 'Invalid runId format');
+      return;
+    }
+

🚀 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 33-34:

`runId` from route parameters is interpolated directly into `executorRelativeUrl` without validation. When `runId` contains path traversal sequences like `../`, `new URL(executorRelativeUrl, this.executorUrl)` normalizes the path, allowing requests to arbitrary endpoints on the executor. For example, `runId='../admin'` resolves `/runs/../admin` to `/admin` on the target server. Consider validating that `runId` matches a safe pattern (e.g., UUID or alphanumeric) before constructing the target URL.

Evidence trail:
packages/agent/src/routes/workflow/workflow-executor-proxy.ts lines 33, 36-37 at REVIEWED_COMMIT: runId extracted from context.params, interpolated into URL path, passed to new URL() which normalizes '..' segments. No validation exists in this file or BaseRoute (packages/agent/src/routes/base-route.ts). Route type is PrivateRoute (line 18) but auth headers forwarded to target (lines 66-67). @koa/router `:runId` matches [^/]+ so '..' (no slashes) is a valid match, and %2F-encoded slashes allow deeper traversal via decodeURIComponent in path-to-regexp.