feat(mcp-server): create approval request on action trigger when required by role

[Scra3]

Context

Some custom actions are gated behind approval-by-role: a user whose role can't execute the action triggers it → an approval request is created → another user (with an approving role) reviews it on the front. Until now this guard existed only via the frontend — triggering the same action through the Forest MCP executed it directly, bypassing approval (compliance hole for refunds, exports, deletions…).

This PR makes an action triggered via MCP create an approval request instead of executing, and reports "approval requested" to the LLM.

fixes PRD-288

Approach — approval logic mutualized in agent-client

The approval handling lives in agent-client Action.execute() so every prod consumer (mcp-server, and later the workflow executor) gets it for free, with no duplicated catch+create per consumer:

• On a 403 CustomActionRequiresApprovalError (mapped to ActionRequiresApprovalError by the deterministic-action-steps base work), execute() calls an injected ApprovalRequestCreator and returns { approvalRequested: true }.
• The creator is data-injected via createRemoteAgentClient({ forestServer: { url, bearerToken } }) — no callback. Prod clients wire it; agent-testing omits it, so triggering an action in tests never creates an approval request (execute() rethrows as before). agent-testing is untouched.

Changes

• agent-client: new ApprovalRequestCreator (SaaS call); execute() approval branch; creator threaded RemoteAgentClient → Collection → Action; createRemoteAgentClient accepts forestServer.
• mcp-server: agent-caller wires forestServer; execute-action returns the "approval requested" message (caught inside the activity-log operation → log stays completed, never failed).

⚠️ Pending before merge

1. SaaS endpoint spec (path / payload / auth) for creating an approval request — used by the front today. ApprovalRequestCreator.create() carries a TODO(PRD-288) placeholder until confirmed.
2. Based on feature/workflow-deterministic-ai-steps (retarget to main once that merges).

Tests

• agent-client: creator wired → create() called with correct args + { approvalRequested: true }; no creator → rethrows, nothing created.
• mcp-server: approval path → LLM "approval requested" message, not an error.
• agent-client 330 ✓ · mcp-server 545 ✓ · lint 0 error.

🤖 Generated with Claude Code

Note

Create approval requests on action trigger when required by role in workflow executor

• TriggerRecordActionStepExecutor now supports dynamic action forms: Manual mode pauses for user input with no AI prefill; AI-assisted mode pre-fills and pauses for review; Full AI mode iteratively fills required fields and executes, falling back to review if fields remain empty.
• When the agent rejects execution with CustomActionRequiresApprovalError, the executor calls makeCreateApprovalRequest to POST to /api/action-approvals on the Forest server, returning { approvalRequested: true } instead of throwing.
• AgentHttpError replaces generic stringified errors throughout agent-client, carrying status, parsed body, and raw responseText for structured downstream handling.
• preRecordedArgs.selectedRecordStepId (stable BPMN step id) replaces the previous index-based selectedRecordStepIndex in both TriggerActionStepDefinitionSchema and LoadRelatedRecordStepDefinitionSchema.
• StepExecutionFormatters gains a formatTriggerAction path that distinguishes pending-approval from executed submissions and highlights AI prefill vs. user edits.
• Risk: TriggerActionStepDefinitionSchema no longer coerces invalid executionType values to AutomatedWithConfirmation; existing configs with unrecognized values will now fail validation.

Changes since #1708 opened

• Added ApprovalRequestCreationError class and wrapped approval request creation in Action.execute method with try/catch block to throw the new error type [106db88]
• Configured createRemoteAgentClient to accept and pass forestServer connection details including url, token, and renderingId to wire approval request creation [106db88]
• Added test coverage for ApprovalRequestCreationError throwing behavior and forestServer configuration wiring [106db88]
• Added inline comment in makeCreateApprovalRequest factory explaining why status attribute is set to null on creation [106db88]

^{Macroscope summarized 23b2b0a.}

[linear-code]

PRD-288

[qltysh]

2 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many parameters (count = 8): constructor	2

[qltysh]

Coverage Impact

⬇️ Merging this pull request will decrease total coverage on main by 0.07%.

Modified Files with Diff Coverage (10)

Rating	File	% Diff	Uncovered Line #s
	packages/mcp-server/src/tools/execute-action.ts	100.0%
	packages/mcp-server/src/http-client/index.ts	100.0%
	packages/mcp-server/src/utils/agent-caller.ts	100.0%
	packages/agent-client/src/domains/collection.ts	100.0%
	packages/mcp-server/src/http-client/mcp-http-client.ts	100.0%
	packages/agent-client/src/errors.ts	100.0%
	packages/agent-client/src/domains/action.ts	100.0%
	packages/agent-client/src/domains/remote-agent-client.ts	100.0%
	packages/agent-client/src/index.ts	100.0%
	packages/agent-client/src/approval-request-creator.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

Ran the standard passes plus a ponytail (over-engineering) pass and a skeptic pass. No blocking bugs.

Skeptic note (flagged, then dismissed). Multiple passes flagged recordIds: this.ids as possibly undefined for global actions, while record_ids is Joi.array().required() server-side. False positive: Collection.action() is the only production path and always builds ids as an array ([] at minimum), so record_ids: [] is sent and validates.

agent-nodejs/packages/agent-client/src/domains/action.ts

Lines 121 to 128 in 9f90e14

	await this.approvalRequestCreator.create({
	collectionName: this.collectionName,
	actionName: this.actionName,
	recordIds: this.ids,
	values: this.fieldsFormStates.getFieldValues(),
	});
	return { approvalRequested: true };

Non-blocking:

1. The Forest server URL is re-derived from process.env.FOREST_SERVER_URL here, duplicating cli.ts and bypassing a forestServerUrl configured via server options. For self-hosted hosts the approval POST could hit a different host than the activity-log calls.

agent-nodejs/packages/mcp-server/src/utils/agent-caller.ts

Lines 38 to 44 in 9f90e14

	const forestServer =
	forestServerToken && renderingId != null
	? {
	url: process.env.FOREST_SERVER_URL || 'https://api.forestadmin.com',
	forestServerToken,
	renderingId,
	}

2. approvalRequestCreator.create() is not wrapped, so a SaaS network/5xx failure escapes execute() as a raw error (outside the typed ActionRequiresApprovalError contract). Acceptable, but the failure path is undocumented.

agent-nodejs/packages/agent-client/src/domains/action.ts

Lines 120 to 131 in 9f90e14

	if (mapped instanceof ActionRequiresApprovalError && this.approvalRequestCreator) {
	await this.approvalRequestCreator.create({
	collectionName: this.collectionName,
	actionName: this.actionName,
	recordIds: this.ids,
	values: this.fieldsFormStates.getFieldValues(),
	});
	return { approvalRequested: true };
	}
	throw mapped;

3. Layering: a prior review on these files (#1403) set the convention "agent-client throws, mcp-server shapes the result". Here execute() swallows the approval error and returns { approvalRequested: true }, gated on whether a creator was wired. Deliberate (mutualizing for the future workflow-executor), but the dual contract for the same 403 is worth a short comment.

Ponytail: ApprovalRequestCreator is a class with a single method and a single caller; a plain createApprovalRequest(options, payload) function would do the same with less surface. Minor.

agent-nodejs/packages/agent-client/src/approval-request-creator.ts

Lines 13 to 23 in 9f90e14

	export default class ApprovalRequestCreator {
	constructor(
	private readonly options: {
	forestServerUrl: string;
	forestServerToken: string;
	renderingId: number | string;
	},
	) {}
	async create(payload: ApprovalRequestPayload): Promise<void> {
	await ServerUtils.queryWithBearerToken({

🤖 Generated with Claude Code

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

[hercemer42]

Claude Opus 4.8 — [Should fix] Approval inputs are built from getFields(), which returns all fields including ones whose value is undefined, whereas the executed path (line 103) uses getFieldValues() that drops undefined. Confirm the server's /api/action-approvals validation accepts entries for unfilled fields — otherwise an action with optional, unfilled fields could have its approval request rejected. Mirror getFieldValues() semantics if so.

[Scra3]

Not changing: the server validator (forestadmin-server #8327) declares value: Joi.any() and explicitly tests "allow a list field type and a missing value", so unfilled fields are accepted (and JSON.stringify omits value: undefined on the wire anyway). Keeping getFields() also preserves the {name, type} of unfilled fields for the approver.

[hercemer42]

Claude Opus 4.8 — [Preferential] If this POST fails, the error propagates and the LLM sees only the transport error — the fact that the action is approval-gated is lost, and "couldn't file the approval request" becomes indistinguishable from "the action failed". Consider wrapping to disambiguate the two failure modes.

[Scra3]

Fixed in 106db88: the approval POST is now wrapped in ApprovalRequestCreationError (carries the original cause), so an approval-gate filing failure is distinguishable from the action itself failing.

[hercemer42]

Claude Opus 4.8 — [Preferential] status: null is an unexplained literal. A one-line note on the server contract (e.g. the server assigns the pending status; null is required on create) would save a future reader the "is this needed?" question.

[Scra3]

Fixed in 106db88: added a comment on the create contract (the Forest server assigns the pending status itself; null is required on create).

[hercemer42]

Claude Opus 4.8 — [Violates conventions] This test ("build a client when forestServer is provided") asserts only toBeInstanceOf(RemoteAgentClient) — identical to the tests above that pass no forestServer, so it passes even if the forestServer → createApprovalRequest wiring is dropped entirely. CLAUDE.md: test name = assertion / avoid weak assertions. Assert the wiring is observable (e.g. spy makeCreateApprovalRequest and verify it's called with the forestServer fields). Semantic — not linter-catchable.

[Scra3]

Fixed in 106db88: the test now mocks makeCreateApprovalRequest and asserts it is called with { forestServerUrl, forestServerToken, renderingId } when forestServer is provided, plus a negative test asserting it is not called when omitted.

[hercemer42]

Claude Opus 4.8 — [Violates conventions] Asserts only that rpcClient is defined — the same as the tests without forest-server fields — so it doesn't verify forestServer was actually passed, and there is no negative test for the guard (forestServerUrl && forestServerToken && renderingId != null). Mock createRemoteAgentClient and assert it's called with forestServer: { serverUrl, serverToken, renderingId } when complete, and undefined when a field is missing. (CLAUDE.md weak-assertion rule; semantic, not linter-catchable.)

[Scra3]

Fixed in 106db88: now asserts createRemoteAgentClient is called with forestServer: { serverUrl, serverToken, renderingId } when complete, and a negative test asserting forestServer: undefined when forestServerToken is missing (the guard's silent-disable branch).

[hercemer42]

Claude Opus 4.8 — [Preferential] isError is never set by either branch of the handler, and withActivityLog is mocked in this suite, so this assertion (and the "activity log is not marked failed" comment) is tautological — it cannot fail. The stringContaining('Approval requested') assertion above is the meaningful one. Drop this line, or assert the activity-log status against the real withActivityLog if the intent is to prove the log stays completed.

[Scra3]

Fixed in 106db88: dropped the tautological isError assertion and its misleading comment; the stringContaining('Approval requested') assertion remains.