fix(workflow): load-related chooses the relation by its target collection#1630
Open
Scra3 wants to merge 12 commits into
Open
fix(workflow): load-related chooses the relation by its target collection#1630Scra3 wants to merge 12 commits into
Scra3 wants to merge 12 commits into
Conversation
…rd (PRD-442 #1) The operation audit-trail entry pointed at the run's trigger record, even when the step acted on a record loaded earlier in the run — possibly in another collection. Root cause: buildActivityLogArgs ran before doExecute (so the acted record wasn't resolved yet) and used context.collectionId + baseRecordRef. Record executors now emit the entry at the point of the agent call, via a new base helper withActivityLog(args, fn), targeting selectedRecordRef and its collection's numeric id (read from the live schema). CollectionSchema gains a required collectionId — the audit is not optional, so a missing id fails loud at the boundary (DomainValidationError); the orchestrator must include it in getCollectionSchema responses (coordinated deploy). Scope: bug #1 (wrong target) only. As a consequence of logging at the operation point, a confirmation step no longer logs before the write — the remaining #2 work (exactly-once / no-log-on-reject, MCP step) is a follow-up. The MCP executor still uses context.collectionId via the existing wrapper. fixes PRD-442 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…logOperation Move the activity-log machinery out of BaseStepExecutor into a new OperationStepExecutor (Base -> Operation -> Record/Mcp). It owns the `operation` descriptor, `logOperation(record, fn)` and the now-private `withActivityLog` helper, so the base class no longer carries any audit concern. McpStepExecutor now extends OperationStepExecutor and logs its tool call against the run base record via context.collectionId. RecordStepExecutor overrides operationCollectionId to resolve the acted record's own collection id. Relocate the activity-log tests from base-step-executor.test.ts to a new operation-step-executor.test.ts. fixes PRD-442 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Tighten operation typing: CreateActivityLogArgs now uses the lib's
ActivityLogAction/ActivityLogType (removes the `as ActivityLogAction`
cast); extract OperationDescriptor (Pick) reused by the MCP executor.
collectionId is now required on CreateActivityLogArgs.
- Drop the dead `errorMessage` param from ActivityLogPort.markFailed: the
server status endpoint only accepts { status }, so the message went
nowhere. The failure cause is still logged by BaseStepExecutor.
- Move the `executing` write-ahead marker inside the logOperation callback
(after createPending, before the side effect) in update/trigger/mcp so an
activity-log creation failure never leaves an orphan executing marker.
- Fix stale comment in base-step-executor + CLAUDE.md idempotency section.
- Reword the misleading MCP formatting-fallback log line.
- Tests: cross-collection activity-log target for update + trigger, log on
the load-related awaiting-input branch, no-orphan-marker on createPending
failure, and markFailed single-arg assertions.
fixes PRD-442
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the activity-log audit out of OperationStepExecutor into a dedicated
AgentWithLog collaborator that wraps AgentPort. Each data-access call now
emits one audit entry (pending -> success/failed); the action/type are
intrinsic to the method and the target (collectionId, recordId) is derived
from the call, so the per-executor `operation` descriptor and the
`operationCollectionId` hook are gone.
Idempotency stays in the executors. Write methods take a `beforeCall` thunk
run between createPending and the side effect, so the executor persists its
`executing` write-ahead marker in the correct order (createPending ->
executing -> write) without AgentWithLog knowing anything about idempotency.
MCP (whose side effect is a tool invocation, not an AgentPort call) uses the
generic `logged()` method.
Also restore the local-only `errorMessage` on markFailed: it never reaches
the server (status endpoint accepts only { status }) but is kept for the
adapter's diagnostic log line.
- delete OperationStepExecutor; RecordStepExecutor + McpStepExecutor extend
BaseStepExecutor directly
- extract shared loadCollectionSchema helper (reused by getCollectionSchema
and AgentWithLog collectionId resolution)
- getActionFormInfo / probe stay on the raw agentPort (intentionally not audited)
- new agent-with-log.test.ts (audit per method, beforeCall ordering,
createPending-throws, userMessage vs 'Unexpected error'); relocate the
audit suite out of the deleted operation-step-executor.test.ts
fixes PRD-442
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- AgentWithLog: replace LoggedOperation with AuditTarget (Omit<CreateActivityLogArgs,'renderingId'>), reused by logged() and audit() so the audit-arg shape stays anchored to the port type. - CLAUDE.md: update the idempotency section to reference AgentWithLog and the beforeCall thunk (the deleted OperationStepExecutor.logOperation). - tests: assert stepOutcome.error (audit-creation failure message) on the no-orphan-marker case; pin getSingleRelatedData (xToOne) audit action. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
resolveSchema fabricated a CollectionSchema on cache miss, including a buggy `collectionId: cached.collectionId` (cached is undefined in that branch). The fallback is unreachable in the normal flow — AgentWithLog resolves (and caches, via the shared SchemaCache) the schema right before delegating to the agent port — so throw a SchemaNotCachedError instead of returning a fabricated schema with a wrong primaryKeyFields/collectionId. - new SchemaNotCachedError (domain error) - agent-client-agent-port test: fallback case becomes a throw assertion Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the cache-or-fetch logic onto SchemaCache as getOrLoad(name, load): the cache owns "check cache, else load via the injected thunk, then store", while staying decoupled from WorkflowPort (the loader closure captures both the port and the per-step runId). Removes the free loadCollectionSchema helper; RecordStepExecutor.getCollectionSchema and AgentWithLog.resolveCollectionId both go through getOrLoad. Also pin the confirmation-flow audit behavior (PRD-442 #2 premature/duplicate symptom, fixed as a consequence of logging at the operation point): a rejected step emits no activity-log entry. Plus direct getOrLoad unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion A load-related step picked the source record first, by "which available record's collection best matches the request", then the relation on it. With records accumulated from prior load-related steps, "load the dvd X" latched onto an already-loaded dvd (collection matches) instead of the store that has the dvds relation — so it followed dvd→store and re-loaded a store. Fuse the two choices into one: gather every (record, relation) pair across available records and pick by what the relation LEADS TO. "Load the dvd X" now follows store→dvds regardless of accumulated dvds. preRecordedArgs (selectedRecordStepIndex / relationDisplayName) still pin source/relation; single candidate skips the AI call. The shared select-record heuristic is untouched for read/update/trigger (acting on an existing record of that type). NOTE: load-related unit tests still mock the old select-relation flow — to be migrated in a follow-up commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 new issues
|
The orchestrator sends a `title` on every step (ServerWorkflowStepBase.title) but the mapper dropped it. Carry it into the domain StepDefinition and inject it into buildContextMessage, so every AI call sees the step's human intent — which matters when the prompt is weak or empty (e.g. a load-related step whose prompt arrives as "Load " while the title says "Load the store"). - add optional `title` to shared StepDefinition fields - map `task.title` / `condition.title` in step-definition-mapper - buildContextMessage appends `Step title: "<title>"` when present Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion
Adapt the suite to the single `select-relation-to-follow` AI call (was
`select-record` + `select-relation`): mocks return `{ relation: <label> }`,
single-candidate schemas make no AI call, bindTools counts/names updated.
Adds a repro test: with account + a loaded store + a loaded dvd available,
"Load the dvd titanic" follows store→dvds (not a relation off the dvd).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (3)
🛟 Help
|
Add coverage for the diff lines flagged by the coverage gate: the step-title branch in buildContextMessage, and resolveTarget's requireRecordAtStepIndex (pinned source via selectedRecordStepIndex, plus the no-match error path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR review findings:
- a pinned relationDisplayName / selectedRecordStepIndex that matches nothing
now throws InvalidPreRecordedArgsError, not the misleading
NoRelationshipFieldsError ("no relations configured"); the latter is kept
only for the genuine zero-relations case.
- matchesRelation normalizes (case + separators) like findField, restoring
fuzzy parity for pre-recorded relation names.
- resolveTarget builds the RelationTarget straight from the chosen candidate's
field (targetFromCandidate) instead of re-resolving by displayName via
buildTarget — removes a redundant lookup and a same-displayName mis-resolve
hazard. RelationCandidate.field is narrowed to guarantee relatedCollectionName.
- tests: pinned-relation/source assertions made meaningful (executionParams +
source record among several), error userMessages asserted, and a test that
the step title reaches the select-relation-to-follow context.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scra3
force-pushed
the
fix/prd-442-activity-log-target
branch
from
8bb1ba6 to
b69167d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Stacked on #1628 (base
fix/prd-442-activity-log-target). Two distinct load-related improvements.1. Choose the relation by its target collection
A load-related step picked the source record first (« which available record's collection best matches the request »), then the relation. With records accumulated from prior load-related steps, « Load the dvd X » latched onto an already-loaded dvd (collection matches) instead of the store that has the
dvdsrelation → it followeddvd → storeand re-loaded a store.Fix: fuse the two choices into one over
(record, relation)pairs, picked by what the relation leads to. New AI toolselect-relation-to-follow(options labeledSource → relation (→ target)) replacesselect-record+select-relationfor load-related.preRecordedArgs(selectedRecordStepIndex/relationDisplayName) still pin source/relation; a single candidate skips the AI call. The sharedselect-recordheuristic is untouched for read/update/trigger.2. Thread the step title to the AI
The orchestrator sends a
titleon every step (ServerWorkflowStepBase.title) but the mapper dropped it. It's now mapped intoStepDefinitionand injected intobuildContextMessage, so every AI call sees the step's intent — important when the prompt is weak/empty (e.g. a load-related prompt arriving as « Load » while the title says « Load the store »).Verified live (
_example)account→ load store → read store name → load dvd "Celine dion" → load dvd "titanic": the last step followsstore#6 → dvds(GET /store/6/relationships/dvds, candidates incl. Titanic), no moredvd → storetrap.Tests
select-relation-to-followcall + a repro test (account + loaded store + loaded dvd → "Load the dvd titanic" follows store→dvds, assertsgetRelatedData(collection: 'store', relation: 'dvds')).titleis carried through.🤖 Generated with Claude Code
Note
Fix load-related step to select relations by target collection
LoadRelatedRecordStepExecutorrelation selection to build candidates from all available records and choose by target collection name, rather than by source record resemblance.resolveTarget()to consolidate record and relation resolution; skips AI when only one eligible candidate exists.SELECT_RELATION_SYSTEM_PROMPTto instruct the AI to choose relations by their target collection.titlefield to step definitions and includes it in the system context message built byBaseStepExecutor.selectedRecordStepIndexnow throwInvalidPreRecordedArgsErrorinstead of falling through silently; the AI tool for relation selection is renamed toselect-relation-to-followand requires choosing from explicit labeled options.Macroscope summarized c264c51.