feat(workflow-executor): approval requests on Full AI / Automatic trigger actions [PRD-688]

[Scra3]

Context

PRD-688 (backend / executor half, single PR per repo). In the legacy browser engine, triggering an approval-gated action created the approval on the frontend. With the backend migration, Automatic & Full AI Trigger Action steps run on the executor, which didn't create the approval — a regression. This PR makes the executor file the approval request (non-blocking) and handles action scope correctly.

What it does

1. Create the approval request when the triggering user's role requires it — instead of executing — and report the step as success (the run continues; the process owner adds an explicit stop if needed). The created approval id is forwarded to the front via the StepOutcome (context.approvalRequest.id) so it can deep-link.
2. Global actions carry no record. A global action runs on no record, so the executor no longer attaches its source record (the approval would otherwise show a nonsensical Resource). Single/bulk actions keep their record. Relies on the action type forwarded by the orchestrator (ForestAdmin/forestadmin-server#8344); degrades safely if absent.
3. Approval-creation failure → step error with a dedicated ApprovalRequestCreationError.

Key changes

• agent-client: makeCreateApprovalRequest returns the created approval id (defensive extraction); Action.execute() bubbles { approvalRequested, approvalRequest }.
• workflow-executor: wire forestServer (per-step forestServerToken threaded runner → factory → AgentWithLog → port); typed ExecuteActionResult; the trigger-action executor branches on approval (persists pending-approval, no actionResult), omits the record for global actions, reconstructs the approval id on idempotent replay; outcome + update-step mapper carry approvalRequest.

Architecture note

approvalRequest is contained to the trigger-action executor: a dedicated buildOutcomeResult override (mirroring condition's selectedOption), not the shared base/record builders. The action-scope decision stays in the executor and reduces to "id present or not" at the port boundary — lower layers stay generic.

Verification

agent-client (340) · mcp-server (547) · workflow-executor (1131) tests pass; build + lint clean.

Frontend half: ForestAdmin/forestadmin#9804. Orchestrator (forwards action type): ForestAdmin/forestadmin-server#8344.

fixes PRD-688

🤖 Generated with Claude Code

Note

Add approval request handling for Full AI and Automatic trigger actions

• When an action requires approval, TriggerRecordActionStepExecutor now persists a pending-approval result and returns a success outcome instead of blocking, optionally including the approval request id for deep-linking.
• AgentClientAgentPort.executeAction returns a normalized ExecuteActionResult union distinguishing executed results from approval-gated submissions, and wires Forest server connectivity when a forestServerToken is provided.
• Global actions (type 'global') are now supported: record id is omitted when building forms and executing actions.
• Idempotent replay of completed pending-approval steps restores the saved approvalRequest id without re-triggering execution.
• The forestServerToken is propagated from each dispatch through Runner → StepExecutorFactory → AgentWithLog → AgentClientAgentPort to enable approval request creation.

^{Macroscope summarized baa7964.}

[linear-code]

PRD-688

[qltysh]

10 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many returns (count = 4): mapActionExecutionError	4
qlty	Structure	Function with many parameters (count = 6): create	3
qlty	Structure	High total complexity (count = 55)	2
qlty	Structure	Function with high complexity (count = 12): executeAction	1

[qltysh]

Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.08%.

Modified Files with Diff Coverage (7)

Rating	File	% Diff	Uncovered Line #s
	packages/agent-client/src/approval-request-creator.ts	100.0%
	...-executor/src/executors/trigger-record-action-step-executor.ts	100.0%
	packages/agent-client/src/domains/action.ts	100.0%
	...ages/workflow-executor/src/adapters/agent-client-agent-port.ts	100.0%
	packages/workflow-executor/src/errors.ts	100.0%
	...ow-executor/src/adapters/step-outcome-to-update-step-mapper.ts	100.0%
	packages/workflow-executor/src/executors/agent-with-log.ts	100.0%
	Total	100.0%

🚦 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.

[Scra3]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[matthv]

Review notes — no correctness blockers, a few things worth hardening

🔴 Should fix before merge

Silent loss of the approval deep-link when the server returns no data.id — packages/agent-client/src/approval-request-creator.ts:46
The POST succeeded (the approval exists server-side), but if the response has no usable data.id, we silently return undefined. The run then reports success, persists pending-approval with no id, forwards no deep-link — and logs nothing. Later a user sees "pending-approval but no link to approve" and logs explain nothing.
The non-throwing fallback is the right call; the silence isn't. Please add a warn log (with collectionName, actionName, renderingId) when the POST succeeds yet data.id is absent. Same for the idempotent-replay coalesce at trigger-record-action-step-executor.ts:65-68.

🟡 Worth addressing

Global-vs-record misclassification is silent if type is absent — trigger-record-action-step-executor.ts:148
isGlobal: action.type === 'global', and type is .optional(). If the orchestrator (#8344) omits it, a global action is silently treated as record-scoped → a recordId gets wrongly attached to the approval. Fine only if the orchestrator contract guarantees type is always sent — worth confirming, since that guarantee lives cross-repo and isn't enforced here.

Test gap — checkIdempotency replay only covers pending-approval — trigger-record-action-step-executor.ts:63-74
The !('skipped' in result) guard and the executed-result replay (the common Full-AI happy path) have no test. Two cheap cases: phase=done + executed → success/no approvalRequest; phase=done + { skipped: true } → success.

Test gap — adapter-level global-action id omission — agent-client-agent-port.ts:249
getActionForm's new id?.length ? [id] : [] is only tested with id: [1], and those tests never assert the recordIds passed to mockCollection.action. Add a no-id case asserting { recordIds: [] } — that's the parity with executeAction the PR relies on.

[Scra3]

Thanks @matthv — addressed in 155d199.

🔴 Silent loss of the deep-link when no data.id — agreed, the fallback stays non-throwing but is no longer silent. Added a Warn (with collectionName, actionName, renderingId) on both paths: when the approval POST succeeds with no id (executeOnExecutor), and on the idempotent-replay coalesce (checkIdempotency). Kept the log in the executor rather than agent-client, which has no logger injected — the executor is where the context (collection/action/rendering + context.logger) lives.

🟡 Global-vs-record misclassification if type absent — confirmed the orchestrator contract guarantees it: workflow-orchestrator-service.ts:890 sends type: resolveActionType(apiMapAction) (never undefined — falls back to Bulk), and CollectionSchemaAction.type is a required ActionType (types.ts:426) in #8344. The .optional() here is only defensive for pre-#8344 orchestrators; with the current contract a global action can't be silently record-scoped. Left as-is.

🟡 Replay test gap — added phase=done + executed (→ success, no approvalRequest) and phase=done + { skipped: true } (→ success), both asserting no re-trigger.

🟡 Adapter global-form test gap — added a no-id getActionForm case asserting mockCollection.action is called with { recordIds: [] } (the parity with executeAction the PR relies on).