feat(workflow-executor): add 3-way execution mode to the load related record step (PRD-148)#1728
Conversation
… record step (PRD-148) Manual presents the narrowed list with no AI; AI-assisted suggests a record; Full AI selects + loads and auto-skips on no source record, empty list, or AI judging none relevant. selectedRecordRef is optional now (no-source skip); the wire serializer guards it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (1)
🛟 Help
|
…resolveTarget (PRD-148) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… candidate list (PRD-148) When the HasMany candidate list is truncated to fit the AI prompt budget, -1 (none relevant) is no longer offered — the AI picks from what it saw, so a match in the unseen tail is not silently skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PRD-148) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| target = await this.resolveTarget(useAi); | ||
| } catch (error) { | ||
| // No source record to load from → Full AI auto-continues. (Manual/AI-assisted let this error | ||
| // surface so the front can offer "continue without" — PRD-550.) |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Violates conventions
Ticket ID PRD-550 in a code comment. Project convention: no PRD/ticket IDs in comments (applies to code and tests). Drop — PRD-550; the sentence reads fine without it. Heads-up: the linter doesn't catch this — worth a lint rule if the team wants it enforced.
|
|
||
| it('surfaces a "no source record" error in await modes when the source step loaded nothing', async () => { | ||
| // Only Full AI auto-skips; Manual / AI-assisted surface the error so the front can offer | ||
| // "continue without" (PRD-550). |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Violates conventions
(PRD-550) in a // comment — the no-ticket-IDs-in-comments rule applies to tests too. Drop the ID; the behavioral note is enough. (The describe('… (PRD-148)') titles are borderline and out of the comment rule, so left alone.)
|
|
||
| // "None relevant" (-1) is only trustworthy when the AI saw the whole list. If it was truncated, | ||
| // force a pick from what was shown — otherwise a match in the unseen tail would be silently skipped. | ||
| const noneAllowed = allowNone && !truncated; |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Should fix
The noneAllowed = allowNone && !truncated rule — the safety core of the feature (-1 "none relevant" only trusted when the AI saw the whole list) — has no test. The truncation tests all run the await path (allowNone:false); the none-relevant test uses a 2-candidate, non-truncated list. So the Full-AI + truncated combination, where noneAllowed collapses to false, minIndex→0, and a -1 response is rejected by the guard at line 764, is unverified — a regression dropping !truncated (silently skipping a record that matches in the unseen tail) would pass the whole suite. Add: Full-AI HasMany, candidates wide enough to truncate, AI returns -1 → assert InvalidAIResponseError, not a skip.
| .enum([AutomatedWithConfirmation, FullyAutomated]) | ||
| .default(AutomatedWithConfirmation) | ||
| .catch(AutomatedWithConfirmation), | ||
| .enum([Manual, AutomatedWithConfirmation, FullyAutomated]) |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Preferential
No test asserts this schema parses executionType: 'manual' and returns 'manual'. The executor tests build definitions directly via makeStep(...), bypassing the schema, and the only no-coercion tests live in an out-of-PR file (trigger-action / condition). A one-line LoadRelatedRecordStepDefinitionSchema.parse({ …, executionType: 'manual' }) → toBe('manual') cheaply guards Manual mode against the enum being narrowed back later.
| private async selectBestFromRelatedData( | ||
| target: Pick<RelationTarget, 'selectedRecordRef' | 'name' | 'relatedCollectionName'>, | ||
| limit: number, | ||
| opts: { rank: boolean; allowNone: boolean } = { rank: true, allowNone: false }, |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Preferential
{ rank; allowNone } permits the meaningless { rank:false, allowNone:true } — when rank:false the method returns (line 492-494) before allowNone is ever read. A union { rank: false } | { rank: true; allowNone: boolean } makes that illegal state unrepresentable. Minor (private, single-file); flag only for clarity should it grow more callers.
| pendingData?: LoadRelatedRecordPendingData; | ||
| selectedRecordRef: RecordRef; | ||
| // Absent only on the Full AI no-source auto-skip; always set on the await/load paths. | ||
| selectedRecordRef?: RecordRef; |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Preferential
Making selectedRecordRef optional widens the type to "sometimes absent", but the real invariant is narrower: absent ⟺ the Full-AI no-source skip (executionResult: { skipped: true }). That coupling is enforced only by this comment + scattered runtime guards, and summary/step-execution-formatters.ts:99 derefs selectedRecordRef.recordId relying on it. Not a bug today (every producer upholds it), but a discriminated union (loaded vs skipped variant) would make the deref safe by construction and remove the !selectedRecordRef guards at lines 107/429.
| .default(AutomatedWithConfirmation) | ||
| .catch(AutomatedWithConfirmation), | ||
| .enum([Manual, AutomatedWithConfirmation, FullyAutomated]) | ||
| .default(AutomatedWithConfirmation), |
There was a problem hiding this comment.
[Claude Opus 4.8 (1M context)] · Preferential (discussion)
Dropping .catch is the right call — it stops silently coercing values into AI-assisted. One downstream consequence to be aware of: an unknown/future executionType now throws a raw ZodError, and on the pending-run path toStepDefinition (run-to-available-step-mapper.ts:165) runs outside the try/catch that maps to DomainValidationError — so getAvailableRuns doesn't see it as a WorkflowExecutorError and logs-and-drops the run rather than reporting it malformed. Low reachability (server contract enum has 3 values), but this is exactly the executor version-skew case the conventions call out: an older executor receiving a newer 4th mode would silently drop the run instead of surfacing an "update needed" error. Fix is downstream/out of scope here. Also stale as a result: the comment at step-definition-mapper.ts:22 ("each schema's .default().catch() handles unsupported values") no longer holds for this schema or trigger-action.
…e AI decline (PRD-148) Full AI no longer silently skips when it can't load a relevant record. With nothing relevant it degrades to an AI-assisted confirmation (a human decides, "No X to load" pre-checked); no source record now surfaces an error like the await modes instead of skipping. AI-assisted may decline too (allowNone), and the record-selection prompt tells the AI to return -1 on an impossible request rather than forcing a weak match. pendingData carries a suggestNoRecord flag so the front distinguishes an active "nothing relevant" from a plain no-suggestion (Manual). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| suggestViaAi, | ||
| ); | ||
|
|
||
| await this.context.runStore.saveStepExecution(this.context.runId, { |
There was a problem hiding this comment.
🟡 Medium executors/load-related-record-step-executor.ts:122
refreshCandidatesForField spreads the old pendingData (line 126) without clearing suggestNoRecord. When a prior relation set suggestNoRecord: true and the user switches to a relation that returns a suggestedRecord, the saved pendingData still carries suggestNoRecord: true, so the UI keeps the "No record to load" state even though a valid record was just suggested. Consider recomputing suggestNoRecord in refreshCandidatesForField the same way saveAndAwaitInput does.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/workflow-executor/src/executors/load-related-record-step-executor.ts around line 122:
`refreshCandidatesForField` spreads the old `pendingData` (line 126) without clearing `suggestNoRecord`. When a prior relation set `suggestNoRecord: true` and the user switches to a relation that returns a `suggestedRecord`, the saved `pendingData` still carries `suggestNoRecord: true`, so the UI keeps the "No record to load" state even though a valid record was just suggested. Consider recomputing `suggestNoRecord` in `refreshCandidatesForField` the same way `saveAndAwaitInput` does.
2 participants
What
Execution logic for the 3-way Execution mode (Manual / AI-assisted / Full AI) on the workflow Load Related Record step. Part of PRD-148 — 3-PR set (executor +
ForestAdmin/forestadmin-serverorchestrator +ForestAdmin/forestadmineditor).manual): present the narrowed candidate list with no AI — no relation chooser, no record suggestion. Single-record relations (xToOne, or HasMany with 1 candidate) still pre-fill, since that's the only option, not an AI pick. The Zod schema drops.catch(AutomatedWithConfirmation)and addsManual, so amanualvalue is no longer silently coerced back into AI prefill (same reasoning as TriggerAction).automated-with-confirmation, default): AI suggests a record, user confirms (unchanged).fully-automated): AI selects and loads. Auto-skips (persistsexecutionResult: { skipped: true }+success) when there is no source record (SourceRecordMissingError, Full-AI only), an empty candidate list, or the AI judges none relevant (select-record-by-contentreturns-1via the new Full-AI-onlyallowNone).selectedRecordRefis now optional onLoadRelatedRecordStepExecutionData(absent only on the no-source Full-AI skip); the wire serializer omits it instead of dereferencingundefined.Tests
97 tests in the executor suite. Added: Manual = no AI / no pre-selection (incl. relation switch), Manual single-record pre-fill, Full-AI skip on each of the 3 causes, await-mode no-source still errors (regression guard), serializer no-source round-trip.
Validation
Validated end-to-end on the local stack (account → bills) across the 3 modes, plus a local multi-agent code review — 1 Critical found & fixed: serializer crash (503) on the no-source Full-AI skip.
Relates to PRD-148.
🤖 Generated with Claude Code
Note
Add Manual execution mode to
LoadRelatedRecordStepExecutorwith auto-skip for FullyAutomatedManualas a validexecutionTypefor the load-related-record step; in Manual mode, AI is not called for relation selection or candidate suggestion — the first eligible relation is picked deterministically and nosuggestedRecordis included.FullyAutomatedmode, steps now auto-skip (via newpersistSkip) when there is no source record, no linked xToOne record, or when the AI ranks all candidates as irrelevant (returns-1).selectBestRecordIndexgains anallowNoneoption so AI can return-1only when the candidate list was not truncated, preventing forced selections.fetchRecordForRelationandfetchXToOneRecordRefnow returnnullon absence instead of throwing, with callers deciding skip vs. error behavior.selectedRecordRefis made optional inLoadRelatedRecordStepExecutionDataand the wire serializer skips the field when absent, fixing a serialization crash on auto-skipped steps.executionTypeschema no longer coerces unknown values toAutomatedWithConfirmation; invalid values will now fail validation instead of being silently swallowed.Changes since #1728 opened
suggestNoRecordflag instead of auto-skipping when no relevant record exists [9ee1164]suggestNoRecordfield toLoadRelatedRecordPendingDataand updatedLoadRelatedRecordStepExecutionDatainterface documentation [9ee1164]collectCandidateIdsandresolveAndLoadAutomatic[9ee1164]Macroscope summarized 4f8b398.