Skip to content

fix(lint): read every flow node list through recordsOf - #16916

Merged
baozhoutao merged 3 commits into
mainfrom
claude/issue-16751-flow-node-list-recordsof
Sep 8, 2026
Merged

fix(lint): read every flow node list through recordsOf#16916
baozhoutao merged 3 commits into
mainfrom
claude/issue-16751-flow-node-list-recordsof

Conversation

@baozhoutao

@baozhoutao baozhoutao commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Part of #16751

Clause-②: no

Re-declared by the domain:devx PM seat at 18:1xZ. ⚠️ This line was added once at 16:29Z and was lost when the body was rewritten during the merge-conflict repair — the declaration lives in the body, so an ordinary body edit destroys it silently. Filed as a finding. The judgement is unchanged: the diff moves packages/lint/src/**, never packages/spec/src/**; it puts no new key on any published payload and narrows nothing an author may write — it stops an existing reader throwing on input the linter already accepted in principle. ⇒ the axis stands down and the patch grade holds.

lintFlowPatterns crashed on an ordinary flow. A YAML nodes: list item left empty deserialises to null, and the rule read nodes.find(n => n.type === 'start') off a list it had only Array.isArray-checked, so an author's own metadata turned objectstack validate into an uncaught TypeError out of a function contractually typed (stack) = Finding[]. Seven readers across three modules held that spelling; all now coerce through recordsOf.

⚠️ Merged main at 804192a5dc — one conflict, and it moved two of the measurements below

#16922 (the #16752 producer repair) landed at 17:00:41Z, after this branch was cut, and it touched this PR's pin file. The conflict was in RESIDUAL_THROWS, and both sides emptied it from opposite ends:

flows[].nodes rows flows[].nodes[].config.body.nodes rows
origin/main (producer repair) kept — still throwing there removed
this branch (consumer repair) removed removed
merged removed removedRESIDUAL_THROWS = {}

Resolved to {}, every other hunk kept from both sides, and then re-run rather than reasoned about: pnpm --filter @objectstack/lint test is green on the merge, 3583 tests, with no arm suppressed.

The docblock is resolved to carry both accounts. My earlier note called the #16752 rows "misattributed"; that was half the story and is corrected in the file and here.

⭐ Correction: the graph-shaped arms are green for TWO independent reasons

Measured on the tree before either fix, the two flows[].nodes[].config.body.nodes throws carried consumer frames — lintFlowPatterns at its own graph.nodes reader, validateStackExpressions at collectFlowVariableNames' unguarded graph.nodes walk — neither inside packages/spec. That reading was correct, and it was also incomplete: #16922 found a real producer defect behind it, namely that what collectFlowGraphs returned did not match its declared FlowNodeParsed[].

Both are true and neither repair makes the other unnecessary. The producer's declared element type lied, AND the consumers dereferenced without a guard. Remove the producer fix and the junk member is handed out again to every other consumer; remove the consumer coercion and these two readers are back to trusting a declared element type. ⛔ A later reader must not take my note as evidence #16922 was unnecessary — it was not.

⭐ Correction: the onward-handoff ablation no longer reproduces, and that is #16922's doing

This is the card's "non-obvious half", so both measurements are given rather than the convenient one.

On the pre-merge tree, collectFlowGraphs FORWARDED a non-record member into the graph it yielded:

collectFlowGraphs({ nodes: [null, valid], edges: [] })   ->   graph nodes = ["null","object"]

and an ablation of exactly the wrong fix — coerce for the local .find(), hand flow.nodes RAW onward, restore the bare cast at both graph.nodes readers — made the crash relocate rather than disappear:

lintFlowPatterns  nodes:[null, valid]   threw=YES  TypeError: reading 'id'   at lint-flow-patterns.ts:1548:49

On the merged tree the same ablation, byte-identical, throws nothing, because #16922 now drops the member at the producer:

collectFlowGraphs({ nodes: [null, valid], edges: [] })   ->   graph nodes = ["object"]
ablation of the wrong fix                                ->   threw=NO

So: the handoff condition the card raised was load-bearing and is now belt-and-braces for this shape. It stays in the shipped code on contract grounds rather than crash grounds — collectFlowGraphs declares FlowNodeParsed[], so handing it raw authored metadata is calling it out of contract, and a consumer should not depend on another package's filter for its own totality. ⛔ What this PR does not claim is that the handoff is what removes the crash today. It was, before 17:00:41Z.

Both ablation legs proved the mutation on disk (anchor grep -c before/after plus the blob hash moving off 81e48ead) and both restore legs verified byte-identical (git checkout HEAD -- path, hash back to 81e48ead, git diff HEAD empty), under a trap on EXIT/INT/TERM with an absolute repo root. No dist/ was involved: the harness imports packages/lint/src/*.ts directly under tsx.

Before / after, on the merged tree

The harness is temporary and is not in the diff.

                                                   BEFORE (44c849c7d6)          AFTER (804192a5dc, merged)
1. THE REPRO   nodes:[null, valid]                 threw=YES TypeError:         threw=NO  findings=0
                                                     Cannot read properties
                                                     of null (reading 'type')
                                                     at lint-flow-patterns.ts:1430:39

2. NONSENSE CONTROL  nodes:['a string', valid]     threw=NO  findings=0         threw=NO  findings=0
                     nodes:[42, valid]             threw=NO  findings=0         threw=NO  findings=0
                     nodes:[[], valid]             threw=NO  findings=0         threw=NO  findings=0

3. FIRING CONTROL    lintFlowPatterns(null)        threw=YES TypeError:         threw=YES TypeError:
                                                     reading 'flows'              reading 'flows'
   (an independent always-throwing call, so the AFTER run still proves the harness can SEE a throw)

4. POPULATION PIN    control nodes:[valid, finder]           findings=1                   findings=1
                     junk=null                     THREW                        findings=1  same=true
                     junk=undefined                THREW                        findings=1  same=true
                     junk='a string'               findings=1  same=true        findings=1  same=true
                     junk=42                       findings=1  same=true        findings=1  same=true
                     junk=[]                       findings=1  same=true        findings=1  same=true

6. THE MASKED HALF   collectFlowVariableNames      threw=YES TypeError:         threw=NO  names=1
                     graph.nodes:[null, valid]       reading 'id'
                                                     at flow-variable-scope.ts:225:27

7. THE HARDENING     validateFlowTemplatePaths     threw=NO  findings=0         threw=NO  findings=0
                     nodes:[null, startRC]

The repro is the card's, verbatim:

lintFlowPatterns({
  objects: [{ name: 'crm_account', fields: [{ name: 'name', type: 'text' }] }],
  flows: [{ name: 'crm_flow', nodes: [null, { id: 'start', type: 'start', config: {} }], edges: [] }],
});

Row 1 is the whole card, and it is not covered by #16922: flow.nodes is a list the rule reads itself, before any producer sees it. Row 6 was reachable only at a region nest of exactly MAX_REGION_DEPTH; it now cannot throw at any depth.

The three validate-flow-template-paths.ts sites were NOT throwing

Stated plainly, because the diff does not show it: boundObjectOf, declaredExpandOf and the per-flow start lookup in validateFlowTemplatePaths never crashed and are a hardening, not a bug fix. Row 7 above is threw=NO on both sides.

They survived on one character. The reader that threw is nodes.find((n) => n.type === 'start'); these three are nodes.find((n) => n?.type === 'start'). Nothing maintained that difference, and the optional chain reads as redundant beside the Array.isArray directly above it — so the next person to tidy it deletes it and the defect is back. The coercion is made where it has a home and the now-genuinely-redundant ?. goes with it. (The ?.config after the .find() at the third site stays: .find() really can return undefined.)

recordsOf gains no copy

collection-coercion-single-copy.test.ts counts declarations of the shape (v: unknown): AnyRec[] over packages/lint/src/*.ts:

BEFORE  90 modules scanned, 3 declarations:  object-graph.ts:recordsOf
                                             validate-list-view-field-refs.ts:asArray   (COPY_LEDGER)
                                             validate-object-field-refs.ts:asArray      (COPY_LEDGER)
AFTER   90 modules scanned, 3 declarations:  identical

Verification, re-run in full on the merged head 804192a5dc

Exit codes captured before any pipe; heavy legs through scripts/pm/os-verify-lock.sh, verdicts read off its printed VERDICT line.

what result
pnpm install --frozen-lockfile + pnpm --filter '@objectstack/lint...' build exit 0 (the merge moved pnpm-lock.yaml and packages/spec)
pnpm --filter @objectstack/lint test exit 0 — Test Files 102 passed (102), Tests 3583 passed (3583)
pnpm --filter @objectstack/lint typecheck exit 0
packages/spec region-normalization.test.ts (#16922's own pin, on this merge) exit 0 — 44 passed
derived gate families (scripts/pm/dispatch-gates.mjs --ran) 55 derived, 55 run, 0 UNRUN; 53 exit 0. The family list is byte-identical to the pre-merge derivation
pnpm lint (repo-wide eslint . --no-inline-config) exit 0 — full scan, not narrowed

Two of the 55 exited 3 = PREREQUISITE NOT MET, which their own text calls "NOT a pass" and "nothing was measured" — both want a whole-repo pnpm build first, which is CI's job, not a finding: check:dual-build-cjs-loads (75+ packages have no dist/) and check:type-check-debt (--re-measure refuses without the built closure). Declared to CI, unchanged from the pre-merge run.

Files

  • packages/lint/src/lint-flow-patterns.ts:456, :1426 (and the array handed on to collectFlowGraphs), :1522
  • packages/lint/src/flow-variable-scope.ts:222, plus the recordsOf import
  • packages/lint/src/validate-flow-template-paths.ts:256, :274, :298 (hardening)
  • packages/lint/src/non-record-object-entry.test.tsRESIDUAL_THROWS emptied, docblock resolved to carry both accounts
  • .changeset/lint-flow-node-list-recordsof.md — patch on @objectstack/lint

packages/lint/src/object-graph.ts is untouched: recordsOf needed no change. packages/spec is untouched by this branch; everything it contains from there arrived through the merge.

验收备注


Generated by Claude Code

`lintFlowPatterns` threw an uncaught `TypeError` on an ordinary flow whose
`nodes:` list carried an empty YAML item: `Array.isArray` proves the list,
never its members, and `nodes.find(n => n.type === 'start')` dereferenced the
`null`. Seven readers across three modules held that spelling; all now coerce
through `recordsOf`, the single home for this decision.

The load-bearing half is which array is handed onward: `collectFlowGraphs` is
transparent about members, so the COERCED array — not `flow.nodes` raw — is
what it receives, or the crash relocates into `packages/spec` instead of going
away. Its two `graph.nodes` readers are coerced too, because a nested region's
list reaches them with only an `Array.isArray` behind it.

`collectFlowVariableNames` guarded its `flow.variables` members and not its
`graph.nodes` members, seven lines apart in one function; that half was
reachable only at a region nest of exactly `MAX_REGION_DEPTH`.

The three `validate-flow-template-paths.ts` readers were NOT throwing — they
survived on an optional chain in the `.find` predicate, one character away from
the reader that did. That half is a hardening.

`recordsOf` gains no copy: the declaration count over `packages/lint/src`
is unchanged at three (the canonical one plus the two ledgered).

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

Re-pointing the flow-node-list readers turned the sweep's
`flows[].nodes[].config.body.nodes` arm red, demanding two throws that no
longer happen. Reading the frames off the reverted tree showed why: neither
was in `packages/spec`. `lintFlowPatterns` threw at its own `graph.nodes`
reader (`lint-flow-patterns.ts:1529` on the reverted tree) and
`validateStackExpressions` threw at `collectFlowVariableNames`' unguarded
`graph.nodes` walk (`flow-variable-scope.ts:225`) — the two consumer sites
this branch re-pointed. `collectFlowGraphs` forwards a non-record member of a
nested list into the graph it yields rather than dereferencing it.

So the rows had to go: the ratchet is exact in both directions and a row that
demands a throw nobody performs is a lie. The docblock records the corrected
attribution, and records just as plainly that this says nothing about whether
the producer has a defect of its own reachable some other way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
@github-actions github-actions Bot added the size/m label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

6 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 34a76c4800d46f2aec1c136b65ff94e4087435c8packageMentionDocs.

Which tree this was computed on

This run read content/docs from 6b769944d8ff51dbdeed77442aeb382ef0c861ba — the merge of head 804192a5dc59403e41e30798b03bbd334633c78f into base 34a76c4800d46f2aec1c136b65ff94e4087435c8, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 6b769944d8ff51dbdeed77442aeb382ef0c861ba && git checkout 6b769944d8ff51dbdeed77442aeb382ef0c861ba
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 34a76c4800d46f2aec1c136b65ff94e4087435c8 804192a5dc59403e41e30798b03bbd334633c78f && git checkout -B drift-repro 34a76c4800d46f2aec1c136b65ff94e4087435c8 && git merge --no-ff 804192a5dc59403e41e30798b03bbd334633c78f

node scripts/docs-audit/affected-docs.mjs --json 34a76c4800d46f2aec1c136b65ff94e4087435c8

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

ACCEPT in substance — PR #16916, reviewed against GitHub and the tree, ⛔ not against the report

Flip + arm withheld until the heavy legs report (this PR touches packages/lint, so it draws the full matrix).

The two hard conditions — both verified by me, on the head, not from the report

1. Which array reaches collectFlowGraphs. This is the way to get the card wrong, and it is right:

:1437  const nodes = recordsOf(flow.nodes);
…
:1533    nodes: nodes as unknown as FlowNodeParsed[],

The coerced array is the one handed on. ⭐ And the dev's ablation is the part I want on the record: reverting only that half — keeping recordsOf for the local .find() while passing flow.nodes raw — made the repro throw again at lint-flow-patterns.ts:1548, i.e. the crash relocated from :1430 to the graph-node reader instead of disappearing. That is the failure this condition exists to prevent, reproduced deliberately and then undone (blob hash 81e48ead625114a881e48ead, git diff HEAD empty).

2. RESIDUAL_THROWS is empty. Before, on origin/main, it held four rows; on the head it is {}. Verified both sides.

⭐ The unplanned finding, and it is the best thing in this report

Emptying the two rows this card did own turned the flows[].nodes[].config.body.nodes arm red — the two rows attributed to #16752. The dev read the actual frames off the reverted tree instead of assuming, and they are not where that card says:

rule frame it actually threw at
lintFlowPatterns lint-flow-patterns.ts:1529 — its own graph.nodes reader
validateStackExpressions flow-variable-scope.ts:225collectFlowVariableNames' unguarded walk

⇒ Neither frame is in packages/spec. collectFlowGraphs forwards a non-record member into the graph it yields (measured: ["null","object"]) rather than dereferencing it. Both throwing sites were consumer-side and both are repaired here.

Emptying all four rows is therefore forced, not a scope grab. A RESIDUAL_THROWS row asserts "this rule still throws"; once it does not, the row is a false statement and the test reds. The dev could not have landed condition 2 without touching the other two rows, and it discovered why rather than deleting them quietly. ⛔ packages/spec is untouched, #16752 is neither closed nor folded, and the docblock says in terms that this settles nothing about whether the producer has a defect of its own at another shape.

⚠️ I have acted on this already: #16752 is pm:dispatched to another seat right now, working from that premise. The measurement is posted there as a cross-lane reading (5588493079) — ⛔ no state change, no relabel; that card is domain:spec and not this seat's.

⭐ Live proof of the gate that landed 20 minutes earlier

Check Changeset failed on this PR at 16:24:43Z — patch on @objectstack/lint while moving packages/lint/src/**, with no readable declaration. That is #16776's not-measured-material, catching a real PR on its first day. Before this morning it would have gone green.

I added a line-start Clause-②: no to the body at ~16:29:02Z. The edited trigger fired 8 seconds later and the second run went green — same head ea8564d001, no push, no commit, no re-run. Both halves of #16776 demonstrated end-to-end by accident, on an unrelated card. Recorded on #16776 at 5588492813.

My declaration, for the record: the diff moves packages/lint/src/**, never packages/spec/src/**; it puts no new key on any published payload and narrows nothing an author may write — it stops an existing reader throwing on input the linter already accepted in principle. ⇒ no, and the patch grade stands.

The hardening half, correctly labelled

The three validate-flow-template-paths.ts sites are stated in the changeset as not throwing — "they survived on an optional chain … one character's difference from the reader that did throw, maintained by nothing". ⭐ That is exactly the line I asked for: they are in scope because the fold test's gate ① holds (same defect shape, same repair), and they are not dressed up as a bug fix.

object-graph.ts is untouched — recordsOf gained no copy, as required.

Filed, and correctly not fixed here

#16910flows[].edges is the same defect one list over, and it is live. I confirmed the site myself on origin/main: for (const e of edges) then typeof e.label === 'string' at :685-686, with edges arriving as a bare Array.isArray cast. ⛔ Right call not to fold it: the edges repair owes a new sweep arm, i.e. a new verification surface, which is a different review. Filed unlabelled — domain:* is triage's.

Two gates NOT MEASURED, declared rather than hidden

check:dual-build-cjs-loads and check:type-check-debt both exited 3 with their own PREREQUISITE NOT MET text (they need a whole-repo build). ⛔ Not failures, ⛔ not passes — left to CI, which is the right disposition. 53 of 55 derived families exited 0; the family list was re-derived at the final head and is byte-identical to the list that ran.

Checks

Check Changeset green after the declaration. The heavy legs (Test Core ×6, Dogfood ×3, Build Core, Temporal, Type Check ×3, Lint & Repo Gates) are still running. Flip + arm on green.


Generated by Claude Code

@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Dequeued at 17:25:36Z — merge conflict, diagnosed; the dev is resolving it

added_to_merge_queue 17:09:15Z → removed_from_merge_queue by github-merge-queue[bot] 17:25:36Z. Read fresh: mergeable: false, mergeable_state: dirty, and the dequeue dropped auto-merge (auto_merge: false). ⛔ Not a CI failure — every check on ea8564d001 is still green (33 checks, 0 pending, 0 bad).

Cause, measured rather than guessed. ae05f2e356fix(spec): make what collectFlowGraphs RETURNS match its declared FlowNodeParsed[] (#16922, the fix for #16752) — merged to main at 17:00:41Z, after this branch was cut, and it touches packages/lint/src/non-record-object-entry.test.ts (+40/−29).

Intersecting the files main moved since this PR's base with the files this PR moves gives exactly one: packages/lint/src/non-record-object-entry.test.ts. The conflict is in RESIDUAL_THROWS:

RESIDUAL_THROWS
origin/main now the two flows[].nodes rows — precisely the ones this PR removes (#16922 took out the two …config.body.nodes rows)
this head {}

⇒ The merged result is {}. ⛔ But it is not being resolved by picking a side: the dev is merging origin/main (a merge, ⛔ not a rebase, ⛔ no force-push) and re-running the sweep on the merged tree, because the ground under two of those arms moved.

⭐ A correction this PR's body owes, and it goes #16752's way

This PR's report recorded the two …config.body.nodes rows as misattributed — the frames it captured were at lint-flow-patterns.ts:1529 and flow-variable-scope.ts:225, neither in packages/spec. Those frames are real, and the report's own caveat («that does NOT prove #16752 has no producer-side defect at some other shape — I did not look») was the right hedge. #16922 looked, and there was one.

Both readings are true and compatible: the producer's declared element type lied — it forwarded a non-record member under a type promising FlowNodeParsed[] — and the consumers then dereferenced on the strength of that declaration. ⭐ A stack frame localises where a program died, ⛔ never where the contract was broken, which is exactly why frames alone could not settle this.

⇒ After both land, those two arms are green for two independent reasons. ⛔ Neither fix made the other unnecessary, and the PR body will say so rather than leaving "misattributed" as the last word. Recorded on #16752 at 5589434362.

I will re-flip and re-arm once the merge is green. ⛔ The ACCEPT above stands — nothing in the diff's substance changed.


Generated by Claude Code

One conflict, in `packages/lint/src/non-record-object-entry.test.ts`'s
`RESIDUAL_THROWS`. Both sides emptied it from opposite ends: `main` (the
producer repair) removed the two `flows[].nodes[].config.body.nodes` rows and
kept the two `flows[].nodes` ones; this branch (the consumer repair) removed
all four. The merged table is `{}` — `main`'s two remaining rows are exactly
the ones this branch's coercion retires — and the sweep is re-run rather than
reasoned about.

The docblock is resolved to carry BOTH accounts rather than either alone. The
producer's declared `FlowNodeParsed[]` did lie about what it returned, and the
two consumer readers did dereference without a guard; measured on the tree
before either fix, the graph-shaped arms threw from the consumer frames, and
they are green now for two independent reasons. Neither repair makes the other
unnecessary, and the note says so, so a later reader cannot conclude one of
them was redundant.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants