Skip to content

Commit 7a25b39

Browse files
fix(lint): flow-region-shape-invalid points at the region container at fault (#29821)
<!-- claude-projects-attribution: {"github_login":"os-warren"} --> _Requested by **Warren** · [project thread](https://claude.ai/code/project/chan_01U5RdnAAmkSQmZbqh6FtqZ8?thread=cmsg_01U5RdnAAmkSQmZbqh6FtqZ8Ja6arWBs4nMY35SgDhwmfH)_ Fixes #29558 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:`flow-region-shape-invalid` 的发现不再只指向整个流程,而是指向引擎拒收时点名的那个区域容器(并行、循环、try_catch),路径与 where 都落到该节点。 - 为什么改:作者只看到「这个流程有问题」,找不到是哪个容器形状不对;引擎现在随拒收抛出带路径和节点 id 的 `FlowRegionShapeError`,lint 直接读它。 - 风险与回滚:只动发现的定位,消息原文一字未改;读不到该错误类型时退回整流程定位。回滚即 revert 本 PR。 - 先看:`packages/lint/src/validate-flow-region-shape.ts` 的 `containerAtFault()`,其后是同目录测试文件里新增的三个容器用例。 - 两分钟验证:`pnpm --filter @objectstack/lint exec vitest run src/validate-flow-region-shape.test.ts`,预期 10/10 通过。 - 需要你做的:无,席位落地。 ## What changes `validateFlowRegionShape` (`packages/lint/src/validate-flow-region-shape.ts`) now anchors its finding on the region container the engine's refusal names. Since #29719 (PR #29789), `validateControlFlow` throws the exported `FlowRegionShapeError`, carrying the container's `nodeId` and its `path` inside the flow. The new `containerAtFault()` reads both with one `instanceof FlowRegionShapeError` check: - `path` becomes the flow path plus the container path, in lint spelling: `flows[1]` + `['nodes', 1, 'config', 'try', 'nodes', 0]` gives `flows[1].nodes[1].config.try.nodes[0]` (`spellPath()`). - `where` becomes `flow 'NAME' › node 'ID'`. - `message` is unchanged: still the engine's sentence behind the flow's name. Still one rule, one judge, one spelling: there is no second call and nothing parses the sentence. - Any other error the engine throws names no container, so the flow stays the anchor, as before. A flow the flow schema refuses is unchanged too: it was never judged, so no container exists. **One finding per flow, which is one finding per container.** The engine throws once, at a flow's first malformed region, and the rule makes exactly the engine's call. Reporting further containers would need a second walk beside the engine's, which the card rules out. A flow with two malformed regions therefore still takes two fixes, as the rule's explanation already says. **Forced path-only pin moves**, each limited to the asserted path (claim 6593246940, amendments 6593285300 and the one adding the dogfood file): - `packages/cli/test/validate-flow-region-shape.test.ts`: the assembled-artifact case's `flows[1]` → `flows[1].nodes[1]`. - `packages/metadata-protocol/src/runtime-authoring-gate.flow-region.test.ts`: three cases move from `flows[0]` to the container. - `packages/metadata-protocol/src/protocol.draft-publish-verdict.test.ts`: the draft read's item-relative path `''` → `nodes.1.config.try.nodes.0`. - `packages/qa/dogfood/test/flow-region-malformed-publish.dogfood.test.ts`: the draft read's `"path":""` → `"path":"nodes.1"`, its comment, and one docblock phrase. A `@objectstack/lint` patch changeset carries the move. ## Why The card: an author reading `flow-region-shape-invalid` learned only that the flow was refused, not which `parallel`, `loop` or `try_catch` was malformed. The location now comes from the contract the spec half (#29719) added, not from the sentence. ## Tests - `packages/lint/src/validate-flow-region-shape.test.ts`: three new cases under `[#29558]` (a top-level `try_catch`, a one-branch `parallel` nested in a try region, a `loop` inside the second branch of a `parallel`), plus the first case and the non-record-entry case moved to the container path. 10/10. All at head `ccbd58d4`, each under `scripts/pm/os-verify-lock.sh`, verdict line quoted: <details><summary>Test runs</summary> - `pnpm --filter @objectstack/lint typecheck && pnpm --filter @objectstack/lint test`: VERDICT command-exit 0; Test Files 192 passed (192), Tests 7945 passed (7945). The lint typecheck covers its tests (`check:test-typecheck`). - `pnpm --filter @objectstack/lint exec vitest run --maxWorkers=2 src/validate-flow-region-shape.test.ts`: VERDICT command-exit 0; Tests 10 passed (10). - `pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2 test/validate-flow-region-shape.test.ts`: VERDICT command-exit 0; Tests 7 passed (7). `authoring-rule-command-parity.test.ts` is integration tier and unedited, so it is left to CI. - `pnpm --filter @objectstack/metadata-protocol exec vitest run --maxWorkers=2 src/runtime-authoring-gate.flow-region.test.ts src/protocol.draft-publish-verdict.test.ts` (after a lint build): VERDICT command-exit 0; Test Files 2 passed (2), Tests 57 passed (57). - `pnpm --filter '@objectstack/dogfood^...' build && pnpm --filter @objectstack/dogfood exec vitest run --maxWorkers=2 test/flow-region-malformed-publish.dogfood.test.ts`: VERDICT command-exit 0; Tests 3 passed (3). </details> <details><summary>Ablation (one, on the reader)</summary> `node scripts/ablation-replace.mjs` replaced the reader's one line (the `instanceof FlowRegionShapeError` return) with `return undefined;`: anchor x1 to x0, blob `37494f85f3c3` to `122265b776ae`. The rule's test file then went red, Tests 5 failed, 5 passed: the first case, the three `[#29558]` container cases and the non-record-entry case. Restored by the tool: blob after restore `37494f85f3c3` equals HEAD, `git diff HEAD` empty. The test imports the rule from source, so no build sits between the mutation and the run. </details> ## Acceptance notes - The resume commit (`56820bac`, cherry-picked as `b9930969`) missed one in-file pin, the non-record-entry case (`flows[2]` → `flows[2].nodes[1]`); fixed in its own commit. - The rule's `os explain` entry (`rule-explanations.ts`) says nothing about the location, so it was left alone. ## Verification - `node scripts/pm/dispatch-gates.mjs --commands` at `ccbd58d4` derived 63 families; all were run with exit codes recorded before any pipe; `--ran` reconciliation: 63 derived, 62 run, 1 NOT MEASURED, 0 unrun. - NOT MEASURED: `pnpm check:dual-build-cjs-loads`, reason: it reads built output of every package (PREREQUISITE NOT MET, exit 3, seven packages without `dist/`); CI's `Lint & Repo Gates` runs it. - `node scripts/check-plugin-teardown-shape.mjs --self-test` first exited 3 because its pinned fixture commit was outside this shallow clone; after fetching that commit it exited 0 (48 cases pass). - Every other derived family exited 0. ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (No `packages/spec` change.) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` runs in CI. - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: n/a. - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. --- _Generated by [Claude Code](https://claude.ai/code/session_01JQwxbWp3uyRGHgDR5oyquZ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 64646ae commit 7a25b39

7 files changed

Lines changed: 98 additions & 21 deletions
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/lint': patch
3+
---
4+
5+
fix(lint): `flow-region-shape-invalid` points at the region container at fault, not only the flow
6+
7+
Clause-②: no
8+
Metadata-protocol: none
9+
10+
A flow the automation engine refuses for its region shape used to be reported on the whole flow (`flows[1]`, `flow 'lead_time'`). The finding now names the container the engine's refusal names: its `path` runs to that node inside the flow (`flows[1].nodes[1].config.try.nodes[0]`) and its `where` reads `flow 'lead_time' › node 'fan'`. The location is read off the `FlowRegionShapeError` that `validateControlFlow` throws, never parsed from the sentence; the message is unchanged, still the engine's refusal behind the flow's name. The engine stops at a flow's first malformed region, so there is still one finding per flow. Any other error keeps the flow as the anchor, as before.
11+
12+
On the server, the save and publish verdict of an active flow and a draft's diagnostics carry the same move in item-relative spelling: the path was the item root and is now the container, for example `nodes.1`.

‎packages/cli/test/validate-flow-region-shape.test.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,9 @@ describe('[#28893] validate, build and lint refuse a flow region the engine refu
224224
// so the finding sits at the union path and names the flow in `where`.
225225
it('[#29059] an assembled artifact: the refused flow in the second packages[] entry is refused once, and build writes nothing', async () => {
226226
const { refusal } = CASES.assembled;
227-
const expected = { rule: RULE, where: "flow 'rgn_ops_lead_time'", path: 'flows[1]' };
227+
// [#29558] Forced edit: the finding is anchored on the container the
228+
// engine names (the try_catch at the flow's nodes[1]), not the flow.
229+
const expected = { rule: RULE, where: "flow 'rgn_ops_lead_time' › node 'guard'", path: 'flows[1].nodes[1]' };
228230

229231
const validated = await validate('assembled', '--json');
230232
expect(validated.code, validated.stdout).toBe(1);
@@ -243,7 +245,7 @@ describe('[#28893] validate, build and lint refuse a flow region the engine refu
243245
const linted = await lint('assembled');
244246
expect(linted.code, linted.stdout).toBe(1);
245247
const lintRows = (json(linted, 'lint assembled').issues as Array<Record<string, unknown>>).filter((i) => i.rule === RULE);
246-
expect(lintRows.map(({ path }) => path)).toEqual(['flows[1]']);
248+
expect(lintRows.map(({ path }) => path)).toEqual(['flows[1].nodes[1]']);
247249
}, 60_000);
248250

249251
it('CONTROL: the same flow with the join restored validates and builds', async () => {

‎packages/lint/src/validate-flow-region-shape.test.ts‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ describe('[#29049] flow-region-shape-invalid', () => {
5555
const bad = flow('rgn_lead_time', guard(fanOut(false)));
5656
const findings = validateFlowRegionShape({ flows: [flow('rgn_ok', guard(fanOut(true))), bad] });
5757
expect(findings.map(({ severity, rule, where, path }) => ({ severity, rule, where, path }))).toEqual([
58-
{ severity: 'error', rule: FLOW_REGION_SHAPE_INVALID, where: "flow 'rgn_lead_time'", path: 'flows[1]' },
58+
{ severity: 'error', rule: FLOW_REGION_SHAPE_INVALID, where: "flow 'rgn_lead_time' › node 'guard'", path: 'flows[1].nodes[1]' },
5959
]);
6060
expect(findings[0]!.message).toBe(`Flow 'rgn_lead_time' would be refused at registration: ${engineRefusal(bad)}`);
6161
expect(findings[0]!.message).toContain("try_catch 'guard' catch: region must be single-exit");
@@ -67,6 +67,40 @@ describe('[#29049] flow-region-shape-invalid', () => {
6767
expect(finding?.message).toContain("parallel 'fan': a parallel block needs at least 2 branches");
6868
});
6969

70+
describe('[#29558] the finding is anchored on the container the engine names', () => {
71+
it('a top-level container: the path and where name the try_catch at fault, the message is unchanged', () => {
72+
const bad = flow('rgn_lead_time', guard(fanOut(false)));
73+
const [finding, ...rest] = validateFlowRegionShape({ flows: [flow('rgn_ok', guard(fanOut(true))), bad] });
74+
expect(rest).toEqual([]);
75+
expect(finding).toMatchObject({ path: 'flows[1].nodes[1]', where: "flow 'rgn_lead_time' › node 'guard'" });
76+
expect(finding!.message).toBe(`Flow 'rgn_lead_time' would be refused at registration: ${engineRefusal(bad)}`);
77+
});
78+
79+
it('a nested container: a one-branch parallel inside a try region is anchored where it sits', () => {
80+
const bad = flow('rgn_nested', n('guard', 'try_catch', {
81+
config: {
82+
try: { nodes: [n('fan', 'parallel', { config: { branches: [{ nodes: [n('only')], edges: [] }] } })], edges: [] },
83+
catch: { nodes: [n('recover')], edges: [] },
84+
},
85+
}));
86+
const [finding] = validateFlowRegionShape({ flows: [bad] });
87+
expect(finding).toMatchObject({ path: 'flows[0].nodes[1].config.try.nodes[0]', where: "flow 'rgn_nested' › node 'fan'" });
88+
expect(finding!.message).toBe(`Flow 'rgn_nested' would be refused at registration: ${engineRefusal(bad)}`);
89+
});
90+
91+
it('a container in a parallel branch: the branch index is part of the path', () => {
92+
const looped = n('sweep', 'loop', {
93+
config: { collection: [1, 2], iteratorVariable: 'row', body: fanOut(false) },
94+
});
95+
const bad = flow('rgn_branch', n('fan', 'parallel', {
96+
config: { branches: [{ nodes: [n('left')], edges: [] }, { nodes: [looped], edges: [] }] },
97+
}));
98+
const [finding] = validateFlowRegionShape({ flows: [bad] });
99+
expect(finding).toMatchObject({ path: 'flows[0].nodes[1].config.branches[1].nodes[0]', where: "flow 'rgn_branch' › node 'sweep'" });
100+
expect(finding!.message).toContain("loop 'sweep' body");
101+
});
102+
});
103+
70104
it('CONTROL: the same flow with the join restored has no finding, nor does a stack with no flows', () => {
71105
expect(validateFlowRegionShape({ flows: [flow('rgn_lead_time', guard(fanOut(true)))] })).toEqual([]);
72106
expect(validateFlowRegionShape({})).toEqual([]);
@@ -84,7 +118,7 @@ describe('[#29049] flow-region-shape-invalid', () => {
84118
it('a non-record entry is no flow and draws nothing; the flow after it keeps its own index', () => {
85119
const bad = flow('rgn_lead_time', guard(fanOut(false)));
86120
const findings = validateFlowRegionShape({ flows: [null, 'x', bad] });
87-
expect(findings.map(({ path }) => path)).toEqual(['flows[2]']);
121+
expect(findings.map(({ path }) => path)).toEqual(['flows[2].nodes[1]']);
88122
});
89123

90124
it('a body with no string name is named by its path', () => {

‎packages/lint/src/validate-flow-region-shape.ts‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,13 @@
1717
* `[Automation] failed to register flow` and the boot goes on without it, so
1818
* its trigger never fires.
1919
*
20+
* The finding is anchored on the container the refusal names (#29558): the
21+
* engine throws a `FlowRegionShapeError` carrying the container's node id and
22+
* its path inside the flow, and the finding's `path` and `where` are read off
23+
* it, never off the sentence. The engine throws once, at a flow's first
24+
* malformed region, so there is one container per finding and one finding per
25+
* flow. Anything else it throws names no container; the flow stays the anchor.
26+
*
2027
* ## ⛔ The verdict is the engine's — nothing here restates the region rules
2128
*
2229
* Each flow goes through the same two calls `registerFlow` makes: `FlowSchema`,
@@ -49,7 +56,7 @@
4956
* exported; region bodies ARE cycle-checked, by `validateControlFlow`.
5057
*/
5158

52-
import { FlowSchema, validateControlFlow } from '@objectstack/spec/automation';
59+
import { FlowRegionShapeError, FlowSchema, validateControlFlow } from '@objectstack/spec/automation';
5360

5461
type AnyRec = Record<string, unknown>;
5562

@@ -63,9 +70,15 @@ export interface FlowRegionShapeFinding {
6370
severity: 'error';
6471
/** Diagnostic rule id. */
6572
rule: string;
66-
/** Human-readable location, e.g. `flow 'lead_time'`. */
73+
/**
74+
* Human-readable location: `flow 'lead_time' › node 'guard'` when the
75+
* engine's refusal names the container at fault, else `flow 'lead_time'`.
76+
*/
6777
where: string;
68-
/** Config path of the flow, e.g. `flows[0]`. */
78+
/**
79+
* Config path of the container at fault inside the flow, e.g.
80+
* `flows[0].nodes[1].config.try.nodes[0]`, else of the flow, `flows[0]`.
81+
*/
6982
path: string;
7083
/** `Flow '<name>' would be refused at registration: ` and the engine's refusal. */
7184
message: string;
@@ -80,6 +93,20 @@ const HINT =
8093
const SCHEMA_HINT =
8194
'Fix the flow so the flow schema accepts it (`os validate` names each issue); its regions are judged then.';
8295

96+
/**
97+
* The container the engine's refusal names, read off the refusal itself:
98+
* `validateControlFlow` throws `FlowRegionShapeError`, carrying the
99+
* container's node id and its path inside the flow. Anything else it throws
100+
* names no container, and the flow stays the anchor.
101+
*/
102+
function containerAtFault(err: unknown): { nodeId: string; path: readonly (string | number)[] } | undefined {
103+
return err instanceof FlowRegionShapeError ? { nodeId: err.nodeId, path: err.path } : undefined;
104+
}
105+
106+
/** `['nodes', 1, 'config', 'try']` → `.nodes[1].config.try`, the lint path spelling. */
107+
const spellPath = (segments: readonly (string | number)[]): string =>
108+
segments.map((s) => (typeof s === 'number' ? `[${s}]` : `.${s}`)).join('');
109+
83110
/**
84111
* Every flow of `stack.flows` the automation engine would refuse to register
85112
* for its region shape, one finding each. Pure.
@@ -112,11 +139,12 @@ export function validateFlowRegionShape(stack: AnyRec): FlowRegionShapeFinding[]
112139
validateControlFlow(result.data);
113140
} catch (err) {
114141
const name = result.data.name;
142+
const container = containerAtFault(err);
115143
findings.push({
116144
severity: 'error',
117145
rule: FLOW_REGION_SHAPE_INVALID,
118-
where: `flow '${name}'`,
119-
path,
146+
where: container ? `flow '${name}' › node '${container.nodeId}'` : `flow '${name}'`,
147+
path: container ? `${path}${spellPath(container.path)}` : path,
120148
message: `Flow '${name}' would be refused at registration: ${err instanceof Error ? err.message : String(err)}`,
121149
hint: HINT,
122150
});

‎packages/metadata-protocol/src/protocol.draft-publish-verdict.test.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -634,13 +634,13 @@ describe('a draft reads the verdict its publish gives (#26707)', () => {
634634
expect(published).toMatchObject({ success: true, publishedCount: 3, failedCount: 0 });
635635
});
636636

637-
it('[#29233] the region verdict on the draft read is the registry rule\'s: the engine\'s sentence, at the item root', async () => {
637+
it('[#29233] the region verdict on the draft read is the registry rule\'s: the engine\'s sentence, at the container it names (#29558)', async () => {
638638
const { engine } = makeStubEngine();
639639
const protocol = new ObjectStackProtocolImplementation(engine);
640640
// A one-branch parallel block nested in a try region. The registry rule
641-
// judges the flow as one unit (as the engine does) and anchors the
642-
// flow itself, so the item-relative path is the item root; the
643-
// sentence names the region.
641+
// judges the flow as one unit (as the engine does) and, since #29558,
642+
// anchors the container the engine's refusal names, so the
643+
// item-relative path is that parallel node inside the try region.
644644
const flow = regionFlow('nested_region', {
645645
id: 'guard', type: 'try_catch', label: 'Guard',
646646
config: {
@@ -656,7 +656,7 @@ describe('a draft reads the verdict its publish gives (#26707)', () => {
656656
const diagnostics = await draftDiagnostics(protocol, 'flow', flow.name);
657657
expect(diagnostics.valid).toBe(false);
658658
expect((diagnostics.errors ?? []).filter((e) => e.code === 'flow-region-shape-invalid')).toEqual([{
659-
path: '',
659+
path: 'nodes.1.config.try.nodes.0',
660660
message: "Flow 'nested_region' would be refused at registration: parallel 'fan': a parallel block needs at least 2 branches",
661661
code: 'flow-region-shape-invalid',
662662
}]);

‎packages/metadata-protocol/src/runtime-authoring-gate.flow-region.test.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,9 @@ describe('[#29233] the publish gate refuses the regions the engine refuses, thro
8282
expect(issues).toEqual([{
8383
severity: 'error',
8484
rule: RULE,
85-
where: "flow 'rgnprobe_loop'",
86-
path: 'flows[0]',
85+
// [#29558] Anchored on the container the engine names, not the flow.
86+
where: "flow 'rgnprobe_loop' › node 'lp'",
87+
path: 'flows[0].nodes[1]',
8788
message: refusedAt('rgnprobe_loop', sentence),
8889
hint: expect.any(String),
8990
}]);
@@ -92,7 +93,7 @@ describe('[#29233] the publish gate refuses the regions the engine refuses, thro
9293
it('a parallel block with one branch is refused', () => {
9394
const flow = flowOf('one_branch', [{ id: 'fan', type: 'parallel', label: 'Fan', config: { branches: [oneStep('only')] } }]);
9495
expect(regionIssues(flow).issues).toEqual([
95-
expect.objectContaining({ path: 'flows[0]', message: refusedAt('one_branch', engineRefusal(flow)) }),
96+
expect.objectContaining({ path: 'flows[0].nodes[1]', message: refusedAt('one_branch', engineRefusal(flow)) }),
9697
]);
9798
});
9899

@@ -120,7 +121,7 @@ describe('[#29233] the publish gate refuses the regions the engine refuses, thro
120121
const sentence = engineRefusal(flow)!;
121122
expect(sentence).toMatch(/^try_catch 'guard' try → loop 'inner' body: /);
122123
expect(regionIssues(flow).issues).toEqual([
123-
expect.objectContaining({ path: 'flows[0]', message: refusedAt('nested', sentence) }),
124+
expect.objectContaining({ path: 'flows[0].nodes[1].config.try.nodes[0]', message: refusedAt('nested', sentence) }),
124125
]);
125126
});
126127

‎packages/qa/dogfood/test/flow-region-malformed-publish.dogfood.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
// ## What these cases pin
1717
//
1818
// The draft still saves (drafts are never gated). Its read answers
19-
// `valid: false` with the `flow-region-shape-invalid` finding on the flow, in
19+
// `valid: false` with the `flow-region-shape-invalid` finding on the container, in
2020
// the engine's own sentence; `publish-drafts`, the per-item publish and a live
2121
// save each refuse it before any write. Control: a well-formed loop, parallel
2222
// and try_catch region each read valid, publish, and the engine registers them.
@@ -117,8 +117,8 @@ describe('dogfood: a flow with a malformed region is refused at publish, and its
117117
const diagnostics = JSON.stringify(read.json);
118118
expect(diagnostics).toContain('"valid":false');
119119
expect(diagnostics).toContain('"code":"flow-region-shape-invalid"');
120-
// The registry rule anchors the flow itself: the item root, in item-relative spelling.
121-
expect(diagnostics).toContain('"path":""');
120+
// The registry rule anchors the container the engine names (the loop at nodes[1]), in item-relative spelling.
121+
expect(diagnostics).toContain('"path":"nodes.1"');
122122
expect(diagnostics).toContain(JSON.stringify(`Flow 'rgnprobe_loop' would be refused at registration: ${REGION_SENTENCE}`));
123123

124124
const published = await call('POST', `/packages/${PKG_BAD}/publish-drafts`, {});

0 commit comments

Comments
 (0)