test: address the automated review on the custom-node suite

Twenty-four of twenty-five findings applied. Three were worth more than the
line they pointed at.

The COMBO output-name one was proposed as an index-based rename. Production
names an output `output_name[index] || output_<index>` with no string
coercion, so a non-string entry leaves the live slot's name as the raw array,
and connectivity matches on slot.name === producer.slotName - renaming would
have traded a duplicate-drop for a guaranteed SLOT_CONTRACT_MISMATCH. Those
outputs go to unknownSlots instead. Tracing it turned up a second bug: a
FALSY non-string entry was named after the slot type where production gives
output_0. RawNodeDef.output_name was the actual lie at string[] - ComfyUI
sets output_name = RETURN_NAMES if present else RETURN_TYPES, so combo
outputs really do ship option arrays there. It is unknown[] now.

The facet-order test was said to over-pin ordering. diffShapes ends in
.sort(), so emission order is erased, and probesEqual compares element by
element - relaxing to arrayContaining would have dropped a live contract. The
real flaw was a fixture too weak to tell the two hypotheses apart; it now is.

The updatedAt assertions were called tautological. They compare the entry's
timestamp against the index's top-level one, which are different fields and
would fail on divergence. Rewritten for clarity anyway, which did close a
real gap: expect.any(Number) on the captured value is now load-bearing.

Also removed a comment claiming a detection-proof row greps a label that no
row greps, and a "geometry ledger" reference left over from the cut S14 tier.

The remaining findings: a 40-minute job timeout sized from the 12-18.8 minute
observed range, the ComfyUI pin declared once so the published summary cannot
advertise a ref the run never used, manifest repo URLs validated before
git clone, fail-fast on the video fixture and the input directory, dead row-14
case arms removed, --strictPort on the one script that assumes 5173, real
waiting instead of isVisible's ignored timeout, HUNG_BACKEND aborting the
outer batch loop so later batches stop recording false timeouts over a wedged
queue, a computed drag-sweep bound instead of setTimeout(0), per-node probe
isolation so a throwing pack names itself, profile and manifest shape
validation, a real off() in the suite fake, graphNodeIds coverage on both
branches, and a rejecting-page harness replacing four hand-rolled copies.

Declined: re-throwing on startup-settings failure. That swallow predates this
PR by two years, every setting it guards is either irrelevant at --workers=1
or set explicitly per spec, the fixture already asserts the resulting
precondition, and re-throwing would turn any devtools-less or auth-gated
backend from one console.error into a full-suite failure - which has happened
(run 30309274120, 359 tests). The real defect is that the swallow cannot tell
an absent endpoint from a broken write, and that belongs in its own PR against
the shared fixture.

Author: benjcooley

.github/workflows/ci-tests-custom-nodes.yaml

@@ -49,6 +49,9 @@ concurrency:
   group: ${{ github.event_name == 'workflow_dispatch' && format('{0}-dispatch-{1}', github.workflow, github.run_id) || format('{0}-{1}', github.workflow, github.ref) }}
   cancel-in-progress: true
 
+env:
+  COMFYUI_PIN: b08e6cf35fac50d3ca8470dffb3f9a1fbb7187d2
+
 jobs:
   # Path gating lives here, not in a trigger-level `paths:` filter: a required
   # check gated by trigger paths never creates a check run on an unrelated PR
@@ -86,6 +89,8 @@ jobs:
       (github.event_name != 'pull_request' ||
       github.event.pull_request.head.repo.full_name == github.repository))
     runs-on: ubuntu-latest
+    # Green runs of this job land at 13-19 minutes.
+    timeout-minutes: 40
     permissions:
       contents: read
     steps:
@@ -145,7 +150,7 @@ jobs:
 
       # Checks out ComfyUI, installs Python/torch/requirements and ComfyUI_devtools.
       # launch_server:false so we can add the manifest packs before booting.
-      # comfyui_ref pins core so the gate is reproducible: this SHA is the
+      # comfyui_ref pins core so the gate is reproducible: COMFYUI_PIN is the
       # exact master commit the suite (incl. the KJNodes ledger entries and
       # per-pack node counts) was last verified green against. Bump it
       # deliberately, in its own PR, recalibrating ledgers/counts if the run
@@ -157,7 +162,7 @@ jobs:
           # workflow_dispatch may override the pin (e.g. probing a core
           # candidate before a deliberate bump); every gating event stays
           # pinned.
-          comfyui_ref: ${{ inputs.comfyui_ref || 'b08e6cf35fac50d3ca8470dffb3f9a1fbb7187d2' }}
+          comfyui_ref: ${{ inputs.comfyui_ref || env.COMFYUI_PIN }}
 
       # Pack sources are SHA-pinned, so their content is fully determined by
       # the manifest: cache the checked-out trees (sans .git) and restore by
@@ -206,6 +211,11 @@ jobs:
             if ! [[ "$pin" =~ ^[0-9a-f]{40}$ ]]; then
               echo "::error::$pack: pin must be a full commit SHA (got '$pin')"; exit 1
             fi
+            # git clone reads a leading `-` as an option, and options such as
+            # --upload-pack= run a command on the runner.
+            if ! [[ "$repo" =~ ^https://github\.com/[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ ]]; then
+              echo "::error::$pack: repo must be an https://github.com/<owner>/<name> URL (got '$repo')"; exit 1
+            fi
             dir="ComfyUI/custom_nodes/$pack"
             cache_dir="$HOME/.cache/cn-packs/$pack"
             # The pin marker is a SIBLING of the tree, never inside it - a
@@ -310,7 +320,11 @@ jobs:
       # The VHS run-tier workflow reads input/plain_video.mp4.
       - name: Stage run-tier assets
         shell: bash
-        run: cp browser_tests/assets/plain_video.mp4 ComfyUI/input/plain_video.mp4
+        run: |
+          set -euo pipefail
+          mkdir -p ComfyUI/input
+          cp browser_tests/assets/plain_video.mp4 ComfyUI/input/plain_video.mp4 \
+            || { echo "::error::could not stage plain_video.mp4 - the VHS run tier has no input to execute on"; exit 1; }
 
       # --cache-none so retried run-tier tests re-execute every node (a cached
       # node emits no `executing` event and would false-fail PARTIAL).
@@ -343,7 +357,7 @@ jobs:
           grep_args=()
           case "$PROOF_ROW" in
             0) [ -n "${GREP_FILTER:-}" ] && grep_args=(--grep "$GREP_FILTER") ;;
-            1|2|3|9|14)
+            1|2|3|9)
               grep_args=(--grep 'all nodes by tier @custom-nodes.*S(1|2|3|9):')
               ;;
           esac
@@ -446,7 +460,6 @@ jobs:
             2) pat=': Vue mounts ' ;;
             3) pat='on (pristine|set-values) reload' ;;
             9) pat='DETECTION PROOF \(row 9\): pack node runtime failure' ;;
-            14) pat='widgets\[[0-9]+\]\.y: expected [^,]+, got ' ;;
           esac
           grep -Eq -- "$pat" tier-isolation-proof-evidence.txt || { echo "::error::S$PROOF_ROW failure was not attributable to its Detection Proof break"; exit 1; }
 
@@ -493,7 +506,7 @@ jobs:
         shell: bash
         env:
           BRANCH_TESTED: ${{ inputs.branch || github.ref_name }}
-          COMFYUI_REF_USED: ${{ inputs.comfyui_ref || 'b08e6cf35fac50d3ca8470dffb3f9a1fbb7187d2' }}
+          COMFYUI_REF_USED: ${{ inputs.comfyui_ref || env.COMFYUI_PIN }}
           GREP_FILTER: ${{ inputs.grep }}
         run: python3 scripts/cicd/custom-nodes-summary.py
 

browser_tests/fixtures/customNode/autoRun.ts

@@ -52,10 +52,8 @@ export const SYNTH_PRODUCERS: Record<
 
 const WIDGET_TYPES = new Set(['INT', 'FLOAT', 'STRING', 'BOOLEAN'])
 
-type InputSpec = [unknown, Record<string, unknown>?] | unknown
-
-function classifyInput(spec: InputSpec): 'widget' | 'socket' | 'empty-combo' {
-  const specArray = Array.isArray(spec) ? spec : [spec]
+function classifyInput(spec: unknown): 'widget' | 'socket' | 'empty-combo' {
+  const specArray: unknown[] = Array.isArray(spec) ? spec : [spec]
   const rawType = specArray[0]
   const options = specArray[1] as
     | { forceInput?: boolean; options?: unknown; widgetType?: string }
@@ -80,8 +78,8 @@ function classifyInput(spec: InputSpec): 'widget' | 'socket' | 'empty-combo' {
     : 'socket'
 }
 
-function socketType(spec: InputSpec): string {
-  const specArray = Array.isArray(spec) ? spec : [spec]
+function socketType(spec: unknown): string {
+  const specArray: unknown[] = Array.isArray(spec) ? spec : [spec]
   return String(specArray[0])
 }
 

browser_tests/fixtures/customNode/interactionProfiles.ts

@@ -49,8 +49,8 @@ export interface PackInteractionProfileFile {
 }
 
 // Nodes whose interaction deltas are not reproducible run-to-run, keyed by
-// the MECHANISM (the geometry/console ledger discipline): registration-
-// guarded in the spec, announced in run output, omitted from baselines.
+// the MECHANISM (the console ledger discipline): registration-guarded in
+// the spec, announced in run output, omitted from baselines.
 // Empty until a record/compare cycle observes real instability - entries
 // are earned by evidence, never pre-emptively.
 export const INTERACTION_UNSTABLE_NODES: Record<
@@ -78,18 +78,74 @@ function profilePath(pack: string): string {
   return `${PROFILE_DIR}${pack}.json`
 }
 
+const PROBES = ['connectFirst', 'connectLast', 'disconnect'] as const
+
+function isNonEmptyString(value: unknown): value is string {
+  return typeof value === 'string' && value.length > 0
+}
+
+function isPlainObject(value: unknown): value is Record<string, unknown> {
+  return typeof value === 'object' && value !== null && !Array.isArray(value)
+}
+
+function isProbeResult(value: unknown): value is ProbeResult {
+  if (value === 'NO_PRODUCER' || value === 'NO_INPUTS') return true
+  return (
+    Array.isArray(value) && value.every((entry) => typeof entry === 'string')
+  )
+}
+
+// The offending field path, or null when the file is a valid profile set.
+// A malformed baseline must name itself here: left unchecked it surfaces as
+// an unrelated TypeError deep inside comparePackProfiles.
+function invalidProfileField(parsed: unknown): string | null {
+  if (!isPlainObject(parsed)) return 'root (expected a JSON object)'
+  if (parsed.schema !== 1) return 'schema (expected 1)'
+  if (!isPlainObject(parsed.recordedAt))
+    return 'recordedAt (expected { core, pin })'
+  if (!isNonEmptyString(parsed.recordedAt.core))
+    return 'recordedAt.core (expected a non-empty string)'
+  if (!isNonEmptyString(parsed.recordedAt.pin))
+    return 'recordedAt.pin (expected a non-empty string)'
+  if (!isPlainObject(parsed.nodes))
+    return 'nodes (expected an object keyed by node type)'
+  for (const [node, profile] of Object.entries(parsed.nodes)) {
+    if (!isPlainObject(profile))
+      return `nodes.${node} (expected a profile object)`
+    for (const probe of PROBES) {
+      const value = profile[probe]
+      if (isProbeResult(value)) continue
+      if (probe === 'connectLast' && value === 'SAME_AS_FIRST') continue
+      const markers =
+        probe === 'connectLast'
+          ? "'NO_PRODUCER', 'NO_INPUTS', or 'SAME_AS_FIRST'"
+          : "'NO_PRODUCER' or 'NO_INPUTS'"
+      return `nodes.${node}.${probe} (expected a string[] delta, ${markers})`
+    }
+  }
+  return null
+}
+
+function assertProfileFile(
+  parsed: unknown,
+  path: string
+): asserts parsed is PackInteractionProfileFile {
+  const field = invalidProfileField(parsed)
+  if (field !== null)
+    throw new Error(
+      `${path} is not a valid interaction profile file - ${field} - re-record it (docs/custom-node-regression-suite.md Step 5d)`
+    )
+}
+
 // null = no baseline recorded yet; compare mode must red on that (an
 // uncovered pack is the failure mode this suite bans), record mode expects it.
 export function loadPackProfiles(
   pack: string
 ): PackInteractionProfileFile | null {
   const path = profilePath(pack)
   if (!existsSync(path)) return null
-  const parsed = JSON.parse(readFileSync(path, 'utf-8'))
-  if (parsed.schema !== 1 || !parsed.recordedAt?.core)
-    throw new Error(
-      `${path} is not schema 1 with recordedAt provenance - re-record it (docs/custom-node-regression-suite.md Step 5d)`
-    )
+  const parsed: unknown = JSON.parse(readFileSync(path, 'utf-8'))
+  assertProfileFile(parsed, path)
   return parsed
 }
 
@@ -108,7 +164,7 @@ function probesEqual(
   b: NodeInteractionProfile
 ): string[] {
   const problems: string[] = []
-  for (const probe of ['connectFirst', 'connectLast', 'disconnect'] as const) {
+  for (const probe of PROBES) {
     const expected = a[probe]
     const actual = b[probe]
     const same =

browser_tests/fixtures/customNode/manifest.ts

@@ -255,9 +255,13 @@ export function rendererPassesFor(
 }
 
 export function loadManifest(): CoreManifestEntry[] {
-  const entries = JSON.parse(
-    readFileSync(dataPath('customNodeManifest.core.json'), 'utf-8')
-  ) as CoreManifestEntry[]
+  const path = dataPath('customNodeManifest.core.json')
+  const parsed: unknown = JSON.parse(readFileSync(path, 'utf-8'))
+  if (!Array.isArray(parsed))
+    throw new Error(
+      `custom-node manifest ${path} must be a JSON array of entries, got ${parsed === null ? 'null' : typeof parsed}`
+    )
+  const entries: CoreManifestEntry[] = parsed
   entries.forEach(assertCoreEntry)
   return entries
 }

browser_tests/fixtures/customNode/typePairing.ts

@@ -9,7 +9,9 @@ export interface RawNodeDef {
     optional?: Record<string, unknown>
   }
   output?: unknown[]
-  output_name?: string[]
+  // object_info repeats RETURN_TYPES here when a node declares no
+  // RETURN_NAMES, so a combo output's entry is its option array.
+  output_name?: unknown[]
   python_module?: string
 }
 
@@ -25,8 +27,9 @@ export interface NormalizedNode {
   pack: string
   inputs: NormalizedSlot[]
   outputs: NormalizedSlot[]
-  // Slots whose raw spec carried no recognizable type (slotTypeOf null):
-  // recorded so a schema change can never silently shrink the corpus.
+  // Slots the corpus cannot address: no recognizable type (slotTypeOf null)
+  // or no string name on the instance (outputSlotName null). Recorded so a
+  // schema change can never silently shrink the corpus.
   unknownSlots?: string[]
 }
 
@@ -55,7 +58,7 @@ export interface PairingPlan {
   // Combos whose option lists match exactly ARE paired like any other type.
   combos: Array<SlotRef & { dir: 'in' | 'out' }>
   // Slots dropped at normalize time because their raw spec had no
-  // recognizable type - surfaced here (and logged by the sweep) so a
+  // recognizable type or name - surfaced here (and logged by the sweep) so a
   // backend or pack schema change cannot silently shrink the corpus.
   unknownShapes: string[]
 }
@@ -88,6 +91,14 @@ function slotTypeOf(rawType: unknown): string | null {
   return typeof rawType === 'string' ? rawType : null
 }
 
+// Faithful mirror of production naming (schemas/nodeDef/migration.ts):
+// `output_name[index] || output_<index>`, uncoerced - so a truthy non-string
+// entry names the live slot something no string lookup can match (null here).
+function outputSlotName(rawName: unknown, index: number): string | null {
+  if (!rawName) return `output_${index}`
+  return typeof rawName === 'string' ? rawName : null
+}
+
 function inputSlots(
   entries: Record<string, unknown> | undefined,
   unknown: string[]
@@ -140,18 +151,12 @@ export function normalizeNodeDefs(
           unknown.push(`output[${index}]`)
           return []
         }
-        // output_name entries can be non-strings (COMBO literals repeat the
-        // option array); the slot name must stay a string.
-        const rawName = def.output_name?.[index]
-        const slot: NormalizedSlot = {
-          name:
-            typeof rawName === 'string'
-              ? rawName || `output_${index}`
-              : rawName === undefined
-                ? `output_${index}`
-                : slotType,
-          type: slotType
+        const name = outputSlotName(def.output_name?.[index], index)
+        if (name === null) {
+          unknown.push(`output[${index}].name`)
+          return []
         }
+        const slot: NormalizedSlot = { name, type: slotType }
         if (slotType === 'COMBO') slot.comboOptions = rawType as unknown[]
         return [slot]
       })

browser_tests/fixtures/utils/consoleErrorCollector.ts

@@ -1,7 +1,6 @@
 import type { ConsoleMessage, Page, TestInfo } from '@playwright/test'
 
 export async function attachPageDiagnosticEvidence(
-  _page: Page,
   testInfo: Pick<TestInfo, 'attach'>,
   name: string,
   values: readonly string[]

browser_tests/tests/customNodes/allNodes.spec.ts

@@ -632,7 +632,8 @@ for (const entry of loadManifest()) {
               if (key in ledger) continue
               const visible = await comfyPage.page
                 .locator(`[data-node-id="${shape.id}"]`)
-                .isVisible({ timeout: 2_000 })
+                .waitFor({ state: 'visible', timeout: 2_000 })
+                .then(() => true)
                 .catch(() => false)
               if (!visible) failures.push(`${key}: no Vue mount`)
             }
@@ -1385,7 +1386,7 @@ for (const entry of loadManifest()) {
               hardFailures.push(
                 `${verdict.key}: ${single} - add to AUTO_RUN_EXCLUDE with its mechanism`
               )
-              break
+              break batchLoop
             } else cannotRun.set(verdict.key, single)
           }
         }
@@ -1453,7 +1454,6 @@ for (const entry of loadManifest()) {
         if (hardFailures.length > 0) {
           console.log(autoRunFailureSummary)
           await attachPageDiagnosticEvidence(
-            comfyPage.page,
             test.info(),
             'auto-run-failures.json',
             hardFailures
@@ -1525,12 +1525,7 @@ test.describe('all nodes by tier @custom-nodes', () => {
       }
       const attachment = `${tier.toLowerCase()}-failures.json`
       if (failures.length > 0)
-        await attachPageDiagnosticEvidence(
-          comfyPage.page,
-          test.info(),
-          attachment,
-          failures
-        )
+        await attachPageDiagnosticEvidence(test.info(), attachment, failures)
       expect(
         failures.length === 0,
         failureSummary(`${tier} pack failures`, failures, attachment)