Skip to content

fix(automation): a screen the caller already answered no longer parks the run, and list_actions publishes a flow action's inputs - #15787

Merged
os-warren merged 12 commits into
mainfrom
claude/issue-15705-screen-headless
Sep 5, 2026
Merged

fix(automation): a screen the caller already answered no longer parks the run, and list_actions publishes a flow action's inputs#15787
os-warren merged 12 commits into
mainfrom
claude/issue-15705-screen-headless

Conversation

@os-warren

@os-warren os-warren commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Part of #15705

⚠️ Part of, not a closing keyword, deliberately. The card lists three expectations; this PR carries two of them. The third — a resume_run verb on the MCP tool surface — expands a published, authorizable surface and needs a maintainer ruling this card does not carry, so it is left open rather than closed by a merge. This PR does not make every screen flow completable over MCP: a call that omits the inputs still parks, and nothing on that surface can continue it.

Revision 4 (head 2041951a1, merged with origin/main at 6acb37eb9) — prose only, no source change. Round-3 review passed the behaviour and found two sentences to sharpen: the accepted-cost claim was imprecise in both directions (overstated for optional fields, understated for the caller-override shapes), and the boundary section named only one of the two constructions that still skip. Both corrected below, in the reviewer's own wording for the first. screen-input-contract.ts, screen-nodes.ts and every other source file are byte-identical to revision 3, so all behaviour readings from that revision stand unchanged.

The defect

An ai.exposed action whose target is a screen flow could be started over MCP and never finished. run_action seeds the flow's isInput variables from the caller's paramsseedFlowActionParams does that correctly, and it is untouched here — and the screen node suspended anyway. The MCP tool set has no resume verb, so the run parked forever. ai.exposed meant "the agent can invoke this", not "the agent can complete this", and the fallback an agent then takes (create_record + update_record to re-implement the flow's tail) bypasses whatever business rules the flow encapsulated.

Two independent causes, fixed independently.

Fix 1 — a screen the caller already answered does not pause

packages/services/service-automation. The pause decision read only "does the node declare fields" and the author's waitForInput flag; whether those fields were already bound was never consulted. It now is, through one pure verdict in screen-input-contract.ts — the module that already owns the screen field contract, so "present" cannot drift into two meanings between this seam and the resume door (both reach the same validateScreenInputs, where a blank string is absent).

The screen is treated as answered only when all of these hold:

  1. the caller named at least one of this screen's own fields. Without this leg an all-optional screen is vacuously satisfied and stops rendering for everyone — the loudest way to break the interactive path.
  2. every required field has a value from that caller. A value from the trigger record, from a prior node or from a declared defaultValue does not answer a required field; optional fields may come from anywhere.
  3. no visibleWhen the caller left unanswered. Enforced here and deliberately NOT on the resume door: the server has no rendered form and no collected values, so it cannot evaluate the predicate, and refusing costs only a pause — which is what the screen does today anyway. The resume door makes the opposite call for the opposite reason (enforcing a hidden field's required there dead-ends a run at Submit).

Two screens never take this path because they declare nothing to satisfy: a message-only screen (no fields), and any screen whose author wrote waitForInput: true — that flag is an explicit "show this", and a confirmation step is not something a params bag may skip. waitForInput: false stays the wrong tool for the headless case, exactly as the card says: it skips the form for interactive users too.

What "the caller supplied it" has to disprove

The params bag the engine receives is not the caller's bag. It carries two dispatcher seeds, and both must be refused before "the key is in params" can mean anything.

The record. seedFlowActionParams spreads the subject row into the bag, so every column is there whether the caller named it or not. Refused by: the record has no such key, or params holds a different value than the record's.

The row id, which is not a column. Both doors put the launched row's id into the bag under names no record leg can see. The trigger door (buildAutomationContext, serving the console's {recordId, objectName, params} POST) seeds recordId and the camelCase object-id alias — crmLeadId for crm_leadand sets no context.record at all; the actions door seeds those two plus the action's declared recordIdParam. Refused two ways:

  • by name, for the two the executor can derive: recordId, and the alias rebuilt from context.object exactly as the doors build it;
  • by value, for recordIdParam, whose name is action-level metadata this executor cannot see. The dispatcher seeds the same row id under every id key it knows, so the id is recoverable from the bag itself. Three candidates are read, and each is separately load-bearing — a record column can shadow any one of them, because seedFlowActionParams writes a key only if (seeded[key] === undefined) and the record spread came first:
    • params.recordId — the only candidate that survives a non-default recordIdField, which is precisely the shape that skipped in revision 2;
    • the alias's value — the reading that survives a record column named recordId;
    • record.id — which covers an object-less action, where no alias is derivable.

Every refusal above fails toward pausing.

⚠️ Accepted cost, precisely: a field is never treated as caller-supplied when it is named recordId or <object>Id, or when its value equals what the bag carries under recordId, <object>Id, or record.id (normally the launched row's id); a required such field is therefore always collected interactively, an optional one simply does not count as answering the screen.

That sentence is stated three times — here, in the changeset and in flows.mdx — and the three copies are identical, which is the point of stating a cost three times. (Revision 3's version was wrong both ways: it claimed an optional row-id-valued field is always collected interactively, when it continues with the value bound, and it omitted the caller-override and column-shadowing shapes, which are also refused.)

The boundary of inference

Three revisions found three layers of the same defect: this module infers what the caller meant, and every dispatcher seed it does not know about re-opens the question. What remains after the three candidates is the case where every candidate is shadowed at once, and it skips rather than pauses — said plainly, in both of the shapes that reach it. Both need a non-default recordIdField (so the row id is not record.id) and a recordIdParam naming a key the record lacks; they differ only in how the alias candidate is lost:

  • an object-less action, where no alias is derivable at all, whose record carries a column literally named recordId;
  • an object-bound action whose record shadows both recordId and the alias key itself — e.g. a crmLeadId column on crm_lead.

Inference cannot close either; only an explicit caller-provenance signal can, and that is an AutomationContext contract change deliberately scoped out of this card (open question B on #15705). A reviewer looked for a third construction and found none; no declaration in the repo has either shape.

Why minimal: no new authorable key, no contract change, no new AutomationContext field, no touch to seedFlowActionParams, and no change to any path that does not enter a flat screen node with fields. waitForInput keeps every meaning it had.

⚠️ One further limit: the record leg compares by identity, and identity does not survive the durable store. A run continued from suspended-run-store judges against a JSON.parsed context, so a later wizard screen colliding with a non-scalar column (array/object) of the trigger record can read as caller-supplied. Scalar columns and un-paused runs are unaffected. Filed as #15812; the remedy is value comparison rather than identity, deliberately not made here.

Fix 2 — list_actions publishes a flow action's inputs

packages/runtime. summarizeActionParams iterated action.params only. A type: 'flow' action almost never declares params — its input contract is the target flow's isInput variables, which is what the caller's bag binds into — so every flow action listed with no params key at all, while the tool description promised "its input parameters". The reported reproduction passed due_date where the flow declares dueDate because guessing was the only move available.

The flow's isInput variables are now surfaced in declaration order, carrying the label / type / required / options of the screen field that collects each one, gathered across all screen nodes (a wizard collects its inputs over several steps; stopping at the first screen would publish a subset while looking complete). required comes from the screen field alone — a flow variable has no required key, and inferring one from "declares no defaultValue" would invent a contract the author never wrote.

Why minimal: second, never first — an author's own action.params still wins outright, so this can only fill a silence. The projection is pure and the caller resolves the flow (domains/mcp.ts asks the automation service's already-declared optional getFlow), so a service without it, a target the registry does not hold, and a getFlow that throws all degrade to exactly today's answer.

Census: what else reaches the pause decision

Searched, not recalled (git grep for shouldPause, waitForInput, type: 'screen', registerNodeExecutor, executor.execute).

  • AutomationEngine.executeNode is the only call site of any executor.execute() the engine acts on (engine.ts), stated as such at the supportsPause guard. Everything below funnels through it.
  • Initial executionexecute()traverseNext / runRegion (loop and parallel region bodies).
  • ResumeresumeInternal continues with traverseNext past the node; only a map: correlation re-runs the node, which a screen never carries. A wizard's later screen is entered fresh, which is where the durable-store limit above applies.
  • The ADR-0018 node alias pathregisterNodeAlias's executor delegates to the canonical executor and returns its result, so an alias of screen inherits the new behaviour with the same guards.
  • supportsPause enforcement (refuseUndeclaredSuspension) gates every suspend: true; screen declares supportsPause: true / resumeAuthority: 'any' and is unaffected.
  • Run starters that can reach a screen: the MCP run_action bridge and the REST POST /api/v1/actions/... door (both through dispatchFlowAction), POST /automation/:name/trigger (buildAutomationContext — the console's door), record-change and scheduled triggers, and subflow / map child runs.
  • A record-change-triggered run pauses on the identity leg, not on absent params. The trigger sets params to the same object it sets as record (record-change-trigger.ts), so the bag is emphatically not empty; every key is identity-equal to the record's own value, which is what refuses it. Pinned as such, because that is the leg the durable-store limit weakens.
  • Non-runtime readers of waitForInputpackages/cli's i18n extractor (static analysis over flow metadata) and the spec config contract. Neither reads a runtime pause decision.
  • Untouched, as scoped: evaluateCondition, validateFlowExpressions, the predicate/ledger surfaces, seedFlowActionParams, packages/mcp, packages/spec, and content/docs/releases/.

Verification

Exit codes captured by redirect after a single command (cmd > log 2>&1; EXIT=$?) — never through a pipe, and never read off a multi-command line, where $? belongs to the last command; where a tool prints its own verdict line, that line is what is quoted. Exit 3 = NOT MEASURED, 2 = NOT WIRED, 124 = timeout kill, 137 = OOM — none read as a pass.

Revision 4 (head 2041951a1, prose only). The gate family re-derived at this head — no stale-tree warning, same 62 gates over the same 9 paths. 59 exit 0, and every gate that reads either changed file is among them: check:changeset-gate-self-tests, check:pm-governed-prose, check:doc-anchors, check:doc-authoring, check:docs-single-h1, check:docs-redirects, check:docs-image-tag, check:docs-audit-scope, check:corpus-claim-drift, check:published-readme-links, check:objectui-changeset, check:partof-closing-keyword, check:nul-bytes. check:docs-transcript-drift — the docs gate most directly on this diff — first answered exit 3 in this unbuilt worktree ("@objectstack/lint is not built … NOT a pass and NOT a finding"), so @objectstack/lint was built at --concurrency=1 and it re-ran exit 0 (405 pages, 4 declared transcript values). node scripts/check-adr-0087-registration.mjs --base origin/main --head 2041951a1exit 0 ("adds no declared-breaking changeset"), with --self-testexit 0 as its control. The three remaining exit 3 = NOT MEASURED are check:dts-closure, check:dual-build-cjs-loads and check:published-readme-exports, all of which read built output of every workspace package; none is reachable from a prose diff, and CI builds the workspace.

Revision 3 (head c31fdc8c7) — behaviour readings, unchanged, because no source file moved since.

Suite Result
@objectstack/service-automation (full) 111 files / 1340 tests passed
@objectstack/runtime (full) 229 files / 3268 tests passed
eslint . --no-inline-config (repo-wide, not narrowed) exit 0
typecheck for both packages (incl. check:test-typecheck) exit 0

Green at earlier revisions and unaffected since: @objectstack/mcp 26 files / 289 tests, @objectstack/plugin-approvals 37 files / 665 tests, @objectstack/dogfood 131 files / 1019 tests + 3 skipped.

Mutation checks — 16, all at revision 3's head. Each leg proved on disk: the HEAD blob hash read before, the mutation refused unless git hash-object changed, the restore via git checkout HEAD -- ABSOLUTE_PATH proved by the hash returning to the HEAD blob and an empty git diff HEAD, under a trap … EXIT INT TERM with an absolute repo root.

One of these was a finding against this PR's own tests. The token pin alone is satisfied by any of the three row-id candidates, so dropping params.recordId — the exact line revision 3 was asked to add — left all 21 green. The pin covered the leg but not its parts. Three fixtures were added, each answerable by exactly one candidate, and each candidate now has its own red.

Mutation Red
drop the params.recordId candidate 1 — "only params.recordId can refuse this one" (was 0 before the fixtures were strengthened)
drop the alias VALUE candidate 1 — "only the alias VALUE can refuse this one"
drop the record.id candidate 1 — "only record.id can refuse this one"
drop the whole row-id value leg 5 — both recordIdParam pins and all three candidate fixtures
revert the row-id NAME refusals 2 — both trigger-door pins
revert the row-id refusal entirely 4 — every row-id pin
revert shouldPause to its pre-change form 4 — every "continues" pin
drop the record-provenance leg 1 — record columns colliding with screen field names
drop condition 1 (caller named a field) 1 — all-optional screen still pauses
let a bag override waitForInput: true 1 — its control
flip the visibility convention to the resume door's 1 — conditional required field keeps it interactive
remove the flow-params fallback 2 — both the projection and the wire pin
cut the getFlow wire only 1 — the wire pin alone
flip declared-params precedence 1 — an author's own params still win
drop the isInput filter 4 — including "keeps a non-input variable private"

No rebuild leg is claimed and none is owed: every mutated subject is reached through relative source imports from its test, and service-automation's vitest.config.ts declares exactly one alias (@objectstack/platform-objects), so nothing under test resolves through a package exports to dist. For the dogfood suite — which does resolve through dist — both packages were rebuilt and the new symbols confirmed present there before that suite ran.

Docs and changeset

content/docs/automation/flows.mdx gains two sections beside the existing screen-resume rules, and .changeset/screen-flow-headless-satisfaction.md is a minor on both packages. Both state the row-id refusal, the accepted cost (in the wording above, identical in both) and the durable-store limit; neither claims "a console run supplies none of the screen's fields". No content/docs/references/ or content/docs/releases/ edit; packages/spec untouched, so none of its generated artifacts move.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

@github-actions github-actions Bot added the size/l label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/runtime, @objectstack/service-automation, touching 13 documentable anchor(s).

29 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 6acb37eb9496c822655edcd056c5047b35b83865.

5 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 4 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 27 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 6acb37eb9496c822655edcd056c5047b35b83865packageMentionDocs.

Which tree this was computed on

This run read content/docs from 47f6166b0f0b783eb7f8919038a2aaee013dee2b — the merge of head 2041951a18e94ada28aeb46b81d0297b0e22d72c into base 6acb37eb9496c822655edcd056c5047b35b83865, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 47f6166b0f0b783eb7f8919038a2aaee013dee2b && git checkout 47f6166b0f0b783eb7f8919038a2aaee013dee2b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 6acb37eb9496c822655edcd056c5047b35b83865 2041951a18e94ada28aeb46b81d0297b0e22d72c && git checkout -B drift-repro 6acb37eb9496c822655edcd056c5047b35b83865 && git merge --no-ff 2041951a18e94ada28aeb46b81d0297b0e22d72c

node scripts/docs-audit/affected-docs.mjs --json 6acb37eb9496c822655edcd056c5047b35b83865

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 6acb37eb9496c822655edcd056c5047b35b83865 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

Clause-② contract review — PR #15787 (card #15705), head 3bd7f54b0

Tier: CONTRACT_REVIEW_TIER = 'claude-fable-5-1' (scripts/pm/dispatch-gates.mjs:9429). Evidence: override + self-report — the PM attests this Agent call carried an explicit model: fable override, and the reviewer's own system-prompt identity is claude-fable-5-1. Not an exact-match claim.

Verdict: PASS — with two noted limits of the provenance leg (details in §1). Neither changes the mainstream fix or any test; both make one published sentence overstated. Recommended: tighten that sentence, and file a follow-up for the one-line hardening. If the PM holds the "any interactive skip is blocking" bar literally, §1-X1 qualifies and the PM should hold for it — the facts are below either way.

Reviewed in a dedicated detached worktree at the head (/home/user/objectstack-review-15787, tree clean, 0 status lines at the end). Nothing pushed, no branch touched, no stash.

1. Provenance leg — driven, not read

Engine-level (AutomationEngine + installBuiltinNodes, screen node reached through executeNode), with the context shapes the real doors build:

Construction Door shape Result
Colliding primitive columns (subject, dueDate on the row), no caller params dispatchFlowAction: record + {...record, recordId, crmLeadId} paused
Caller genuinely overrides the colliding fields same continued, output carries the caller's values
Ambiguous — caller sends the same values the row holds same paused (resolves to not-caller)
Record-change trigger shape: params === record, same object (record-change-trigger.ts:499) trigger paused
Same, after a JSON.parse(JSON.stringify(ctx)) round-trip (primitive columns) trigger paused

Two collision classes the PR does not name, both measured skipping a screen with no caller input:

X1 — dispatcher-synthesized id keys. The console's flow handler POSTs /api/v1/automation/{name}/trigger with {recordId, objectName, params} (objectui useConsoleActionRuntime.tsx:511-553, RecordDetailView.tsx:891); buildAutomationContext (domains/automation.ts:80-115) puts recordId and <objectName>Id into params and sets no context.record. seedFlowActionParams does the same on the actions door (recordId, <objectName>Id, recordIdParam). None of these is a record column, so the record leg cannot disprove them and callerSupplied answers true. Measured: a screen whose only required field is recordId (+ optional notes), interactive, no caller params → not paused, run completed with {recordId:'lead_1'}; same on the actions door for crmLeadId. The bound value is the launched record's own id, so this loses an optional-field step rather than binding a wrong value; no example flow declares a screen field with one of these names (the examples declare recordId as an isInput variable only). recordIdParam is the same class — derived, not run.

X2 — identity does not survive the durable store. callerSupplied proves the record leg by Object.is. suspended-run-store.ts:673/691 persists context as context_json and restores it by JSON.parse; resumeInternal continues with run.context (engine.ts:5412-5423), so a later screen in a wizard, entered after a durable resume, judges against de-identified objects. Measured: actions-door context after a JSON round-trip, all-optional screen with field tags colliding with the array column tags: ['a','b'], no caller params → skipped, output {tags:['a','b']}; the identical context without the round-trip → paused. Primitive columns are unaffected (row 5 above). Narrow: actions door or record-change trigger + a store wired + a later screen + a non-primitive colliding column + no other required field on that screen. The store itself was not wired in the run — the transform it applies was.

Both overstate one published sentence (PR body, changeset, flows.mdx): "A console run supplies none of the screen's fields … including when the subject record carries a column named like one of them." The console does supply recordId and the doors alias it; identity does not hold across a durable resume. Suggested: (a) tighten the sentence; (b) follow-up hardening — treat the dispatcher-seeded id keys as non-caller, and/or compare by value rather than identity. Neither touches the fix's mainstream path.

2. The three conditions and the escape hatches — each verified independently

  • C1 (caller named one of this screen's fields): dev pin; mutation M2 reddens exactly it. Note: C1 counts !== undefined while presence uses isPresent, so {notes: ''} on an all-optional screen skips it (measured). Consistent with the resume door accepting an empty optional; an interactive run never sends it — noted, not a safety issue.
  • C2 (every required answered by the caller): required dueDate answered only by a declared defaultValue, caller named subjectpaused; answered by a prior assignment node → paused; optional notes taken from the row while both required fields came from the caller → continued with the row's value. Mechanism: only caller-supplied names enter the bag, so the variable's source is irrelevant by construction.
  • C3 (visibleWhen): the headless seam passes () => true, so every unanswered conditional required fires → pause. Dev pin; reading confirms.
  • Escape hatches: message-only (hasFields false → headless undefined → shouldPause = wantsPause, byte-identical to before); waitForInput: true (guard cfg.waitForInput !== trueheadless undefined; M3 reddens exactly this control); waitForInput: falsewantsPause false → pass-through, dev pin. Object-form screens return before the block.

3. The asymmetry with the resume door — sound

Both seams are the one validateScreenInputs → one isPresent; the only difference is the visibility probe argument (headless: () => true; resume: the evaluator, engine.ts:5661, where undefined/false leaves required alone). Measured: ' ' yields required at both seams; a required field behind an unevaluable visibleWhen is unenforced at resume and refuses headless. "Present" cannot drift — one function, one predicate, one call each.

4. Census — re-derived

  • executor.execute( in src: engine.ts:7407/7412 (inside executeNode, timeout and direct arms) and engine.ts:2462 (alias delegate, return target.execute(...), itself only reached through executeNode). Nothing else.
  • Resume: engine.ts:5420-5423map: correlation → executeNode re-run, otherwise traverseNext past the node. A screen mints no map:. Confirmed.
  • Subflow/map children re-enter engine.execute (map-node.ts:171-181: params = the author's input mapping, record: item when the item is a record) — same funnel.
  • waitForInput readers: spec zod, screen-input-contract.ts (docs), screen-nodes.ts, cli/i18n-extract.ts (static). One runtime reader. shouldPause: screen-nodes.ts only. One screen executor registration.
  • Alias path driven: registerNodeAlias('screen_legacy','screen') — supplied continues, unsupplied pauses.
  • One imprecision: "a trigger-started run carries no such params" — the record-change trigger sets params: isolatedRecord, deliberately the same object as record (record-change-trigger.ts:494-499). It pauses because of the identity leg, not because params are absent — and that identity leg is what §1-X2 shows is store-fragile. Outcome correct today; sentence imprecise.

5. Mutations — four reproduced, no rebuild

Harness: git hash-object before and after the edit; git checkout HEAD --; hash back to the HEAD blob and empty git diff HEAD; under trap … EXIT INT TERM.

Mutation Blob Red Restore
M1 drop the caller-provenance leg screen-input-contract.ts a988f82→5a68ca0 1 — record-columns control a988f82, diff 0
M2 drop condition 1 a988f82→022a101 1 — all-optional control a988f82, diff 0
M3 bag overrides waitForInput: true screen-nodes.ts db5be81→5ce9913 1 — explicit waitForInput: true control db5be81, diff 0
M4 cut the getFlow wire only domains/mcp.ts 3503c99→309deb1 1 — the wire pin; 11 others green 3503c99, diff 0

No-rebuild claim verified: test → ../engine.js / ./index.js./screen-nodes.js../screen-input-contract.js; runtime test → ./http-dispatcher.js./domains/mcp.js../action-execution.js — all relative; service-automation's only vitest alias is @objectstack/platform-objects. Empirically, all four reds appeared with no build step between mutate and run.

6. Fix 2 degradation paths — confirmed by reading and 12/12 pins

typeof automation?.getFlow === 'function' (service without it → undefined), try/catch (throws → undefined), registry nullflow ?? undefinedsummarizeFlowInputParams returns [] → no params key: today's answer, all three. Precedence if (out.length === 0) — an author's params wins outright. required: field?.required === true — from the screen field, never inferred from defaultValue. collectScreenFieldSpecs walks every screen node, first declaration wins. getFlow is optional on IAutomationService (automation-service.ts:529). judgeHeadlessScreen is not re-exported from the package index — internal surface.

7. Honesty audit — confirmed

  • Both gates observed exit 3 in this partially built worktree, each printing PREREQUISITE NOT MET (EXIT_PREREQUISITE_NOT_MET = 3, EXIT_PREREQ = 3 in the scripts). NOT MEASURED; not counted as passes by the dev.
  • plugin-approvals: with @objectstack/trigger-record-change unbuilt → Failed to resolve entry for package "@objectstack/trigger-record-change", 1 failed file / no tests — the reported shape, reproduced. After turbo build --filter='@objectstack/plugin-approvals...'37 files / 665 tests passed. Environmental diagnosis confirmed.

Re-run here: @objectstack/service-automation 110 files / 1317 tests passed; @objectstack/runtime 228 files / 3255 tests passed (vitest exit 0, output captured to file — a first attempt whose head-terminated pipeline hid the summary was discarded, not counted).

NOT MEASURED by this review: @objectstack/mcp and dogfood suites, eslint, typecheck, the 60 green gates, ADR-0087 (PM verified exit 0); the recordIdParam variant of X1 (derived only); the DB-backed store end-to-end (X2 was driven by applying the store's own JSON transform to the context, not by wiring the store).


Reviewer: domain:services PM seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909), clause-② contract review.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

…aking

Both dispatch doors put the launched row's id into params under names that
are not record columns — recordId and the camelCase object-id alias on the
trigger door (which sets no context.record at all), plus the action's declared
recordIdParam on the actions door. A screen field named like one of them read
as caller-supplied on a run that supplied nothing, so an interactive console
launch could skip the screen. Refuse the two derivable names outright and the
third by value (identical to the row id).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
The trigger door seeds recordId and the camelCase object-id alias into params
and sets no context.record, so 'a console run supplies none of the screen's
fields' was not true as written; and a record-change trigger sets params to the
SAME object as record, so it pauses on the identity leg rather than on absent
params. Also states the durable-resume gap: a JSON-rehydrated context loses the
identity a non-scalar column's comparison relies on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

Copy link
Copy Markdown
Collaborator Author

Clause-② contract review, round 2 — PR #15787 (card #15705), head 3ef5e6ccc (merge-base 3508869ff)

Tier: CONTRACT_REVIEW_TIER = 'claude-fable-5-1' (scripts/pm/dispatch-gates.mjs:9429). Evidence: override + self-report — the PM attests this Agent call carried an explicit model: fable override, and the reviewer's own system-prompt identity is claude-fable-5-1. Not an exact-match claim.

Verdict: PASS on items 1–3 and 5–7; item 4 does NOT pass as stated. X1 is closed on both doors and the feature still works for a genuine caller; the per-leg attribution is exactly as the PR says; the three prose corrections landed; the merge preserved the file set. But the residual the PR declares as "pause-only" skips when driven — one line closes it, plus that one sentence. Details in §4.

Same detached worktree, moved to the new head (tree clean, 0 status lines at the end). Round-1 findings not re-run, per the brief. Nothing pushed, no branch touched, no stash, no build run (contended box — every reading below used the round-1 dist; limit stated at the end).

1. X1 re-driven — closed on both doors

Engine-level, real AutomationEngine + installBuiltinNodes, the doors' actual context shapes (buildAutomationContext for the console: no context.record, params = body + recordId + <object>Id; seedFlowActionParams for the actions door), no caller params in every row:

Screen's only required field Door Result
recordId (+ optional notes) console / trigger paused
crmLeadId (the alias) console / trigger paused
crmLeadNoteId for object crm_lead_note (multi-underscore alias, derived as the doors derive it) console / trigger paused
recordId + crmLeadId actions paused
leadRef carrying record.id (the recordIdParam shape, value leg) actions paused

The dev's 6 new pins pass (20/20 in the file). The alias derivation in callerSupplied is the same /_([a-z])/g → upper-case + Id both buildAutomationContext and seedFlowActionParams use, keyed off context.object, which both doors set exactly when they seed the alias.

2. The feature is not disabled

Genuine caller supplying subject + dueDatecontinues, output carries the caller's values — on the console door and on the actions door (where subject also collides with a row column). The dev's own "trigger door: a genuine caller param still satisfies" pin passes. One pause-only cost to note, not in the PR text: a screen field literally named recordId is now permanently interactive — a genuine caller sending recordId: 'lead_1' still pauses (measured). Correct direction; worth a sentence.

3. Per-leg / per-door attribution — confirmed exactly

Harness as in round 1 (git hash-object before/after; git checkout HEAD --; hash back to HEAD blob ab91228 and empty git diff HEAD; under trap … EXIT INT TERM):

Mutation Red Survives
drop only the two derivable name refusals 2 — both trigger-door pins the actions-door pin (on the value leg) and the recordIdParam pin
drop only the value leg 1 — the recordIdParam pin both trigger-door pins and the actions-door pin
revert the row-id refusal entirely 4 — all of the above

So the honest reading holds: only the name legs cover the record-less trigger door; the actions door is additionally guarded by value. If the name legs were ever removed, the actions door would stay guarded and the trigger door would not — and the two trigger-door pins are what would catch it.

4. The declared residual — it skips, not pauses

PR body: "recordIdParam naming a key the record lacks while recordIdField names a non-id column is the one row-id shape the value leg cannot see. Pause-only, like the rest."

Driven with the authentic bag (seedFlowActionParams({recordIdField:'token', recordIdParam:'sessionToken'}, {record:{id:'lead_1', token:'tok_9', …}}){…record, recordId:'tok_9', crmLeadId:'tok_9', sessionToken:'tok_9'}, printed from the real producer): a screen whose only required field is sessionToken (+ optional notes), interactive, no caller params → not paused; run completed with {sessionToken:'tok_9'}. Mechanism: recordId/crmLeadId are refused by name, but sessionToken is not the alias, its value 'tok_9' is not record.id ('lead_1'), and the record lacks the key → callerSupplied answers true. The failure direction of a missing refusal is skip; only the refusals themselves fail toward pausing.

Controls: the same shape with the default recordIdField (sessionToken:'lead_1') → paused (value leg); recordIdField:'token' + recordIdParam:'token' (a key the record has) → paused (record leg).

Realism: no declaration in the repo has the residual shape. sys_session pairs recordIdField:'token' with recordIdParam:'token' (record leg refuses); every other recordIdParam (sys_team, sys_invitation, sys_member, sys_user, sys_organization, the examples) uses the default recordIdField (value leg refuses). So there is no live instance — but the sentence is false in the direction the acceptance rule turns on.

Smallest close: in callerSupplied, also refuse a value Object.is-equal to params.recordId — the dispatcher seeds recordId with the actual row id under any recordIdField, so the recordIdParam key always carries that same value; a genuine caller sending the row id as a screen value only loses the skip (pause-only). Keep the record.id comparison too (a caller can override params.recordId). Then correct the sentence. Pin: the token/sessionToken shape above.

5. The three prose corrections — verified

  • (a) "a trigger-started run carries no such params" → the body now credits the identity leg ("sets params to the same object it sets as record … every key is identity-equal"), and it is pinned (record-change trigger shape: params IS the record, and it still pauses, asserting company is in the bag).
  • (b) "a console run supplies none of the screen's fields" — zero occurrences at HEAD in the changeset and flows.mdx; the body's replacement section names both seeds and the record-less trigger door correctly.
  • (c) X2: the durable-store limit is stated in the body, the changeset, flows.mdx and the callerSupplied doc comment (scalar columns and un-paused runs unaffected; remedy is value comparison; filed as service-automation: a caller-provenance check that proves its negative by Object.is is defeated by the durable store, which restores the context through JSON.parse #15812). The "Interactive runs are unchanged" paragraph in the changeset and docs is immediately followed by the gap paragraph, so no reader gets the unqualified claim alone. The one sentence that now claims something false is the §4 "pause-only".

6. The origin/main merge — clean on every axis

git show --remerge-diff is empty (no edits beyond auto-merge). Diff against the origin/main parent = exactly the PR's 9 files. Diff against the PR-tip parent = 104 files = main's own 24-commit set (104). HEAD^2 = the new merge-base 3508869ff. The PR's content diff against the new base is byte-identical to the pre-merge diff against the old base (0 lines of difference — main touched none of the 9 files). No sibling file was attributed to this branch.

7. Honesty spot-checks — confirmed

  • check:dual-build-cjs-loads and check:published-readme-exports observed exit 3 at the new head, each printing PREREQUISITE NOT MET. NOT MEASURED.
  • "Cannot find package @objectstack/spec/contracts" in a fresh worktree: engine.ts:14 value-imports RESUME_AUTHORITY_SERVICE from @objectstack/spec/contracts, which service-automation resolves through dist (its only vitest alias is @objectstack/platform-objects). Unbuilt-closure trap, not a real failure — the dev's reading is right.

Re-run here: @objectstack/service-automation full suite 110 files / 1323 tests passed (vitest exit 0, output captured to file, 0 FAIL lines).

Limits / NOT MEASURED by this round: every reading used the round-1 dist (built at the pre-merge base) — main's merge touched packages/spec/src/automation/flow.zod.ts only to add a duplicate-edge-id superRefine, immaterial to these readings, and no build was run on the contended box; @objectstack/runtime not re-run (no PR-side runtime change between the two heads — the PR-side diff of packages/runtime is empty); eslint, typecheck, the 60 green gates, @objectstack/mcp / dogfood / approvals suites; X2 (#15812) not re-driven; round-1 items not re-run, per the brief.


Reviewer: domain:services PM seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909), clause-② contract review, round 2.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

…ord.id

A non-default recordIdField makes the seeded row id a column other than id,
so an action's declared recordIdParam gave that value a third key the record
does not carry: not a column, not record.id, not a derivable name. It read as
caller-supplied and the screen was SKIPPED, not paused, on a launch that
supplied nothing. The dispatcher seeds the same row id under every id key it
knows, so params.recordId (and the camelCase alias) recover it without knowing
the action-level name.

Also states the accepted cost: a screen field named recordId or the object-id
alias is always collected interactively.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
The token pin is satisfied by any of the three row-id candidates, so on its
own it pinned the leg but not its parts: dropping params.recordId alone left
all 21 green. Three fixtures now give each candidate a case only it can
answer, by letting a record column shadow the other seed keys.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y

Copy link
Copy Markdown
Collaborator Author

Clause-② contract review, round 3 — PR #15787 (card #15705), head c31fdc8c7 (merge-base cc5b3dd0c)

Tier: CONTRACT_REVIEW_TIER = 'claude-fable-5-1' (scripts/pm/dispatch-gates.mjs:9429). Evidence: override + self-report — the PM attests this Agent call carried an explicit model: fable override, and the reviewer's own system-prompt identity is claude-fable-5-1. Not an exact-match claim.

Verdict: PASS. The round-2 residual is closed on the door that carries it and on the door that disproved revision 1; a genuine caller still continues; each of the three row-id candidates is separately load-bearing under mutation; the boundary is stated as a skip. Two findings to carry, neither of which makes a published behaviour claim false in the unsafe direction: the accepted-cost sentence is imprecise both ways (§2), and the boundary has a second construction the section does not name (§4). Both are one-sentence edits; I would not hold the PR for them, and I say why.

Scope: only what revision 3 changed. Rounds 1–2 stand. Same detached worktree at the new head; tree clean at the end; nothing pushed, no branch touched, no stash. One build this round: @objectstack/spec alone at --concurrency=1, because main's second merge made engine.ts import structuralConditionRefusal, which my pre-merge dist lacked (see limits).

1. The residual — closed, end to end

The shape that skipped in round 2 (recordIdField: 'token' + recordIdParam: 'sessionToken', a key the record lacks) is seeded only by seedFlowActionParams — i.e. the actions door; buildAutomationContext (the console door) seeds recordId and the alias and nothing else. So the residual lives on the actions door, and the console door is the one that disproved revision 1. Driven on both, engine-level, no caller params:

Construction Door Result
only-required sessionToken (+ optional notes), authentic bag {…record, recordId:'tok_9', crmLeadId:'tok_9', sessionToken:'tok_9'} actions paused
only-required sessionToken console / trigger paused (the key is never seeded there)
only-required recordId console / trigger paused (name)

Mechanism confirmed by reading: params.recordId carries the dispatcher's row id under any recordIdField, so 'tok_9' is now Object.is-equal to a candidate and refused. The dev's 24 pins pass at the new head.

2. The widening — sound; the stated cost is imprecise in both directions

Adding the alias's value and record.id as candidates fails toward pausing on every shape I could build; on the merits it is fine. Feature alive: a genuine caller supplying subject + dueDate continues with its values on the actions door.

The cost claim (body, changeset, flows.mdx): "a screen field named recordId or the object-id alias is now always collected interactively, even from a headless caller, and so is any field whose value happens to equal the launched row's id." Measured:

  • Accurate for the central case: a required field whose value equals the row id, genuinely supplied → paused.
  • Overstated (safe direction): an optional field whose value equals the row id, with another required field supplied → continued, and the value bound ({subject:'x', parentId:'lead_1'} in the output). The same holds for an optional field named recordId. "Always collected interactively" is true only of required fields; an optional one is simply not counted toward condition 1.
  • Understated (pause-only, contrived): the refusal set is "equal to whatever the bag carries under recordId, the alias, or record.id", which is the launched row's id on every real door unless the caller or the schema redefines those keys. Three such shapes measured, all paused: console door with the caller overriding params.recordId = 'X' and sending ref: 'X'; console door with the caller overriding crmLeadId = 'Y' and sending parent: 'Y'; actions door where the record carries a recordId column 'shadow' and the caller sends ref: 'shadow'. None of those values is the launched row's id.

Why I do not read this as blocking: the sentence is true for every unmodified door; the shapes it omits require a self-inconsistent caller (overriding the id keys with a different value) or a column literally named recordId/<object>Id — the same shadowing the boundary section already describes — and every omitted case is a pause. Precise wording, one sentence: "a field is never treated as caller-supplied when it is named recordId or <object>Id, or when its value equals what the bag carries under recordId, <object>Id, or record.id (normally the launched row's id); a required such field is therefore always collected interactively, an optional one simply does not count as answering the screen."

3. Candidate discrimination — real, not asserted

All four, each proved on disk (git hash-object before/after), restored to HEAD blob 34585de with an empty git diff HEAD, under trap … EXIT INT TERM:

Mutation Red On exactly
drop the params.recordId candidate 1 "only params.recordId can refuse this one — the record shadows the alias key"
drop the alias VALUE candidate 1 "only the alias VALUE can refuse this one — the record shadows recordId"
drop the record.id candidate 1 "only record.id can refuse this one — object-less action, and the record shadows recordId"
drop the whole value leg 5 both recordIdParam pins + the three fixtures above

23 of 24 stay green under each single-candidate drop, so no candidate is answerable by another fixture. The disclosed "was 0 red before the fixtures" is consistent by construction — the token pin seeds 'tok_9' under all three keys, so any one candidate satisfies it — reasoned, not re-measured.

4. The boundary — stated as a skip; a second construction exists

The section says the remaining conjunction "skips rather than pauses — said plainly", and it does: object-less action, record carrying a recordId column, recordIdField: 'token', recordIdParam: 'sessionToken', authentic bag {…record, recordId:'shadow', sessionToken:'tok_9'} (no alias seeded — '', '*' and 'global' are the object-less keys) → run completed with {sessionToken:'tok_9'}. Not softened.

Finding — a second construction, same class: the alias candidate is lost not only when no object is bound but also when the record shadows the alias key itself. Object-bound crm_lead, record {id, token:'tok_9', recordId:'shadow', crmLeadId:'shadow2'}, same recordIdField/recordIdParam, authentic bag {…record, sessionToken:'tok_9'}run completed with {sessionToken:'tok_9'}. Controls: shadow only recordId on an object-bound action → paused (alias value); object-less with the default recordIdFieldpaused (record.id). So the boundary is "every candidate shadowed", of which object-lessness is one way to lose the alias and a <object>Id column is the other. The body's own sentence ("a record column can shadow any one of them") already implies it; the boundary paragraph should name it. Same inference limit, same skip direction, same remedy (an explicit provenance signal, scoped out); no real declaration has either shape. Not blocking on that basis.

No third construction found: every other seeded key is name-refused, the record leg refuses every column, and the console door seeds nothing the name legs miss.

5. Merge and honesty

  • Merge with origin/main at cc5b3dd0c: --remerge-diff empty; 9 files against the main parent; the PR's content diff is byte-identical across the merge (0 lines of difference).
  • Pins: 24/24 at the new head. Re-run here after the spec rebuild: @objectstack/service-automation full suite 111 files / 1340 tests passed (vitest exit 0, captured to file, 0 FAIL lines).
  • The dev's exit-code discipline (single command, redirect, $? immediately) is the right reading of the trap I fell into myself in round 1.

Limits / NOT MEASURED: @objectstack/runtime full suite not re-run (the PR's runtime files are unchanged in revision 3); eslint, typecheck, the 60 green gates and the new check:dts-closure, @objectstack/mcp / dogfood / approvals; the "0 red before the fixtures" claim (reasoned only); the two exit-3 gates not re-run this round (confirmed at both earlier heads). Every reading before the spec rebuild ran with structuralConditionRefusal undefined in dist — provably off the screen-judging path (registerFlow would otherwise have thrown) — and the pins and my constructions were re-run after the rebuild with the same results.


Reviewer: domain:services PM seat (session 03324ae2-0f5b-5ad2-8a2e-cf4aaff5a909), clause-② contract review, round 3.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y


Generated by Claude Code

…rding

The sentence was imprecise both ways: overstated, because an OPTIONAL row-id
valued field is not collected interactively — it continues with the value
bound, it simply does not count toward the caller-named condition; and
understated, because values equal to a caller-overridden recordId or alias, or
to a shadowing recordId column, are refused too. Replaced in both shipped
places with one wording, identical in each.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
@os-warren
os-warren marked this pull request as ready for review September 5, 2026 13:30

Copy link
Copy Markdown
Collaborator Author

PM verification of the revision-4 fix-up — undrafted and armed

Verified independently at head 2041951a18e94ada28aeb46b81d0297b0e22d72c before undrafting. No fourth review round: the change is prose only, and I checked that rather than accepting it.

"No source change" proved by blob hash, not by the diffstat. Against revision 3 (c31fdc8c7), each of the four source files the behaviour lives in is byte-identical:

file rev3 → rev4
service-automation/src/screen-input-contract.ts IDENTICAL
service-automation/src/builtin/screen-nodes.ts IDENTICAL
runtime/src/action-execution.ts IDENTICAL
runtime/src/domains/mcp.ts IDENTICAL

with .changeset/screen-flow-headless-satisfaction.md as the control in the same comparison — it reports CHANGED, so an all-identical result is a measurement rather than a broken comparison. ⚠️ Note the raw c31fdc8c7 → 2041951a1 diffstat shows 132 files; that is origin/main moving under the branch (merged at 6acb37eb9), not this PR's content. The PR's own merge-base diff is 9 files, +1148/−7 — revision 3's 9 files plus the +8/−6 of prose.

Fences on the merge-base diff, all 0: ^packages/mcp/, ^packages/spec/, content/docs/releases, packages/plugins/ — with service-automation = 3 as the control that the scan fires at all.

node scripts/check-adr-0087-registration.mjs --base origin/main --head 2041951a1…exit 0.

The two closes

1. The cost sentence is genuinely identical across both shipped files — extracted from each and compared: 345 characters, byte-equal, with a deliberately-impossible variant as the control returning 0 length. ⚠️ My first comparison reported a mismatch and was wrong: I had extracted to end-of-line, and the changeset's line continues with two further sentences that flows.mdx places elsewhere. Scoping the extraction to the sentence itself is what settles it. The wording is the reviewer's, not re-invented.

The half that mattered was the overstatement, and it was in the changeset, which feeds release notes: revision 3 claimed a row-id-valued field is "always collected interactively", when an optional one continues with the value bound. A reader would have expected a prompt they will not get. The understated half (values equal to a caller-overridden params.recordId/alias, or to a shadowing recordId column) is now covered too, though it was always the safe direction — every one of those pauses.

2. The boundary section names both constructions and calls both what they are. It now reads "it skips rather than pauses — said plainly, in both of the shapes that reach it", then separates them by how the alias candidate is lost: an object-less action whose record carries a column literally named recordId, and an object-bound action whose record shadows both recordId and the alias key (a crmLeadId column on crm_lead). ⛔ Neither is softened into a pause, and the remedy is unchanged — an explicit caller-provenance signal, which is #15705's open question B and the maintainer's.

What this PR's history is actually worth recording

Four passes, and each one found a construction the previous had not — X1 at the buildAutomationContext door, then a self-declared residual that measured as a skip rather than the pause it was described as, then a fixture set that could not tell its own three candidates apart, then a cost sentence wrong in both directions. ⭐ The dev's own reading of that pattern is the right one and it is in the PR body: this module infers what the caller meant, and every dispatcher seed it does not know about re-opens the question. That is an argument for the provenance signal, not against this PR — which ships the documented-boundary option and states the boundary plainly.

⭐ Worth keeping separately: the dev found the fixture defect against its own tests and reported it rather than quietly fixing it. Dropping params.recordId — the exact line revision 3 was asked to add — left all 21 pins green, because one pin was satisfiable by any of the three candidates. Had that gone unreported, the residual could have come back silently, which is the one thing those pins exist to prevent.

⛔ Still Part of #15705, not a closing keyword. The third expectation — a resume_run verb on the MCP surface — expands a published, authorizable surface and is not carried here; the card stays open as its carrier.

Undrafted and auto-merge armed.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/xl tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants