Repository navigation
Commit eb05ea2
feat(spec): validateControlFlow throws FlowRegionShapeError carrying the container path and node id (#29789)
domain:spec#3 · os-support-ai · session_01WGufzj9yXiuW9h8YrpyLMy
<!-- claude-projects-attribution: {"github_login":"os-support-ai"} -->
_Requested by **Yibo Wang** · [project
thread](https://claude.ai/code/project/chan_01BVUyyPie8gHwoPM18iABwB?thread=cmsg_01BVUyyPie8gHwoPM18iABwBBvdbwqo6Exhkx4VrtNQBPA)_
Fixes #29719
Clause-②: yes (widening)
Metadata-protocol: none
Skills-docs: none — `skills/**` has 0 hits for `validateControlFlow`; no
skill teaches the refusal's type, and the message every skill-facing
surface prints is unchanged
## 维护者速读
- 改了什么:流程结构校验 `validateControlFlow` 拒收一个流程时,抛出的不再是裸 `Error`,而是新导出的
`FlowRegionShapeError`,错误上带两项数据:出错容器节点在流程里的路径 `path`(如 `['nodes', 1,
'config', 'try', 'nodes', 0]`)和该节点的 `nodeId`。错误文字一个字节都没变。
- 为什么改:lint 规则
`flow-region-shape-invalid`(#29558)只能把问题挂在整个流程上;要定位到具体容器就得去解析报错句子,那是第二套判定写法,而且嵌套在别的容器里的
`parallel` 句子根本不带外层位置。现在读 `err.path` 即可。
- 风险与回滚:只加数据不改判定:哪些流程被拒、拒收文字、HTTP 答复(仍是 400 VALIDATION_FAILED,错误不带
`status`)都不变;类继承 `Error`,读 `err.message` 或 `instanceof Error`
的调用方无感。回滚即还原本 PR。
- 先看:`packages/spec/src/automation/control-flow.zod.ts`(新类与
`containerPathOf`:索引按原始列表取,不按过滤后的
`graph.nodes`)、`packages/spec/src/automation/control-flow.test.ts`(末尾 5
个新 pin)、`.changeset/29719-flow-region-shape-error.md`。
- 两分钟验证:`pnpm --filter @objectstack/spec exec vitest run
src/automation/control-flow.test.ts`,应见 56 个用例全绿;把 `control-flow.zod.ts`
换回 `origin/main` 版本再跑,新增 5 个用例全红。
- 需要你做的:无,席位落地。
## What changes
`validateControlFlow`
(`packages/spec/src/automation/control-flow.zod.ts`) throws the new
exported `FlowRegionShapeError` at all three of its throw sites. The
class extends `Error` and adds two readonly fields:
- `path: readonly (string | number)[]`: the key path from the flow root
to the container node that owns the fault. It is a `FlowGraph.path` that
`collectFlowGraphs` hands out, plus `['nodes', i]`, so a reader that
already anchors on a graph's path can use it as is. Examples: `['nodes',
1]` for a top-level container, `['nodes', 1, 'config', 'try', 'nodes',
0]` for a container inside a `try` region, `['nodes', 1, 'config',
'branches', 1, 'nodes', 1]` for one inside parallel branch 1.
- `nodeId: string`: that container's `id`.
**Name and precedent.** `FlowRegionShapeError`, the seat's call under
the veto window (claim 6594934995). Shape mirrored from
`ListViewGroupQueryError` (`src/ui/view-grouping-query.ts`): an `Error`
subclass with readonly data fields, `this.name` set to the class name,
the message passed through unchanged, and `path` as a `(string |
number)` key path. Like `MigrationFloorError`
(`src/migrations/chain.ts`) it carries no ADR-0112 `code`/`status`. That
is deliberate: the runtime's `flowDefinitionRefusal`
(`packages/runtime/src/domains/automation.ts`) keeps a thrown error's
own `.status` when it has one, so a declared status would overrule the
door's 400 classification. Not mirrored: `ListViewGroupQueryError`'s
closed `reason`. The card asks for path and node id only, and a refusal
vocabulary would be one more export to review.
**The index is the raw list's.** `collectFlowGraphs` drops a non-record
member from the `nodes` it hands out, while `FlowGraph.path` keeps
indexing the raw list. `containerPathOf` therefore reads the raw list
back through `graph.path` and finds the node by identity (the walk
filters and never copies), so a dropped member before the container does
not renumber it. The lookup runs only on a refusal, so the accepting
path does no extra work.
**One judge, one spelling.** No new validation path: the same loop, the
same three conditions and the same sentences. The only edits are `new
Error(m)` → `new FlowRegionShapeError(path, id, m)` and the docblock.
### Throw sites
| Site on `origin/main` `3eeb81d1` | Refusal | Before | Now carries |
|:---|:---|:---|:---|
| `control-flow.zod.ts:651` | `parallel 'ID': a parallel block needs at
least 2 branches` | plain `Error`; the sentence names no enclosing scope
| `path` to the `parallel` node, at any depth; `nodeId` |
| `control-flow.zod.ts:657` | `WHERE: invalid region — ZOD ISSUES` (the
slot's schema refuses it) | plain `Error`; location in prose only |
`path` to the container that owns the slot; `nodeId` |
| `control-flow.zod.ts:665` | `WHERE: ANALYSIS ERRORS` (`analyzeRegion`:
entry/exit/cycle/ids/edges) | plain `Error`; location in prose only |
`path` to the container that owns the slot; `nodeId` |
| `control-flow.zod.ts:532` `findRegionEntry` | `malformed control-flow
region: …` | unchanged, still a plain `Error` | not covered: see below |
**`:532` is a separate judge, not on `validateControlFlow`'s path** (the
PM assumption, measured). `validateControlFlow` never calls
`findRegionEntry`. Its one production caller is the engine's `runRegion`
in `packages/services/service-automation/src/dag-traversal.ts:761`, at
run time, and it receives a bare region with no flow around it, so it
has no flow path or container id to carry. A flow that reaches it
registered already. This PR leaves it as it is.
## Tests
`control-flow.test.ts` gains `validateControlFlow throws
FlowRegionShapeError with the container path and id`. Every case asserts
`instanceof FlowRegionShapeError`, `instanceof Error`, `name`, `path`,
`nodeId` and the full message:
| Case | `path` | `nodeId` | Message pinned |
|:---|:---|:---|:---|
| invalid region: top-level `try_catch` with an unknown region key |
`['nodes', 1]` | `tc` | literal `try_catch 'tc' try: invalid region — `
+ `FlowRegionSchema`'s own issue messages |
| region analysis error: a two-entry `loop` body, inside parallel branch
1 | `['nodes', 1, 'config', 'branches', 1, 'nodes', 1]` | `sweep` |
literal `parallel 'fan' branch 1 → loop 'sweep' body: ` +
`analyzeRegion`'s errors (plus a literal `region must be single-entry
but has 2: x, y; ` lead) |
| `parallel` with 1 branch, top level | `['nodes', 0]` | `p` | literal,
whole sentence |
| the nested case: a 1-branch `parallel` inside a `try` region (the
draft-publish-verdict fixture shape) | `['nodes', 1, 'config', 'try',
'nodes', 0]` | `fan` | literal `parallel 'fan': a parallel block needs
at least 2 branches` |
| raw index: `null` before the container in the list | `['nodes', 2]` |
`p` | (not a message case) |
The first four also assert one spelling: `path` minus its last two keys
is a path that `collectFlowGraphs(flow)` hands out, and the
second-to-last key is `'nodes'`.
**Red first.** With `control-flow.zod.ts` checked out from
`origin/main`, the five new cases fail (`Tests 5 failed | 51 passed
(56)`, lock VERDICT command-exit 1). Restored with `git checkout HEAD
--`: blob hash equal to HEAD's, `git diff HEAD` empty.
**Ablation of the raw index** (`scripts/ablation-replace.mjs`, anchor
hit 1 → 0, blob `0ac7eeaa` → `370278a2`): `raw.indexOf(node)` →
`graph.nodes.indexOf(node)`. Only the raw-index case fails (`Tests 1
failed | 55 passed (56)`). Restore checked by blob equal to HEAD and an
empty `git diff HEAD`. The spec suite imports `src/` directly, so no
`dist/` is involved.
**Message parity, measured.** A one-off probe (not committed) ran
`origin/main`'s `validateControlFlow` and this head's over six refused
flows: the five cases above plus a multi-exit loop body. Result:
`byte-identical messages: 6/6`.
**The pinned sentence holds.**
`packages/metadata-protocol/src/protocol.draft-publish-verdict.test.ts`
pins `"Flow 'nested_region' would be refused at registration: parallel
'fan': a parallel block needs at least 2 branches"`. The
`@objectstack/metadata-protocol` suite ran green on this head against a
spec `dist` built from it (see Verification).
## Acceptance notes
- **Changeset level: `patch`, not the dispatch's assumed `minor`.**
AGENTS.md Post-Task step 3: "The level follows the kind alone: a kind
takes at least `minor` …, `none` takes `patch`". An exported error class
is not authorable metadata, so `Metadata-protocol: none` and
`@objectstack/spec: patch`. `check-changeset-no-major` on this body: see
Verification. The control with `minor` is refused `none-minor`.
Precedent: `.changeset/26464-mcp-metadata-write-scope.md` (`Clause-②:
yes (widening)`, `none`, spec `patch`).
- Declaration map: `check:generated` reported it fresh
(`check:declaration-map ✓`). Only `api-surface/automation.json` and
`export-origins/automation.json` moved, one line each.
- Consumer: #29558 (lint half) reads `err.path` when present, else keeps
the flow as the anchor. `packages/lint/**` is untouched here.
## Verification
All on head `78d8ed02`, under `os-verify-lock`.
<details><summary>spec local suite + typecheck, metadata-protocol
suite</summary>
- Closure build `pnpm --filter '@objectstack/metadata-protocol^...' run
build`: exit 0 (spec `dist` built from this head).
- `pnpm --filter @objectstack/spec test`: `Test Files 803 passed (803)`,
`Tests 23805 passed | 1 todo (23806)`, exit 0.
- `pnpm --filter @objectstack/spec typecheck`: exit 0.
- `pnpm --filter @objectstack/metadata-protocol test`: `Test Files 310
passed | 4 skipped (314)`, `Tests 22292 passed | 26 skipped (22318)`,
exit 0. This includes `protocol.draft-publish-verdict.test.ts` and its
pinned nested-parallel sentence.
- Lock: `VERDICT command-exit 0 · held the lock 843s`.
- Spec `test:repo` was not run locally (CI's `Test Core` owns it). The
diff edits no `repo`-project test, and the derived list does not name
it.
</details>
<details><summary>generated artifacts</summary>
- `pnpm --filter @objectstack/spec build && … check:generated`: `✗ 2 of
14 artifact(s) stale` (`check:api-surface`, `check:export-origins`).
- `check:generated --fix` rewrote those two, one line each. The re-check
printed all 14 `✓`, `check:declaration-map` included. Lock VERDICT
command-exit 0.
</details>
<details><summary>check-changeset-no-major with this body as the
event</summary>
- `GITHUB_EVENT_NAME=pull_request node
scripts/check-changeset-no-major.mjs --base origin/main --event EVENT`:
exit 0, `✓ LEVEL AXIS: … metadata-protocol none … a non-protocol change
at patch`.
- Control (a local probe commit that was reset and never pushed: the
changeset graded `@objectstack/spec: minor`, same body): exit 1, `Check
Changeset (level axis): metadata-protocol declares none while a
changeset entry is graded minor or above`.
</details>
<details><summary>derived gates (dispatch-gates --commands, 80
families)</summary>
- 77 of 80 green on the first serial pass.
- 3 exited 3 for missing prerequisites:
- `check-plugin-teardown-shape --self-test` needed its pinned fixture
commit `621a4876` in this shallow clone.
- `check:dual-build-cjs-loads` and `check:lean-entry-closure` read built
output of packages outside the closure.
- After `git fetch origin 621a487…` and a full `pnpm turbo run build
--concurrency=2` (`Tasks: 118 successful, 118 total`, lock VERDICT
command-exit 0), all three exited 0.
- `dispatch-gates --ran`: `✓ 80 derived famil(ies) accounted for — 80
run, 0 NOT-MEASURED`.
- CI families: see this PR's check-runs.
</details>
---
_Generated by [Claude
Code](https://claude.ai/code/session_01WGufzj9yXiuW9h8YrpyLMy)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 339f63d commit eb05ea2
5 files changed
Lines changed: 203 additions & 5 deletions
File tree
- .changeset
- packages/spec
- api-surface
- export-origins
- src/automation
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
179 | 179 | | |
180 | 180 | | |
181 | 181 | | |
| 182 | + | |
182 | 183 | | |
183 | 184 | | |
184 | 185 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
173 | 173 | | |
174 | 174 | | |
175 | 175 | | |
| 176 | + | |
176 | 177 | | |
177 | 178 | | |
178 | 179 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
12 | 12 | | |
13 | 13 | | |
14 | 14 | | |
| 15 | + | |
| 16 | + | |
15 | 17 | | |
16 | 18 | | |
17 | 19 | | |
| |||
643 | 645 | | |
644 | 646 | | |
645 | 647 | | |
| 648 | + | |
| 649 | + | |
| 650 | + | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
| 662 | + | |
| 663 | + | |
| 664 | + | |
| 665 | + | |
| 666 | + | |
| 667 | + | |
| 668 | + | |
| 669 | + | |
| 670 | + | |
| 671 | + | |
| 672 | + | |
| 673 | + | |
| 674 | + | |
| 675 | + | |
| 676 | + | |
| 677 | + | |
| 678 | + | |
| 679 | + | |
| 680 | + | |
| 681 | + | |
| 682 | + | |
| 683 | + | |
| 684 | + | |
| 685 | + | |
| 686 | + | |
| 687 | + | |
| 688 | + | |
| 689 | + | |
| 690 | + | |
| 691 | + | |
| 692 | + | |
| 693 | + | |
| 694 | + | |
| 695 | + | |
| 696 | + | |
| 697 | + | |
| 698 | + | |
| 699 | + | |
| 700 | + | |
| 701 | + | |
| 702 | + | |
| 703 | + | |
| 704 | + | |
| 705 | + | |
| 706 | + | |
| 707 | + | |
| 708 | + | |
| 709 | + | |
| 710 | + | |
| 711 | + | |
| 712 | + | |
| 713 | + | |
| 714 | + | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
| 735 | + | |
| 736 | + | |
| 737 | + | |
| 738 | + | |
| 739 | + | |
| 740 | + | |
| 741 | + | |
| 742 | + | |
| 743 | + | |
| 744 | + | |
| 745 | + | |
| 746 | + | |
| 747 | + | |
| 748 | + | |
| 749 | + | |
| 750 | + | |
| 751 | + | |
| 752 | + | |
| 753 | + | |
| 754 | + | |
| 755 | + | |
| 756 | + | |
| 757 | + | |
| 758 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
621 | 621 | | |
622 | 622 | | |
623 | 623 | | |
624 | | - | |
625 | | - | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
| 629 | + | |
| 630 | + | |
| 631 | + | |
| 632 | + | |
| 633 | + | |
| 634 | + | |
| 635 | + | |
| 636 | + | |
| 637 | + | |
| 638 | + | |
| 639 | + | |
| 640 | + | |
| 641 | + | |
| 642 | + | |
| 643 | + | |
| 644 | + | |
| 645 | + | |
| 646 | + | |
| 647 | + | |
| 648 | + | |
| 649 | + | |
| 650 | + | |
| 651 | + | |
| 652 | + | |
| 653 | + | |
| 654 | + | |
| 655 | + | |
| 656 | + | |
| 657 | + | |
| 658 | + | |
| 659 | + | |
| 660 | + | |
| 661 | + | |
626 | 662 | | |
627 | 663 | | |
628 | 664 | | |
| |||
648 | 684 | | |
649 | 685 | | |
650 | 686 | | |
651 | | - | |
| 687 | + | |
| 688 | + | |
| 689 | + | |
| 690 | + | |
| 691 | + | |
652 | 692 | | |
653 | 693 | | |
654 | 694 | | |
655 | 695 | | |
656 | 696 | | |
657 | | - | |
| 697 | + | |
| 698 | + | |
| 699 | + | |
658 | 700 | | |
659 | 701 | | |
660 | 702 | | |
661 | 703 | | |
662 | 704 | | |
663 | 705 | | |
664 | 706 | | |
665 | | - | |
| 707 | + | |
| 708 | + | |
| 709 | + | |
| 710 | + | |
| 711 | + | |
666 | 712 | | |
667 | 713 | | |
668 | 714 | | |
669 | 715 | | |
670 | 716 | | |
671 | 717 | | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
| 735 | + | |
| 736 | + | |
| 737 | + | |
| 738 | + | |
| 739 | + | |
| 740 | + | |
| 741 | + | |
| 742 | + | |
672 | 743 | | |
673 | 744 | | |
674 | 745 | | |
| |||
0 commit comments