Skip to content

Contain a failing item to its own loop iteration: ten sweeps stop aborting mid-batch (42 flow-loop-body-uncontained to 0) - #1611

Merged
os-steve merged 3 commits into
mainfrom
claude/issue-1604-guarded-loop-iteration
Sep 5, 2026
Merged

Contain a failing item to its own loop iteration: ten sweeps stop aborting mid-batch (42 flow-loop-body-uncontained to 0)#1611
os-steve merged 3 commits into
mainfrom
claude/issue-1604-guarded-loop-iteration

Conversation

@os-steve

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

Copy link
Copy Markdown
Collaborator

Fixes #1604

Route 1 as ruled in comment 5555384744: the flows and the flow-graph walkers in
one revertible PR. objectstack lint's flow-loop-body-uncontained count goes
42 to 0, and pnpm verify is green end to end.

The per-flow product read — the ruling's required addition

The rule's own text says a sweep meant to stop at its first failure is a
legitimate reading and stays a warning. Ten flows, ten readings. All ten take
guarded(); nothing was left warning, and the reason is per flow rather
than "they are all batch sweeps".

flow loops the reading
task_due_reminder 1 Reminders are per owner and independent. One un-notifiable owner currently drops every later reminder that hour — the card's headline case, and there is no sense in which the ninth person's reminder depends on the eighth's succeeding. Continue.
case_sla_monitor 1 This flow's own header already rules the shape: when a breached case cannot be alerted, "the alert is skipped at a named gate, and the RUN SURVIVES: the breach is still recorded and every other breached case is still swept" (maintainer ruling 2026-09-03). Per-iteration containment is that ruling applied to the failure modes the named gate does not cover. Continue.
opportunity_stagnation 1 Per-deal nudge, already idempotent through the open-stall-task lookup, so a skipped deal is nudged on the next daily pass. A stalled deal is not evidence about any other deal. Continue.
contract_renewal 1 Per-contract renewal task plus notify, gated on "already reminded". One contract whose owner cannot be notified must not silence the whole morning's renewal window — the window is time-boxed by renewal_notice_days, so a lost pass is a renewal nobody is told about. Continue.
contract_expiration 1 Status flip plus notify, one contract at a time. An expiry that fails to write is not a reason to leave later contracts activated past their end_date. Continue.
quote_expiration 1 Same shape, one field, no cross-record meaning at all. Continue.
campaign_completion 1 Same: flip in_progress campaigns past end_date. The downstream metrics snapshot rides on each flip independently. Continue.
forecast_snapshot 1 One crm_forecast row per owner, each computed from that owner's own opportunities. One rep whose write fails must not cost every other rep the night's snapshot — and the sweep is nightly, so a lost pass is a missing day in a time series. Continue.
campaign_enrollment 2 The one screen flow here, not a scheduled sweep: user-invoked bulk enrolment behind the Enroll Members action. That makes containment more important, not less — an abort halfway through 1,000 leads leaves the operator staring at a partially enrolled campaign with no report of where it stopped. Per-member dedupe already makes a re-run safe. Continue.
demo_bootstrap 12 Argued in full below.

demo_bootstrap, argued rather than swept in

The ruling names this as the one most likely to want the other answer: 12 loops
from one claim() factory, and "a half-seeded demo that continues past a
failure" is a defensible thing to not want. Four things decide it, and the
first is the one that settles it.

1. Aborting does not buy all-or-nothing — it produces the half-seeded org.
The flow is a straight line of twelve find/loop pairs. An abort inside pass 3
leaves passes 1–2 fully claimed and passes 4–12 untouched. There is no
transaction and nothing rolls back, so "stop at the first failure" is not
"leave the demo clean"; it is "stop at an arbitrary point and leave nine
objects entirely ownerless". The choice is not whole vs half. It is all but
the failing rows
vs an arbitrary prefix.

2. A permanently un-claimable row is a poison pill today. The sweep runs
every 10 minutes and selects { owner_id: null }, so a row it cannot claim is
selected again on the next tick — and aborts again, at the same place, forever.
Every object after it in the chain stays ownerless permanently. That is exactly
the #622 shape the flow exists to prevent: rows that are invisible under a
private OWD, uneditable by everyone including system_admin, absent from
every "My …" view, with owner-addressed notify reaching nobody. Containment
turns "one bad row disables all twelve passes forever" into "one bad row stays
ownerless and is retried in ten minutes".

3. Idempotency means continuing costs nothing that stopping would have
saved.
A claimed row stops matching the filter; a skipped row is retried on
the next pass. Stopping does not preserve a retry that continuing spends.

4. On visibility, the concession and the answer. The card's measurement is
that the failed attempt's steps stay in the run log ahead of the handler's
— re-confirmed here against the pinned 17.3.0 artifact, not the platform source
(childSteps: [...failedAttemptSteps, ...catchSteps] in try-catch-node.ts,
and runRegion pushes the failing node's own status: 'failure' step before it
throws). The ruling is right that "visible in the run log" is not "an operator
will see it", and I am not claiming otherwise. But the alternative on offer is
not an operator seeing it either: it is an aborted run reporting acted: 0
with no per-item attribution, which is less legible, not more. Containment is
strictly the more visible of the two — the failure step exists in both, and
continuing adds the successful items beside it, so the per-node fold in
sys_automation_run can tell "3 of 40 skipped" apart from "the sweep died at
item 3". Nothing is lost by continuing; a discriminable signal is gained.

guarded() applied to all twelve.

The three premises, re-measured on this branch

Every figure on the card came from #1596's branch at base 81a79ee. This branch
is cut from 8223d0a (post-#1605). Re-derived here:

The walkers — 8 red, and 2 that were worse than red

Nothing is deleted, loosened, skipped, disabled or quarantined. Each walker
gains the same region descent the platform's own collectFlowGraphs takes.

The eight red files are the card's eight. Two more were green and blind:
test/demo-staffing.test.ts's "ships no flow node that writes an identity
table" (the #640 guard) and test/flow-variable-conditions.test.ts's condition
census both ask "nothing in this tree does X", and a walk that reaches an empty
tree answers that vacuously. The census is parameterised, so the loss is
countable — ablation on this branch, restoring only that file's pre-change
walker and running it both ways:

deep walk (this branch)      Tests  61 passed (61)
shallow walk (pre-change)    Tests  47 passed (47)

Green either way, 14 generated cases fewer. Fixing only the eight that went
red would have shipped that silently. Both are in the same defect class as the
eight, mechanical, in the same gate family, and add no new verification surface,
so they are fixed here rather than filed.

The descent reads the platform's slot map (FLOW_REGION_SLOTS_BY_TYPE) rather
than a hand-written key list, so loop.body, parallel.branches,
try_catch.try and try_catch.catch are all covered and a slot added later is
descended into without anybody remembering these suites exist. That is the same
choice test/flow-scheduled-org-partition.test.ts already made — which is why
it is the one flow walker in this repo that needed no change at all, and its own
header predicted today: "a walk that only knew about loop would silently stop
covering a sweep the day one is authored inside parallel or try_catch".

test/flow-run-summary.test.ts is the one that RUNS rather than inspects: it
builds a live-engine fixture by planting a dead predicate on a body edge, so it
now executes the guarded shape rather than only reading it.

The shape, and why not the literal spelling

At each site body: { becomes body: guarded('KEY', { and the closing },
becomes }),zero re-indentation, which is what makes it fit. The literal
22x inline try_catch stays refused per the ruling: its re-indentation alone
measured ~1,327 tokens over the ceiling, and the ratchet strips comments so
prose-slimming cannot buy it back.

Three facts checked against the pinned @objectstack/* 17.3.0 this repo
installs, not the platform source tree — all three re-confirmed:

  • try_catch is registered by the pinned engine, runs its try region, and on
    a caught failure runs catch and returns success: true, so the loop
    continues.
  • A bare assignment node with no config returns success: true — its
    executor folds an absent config to zero assignments, and config is optional
    on FlowNodeSchema. It is also the exact handler the lint rule's own hint
    prescribes.
  • The failed attempt's steps precede the handler's in the run log.

Verification

pnpm verify exit 0 — validate, typecheck, lint, lint:i18n-gate,
hygiene, hygiene:tokens, build, test, in that order.

lint      17 warning(s), 12 suggestion(s)     flow-loop-body-uncontained: 0  (was 42)
typecheck tsc --noEmit, exit 0
tokens    business semantics ~84,838 (ceiling ~85,000; headroom ~162)  clean
test      Test Files  160 passed (160)
          Tests  3391 passed | 1 skipped (3392)

The 17 residual lint warnings are the component-props-* family on pages and
views — a different family, untouched by this card. No lint rule was
suppressed, whitelisted or locally re-severitied, and no warning was found to
be wrong on this metadata, so nothing was filed upstream.

Changeset added: .changeset/scheduled-sweeps-survive-a-failing-record.md,
'hotcrm': patch. This is a live behaviour change.


Generated by Claude Code

A `loop` body has no error handling of its own — `loop-node.ts` iterates
with a bare `await` and carries no `try`/`catch` at all — so the first item
whose node fails ends the WHOLE run: every later item goes unprocessed and
the work already done is not reported. Measured by the platform on the real
engine: a 5-item sweep failing at item 3 touched 3 items, reported `acted: 0`.

`objectstack lint` names this 42 times, across 22 loops in 10 flows
(`flow-loop-body-uncontained`). Every one of them is a "process the batch"
sweep, so continuing past a failing record is the right reading in each; the
per-flow argument is in the PR body, `demo_bootstrap` included.

The containment is the one the rule prescribes: a `try_catch` inside the body
with the work in its `try` region and a single bare `assignment` as the
handler. Written out at each site it re-indents every body two levels, which
alone measured ~1,327 tokens over the `business semantics` ratchet ceiling —
a ceiling that moves only on a maintainer ruling. `guarded()` wraps the body
from outside instead: `body: {` becomes `body: guarded('<key>', {` and the
closing `},` becomes `}),`, so no body is re-indented and the whole change
costs 259 tokens, leaving ~162 of headroom.

Verified against the pinned `@objectstack/*` 17.3.0 this repo installs, not
the platform source: `try_catch` is registered and returns `success: true`
after running `catch`, so the loop continues; a bare `assignment` with no
`config` returns `success: true`; and the failed attempt's steps are kept in
the run log AHEAD of the handler's, so a skipped item stays visible.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DuzfS5chho38Yx1jxx9DEj
A dozen suites here hand-roll a walk over a flow's nodes, and every one knew
about exactly one region: `loop`'s `config.body`. With the work now one region
deeper — inside the `try_catch` guard each body opens with — those walks stop
at the guard.

Eight files said so by going red, 14 assertions in all. Two did not, and those
are the reason this commit touches more than the eight: `demo-staffing`'s
"ships no flow node that writes an identity table" (the #640 guard) and
`flow-variable-conditions`' condition census both ask "nothing in this tree
does X", and a walk that reaches an empty tree answers that vacuously. They
stayed GREEN while inspecting nothing. The census is parameterised, so the
loss is countable: it generated 14 fewer cases.

Nothing is deleted, loosened, skipped or quarantined. Each walker gains the
same descent the platform's own `collectFlowGraphs` takes, and takes it by
reading the platform's slot map (`FLOW_REGION_SLOTS_BY_TYPE`) rather than a
hand-written key list — so `loop.body`, `parallel.branches`, `try_catch.try`
and `try_catch.catch` are all covered, and a slot added later is descended
into without anybody remembering these suites exist. That is the same choice
`test/flow-scheduled-org-partition.test.ts` already made, which is why it is
the one flow walker in this repo that needed no change at all.

`test/flow-run-summary.test.ts` is the one that RUNS rather than inspects: it
builds a live-engine fixture by planting a dead predicate on a body edge, so
it now executes the guarded shape instead of only reading it.

Two type narrowings tightened rather than widened on the way: `node()` in
`forecast-manual-override` throws on a missing id instead of dereferencing
`undefined`, and the dead-gate fixture asserts it found the edge it plants on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DuzfS5chho38Yx1jxx9DEj
A user-visible behaviour change, so it takes a changeset: ten flows'
scheduled and bulk sweeps now skip a failing record instead of ending the
run on it.

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

vercel Bot commented Sep 5, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
hotcrm Ignored Ignored Sep 5, 2026 11:47pm UTC

Request Review

@github-actions github-actions Bot added ci/cd CI plumbing and the verification pipeline backend Server-side behaviour — hooks, flows, actions labels Sep 5, 2026

os-steve commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

⚠️ PM correction — the token headroom figure is superseded. The measurement was right; the ceiling moved under it.

The PR body reads "84,838 / 85,000, headroom ~162 … It is tight — 0.19%". That was correct at this branch's base 8223d0a. It is no longer the state of main.

PR #1606 landed at 57ce7204, 23:38:38Z — between this branch's cut and this PR being opened — and it raises CEILINGS['business semantics'] 85,000 → 100,000 on a maintainer ruling (「business-semantics 棘轮 提升到 100000」).

as the body states it actual, on merge
reading 84,838 84,838
ceiling 85,000 100,000
headroom ~162 (0.19%) ~15,162 (15.2%)

⇒ ⛔ Nothing to change here. The measurement was conservative — the work passes under the stricter of the two ceilings, so it is safe under either, and Quality Checks (green, 23:48:32) ran the gate against a merge with the raised ceiling and agreed. This correction exists so the number is not carried forward as a constraint on the rest of epic #1579: I would otherwise have sequenced later cards around a 162-token budget that does not exist.

Not a mistake by this dev, and worth stating plainly: it re-measured exactly what it was asked to, honestly, on the right base. main took two commits in the ~40 minutes between measurement and PR open. That is the fourth time this session a well-formed reading has proved something other than what it appears to — and the first where the instruction to re-measure was itself already the fix and still was not enough.

The structural lesson, recorded on #1579: at this repo's merge rate, a dev-reported measurement is stale before it is reviewed. The authority is the gate running on the merge commit in CI, not the number in the report. A dev's figures are narrative — useful for showing the reasoning, ⛔ never the thing that decides a merge.

Related and deliberately untouched: #1607 is open on the consequence of that same raise — the ratchet's opportunistic-tightening advisory now tells every run to re-anchor back to ~89,000, i.e. to undo the ruling. It is already pm:dispatched to another seat. ⛔ Not this card's, and not this epic's.


Generated by Claude Code

@os-steve
os-steve marked this pull request as ready for review September 5, 2026 23:53
@os-steve
os-steve added this pull request to the merge queue Sep 5, 2026
Merged via the queue into main with commit 9924ee1 Sep 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Server-side behaviour — hooks, flows, actions ci/cd CI plumbing and the verification pipeline

Projects

None yet

2 participants