You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit ca78860
Browse filesBrowse the repository at this point in the historyBrowse files
fix(lint): a flow's edges list drops a non-record member instead of throwing (#18103)
Closes#16910
Clause-②: no
The diff is `packages/lint/src/**` plus one changeset. It touches no
path under `packages/spec/src/**` — the contract surface
`SUSPECT_TIER_GLOBS` declares — adds no schema, no published error code
and no new finding id, so the PR-body carrier agrees with the card's.
Self-read with `readClause2Line()` against `CLAUSE2_KEY_LINE` taken
fresh from `scripts/pm/check-clause2-carriers.mjs` (three capture
groups; the value is the last): `{ kind: 'declared', value: 'no' }`.
## What was wrong
`lintFlowPatterns` read `.label` off each member of a flow's `edges`
list behind nothing but an `Array.isArray` check, which proves the LIST
and never its MEMBERS. A YAML `edges:` item left empty deserialises to
`null`, so hand-written metadata turned `objectstack validate` into an
uncaught `TypeError` out of a function contractually typed `(stack) =>
FlowLintFinding[]`.
Reproduced on `origin/main` at `66e34d14d9` before any edit — the same
frame the card names:
```
edges:[null, valid] threw=YES TypeError: Cannot read properties of null (reading 'label')
at scanErrorLabelledEdges (lint-flow-patterns.ts:691:28)
edges:[undefined, valid] threw=YES TypeError: Cannot read properties of undefined (reading 'label')
edges:['a string', valid] threw=NO findings=1
edges:[42, valid] threw=NO findings=1
CONTROL edges:[valid] findings=1 rules=flow-error-label-not-fault
```
A linter that throws instead of reporting takes the whole gate down on
exactly the malformed document it exists to catch, and the author gets a
stack trace where a diagnostic belongs.
## The semantics chosen, and why it matches #16751
**The junk member is DROPPED, silently**, through `recordsOf`'s `isRec`
filter — the same coercion, from the same one home (`object-graph.ts`),
that #16751 chose for the seven flow-NODE-list readers. Two sibling
lists on one flow member now cannot disagree about what a malformed
member means. Dropping is not reporting: inventing a finding about an
entry no author wrote is the phantom half of this same defect class,
which is why every assertion below is an equality against a control and
not a lower bound.
## Positive control — the valid edge is still judged
Not throwing is half a contract; a guard that abandoned the list would
satisfy it and would have traded the crash for silence. Pinned by
IDENTITY (rule id and `where`), not by "did not throw", at both
addressing depths and for all four bad-member kinds:
```
edges:[null, valid] findings=1 flow-error-label-not-fault where="flow 'crm_flow' · edge 'act' → 'done'"
edges:[undefined, valid] findings=1 (identical)
edges:['a string', valid] findings=1 (identical)
edges:[42, valid] findings=1 (identical)
CONTROL edges:[valid] findings=1 (identical)
```
and the whole `lintFlowPatterns(...)` result is asserted `toEqual` the
control's, so nothing is invented either. The family sweep adds the same
pin over the WHOLE rule table (`AUTHORING_RULES`), for `null` /
`undefined` / a string / a number / an array, at both depths —
`RESIDUAL_THROWS` and `RESIDUAL_INVENTED` both stay empty.
## The census — list-shaped record reads in `lint-flow-patterns.ts`
Triage asked how many exist and how many go through `recordsOf`. Read
off the file, not off memory. **Nine sites; four went through
`recordsOf` before this PR, six do now, three never did and do not need
to.** Zeros reported.
| # | Site | Before | After |
|---|---|---|---|
| 1 | `:1430` `recordsOf(stack.flows)` | `recordsOf` | unchanged |
| 2 | `:1441` `recordsOf(flow.nodes)` | `recordsOf` | unchanged |
| 3 | `:462` `recordsOf(graph.nodes)` — `findDataNodeAnywhere` |
`recordsOf` | unchanged |
| 4 | `:1541` `recordsOf(graph.nodes)` — main walk | `recordsOf` |
unchanged |
| 5 | `:1442` `flow.edges` via `Array.isArray` cast | ⛔ member-blind —
**THREW** | **`recordsOf`** |
| 6 | `:1542` `graph.edges as unknown as AnyRec[]` | ⛔ member-blind —
**THREW** | **`recordsOf`** |
| 7 | `:1276` `regionNodesOf` — `Array.isArray(nodes)` cast | not
`recordsOf`; **member-safe** | unchanged |
| 8 | `:866` `cfg.conditions` via `Array.isArray` cast | not
`recordsOf`; **member-safe** | unchanged |
| 9 | `:1359` `cfg[slot]` region `branches` list | not `recordsOf`;
**member-safe** | unchanged |
**The eighth triage predicted is real, and it is #6.** It is not
reachable through the call-site coercion: a nested region's edge list is
read out of a container's open `z.record` config by `collectFlowGraphs`
behind only `Array.isArray`. Measured on a tree carrying the top-level
repair alone, a nested `body.edges` holding `null` still threw from the
identical `scanErrorLabelledEdges` frame.
Sites 7–9 stay as they are, deliberately, and each was checked rather
than assumed: `regionNodesOf`'s only consumer guards with `if (!child ||
typeof child !== 'object') continue`, `cfg.conditions` is read with
optional chaining (`c?.label`), and the `branches` list hands every
member to `regionNodesOf`, which refuses a non-object. Re-pointing them
at `recordsOf` would change no behaviour and would add copies for
`collection-coercion-single-copy.test.ts` to count.
⭐ **A tenth and eleventh site, one file over, and they are in this PR.**
`os validate` runs the rule TABLE, so one throwing reader takes every
other rule's verdict down with it. The moment `lintFlowPatterns` stopped
throwing, the new sweep arm went red on `validateStackExpressions`
(`validate-expressions.ts`), which read the same list through the same
double cast at `:1405` and handed `flow.edges` on raw out of a `{
...flow }` spread. Both are re-pointed here. Fixing only the filed rule
would have satisfied the card's letter and left the gate down on the
same document.
## Ablation — which reader actually carried the crash
Each reader reverted one at a time on top of the final commit, the
mutation proved on disk (`grep -cF` on both texts plus a `git
hash-object` blob-changed check), then restored. ⛔ Not settled by an
exit code and ⛔ not by `trap` alone: every restore is settled by `git
hash-object` equal to the HEAD blob AND an empty `git diff HEAD`, both
printed.
| Ablation | Result |
|---|---|
| A · `lint-flow-patterns.ts` `flow.edges` back to the cast | **519
passed, 0 failed** — green |
| B · `lint-flow-patterns.ts` `graph.edges` back to the cast | **10
failed** / 509 passed |
| C · `validate-expressions.ts` `graph.edges` back to the cast | **6
failed** / 513 passed |
| D · `validate-expressions.ts` `flow.edges` to a member-blind cast |
**519 passed, 0 failed** — green |
⭐ **Reported as measured, not as predicted.** Both edge walks read
`graph.edges`, because `collectFlowGraphs` re-exposes whatever array it
is handed — so B and C are the load-bearing repairs and A and D are
**defence in depth, not the fix**. A and D are kept deliberately: they
hand the COERCED array to `collectFlowGraphs` instead of the raw one,
which is the discipline the node lists already follow, and they keep two
sibling lists on one flow member reading the same way. ⛔ Read them as
belt and braces, not as one repair written twice.
⚠️ D's first run was a no-op — the anchor arguments were inverted, the
replacement asserted zero occurrences, and a hole in the landing check
(it could not tell "swapped" from "never had the new text") reported it
as landed. That reading was **discarded as void**, the check was
strengthened with the blob-changed assertion above, and the row in the
table is the re-run.
## Reverse-read — which existing sentence does this make false
**One sentence changed truth value, and it changed in the direction that
is easy to miss.** `findDataNodeAnywhere`'s docblock
(`lint-flow-patterns.ts:450-453`) says the arrays handed in "are the
ones the caller already coerced through `recordsOf`, so a malformed
member cannot make this throw". Its call site at `:1505` passes `nodes,
edges` — and before this PR `edges` was NOT coerced, so that sentence
was **already false about half its subject**. This change makes it true.
It is left unedited because it is now accurate as written.
**Nothing became false.** The comments at `:1432-1440` and `:1537-1540`
are scoped to the node lists and stay true; `:1531-1533` ("a non-array
`nodes` still cannot throw") stays true. The
`non-record-object-entry.test.ts` narrative sentences carrying issue
numbers and dates are history and were not touched.
**A bare present-tense count DID rot, and its ratchet caught it rather
than my reading it.** `validate-expressions.test.ts`' read-surface table
pinned `flow` as reading exactly `['name', 'nodes']`; the new literal
`flow.edges` read makes that `['edges', 'name', 'nodes']`. That table
exists to force a deliberate visit when a read is added, so the visit is
the update, with the reason recorded beside it. `edges` is declared by
`ObjectStackSchema.flows[]`, so the guard's second half (every key read
is a key the spec declares) still holds and `TRACKED_UNDECLARED_READS`
stays empty.
**Tests pinning today's throwing behaviour: ZERO.** Searched `toThrow`
across `packages/lint/src/*.test.ts` — no case anywhere asserted that a
flow edge list throws, in either file. Nothing was re-judged and nothing
was deleted.
## Verification
- `pnpm --filter @objectstack/lint test` — **103 files, 3817 passed, 5
skipped, 0 failed** (`VERDICT command-exit 0` under the shared verify
lock).
- `pnpm --filter @objectstack/lint typecheck` — exit 0, including
`check:test-typecheck`.
- `pnpm --filter '@objectstack/lint^...' build` — dependency closure
built before anything was judged (`VERDICT command-exit 0`).
- **Repo-wide lint, not narrowed**: `node --stack-size=4000 eslint .
--no-inline-config --format json` ran to completion in the foreground at
`4b70081eb8` — **6743 files linted, 0 errors, 0 warnings, exit 0**. The
population is read from eslint's own config and the count from its
`--format json` output; no narrowing was claimed or needed.
- **Derived gate family**, from `scripts/pm/dispatch-gates.mjs
--commands --repo objectstack-ai/objectstack` over the real change set
(it takes the merge base itself), reconciled with `--ran` carrying exit
codes: **59 derived, 59 accounted, 56 green, 0 UNRUN.** The 3 non-green
are `check:dual-build-cjs-loads`, `check:lean-entry-closure` and
`check:type-check-debt`, each `exit 3` = the gate's own `PREREQUISITE
NOT MET` (they read a whole-repo `dist/`). ⛔ Those are NOT MEASURED —
neither a pass nor a failure — and CI builds the repo before running
them.
## Changeset — measured, not defaulted
**A changeset, ⛔ not `skip-changeset`.** `@objectstack/lint` is
published (no `private`), its `files[]` is `["dist", "README.md",
"CHANGELOG.md"]`, and after `pnpm --filter @objectstack/lint build` the
changed modules are present in `dist/index.js` and `dist/runtime.js`
with a positive control (`flow-error-label-not-fault` found) and a
negative control (the test-only `underNestedRegionEdges` absent).
Behaviour on malformed input changes on a shipped path, so `patch`. No
`skip-changeset` label applied.
**No new published finding id.** No `FLOW_*` rule constant is added and
no new diagnostic is emitted; on every well-formed document the output
is byte-identical. The only behaviour that changes is on input that
previously crashed.
## 验收备注
- **filed as #18102** — `collectFlowGraphs` (`packages/spec`) filters
the NODE members it hands out (#16752) and forwards `edges` untouched,
so `FlowGraph.edges` is declared `readonly FlowEdgeParsed[]` and can
hold a non-record. Latent, ⛔ not a live crash: both `graph.edges`
consumers are coerced by this PR and the producer never dereferences an
edge member. Filed rather than fixed here because it is a different
package and would widen this PR's verification surface past the card's
declared landing point. Deduped first — the REST search endpoint is 403
by proxy design and the MCP search quota was exhausted, so the
documented fallback was used (REST list of `domain:devx` issues,
`state=all`, 198 read, local grep) with a positive control:
`collectFlowGraphs` hit 2 cards including the known #16751, while
`FlowGraph.edges` and `isRegionDict` hit zero.
- **noted, not filed** — `validate-expressions.ts:1200`'s
`collectFlowGraphs({ ...flow, nodes, edges })` still spreads `flow`,
though the callee reads only `nodes` and `edges`. The spread is now
inert. Successor: none — no PR or person is heading for that line, and
removing it would be an unmeasured behaviour change for no gain.
- ⚠️ **Scope declaration**: `validate-expressions.ts` and
`validate-expressions.test.ts` are edited beyond the card's named file.
Taken in place under the bounded-repair exemption — identical defect
class, mechanical repair whose shape is already pinned by #15793 at
`:1279` in that same file, no open PR touches `packages/lint` (5 open
PRs checked), and it adds no verification surface: the sweep arm this
card required is what caught it and what pins it. The claim comment is
the PM's and carries no file list, so this paragraph is the file-surface
declaration.
---
_Generated by [Claude
Code](https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU)_
Co-authored-by: Claude <noreply@anthropic.com>
A flow's `edges` list no longer takes the whole authoring gate down when one of its members is not a record — the sibling list #16751's repair did not reach (#16910).
6
+
7
+
`lintFlowPatterns` read `.label` off each member of `flow.edges` behind nothing but an `Array.isArray` check, which proves the LIST and never its MEMBERS. A YAML `edges:` list item left empty deserialises to `null`, so hand-written metadata turned `objectstack validate` into an uncaught `TypeError` out of a function contractually typed `(stack) => FlowLintFinding[]`:
A linter that throws instead of reporting fails hardest on exactly the documents it is most needed for, and the author gets a stack trace where a diagnostic belongs.
15
+
16
+
-**The junk member is DROPPED, silently**, through `recordsOf` — the same coercion, from the same one home (`object-graph.ts`), that #16751 chose for the seven flow-NODE-list readers, so two sibling lists on one flow member cannot disagree about what a malformed member means.
17
+
- ⭐ **The valid edge beside it is still JUDGED.** "No longer throws" is half a contract: a guard that abandoned the list would satisfy it and would have traded the crash for silence. Measured against a control holding the same flow without the junk member, the surviving finding is identical in rule and location, and no finding is invented about an entry no author wrote.
18
+
-**Two rules, not one.**`os validate` runs the rule TABLE, so one throwing reader takes every other rule's verdict down with it: once `lintFlowPatterns` stopped throwing, the identical defect surfaced one file over in `validateStackExpressions`, which read the same list through the same double cast. Both are repaired here; repairing only the filed one would have left the gate down on the same document.
19
+
- ⭐ **Which reader actually carried the crash, measured by ablation** — both edge walks read `graph.edges`, not the flow's own list, because `collectFlowGraphs` re-exposes whatever array it is handed. Reverting `graph.edges` alone in either file reds the new cases (10 failures in `lintFlowPatterns`, 6 in `validateStackExpressions`); reverting either `flow.edges` coercion alone leaves them green. The two `flow.edges` coercions are therefore **defence in depth, not the load-bearing fix**, and are kept deliberately: they hand the COERCED array to `collectFlowGraphs` rather than the raw one, which is the discipline the node lists already follow, and they keep two sibling lists on one flow member reading the same way. ⛔ Read them as belt and braces, not as one repair written twice.
20
+
-**The producer's edge side is still member-blind.**`collectFlowGraphs` filters the nodes it hands out and forwards edges untouched, so `FlowGraph.edges` is declared `FlowEdgeParsed[]` and can contain a non-record. It does not dereference them today, which is why the consumer coercion is sufficient; that asymmetry is filed separately rather than widened here.
21
+
-**No new finding id and no new diagnostic.** On every well-formed document the output is byte-identical; the only behaviour that changes is on input that previously crashed.
0 commit comments