feat(workflow-executor): deterministic source record for Get Data steps (PRD-775)#1760
feat(workflow-executor): deterministic source record for Get Data steps (PRD-775)#1760EnkiP wants to merge 1 commit into
Conversation
1 new issue
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EnkiP
force-pushed
the
workflow/deterministic-get-data
branch
from
5314018 to
b0f298f
Compare
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (2)
🛟 Help
|
| step.prompt, | ||
| preRecordedArgs?.selectedRecordStepIndex, | ||
| ); | ||
| const selectedRecordRef = preRecordedArgs?.selectedRecordStepId |
There was a problem hiding this comment.
🟠 Truthiness gate: an empty-string selectedRecordStepId silently downgrades to AI/legacy resolution
The branch is preRecordedArgs?.selectedRecordStepId ? … (truthiness), but the zod schema types it z.string().optional(), which accepts ''. If a config ever persists selectedRecordStepId: '' (partial revision, serialization glitch, a cleared field), '' ? A : B takes the legacy/AI branch — a step the operator configured as deterministic silently reads an AI-chosen record.
Suggest gating on presence rather than truthiness (!= null) or rejecting empty in the schema (z.string().min(1)), so a malformed id fails loudly instead of degrading.
🟡 Related (observability): nothing here logs which path ran. When selectedRecordStepId/fieldNames are absent (e.g. after a revision that dropped them), the step runs fully AI-driven with status: success and no trace distinguishing "resolved deterministically" from "fell back". On a feature branch literally named deterministic-get-data, a Warn log on the fallback would save a lot of debugging.
| ); | ||
| const schema = await this.getCollectionSchema(selectedRecordRef.collectionName); | ||
| const fieldNames = | ||
| preRecordedArgs?.fieldNames ?? (await this.selectFields(schema, step.prompt)); |
There was a problem hiding this comment.
🟠 An explicitly-configured empty fieldNames: [] throws an error that blames the AI
preRecordedArgs?.fieldNames ?? (await this.selectFields(...)) uses ??, so an explicit empty array [] is kept (not replaced by the AI). Downstream that yields resolvedFieldNames.length === 0 → NoResolvedFieldsError, whose userMessage is "The AI selected fields that don't exist on this record. Try rephrasing the step's prompt." — but in this case the AI was never invoked; the cause is an empty/misconfigured deterministic fieldNames. The operator will chase a non-existent AI/prompt issue.
The message (and ideally the error type) should branch on whether the fields came from config vs the AI. Note this path is newly reachable: before this PR the mapper didn't forward preRecordedArgs to get-data, so fieldNames was always undefined here.
| interface ServerWorkflowTaskGetData extends ServerWorkflowTaskBase { | ||
| taskType: ServerTaskTypeEnum.GetData; | ||
| executionType: ServerStepExecutionTypeEnum.FullyAutomated; | ||
| preRecordedArgs?: { selectedRecordStepId?: string }; |
There was a problem hiding this comment.
🟡 This "local mirror of the orchestrator contract" is now inaccurate for get-data
The added type declares only { selectedRecordStepId?: string }, but doExecute actively consumes two more keys the orchestrator sends in the same object: fieldNames (read-record-step-executor.ts) and selectedRecordStepIndex (legacy). Runtime is fine (the mapper forwards task.preRecordedArgs wholesale and the zod schema keeps both), so this is type-only — but since this file is documented as the orchestrator contract mirror, it should reflect what read-record actually reads:
preRecordedArgs?: {
selectedRecordStepId?: string;
selectedRecordStepIndex?: number; // legacy
fieldNames?: string[];
};
|
|
||
| const result = await executor.execute(); | ||
|
|
||
| expect(result.stepOutcome.status).toBe('error'); |
There was a problem hiding this comment.
🟡 This negative test is too weak to tell the two error paths apart
It asserts only status).toBe('error'). Per this repo's CLAUDE.md ("assertions must verify behavior, not just that something happened") — and the file's own pattern elsewhere (expect(result.stepOutcome.error).toBe(...)) — it should assert the error type/message (InvalidPreRecordedArgsError, "No source record found for step…"). As-is, this test still passes if InvalidPreRecordedArgsError and SourceRecordMissingError were swapped, since both produce status: 'error'.
Two coverage gaps in the same area worth adding:
SourceRecordMissingError(source step pinned but it loaded nothing) — the one branch of the new resolution chain with zero coverage.- Fully-deterministic → zero AI: a test with both
selectedRecordStepIdandfieldNamesassertingbindToolsis called 0 times. That's the headline promise of PRD-775; the current deterministic test still runsselectFields(1 AI round).
2 participants
Part of the deterministic Get Data step work.
fixes PRD-775
What
The
read-record(Get Data) step can now resolve its source record deterministically from a build-time-configured stable BPMN step id (selectedRecordStepId), mirroring howtrigger-actionandload-related-recordalready work — instead of always asking the AI to pick among the available records.Changes
ReadRecordStepDefinitionSchema.preRecordedArgsgainsselectedRecordStepId(legacyselectedRecordStepIndexkept for back-compat).read-record-step-executor: whenselectedRecordStepIdis set, resolve viaresolveSourceRecordRef(revise-safe;WORKFLOW_START_STEP_ID→ base record). Falls back to legacy index / AI selection when unset.ServerWorkflowTaskGetData) + mapper forwardpreRecordedArgsforget-data.The
fieldNamespre-recorded path (build-time field selection) already existed and is unchanged.Tests
WORKFLOW_START_STEP_ID→ base record; unresolvable id → error.preRecordedArgs.selectedRecordStepId.Companion PRs: forestadmin-server (orchestrator), forestadmin (editor UI).
🤖 Generated with Claude Code
Note
Add deterministic source record resolution by step ID to
ReadRecordStepExecutorselectedRecordStepIdfield topreRecordedArgsinReadRecordStepDefinitionand theServerWorkflowTaskGetDatainterface, alongside the legacyselectedRecordStepIndexfor back-compat.preRecordedArgs.selectedRecordStepIdis set,ReadRecordStepExecutor.doExecutecallsresolveSourceRecordRef(selectedRecordStepId)and skips the interactive record selection flow entirely.mapTaskmapper in step-definition-mapper.ts now forwardspreRecordedArgsfromGetDataserver tasks to the resultingReadRecordstep definition.selectedRecordStepIdset will bypass record selection; an unmatched step ID results in an error rather than falling back to selection.Macroscope summarized b0f298f.