feat(mcp-server): expose workflow tools in Forest MCP server (PRD-49)

[christophebrun-forest]

Summary

Integration branch for the PRD-49 epic — expose Forest workflow triggering to MCP clients. Adds a report-only (v1) toolset so an LLM can list, trigger, and observe Forest workflows through the MCP server.

The MCP tools call into @forestadmin/forestadmin-client (WorkflowsService → ForestHttpApi), which hits the Forest orchestrator (/api/workflow-orchestrator/mcp-workflows/*) under the MCP session identity (forestServerToken, Forest-Application-Source: MCP).

Included work

• PRD-736 — listWorkflows tool (feat(mcp-server): add listWorkflows tool (PRD-736) #1771)
• PRD-738 — triggerWorkflow tool (feat(mcp-server): add triggerWorkflow tool (PRD-738) #1777)
• PRD-740 — getWorkflowRun tool, returns the full hydrated run (feat(mcp-server): add getWorkflowRun tool (PRD-740) #1785)
• PRD-831 — trigger by id instead of listing all workflows (perf(mcp-server): trigger workflow by id instead of listing all workflows (PRD-831) #1805)
• PRD-832 — workflow-executor accepts triggerType='mcp' in the run mapper (fix(workflow-executor): accept triggerType='mcp' in run mapper (PRD-832) #1786)
• PRD-49 — triggerWorkflow audit is now fail-closed: it resolves the workflow via an O(1) by-id lookup and writes the pending activity log (labelled triggered the workflow "…" via MCP, attached to the collection) before the run starts, like create/update/delete.

Server dependency: the fail-closed audit relies on the by-id endpoint GET /api/workflow-orchestrator/mcp-workflows/:workflowId (forestadmin-server, PRD-49). Deploy the server side first.

Behavior (v1, report-only)

1. listWorkflows → available MCP-enabled workflows
2. triggerWorkflow → { runId, runState }. The workflow is resolved by id first (O(1)); an unknown or MCP-disabled workflow is rejected without starting a run, and the audit log is written before the trigger (fail-closed — a run with side effects is never started without an audit trail). The record itself is not validated at trigger time.
3. getWorkflowRun → the full hydrated run: runState plus the complete workflowHistory — every step with its resolved definition (type, title, prompt, task type, outgoing branches) and its per-step context (completion, selected option, error, escalation state, awaiting-input reason).

A run parked on a human-gated step is not resumable via MCP in v1 and must be finished from the Forest UI. It is recognised by runState: started with no context.error on the last history entry — which covers two shapes: a step still done: false awaiting an answer, and one already done: true waiting for someone to confirm before the run advances. Do not key on done: false alone. Resuming via MCP is handled in the follow-up PRD-441 (submitWorkflowInput).

Tests

New/updated unit tests across mcp-server, forestadmin-client, workflow-executor, plus cross-package mocks in agent-testing and agent. The triggerWorkflow suite covers the fail-closed ordering (log before trigger), rejection of unknown/MCP-disabled workflows, and failed marking on a trigger-time 404/409.

Rollout & release notes

• Deploy order — the sequence is constrained, and it spans three repos:
  1. Forest Runtime / workflow executors to the PRD-832 release — an older executor rejects triggerType='mcp' at validation. It does pick the run up: the mapper's DomainValidationError is classified as a malformed run and reportMalformedRun posts an error outcome, so the run is marked failed with a zod validation message on its first step and routed to the fallback inbox. That reporting path predates the executor's MCP support, so every affected runtime has it. Net effect: every MCP trigger in the environment fails loudly and getWorkflowRun shows the error — it does not sit pending forever, as an earlier version of this section claimed. No server-side version gate catches it and the message reaching the assistant is opaque, so the upgrade still has to come first. (oauth2 MCP steps are separately gated on executor ≥ 1.14.0; a different axis from the trigger-type enum.)
  2. forestadmin-server (PRD-49) — provides WorkflowTriggerType.Mcp (and therefore the layout validator the frontend needs), the four mcp-workflows routes, and the by-id lookup this PR's fail-closed audit depends on.
  3. This PR and the frontend (ForestAdmin/forestadmin#9870), in either order.
• Rollback is the case worth rehearsing. Reverting step 2 while step 3 stays deployed makes the by-id lookup 404 on every trigger, while listWorkflows keeps returning the same ids. The tool now logs an Error server-side on any lookup failure, and its message tells the caller not to retry an id listWorkflows just returned — without that, the assistant lists, triggers, is told to list again, and loops with nothing in the agent logs.
• New default-on MCP tools: listWorkflows, triggerWorkflow, getWorkflowRun. Integrations that pin a subset via enabledTools are unaffected; others gain them on upgrade. triggerWorkflow is side-effectful but inert until an admin enables the mcp trigger on a workflow — the server rejects a trigger on a non-opted-in workflow (WorkflowMcpTriggerNotEnabledError). It also declares MCP annotations (readOnlyHint: false, destructiveHint: true, idempotentHint: false, openWorldHint: true) so clients can tell it apart from the read-only tools.
• Audit semantics: the triggerWorkflow activity log records the trigger call (pending→completed = trigger accepted); the run continues asynchronously — its terminal state is read via getWorkflowRun, not the log. The fail policy is gated by action type and by cause: write actions fail closed (no audit → operation blocked), read actions fail open (the read proceeds with a warning and no status tracking) so an audit-store outage never takes down the read surface, and an authorization refusal (401/403) propagates either way — a refused identity is not an outage. It covers both ways the log can fail to exist: the route rejecting the write (5xx, timeout, or the 400/404 it returns for a missing or unresolvable collection — the likely modes) and the route answering 200 with a null log id (audit store write dropped). The arbitration lives in createPendingActivityLog, next to the null-id guard, so the policy is decided in one place, and it is pinned in both directions on both paths.
• Two audit rows per MCP trigger, worded differently on purpose: the MCP server writes requested the workflow "X" via MCP before the start — fail-closed, and with no runId since the run does not exist yet — and the orchestrator writes triggered the workflow "X" via MCP once the run is committed, carrying its run id. Only the first is guaranteed; the orchestrator's is best-effort, so a successful trigger leaves two rows or one. An earlier round aligned the two labels, which made a single trigger read as two identical events and left the count answerable only by deduplicating on the run id — hence the split wording. triggered keeps its parity with the webhook channel.
• WorkflowRunTriggerResult slimmed: the never-consumed workflowName/collectionName fields are dropped — the contract is exactly { runId, runState } (the audit label is resolved via getMcpWorkflowById).
• Public API of @forestadmin/forestadmin-client: the ForestAdminClient interface gains a required readonly workflowsService member — a compile-time breaking addition for external implementations of the interface. The ForestAdminClientWithCache constructor is a strict append (workflowsService is now the last parameter), so positional construction with the previous signature keeps working.

Changes from the third adversarial review

• Transport failures are classified before a model sees them, on all four workflow calls. The previous round sanitized the pre-flight lookup and left the other three untouched, so listWorkflows, getWorkflowRun and the start call still handed the model the raw error — ServerUtils interpolates the full Forest server URL into its timeout message and rethrows the raw Node error otherwise. A test was pinning that pass-through. The rule is now shared: anything that is not an HttpError arrived as a raw Node/superagent error, and a 408 is the one HttpError whose message is built client-side; everything else carries Forest's own JSON:API detail and still reaches the model, because it says something actionable.
• A terminal refusal no longer tells the model to retry. Only NotFoundError was treated as terminal, so a 400 on a non-UUID workflowId — the shape a model produces when it guesses a workflow name — came back as "temporary, retry later" and looped forever. Terminal now means any 4xx that is not a timeout or a rate limit. The 404 keeps its uniform wording so unknown / MCP-disabled / out-of-rendering stay indistinguishable; the others quote Forest's reason, which is safe by construction since that branch is only reachable for an HttpError. The trigger's own failure deliberately does not advise a retry: the call is not idempotent and the write may have landed before the transport broke.
• The by-id lookup projects its response, like the other three routes. It was the last one returning the raw body while the package notes claimed the projection held for the whole family. Its payload does not reach a model, but name is written verbatim into a persisted Activity Log label.
• A comment that described a mechanism that does not exist is corrected: skipping the status update was justified by "an empty Bearer would 401 and then be retried on the 404 branch". That branch tests instanceof NotFoundError, and a 401 maps to a plain HttpError.

Changes from the second adversarial review

• The read fail-open no longer swallows an authorization refusal. The policy arbitrated on the action type alone, so a 401/403 from the audit route became a warning and the read proceeded. A refused identity is not an audit-store outage; both now propagate. The rejection's cause is logged too — every fail-open read used to emit the same fixed sentence, leaving an operator unable to tell a validation refusal from a transient outage.
• listWorkflows projects its response, like the other two MCP routes. It was the only one returning the payload as-is, and it is the one whose result is stringified straight into a model's context. Per-step context is projected as well: it is a closed interface client-side but an open bag server-side, so the type promised a fence the code did not build.
• A non-404 lookup failure no longer hands the model transport detail (the Forest server URL, an internal host:port). It gets a message that distinguishes "Forest is unreachable, retry later" from "this id is not triggerable, do not retry"; the full error stays in the operator log.
• tools/list is asserted. Registration is the one of the three coordinated server.ts edits that nothing type-checks, so a rebase dropping it would have left a tool advertised, never registered, and the suite green. The annotations are asserted over the wire with it.
• The server drops a duplicate query on the MCP start path. Folding the mcp-enabled predicate into getWorkflowMetadata removed the second predicate but not the second query — createAndStartRun then called getBpmnAwsS3Identifier, the same method under another name. Passing the identifier already in hand makes "one predicate, one query, one round trip" true and closes the republish window between the gate and the bpmn read.
• What the MCP run must not expose is now pinned (not.toHaveProperty('userProfile' | 'serverToken')). Every contract assertion used objectContaining, and WorkflowRunForExecutor extends HydratedWorkflowRun, so swapping the builder compiled and kept the suite green while leaking a live Forest serverToken.
• The three trigger rows require workflow-manage permission in the UI. Enabling an automated trigger is what makes a workflow startable by an unattended caller, so it takes the same level as managing the workflow — not the broader layout permission the PATCH is classified under server-side. That server half is PRD-981: an Editor can still flip it through the API.
• Trigger saves are serialized per workflow, across channels. Each row had its own in-flight flag, and the PATCH replaces the whole triggers array, so an earlier request landing last could leave the server holding a channel the UI shows as off.
• Docs: the error table's promised split landed, the unaudited-tool list went from three to four (requestActionFileUpload is not a read — it mints a pre-authorized upload, and it is on by default), the OAuth2 row is version-scoped rather than "not yet supported", the two audit rows are no longer presented as equally reliable, and the 200-workflow cap is documented.

Changes from the adversarial review

• Audit fail policy — the read-fail-open path only covered the 200-with-null-id case; a rejection propagated and failed the tool, reads included. A test pinned the contradictory behaviour on a read action. Arbitration moved into createPendingActivityLog.
• listWorkflows no longer lists what it cannot trigger — a workflow whose collection was renamed or removed came back with a null collectionName, which triggerWorkflow rejects up front, so the assistant looped. They are now filtered out, with a warning for the operator.
• Forest-Application-Source: MCP is stamped on the MCP-only routes rather than taken from the caller's service options. The embedded mountAiMcpServer path built its services from the shared client options, which carry no headers, so agent-hosted MCP traffic reached the server unlabelled.
• getMcpWorkflowRun projects its response instead of casting it. The tool stringifies the run straight into a model's context, and the orchestrator builds a second shape of the same run carrying a userProfile with a live Forest serverToken. The MCP route uses a different builder, but the two types are mutually assignable — the whitelist is the guardrail, not the annotation.
• recordId is bounded at 255, matching the server column, so an over-long id no longer writes a pending audit row before being rejected.

fixes PRD-49

🤖 Generated with Claude Code

Note

Expose workflow tools (listWorkflows, triggerWorkflow, getWorkflowRun) in the Forest MCP server

• Adds three new MCP tools to server.ts: listWorkflows (list MCP-enabled workflows, optionally filtered by collection), triggerWorkflow (start a workflow run on a record with preflight validation and audit logging), and getWorkflowRun (fetch hydrated run status and history by runId).
• Wires workflow HTTP calls through a new WorkflowsService in forestadmin-client, which delegates to four new ForestHttpApi endpoints under /api/workflow-orchestrator/mcp-workflows.
• Extends ForestServerClientImpl and createForestServerClient to accept and expose WorkflowsService, and propagates the instance through ForestAdminClientWithCache and the Agent mount path.
• Hardens activity log creation in createPendingActivityLog: write actions (including the new triggerWorkflow) fail closed if log creation fails or returns no id; read actions fail open with a warning and proceed without an audit trail.
• Adds a TriggerType.Mcp value to the workflow executor's validated execution types and server adapter enum.
• Risk: triggerWorkflow is fail-closed on audit log creation — if the activity log service is unavailable, the workflow will not be triggered.

Changes since #1792 opened

• Added response field projection to ForestHttpApi.getMcpWorkflowRun method [2be4504]
• Added workflow filtering by collectionName in listWorkflows tool [2be4504]
• Added Forest-Application-Source: MCP header to all MCP-related API methods [2be4504]
• Added maximum length validation for recordId parameter in triggerWorkflow tool [2be4504]
• Updated documentation comments for McpWorkflowLookup type [2be4504]
• Fixed updateActivityLogStatus function to validate forestServerToken presence before attempting activity log updates [a3b04ef]
• Added explicit MCP specification annotations to workflow tool registrations [a3b04ef]
• Scoped workflow list tool helper types and functions to module-internal visibility [a3b04ef]
• Enhanced test coverage for workflow tool MCP annotations and activity log tracking [a3b04ef]
• Hardened constructor wiring test for ForestAdminClientWithCache to validate all positional arguments [a3b04ef]
• Added documentation clarifying workflowId field purpose in McpWorkflowLookup interface [a3b04ef]
• Added parameterized test for markActivityLogAsFailed error handling with invalid auth tokens [23b878b]
• Modified audit creation policy in createPendingActivityLog utility to propagate authorization errors (401/403) even for read operations and log the cause of other read failures via an optional logger parameter while proceeding unaudited [b0aac65]
• Changed error handling in triggerWorkflow tool handler to return generic retry-later message when pre-trigger workflow lookup fails for non-404 reasons instead of exposing internal transport details [b0aac65]
• Implemented response field whitelisting for workflow-related data returned by ForestHttpApi methods through new projection utilities [b0aac65]
• Updated workflow filtering logic in listWorkflows handler to exclude workflows where collectionName is either null or undefined using loose inequality check [b0aac65]
• Added integration and unit tests covering workflow tool registration, response projection, error handling for lookup failures, and expanded audit policy behavior [b0aac65]
• Updated documentation in CLAUDE.md to clarify cross-file flow distinctions between record-level and workflow tools, response projection whitelisting, and centralized audit fail policy details [b0aac65]
• Introduced error classification and sanitization utilities in workflow-error module [caa542c]
• Implemented sanitized error handling in declareGetWorkflowRunTool, declareListWorkflowsTool, and declareTriggerWorkflowTool handlers within mcp-server package [caa542c]
• Modified ForestHttpApi.getMcpWorkflowById method in forestadmin-client package to return projected response object [caa542c]
• Added comprehensive test coverage for sanitized error handling across workflow tools and response projection [caa542c]
• Updated comments in updateActivityLogStatus function within activity-logs-creator module [caa542c]
• Changed Activity Log label for pre-trigger workflow execution [31976ed]

^{Macroscope summarized 790b913.}

[linear-code]

PRD-49

[qltysh]

5 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many parameters (count = 14): constructor	4
qlty	Structure	Function with high complexity (count = 29): declareTriggerWorkflowTool	1

[qltysh]

Coverage Impact

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

Modified Files with Diff Coverage (18)

Rating	File	% Diff	Uncovered Line #s
	packages/workflow-executor/src/types/validated/execution.ts	100.0%
	packages/mcp-server/src/utils/activity-logs-creator.ts	100.0%
	packages/mcp-server/src/http-client/index.ts	100.0%
	packages/forestadmin-client/src/forest-admin-client-with-cache.ts	100.0%
	packages/workflow-executor/src/adapters/server-types.ts	100.0%
	packages/agent-testing/src/forest-admin-client-mock.ts	100.0%
	packages/forestadmin-client/src/index.ts	100.0%
	packages/mcp-server/src/http-client/mcp-http-client.ts	100.0%
	packages/forestadmin-client/src/build-application-services.ts	100.0%
	packages/mcp-server/src/utils/with-activity-log.ts	100.0%
	packages/mcp-server/src/server.ts	100.0%
	packages/forestadmin-client/src/permissions/forest-http-api.ts	100.0%
	packages/mcp-server/src/tools/get-workflow-run.ts	100.0%
	packages/forestadmin-client/src/workflows/index.ts	100.0%
	packages/mcp-server/src/tools/list-workflows.ts	100.0%
	packages/mcp-server/src/utils/workflow-error.ts	100.0%
	packages/mcp-server/src/tools/trigger-workflow.ts	100.0%
	packages/mcp-server/src/utils/auth-context.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.

[EnkiP]

triggerWorkflow: make the pre-check O(1) (re: PRD-831)

The round-trip can't just be dropped — withActivityLog needs the workflow name for the label before the trigger, and there's no label-update path on the activity-log API. But it shouldn't call listMcpWorkflows (unnests every workflow + the multi-MB collections blob, see forestadmin-server#8418) just to resolve one name.

Add a by-id endpoint — the server already has by-id lookups returning the name (getCollectionAndBpmnAwsS3Identifier → { collectionId, name }):

GET /api/workflow-orchestrator/mcp-workflows/:workflowId → { workflowId, name, collectionName, mcpEnabled }

Tool replaces list+find with one indexed fetch. Same behavior/auditing, name kept in the label, pre-check goes from O(all-workflows) to O(1).

[PMerlet]

Adversarial review of the branch (head a4d3949). Overall solid: URL injection is properly mitigated (encodeURIComponent + tests), no stack/token/internal-URL leakage in error paths (404/403/409/500 all verified), the enum addition is non-breaking within the monorepo, and test coverage goes well beyond happy path. One cross-repo blocker inherited from the server branch, one audit-trail decision to make explicit — details inline.

[PMerlet]

Blocker (cross-repo) — this contract does not exist server-side.

WorkflowRunStatus (currentStep, waitingForHumanInput, result, error) is only a TypeScript cast: ForestHttpApi.getMcpWorkflowRun does queryWithBearerToken<WorkflowRunStatus> with no runtime mapping, and on forestadmin-server#8418 (head 6b9c421) the normalized status mapper was removed after review — GET /mcp-workflows/runs/:runId deliberately returns the full hydrated run ({ runState, triggerType, workflowId, collectionId, workflowHistory: [{ stepName, done, context, stepDefinition… }] }).

Tests pass because they mock the normalized shape. In production, getWorkflowRun will stringify the hydrated run and waitingForHumanInput — the pivot of the documented human-gated flow — will never appear.

Either the server re-lands the mapper, or this package should derive the normalized status from workflowHistory client-side (or embrace the hydrated shape in the types, the tool description, and docs#21). Needs to be settled before merge.

[christophebrun-forest]

Sorry, friday demo --> I forgot to push

[christophebrun-forest]

Resolved (de2872b) — the normalized WorkflowRunStatus cast was dropped and getWorkflowRun now returns the full hydrated run (HydratedWorkflowRun: runState + workflowHistory[] with each step's resolved definition and per-step context), matching what the orchestrator actually sends. The human-gated pivot is no longer a waitingForHumanInput flag but is derived from workflowHistory (a step with done: false and an escalationState/awaitingInputReason context). Types, the tool description and the PR body are updated accordingly.

[PMerlet]

Important — audit trail is fail-open here, unlike every other write tool.

withActivityLog awaits createPendingActivityLog before the operation, outside the try/catch — create/update/delete/executeAction are all fail-closed (no audit log → the action is not executed). Here the log is written entirely post-hoc and any failure is swallowed with a warn: with the activity-log service down, a workflow with real side effects gets triggered with zero audit trail.

Fail-open may well be the right call (the run is already started server-side; failing the tool would mislead the LLM into retrying → 409). But then the pending log should at least be created before the trigger, like everywhere else, so intent is captured even if the trigger itself fails. Either way this deserves an explicit decision from the audit/compliance side rather than being an implementation side effect.

[christophebrun-forest]

Addressed — went fail-closed, as suggested. Latest commit (0ecd716a) reworks triggerWorkflow:

• The pending activity log is now written before the trigger, via the shared withActivityLog wrapper — same fail-closed contract as create/update/delete. Intent is captured even when the trigger itself fails: a trigger-time 404/409 marks the log failed instead of leaving no trace.
• To keep the label rich (workflow name + collection) without the O(n) listing @EnkiP flagged, it first resolves the workflow via a new O(1) by-id endpoint GET /api/workflow-orchestrator/mcp-workflows/:workflowId (forestadmin-server, PRD-49) returning { name, collectionName, mcpEnabled }. So we get fail-closed and a named label and O(1).
• Unknown or MCP-disabled workflows are rejected up front — no run started, no log written.

Tests assert the log-before-trigger ordering and the failed marking on 404/409. Deployment note (also added to the PR description): the server-side by-id endpoint must ship first.

[PMerlet]

Minor: the log is marked succeeded as soon as POST /start returns, but the run is asynchronous and can still abort later. The label ("triggered the workflow …") technically covers it, yet an auditor reading a completed entry may assume the workflow succeeded. Worth a note in the label or a distinct status.

[christophebrun-forest]

Accepted for v1 — this activity log records the trigger call (triggerWorkflow), not the run's terminal state: completed here means the trigger was accepted; the run's actual outcome is read via getWorkflowRun. The label reflects that (triggered the workflow "…", not "completed"). Making the audit mirror the run's terminal state would need server-side status reconciliation (the activity-log API has no async-pending status today) — captured as a follow-up rather than v1. Noted in the PR's "Rollout & release notes" section.

[PMerlet]

Minor: workflowId/recordId (and runId in getWorkflowRun) accept empty strings — z.string() without .min(1). An empty runId even turns the request path into a different endpoint (…/mcp-workflows/runs/); the server 404s, but it is a pointless round-trip. .min(1) closes the gap cheaply. No test covers empty-string args.

[christophebrun-forest]

Fixed in 1e988e28 — workflowId/recordId (triggerWorkflow) and runId (getWorkflowRun) now use z.string().min(1), so an empty string is rejected client-side instead of hitting the server (or, for an empty runId, a different endpoint). Added empty-string assertions to the tool tests.

[PMerlet]

Minor: the description says "Poll the status…" with no interval/backoff guidance, and there is no dedicated rate limiter on the server routes (docs#21 states it explicitly). An LLM can hammer this in a tight loop on a long or human-gated run (which never resolves via MCP in v1). One sentence — "wait at least N seconds between calls" — is free mitigation.

[christophebrun-forest]

Done in 1e988e28 — the description now tells the LLM to poll at a reasonable interval (wait at least a few seconds between calls) and not busy-loop on a long-running or human-gated run that never resolves via MCP in v1. Still no server-side rate limiter, so this is guidance only.

[PMerlet]

Deployment-order constraint worth calling out in the epic/release notes: an executor running a pre-PR version validates triggerType with a z.nativeEnum that lacks mcp → DomainValidationError/MalformedRunError on the first MCP run it picks up. Executors must be upgraded before the orchestrator starts routing MCP-triggered runs. (Within the monorepo the addition is clean — no non-exhaustive switch, run-to-available-step-mapper handles it.)

[christophebrun-forest]

Documented in the PR's "Rollout & release notes": executors must be on the PRD-832 release before the orchestrator routes MCP-triggered runs, otherwise an older executor rejects triggerType='mcp' at validation. Note the server already gates oauth2 MCP steps on executor ≥ 1.14.0, but that's a separate axis from the triggerType enum — we'll confirm the routing/rollout sequencing with the deploy owner (the orchestrator already tracks executor versions via reportExecutorVersion, so version-gating the assignment is a possible follow-up).

[PMerlet]

Release-notes callout: the three tools join allToolNames, so any integration mounting the MCP server without an explicit enabledTools silently gains triggerWorkflow — a side-effectful tool — on upgrade. Consistent with create/update/delete/executeAction being default-on, so no change requested; just make it prominent in the changelog. (Per-workflow opt-in server-side still gates actual exposure.)

[christophebrun-forest]

Acknowledged — no code change (consistent with create/update/delete being default-on). Documented in the PR's "Rollout & release notes": the three tools are default-on, integrations pinning enabledTools are unaffected, and triggerWorkflow is inert until an admin enables the mcp trigger on a workflow — the server rejects a trigger on a non-opted-in workflow (WorkflowMcpTriggerNotEnabledError).

[christophebrun-forest]

@EnkiP done — the pre-check is O(1) now. #1805 stopped listing all workflows to trigger, and the fail-closed audit label no longer needs the listing either: triggerWorkflow resolves the workflow via the new by-id endpoint GET /api/workflow-orchestrator/mcp-workflows/:workflowId (forestadmin-server, PRD-49) returning { name, collectionName, mcpEnabled }. So the audit log is written before the trigger (fail-closed) with a named label, without unnesting every workflow or the multi-MB collections blob. See 0ecd716a.

[PMerlet]

Adversarial review of the MCP workflow tools. Two points worth addressing before merge: the fail-closed audit has a hole (a run can be triggered with no audit log when the audit store is down), and the ForestAdminClientWithCache constructor change is a silent breaking change for external consumers. Plus a minor note on tool annotations. Details inline.

[PMerlet]

The fail-closed guarantee has a hole: a workflow can be triggered with no audit log.

withActivityLog correctly awaits this before triggering, so a rejection blocks the trigger. But createPendingActivityLog never inspects the returned id, and the server route returns HTTP 200 with { data: { id: null, attributes: {} } } (not an error) whenever ActivityLogCreator.create() returns null — which happens in two real cases:

1. Elasticsearch write failure (activity-log-creator.ts → catch { logger.error; return null }) — precisely the "audit store down" scenario fail-closed is meant to cover.
2. collectionName: null — a workflow whose collection was renamed/deleted (the by-id lookup returns collectionName: null via LEFT JOIN). checkAuthorization then returns null (no collection and no dashboard/workspace/inbox).

In both cases this resolves with { id: null }, the trigger fires anyway, and the only trace is a later PATCH /.../null/status that 404s after the run already started. That directly contradicts the PR's claim that "a run with side effects is never started without an audit trail."

Suggested fix: reject in createPendingActivityLog when the returned id is null/undefined (so withActivityLog blocks the trigger), and add a test for the 200-with-null-id response. (The server's 200-on-null is a pre-existing backwards-compat behavior shared with the other tools, but only triggerWorkflow claims fail-closed, so the guard belongs here.)

[christophebrun-forest]

Fixed in 29a4cf1a — createPendingActivityLog now rejects when the 200 response carries a null/undefined id, so withActivityLog blocks the operation before the trigger fires and no PATCH .../null/status is ever issued. The guard lives in the shared creator, so every write tool going through withActivityLog gets the same protection against a dropped audit write (Elasticsearch failure or collectionName: null), not just triggerWorkflow.

Tests added: unit tests on the null-id and undefined-id responses, plus a triggerWorkflow test asserting that a 200-with-{ id: null } response blocks the run — triggerMcpWorkflow and updateActivityLogStatus are never called. The PR's rollout section now mentions this case explicitly.

[christophebrun-forest]

Follow-up hardening on your case 2 (collectionName: null) in 1c30c768: instead of letting it surface as the generic "no activity log id" fail-closed error, triggerWorkflow now rejects it up front — before any activity-log write — with an explicit message: Workflow "<id>" cannot be triggered via MCP because its collection is unavailable. Check the workflow's configuration in Forest. Covered by a test asserting none of the three HTTP calls fire (no pending log, no trigger, no status update).

While there, 5027916c adds a real 409 transport test (nock): a genuine HTTP 409 carrying errors[0].detail is proven to surface through ServerUtils.handleResponseError with the exact message the tool relays — the previous tool-level test only simulated it with a plain Error.

[PMerlet]

Silent breaking change for external consumers — the new param is inserted mid-constructor.

ForestAdminClientWithCache is exported publicly (src/index.ts), and workflowsService is added as the 9th positional argument, before authService. A JS consumer constructing this class directly with the old signature gets a silent argument shift at runtime (workflowsService receives what used to be authService, etc.) — no error, just a corrupted client. Appending at the end of the parameter list would be non-breaking.

Also: the public ForestAdminClient interface gains a required readonly workflowsService, so any external implementation breaks at compile time. The commits are feat/fix → semantic-release cuts a minor, but this is breaking in the strict sense. Either move the param to the end of the constructor, or treat it as a major. (Inside the monorepo versions are pinned exact, so no internal skew.)

[christophebrun-forest]

Fixed in 7f60f458 — workflowsService is now appended as the last constructor parameter; params 1–13 are byte-for-byte the pre-branch main order, so positional construction with the old signature no longer shifts. All construction sites are aligned (the createForestAdminClient factory in src/index.ts is the only non-test site in the monorepo; no subclass or super(...) exists), and a new constructor-wiring test asserts by identity that each positional service lands on its matching property — a direct regression guard for this silent-shift class.

The interface's required readonly workflowsService stays as-is: every member of ForestAdminClient is required, and a lone optional member would misrepresent it as sometimes-absent while the factory always wires it. The compile-time-breaking addition for external implementations is now called out in the PR's "Rollout & release notes".

[PMerlet]

Minor: triggerWorkflow is a side-effectful, default-on tool with no MCP annotations.

resolveEnabledTools defaults to all tools, so an integration calling mountAiMcpServer() without an enabledTools allowlist silently gains this write tool on upgrade. That's mitigated server-side (the trigger 404s until an admin opts a workflow into the mcp trigger), but the tool itself carries no destructiveHint/idempotentHint annotation, so MCP clients can't tell it apart from the read tools. Consistent with create/update/delete today, but worth considering for a tool an LLM can invoke autonomously.

While here: the comment at L25-28 ("The server answers 404 both for an unknown workflow and for one whose MCP trigger is disabled") is inaccurate — the by-id lookup returns 200 with mcpEnabled: false for a disabled workflow (the code handles that via the !workflow.mcpEnabled check just below); only /start 404s both. The code is correct; the comment misleads.

[christophebrun-forest]

Done in 29a4cf1a — registerToolWithLogging already forwards annotations to the SDK's registerTool (the read tools already carry readOnlyHint: true), so triggerWorkflow now declares readOnlyHint: false, destructiveHint: false, idempotentHint: false, openWorldHint: true — open-world because a triggered workflow can reach external systems (emails, webhooks, integrations). No registration refactor needed; the tool test asserts the exact annotation set.

The misleading L25-28 comment is rewritten to match the actual behavior: the by-id lookup returns 200 with mcpEnabled: false for a disabled workflow (handled by the !workflow.mcpEnabled check), only /start 404s both cases.

[PMerlet]

Adversarial review — MCP side (PRD-49)

Read against the server branch (ForestAdmin/forestadmin-server#8418) and the existing tool/audit plumbing.

What holds up: the fail-closed ordering in triggerWorkflow is right and well tested (log before trigger, unknown/disabled/unavailable-collection rejected with no run and no log, trigger-time 404 race mapped, 409 passed through). Error mapping is correct (ServerUtils → NotFoundError/ForbiddenError). Extracting getAuthContext was the right call. WorkflowRunState matches the server enum exactly.

One blocking ask: the fail-closed guard was written for triggerWorkflow but lives in createPendingActivityLog, so it changes behavior for all nine existing tools — including read-only ones — and turns an audit-store outage into a full MCP outage. That is the "fail policy" question from the audit-trail QA list being answered implicitly, for everything. Details inline.

Also: a single MCP trigger now produces two activity-log rows (this one, plus the orchestrator's via MCP one), and listWorkflows/getWorkflowRun produce none.

[PMerlet]

This guard is not scoped to triggerWorkflow — it changes behavior for every tool, including read-only ones.

createPendingActivityLog is what withActivityLog calls for all nine existing tools: list, listRelated, describeCollection, create, update, delete, associate, dissociate, executeAction. So this throw applies to all of them.

Server-side, the null-id response is not hypothetical: activityLogCreator.create returns null when the Elasticsearch write throws (services/activity-logs/activity-log-creator.ts:218-226 → catch → return null), and the route then answers 200 with id: null. Which means an audit-store outage now hard-fails the entire MCP surface, where before it degraded to "operation proceeds, log lost".

For writes, fail-closed is defensible and I would keep it. For list and describeCollection, blocking a read because Elastic is down is a different trade-off, and it is not in the release notes — which present this as a triggerWorkflow change.

Concretely, one of:

• gate on action type (ACTION_TO_TYPE[action] === 'write' → fail-closed, 'read' → log a warning and proceed); or
• keep it global, but say so explicitly in the release notes as an availability trade-off for all tools.

Either way this should be a conscious decision rather than a side effect of the workflow epic — it is exactly the open fail policy item from the audit-trail QA findings.

[christophebrun-forest]

Fixed in bdc82084 — the guard is now gated on action type, exactly as suggested: write actions stay fail-closed (no audit → the operation is blocked), read actions are fail-open — a dropped audit write logs a warning and the read proceeds, skipping status tracking so no PATCH .../null/status is ever issued. An audit-store outage no longer takes down the read surface.

The policy is also stated explicitly in the PR's Rollout & release notes, so it's a documented decision rather than a side effect of the workflow epic.

[PMerlet]

Both new fail-closed tests use 'triggerWorkflow', so the suite documents the guarantee only for the new action — while the change actually applies to the eight pre-existing tools too (see my comment on activity-logs-creator.ts).

Whichever policy you land on, please pin it here with a case on a read action (e.g. 'browse'): either rejects (global fail-closed, deliberate) or resolves (read stays fail-open). Right now a future change in either direction breaks nothing in CI.

[christophebrun-forest]

Done in bdc82084 — the policy is pinned in both directions: an it.each over the five write actions (action, create, update, delete, triggerWorkflow) asserts rejects on a null-id response, and one over the five read actions (index, search, filter, listRelatedData, describeCollection) asserts resolves to null (fail-open). The withActivityLog suite additionally pins the read path end-to-end: warning logged, operation still runs (result returned / error rethrown), and no status tracking in either case. A future change in either direction now breaks CI.

[PMerlet]

This label produces a second Activity Log row for one trigger, and it is the row that does not say via MCP.

The orchestrator independently writes its own audit in createAndStartRun (buildTriggerAuditLabel → triggered the workflow "X" via MCP, attached to the run). This one is written first, attached to the collection, with applicationSource: MCP but no channel in the label.

Net effect: two rows per MCP trigger (webhook has one), and the MCP-originated row reads like a manual trigger at a glance. ForestAdmin/docs#21 tells readers MCP runs are "labelled via MCP", which is only true of the row this PR does not write.

The duplication is defensible — the early row is what makes the trigger fail-closed, and it carries no runId because it cannot — but please align this label (… via MCP) and document the two complementary entries.

[christophebrun-forest]

Fixed in ecb3125f — the MCP-side row is now labelled triggered the workflow "X" via MCP, aligned with the orchestrator's. The duplication is kept deliberately (the early row is what makes the trigger fail-closed, and it cannot carry a runId); the two complementary entries are now documented in the PR's Rollout & release notes, and docs#21 gets a line describing them so an auditor knows what the two rows mean.

[PMerlet]

Mismatch with the PR description, which states destructiveHint: false. The code (and its test, should annotate the tool as a non-read-only, destructive, non-idempotent write) says true. true looks right to me for a tool that starts a workflow with side effects — so it is the description that needs fixing, but it is worth fixing: reviewers and release notes are reading false.

[christophebrun-forest]

Fixed — the PR description now reads destructiveHint: true, matching the code and its test. Thanks for catching the reviewer-facing drift.

[PMerlet]

No activity log on this read.

Every other read tool is audited — list, listRelated, describeCollection all go through withActivityLog with a 'read'-typed action. This one hands the full hydrated run (all steps, per-step context, selectedRecordId) to a third-party LLM and leaves no trace at all.

That also breaks a documented promise: get-started/expose-to-ai-agents.mdx says "Every operation … is logged just like a UI action", and ForestAdmin/docs#21 adds the workflow bullet directly above that line.

Either wrap it like the other reads (a new 'read' action, or reuse an existing one), or state in the docs that discovery and polling are not audited — but the current silent asymmetry is the worst of the three options.

[christophebrun-forest]

Settled as your option 2 (docs qualifier), with the real fix tracked. Wrapping this read would not produce a persisted audit today: the MCP activity-log route requires a resolvable resource (collection/dashboard/workspace/inbox) and silently drops a log without one, and this tool has no collection in hand — resolving one would add a by-id lookup per poll on exactly the path flagged for polling volume. Under the read-fail-open policy (bdc82084) the wrap would send a pending write the server drops anyway: an extra round trip per poll and still no audit row.

So v1 documents the asymmetry instead of hiding it: docs#21's "logged just like a UI action" claim is being qualified to state-changing operations (per your suggestion there), and server-side support for a workflow/run resource on MCP activity logs — after which both reads get wrapped like the other read tools — is tracked in PRD-967 (child of PRD-49).

[PMerlet]

Same as getWorkflowRun: no activity log, while every other read tool writes one. Less sensitive than the run history, but it is the discovery step of a side-effectful flow, so having it absent from the audit trail makes the trigger row harder to explain after the fact.

[christophebrun-forest]

Same resolution as the getWorkflowRun thread just above: not wrapped in v1 because the audit route silently drops resource-less logs — and listWorkflows often has no collection at all (the collectionName filter is optional) — documented via the docs#21 qualifier, and tracked for a real fix (server-side workflow/run resource support) in PRD-967.

[PMerlet]

Worth going one step further than "kept for compatibility": nothing has ever consumed these two fields — this contract ships in the same epic as the endpoint that returns them.

On the server they are the only reason getWorkflowMetadata grew a correlated subquery over renderings.collections, now paid by the manual and webhook start paths too (see my comment on ForestAdmin/forestadmin-server#8418). Dropping them here and there removes the compat wart and the regression at once.

[christophebrun-forest]

Done in 0cdbcce4 — WorkflowRunTriggerResult is now exactly { runId, runState }, and the HTTP layer projects the response explicitly so nothing the server echoes leaks back into the contract. The server-side half (dropping the fields from the start response and reverting the getWorkflowMetadata subquery) is queued on ForestAdmin/forestadmin-server#8418 per your comments there.

[PMerlet]

"The orchestrator holds no customer record data … so the full run is safe to surface" is a bit stronger than what the shape guarantees, and this sentence is being repeated verbatim into the server comments and the public docs.

Two counter-examples in this very interface: selectedRecordId is a customer record identifier, and WorkflowHistoryStepContext.error is free-form text reported by the executor — a failing get-data/update-data step can easily embed values from the customer's database in it. The docs even instruct readers to read context.error to diagnose a bad record.

Suggest hedging to something like "carries no record payload; identifiers and executor-reported error strings may still contain customer data" — and mirroring that in the docs rather than a flat guarantee.

[christophebrun-forest]

Fixed in 1ce50dce with your wording — the docstring now reads: carries no record payload (records live in the executor), but identifiers (selectedRecordId) and executor-reported error strings (context.error) may still contain customer data. The two other copies of the flat guarantee (the server-side service comment on #8418 and the docs#21 Security section) get the same hedge on their respective PRs.

[PMerlet]

Four near-identical if (!this.forestAdminServerInterface.X) throw new Error('… does not support X.') blocks. Making the interface methods optional to avoid breaking external implementations is the right call, but the guard could be one small helper (assertSupported('getMcpWorkflowById') or a resolve(name) returning the bound method) — four copies of the same string template is the kind of thing that drifts.

[christophebrun-forest]

Done in ade72f8d — the four guards are now a single resolveTransportMethod(name) helper returning the bound transport method (one message template, no drift), plus an httpOptions(token) builder for the repeated options literal. The existing error-message assertions pass unchanged.

[PMerlet]

New file with no test/utils/auth-context.test.ts. It is covered indirectly through the tool suites, but it is now the single choke point through which every tool derives the caller's identity — the two throw branches (missing/non-string token, null-or-undefined renderingId) and the number→string renderingId coercion deserve to be pinned directly, since a regression here is an authentication-scoping bug rather than a tool bug.

[christophebrun-forest]

Added in d8ceca49 — test/utils/auth-context.test.ts pins the extraction directly: happy path, both throw branches (missing and non-string forestServerToken; missing and null renderingId), the number→string renderingId coercion, and the absent-authInfo case.

[PMerlet]

Blocker — the "reads fail open" contract is only half implemented.

createPendingActivityLog is awaited outside the try, so the fail-open path added in d8ceca4 only covers one case: the server answering 200 with a null log id. If the call rejects — 5xx, timeout, ECONNREFUSED, or the 400 that createFromMcp returns when collectionModelName / label don't validate — the rejection propagates from this line and the tool fails, type: 'read' included.

That takes down every read tool routed through this wrapper (list, describeCollection, listRelatedData), which is exactly what the PR description says cannot happen:

read actions fail open (a dropped audit write logs a warning and the read proceeds, without status tracking) — so an audit-store outage never takes down the read surface.

An audit-store outage is the far more likely failure mode than a 200-with-null-id, and it is the one case the guard doesn't reach.

The file also carries both policies at once — the comment two lines above still states the old one:

// We want to create the activity log before executing the operation
// If activity log creation fails, we must prevent the execution of the operation

Either way out works:

1. Make the code match the claim — wrap the creation call and apply to a thrown error the same type === 'write' arbitration that activity-logs-creator.ts already applies to the null id: writes rethrow, reads log the warning and continue with activityLog = null. The downstream if (activityLog) guards already handle that shape, so it is a local change.
2. Make the claim match the code — narrow the PR description and mcp-server.mdx (Identity, auditing, and limits) to "a dropped audit write acknowledgement", so we stop documenting a guarantee the code doesn't give.

Worth a test in both cases: createMcpActivityLog rejecting should let a read through and still block a write. Today only the null-id variant is covered.

[christophebrun-forest]

Fixed in e97b498 — and you were right that this was the case that mattered, not the one the guard covered.

createPendingActivityLog now wraps the transport call, and a rejection goes through the same ACTION_TO_TYPE arbitration as the null id: writes rethrow, reads log the warning and continue with activityLog = null. The whole policy lives in one place next to the null-id guard, so there is a single spot where the decision is made. getAuthContext stays outside it — a missing token is a caller bug, not an audit outage, and your existing tests pinned that.

Two things I found while doing it, both of which make your point stronger than the thread states:

1. The stale comment was worse than a leftover — a test pinned the contradiction. activity-logs-creator.test.ts:239-249, "should propagate error when createMcpActivityLog fails", used the 'index' action — a read — and asserted rejects.toThrow. So CI was actively defending the behaviour the release notes said was impossible. That test is now the write-action case, and the read matrix asserts the opposite.
2. The case the guard did cover is the least likely of the three. On /mcp, collectionModelName is Joi.string().required() → 400, and an unresolvable modelName → 404. Both are rejections. The 200-with-null-id only happens on an Elasticsearch write failure. So the guard was covering the rare path and missing the common ones — which also means my own justification on the getWorkflowRun thread ("the server silently drops it") was wrong; corrected there.

Tests: it.each over the five write actions asserting the rejection propagates, it.each over the five read actions asserting it resolves to null, the three failure modes named individually (400 unresolvable collection, 5xx, ECONNREFUSED), a case proving a read still throws on an invalid auth context without calling the server, and a withActivityLog test asserting a rejected creation never runs a write operation. The comment now describes the policy it implements, and the PR body says which two paths it covers.

[PMerlet]

Adversarial pass on the remaining surface, after the fail-closed audit and the by-id lookup landed. The blocker is in a separate thread on with-activity-log.ts; these four are the rest of what I found on this side.

[PMerlet]

Important — the two new read tools are the only ones on the MCP server that leave no audit trail.

Neither get-workflow-run.ts nor list-workflows.ts imports withActivityLog. Every other read tool does — list, describeCollection, listRelatedData all route through it with a type: 'read' entry.

That matters most here, because getWorkflowRun returns the heaviest payload of the three: selectedRecordId, the executor-reported context.error strings, and the full resolved step definitions. The PR description and mcp-server.mdx both acknowledge that payload may carry customer data — and reading it is currently untraceable. Same for getMcpWorkflowById, which triggerWorkflow calls before anything is logged: a caller can probe workflow ids, names and collections without leaving a row.

The tell is in the docs PR: expose-to-ai-agents.mdx goes from "Every operation […] is logged" to "every state-changing operation is logged". A global promise was weakened to fit a local gap — and the new sentence now under-describes what the pre-existing read tools actually do.

Two coherent options:

1. Wrap both tools in withActivityLog with type: 'read' (ACTION_TO_TYPE already has the shape for it), which restores parity and lets the docs keep the general sentence.
2. Keep them unaudited as a deliberate v1 call — but then say so explicitly in mcp-server.mdx (Identity, auditing, and limits) rather than weakening the global claim, and restore the original wording on expose-to-ai-agents.mdx.

Option 1 is the smaller diff and the one that doesn't cost a documented guarantee.

[christophebrun-forest]

Settling this as your option 2, but rewritten — because both my earlier rationale and the thread's premise turn out to be wrong.

My "the server silently drops it" justification was false. On /mcp, collectionModelName is Joi.string().required() → 400, and an unresolvable modelName → 404. The silent drop exists in activity-log-creator.ts but is unreachable through this route (only an Elasticsearch failure gets there). The conclusion holds — no persisted row, one extra round trip per poll — but the mechanism is a rejection, which is exactly the path you flagged as uncovered in with-activity-log.ts. The two threads were the same defect seen from opposite ends; that one is fixed in e97b498.

And these are not the only unaudited tools. getActionForm is a read tool that doesn't import withActivityLog either, and it predates this epic. So "Every operation […] is logged" was already inaccurate before PRD-49 — which changes the right fix.

So instead of weakening the global claim, 19fe126 restores it and carves out the exception where it belongs, naming all three tools under Identity, auditing, and limits, plus the fail policy in one line. expose-to-ai-agents.mdx points there rather than hedging, and the two other global claims on the page (:19, the security bullet list) are qualified the same way instead of being left to contradict it.

Your sub-point about getMcpWorkflowById letting a caller probe ids before anything is logged is real and unchanged — it's part of what PRD-967 now covers, along with getActionForm.

[PMerlet]

Important — a server rollback turns this into a silent loop for the assistant.

getMcpWorkflowById hits GET /api/workflow-orchestrator/mcp-workflows/:workflowId, which only exists once MS8 is deployed. If the orchestrator is older — or gets rolled back after an incident — that route 404s, ServerUtils maps it to NotFoundError, and this branch answers:

Workflow "X" is not an MCP-enabled workflow you can access. Use listWorkflows to discover triggerable workflows.

Meanwhile listWorkflows (MS3, already deployed) keeps working and keeps returning that exact workflow. So the assistant lists, picks an id, triggers, is told to list again — and loops. Every trigger in the rendering is dead, with a message that actively points the model back at the call that produced the id.

The "deploy the server side first" note covers the forward order, not the rollback, and rollback is the case where nobody is watching the deploy notes.

Worth distinguishing route absent from workflow unknown — the orchestrator's 404s carry a structured error body while an unmatched route doesn't, so the discriminator is cheap — and logging a real Error server-side when it's the former, so the failure is visible to an operator instead of only to the model.

[christophebrun-forest]

Fixed in 7052c54, both halves.

The lookup's catch now always logs an Error before translating, so an orchestrator that predates or was rolled back before the by-id endpoint is visible to an operator instead of only to the model. And the tool error ends with "If listWorkflows just returned this id, do not retry — report it to your Forest administrator instead." — which breaks the loop without telling the caller whether the workflow exists, so the uniform 404 contract holds.

I didn't take the structured-body discriminator. It needs ServerUtils to carry that information through HttpError, which is shared code well outside this epic, and comparing error.message against the generic sentinel would be a magic-string hack. Logging unconditionally gets the operator visibility, which was the part nothing provided.

Two tests pin it: the exact message text, and the exact log line on a lookup failure.

While in here I also found a second loop with no deploy dependency at all: listWorkflows returned workflows whose collectionName is null, which triggerWorkflow rejects up front — so list → pick → rejected → list, reproducible today. Fixed in 9ec10b78 by filtering them out, with a warning naming how many were hidden.

Rollback is now in the PR body as its own bullet, next to the forward order, since that was the gap.

[PMerlet]

Minor — the guard is right, the stated reason isn't.

The comment says the activity log "would be dropped server-side". It wouldn't: createFromMcp validates collectionModelName: Joi.string().required() (packages/private-api/src/validators/routes/activity-logs-requests.js), so a null collection is a 400, which then propagates out of createPendingActivityLog — a hard failure, not a silent drop. Same outcome for the caller, different mechanism, and the comment will send the next reader looking for a drop path that doesn't exist.

While here: recordId is z.string().min(1) with no upper bound, while the server caps selectedRecordId at 255. A 256-char id gets past this guard, writes the pending audit row, and only then takes a 400 on the trigger. Adding .max(255) to the zod schema makes the rejection actually happen up front — which is what the docs error table already claims (mcp-server.mdx, Errors: "rejected up front, nothing starts").

[christophebrun-forest]

(a) Right, and corrected in 7052c54. The comment claimed the log "would be dropped server-side"; the audit route validates collectionModelName as required, so a null collection is a 400 — a hard failure, not a drop. It now says the route rejects it and that the up-front check exists to replace a misleading fail-closed message with one that names the actual problem. Same family as the McpWorkflowLookup docstring, corrected in 2be4504.

(b) .max(255) added in 2be4504, with a schema test asserting 255 passes and 256 doesn't. Worth recording what it does and doesn't buy: I checked the server side, and selectedRecordId is VARCHAR(255) with the Joi validator already capping at 255 — so an over-long id was a clean 400 before any INSERT, never a 500. The gain is avoiding a round trip and a pending audit row marked failed, not closing a risk.

On the docs contradiction: I couldn't find it. The page states the record isn't validated at trigger time, and the tool description repeats it. The line that was imprecise is the Errors-table row, which lumped the empty and out-of-bounds cases together — split on docs#21, where the >255 case is now accurate for the right reason (the tool rejects it, so "nothing starts" is true).

Related, from Macroscope on the server PR and worth knowing here: a numeric recordId past MAX_SAFE_INTEGER was silently rounded by JSON.parse before String(recordId) persisted it — the run started on a different record. Rejected now (6205c9e0f). The MCP tool always sends a string, so it was never exposed, but the endpoint was.

[PMerlet]

Minor — this enum is built from the wrong side of the boundary.

ctx.collectionNames comes from the agent's Forest schema, but the server filters on coll."modelName" read out of the rendering layout's collections blob (layout-workflows-store.ts, the coll CTE). They normally agree; when they drift — a collection renamed in the layout, or present in one and not the other — the enum rejects a value the server would have accepted, and the assistant has no way to express the filter.

Since the argument is optional and the server already handles an unknown collectionName by returning an empty list, z.string() would be the safer type here; the enum buys autocomplete at the cost of a hard failure on drift. Not blocking, but worth a deliberate call rather than inheriting the pattern from the record-level tools, where collectionNames is the authoritative source.

[christophebrun-forest]

Traced it, and the mechanism you describe doesn't exist — but there is a real problem next to it, so thank you for the pull.

coll."modelName" and the agent schema's name are the same identifier by construction. The layout builder writes modelName: model.name (layout-builder.ts:99-104), and models.name is the collection's id from the apimap — the same value the schema exposes as name. They are not two independently-maintained fields. And modelName is not mutable: make-layout-patch-patterns.ts exposes PATCH patterns for displayName and displayNamePlural, none for modelName. Renaming a collection in the UI touches the display name. So no drift by rename, in either direction.

(For completeness: the three namespaces I mentioned earlier — "legacy integer, uuid or modelName" — are about collection.id, which is the LEFT JOIN key, a different axis. That one is the dedup question, now fixed with DISTINCT ON.)

The real risk is staleness, not disagreement. The enum is frozen at startup from a schema cache with a 24-hour TTL (schema-fetcher.ts, ONE_DAY_MS + a module-level schemaCache). A collection added after boot is rejected by zod for up to a day, even though the server would accept it. And the degraded path already falls back to z.string() when the schema fetch fails, so both behaviours coexist in production today.

I'm keeping the enum: it matches the eight other tools and the package's documented convention, and it gives the model autocomplete on the overwhelmingly common case. But say the word if you think a 24-hour window justifies z.string() here — the argument is optional and the server returns an empty list for an unknown name, so the downside of loosening it is small. Happy to flip it.