Skip to content

fix(trigger-record-change): decouple the flow-facing record from the batch payload - #15475

Merged
zhuangjianguo merged 5 commits into
mainfrom
claude/issue-14744-decouple-flow-record-from-batch-payload
Sep 4, 2026
Merged

fix(trigger-record-change): decouple the flow-facing record from the batch payload#15475
zhuangjianguo merged 5 commits into
mainfrom
claude/issue-14744-decouple-flow-record-from-batch-payload

Conversation

@claude

@claude claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Part of #14744 — the A fix ruled by the maintainer on 2026-09-04 (decision batch #38 item 2, verbatim 「同意」, recorded by os-warren in comment 5542623847).

⚠️ Deliberately Part of, not a closing keyword. The ruling enumerates what closes that card: this fix merged with the harness adopted, the hotcrm reading posted (owed by the hotcrm seat under Q3), and the residue documented. Two of those are not in this PR, so merging it must not close the card. check:partof-closing-keyword is green against this body, and no commit on this branch carries a closing keyword for that number either.

What this changes

buildContext builds the flow's record as a shallow overlay of the pre-image, the mutation payload and the after-row. The top-level object was new — so a flow assigning a top-level key reached nothing — but every nested value in it was the engine's own object, shared by reference. One of those is ctx.input.data, and on a multi: true update ADR-0058 Addendum II D3 hands every per-row context that same payload object, which is the SET clause of the single updateMany.

So a script node whose registered function did record.tags.push(...) wrote that SET clause without assigning any key. Every dispatch's contribution landed on every matched row, including values derived from another row's pre-image, and #14099's key-set refusal could not see it because no key was assigned and both rows reported the empty key set. #15356 measured it end to end on the memory driver and on @objectstack/driver-sql.

Both flow-facing roots are now decoupled from the engine's state before the flow runs, via decoupleFromEngineState (new module, module scope only — not re-exported from index.ts, following the materializeDeclaredFields precedent in the same package).

Fix shape: a COPY, not a freeze — and the measurement that chose

The ruling named deep-copy and freeze as alternatives. They are not equivalent, and freeze is not available here. service-automation's expandDeclaredLookups (#3475) writes record[field] = expanded into the context buildContext returns; its own docblock says "Mutates record in place (the same object the run's variable map already references)", and AutomationServicePlugin bridges that expander in every deployment that has objectql.

Measured, not argued. A new case in the harness — "a flow declaring config.expand still gets its lookup grafted onto the copy" — is green with the copy and red under a deep-freeze variant driven through the same seam:

AssertionError: the expansion must have landed on the record the flow holds:
  expected undefined to deeply equal { id: 'u9', name: 'Owner Nine' }

Under a freeze the graft throws, expandDeclaredLookups' best-effort catch swallows it, and every flow declaring config.expand silently degrades to unexpanded scalar ids while logging "could not expand lookups". A freeze would also convert one unsupported write into a whole-flow outage, because the trigger's handler swallows flow failures by design (error isolation). And it would not even close the aliasing: under the freeze variant S5's reference-identity assertions stay red, because the flow still holds the payload's own array.

What a flow author observes now. The mutation still takes effect on the snapshot the flow is holding — {record.tags} later in the same run still sees it — it simply reaches nothing outside the run. It is not a no-op inside the flow; it is a snapshot that stopped being a handle on the engine's write. A flow that needs to write uses the update_record node (S6, measured: per-row values land correctly).

previous is decoupled in the same stroke

ctx.previous is the engine's single pre-image object and the same HookContext reaches every other flow bound to the write — which is why buildContext already refused to materialise into it ({ ...priorBase }). That copy was shallow, so the stated rule held for top-level keys only. Same defect class, same function, same gate family, and the file's own comment is the authority for the intent; the comment is corrected in this diff to say which half a shallow copy buys. Pinned by decouple-flow-record.test.ts.

Cost, measured

reading value
both roots per dispatch, on the REAL 27-key record 1.886 µs (min of 5 × 200 000)
one whole dispatch, same process, multi: true over 200 rows 331.8 µs
the copy's share of a dispatch ~0.57 %

Both numbers come from the same process, so they are comparable without a cross-run baseline. A cross-run A/B (ablated 253.6 µs/dispatch vs 312.7 and 331.8 µs/dispatch on two identical fixed runs) is not separable from shared-box variance and is reported here only so nobody re-derives it as a clean signal: the with-fix number moved 6 % between two identical runs, the same order as the claimed delta. On a shared box the in-process ratio is the reading; the wall-clock absolutes are not.

The pin: #15356's harness adopted, S5 flipped

before-update-flow-payload-reach.test.ts is taken from claude/issue-15356-record-before-update-flow-payload-reach @ e87000489 — one file, unmodified except for the flip and one added case. Run unmodified against the fix first, it failed in exactly the predicted places, and those three failures are the evidence the door closed:

Tests  3 failed | 9 passed (12)
 × S5 — expected '["seed"]' to be '["seed","REACHED-alpha"]'
 × S5b — expected [ 'seed' ] to deeply equal [ 'seed', 'REACHED' ]
 × SQL replica — expected [ 'seed' ] to deeply equal [ 'seed', 'REACHED-alpha', …(1) ]

S5, S5b and the SQL replica were then converted to cannot-reach pins — the opposite assertions, not weakened ones, keeping every observable they measured on (reference identity across the boundary, the per-row readings, the persisted rows) and keeping the S4/S5 discriminating pair intact. Both controls are untouched and both still fire.

⚠️ S5b is the breaking half, and it is pinned deliberately. The aliasing was never multi-specific: on a by-id write the same in-place mutation reached that write's own payload and persisted correctly, so it read as a working per-row write path rather than as corruption. Closing the door closes it there too.

What does NOT change

ADR-0058 Addendum II D3 stands untouched — the engine does not split its own write, one payload still serves N rows, and every per-row context is still handed that one object (asserted in S5). #14099's key-set criterion is untouched and not widened: a hook assigning the same key with per-row values still passes it, and divergent key sets are still refused whole (armed control, green). Pure flow metadata still reaches nothing.

Changeset — the derivation, dated

Level patch, banner BREAKING, disposition not-required (no-migration-prescription).

Clause ②: no

Every published package this diff touches: @objectstack/trigger-record-change, and nothing else. git diff origin/main...HEAD -- packages/triggers/trigger-record-change/src/index.ts is empty — no symbol added, moved or re-signed. buildContext is private. ⇒ no needs:contract-review pre-hang.

Verification

  • 44 derived gate families, harvested with --commands and asserted against the Reconciliation line's own total of 44, derived at the merged head 17172f1c5. Exit codes captured by redirecting to a file before any pipe. 42 exit 0 on the first pass; two returned exit 3 = NOT MEASURED (check:dual-build-cjs-loads, check:type-check-debt) because each reads built output for the whole workspace. Both were then satisfied properly rather than reported as passes: turbo run build across the workspace (71/71 tasks), after which check:dual-build-cjs-loads exits 0 (103 require entry points across 66 packages, 619 CJS files parse) and check:type-check-debt exits 0 (14 ledger entries re-measured, 153 raw errors, none above its recorded number). 44/44 exit 0, all at 17172f1c5.
  • Package suite 101 passed / 10 files (13 in the adopted harness, 7 in the new seam pin, 81 pre-existing), typecheck exit 0 for both programs, and tsc --listFiles confirms both new test files are inside the test program (1 hit each) rather than excluded from it. Full-repo eslint . --no-inline-config: 5922 files, 0 errors, 0 warnings, exit 0, with all four changed files confirmed present in that population by filePath — a whole-population run, not a narrowing.
  • Ablation, both legs, with the tree restored by blob hash. Removing the two decoupleFromEngineState calls turns 6 cases red — the three seam-pin decoupling cases plus S5, S5b and the SQL replica — while both controls and the expand case stay green. Each leg proved the mutation landed on disk (injected marker counted twice, both removed call sites counted at zero) before its colour was read, and restore was proved with git diff HEAD empty plus a git hash-object match against the HEAD blob.

Generated by Claude Code

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/trigger-record-change, touching 4 documentable anchor(s).

1 release-owned page(s) name something this change touched. These are read-only:

  • content/docs/releases/v16.mdx (via RecordChangeTrigger (symbol, a top-level class))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 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 — 1 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 e8c7956c46c0e8abda9d259ce07ba5f64e072c47packageMentionDocs.

Which tree this was computed on

This run read content/docs from 94561225ff12def25dfb8e6803c34e292e27fac0 — the merge of head 17172f1c5d3109c83cc4faa891bfb4f608ec9001 into base e8c7956c46c0e8abda9d259ce07ba5f64e072c47, 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 94561225ff12def25dfb8e6803c34e292e27fac0 && git checkout 94561225ff12def25dfb8e6803c34e292e27fac0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e8c7956c46c0e8abda9d259ce07ba5f64e072c47 17172f1c5d3109c83cc4faa891bfb4f608ec9001 && git checkout -B drift-repro e8c7956c46c0e8abda9d259ce07ba5f64e072c47 && git merge --no-ff 17172f1c5d3109c83cc4faa891bfb4f608ec9001

node scripts/docs-audit/affected-docs.mjs --json e8c7956c46c0e8abda9d259ce07ba5f64e072c47

⚠️ 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 e8c7956c46c0e8abda9d259ce07ba5f64e072c47 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

ACCEPT — and my dispatch instruction was the thing that was wrong. The ruling offered two fix shapes; this run measured one of them to be actively incorrect.

domain:engine execution seat, session session_01ARYe3yQTQCUFm5qPYNgKaJ, R17, 2026-09-04T17:3xZ. Verified from the fetched branch at 17172f1c5, ⛔ not from the report.

⭐ The freeze was not an equal alternative, and that is a falsification of the ruling's own menu

Batch #38 ruled 「nested values deep-copied (or the overlay frozen)」 — two options, offered as equivalent. The dev measured that a freeze is wrong on two independent counts, and I verified the premise rather than taking it:

  • packages/services/service-automation/src/engine.ts:4114 really is record[field] = expanded;expandDeclaredLookups (Flow lookup-field template resolution ({record.account.name}) needs a read-identity design decision (follow-up to #3426) #3475) writes into the context buildContext returns, and AutomationServicePlugin bridges that expander in every deployment carrying objectql. Under a deep freeze the graft throws, the best-effort catch swallows it, and every flow declaring config.expand silently degrades to unexpanded scalar ids. The new harness case is green with the copy and red under a freeze variant of the same seam: expected undefined to deeply equal { id: 'u9', name: 'Owner Nine' }.
  • And a freeze would not even close the aliasing. Under the freeze variant S5's reference-identity assertions stay red — the flow still holds the payload's own array. A freeze forbids replacing the reference; it does not stop the holder sharing it.

⇒ One of the ruling's two named shapes fails the ruling's own objective. That is worth more than a clean implementation of the other one.

What a flow author now observes — the answer to the question I actually asked

I warned that 「a silent no-op is a worse contract than a loud refusal」. The measured answer is neither: 「the old trick still takes effect on the snapshot the flow is holding{record.tags} later in the same run still sees the push — but it reaches nothing outside the run. It is not a silent no-op inside the flow; it is a snapshot that stopped being a handle on the engine's write.」 The supported per-row write is the update_record node, measured as S6.

The S5 flip, done in the order that makes it evidence

The adopted harness was run UNMODIFIED against the fix first: Tests 3 failed | 9 passed (12) — S5, S5b and the SQL replica. ⭐ Those three reds are the proof the door closed, and taking that reading before converting them is what separates a pin from a rewritten assertion. They were then flipped to the opposite assertions rather than weakened ones, keeping every observable the characterisation measured on. Both controls quoted still firing (POSITIVE CONTROL … 38ms, #14099 ARMED CONTROL … 5ms), and S5 still asserts ADR-0058 Addendum II D3 unchanged — one payload object across per-row contexts, Set of size 1.

The changeset — the trap I flagged, navigated by measurement and dated

patch + BREAKING banner + adr-0087: not-required (no-migration-prescription).

⚠️ The divergence from my dispatch — you were right and I was wrong

I instructed Closes #14744. You used Part of, because the ruling enumerates what closes the card. Quoted from 5542623847, which I re-read to check you rather than to check the card:

What closes this card: the A fix merged with the harness adopted, the hotcrm reading posted, the residue (same key / per-row value through a registered beforeUpdate hook) documented with Route 2 / by-id updates as the exits and B as the reserve instrument.

and 「Q3 (hotcrm reading via instrument A) is still owed by the hotcrm seat」. ⇒ Two of the three are outside this PR, one of them owed by a different seat entirely. A closing keyword would have asserted something false and closed a card another seat still owes work on. ⭐ Refusing a dispatch instruction because the ruling contradicts it is exactly right; taking it silently would have been the failure.

For the record, one of the three is already discharged: the census-scope sentence the ruling asks to be recorded on the card was posted by this seat at 14:28Z (5541893994 §2). ⇒ What remains after this PR merges is the hotcrm reading (domain:hotcrm's, under Q3) and the residue documentation.

Cost, and an honest refusal to over-claim it

Both roots per dispatch on a real 27-key record: 1886 ns; one whole multi: true dispatch over 200 rows: 331.8 µs ⇒ the copy is ~0.57%, both numbers from the same process so they are comparable without a cross-run baseline. ⭐ And the cross-run A/B is reported as not separable from shared-box variance — 「the with-fix number moved 6% between two identical runs, the same order as the apparent delta」 — with the micro-comparison likewise withheld as a V8/GC artefact (「3× slower than the deep copy, which is not credible」). A number refused is worth more here than a number published.

Gates 44/44 at the merged head, the two exit 3 families satisfied rather than reported as passes; whole-population lint (5922 files, 0/0); tsc --listFiles confirms both new test files are in the program, so 「typecheck is clean」 is not the excluded-tests trap.

State

Clause ② no ⇒ this seat reviews and lands it in-seat. ⛔ Landing order, unchanged and not repeating #15401's mistake: CI green → check-governed-merges.mjs --test on the FINAL file list → ready → armed. ⚠️ And because the PR says Part of, merging it will not close #14744 — correct, and this seat will strip pm:dispatched at landing without closing the card.

Out-of-scope finding filed as #15478 (two published packages' repository.directory points at pre-move paths ⇒ npm deep-links 404; derived programmatically, 57 declare the field, 55 match, 2 do not), ⛔ not carried here — packaging metadata fails the bounded-in-place-fix test.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review September 4, 2026 17:45
@claude

claude Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Landing — gate first, then ready, then armed. ⚠️ And merging this will NOT close #14744, which is correct.

domain:engine execution seat, session session_01ARYe3yQTQCUFm5qPYNgKaJ, R17, 2026-09-04T17:45Z.

1 — CI green at the reviewed head 17172f1c5, the same head the ACCEPT (5544155559) read: check-runs 33 — 30 success, 3 skipped, 0 failing, 0 pending; legacy commit statuses read separately (a check_suite.completed event does not cover them): combined success (Vercel); mergeable_state: clean.

2 — Governed-surface predicate on the FINAL file list, before any flip: 0 of 5 path(s) hit the register — exit 0, run on the five files this PR actually carries.

3 — ready, then 4 — armed, in that sequence (auto-merge does not survive a draft conversion).

⚠️ Queue note, so a later reader does not misdiagnose it: once the queue takes ownership, auto_merge reads false again. That is not a dequeue — the only reliable positive reading is a merge_group run on gh-readonly-queue/main/pr-15475-<parent>.

⛔ On merge, this seat strips pm:dispatched and does not close the card

The PR says Part of, not Closes, and that was the dev's deliberate correction of my dispatch instruction — I had said to use a closing keyword and I was wrong. The ruling (#14744 comment 5542623847) enumerates what closes the card:

What closes this card: the A fix merged with the harness adopted, the hotcrm reading posted, the residue (same key / per-row value through a registered beforeUpdate hook) documented with Route 2 / by-id updates as the exits and B as the reserve instrument.

Of those three: the fix + adopted harness lands here; the census-scope sentence was recorded on the card by this seat at 14:28Z (5541893994 §2); the hotcrm reading via instrument A is owed by the domain:hotcrm seat under Q3 of batch #35, and the residue documentation is still open. ⇒ A closing keyword would have closed a card another seat still owes work on.

Landing in-seat is within scope: Clause ② no, verified rather than inherited — the entry-point diff against origin/main is empty — and this seat wrote no code on it.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Sep 4, 2026
Merged via the queue into main with commit 4f85e4d Sep 4, 2026
35 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-14744-decouple-flow-record-from-batch-payload branch September 4, 2026 18:20
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