Skip to content

fix: load related record#1610

Open
EnkiP wants to merge 11 commits into
feat/prd-214-server-step-mapperfrom
fix/load-related-record
Open

fix: load related record#1610
EnkiP wants to merge 11 commits into
feat/prd-214-server-step-mapperfrom
fix/load-related-record

Conversation

@EnkiP
Copy link
Copy Markdown
Member

@EnkiP EnkiP commented May 29, 2026

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

Note

Rework load-related-record step to support field selection, candidate ranking, and x-to-one fetching

  • Redesigns the LoadRelatedRecordStepExecutor to produce richer pendingData containing availableFields, suggestedField, availableRecordIds (with optional referenceFieldValue), and suggestedRecord instead of the previous flat shape.
  • Adds getSingleRelatedData to AgentPort and AgentClientAgentPort for fetching x-to-one (BelongsTo/HasOne) relations via parent-record projection rather than a list query.
  • Supports mid-step field switching: a patch with only fieldName (no userConfirmed) triggers candidate refresh and returns awaiting-input without completing the step.
  • Normalizes relatedCollectionName in getCollectionSchema to strip trailing target-key suffixes (e.g. store.idstore) and surfaces an optional referenceField from collection schemas.
  • Updates loadRelatedRecordPatchSchema HTTP validator to accept preview patches (fieldName-only) and rename namefieldName; selectedRecordId is only required when confirming with a relation override.
  • Risk: getRelatedData now requires callers to supply relatedSchema; any adapter calling it without the new argument will break at runtime.

Changes since #1610 opened

  • Updated comment in stripReferenceKey utility function to clarify that the related primary key is derived from the schema's primaryKeyFields rather than from a relatedPrimaryKey field [88bbefa]
  • Removed relatedPrimaryKey property from mocked field definitions across test cases in ForestServerWorkflowPort.getCollectionSchema test suite and updated assertions to verify removal of arbitrary unknown field keys [88bbefa]

Macroscope summarized 7b222de.

@EnkiP EnkiP changed the base branch from main to feat/prd-214-server-step-mapper May 29, 2026 07:29
@qltysh
Copy link
Copy Markdown

qltysh Bot commented May 29, 2026

All good ✅

Comment thread packages/workflow-executor/src/executors/load-related-record-step-executor.ts Outdated
Comment thread packages/workflow-executor/src/adapters/agent-client-agent-port.ts
Comment thread packages/workflow-executor/src/adapters/agent-client-agent-port.ts
@qltysh
Copy link
Copy Markdown

qltysh Bot commented May 29, 2026

Qlty


Coverage Impact

Unable to calculate total coverage change because base branch coverage was not found.

Modified Files with Diff Coverage (4)

RatingFile% DiffUncovered Line #s
New Coverage rating: A
...ow-executor/src/executors/load-related-record-step-executor.ts100.0%
New Coverage rating: A
packages/workflow-executor/src/http/pending-data-validators.ts100.0%
New Coverage rating: A
.../workflow-executor/src/adapters/forest-server-workflow-port.ts100.0%
New Coverage rating: A
...ages/workflow-executor/src/adapters/agent-client-agent-port.ts100.0%
Total100.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.

@hercemer42
Copy link
Copy Markdown

Claude has added a lot of comments, but your code is readable as is, so in my opinion they aren't necessary. I'd recommend to keep them to a minimum.

Comment thread packages/workflow-executor/src/types/step-execution-data.ts Outdated
Comment thread packages/workflow-executor/src/http/pending-data-validators.ts Outdated
Comment thread packages/workflow-executor/src/ports/agent-port.ts Outdated
Comment thread packages/workflow-executor/src/executors/load-related-record-step-executor.ts Outdated
Comment thread packages/workflow-executor/src/executors/load-related-record-step-executor.ts Outdated
Comment thread packages/workflow-executor/src/adapters/agent-client-agent-port.ts Outdated
Comment thread packages/workflow-executor/src/executors/load-related-record-step-executor.ts Outdated
@EnkiP EnkiP force-pushed the fix/load-related-record branch from 23d4ee8 to a47f15f Compare June 1, 2026 10:16
Comment thread packages/workflow-executor/src/executors/load-related-record-step-executor.ts Outdated
@EnkiP EnkiP force-pushed the fix/load-related-record branch from 9d4e394 to 6224c76 Compare June 1, 2026 11:27
// Preview patch (no confirm): fieldName alone is sufficient.
if (data.userConfirmed === undefined) return data.fieldName !== undefined;
// Confirm patch with relation override: selectedRecordId required.
if (data.fieldName !== undefined) return data.selectedRecordId !== undefined;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium http/pending-data-validators.ts:56

The .refine() validation on line 56 requires selectedRecordId whenever fieldName is provided, even when userConfirmed is false. This means sending { userConfirmed: false, fieldName: "someField" } incorrectly fails validation with a message about "confirming with a relation override" — but declining a step is not confirming. The check should include data.userConfirmed === true to match the documented intent.

Suggested change
if (data.fieldName !== undefined) return data.selectedRecordId !== undefined;
if (data.userConfirmed === true && data.fieldName !== undefined) return data.selectedRecordId !== undefined;
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/workflow-executor/src/http/pending-data-validators.ts around line 56:

The `.refine()` validation on line 56 requires `selectedRecordId` whenever `fieldName` is provided, even when `userConfirmed` is `false`. This means sending `{ userConfirmed: false, fieldName: "someField" }` incorrectly fails validation with a message about "confirming with a relation override" — but declining a step is not confirming. The check should include `data.userConfirmed === true` to match the documented intent.

Evidence trail:
packages/workflow-executor/src/http/pending-data-validators.ts lines 34-65 at REVIEWED_COMMIT. Line 54 only catches `userConfirmed === undefined`, line 56 applies to any defined `userConfirmed` (including `false`). The inline comment on line 55 says 'Confirm patch' but code doesn't check `userConfirmed === true`.

Comment thread packages/workflow-executor/src/adapters/agent-client-agent-port.ts Outdated
Comment thread packages/workflow-executor/src/types/step-execution-data.ts Outdated
Comment thread packages/workflow-executor/src/types/step-execution-data.ts Outdated
Copy link
Copy Markdown
Member

@Scra3 Scra3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a blocking bug in the xToOne relation projection (getSingleRelatedData).

Comment thread packages/workflow-executor/src/adapters/agent-client-agent-port.ts Outdated
Enki Pontvianne and others added 4 commits June 2, 2026 15:46
…drop unused export

- Add a test for resolveFromSelection when the confirmed relation is not in
  availableFields (stale/renamed) — asserts the step errors and neither
  getRelatedData nor saveStepExecution is called.
- restoreFieldNames is only used inside the agent-client adapter now; drop its
  unused export.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getSingleRelatedData projected both the PK and the reference field on the
relation (fields[store]=id,name), which the agent's parseProjection can't split
into two sub-fields — it 400s with "Invalid projection: '<relation>.id,name'".

The JSON:API serializer fills the linkage `id` regardless of projection, so we
only need ONE sub-field: the requested reference field (for display), else a
single PK field just to pull the relation into the response.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lainer port error

- The orchestrator sends relatedCollectionName as Forest's `collection.targetKey`
  reference (e.g. "store.id"), which made getCollectionSchema("store.id") 404 and
  broke xToOne relation loading. Strip it to a plain collection name in the
  forest-server-workflow-port adapter (transport concern stays in the adapter; the
  zod schema stays a clean domain contract). The target key lives in relatedPrimaryKey.
- WorkflowPortError.userMessage no longer leaks internal jargon ("orchestrator") nor
  claims a connection cause (it also wraps 4xx like "collection not found"):
  "This step couldn't be completed. Please try again, and contact your administrator
  if the problem continues."

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Scra3 Scra3 force-pushed the fix/load-related-record branch 2 times, most recently from 0eb03d1 to 2799340 Compare June 2, 2026 13:53
…handling

- adapter: look up xToOne linkage under the camelCased relation key
  (agent-client camelCases JSON:API keys); snake_case relations like
  billing_address no longer resolve to null / false "not found"
- awaiting-input (Branch C): an empty related-record list now returns an
  empty candidate list with no suggestedRecord instead of erroring; the
  user can switch relation. Fully-automated (Branch B) still errors —
  it cannot load nothing
- buildTarget throws instead of masking a missing relatedCollectionName
- reword stale Branch B dispatch comment

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Scra3 Scra3 force-pushed the fix/load-related-record branch from 2799340 to 7b222de Compare June 2, 2026 13:57
The relatedPrimaryKey field was removed from the schema/RelationTarget
(dead field; the related PK comes from primaryKeyFields[]). Clean up the
leftover mentions: a stale comment in forest-server-workflow-port and
raw-input occurrences in tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants