test: custom-node E2E regression suite - #13389
Conversation
test:custom-nodes runs the whole suite headless (the gate); :watch opens a headed slow-motion run of the browser tiers; :debug steps through them in the Playwright Inspector. All target the local dev server on :5173 and use the committed system-Chrome config (no bundled-chromium download). Pass -g to :watch / :debug to run a single test, e.g. -g 'VideoHelperSuite.*T1'.
…uite One script per pack tier (impact-render/impact-run/vhs-render/vhs-run) plus the self-check, all opening the Playwright Inspector so anyone can step through what the robot does. README covers prerequisites, every script, a worked example, the zero-visible-errors contract, and how to add a pack.
🌐 Website E2ETip All tests passed.
🔗 Website PreviewWebsite Preview: https://comfy-website-preview-pr-13389.vercel.app This commit: https://website-frontend-nbgm3qito-comfyui.vercel.app Last updated: 2026-08-13T00:23:56Z for |
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: 🕵🏻 0 passed, 0 failed📊 Browser Reports
📦 Bundle: 8.72 MB gzip 🔴 +14 BDetailsSummary
Category Glance App Entry Points — 3.67 kB (baseline 3.67 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.36 MB (baseline 1.36 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 2 added / 2 removed / 1 unchanged Views & Navigation — 124 kB (baseline 124 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 13 added / 13 removed / 4 unchanged Panels & Settings — 565 kB (baseline 565 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 16 unchanged User & Accounts — 27 kB (baseline 27 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed / 4 unchanged Editors & Dialogs — 125 kB (baseline 125 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 7 added / 7 removed / 1 unchanged UI Components — 67.1 kB (baseline 67.1 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed / 8 unchanged Data & Services — 3.51 MB (baseline 3.51 MB) • 🔴 +35 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 550 kB (baseline 550 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 18 added / 18 removed / 20 unchanged Vendor & Third-Party — 16.3 MB (baseline 16.3 MB) • 🔴 +552 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed / 16 unchanged Other — 14.2 MB (baseline 14.2 MB) • 🟢 -33 BBundles that do not match a named category
Status: 68 added / 68 removed / 217 unchanged ⚡ Performance
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a manifest-driven custom-node browser regression suite, with new fixtures, validation helpers, browser specs, workflow assets, scripts, CI, and docs. It also updates ChangesCustom-node regression suite
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CodeJuggernaut
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
browser_tests/README.md (1)
22-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAvoid hardcoding the suite total.
16 passedwill drift as soon as the manifest grows, which is the documented extension path for this suite. Describe success in terms of all manifest packs passing instead.♻️ Suggested wording
-| `pnpm test:custom-nodes` | whole suite headless - the pass/fail gate (expect `16 passed`, zero skips) | +| `pnpm test:custom-nodes` | whole suite headless - the pass/fail gate (all manifest packs pass, zero unexpected skips) |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser_tests/README.md` at line 22, The browser_tests README currently hardcodes the suite result as 16 passed, which will become stale as the manifest grows. Update the success wording in the README to describe the outcome generically in terms of all manifest packs passing, and adjust the related browser_tests documentation text so it no longer depends on a fixed total.
♻️ Duplicate comments (1)
browser_tests/tests/customNodes/spikeDesktop.spec.ts (1)
28-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate of
customNode.regression.spec.tsbeforeEach.Same concern as flagged in
customNode.regression.spec.ts(lines 42-47): this block is copy-pasted verbatim. Consolidating into a shared helper avoids the two specs diverging silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser_tests/tests/customNodes/spikeDesktop.spec.ts` around lines 28 - 33, The test setup in test.beforeEach is duplicated verbatim from customNode.regression.spec.ts, so consolidate the repeated template-workflows-content hide logic into a shared helper and call it from both specs. Move the shared steps (getting the getByTestId('template-workflows-content'), waiting for visible, pressing Escape, and waiting for hidden) into one reusable function or fixture, then update spikeDesktop.spec.ts and the regression spec to use that helper so they stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/fixtures/customNode/manifest.ts`:
- Line 4: The manifest lookup in the customNode manifest module still relies on
the process working directory, so update the path handling around MANIFEST_PATH
and the readFileSync(resolve(...)) usage to resolve relative to import.meta.url
instead. Adjust the manifest loading logic in manifest.ts so it builds an
absolute path from this module’s location, keeping the existing MANIFEST_PATH
constant but no longer depending on a repo-root relative string.
In `@browser_tests/fixtures/utils/errorSurfaces.ts`:
- Around line 9-10: The test selectors in errorSurfaces use raw string literals
for test ids, which should be centralized to avoid drift. Update the
errorOverlay and errorDialog locators to reference the shared TestIds constants
used elsewhere in ComfyPage.ts instead of hardcoded strings, keeping the
selector definitions aligned with the centralized test-id source.
In `@browser_tests/tests/customNodes/customNode.regression.spec.ts`:
- Around line 29-38: The test.use initialSettings block is duplicated across
customNode.regression.spec.ts and spikeDesktop.spec.ts, so centralize the shared
settings into a common constant such as customNodeSuiteSettings and reuse it in
both specs. Keep the existing values for Comfy.TutorialCompleted, Comfy.userId,
and Comfy.RightSidePanel.ShowErrorsTab, and update the relevant test.use calls
to reference the shared symbol so both suites stay in sync.
- Around line 42-47: The `beforeEach` in `customNode.regression.spec.ts`
duplicates the template-dismissal steps already used in `spikeDesktop.spec.ts`.
Extract this logic into a shared helper such as
`dismissTemplatesDialog(comfyPage)` in the same utilities area as
`errorSurfaces` and `collectConsoleErrors`, then replace both inline
`beforeEach` blocks with the helper call to keep the behavior centralized and
prevent drift.
In `@browser_tests/tests/customNodes/README.md`:
- Around line 63-68: The add-pack checklist in the customNodes README is missing
manifest fields that loadManifest() actually validates, so the documented
example would fail before tests run. Update the instructions around the
customNodeManifest entry and the run/load steps to include the required schema
fields repo, pin, workflow, requiresGpu, and requiresModels, and keep the
guidance aligned with the existing fixtures and assets referenced by
browser_tests/fixtures/data/customNodeManifest.json and
browser_tests/assets/customNodes/.
In `@browser_tests/tests/customNodes/spikeDesktop.spec.ts`:
- Around line 1-63: The test file name and its purpose are inconsistent: the
current spike-oriented name does not match the maintained smoke/regression suite
described by the `test.describe('smoke: core workflow')` block and the
`smokeWorkflow` setup. Rename the spec file to a stable, descriptive name such
as `coreSmoke.spec.ts` (or equivalent) so the filename matches the long-lived
test intent and is easy to discover alongside the `collectConsoleErrors` and
`errorSurfaces` checks.
---
Outside diff comments:
In `@browser_tests/README.md`:
- Line 22: The browser_tests README currently hardcodes the suite result as 16
passed, which will become stale as the manifest grows. Update the success
wording in the README to describe the outcome generically in terms of all
manifest packs passing, and adjust the related browser_tests documentation text
so it no longer depends on a fixed total.
---
Duplicate comments:
In `@browser_tests/tests/customNodes/spikeDesktop.spec.ts`:
- Around line 28-33: The test setup in test.beforeEach is duplicated verbatim
from customNode.regression.spec.ts, so consolidate the repeated
template-workflows-content hide logic into a shared helper and call it from both
specs. Move the shared steps (getting the
getByTestId('template-workflows-content'), waiting for visible, pressing Escape,
and waiting for hidden) into one reusable function or fixture, then update
spikeDesktop.spec.ts and the regression spec to use that helper so they stay in
sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae0a1a1a-deae-4829-aa7b-479f132dcd81
📒 Files selected for processing (20)
browser_tests/README.mdbrowser_tests/assets/customNodes/core_smoke.jsonbrowser_tests/assets/customNodes/impact_primitives_run.jsonbrowser_tests/assets/customNodes/vhs_video_pipeline_run.jsonbrowser_tests/fixtures/ComfyPage.tsbrowser_tests/fixtures/customNode/ComfyTarget.tsbrowser_tests/fixtures/customNode/manifest.tsbrowser_tests/fixtures/customNode/objectInfoValidator.tsbrowser_tests/fixtures/customNode/runResult.tsbrowser_tests/fixtures/data/customNodeManifest.jsonbrowser_tests/fixtures/utils/consoleErrorCollector.tsbrowser_tests/fixtures/utils/errorSurfaces.tsbrowser_tests/tests/customNodes/README.mdbrowser_tests/tests/customNodes/customNode.regression.spec.tsbrowser_tests/tests/customNodes/manifest.pure.spec.tsbrowser_tests/tests/customNodes/objectInfoValidator.pure.spec.tsbrowser_tests/tests/customNodes/runResult.pure.spec.tsbrowser_tests/tests/customNodes/spikeDesktop.spec.tspackage.jsonplaywright.chrome.config.ts
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #13389 +/- ##
=======================================
Coverage 81.33% 81.34%
=======================================
Files 1873 1873
Lines 106533 106545 +12
Branches 33235 33236 +1
=======================================
+ Hits 86653 86666 +13
+ Misses 19536 19525 -11
- Partials 344 354 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
f8d2ee6 to
addcb60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
browser_tests/fixtures/customNode/manifest.ts (1)
25-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
requiresGpuhere as well.The regression spec already branches on
entry.requiresGpu, butassertEntry()does not require that field. If it is missing or misspelled in the manifest, GPU-only packs can be treated as CPU-runnable and the run tier will misclassify them.🔧 Minimal fix
- ['pack', 'workflow', 'expectedNodes', 'tiers', 'requiresModels'] as const + ['pack', 'workflow', 'expectedNodes', 'tiers', 'requiresModels', 'requiresGpu'] as const🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser_tests/fixtures/customNode/manifest.ts` around lines 25 - 34, `assertEntry()` in `manifest.ts` is missing validation for `requiresGpu`, so manifest entries can omit or misspell it and still pass. Update the missing-field checks in `assertEntry(entry, index)` to require `requiresGpu` alongside the other mandatory properties, using the same null/undefined validation pattern and error reporting already used for `pack`, `workflow`, `expectedNodes`, `tiers`, and `requiresModels`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/customNodes/connectivity.spec.ts`:
- Around line 292-294: The `outDot.dragTo` call in `connectivity.spec.ts` is
tripping the `no-force-option` lint rule because it uses `{ force: true }`; keep
the forced drag since the z-999 overlay blocks pointer events, but add a local
lint suppression on that specific call using the existing overlay justification
so the `connectivity.spec` test remains valid and CI passes.
---
Outside diff comments:
In `@browser_tests/fixtures/customNode/manifest.ts`:
- Around line 25-34: `assertEntry()` in `manifest.ts` is missing validation for
`requiresGpu`, so manifest entries can omit or misspell it and still pass.
Update the missing-field checks in `assertEntry(entry, index)` to require
`requiresGpu` alongside the other mandatory properties, using the same
null/undefined validation pattern and error reporting already used for `pack`,
`workflow`, `expectedNodes`, `tiers`, and `requiresModels`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: faeeeab0-0c5a-45ba-9239-21ef7d2f47b2
📒 Files selected for processing (7)
browser_tests/fixtures/customNode/manifest.tsbrowser_tests/fixtures/customNode/typePairing.tsbrowser_tests/fixtures/data/customNodeManifest.jsonbrowser_tests/tests/customNodes/README.mdbrowser_tests/tests/customNodes/connectivity.spec.tsbrowser_tests/tests/customNodes/typePairing.pure.spec.tspackage.json
bbb543c to
7a92da1
Compare
A type-pairing generator indexes /object_info producers and consumers and plans one representative typed edge per slot, excluding wildcard slots (isValidConnection short-circuits on * before the real type compare, so a wildcard link proves reachability, not interop). The breadth sweep connects every planned edge through the real validator in-page and requires each link to survive serialize/configure and appear in graphToPrompt output; verified up front that graphToPrompt emits links even when other required inputs dangle. A curated subset is dragged slot-dot to slot-dot under both renderers, addressed by data-slot-key so shared labels cannot misfire. Orphan types are reported, never failed; connect vetoes must match a committed allow-list. Manifest packs opt in via a connectivity tier that needs no extra assets.
7a92da1 to
8b81a4f
Compare
A permanent self-check feeds the shared pair executor a type-incompatible pair and a fabricated slot name and requires CONNECT_REJECTED and SLOT_CONTRACT_MISMATCH back, so a green sweep can never come from a classifier that lost the ability to fail. The breadth test asserts every connectivity-tier pack contributes pairs, guarding pack attribution. The drag tier's widget-primitive exclusion is removed: widget-backed inputs render real slot dots under Vue Nodes (verified empirically), so every pack now gets an in-pack drag in both renderers, asserted present.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
browser_tests/fixtures/customNode/manifest.ts (1)
25-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten manifest shape checks.
assertEntryonly rejects nullish values, so malformedtiers/expectedNodesvalues can still pass and later skew the connectivity filter and length-based checks. Validate the actual array types here (and keeptimeoutMsfinite/positive) so bad manifests fail fast.Based on source_other:
browser_tests/tests/customNodes/connectivity.spec.tsandbrowser_tests/tests/customNodes/customNode.regression.spec.tsconsume these fields as arrays.🛠️ Proposed fix
function assertEntry(entry: CustomNodeManifestEntry, index: number): void { - const missing: string[] = ( - ['pack', 'workflow', 'expectedNodes', 'tiers', 'requiresModels'] as const - ).filter((key) => entry[key] == null) - if (typeof entry.timeoutMs !== 'number') missing.push('timeoutMs') + const missing: string[] = [] + if (typeof entry.pack !== 'string' || entry.pack.length === 0) missing.push('pack') + if (typeof entry.workflow !== 'string' || entry.workflow.length === 0) missing.push('workflow') + if (!Array.isArray(entry.expectedNodes)) missing.push('expectedNodes') + if (!Array.isArray(entry.tiers)) missing.push('tiers') + if (!Array.isArray(entry.requiresModels)) missing.push('requiresModels') + if (!Number.isFinite(entry.timeoutMs) || entry.timeoutMs <= 0) missing.push('timeoutMs')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@browser_tests/fixtures/customNode/manifest.ts` around lines 25 - 34, The manifest validation in assertEntry only checks for nullish values, so malformed tiers and expectedNodes can still pass through and break later array-based logic. Update assertEntry to verify those fields are actual arrays before accepting the entry, and also tighten timeoutMs validation in the same function to require a finite positive number. Keep the checks centralized in assertEntry so bad custom node manifests fail fast before connectivity.spec and customNode.regression.spec consume the data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@browser_tests/tests/customNodes/connectivity.spec.ts`:
- Line 94: The breadth sweep in connectivity.spec.ts starts a console error
collector via collectConsoleErrors(comfyPage.page), but the collected errors are
never asserted before stopping it. Update the breadth-sweep test around the
collectConsoleErrors usage and the final stop/teardown block to check
consoleErrors.errors the same way the fidelity test does, so any console errors
during the connect/serialize/prompt loop fail the test.
- Around line 59-61: The concrete() helper is duplicating the wildcard check
instead of reusing the shared predicate. Export isWildcard from typePairing.ts
and update concrete() in connectivity.spec.ts to use !isWildcard(slot.type) so
the wildcard logic stays centralized and cannot drift apart.
- Around line 217-230: The “in-pack” selection in the connectivity test is using
the full node pool, so it can return a compatible pair from any pack instead of
the current one. Update the logic around connectivityEntries to build the
candidate set from only the nodes in entry.pack before calling planPairs, rather
than filtering after the fact. Use the planPairs result and the
pair.producer.pack/pair.consumer.pack check only as a sanity guard, and keep
dragEdges populated from same-pack matches.
---
Outside diff comments:
In `@browser_tests/fixtures/customNode/manifest.ts`:
- Around line 25-34: The manifest validation in assertEntry only checks for
nullish values, so malformed tiers and expectedNodes can still pass through and
break later array-based logic. Update assertEntry to verify those fields are
actual arrays before accepting the entry, and also tighten timeoutMs validation
in the same function to require a finite positive number. Keep the checks
centralized in assertEntry so bad custom node manifests fail fast before
connectivity.spec and customNode.regression.spec consume the data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc2ff995-7c7c-4e41-b687-b2109380e87f
📒 Files selected for processing (7)
browser_tests/fixtures/customNode/manifest.tsbrowser_tests/fixtures/customNode/typePairing.tsbrowser_tests/fixtures/data/customNodeManifest.jsonbrowser_tests/tests/customNodes/README.mdbrowser_tests/tests/customNodes/connectivity.spec.tsbrowser_tests/tests/customNodes/typePairing.pure.spec.tspackage.json
Resolve the manifest path from import.meta.url so tests are cwd-independent, and validate requiresGpu at manifest load. Reuse the centralized TestIds for the error overlay, error dialog, and templates dialog selectors. Extract the shared suite settings and templates-dialog dismissal into fixtures/utils/customNodeSuite so the three specs cannot drift. Rename spikeDesktop.spec.ts to coreSmoke.spec.ts to match its maintained purpose, document the full manifest schema in the README, and describe the gate outcome without a hardcoded test count.
…checks The breadth sweep now fails on any console error captured during the connect/serialize/prompt loop, matching the fidelity test. The wildcard predicate is exported from typePairing and reused instead of re-derived. assertEntry validates real shapes (non-empty pack/expectedNodes/tiers, arrays, boolean requiresGpu, finite positive timeoutMs); workflow stays allowed as an empty string until a pack gains a run-tier fixture.
knip flags exported types with no external consumers; CustomNodeTier, ObjectInfoNode, NormalizedSlot, and SlotRef are referenced only within their own modules.
The Comfy.userId=default settings override broke every test on multi-user backends (the repo's stated browser-test prerequisite): devtools set_settings wrote to a user no session reads, so Comfy.TutorialCompleted never landed, the templates dialog never opened, and the beforeEach wait timed out - CI sessions even inherited leftover settings (a zh locale) from earlier tests on the same worker user. Dropping the override lets the fixture target the real per-worker user everywhere; the harness backend now runs --multi-user like CI. Connectivity's per-pack guards and drag derivation apply only to installed packs, so a backend without the manifest packs reports the absence instead of hard-failing while the core sweep, native drag, and self-checks still run.
CI caught what a pack-rich local backend masked: isValidConnection compares only the string COMBO while every combo slot carries its own option set, so the planner would wire a checkpoint dropdown into a scheduler dropdown and call it proof, and combo outputs declare a non-string output_name whose instance slot name never matches (DevToolsNodeWithOutputCombo failed 5 pairs on CI as SLOT_CONTRACT_MISMATCH). Combo slots are now recorded and counted like wildcards instead of paired, the normalizer coerces slot names to strings, and a pure spec locks both behaviors. Targeted fixtures remain the way to cover combo semantics.
T-conn was planning-doc shorthand for the connectivity tier; test titles and logs now say connectivity outright so CI output reads without tribal knowledge.
Phase 5. A new informational (non-gating) workflow that reuses the repo's setup-frontend/setup-playwright/setup-comfyui-server actions, then installs every pack the manifest declares (jq loop over customNodeManifest.json, so a new pack row installs itself with no workflow change) and boots ComfyUI with --multi-user --cache-none before running browser_tests/tests/customNodes. This makes the load and run tiers actually execute in CI instead of skipping for want of the packs - the whole point of the suite. A pack whose deps fail degrades to an honest skip rather than reddening the job.
…ny skip A regression gate that lets a broken pack through as a skip is theater. Pack clone/dependency failures now fail the job (array+loop instead of a failure-swallowing jq|while pipe), and a post-run check fails the job if any test was skipped - on this backend every tier is meant to run, so a skip means a pack or devtools did not load. Drops the informational framing; mark custom-nodes-e2e required in branch protection to block merges.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
…ict (#15027) ## Summary A backend 5xx on `POST /prompt` now fails the run tier once, naming the backend, instead of classifying `VALIDATION_FAIL` against the node under test. 400 stays a pack-attributable validation reject. ## Changes - **What**: `runWorkflow` keeps the captured `/prompt` status alongside the summarized body (`PromptRejection`). When the final captured status is >= 500 it throws `prompt submission failed server-side (HTTP <status> POST /prompt) - <body message> - backend/environment fault, not a pack validation reject` at both classification sites (double-refusal and resolved-with-rejection), matching the fail-loud nomenclature of the boot check (`cloud required boot request failed: HTTP ...`) and the token check (`workspace token mint failed (HTTP ...)`). A captured 5xx outranks a client-side throw: the backend demonstrably failed that submission server-side. The client-flap retry and the stale-response sequence guard are unchanged - the 500-then-200 retry pure test passes untouched. ## Review Focus - Attribution is the point. During the 2026-08-09 testcloud incident (out-of-order migration, fixed by Comfy-Org/cloud#6486, left ingest unable to INSERT jobs) every submission answered 500 `DATABASE_ERROR` and the cloud gate reported **1,563 per-node VALIDATION_FAIL "regressions" across 68 packs**, each labeled `not in cannotRunAlone; a regression, or a new baseline entry` (run 31418724428). With this change each affected test fails once with the true cause. - One core-gate edge is deliberate: pack Python that crashes inside `/prompt` validation also 500s, and now reads as an environment fault rather than a per-node verdict. The tier still fails red either way; only the attribution changes. The 400-with-`node_errors` path packs actually exercise is untouched. - Red-green: both new pure tests fail against the previous classifier (resolve to `VALIDATION_FAIL`); 9/9 promptError pure tests and the 183-test pure/self-check battery pass with the change. No `AUTO_RUN_ALLOWED_FAILURES` entry depends on a 5xx-shaped `VALIDATION_FAIL` (all are `TIMEOUT`/`EXECUTION_ERROR`-shaped). Fixes [FE-1555](https://linear.app/comfyorg/issue/FE-1555/custom-node-suite-backend-5xx-misattributed-as-per-node-pack)
…-e2e-suite # Conflicts: # browser_tests/fixtures/ComfyPage.ts
…15047) ## Summary The shared coverage fixture called `stopJSCoverage` on a page the test had legitimately closed, converting a passing test into a deterministic CI failure. ## Changes - **What**: `page.isClosed()` guard before `stopJSCoverage` in the coverage-collecting `page` fixture override. A closed page has no coverage to collect; behavior is unchanged for every test that leaves its page open. ## Review Focus - `customNodeSuite.pure.spec.ts › does not count same-document hash navigation as a second boot` calls `finalizeCloudCustomNodeBootGuard(page)`, whose `close` action is `page.close()` by design (the enforcement boundary). The fixture teardown then threw `coverage.stopJSCoverage: Target page, context or browser has been closed` (`ComfyPage.ts:816`) - failing chromium shard 3/16 identically on three consecutive runs while passing locally, because the fixture is gated on `COLLECT_COVERAGE=true` (CI shards only). - Red-green reproduced locally: `COLLECT_COVERAGE=true pnpm exec playwright test browser_tests/tests/customNodes/customNodeSuite.pure.spec.ts -g "hash navigation"` fails on the base branch and passes with the guard; the promptError pure specs confirm coverage collection is untouched for tests that keep their page open.
…15055) ## Summary Follow-up to #15027 review: the incident regression test claimed the production outage path but exercised the double-refusal classifier - its mocked 500 body has no `node_errors`, and `app.queuePrompt` sets `queueResultOverride = !nodeErrors` (`app.ts:1855`), so production resolved that submission `true` and the real outage traversed the resolved-with-rejection classifier. ## Changes - **What**: the incident mock now returns `true` with the comment corrected (asserts `evaluateCalls === 2`: no retry on the resolved path); double-refusal keeps dedicated coverage in a separately named test whose body genuinely makes production return `false` (`node_errors` present), asserting the client-flap retry is spent (`evaluateCalls === 3`) before the environment verdict. Both 5xx classification sites now have path-faithful coverage; the fault message and `isServerSideFault` contract assertions are unchanged. ## Review Focus - Addresses #15027 (comment) as proposed. The test timings confirm the split: resolved path ~1ms, double-refusal ~251ms (the 250ms retry beat). 10/10 promptError pure specs green.
…#15056) ## Summary Manual-run UI for the core gate: dispatch it at any frontend branch, optionally as a tier/pack subset or against a ComfyUI candidate, and read the verdict as a per-pack table in the job log and the run's Summary panel. ## Changes - **What**: - `workflow_dispatch` inputs: `branch` (frontend ref under test - the checkout honors it; every other event tests its own ref exactly as before), `grep` (Playwright `-g` subset per the suite's title-pattern design; passed via env, never shell-interpolated; the exact-count gate logs and skips for subsets while the failed/skipped/flaky gates stay active), `comfyui_ref` (probe a core candidate without editing the pin - gating events stay pinned), and `enable_s14`/`enable_s15` toggles (non-dispatch events keep both at `1`). - Dispatches get a per-run concurrency group so two manual runs (e.g. comparing branches) cannot cancel each other; PR pushes keep per-ref supersede. - A `Publish results table` step (`if: always()`) parses `custom-nodes-results.json` and renders: branch tested / head SHA / ComfyUI ref / event+actor / grep / S14-S15 / totals+duration, a per-pack x tier (startup, all-nodes, curated, dynamic inputs, interaction) verdict table, suite-wide rows (connectivity, smoke, manifest coverage, harness self-checks, pure specs), and failed tests with their first error line - to both the job log and `$GITHUB_STEP_SUMMARY`. - **Deliberately not parameterized**: workers (the serial contract is a suite invariant), retries (gate honesty), pack pins (the manifest owns them), golden regeneration (the record workflows own it). ## Review Focus - The "Run workflow" button appears only once the workflow file reaches the default branch (post-#13389); until then dispatch via `gh workflow run ci-tests-custom-nodes.yaml --ref <branch> -f branch=... -f grep=...`. - Summary renderer dry-run against a real gate artifact (run 31513986272): correctly pinpointed the VHS all-nodes geometry failure as `FAIL 1/1` with all other cells PASS. Live dispatch validation on this PR's branch to follow in a comment.
…sh (#15059) ## Summary Fixes the two flakes in [run 31537261792](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31537261792/job/93931291364) via the suite's own escape hatches - ledger entries with named mechanisms, not code changes. ## Changes - **Geometry (was-node-suite)**: nine nodes measured 8-48px shorter in `vue.h` than run 31518805275 recorded at identical suite code, while **every widget height on the same nodes matched the baseline exactly** - the flip sits in slot/section layout, not in any widget. That is the failure message's ledger case verbatim ("delta flips between identical runs"). Added `GEOMETRY_UNSTABLE_PATHS` entries relaxing only `vue.h` per node, observed values cited; widget geometry stays strict. - **Geometry (VHS)**: the earlier episode ([run 31513986272](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31513986272), rerun of the identical SHA passed) hit the preview widget **one index below** the ledgered one (`widgets[4]/[8]/[7]`) plus un-ledgered `VHS_LoadAudioUpload.vue.h` - covered with the same async default-media mechanism so that flake cannot return either. - **Connectivity sweep**: the uncaught page error is pack-owned - `Custom-Scripts/js/mathExpression.js:31` draws `app.nodeOutputs[this.id].value[0]` in `onDrawForeground`; the sweep repeatedly clears graphs, ids recycle, and a stale value-less `nodeOutputs` entry under a reused id throws. Allowlisted with the mechanism, deliberately **without** `requiredConnectivityId`: a frame-timing crash must not become a must-fire staleness obligation. Upstream-report candidate for pysssss. - Ledger-guard pure specs updated to pin the new sets (the WAS guard asserts structurally that only `vue.h` is relaxed). 35/35 pure specs green. ## Review Focus - The WAS mechanism wording states exactly what is known (whole-node height flip, widgets stable) without claiming a deeper cause not yet observed - if the underlying Vue-renderer layout wobble gets root-caused, these entries name precisely where to look.
## Summary The cloud gate has zero genuine green runs and constant cancellations - both are structural, not test-content, problems. This makes every run serial and stops per-PR pushes from evicting each other. ## Changes - **What**: - **Drop the `pull_request` trigger.** Every automatic run shares one serial resource (the smoke account's queue) behind a single-pending-slot concurrency group, so per-PR pushes stampeded it: **16 of the last 40 runs were cancelled**, mostly PR-triggered runs evicting each other (four in a row 21:03-21:18 on 2026-08-11), and no PR-triggered run produced a verdict a PR acted on. PR-time custom-node coverage stays with the core gate and the pure specs in the main shards; Cloud verdicts come from suite-branch/main pushes, the merge queue, and dispatch. - **Fold dispatches into the instance group.** The separate dispatch group allowed a manual run to execute concurrently with an instance-group run - observed live: dispatch 31541231667 co-tenant with run 31530265854, which then sat 125m+ in its suite step. Co-tenancy corrupts whole-queue observation (`waitForQueueQuiet`) for both runs. The eviction pressure that justified the split dies with the per-PR trigger. - **`timeout-minutes: 150`** on the job (longest healthy suite ~110m): a wedged run can no longer hold the serial group for GitHub's 360m default. ## Review Focus - This trades PR-time cloud signal (which the eviction data shows was ~never delivered) for runs that actually complete. If per-PR cloud coverage is wanted later, it needs the prompt-scoped queue observation change first - the serial contract is the binding constraint, not this workflow. - The fork-safety job `if:` still references `pull_request`; left in place deliberately as defence-in-depth if the trigger ever returns.
…15073) ## Summary The bare `VALIDATION_FAIL` flip-flop is a queue race with a verified source chain - not a submit-time hook, and not a pack defect of the failing node. The emitters are excluded as the cause; the victim keeps executing under an allowed-failure entry. ## Changes - **What**: - `ImpactQueueTrigger` joins `AUTO_RUN_EXCLUDE`: its backend execution emits `impact-add-queue` whenever its mode widget is on - **the default** (`modules/impact/logics.py`, `doit`); Impact's JS answers with a background `app.queuePrompt(0, 1)` (`js/common.js:113`); the frontend re-entrancy guard (`app.ts:1640`, `if (this.processingQueue) return false`) then refuses the harness's next submission with **no POST** - the bare-`VALIDATION_FAIL` signature - pinning the failure on whichever node queues next, through the 250ms retry when the background submission is still processing. - `ImpactQueueTriggerCountdown`'s inherited reason text is corrected: the pack installs **no** `queuePrompt`/`beforeQueued`/`graphToPrompt` hook anywhere in its JS; its real mechanism is the same backend-emit chain. - `ImpactSelectNthItemOfAnyList` - the victim, not the cause - moves from `AUTO_RUN_EXCLUDE` to `AUTO_RUN_ALLOWED_FAILURES` (`outcomes: ['VALIDATION_FAIL']`): it keeps executing every run with full coverage, the bare refusal is tolerated while the emitters sit excluded, and no stale-entry alarm fires on passing runs. ## Review Focus - Evidence: S9 failed with exactly this node in [run 31545300324](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31545300324) and passed in [run 31537458068](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31537458068) on the identical commit; every mechanism link above is cited to pinned pack source or frontend source, addressing #15073 (comment). - A candidate follow-up outside this PR: the harness could wait for `app.processingQueue` to clear before submitting, closing this race class for every pack at the source.
…15083) ## Summary The core gate's suite runs in 10-16m but setup adds ~12-16m; this caches the pinned pack trees - the half of the original proposal the measurements and review supported. (The pip-cache half was removed: its key froze on the pinned requirements hash while torch floats, and live measurement showed it wall-clock neutral.) ## Changes - **What**: the manifest packs are fully determined by their SHA pins, so their checked-out trees (sans `.git`) cache under `cn-packs-v2-<manifest hash>` with a prefix fallback - a single-pin bump still reuses every other pack. Hardened per review: - Save gates on the **install step's** outcome (`always() && steps.install.outcome == 'success'`): a suite failure still saves a complete tree; a failed or cancelled install can never freeze a partial one under the immutable key. - Save skips entirely on an exact-key restore (`cache-hit != 'true'`): `actions/cache/save` re-tars and uploads unconditionally before the server's duplicate-key rejection, so the steady-state green run now costs nothing. - Entries prune to the current manifest's pack set before saving - no monotonic growth into the repo's shared 10GB budget. - The integrity marker is a **sibling** `$cache_dir.pin` written last: restored trees are byte-identical to fresh checkouts, truncated copies fail the check, and a pack shipping its own top-level `.pin` installs identically either way. - Cache misses take exactly the pre-change install path; pack `requirements.txt` installs run on every path so a dependency break still fails the gate loudly. ## Review Focus - v1-key validation (populate [31549672781](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31549672781) / restore [31550313207](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31550313207)): all six `restore <pack> from cache` lines in the logs, install step 100s -> 77s. A fresh populate+restore pair on the v2 schema will be dispatched and linked here.
…15099) ## Summary The cloud gate's dominant remaining failure class - 12x `at startup: errorToasts` in the serialized measurement ([run 31541231667](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31541231667)) - reports a bare element count, so testcloud-health noise and real frontend errors are indistinguishable without a trace dive. Every red on these surfaces now carries the surface's visible text. ## Changes - **What**: `expectNoVisibleErrors` polls the surface's inner texts to `[]` instead of `toHaveCount(0)`. Semantics preserved: a transient surface that clears within the expect window still passes (proved by the pure spec's 999ms transient case vs 5.0s persistent case); a persistent one fails with its texts as the last polled value; the class-stable `<context>: <surface>` label survives in the failure message for the detection-proof greps; a mid-poll read race returns a sentinel rather than throwing, so the poll retries exactly where `toHaveCount` would have. - New `errorSurfaces.pure.spec.ts` pins all three behaviors with a real page and no backend; red-green proven - against the previous fixture the text assertions fail (`toHaveCount` output carries no toast text). ## Review Focus - Single enforcement point: every caller (startup checks, curated runs, all four surfaces) inherits the diagnostics at once. With this in place, the 12-failure class becomes one-glance triage: a wall of `HTTP 502`-flavored toasts is a testcloud ticket, anything else is ours.
The only failure in [run 31553987373](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31553987373) (this branch's last completed cloud run): `AudioConcatenate` rejects with 400 `required_input_missing` for `audio1`, `audio2`, and `direction` - it needs two wired audio inputs and belongs in `cannotRunAlone`. Two-line ledger addition (both cloud ledger files), mirroring the conclusion already reached independently on the tier-isolation branch. - `pnpm exec playwright test manifest.pure --project=chromium`: 15 passed - Green-candidate evidence run dispatched from this branch: [31651305020](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/31651305020) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Conflicts resolved: pnpm-lock.yaml regenerated from the merged package.json; CustomerIoTelemetryProvider and its test converged on main's version (main's bounded-await identify rework supersedes the branch's earlier fire-and-forget attempt at the same non-stalling goal; 42/42 provider tests pass, vue-tsc clean).
github-actions
Bot
added
the
risk:R3
Interaction profiles are recorder output and the cloud manifest is gen:cloud-manifest output - same nature as the geometry goldens already marked above; collapse them in review like the goldens.
The dev web API key is a public client identifier (it ships in every bundle via src/config/firebase.ts), but the fixture duplicated the literal because the SDK's storage lookup keys embed it. Export it from the config module and import it, so review does not read a duplicated key as a leaked secret and the value cannot drift.
There was a problem hiding this comment.
Review verdict: request changes
This is an ambitious regression suite, but its current architecture mixes useful behavioral coverage with security-sensitive workflows, test-only authentication that bypasses the application contract, broad suppression/allowlisting, and a large body of change-detector tests. I do not think it is safe or maintainable as a required gate in this form.
I reviewed current head 8ad9a478 against main, with independent passes over type safety, CI security, authentication, Playwright isolation, generated baselines, manifests, and test quality. The concrete defects are annotated inline; this summary explains how they interact and why diagnosing failures will be unusually difficult.
1. Trust boundaries are crossed incorrectly
The Cloud recording workflow executes branch-controlled code while exposing account credentials. The nightly canaries execute floating upstream repositories and dependencies in a job holding a write-capable token. GitHub's security-hardening guidance recommends treating third-party and mutable code as untrusted and minimizing token permissions.
flowchart LR
A[Collaborator pushes record branch] --> B[Checkout branch-controlled workflow and code]
B --> C[Local setup actions and package scripts]
C --> D[Playwright and custom-node execution]
S[Smoke email and password] --> C
T[Persisted GitHub credential] --> D
D --> E[Credential exfiltration or repository mutation]
Hypothetical failure
A collaborator creates record/custom-nodes-cloud-debug and changes setup-frontend, build:cloud-e2e, or a Playwright import to send environment values to an external endpoint. The workflow intentionally supplies the smoke credentials to branch code. Exfiltration can happen while the recording still produces a normal-looking artifact.
Diagnostic problem
There may be no failed test or suspicious job output. Reviewers would have to audit every executable transitive import on the pushed branch after the credential has already been exposed. That is not a useful detection boundary.
Required direction
- Remove the secret-bearing
pushtrigger. - Execute secret-bearing workflow logic from trusted default-branch code only.
- Set
persist-credentials: falsefor jobs that execute external code. - Give test jobs
contents: readonly. - Move issue creation into a dependent fixed-code job with
issues: write.
2. Cloud authentication is repaired after application code runs
The fixture seeds Firebase implementation details and then replaces outbound authorization headers. This may bootstrap custom-node tests, but it cannot demonstrate that the built Cloud application authenticated correctly.
flowchart TD
A[Application chooses auth token] --> B[Outgoing API request]
B --> C[Playwright route handler]
C -->|replace Authorization| D[Fixture workspace JWT]
D --> E[Cloud API accepts request]
E --> F[Test reports green]
A -. missing, stale, or wrong token .-> B
Hypothetical failure
authStore.getAuthHeader() regresses and returns null, a stale Firebase token, or a token for the wrong workspace. In production, authenticated API requests fail. In this suite, Playwright replaces the bad header with its cached workspace JWT, and all custom-node calls continue successfully.
Diagnostic problem
There is no red signal to diagnose. The test fixture has repaired the behavior whose correctness a Cloud run appears to imply. Conversely, after roughly one hour, the fixture's cached Firebase token can expire; when the workspace token needs renewal, the suite can produce a wall of unrelated authentication failures. Developers would need to distinguish:
- production token-selection regression,
- expired fixture credentials,
- Cloud outage,
- smoke-account state pollution, and
- actual custom-node failure.
The present logs do not establish that distinction.
Required direction
Either authenticate through the real application store/session path, or explicitly declare authentication out of scope and assert the application's original header before installing any rewrite. Refresh Firebase credentials before workspace-token renewal and key caches by origin plus account.
3. The suite contains many change-detector tests
Google's “Change-Detector Tests Considered Harmful” distinguishes tests that prove externally meaningful behavior from tests that merely report that today's implementation changed.
This PR explicitly adopts the latter model in several places:
- Interaction profiles state that “whatever a node's JS does today is the baseline.”
- Geometry snapshots lock tens of thousands of incidental renderer coordinates.
- Pure specs duplicate complete exception-ledger inventories.
- Workflow tests parse YAML and assert exact command, reporter, environment, and test-count literals.
- Generated baselines are marked generated, reducing ordinary review visibility while remaining required-test inputs.
flowchart TD
A[Implementation or environment changes] --> B[Snapshot or inventory differs]
B --> C[Required gate turns red]
C --> D[Developer inspects generated data and ledgers]
D --> E{User behavior broken?}
E -->|No| F[Noise: regenerate or expand exception ledger]
E -->|Yes| G[Real signal]
F --> H[Baseline becomes more permissive]
H --> I[Future regression can be hidden]
G --> J[Same initial symptom as noise]
Signal-versus-noise scenarios
| Change | Observed result | Diagnostic ambiguity |
|---|---|---|
| Browser, font, rasterization, or device-pixel rounding changes | Potentially thousands of geometry-coordinate deltas | Same presentation as a real layout regression; developers must visually reconstruct whether each float matters |
| Pack initializes a custom element one task later | Geometry/profile drift or a newly “unstable” node | Could be harmless scheduling, a pack regression, or a frontend lifecycle regression |
| Pack adds a duplicate same-named slot | Interaction profile may remain green | Set conversion discards multiplicity, so a real topology change has no signal |
| Workflow command is refactored with identical collected tests | Unit test fails | YAML spelling changed, behavior did not; current PR is already red for this exact reason |
| PNG encoder changes filter/compression strategy | “Pixel hash” fails | Decoded pixels are identical, but compressed IDAT bytes differ |
| MP4 output is empty or corrupt | Output hash remains green | Non-PNG file content is never fetched or hashed |
| Exception ledger adds/removes one calibrated node | Inventory test fails | Test only proves two copies of the inventory differ; it does not evaluate behavior |
| A transient error toast appears for two seconds | Error check eventually passes | The user saw an error, but polling erased the evidence |
Why this matters operationally
A required gate must make failures cheaper to classify than the bugs it catches. Here, broad snapshots and ledgers create an expensive recurring triage loop:
flowchart LR
R[Red CI] --> A[Download large artifact]
A --> B[Compare generated baseline]
B --> C[Reproduce pinned core and packs]
C --> D[Decide frontend vs pack vs environment]
D --> E[Regenerate baseline or add exception]
E --> F[Rerun 1 to 5 hour workflow]
F -->|still red| A
The likely long-term response to recurring noise is expanding allowlists and unstable-path ledgers. That reduces confidence exactly when the suite becomes expensive enough that developers stop investigating each delta deeply.
Required direction
Replace exhaustive implementation snapshots with semantic behavioral invariants:
- declared inputs remain present and usable,
- expected sinks and outputs exist,
- slot/widget counts preserve multiplicity,
- coordinates are finite and elements do not overlap,
- interaction topology stabilizes after lifecycle work,
- decoded output content matches meaningful expectations,
- every observed visible error is retained,
- cleanup reaches a verified idle state.
Use a small curated visual/geometry set for historically risky nodes rather than exact coordinates for the entire ecosystem.
4. Error attribution creates false positives and false negatives
The suite currently:
- polls until transient visible errors disappear,
- suppresses prompt-related console messages using broad text patterns,
- accepts websocket events without prompt IDs into the current run,
- ignores backend-drain failure in normal teardown,
- globally waits on a shared queue,
- and permits a Cloud positive-control test to return without executing its control.
flowchart TD
A[Test A submits prompt] --> B[Test A teardown times out silently]
B --> C[Test B starts on dirty backend]
C --> D[Late event or error reaches Test B]
D --> E{Classifier}
E -->|matches foreign-noise regex| F[Error silently discarded]
E -->|does not match| G[Test B fails]
F --> H[False green]
G --> I[False attribution to Test B]
Hypothetical failure
Pack A starts a non-interruptible prompt. drainBackendToIdle() returns 1, but afterEach discards it. Pack B starts, receives Pack A's late bare executing event and a prompt error. The event can be retained because it lacks prompt_id; the console error can be removed because it matches “foreign execution noise.” Depending on ordering, B can falsely pass, falsely report partial execution, or fail for A's error.
Diagnostic problem
The same underlying contamination produces three different symptoms. Developers will not know whether to inspect Pack A, Pack B, prompt attribution, queue isolation, or the allowlist. Historical run-ID comments do not solve this because they describe prior incidents rather than establish runtime provenance.
Required direction
flowchart LR
A[Submit prompt] --> B[Capture prompt ID]
B --> C[Collect only events for that ID]
C --> D[Retain every visible and console error]
D --> E[Assert terminal state]
E --> F[Assert owned work is idle]
F --> G[Report prompt, node, tier, and first causal error]
Prefer an isolated backend. Otherwise carry explicit prompt ownership end-to-end and reject unattributable events. Teardown must throw when owned work remains.
5. Type safety stops at the most important boundaries
No newly introduced any/as any was found, but several assertions hide unvalidated external data:
- extension-sentinel JSON is asserted to
Record<string, string[]>, - geometry JSON is asserted to
PackGeometryFile, - output-hash JSON is assigned to
CuratedOutputHashes, - workspace API response enums are widened to
string, - generated baseline provenance is present but not validated against the active environment.
These are not harmless compiler conveniences. They convert malformed or stale external data into trusted domain objects without runtime proof.
Hypothetical failure
A sentinel key is misspelled. Manifest generation succeeds because unmatched keys are not rejected. The corresponding pack receives expectedExtensions: [], and frontend-extension loading is no longer checked. CI becomes greener by losing coverage.
Required direction
Parse as unknown, validate at ingress, reject unknown keys, require exactly one core row, and reuse production/generated API schemas rather than duplicating looser interfaces.
6. Output-regression claims exceed actual coverage
The PNG path hashes compressed IDAT payloads, not decoded pixels. The non-PNG path canonicalizes file references to extensions without reading content.
flowchart TD
A[Sink payload] --> B{File extension}
B -->|PNG| C[Hash compressed IDAT bytes]
B -->|MP4, audio, GIF| D[Keep extension only]
C --> E[False red when encoding changes]
D --> F[False green when content changes]
A trustworthy content tier should decode PNGs to normalized RGBA and hash the decoded bytes. It should hash or decode supported video/audio content, or fail explicitly and narrow its documented scope. It should also require declared sinks; iterating an empty sink list is not evidence.
7. Baseline recording can publish partial or stale data
Record steps use continue-on-error. Geometry files are cleared, but interaction profiles are not. Artifact checks establish only that a directory contains files, not that every manifest pack was recorded with matching schema and provenance. The Cloud step ceilings total 380 minutes before setup while the job ceiling is 350 minutes, so the job can be cancelled before upload despite comments claiming uploads remain reachable.
Hypothetical failure
Recording crashes after the first pack. Geometry artifact contains one fresh file. Interaction-profile artifact contains one fresh file plus old committed files for all remaining packs. Both uploads look complete to a human downloading them.
Diagnostic problem
A later comparison can report drift against mixed provenance. Developers must infer which files were generated in which run. The recorded metadata is not sufficiently validated to reject the mixture.
Required direction
Write every product into a fresh run-specific directory, clear all destinations, validate the exact pack set/count/schema/provenance, and upload only after validation passes.
8. Comment volume violates repository guidance
A conservative diff-only prefix count found 1,623 added comment-only lines across 17,166 added TS/YAML/JS/Vue lines (~9.5%, excluding JSON and Markdown). Largest concentrations:
| File | Added comment-only lines |
|---|---|
allNodes.spec.ts |
165 |
ci-tests-custom-nodes-cloud.yaml |
129 |
ci-tests-custom-nodes.yaml |
112 |
ci-nightly-custom-nodes-canary.yaml |
95 |
consoleErrorLedger.ts |
79 |
customNode.regression.spec.ts |
78 |
record-custom-nodes-geometry-cloud.yaml |
72 |
connectivity.spec.ts |
72 |
Many comments are:
- historical run-ID incident journals,
- explanations of immediately following commands,
- repeated descriptions of names and types,
- claims that executable assertions should establish,
- multi-paragraph defenses of unusual return codes,
- and stale operational instructions embedded beside implementation.
This directly conflicts with AGENTS.md lines 208–221: do not add multi-line comments for trivial changes, do not paraphrase test setup, and delete comments that merely explain obvious code.
Why this impairs diagnosis
When comments contain many historical explanations, developers cannot know which statements remain current. For example, a comment may say a timeout keeps uploads reachable while the arithmetic of current step/job ceilings proves otherwise. The comment increases confidence while concealing drift.
Move incident timelines and run IDs to the design document or PR discussion. Keep only durable, non-obvious constraints. If an API needs a paragraph warning callers not to ignore its return value, change the API to throw or use a result type that callers must handle.
Required changes before approval
- Correct secret and repository-token trust boundaries.
- Decide whether Cloud auth is setup or tested behavior; stop silently repairing it if it is behavior.
- Replace broad geometry/interaction change detectors with semantic invariants and a small curated visual set.
- Make event/error attribution prompt-scoped and teardown fail-closed.
- Decode output content before hashing and require declared sinks.
- Runtime-validate every JSON/YAML/API boundary.
- Remove inventory and YAML-text change-detector tests.
- Make record artifacts complete, fresh, and provenance-validated.
- Remove incident-log and code-paraphrasing comments.
- Restore required checks: current head fails
scripts/playwright-cloud-trace.test.ts, and action pin validation rejects the two cache actions.
Validation evidence
pnpm typecheck:browser— passed- targeted ESLint over changed custom-node/auth/script files — passed
- 65 targeted pure Playwright tests — passed
scripts/cloud-manifest.test.ts— 33 passedscripts/playwright-cloud-trace.test.ts— 1 failed- GitHub pin validation — failed on
actions/cache/restore@v5andactions/cache/save@v5 git diff --check origin/main...HEAD— passed
| # default branch; pushing a record/custom-nodes-cloud-* ref runs this | ||
| # file from that ref (the core record workflow's escape hatch). Delete | ||
| # the ref after collecting the artifact. | ||
| push: |
There was a problem hiding this comment.
Blocking — secret-bearing workflow executes branch-controlled code.
A collaborator can push record/custom-nodes-cloud-*; this workflow then checks out that ref and runs its local actions, package scripts, Vite build, and Playwright code with SMOKE_ACCOUNT_EMAIL and SMOKE_ACCOUNT_PASSWORD available. Repository write access does not imply permission to read shared account credentials.
Hypothetical: a branch modifies setup-frontend or build:cloud-e2e to POST environment values elsewhere. The run still looks like a recording run, and the exfiltration may leave no useful failure signal. Reviewers cannot differentiate a legitimate branch implementation from credential-stealing code after the fact.
Remove the push escape hatch. Keep the workflow implementation on the trusted default branch and accept only inert inputs/artifacts, or use an environment with required approval and no arbitrary ref checkout. See GitHub's secure use reference.
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| issues: write |
There was a problem hiding this comment.
Blocking — floating third-party code runs with a write-capable repository token.
This job installs current ComfyUI/custom-node code and unpinned Python requirements, then executes them while the job has issues: write. actions/checkout also persists credentials by default. A compromised upstream dependency can recover that credential and mutate repository issues.
Split this into (1) a read-only test job using persist-credentials: false, and (2) a dependent issue-reporting job with issues: write that executes only fixed repository code and consumes the prior job's status. This makes a canary compromise unable to cross the repository write boundary.
| # unconfigured secret is a hard activation failure. | ||
| name: 'CI: Tests Custom Nodes Cloud' | ||
|
|
||
| on: |
There was a problem hiding this comment.
Blocking — this cannot provide the advertised PR gate.
There is no pull_request trigger, so ordinary PR commits receive no Cloud result. merge_group helps only when every merge goes through an enabled merge queue; a branch can otherwise merge with broken Cloud extension loading/auth/proxy behavior and discover it only after main is updated.
Hypothetical: a PR changes a Cloud-only API path. Core and pure tests pass, the PR has no Cloud check, and the push-to-main run becomes the first signal. Diagnostics now start after the bad revision is shared, and developers must distinguish that PR from concurrent main changes.
Either restore same-repository pull_request execution with a scalable account/resource strategy, enforce merge queue universally, or stop describing this as a PR gating check.
| cached.expiresAt - Date.now() > WORKSPACE_TOKEN_MIN_REMAINING_MS | ||
| ) | ||
| return cached | ||
| workspaceSession = mintWorkspaceSession(appUrl, user).catch( |
There was a problem hiding this comment.
Blocking — workspace-token refresh can use an expired Firebase ID token.
workspaceSession is refreshed when its JWT nears expiry, but user comes from the module-cached smokeUser; its Firebase access token is never refreshed. Firebase ID tokens normally expire after about one hour (Firebase session documentation), while this workflow allows 110–150-minute runs.
Scenario: at minute 65 the workspace JWT needs reminting. /api/auth/token receives the original expired Firebase bearer and every remaining pack fails. The resulting wall of auth failures looks like Cloud instability or a frontend regression rather than deterministic fixture expiration.
Refresh/re-sign the Firebase user before reminting, and key caches by account plus origin instead of process-global identity.
| ): Promise<void> { | ||
| const apiPrefix = new URL('/api/', appUrl).toString() | ||
| await page.route( | ||
| (url) => shouldRewriteAuthHeader(url, apiPrefix), |
There was a problem hiding this comment.
Blocking — the fixture repairs the application's authorization behavior on the wire.
Every matching API request has its Authorization header replaced with the fixture's workspace JWT. If authStore selects no token, a stale token, or the wrong token, this suite still succeeds because Playwright fixes the request after application code emits it.
That creates a zero-signal failure mode: a production auth regression remains green. If auth is intentionally outside this suite's scope, assert the app's generated header once before installing any rewrite and document the exclusion. Preferably seed state through the production session/store path and remove the route rewrite entirely.
| } from '@e2e/fixtures/customNode/geometry' | ||
| import { loadManifest } from '@e2e/fixtures/customNode/manifest' | ||
|
|
||
| test('ledgers only the source-driven VHS preview height paths', () => { |
There was a problem hiding this comment.
Change-detector inventory test.
This restates the complete exception ledger—pack names, node names, and field paths. Any legitimate calibration change requires editing production constants and their duplicated test inventory, but the test does not prove that ignored paths are narrow or that adjacent paths remain strict.
Keep one representative behavioral test: a ledgered path is ignored and a neighboring unledgered path fails. Remove exhaustive inventory duplication. The ledger diff itself is what reviewers should inspect.
| }) | ||
| }) | ||
|
|
||
| test('routes each artifact-proven mechanism to every observed renderer', () => { |
There was a problem hiding this comment.
Another change-detector inventory.
This snapshots all current exception members rather than a public behavior. Adding/removing a calibrated node breaks the test by design and tells the developer only that two copies differ. It gives no independent evidence that renderer partitioning, staleness, or topology matching is correct; those behaviors are tested below.
Delete the inventory assertion and retain small composable cases for renderer selection, stale entries, and exact transition matching.
| trackSubmittedPrompts(comfyPage.page) | ||
| }) | ||
|
|
||
| // Leave the shared backend idle after every test so the next test's fresh |
There was a problem hiding this comment.
Repository-guideline violation: excessive incident/paraphrase comments.
A conservative diff count finds 1,623 added comment-only lines in changed TS/YAML/JS/Vue files. This file alone adds about 165. Many blocks narrate the next statement, preserve CI run IDs/incidents, or compensate for APIs whose failure mode is not encoded in their type.
AGENTS.md explicitly forbids multi-line comments for trivial changes and comments paraphrasing setup lines. Move historical diagnostics to the design document/PR, keep only durable non-obvious rationale, and improve APIs so callers cannot silently misuse return codes.
| # provenance stamps 'unrecorded' (allNodes.spec.ts); the row's deployRef | ||
| # is recorded as the pin. | ||
| - name: Record cloud geometry baselines | ||
| continue-on-error: true |
There was a problem hiding this comment.
Record artifacts can be partial or stale while looking complete.
The geometry destination is cleared, but interaction profiles are not. All three producers use continue-on-error, and upload checks only for some file(s). A crash after one pack can publish a partial geometry set; failed interaction packs can be silently supplied by committed checkout files.
Clear every destination first, write into a fresh run-specific directory, and validate the exact manifest pack set, schema, provenance, and node counts before upload. Also note that the three step ceilings total 380 minutes before setup while the job ceiling is 350, so the job can be cancelled before uploads despite comments promising reachability.
| # check - a stale or partial entry re-clones. | ||
| - name: Restore manifest pack cache | ||
| id: pack-cache | ||
| uses: actions/cache/restore@v5 |
There was a problem hiding this comment.
Required check currently fails action pin validation.
Repository policy requires third-party actions to be pinned by full SHA. CI reports this line and the corresponding actions/cache/save@v5 line as unpinned. Pin both to the reviewed commit SHA; tags are mutable supply-chain inputs.
Move the shared public dev Firebase key into a pure module so Playwright fixture discovery does not evaluate the Vite-only build define.
## Summary Makes Custom Nodes E2E results independently attributable to S1, S2, S3, S9, and S14. Previously, one per-pack Playwright row ran several of those contracts together, so one failure could obscure which tier failed and could prevent later tier checks from producing their own result. This PR gives each tier its own test and page lifecycle, while retaining the same pack coverage and environment boundaries. ## Changes - **What**: - Replaces the combined all-node row with independent S1, S2, S3, S9, and S14 tests. Each tier receives a fresh application page and emits per-pack, per-tier results. - Preserves normal Core coverage: S14 and S15 remain active. Normal Cloud keeps its 87 manifest packs and 102 active S1-S12 test rows; Cloud S13-S15 remain deferred. - Publishes the independent tier results in the Core and Cloud job summaries, rather than collapsing them into one `all nodes` cell. - Adds isolated controlled-break patches for the split tiers, so a deliberate tier failure can be attributed to that tier. - Keeps Cloud S9 baseline handling strict while correcting source-proven stale outcomes. New or unexpectedly clean outcomes still fail. - Preserves geometry baseline recording after the split: explicit recorder mode writes S14 baselines, while normal Cloud S14 remains disabled. - **Breaking**: none. No production behavior changes. - **Dependencies**: none. ## Review Focus - Confirm S1, S2, S3, S9, and S14 are independently selected and reported, with no tier silently dropped from the normal Core or Cloud scopes. - Confirm one tier failure cannot reuse its page or suppress a later tier result. - Confirm the Cloud manifest remains 87 packs, and its 102-test collection reflects the split test shape rather than lost pack coverage. - Confirm normal Cloud does not enable S14 or S15. Geometry recording is a separate `CN_GEOMETRY=record` operation that writes baselines and deliberately fails its recording step. - Confirm Cloud S9 baseline changes remain fail-closed for any outcome not explicitly supported by the current evidence. ## Validation - `vitest run scripts/playwright-cloud-trace.test.ts`: 25 passed. - Core and Cloud geometry-record selectors each collect exactly one S14 test. - Normal Cloud collection remains 102 tests with no S14 test. - Browser typecheck, scoped ESLint/Oxlint, YAML parsing, formatting, and `git diff --check` passed. - Fresh Principal Engineer, Senior QA, Senior Product, and adversarial reviews passed on the final recorder repair. ## Screenshots Not applicable. --------- Co-authored-by: Codex <noreply@openai.com> Co-authored-by: benjcooley <benjcooley@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
First full review of this PR from me. Reviewed at b4494ecb61, with deps installed, vitest and playwright --list run, and mutations used where a claim needed a control. All mutations reverted.
Reviewing it because it is the vehicle: #15200 and #15155 land into this branch, so this is what eventually reaches main.
The ten findings from Alex's review that survive the cloud cut are all still live
None has been addressed in the 254 commits since he reviewed at 8ad9a478. Details inline; the summary:
Finding 6 has moved backwards. The error-toast tolerance is now documented as intended at errorSurfaces.ts:42-47 and pinned by a passing test titled "a transient toast that clears within the window still passes". Neither playwright config sets an expect: block, so the window is Playwright's default 5000 ms per surface per call.
Findings 7 and 8 compose into a specific false PASS. A discarded drain timeout leaves work on the shared backend; each test gets a fresh page so __cnIdBase restarts at 0 and node ids repeat; the straggler's bare executing is then admitted into the next test's verdict. I reproduced both halves of 8 against the real classifyRun.
Finding 22 is one line. .pinact.yaml is byte-identical to main and its ignore list names actions/cache; pinact matches names exactly, and actions/cache/restore and actions/cache/save are different names. main uses neither subpath anywhere, so this PR is their first consumer.
Findings 11, 15, 16, 19 and 20 are verbatim unchanged. Finding 18's "currently red" half is fixed — the script suites are 62 passed / 0 failed — but its substantive claim is now larger, and I have a control for it inline.
New findings nobody has looked at
The highest-value one is on manifest.ts:373: the pack-ledger staleness rail does not hold for 5 of the 6 core packs, because it validates against the union of both environments' manifests. I proved it by dropping a pack from the core manifest and watching the rail still pass. The contract is asserted six times in this codebase.
The largest available deletion is on geometry.pure.spec.ts: 19 *.pure.spec.ts files, 4,721 lines, 191 tests, are collected into chromium and therefore into e2e-status — a required check on main — on every PR in the repo. They are vitest tests running under Playwright workers with retries: 3.
Also: the detection proofs never run on anything that gates; the tier split runs all six packs on one shared page; the three snapshot tiers are no-ops off CI so a red cannot be reproduced locally; and geometry baselines carry provenance that is written and never read while both sibling loaders validate theirs.
A correction to something I told Ben on #15200
I said the rebase was semantic because #15104 and #15057 would have made some of this scaffolding redundant. That is wrong. I grepped every file this branch adds or modifies for mockReset|clearAllMocks|resetAllMocks|createTestingPinia|setActivePinia: three hits, none of which count — a README, vite.config.mts itself, and a pre-existing test the branch adds ten lines to. This line adds essentially zero vitest suites under src/; its new vitest files are all under scripts/. The consolidations do not intersect it.
Merge state
The only conflict is pnpm-lock.yaml. Trial merge against origin/main: package.json, .oxfmtrc.json and pnpm-workspace.yaml all auto-merge, and I checked the semantic outcomes — main's i18n migration wins correctly and the branch's script and catalog additions all survive. Regenerate the lockfile and the merge is clean.
No required check has ever run on this PR. test, lint-and-format, e2e-status and website-e2e are all absent at head, because GitHub cannot build a merge commit while the PR conflicts. The core gate itself has not run at head either — CI: Tests Custom Nodes last succeeded at 56ea1d1d, the commit before #15155 merged, so the tier split has never had a green core gate.
The cloud gate is running right now. CI: Tests Custom Nodes Cloud started at 21:19Z on this head and was still in progress an hour and forty minutes later; the prior run was cancelled. That is ADR-001's exact symptom, occurring on the vehicle PR after the decision, holding the single-slot concurrency group against every other consumer. #15200 deletes it, but #15200 is a draft that conflicts against its own base.
Alignment with the settled direction
8,199 lines of cloud, canary and S15 surface are still in the diff. #15200 closes almost all of it, with one exception worth naming: the rules-over-snapshots decision is reflected in no code, in this PR or any of its satellites. What exists is 1.14 MB of geometry baselines across six files and 4,126 lines of interaction profiles. #15200 touches geometry.ts by +3/−10, removing only the cloud path. That is the largest gap between the agreed direction and the code.
Two smaller items: CLOUD HEAD survives as a live axis in docs/custom-node-regression-suite.md, and after #15200's cut src/config/firebaseConstants.ts becomes an orphaned two-line production module whose only remaining consumer is the file that was refactored to import it.
One production fix is hostage in here
useImageUploadWidget.ts:126-133 is a real user-facing bug fix — without the guard an unbound combo value makes the app request a file literally named "undefined" — with two proper vitest cases. Seven lines, independently landable today, currently blocked behind 73,273 lines and a conflicting merge. Worth splitting out.
Checked and sound, so nobody re-litigates
diffGeometry's two-way reconciliation and its ignored-path construction including the array-index branch; comparePackProfiles fail-closed on a missing baseline and two-way on both node sets; cannotRunAlone genuinely two-way in all three directions; the manifest covers every registered pack test closing the new-pack-lands-with-no-coverage hole; EXPECTED_TESTS as a real collected-count rail, confirmed at 33 with --list; and the attribution self-check's stimulus-landed positive control at customNode.regression.spec.ts:498, which refuses to trust its own PASS until it proves the injection happened. That last one is the right discipline — it is just pointed at the case the filter already handles rather than the one that bites.
Not reviewed
autoRun.ts (368 lines), typePairing.ts (298), connectivity.spec.ts (667) and dynamicInputs.spec.ts (284) beyond grep level. Those remain unread by anyone.
| return entries | ||
| } | ||
|
|
||
| export function loadAllManifestPackNames(): string[] { |
There was a problem hiding this comment.
issue: this rail does not hold for 5 of the 6 core packs, and I proved it.
loadAllManifestPackNames() concatenates core and cloud pack names unconditionally, 6 + 87 = 93.
assertPackLedgerKeys then case-folds and checks membership against that union. Five of the six core
packs also appear in the cloud manifest under lower-cased dirnames that fold to the same key, so
their ledger entries can never go stale.
Mutation, run against head:
baseline: 93 names, RAIL PASSED
drop ComfyUI-VideoHelperSuite from the core manifest (6 -> 5 rows):
92 names, RAIL PASSED (still)
GEOMETRY_UNSTABLE_PATHS['ComfyUI-VideoHelperSuite'] — 6 nodes, 20 individually-ignored geometry
fields — keeps passing the staleness check after its pack has left the core manifest entirely,
because comfyui-videohelpersuite is still row 40-something of the cloud one.
The contract is stated six times in this codebase as "an entry whose pack leaves the manifest reds".
Concrete consequence: a pin bump that drops or renames a core pack silently orphans its whole
exception ledger, which then suppresses assertions for a pack nobody is testing.
Fix is to key the rail to the active environment — loadManifest().map(e => e.pack), not the union.
Note this self-heals once the cloud half is gone, but #15200 will make this function throw if it is
not updated at the same time.
| ) | ||
| } | ||
|
|
||
| function readCoreManifest(): CoreManifestEntry[] { |
There was a problem hiding this comment.
issue: readCoreManifest is fail-open on an empty array; the cloud loader ten lines above is fail-closed.
assertCloudManifestShape explicitly rejects manifest.packs.length === 0 at :331. This has no
equivalent — JSON.parse(...) as CoreManifestEntry[] then entries.forEach(assertCoreEntry), which
is a no-op on [].
Mutation with the core manifest set to []:
core loadManifest() -> 0 entries, NO THROW
cloud (packs: []) -> THREW: ... is malformed (expected { coreDisabledNodes, packs, ... })
With zero entries every for (const entry of loadManifest()) registers no tests, and the five tier
tests pass vacuously over zero packs.
EXPECTED_TESTS in the workflow catches the count drop in CI, which contains it — but it does not
apply to pnpm test:custom-nodes locally, and it is skipped entirely when inputs.grep is set. One
length === 0 check here makes the two loaders symmetric.
| // keeps toHaveCount(0)'s tolerance - a transient surface that clears within | ||
| // the expect timeout still passes - while a persistent one fails with its | ||
| // visible text as the last polled value. | ||
| export async function expectNoVisibleErrors( |
There was a problem hiding this comment.
issue: this finding has moved backwards since Alex raised it.
The poll still retries until the surface list is empty, so an error toast that appears and clears
inside the window passes. What changed is that the behaviour is now documented as intended at
:42-47 — "a transient surface that clears within the expect timeout still passes" — and pinned by
a passing test at errorSurfaces.pure.spec.ts:50-60 titled "a transient toast that clears within
the window still passes".
Neither playwright config sets an expect: block, so the tolerance is Playwright's default 5000 ms,
per surface, per call. Five seconds is a long time for a user-visible error to be invisible to CI.
Christian's ruling on 2026-08-13 was to replace the mechanism rather than answer the
fail-versus-pass question: assert on the deterministic console/pageerror ledger, which this suite
already maintains, instead of polling the DOM for a transient. That dissolves the calibration
argument. It also repairs consoleErrorLedger.ts:466, which currently justifies filtering
PromptExecutionError on the grounds that "the visible error SURFACES are still asserted separately
by expectNoVisibleErrors" — i.e. it leans on this function being strict.
| // `executing` strings - stay, and graph membership still vets them); | ||
| // otherwise the legacy seen-set exclusion. | ||
| capturedPromptId !== undefined | ||
| ? event.prompt_id === undefined || event.prompt_id === capturedPromptId |
There was a problem hiding this comment.
issue: both directions of this reproduce. Control below, using the real classifyRun and toPromptEvent.
curated tier (graphNodeIds undefined), unscoped foreign error ->
{"outcome":"EXECUTION_ERROR", "error":{"exceptionType":"ForeignError","nodeId":"424242"}}
auto-run tier (graphNodeIds=[1,2]), bare straggler 'executing' ->
{"outcome":"PASS","executedNodes":["1","2"]}
False red. customNode.regression.spec.ts:276 calls runWorkflow without graphNodeIds, so the
graph-membership guard at runResult.ts:103-105 is inert and an unscoped execution_error naming a
node that is not in the graph reds the curated test and blames the pack.
False green. executedNodesFrom (runResult.ts:59-68) never consults graphNodeIds at all, so
a late bare executing for a recycled in-graph id inflates executedNodes and turns a genuine
PARTIAL into a PASS. Passing graphNodeIds does not help.
The self-check at :457 does not cover this: it injects a foreign event that carries a
prompt_id ('cn-foreign-self-check', :479), which is the case the filter already handles. It is
blind to the unscoped case, which is the one that bites.
This composes with the discarded drain timeout: work left running on the shared backend, each test
getting a fresh page so __cnIdBase restarts at 0 and node ids repeat, and the straggler's bare
executing admitted into the next test's verdict. False PASS, no symptom, from two separately
flagged defects neither of which is fixed.
| for (const id of running) await window.app!.api.interrupt(id) | ||
| for (const id of pending) await window.app!.api.deleteItem('queue', id) | ||
| }, owned) | ||
| if (Date.now() >= deadline) return 1 |
There was a problem hiding this comment.
issue: five of six teardown sites still discard this.
drainBackendToIdle returns 1 on timeout. Exactly one caller checks it —
allNodes.spec.ts:1965, inside the auto-run tier. The other six discard it: allNodes.spec.ts:373
and :720, connectivity.spec.ts:71, coreSmoke.spec.ts:36, customNode.regression.spec.ts:89,
interactionProfiles.spec.ts:207.
The comment at allNodes.spec.ts:370-372 justifies discarding on the grounds that "the auto-run
tier's 150s guard surfaces that". That guard is in a different test, on a different page, and does
not run for the S1/S2/S3/S14 tiers, connectivity, coreSmoke, regression, or interaction profiles.
See the ComfyTarget.ts thread for how this composes into a false PASS.
| // second needs declaring - the first is already in the manifest row - and both | ||
| // are skipped so the staleness guards do not force every ledger to fork per | ||
| // environment. Anything else absent still fires. | ||
| const PIN_SKEWED_LEDGER_NODES: Record<string, string> = { |
There was a problem hiding this comment.
issue: this is the one exception ledger in the suite with no staleness rail, and it sits inside the staleness checker.
Every other ledger here is registration-guarded — GEOMETRY_UNSTABLE_NODES and ..._PATHS at
allNodes.spec.ts:771-783, MOUNT_WIDGET_ALLOWLIST at :790, INTERACTION_UNSTABLE_NODES at
interactionProfiles.spec.ts:209, AUTO_RUN_UNSTABLE_NODES and AUTO_RUN_ALLOWED_FAILURES at
:1735-1757, cannotRunAlone two-way at :1773. PIN_SKEWED_LEDGER_NODES has none.
Its single entry hardcodes ComfyUI-KJNodes.ContextWindowsVisualizerKJ with the reason "the cloud
deployRef 377ed49f predates it". Nothing checks that 377ed49f is still the deployRef, that the node
still exists, or that the exemption is still needed. The moment the deployRef is bumped past
2026-06-17 this permanently suppresses the staleness check for that node — inside
stalenessCheckedKeys, the function every other ledger's rail routes through.
#15200 deletes this file, so the answer may be "delete". But stalenessCheckedKeys has seven callers
in allNodes.spec.ts plus one in the auto-run reconciliation, so whatever replaces it needs care.
| return `${geometryDir()}${pack}.json` | ||
| } | ||
|
|
||
| export function loadPackGeometry(pack: string): PackGeometryFile | null { |
There was a problem hiding this comment.
issue: geometry provenance is written and never read, unlike both sibling loaders.
This is Alex's body-level item that had no inline anchor. loadPackProfiles
(interactionProfiles.ts:89-100) validates schema !== 1 || !recordedAt?.core and throws;
loadOutputHashes (outputHashes.ts:168-174) does the same. loadPackGeometry is a bare
JSON.parse(...) as PackGeometryFile. Repo-wide, the geometry recordedAt is written at
allNodes.spec.ts:1039 and read by nothing.
There is already a UI for the failure. All six baselines carry
recordedAt.core = b08e6cf35fac50d3ca8470dffb3f9a1fbb7187d2, matching the gate's pin at
ci-tests-custom-nodes.yaml:166 — but :42 exposes a comfyui_ref dispatch input. Dispatch with
any other core SHA and S14 compares against a baseline recorded under a different ComfyUI, producing
reds nobody can attribute, while the run summary at :512 prints COMFYUI_REF_USED next to a
baseline whose provenance it never checked.
Validate schema and recordedAt like the siblings, and red when
recordedAt.core !== COMFYUI_REF_USED.
| throw new Error( | ||
| `geometry baselines recorded for ${entry.pack} - commit ${packGeometryRelativePath(entry.pack)} and re-run without CN_GEOMETRY` | ||
| ) | ||
| } else if (!process.env.CI) { |
There was a problem hiding this comment.
issue: all three snapshot tiers are no-ops off CI, so a red cannot be reproduced locally.
S14 here, S13 at interactionProfiles.spec.ts:231, S15 at customNode.regression.spec.ts:361 — all
gate their compare on process.env.CI and log-and-return otherwise. playwright.chrome.config.ts,
which every documented local script uses (test:custom-nodes, :watch, :debug, :local), sets no
CI. So a green pnpm test:custom-nodes:local is compatible with S13, S14 and S15 being entirely
broken, and the only way to see a geometry red is to dispatch a CI run.
This is worth connecting to the growing exception ledger. An engineer who cannot iterate locally on a
0.01px tolerance against a 1.14 MB baseline will ledger the node instead. GEOMETRY_UNSTABLE_PATHS
now relaxes 9 WAS nodes at the whole vue subtree — those nodes have zero Vue geometry coverage. The
ledger growth is a symptom of this as much as of the one-rAF sampling.
| const observed: Record<string, NodeInteractionProfile> = {} | ||
| for (let start = 0; start < plans.length; start += PROBE_CHUNK) { | ||
| const chunk = plans.slice(start, start + PROBE_CHUNK) | ||
| const chunkResults = await comfyPage.page.evaluate((probePlans) => { |
There was a problem hiding this comment.
issue: still one page.evaluate, so deferred pack init still never runs.
Create, connect-first, measure, connect-last, measure, disconnect, measure, remove — all inside the
single callback from :94 to :202. No yields, no phase split, no settle loop. Any pack that defers
its setup to a microtask or a timer has that work happen after every measurement, so the committed
baselines encode pre-init state and the tier cannot see the thing it exists to watch.
Unchanged since Alex raised it.
| (rule) => | ||
| rule.requiredConnectivityId !== undefined && | ||
| !errors.some((error) => ruleMatches(rule, error)) | ||
| ) |
There was a problem hiding this comment.
issue: this filter's justification depends on a mechanism that is documented to be tolerant.
isForeignExecutionNoise drops /PromptExecutionError/, /Prompt execution failed/ and
/Failed to load resource.*\/api\/prompt/ from the mount, persistence and wiring tiers, on the
grounds that those tiers queue nothing. The comment then says, verbatim, "This is not error
suppression: the visible error SURFACES (overlay/dialog/toast) are still asserted separately by
expectNoVisibleErrors."
But expectNoVisibleErrors passes any surface that clears within 5 s, and
errorSurfaces.ts:42-47 now documents that as intended.
The suite itself acknowledges packs queue unprompted — interactionProfiles.spec.ts:205-206, "the
guard for pack JS that queues behind our back while being probed". A pack that queues during the
mount tier and fails produces a PromptExecutionError that is filtered from the console assertion by
design and admitted by the toast assertion if the toast auto-dismisses. Both mechanisms that are
supposed to cover each other are open at once.
Making expectNoVisibleErrors fail-closed on first observation makes this justification true.
…Comfy-Org#15225) ## Summary Adds a local-backend custom-node E2E suite with two complementary populations. **Not a PR gate yet**: the workflow runs on a nightly schedule and manual dispatch only - PRs neither trigger it nor wait on any of its checks, and none of its checks are required in branch protection. Within a run it fails closed (an install failure or skipped tier is red). - **Core depth:** the original six pinned packs retain S1-S13 and S15. S14 geometry snapshots remain removed by team decision. - **Cloud breadth:** the pinned Cloud snapshot has 87 joined manifest rows; 83 run across five fixed shards on a local CPU ComfyUI backend. One source row is unjoined and four rows are explicitly quarantined. S15 is restored inside the six Core curated workflow tests that already execute. It adds output comparison, not another prompt or Playwright test, so its incremental runtime is negligible. ## Changes - Pins ComfyUI core, pack sources, registry artifacts, staged inputs, worker count, retry count, and shard composition. - Keeps each cloud pack in a stable shared Python environment; changing a quarantine entry cannot reshuffle other packs. - Runs Core as its own matrix entry and Cloud as five weight-balanced breadth shards. - Fails on any Playwright failure, skip, flaky result, count mismatch, dirty backend teardown, or stale exact expectation. - Records visible errors for the page lifetime, so a transient toast that clears before the assertion still fails S7. - Reports every coverage exclusion in bold in the GitHub Actions summary with its mechanism and removal condition. - Includes two bounded frontend fixes surfaced by the suite: unset image-upload combos no longer request filename=undefined, and empty audio-upload sentinels no longer request a preview. These are the only live src behavior changes in this PR. - Provides deliberate-break proofs for S1, S2, S3, S9, and S15. ## Tier coverage and applicability | Tier | Core | Cloud breadth | Assertion | |---|---|---|---| | S1 | 6 packs | rows declaring `load` | Every enrolled registered node instantiates in LiteGraph with exact declared slot materialization. | | S2 | 6 packs | rows declaring `load` | The enrolled S1 corpus mounts under Vue Nodes with its visible widgets and slots represented in the DOM. | | S3 | 6 packs | rows declaring `load` | Enrolled node identity and type, initialized live widget topology, and serialized widget values survive save/reload; exact pinned pack divergences are two-way ledgered. | | S4 | retained | retained | Representative type-correct connections among enrolled nodes survive graph connection and round-trip validation. | | S5 | retained | retained | Curated anchor links and one materialized in-pack link per applicable pack use real drag/connection APIs in both renderers, excluding only explicitly reported nodes. | | S6 | retained | retained | Connectivity round-trips reach prompt conversion and validate the serialized edge contract. | | S7 | retained, strengthened | retained, strengthened | All user-visible error surfaces are sampled every animation frame from initial navigation; transient and final-state errors fail. | | S8 | retained | retained | Console errors and uncaught page errors are collected across startup and operations; only exact attributed signatures are accepted. | | S9 | all Core `run` rows | VideoHelperSuite, the only Cloud `run` row | Exact calibrated model-free corpora queue against the real backend and must execute or produce an observable output. | | S10 | retained | retained | Manifest shape, exact local node counts, registered-pack attribution, and collection counts are sentinels. | | S11 | retained | retained where declared | Expected frontend extensions and served web-directory assets must register. | | S12 | Impact case | Impact case | Dynamic list input grows and shrinks through programmatic and real drag connections in both renderers. | | S13 | 6 pinned Core profiles | not enrolled | Existing Core interaction-delta profiles compare at the exact recorded pack refs. Cloud expansion is [FE-1659](https://linear.app/comfyorg/issue/FE-1659/define-scalable-s13-interaction-regression-coverage-beyond-core). | | S14 | removed | removed | Team-approved removal of full node geometry/position/size snapshots. | | S15 | 6 Core curated workflows | not enrolled | Deterministic sink payload hashes detect valid-but-wrong serialized output. Full-pack expansion is [FE-1657](https://linear.app/comfyorg/issue/FE-1657/extend-s15-output-regression-coverage-to-every-custom-node-pack). | The suite contains no `test.skip` or `test.fixme`, uses one worker and `--retries=0`, and independently rejects Playwright-reported skips or flaky results. ## Explicit coverage debt - `comfyui-fl-path-animator` does not join the pinned Cloud snapshot. - LivePortraitKJ has an unfetchable SHA and radiance has an unsatisfiable `Imath` requirement. Their upstream fixes are tracked by [FE-1660](https://linear.app/comfyorg/issue/FE-1660/fix-upstream-pack-metadata-and-remove-custom-node-e2e-quarantine). - SeedVR2 and NVIDIA RTX register zero nodes on a CPU runner. GPU-backed restoration is [FE-1658](https://linear.app/comfyorg/issue/FE-1658/add-gpu-backed-custom-node-e2e-coverage-and-remove-cpu-runner). - `comfyui-itools@0.6.8` is a banned registry artifact whose `iToolsCropImage` hook has two terminal race outcomes under the same pin. Only that node is excluded from S1-S8; the pack count remains exact and its other 21 nodes run. Restoration is [FE-1675](https://linear.app/comfyorg/issue/FE-1675/e2e-nodes-tests-fix-itools-crop-lifecycle-race-and-restore-s1-s8). - `VHS_SelectLatest` requires the pack-owned prompt transformation and is the one model-free node not executed by Cloud S9. Restoration is [FE-1661](https://linear.app/comfyorg/issue/FE-1661/restore-vhs_selectlatest-s9-execution-coverage). - `was-node-suite-comfyui/Text Random Prompt` performs an unbounded public Lexica API request and is excluded only from Core S9. Deterministic restoration is [FE-1682](https://linear.app/comfyorg/issue/FE-1682/e2e-nodes-tests-restore-was-text-random-prompt-s9-execution-coverage). - Exact known pack defects remain exercised under two-way stale ledgers; they are not skipped. A fixed or changed outcome fails until the expectation is removed or recalibrated with evidence. ## Review focus - Whether the two disclosed preview guards are correct and appropriately scoped; all remaining changes are tests, fixtures, scripts, tooling, documentation, or CI. - Whether every tier claim above matches its assertion and applicability. - Whether each temporary exclusion is specific, visible, owned, and removable. - Whether exact expectation ledgers describe attributable pack behavior without weakening the asserted contract. - Whether representative-per-slot connectivity plus per-pack two-renderer drags is the right bounded surface; this does not claim a producer-by-consumer cross-product or cross-shard pairing. - Whether the fixed-shard dependency environment and manifest provenance are sufficiently deterministic. Supersedes Comfy-Org#13389 and Comfy-Org#15200. Existing review follow-ups remain tracked in [FE-1611](https://linear.app/comfyorg/issue/FE-1611/custom-node-e2e-the-10-review-findings-that-survive-the-cloud-cut). --------- Co-authored-by: Nathaniel Parson Koroso <tetratrade@zoho.com> Co-authored-by: IAMtheIAM <iamtheiam@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: CodeJuggernaut <81205671+CodeJuggernaut@users.noreply.github.com> Co-authored-by: Codex <noreply@openai.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: github-actions <github-actions@github.com>
8 participants
Summary
Manifest-driven Playwright conformance suite for community custom-node packs, plus the two CI gates that run it:
custom-nodes-e2e-coreagainst a local pinned ComfyUI with every manifest pack pip-installed, andcustom-nodes-e2e-cloudagainst Comfy Cloud through avite previewof the cloud dist. There is no per-pack test code. A pack is one manifest row; the suite re-reads that pack's real node list from/object_infoon every run, derives what each node should be able to do, and verifies it in a real browser against a real backend with the pack's own frontend scripts loaded. Every exception is a reviewed ledger entry carrying its mechanism, and execution results are reconciled in both directions against a committed baseline, so the gate can neither hide a regression nor accumulate dead exemptions. Nothing is ever skipped: a skip fails the job.Architecture, scope, local runs, pack onboarding, and the detection proof live in the source-of-truth design doc: Custom Nodes E2E Regression Suite - Technical Design Doc.
docs/custom-node-regression-suite.mdis the repo stub that points at it.Core covers 6 pinned packs on a local backend; cloud covers 87 deployed packs. Scale snapshot from the doc, printed by a run and moving with the manifest and pins: about 800 registered nodes, about 5,000 planned wiring checks, about 440 nodes executing clean per run.
Coverage tiers, each one falsified against the core gate by applying its break in isolation and confirming the tier catches and names it. The proof is #13534 (DO NOT MERGE): one deliberate break per tier, with the CI runs showing each break caught and named; the per-row matrix is also summarized in the doc. All 15 legs verified red-when-broken:
S1 through S12 run in both environments, so a frontend regression is caught wherever it lands. S13 through S15 are core-only today: all three compare against committed baselines, and the cloud baselines come from the two record workflows (geometry, output hashes), neither of which has been run to a commit yet. All three defers are deliberate and self-expiring: S13 and S14 wait until every other cloud tier is green, S15 until cloud output hashes are recorded, and each tier arms itself the moment its baselines land.
Current state. Core gate green as of 226b0b2; the commits since touch shared files, so a core re-run at head is required before merge. Cloud gate: the auth rework has landed, since testcloud no longer accepts a raw Firebase token, so the fixture seeds a Firebase session and mints a real workspace JWT; a follow-up boot-ordering fix keeps
/api/featuresanonymous, so the router guard sees production's feature-flag payload instead of a prematureunified_cloud_auth=trueand its redirect/reload loop. The first full cloud run is in progress at time of writing.Changes
browser_tests/tests/customNodes/and the harness inbrowser_tests/fixtures/customNode/; gating workflowsci-tests-custom-nodes.yaml(core) andci-tests-custom-nodes-cloud.yaml(cloud); a nightly non-gating pack-drift canary; two baseline record workflows covering geometry and output hashes;scripts/gen-cloud-manifest.ts, which builds the cloud manifest from an/object_infoprobe snapshot joined to the deployed supported-nodes list, plus one small hand-maintained curated overlay that decides which rows carry the run tier.ComfyPagegains cloud smoke-auth seeding and makescreateUseridempotent on400 Duplicate username.;litegraphUtilsslot positions now go throughcanvasPosToClientPos, so mouse targeting survives pack JS that shifts the canvas off (0,0).start-server-and-testandyaml, both devDependencies.Review Focus
changesjob rather than a trigger-levelpaths:filter, so a required check never wedges Pending on an unrelated PR, and fork PRs skip the job (the core gate's manifest install loop runssetup.pyfrom cloned repos, and the cloud gate needs secrets forks never receive). Fork coverage stays with the main e2e shards.executingevents count as executed, so the core run tier requires a--cache-nonebackend.[cloud-auth-probe]console.warninstrumentation insrc/stores/authStore.tsandsrc/platform/workspace/stores/workspaceAuthStore.tsis temporary debugging for the cloud auth boot order and comes out before merge. It is the onlysrc/change in this PR beyond a test file.