Repository navigation
Commit 02bdeaa
fix(spec): filter the edge members collectFlowGraphs hands out (#18906)
Fixes #18102
Clause-②: no
`collectFlowGraphs` handed out a `FlowGraph` whose `edges` could hold a
non-record. The node side of that same walk has been member-filtered
since the repair #16752 landed; the edge side was forwarded untouched,
four lines away. One filter per list, side by side, in the same walk —
so `FlowGraph.edges` means what its declared type says.
## What the card claimed, re-derived on this tree
All three readings hold, and the line numbers were re-derived rather
than copied.
| card's reading | re-derived at | verdict |
|---|---|---|
| node side does `nodes.filter(isRegionDict)` around `:773` |
`control-flow.zod.ts:773` | holds, exact |
| `edges` forwarded untouched in the same `graphs.push` | `:774` | holds
|
| a nested region's edge list admitted on `Array.isArray` alone around
`:786` | `:786` | holds, exact |
The reproduction from the card body, run on `main` at `0b31d90fb`:
```
collectFlowGraphs({ nodes: [start, loop{ body: { nodes: [], edges: [null] } }], edges: [] })
graph[0] scope="" path=[] edges=[]
graph[1] scope="loop 'lp' body" path=["nodes",1,"config","body"] edges=[null]
NON-RECORD EDGE MEMBERS HANDED OUT: [null]
```
## The shape of the repair
- **A drop, in the producer.** `edges.filter(isRegionDict)`, through the
same single predicate the node side uses — extended in this PR to name
its third caller, so the two lists the walk hands out cannot drift from
each other. Copy-on-write per list: a well-formed flow is handed back
the very same arrays, by identity, and pays nothing.
- **Not a refusal, and not a looser signature.** No authoring door's
accept set moves. `FlowSchema.safeParse` still returns an envelope
rather than throwing (the walk runs inside the parse, so the repair has
to stay a drop and a skip), the region `safeParse` refusal in
`validateControlFlow` still owns and still reports the malformed region,
and `FlowGraph.path` still indexes the RAW node list so a Zod issue
stays anchored where the author wrote it. The declared element type is
unchanged; it is now true.
- **No consumer indexes `graph.edges` positionally.** Both
`packages/lint` readers key findings by `edge.id`; the edge-id
uniqueness rule in `flow.zod.ts` reads `flow.edges` raw, never
`graph.edges`. So a dropped member changes the index and never whether
an edge was judged.
## Latent, not live — measured, and NOT for the reason the card gave
The card's reachability argument rests on "#16910 re-pointed both
`graph.edges` consumers in `packages/lint`". There are **three**
`graph.edges` consumers on this tree, not two.
| consumer | guard |
|---|---|
| `packages/lint/src/lint-flow-patterns.ts:1560` |
`recordsOf(graph.edges)` |
| `packages/lint/src/validate-expressions.ts:1462` |
`recordsOf(graph.edges)` |
| `packages/services/service-automation/src/engine.ts:9244` | **none** —
reads `edge.id` / `edge.source` / `edge.target` straight off each member
|
The third one is shielded only by call ORDER: inside `registerFlow`,
`canonicalizeStoredFlow` runs `validateControlFlow(parsed)` at
`engine.ts:4021`, which refuses the malformed region before
`validateFlowExpressions` is reached at `:4058`. Measured:
```
validateControlFlow(the card's repro)
THROWS Error: loop 'lp' body: invalid region — Too small: expected array to have >=1 items;
Invalid input: expected object, received null
```
So the conclusion "no reachable throw today" survives — by one belt more
than was counted, and a belt made of validator ordering rather than of a
member check. That is exactly the asymmetry this card was filed about,
one layer up. After this change the declared type carries it, and the
next consumer of this list needs neither a coercion nor a call-order
argument. ⛔ Not written up as a crash report: no throw was measured on
the tree.
## `analyzeRegion` — the card's open question, answered
`analyzeRegion` (`:412`, `for (const e of edges)` at `:442`) **throws**
on a non-record edge member, measured on all five shapes:
```
null THROWS TypeError: Cannot read properties of null (reading 'source')
undefined THROWS TypeError: Cannot read properties of undefined (reading 'source')
a string no throw — errors: ["edge 'undefined' source 'undefined' is not a region node", …]
a number no throw — same
an array no throw — same
```
**But it is not a consumer of `FlowGraph.edges`, and the card's "second
consumer" framing does not hold.** Nothing on this tree routes producer
output into it. Its three in-repo entry points:
- `validateControlFlow:624` hands it `parsed.data` from a *successful*
`slot.schema.safeParse` — the failure branch throws first, so its
`edges` members are always parsed records;
- `findRegionEntry` ← `AutomationEngine.runRegion(region:
FlowRegionParsed)` — parsed;
- `bpmn-mapping.ts:111` casts a raw `cfg(c).body` region in, which is a
pre-existing raw-cast surface of that file and untouched here.
It reads an edge list; it does not read `FlowGraph.edges`. Its declared
parameter type is `FlowEdgeParsed[]`, so a throw on input that type
excludes is not a defect at that door — hardening it would be the
lenient-consumer direction Prime Directive #12 refuses, and the
direction #16752's own note refuses ("the repair is a drop, never a
looser signature"). Left alone deliberately.
## Where the pin went
Beside #16752's, in
`packages/spec/src/automation/region-normalization.test.ts` — the
`packages/spec`-side pin the card said was the one still missing. Both
halves of that claim were verified first:
- `packages/lint/src/non-record-object-entry.test.ts` already carries
the `flows[].nodes[].config.body.edges` arm (`underNestedRegionEdges`,
in `SWEPT_COLLECTIONS`), so no arm is added there and that file is
untouched;
- #16752's own spec-side pin is the `describe('#16752 — a non-record
member never reaches a returned graph')` block in
`region-normalization.test.ts`, not in `control-flow.test.ts`. The new
`describe('#18102 — …')` sits directly after it, under the same `#4347 —
collectFlowGraphs` parent.
31 new cases: five shapes × (three nesting depths + an
unguarded-dereference arm + an anti-vacuity arm), plus the card's own
reproduction and a per-list identity arm.
## Verification
Union run at `369c4cca5` (the merge commit; `origin/main` merged in at
`600b1e2ab` before opening this PR).
**Reverse verification — the pin can fail.** Mutation through
`scripts/ablation-replace.mjs`, anchor hit asserted on disk, restore
proven by blob equality with HEAD:
```
anchor "edges: keptEdges.length === edges.length ? edges : keptEdges," x1 -> x0
replace "edges," x0 -> x1
blob 38cf36c -> b293f7924509 (the mutation landed)
result 23 failing tests, all inside the #18102 describe, zero elsewhere
restore blob == HEAD (38cf36c) and `git diff HEAD` empty
```
Direction: **red**, as expected. The suite resolves
`./control-flow.zod.js` relatively inside its own package, so the
mutation reddened it with no rebuild — this ablation has no `dist/` leg
to prove.
**Tests.**
| run | result |
|---|---|
| `pnpm --filter @objectstack/spec test` | 488 files / 14159 tests
passed |
| `pnpm --filter @objectstack/spec typecheck` | pass (test layer
compiles; debt ledger unmoved) |
| `pnpm --filter @objectstack/lint test` | 104 files / 3909 tests passed
|
| `... service-automation exec vitest run
src/nested-region-parity.test.ts` | 22 tests passed |
The `packages/lint` run is the load-bearing one for the cross-package
direction: `non-record-object-entry.test.ts` keeps `RESIDUAL_THROWS` and
`RESIDUAL_INVENTED` exact in BOTH directions, so a producer change that
flipped a row there would red it. Both stay empty.
**Gates.** Derived from the actual changed paths, never from a named
list:
```
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands
-> 81 families
81 run, every exit code captured to a file BEFORE any pipe, all 0
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --ran ran.txt
-> 81 derived, 81 run, 0 NOT-MEASURED, 0 UNRUN
(a DERIVED zero — all 81 recorded an exit code and none of them is 3)
```
Two conditions were cleared rather than reported as reds, because
neither measured anything: `check-plugin-teardown-shape --self-test`
exited 3 on a shallow clone that could not reach its pinned fixture
(cleared by `git fetch --unshallow`, as its own text prescribes), and
four gates exited 3 with `PREREQUISITE NOT MET` before the package
closure was built (cleared by `turbo run build` over `./packages/*`).
The whole set was then re-run at the final `369c4cca5`, so no ratchet
reading predates the last commit.
⛔ Derived families are not a complete account of CI on this PR: the 50
artifact-roster families, the 11 wide-population families, the 5
path-scheduled CI jobs and the always-runs tail are each outside that
total, and a green from a `--self-test`-only family grades the checker,
not this diff.
**Lint, as a proven narrowing rather than a repo scan.** `pnpm lint` is
`eslint . --no-inline-config`, a repo-wide scan CI owns. The three facts
that make the narrow run a measurement:
1. **Population, read from `eslint.config.mjs` itself** — its
file-matching objects declare `**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}` and
narrower per-root subsets of it, minus `NEVER_LINTED`. A `.md` path
matches no config object, which eslint states in its own report for the
changeset (`File ignored because no matching configuration was
supplied`).
2. **Count, read from `--format json`** — 3 paths handed in, 3 in the
report, 2 linted: 0 errors, 0 warnings, 0 suppressed. Exit 0.
3. **Invariance** — `eslint.config.mjs` states, with its own measured
positive control, that this repo "never enables type-aware linting (no
`parserOptions.project`, no typed `@typescript-eslint` rules) for ANY
file, test or not". With no rule reading a type, nothing in this diff
can move the verdict on a file this diff does not contain.
## Acceptance notes
Observations, none of them filed — each is noted here for whoever next
touches the file.
- **Two `packages/lint` comments now misstate the producer.**
`lint-flow-patterns.ts:1555` and `validate-expressions.ts:1459` each say
the edge side of this walk "is not" member-filtered, as the
justification for their `recordsOf` coercion. Their conclusion stays
right (belt and braces, ⛔ not one repair written twice) — only the
parenthetical is now stale. Left untouched because the dispatch's
declared file surface does not include those two files and a stale
comment is not one of the three filing classes. Exact edit needed: in
both, "the edge side is not" becomes "both sides are since #18102".
- **`non-record-object-entry.test.ts:378` carries the same stale
sentence** in `underNestedRegionEdges`' docblock. Same reasoning; the
arm's behaviour is unaffected and the file is untouched.
- **The edge-id uniqueness rule is top-level only.** `flow.zod.ts:1164`
walks `flow.edges` and no region's, so two region edges may share an id
where two top-level edges may not. A coverage asymmetry, not an error:
no example fails, nothing is mis-documented. Not filed.
- **`bpmn-mapping.ts` casts raw region config into `analyzeRegion`.**
`cfg(c).body as Region` at `:313` (and the `try`/`catch`/`branches`
siblings) is a bare cast off an open `z.record`, so a malformed region
reaches `analyzeRegion` as a `TypeError` where that file's own located
diagnostic belongs. Reachability was NOT measured, so this stays an
un-rehearsed observation rather than a card. Successor: none — no open
PR touches that file.
- #16910 is not addressed here, as the card and triage both state.
## 维护者速读(草稿)
**改了什么。** `packages/spec` 里那个把一张 flow
拆成「每个子图」的函数,原先对节点列表做了成员过滤、对边列表没做。这次给边列表补上同一个过滤,一条路一个过滤器,并排放着。
**为什么改。** 它对外声明「我交出去的边一定是一条边」,实际可能交出一个 `null` —— 作者在 YAML 里把 `edges:`
下面一项写空了,反序列化出来就是
`null`。同一个函数、同一次遍历,节点那一半修了、边这一半没修,声明与事实不符的那一半就还在。今天没人被它崩到,但每个现在和将来的消费者都只差一次不设防的取值。本次实测还发现:挡住它的其实不止卡上写的那两道消费端强制转换,还有第三个消费者靠「校验顺序」侥幸躲开
—— 也就是说,原来的安全边界比卡上记的更薄。
**风险与代价(含回滚)。** 行为变化只发生在「声明里本来就说不可能存在」的输入上;结构良好的 flow
按引用原样返回,零代价。不新增任何拒收,作者能写的东西一点没少。回滚 = revert 本 PR 单个
commit,无数据迁移、无配置、无兼容窗口。
**席位意见。**
**你要做的。** 确认这里只补过滤、不新增拒收(这是本卡的裁决前提),然后按常规流程合。若要顺手清掉上面 Acceptance notes
里那三处已过期的注释,那是另一个 PR 的事。
---
_Generated by [Claude
Code](https://claude.ai/code/session_019srGWGCBBCBHqcDoRZpQRh)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent d5ccfb4 commit 02bdeaa
3 files changed
Lines changed: 191 additions & 12 deletions
File tree
- .changeset
- packages/spec/src/automation
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
500 | 500 | | |
501 | 501 | | |
502 | 502 | | |
503 | | - | |
504 | | - | |
505 | | - | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
506 | 507 | | |
507 | 508 | | |
508 | 509 | | |
| |||
722 | 723 | | |
723 | 724 | | |
724 | 725 | | |
725 | | - | |
726 | | - | |
727 | | - | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
728 | 730 | | |
729 | 731 | | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
| 735 | + | |
| 736 | + | |
| 737 | + | |
| 738 | + | |
| 739 | + | |
| 740 | + | |
| 741 | + | |
| 742 | + | |
| 743 | + | |
| 744 | + | |
730 | 745 | | |
731 | 746 | | |
732 | 747 | | |
| |||
759 | 774 | | |
760 | 775 | | |
761 | 776 | | |
762 | | - | |
763 | | - | |
764 | | - | |
765 | | - | |
766 | | - | |
| 777 | + | |
| 778 | + | |
| 779 | + | |
| 780 | + | |
| 781 | + | |
| 782 | + | |
| 783 | + | |
| 784 | + | |
767 | 785 | | |
768 | 786 | | |
769 | 787 | | |
770 | 788 | | |
771 | 789 | | |
772 | 790 | | |
773 | 791 | | |
774 | | - | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
| 797 | + | |
| 798 | + | |
775 | 799 | | |
776 | 800 | | |
777 | 801 | | |
778 | 802 | | |
779 | 803 | | |
| 804 | + | |
| 805 | + | |
| 806 | + | |
780 | 807 | | |
781 | 808 | | |
782 | 809 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
388 | 388 | | |
389 | 389 | | |
390 | 390 | | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
| 492 | + | |
| 493 | + | |
| 494 | + | |
| 495 | + | |
| 496 | + | |
| 497 | + | |
| 498 | + | |
| 499 | + | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
391 | 522 | | |
0 commit comments