feat(agent): resolve select-all selections for approval-required actions

[EnkiP]

Summary

Part of enabling bulk actions with approval on "select all" selections.

When an approval-required action is triggered with all_records: true, the frontend has no explicit id list to store in the approval request. The agent now:

• Resolves the select-all selection (filters + search + segment + scope, minus excluded ids) to concrete record ids and returns them in the CustomActionRequiresApprovalError payload (data.recordIds), so the frontend can store the full target set in the approval request.
• Caps the resolution with the new maxRecordsForApproval agent option (default 500, matching the Forest server's authoritative cap on record_ids). Above the cap, the trigger is rejected with ApprovalSelectionTooLargeError (422).
• Leaves normal (non-approval) executes untouched — they still run on the whole selection, uncapped.

Related PRs

• agent-ruby: feat(actions): resolve select-all selections for approval-required actions agent-ruby#374

• forest-express: feat(actions): return resolved record ids for select-all approval requests forest-express#1064

• forest-rails: feat(actions): resolve select-all selections for approval-required actions forest-rails#793

• Frontend: ForestAdmin/forestadmin#9924

• Server (authoritative 500 cap): ForestAdmin/forestadmin-server#8462

fixes PRD-934

Test plan

• Unit tests: select-all resolution returned in the approval error, over-cap rejection, explicit selections unaffected (44 action route tests + 15 authorization tests pass)
• Manual test against a 120-record select-all approval flow

🤖 Generated with Claude Code

Note

Resolve select-all record IDs for approval-required custom actions

• When a select-all bulk action requires approval, ActionRoute now resolves the selection to concrete record IDs via resolveApprovalRecordIds and includes them in the CustomActionRequiresApprovalError payload; Global-scoped actions are exempt.
• Adds maxRecordsForApproval option (default 500 via DEFAULT_MAX_RECORDS_FOR_APPROVAL) to AgentOptions; select-all selections exceeding this cap throw ApprovalSelectionTooLargeError (422).
• parseAgentError in the MCP server now recursively extracts cause details (bounded to 3 levels) so nested error context surfaces to MCP clients.
• Risk: assertCanTriggerCustomAction signature changed to accept an optional resolveSelectAllRecordIds callback — any out-of-tree callers of this method need to update. The new cap means select-all approval requests with more than maxRecordsForApproval records now return 422 instead of an approval prompt.

Changes since #1847 opened

• Removed customization for the account_bills collection from the makeAgent factory function [481aa92]

^{Macroscope summarized db92fb6.}

[linear-code]

PRD-934

[qltysh]

4 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with high complexity (count = 12): handleExecute	3
qlty	Structure	Function with many returns (count = 4): parseAgentError	1

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (5)

Rating	File	% Diff	Uncovered Line #s
	packages/agent/src/routes/modification/action/action.ts	100.0%
	packages/mcp-server/src/utils/error-parser.ts	100.0%
	packages/agent/src/utils/options-validator.ts	100.0%
	...s/agent/src/routes/modification/action/action-authorization.ts	100.0%
	...dification/action/errors/approval-selection-too-large-error.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.

[matthv]

Review findings — select-all → approval record-id resolution

Cross-checked against the companion PRs (forestadmin#9924, forestadmin-server#8462) as part of a coordinated review of PRD-934. Summarizing critical/important findings only.

🔴 Critical

1. maxRecordsForApproval is never validated — a NaN value silently disables the cap entirely
packages/agent/src/utils/options-validator.ts only does copyOptions.maxRecordsForApproval = copyOptions.maxRecordsForApproval ?? 500; (no check in checkOtherOptions, unlike permissionsCacheDurationInSeconds a few lines below which is actively clamped).

If maxRecordsForApproval ends up NaN (a realistic case: Number(process.env.X) with an unset env var), then in resolveApprovalRecordIds (action.ts:~300-327), records.length > max becomes records.length > NaN, which is always false in JS. The over-cap check can never fire — an unbounded id list gets snapshotted into the approval request, exactly what this PR was written to prevent. No error, no log.

Suggest validating at Agent construction time (fail fast), mirroring the existing checks for other options in the same file:

if (options.maxRecordsForApproval != null &&
    (!Number.isInteger(options.maxRecordsForApproval) || options.maxRecordsForApproval < 1)) {
  throw new Error('options.maxRecordsForApproval is invalid. It should be a positive integer.');
}

(0/negative values already fail loudly today with a confusing message, so they're lower severity — but the same fix covers them.)

2. resolveApprovalRecordIds doesn't guard scope === 'Global'
action.ts gates resolution solely on all_records, with no check on the action's scope — but getRecordSelection deliberately ignores the selection for Global actions. The sibling method auditedRecordIds already handles this (return [] for scope === 'Global', action.ts:~268), and resolveApprovalRecordIds doesn't mirror it.

Concretely: a global, approval-required action triggered with all_records: true on a collection with more than maxRecordsForApproval rows now fails with a new ApprovalSelectionTooLargeError (422) where it previously worked — a regression on a flow this PR isn't meant to touch. Under the cap, it snapshots the entire collection into recordIds for an action that targets no specific records.

🟠 Important

• Magic number duplication: 500 is hardcoded in options-validator.ts, the test factory (forest-admin-http-driver-options.ts), and the JSDoc in types.ts, with no shared named constant — unlike the existing MAX_SNAPSHOT_RECORDS pattern used for the analogous audit path. Given this value is a cross-service contract (must match the server's cap), it's the one number here that most deserves a name.
• Test coverage gaps in action.test.ts: no test at exactly max records (the boundary the cap+1 fetch trick exists to disambiguate); no assertion on the actual collection.list call args (filter/projection/Page) — current tests would pass even with a wrong filter or an off-by-one page size; no test for collection.list rejecting; no test for maxRecordsForApproval at 0/negative/non-integer; no test confirming the resolver is skipped when approval isn't required.
• The comment on maxRecordsForApproval ("must not exceed the Forest server's own cap") documents a cross-repo invariant that's asserted nowhere and unenforced — worth a code comment caveat or a startup-time upper-bound check rather than a bare doc claim.

Happy to open follow-up tickets for the test-coverage items if useful.

[EnkiP]

Addressed in 1530139:

Critical 1 (NaN disables the cap) — confirmed the reported behavior: NaN ?? 500 stays NaN (?? only catches null/undefined) and length > NaN is always false, so Number() on an unset env var silently disabled the cap. The default now uses Number.isFinite, so NaN/undefined/null all fall back to 500 (new tests cover it). Other invalid values (0, negative) intentionally keep failing loudly — wrong configuration breaking is acceptable.

Critical 2 (Global scope) — fixed: the resolver is only passed for non-global actions, mirroring auditedRecordIds. A global approval-required action triggered with all_records no longer 422s nor snapshots the collection (test added asserting no recordIds and no list call).

Important — the 500 default is now the named DEFAULT_MAX_RECORDS_FOR_APPROVAL constant with the cross-service contract documented. Test gaps: added the exact-cap boundary test asserting the Page(0, cap+1) list call args. The remaining gaps (list rejection, 0/negative option values) and the qlty handleExecute complexity warning are left as-is.