test(gardening): cover the node factories and pipeline file events (36 cases, 4 new files)

[camielvs]

🤖 Automated draft PR — opened by the gardening skill. Nothing here auto-merges.
4 new test files, 36 new cases. The pillar's rule is that a test which passes while asserting the wrong
thing is worse than no test, so every assertion here was checked against the implementation, and the
riskiest one was mutation-tested (below).

Update — the readOnly question below has been answered, and this PR now carries the fix.
The pillar flagged the readOnly precedence it had pinned and asked for a ruling. The ruling is bug,
so a source commit was added here rather than left to a follow-up. See
The readOnly precedence ruling — this PR is no longer
source-change-free.

pnpm run test:coverage does not run on master

The pillar's primary signal is coverage × churn. It was unavailable:

SyntaxError: The requested module 'vitest/node' does not provide an export named 'BaseCoverageProvider'
  ❯ loadProvider node_modules/.../@vitest/coverage-v8/dist/load-provider-CdgAx3rL.js:4:33

@vitest/coverage-v8@4.1.10 is installed against vitest@3.2.6 — a major-version mismatch, so no
coverage report can be produced at all. Two separate defects, neither introduced here:

1. The peer versions disagree. pnpm run test:coverage fails on a clean master checkout.
2. The script omits run. "test:coverage": "pnpm vitest --coverage" starts watch mode; in CI
   it would hang rather than exit. Compare "test": "vitest run".

Update: both are fixed in #2632, which upgrades vitest to ^4.1.10 to match the installed
@vitest/coverage-v8, adds run to the script, and sets coverage.include (v4 otherwise reports only
files loaded during the run). Coverage should be available as a ranking signal for the next pass.

This run substituted churn × no-co-located-test-file over
src/{utils,hooks,lib,services} as the ranking signal, which found 13 candidate files. That substitution
is a weaker signal than real coverage — it cannot see partially-covered files — and is disclosed here
rather than presented as equivalent.

What was added

File under test	Churn	New file	Cases
src/utils/nodes/createTaskNode.ts	9	createTaskNode.test.ts	12
src/utils/nodes/createInputNode.ts	5	createInputNode.test.ts	8
src/utils/nodes/createOutputNode.ts	5	createOutputNode.test.ts	7
src/services/pipelineStorage/pipelineFileEvents.ts	1	pipelineFileEvents.test.ts	9

The three node factories were not untested in the coverage sense — createNodesFromComponentSpec.test.ts
drives them indirectly. But that test asserts only id / position / type / taskId / taskSpec, and
never touches zIndex or readOnly. The new files cover what it does not:

• zIndex extraction — default (0 for all three types, from Z_INDEX_RANGES), numeric annotation,
  numeric string annotation, fractional value (rounded), and non-numeric string (falls back to default).
• Position fallbacks — absent annotation and malformed JSON both yield { x: 0, y: 0 }.
• Spread precedence, which is the part most likely to break silently:
  • createInputNode/createOutputNode build data as {...rest, ...nodeData, label, readOnly}, so
    nodeData wins over a colliding input-spec field. (The readOnly assertions originally in this
    group have been rewritten — see the ruling section below.)
  • createTaskNode likewise always forces highlighted: false.
• name and annotations are deliberately not copied into node data (they are destructured out).
• pipelineFileEvents — delivery, unsubscribe, multi-listener fan-out, and every branch of
  getLastForeignWriteTime: unknown key, own-source-only, a foreign write, latest-per-source, an own
  write not masking an earlier foreign one, and per-key isolation.

The assertions were checked for teeth, not just green

createInputNode's spread order was mutated in place ({...rest, ...nodeData} → {...nodeData, ...rest})
and the suite re-run:

× createInputNode > lets nodeData win over a colliding input spec field
  Tests  1 failed | 7 passed (8)

Exactly one test failed, and it was the right one. The mutation was reverted.

The readOnly precedence ruling (B5)

The checklist item below asked for a decision. It is a bug, and the fix is now the last commit here.

All three factories declared readOnly: boolean = false and spread it after ...nodeData, so
createTaskNode(task, { readOnly: true }) yielded data.readOnly === false. The spread order is
defensible — an explicit argument should beat a shared nodeData bag — but the default is not: it made
an omitted argument indistinguishable from an explicit false, so a phantom default outranked a real value.

readOnly?: boolean            // was: readOnly: boolean = false
// …
readOnly: readOnly ?? nodeData.readOnly ?? false

An explicit argument still wins, an omitted one now inherits from nodeData, and ?? false keeps
data.readOnly a boolean so nothing downstream shifts.

Why it matters: duplicateNodes
is the one caller that omits the argument — it passes originalNodeData for all three node types — so
duplicates were flagged editable regardless of the original. Not reachable today: every route into
duplicateNodes is gated on readOnly upstream (selection toolbar at FlowCanvas.tsx:1226, paste at
:1088, node action at TaskDetails/Actions.tsx:64). But data.readOnly does drive read-only UI through
TaskNodeProvider.tsx:189 and IONode.tsx:92, so the inheritance should be correct rather than
incidentally unused. createFlexNode keeps its = false default — it takes no nodeData, so it has no
collision to resolve.

Test changes: the three pinned assertions now read argument → nodeData → false instead of
documenting the clobber, and a new duplicateNodes case covers inheritance across all three node types.
Reverting the three factories alone fails exactly those four tests and nothing else.

Test files added	4
Cases added	37 (36 + 1 for the ruling)
Existing test files edited	1 (duplicateNodes.test.ts)
Source files changed	3 (the node factories — see the ruling above)
Validation	typecheck · lint · knip · prettier ✅ — 195 files / 2,004 tests (was 191 / 1,966)
Confidence threshold	0.85 (tests, config)

Reviewer checklist

• Each new/updated assertion reflects intended behavior, not merely that the test passes
  (mandatory: requiresBehaviorReview: true)
• ⚠️ Self-review was skipped (review skill unavailable) — review this diff manually.
  The engine requires every PR to survive the review skill first, but that skill is
  disable-model-invocation and only a human can run it.
• Decide whether the readOnly precedence I pinned down is the behavior you want.
  Resolved: ruled a bug. The source is fixed and the three assertions rewritten in this PR — see
  the ruling section. Review that commit on its own merits;
  it is the only source change here.
• No e2e specs were added — see below.

Deliberately not done

• The 2 it.todos in hydrateComponentReference.test.ts (:892, :979) are left alone. Their own
  text says the behavior is undecided — "todo: decide if this case is valid" and "todo: decide how we
  should handle this". Implementing them means inventing intended behavior for
  text-vs-spec conflicts and for invalid-YAML-with-valid-spec. Queued for a human decision instead.
• tests/e2e/aggregator.spec.ts:49 test.skip ("should add dynamic inputs when connection is made
  to add-input handle") is left skipped. Un-skipping it without being able to run Playwright here would
  be shipping an unverified spec.
• No new e2e specs. pnpm run test:e2e:ci needs browsers and a dev server that this environment does
  not provide, so per the pillar I am not claiming a pass — and I would rather add none than add specs I
  could not execute.
• generateDynamicNodeCallbacks (churn 3) has no new test, and the reason is a type bug worth
  fixing first: it is declared to return NodeCallbacks, whose members take (ids: NodeAndTaskId, ...args),
  but the values it actually returns are wrappers that take only the trailing args (they supply ids
  themselves). Calling the result the way its own type describes would pass the ids object twice, so an
  honest test needs an as unknown as double cast — which typescript-standards#avoid-unsafe-type-casting
  rules out. Queued as a source fix; the call-through path is already covered indirectly by
  createNodesFromComponentSpec.test.ts:176.
  Update: fixed in fix: correct generateDynamicNodeCallbacks return type #2633, which corrects the return type to TaskNodeCallbacks and adds the 6 tests
  this pillar could not write without the cast. No change needed here.
• src/services/googleDrive/* and pipelineStorage/{db,createDriver,pipelineRegistry}.ts were
  ranked but skipped this run: they need IndexedDB / Google API mock scaffolding, which is a larger
  design decision than a gardening pass should make unilaterally.

Method notes

• One commit per new test file (4), no repair commits, so any single file can be dropped.
• This pillar ran last and saw the tree as master, not as the earlier pillars' branches left it; none
  of the 4 files it touched overlaps the 46 files claimed by PRs garden(comments): weekly groundskeeping — 2026-W33 #2620–chore(gardening): adopt UI primitives + tone prop where the class delta is provably zero (18 swaps) #2624.

[github-actions]

🎩 Preview

A preview build has been created at: automated-gardening/tests/2026-W33/d1868fa

[morgan-wowk]

🤖 Agent review. The new tests are strong — real factory/event assertions, no snapshots, no flaky (time/random/order) patterns. One thing to flag: despite the test(...) title, this isn't test-only (inline).

[morgan-wowk]

🤖 This is a runtime behaviour change, not just tests: duplicated nodes now inherit readOnly from the original (readOnly ?? nodeData.readOnly ?? false) instead of always being false. The only caller that omits the arg is duplicateNodes (createNodesFromComponentSpec always passes it explicitly, so its branch is unchanged). This is safe only if every duplicateNodes entry point is genuinely gated on readOnly upstream — please confirm that invariant, since data.readOnly drives the read-only UI. Also worth retitling off test(...) since it carries a source behaviour change. (Same pattern in createInputNode.ts / createOutputNode.ts.)