Assignment fan-out: wrap the loop body in try_catch so one failing person does not abort the fan-out - #124
Merged
Conversation
`loop-node.ts` iterates its body with a bare `await engine.runRegion(...)`
and holds no try/catch of its own, so one failing assignee threw straight
out of the container: every later assignee was never processed, and the
loop never returned its `childSteps`, so the rows it HAD written were not
counted either. Measured on the real engine with a three-name assignment
whose middle row was bad — `status: failed`, `acted: 0`, one duly_task
actually on disk.
The loop body is now one `try_catch` container. The three data nodes that
do one person's work sit in its `try`; the `catch` notifies the assigner,
once per assignee who got no task, with the engine's own reason and a
click-through to the assignment. Same run, after: `success`, `acted: 2`,
two tasks, `task_count: 2`, and one warning in the assigner's inbox.
The flow still writes nothing back onto `duly_assignment` — a write to the
trigger record re-enters this same `record-after-write` flow on a row still
`dispatched`, which would re-send the notification on every re-entry. The
handler reads `{fanout_assignee}` and never `fanout_assignee_user`: the
loop shares one variable scope, so a region that throws at `fanout_find_unit`
leaves that variable holding the PREVIOUS assignee and a handler reading a
name from it would calmly name the wrong colleague.
Also here, both required by the above rather than incidental:
- `src/email-templates/assignment.email-template.ts` — the handler's words
as an `en` + `zh-CN` bundle. `NotifyConfigSchema` makes inline title/message
and `template` mutually exclusive and calls the inline path "not
localizable", so AGENTS.md §8 is only satisfiable on the template path.
- `src/translations/authored-text.ts` — two `REENTER` entries so the i18n
walk knows a `try_catch`'s regions re-enter the flow vocabulary, exactly
as `loop.config.body` already does. Without them every node inside the new
regions is an unclassified path and the coverage gate fails naming them.
`parallel`'s `config.branches[]` is deliberately NOT added: a branch
carries a `name` that would land on the `flow.name` verdict and be
misread.
`test/email-templates.test.ts`'s notify walk now recurses into container
regions — without that it silently stopped covering the newest notify node,
whose template miss would dead-letter permanently while the run reported
success.
Gates: pnpm validate, typecheck, test (884), build — all green.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
os-warren
marked this pull request as ready for review
September 5, 2026 06:15
os-warren
added a commit
that referenced
this pull request
Sep 5, 2026
…125) Closes #106, closes #102. Reviewed on b8bbdb0 (already carrying main/#124): four gates green (validate, typecheck, 885 tests, build); 14 packages ^17.3.0; task hook keys the clearing branch on the shared payload so 17.3.0's divergence check passes; 84 zh-CN keys over the new dataset / bulkActions / _validations slots with the pin re-derived; browser proof shows Chinese tile sub-labels, bulk-bar buttons and a Chinese validation refusal. Pie legend stays English pending objectui#7696.
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
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.
Closes #123
What was wrong
loop-node.tsiterates its body with a bareawait engine.runRegion(...)and has no try/catch of its own, so a body node that fails throws straight out of the container. The first bad assignee ended the whole run, every later assignee was never processed, and — because an aborted loop never returns itschildSteps— the rows it had already written were not counted either.Measured on the real engine at
origin/main, three names with the middle row bad (a blankassigneesentry — the "missing owner" shape):status: failedsuccessacted: 0acted: 2duly_taskrows writtenduly_assignment.task_countWhat this does
The loop body is now one
try_catchcontainer. The three data nodes that do one person's work sit in itstry; thecatchrunsnotifyto the assigner, once per assignee who got no task.What the assignment shows afterwards — decided here so it is a contract, and written into the flow's header comment:
task_countis aField.summaryover the children, so it reports the tasks that actually exist. The run summary now agrees with it, because a loop that completes returns thechildStepsan aborted one threw away.severity: warning, and a click-through to the assignment. The run log alone does not count — it is an operator surface and the assigner never opens it.Two decisions worth reading as decisions rather than as details:
duly_assignment— not a note, not a status. It cannot: the start trigger isrecord-after-write, so a write to the trigger record re-enters this same flow on a row stilldispatched, and the notification would be re-sent on every re-entry. The inbox is the assigner-visible surface that costs no such loop. (The existing invariant test "the flow never writes duly_assignment" still holds.){fanout_assignee}and nothing else about the person. The iterator is re-bound before every iteration;fanout_assignee_useris not. A region that throws atfanout_find_unitleaves that variable holding the previous iteration's row, so a handler that read a name from it would calmly name the wrong colleague.test/assignment-fanout.test.tspins this both structurally and against the engine.Two changes that came with it
Both are required by the above rather than incidental, so they are called out rather than buried:
src/email-templates/assignment.email-template.ts(new,en+zh-CN).NotifyConfigSchemamakes inlinetitle/messageandtemplatemutually exclusive and its own.describe()calls the inline path "not localizable", so AGENTS.md §8 is only satisfiable on the template path.assigneeis deliberately not a required render variable: the commonest bad row is an assignee entry that is itself blank, and a required-but-absent hole would dead-letter the one message whose job is to report it.src/translations/authored-text.ts— twoREENTERentries (config.try,config.catch) so the i18n walk knows atry_catch's regions re-enter the flow vocabulary, exactly asloop.config.bodyalready does. Without them every node inside the new regions is an unclassified path andtest/i18n-coverage.test.tsfails naming all 30 of them.parallel'sconfig.branches[]is deliberately not added: a branch carries anamethat would land on theflow.nameverdict and be misread.src/translations/, which Retire the three i18n exemptions once the platform ships their keys — and re-take the two English-island screenshots #106 is also working in — the two lines are in theREENTERtable, away from the locale bundles, but the PM should sequence the merges knowingly.test/email-templates.test.ts's notify walk now recurses into container regions. Without that it silently stopped covering the newest notify node, whose template miss would dead-letter permanently while the run still reported success.Verification, at
9715f88All four gates green on this exact commit:
The 17.3.0 lint the card was raised by — checked read-only in this worktree, and checked in both directions so "no warning" is a measurement rather than an absence:
npx @objectstack/cli@17.3.0 validateon this branch: exit 0, 42 author-time rules, notry_catchwarning.origin/main's pre-fix flow restored in place: the warning fires three times, verbatim as quoted in the issue (fanout_find_existing/fanout_find_unit/fanout_create_task). Restored afterwards; the tree is byte-identical to the commit.Reverse verification of the new tests. With the
catchregion ablated from the committed flow (tests untouched),test/assignment-fanout.test.tsgoes 9 red / 30 green — every engine assertion plus the two handler-structure ones — withexpected [ alice ] to deeply equal [ alice, carol ]andtry_catch 'fanout_attempt': try region failed — … Owner is required. The ablation script restored under atrap … EXIT INT TERM; the mutation was confirmed on disk by grepping the marker to 0 before the run and back to 1 after.One thing this could not test the obvious way. This repo installs no
record_changetrigger — they ship as separate@objectstack/trigger-*packages and onlytrigger-scheduleis a devDependency — so aduly_assignmentinsert fires nothing and the boot says so. The engine test drives the flow withautomation.execute(flowName, context), which is the same door the trigger uses:registerFlowarms every trigger withtrigger.start(binding, (ctx) => this.execute(flowName, ctx)). Which context a record-change fire would assemble is pinned separately by the start-node assertions already in the file. This is written into the test's own header.🤖 Generated with Claude Code
https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p
Generated by Claude Code