test: remove the live-cloud surface from the custom-node suite - #15200
test: remove the live-cloud surface from the custom-node suite#15200benjcooley wants to merge 1 commit into
Conversation
Per the landing plan: the cloud gate, nightly canary, geometry recorder, smoke auth, cloud manifests and ledgers, and the S15 output tier leave this PR. Every security finding in review 4922392902 lived in this surface; it returns as separate hardened PRs. The local hermetic suite is unchanged: cloud-conditional code paths keep their local arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
the
size:XXL
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
🎭 Playwright: ✅ 1918 passed, 0 failed · 3 flaky📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle Size
⚡ Performance Report
Absolute values
Raw data{
"timestamp": "2026-08-13T18:38:36.991Z",
"gitSha": "c09b9ee7d3d82cc5c677e848a045bab5d7c40907",
"branch": "benjcooley/e2e-core-landing",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2051.499000000007,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.872000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 468.337,
"heapDeltaBytes": 5935228,
"heapUsedBytes": 70320104,
"domNodes": 20,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 15.508,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-idle",
"durationMs": 2029.0169999999534,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.249999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 480.08700000000005,
"heapDeltaBytes": 5831912,
"heapUsedBytes": 70225496,
"domNodes": 18,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 17.748,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2097.889000000009,
"styleRecalcs": 80,
"styleRecalcDurationMs": 49.522000000000006,
"layouts": 12,
"layoutDurationMs": 4.335999999999999,
"taskDurationMs": 1084.6070000000002,
"heapDeltaBytes": -13875172,
"heapUsedBytes": 50339208,
"domNodes": -281,
"jsHeapTotalBytes": 24088576,
"scriptDurationMs": 130.161,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1939.9259999999572,
"styleRecalcs": 77,
"styleRecalcDurationMs": 43.662,
"layouts": 12,
"layoutDurationMs": 3.915,
"taskDurationMs": 954.0880000000001,
"heapDeltaBytes": -11189008,
"heapUsedBytes": 53300608,
"domNodes": -282,
"jsHeapTotalBytes": 24088576,
"scriptDurationMs": 132.14800000000002,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1738.8159999999857,
"styleRecalcs": 32,
"styleRecalcDurationMs": 18.066,
"layouts": 6,
"layoutDurationMs": 0.5910000000000002,
"taskDurationMs": 400.528,
"heapDeltaBytes": 8787192,
"heapUsedBytes": 73196124,
"domNodes": 78,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 23.073999999999998,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1751.2010000000373,
"styleRecalcs": 30,
"styleRecalcDurationMs": 18.090000000000003,
"layouts": 6,
"layoutDurationMs": 0.7279999999999999,
"taskDurationMs": 401.0779999999999,
"heapDeltaBytes": 8812704,
"heapUsedBytes": 73382556,
"domNodes": 75,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 22.377000000000002,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 614.6389999999826,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.794000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 394.13,
"heapDeltaBytes": -11203100,
"heapUsedBytes": 53162388,
"domNodes": 20,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 57.440000000000005,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 622.5869999999532,
"styleRecalcs": 11,
"styleRecalcDurationMs": 12.555,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 402.837,
"heapDeltaBytes": -11140796,
"heapUsedBytes": 53302688,
"domNodes": 18,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 58.549,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2032.7479999999696,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.108999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 679.535,
"heapDeltaBytes": 7210396,
"heapUsedBytes": 67394084,
"domNodes": -280,
"jsHeapTotalBytes": 3768320,
"scriptDurationMs": 95.97599999999998,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2061.423999999988,
"styleRecalcs": 8,
"styleRecalcDurationMs": 10.847000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 725.171,
"heapDeltaBytes": 3396752,
"heapUsedBytes": 63325776,
"domNodes": -285,
"jsHeapTotalBytes": 2719744,
"scriptDurationMs": 111.299,
"eventListeners": -179,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2160.4980000000182,
"styleRecalcs": 69,
"styleRecalcDurationMs": 15.796999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1262.155,
"heapDeltaBytes": 4877848,
"heapUsedBytes": 65445280,
"domNodes": -283,
"jsHeapTotalBytes": 3960832,
"scriptDurationMs": 407.514,
"eventListeners": -147,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2201.7190000000255,
"styleRecalcs": 69,
"styleRecalcDurationMs": 15.985000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1322.732,
"heapDeltaBytes": 1218688,
"heapUsedBytes": 62478264,
"domNodes": -285,
"jsHeapTotalBytes": 4222976,
"scriptDurationMs": 413.40900000000005,
"eventListeners": -179,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3248.2049999999845,
"styleRecalcs": 64,
"styleRecalcDurationMs": 20.658,
"layouts": 60,
"layoutDurationMs": 8.153,
"taskDurationMs": 1598.885,
"heapDeltaBytes": 1017700,
"heapUsedBytes": 63496492,
"domNodes": -290,
"jsHeapTotalBytes": 7962624,
"scriptDurationMs": 578.484,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3327.474999999936,
"styleRecalcs": 64,
"styleRecalcDurationMs": 15.865,
"layouts": 60,
"layoutDurationMs": 8.459000000000001,
"taskDurationMs": 1675.3369999999998,
"heapDeltaBytes": 4061868,
"heapUsedBytes": 66553240,
"domNodes": -289,
"jsHeapTotalBytes": 6389760,
"scriptDurationMs": 580.689,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "minimap-idle",
"durationMs": 2020.6919999999968,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.8599999999999985,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 693.489,
"heapDeltaBytes": 5968712,
"heapUsedBytes": 66599868,
"domNodes": -284,
"jsHeapTotalBytes": 4030464,
"scriptDurationMs": 99.274,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2028.2889999999725,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.725999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 740.888,
"heapDeltaBytes": 6567608,
"heapUsedBytes": 67230232,
"domNodes": -283,
"jsHeapTotalBytes": 4030464,
"scriptDurationMs": 113.766,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 621.3110000000484,
"styleRecalcs": 46,
"styleRecalcDurationMs": 13.721,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 441.435,
"heapDeltaBytes": -11064008,
"heapUsedBytes": 53481848,
"domNodes": 18,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 129.59500000000003,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 629.0669999999636,
"styleRecalcs": 47,
"styleRecalcDurationMs": 11.762000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 417.77799999999996,
"heapDeltaBytes": -10399396,
"heapUsedBytes": 54127020,
"domNodes": 20,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 125.43799999999999,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-idle",
"durationMs": 2009.0260000000058,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.372,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 490.29900000000004,
"heapDeltaBytes": 5819812,
"heapUsedBytes": 70418304,
"domNodes": 18,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 15.064000000000002,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2038.8269999999693,
"styleRecalcs": 10,
"styleRecalcDurationMs": 9.209999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 505.20699999999994,
"heapDeltaBytes": 5924292,
"heapUsedBytes": 70400952,
"domNodes": 20,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 17.994999999999997,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1709.14300000004,
"styleRecalcs": 76,
"styleRecalcDurationMs": 36.452,
"layouts": 16,
"layoutDurationMs": 4.427,
"taskDurationMs": 816.294,
"heapDeltaBytes": -18538592,
"heapUsedBytes": 46133720,
"domNodes": 0,
"jsHeapTotalBytes": 23040000,
"scriptDurationMs": 92.086,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1703.659000000016,
"styleRecalcs": 76,
"styleRecalcDurationMs": 38.537,
"layouts": 16,
"layoutDurationMs": 4.7989999999999995,
"taskDurationMs": 786.923,
"heapDeltaBytes": -3408360,
"heapUsedBytes": 61005840,
"domNodes": 63,
"jsHeapTotalBytes": 24903680,
"scriptDurationMs": 94.689,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-transition-enter",
"durationMs": 965.1080000000434,
"styleRecalcs": 19,
"styleRecalcDurationMs": 28.642,
"layouts": 15,
"layoutDurationMs": 11.553999999999998,
"taskDurationMs": 743.8899999999998,
"heapDeltaBytes": 4695432,
"heapUsedBytes": 95205568,
"domNodes": 13673,
"jsHeapTotalBytes": 16252928,
"scriptDurationMs": 28.911000000000005,
"eventListeners": 2375,
"totalBlockingTimeMs": 121,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "viewport-pan-sweep",
"durationMs": 8331.219999999974,
"styleRecalcs": 250,
"styleRecalcDurationMs": 41.666,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4438.503,
"heapDeltaBytes": 5577624,
"heapUsedBytes": 65110384,
"domNodes": -281,
"jsHeapTotalBytes": 6320128,
"scriptDurationMs": 1334.9869999999999,
"eventListeners": -163,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8394.520000000057,
"styleRecalcs": 250,
"styleRecalcDurationMs": 43.51199999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4659.6230000000005,
"heapDeltaBytes": 11363564,
"heapUsedBytes": 71416288,
"domNodes": -283,
"jsHeapTotalBytes": 5533696,
"scriptDurationMs": 1376.043,
"eventListeners": -131,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.80000000000109
},
{
"name": "vue-large-graph-idle",
"durationMs": 17619.781999999985,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17590.989,
"heapDeltaBytes": -57962472,
"heapUsedBytes": 166857912,
"domNodes": -8312,
"jsHeapTotalBytes": -8855552,
"scriptDurationMs": 547.559,
"eventListeners": -16389,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 18142.33200000001,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 18122.661,
"heapDeltaBytes": -46082980,
"heapUsedBytes": 159953360,
"domNodes": -8312,
"jsHeapTotalBytes": -8593408,
"scriptDurationMs": 560.941,
"eventListeners": -16387,
"totalBlockingTimeMs": 0,
"frameDurationMs": 18.333333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 21618.133,
"styleRecalcs": 152,
"styleRecalcDurationMs": 20.484999999999975,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 21561.687,
"heapDeltaBytes": -37614336,
"heapUsedBytes": 169024372,
"domNodes": -8312,
"jsHeapTotalBytes": -7544832,
"scriptDurationMs": 890.8810000000001,
"eventListeners": -16383,
"totalBlockingTimeMs": 524,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 23325.83999999997,
"styleRecalcs": 175,
"styleRecalcDurationMs": 27.444999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 23288.488999999998,
"heapDeltaBytes": -40256768,
"heapUsedBytes": 168130440,
"domNodes": -8312,
"jsHeapTotalBytes": -10690560,
"scriptDurationMs": 991.771,
"eventListeners": -16381,
"totalBlockingTimeMs": 841,
"frameDurationMs": 17.77333333333336,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 445.8970000000022,
"styleRecalcs": 12,
"styleRecalcDurationMs": 17.994999999999997,
"layouts": 3,
"layoutDurationMs": 0.714,
"taskDurationMs": 110.686,
"heapDeltaBytes": 5054288,
"heapUsedBytes": 68499128,
"domNodes": 124,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 9.321000000000002,
"eventListeners": 99,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 447.7520000000368,
"styleRecalcs": 13,
"styleRecalcDurationMs": 17.661,
"layouts": 3,
"layoutDurationMs": 0.5270000000000001,
"taskDurationMs": 104.28499999999998,
"heapDeltaBytes": 5035892,
"heapUsedBytes": 68616164,
"domNodes": 123,
"jsHeapTotalBytes": 4980736,
"scriptDurationMs": 8.279,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
}
]
} |
🌐 Website E2ETip All tests passed.
|
github-actions
Bot
added
the
risk:R3
|
Sequencing note: tier isolation + sharding (#15155 line) is landing on the base branch first, and it restructures several files this cut deletes or untangles - so this PR will be re-executed on the new tip rather than rebased through the conflicts. Treat this revision as the reference for scope and method, not as reviewable-final:
Will re-run the same procedure once the base settles. |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## nathaniel/custom-node-e2e-suite #15200 +/- ##
==================================================================
Coverage ? 81.36%
==================================================================
Files ? 1882
Lines ? 106702
Branches ? 29259
==================================================================
Hits ? 86818
Misses ? 19533
Partials ? 351
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Endorsing the cut. The numbers carry it on their own: cloud gate 41% failure, 52% of runs cancelled by the single-slot concurrency group, p90 160 min, one run at 22.5 h, against 28.6% and p90 40 min for core. And every security finding in the review lives in this surface.
Worth noting Comfy-Org/cloud#6486 merged 08-13 and fixes the 500 Failed to create job record that was the S9 blocker. It does not reopen the decision, because the backend errors were never the cancellation or runtime problem, but it is the reason not to describe cloud as simply broken.
Three things before this comes out of draft.
Say which removals are cloud and which are not
The title and body read as "remove the live-cloud surface", but the diff also removes two things that are not cloud:
- all of S15, including its core half
- the entire nightly canary, whose jobs A and B are local-backend
Both are, I think, the right call. But someone reading this in three months should not have to diff it to discover that.
On the canary specifically, the reasoning is worth writing down because the obvious objection is "that was our only drift detector". It was, but pinning already does the job: packs are pinned by SHA in the manifest and ComfyUI is pinned by container tag, so a red is ours by construction. What canary-pack-drift actually answered was are the pins stale, and that is cheaper at bump time — run the suite once at old-core plus new-pack when bumping a pin. Recommend adding that bump-time rule wherever pin bumps are documented, so the practice does not just evaporate with the job.
On S15, the mechanism was the problem rather than the goal. It routed a frontend serialization assertion through backend execution and PNG encoding, which is exactly why it hashed compressed bytes instead of decoded pixels and checked non-PNG outputs by extension. The same regression class — a valid but wrong prompt — is catchable with a prompt snapshot per curated workflow: no execution, no encoder dependency, and it subsumes S6's invalid-prompt case too. Suggest naming that as the replacement rather than "S15 returns later", since the latter invites rebuilding the output-hash version.
Release Velocity B4's DoD said "detection proofs green across all 15 test surfaces", which this made unmeetable. I have amended it to S1–S14 with the S15 rationale recorded inline, so that is handled.
validate-pins is red
actions/cache/restore@v5 and actions/cache/save@v5 unpinned — review finding 22, one-line fix, and it is the only red on the PR right now. Run 31730735487.
Sequencing against #15155
#15155 overlaps this on 12 files, four of which this PR deletes, and the two disagree on the collected test count (34 here, 33 there) and on playwright-cloud-trace.test.ts — #15155 extends it (+109/−8), this deletes it, and finding 18 says it is a change-detector that is currently red.
Worth resolving deliberately rather than by merge order. The per-tier attribution in #15155 looks worth keeping on its own merits, and the timing objection to it has evidence against it: the deconflated run adds trivial wall-clock (31732657230 vs 31733903136), and the 34→33 change is structural regrouping, not lost coverage.
Two notes
The ten review findings that survive this cut are now tracked at https://linear.app/comfyorg/issue/FE-1611/custom-node-e2e-the-10-review-findings-that-survive-the-cloud-cut — they were only in #13389's threads, and if that PR gets closed in favour of this one the threads go with it. Four of them are the "can this test actually fail?" set, all local-backend.
The rebase is semantic rather than textual. The two main commits touching these files are #15104 (removed 249 redundant mockReset/mockClear/vi.clearAllMocks across 120 files) and #15057 (createTestingPinia by default, removed 207 per-suite activations). Test scaffolding written before those may want deleting rather than merging, and Alex wrote both.
|
Correcting myself from the review above. I said the rebase was semantic because #15104 and #15057 (the Structural reason: this line adds essentially zero vitest suites under There is a large deletion available, just a different one. 19 They are pure unit tests — Detail is in my review on #13389, since that is where the files live. |
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Architectural pass on the draft, following up my earlier review. Ran the gates at head 0325ddd286; all results below are from the unmerged PR head, not the merged tree.
The cut is clean, and that is worth saying first
pnpm typecheck exit 0. pnpm oxlint --type-aware exit 0, no warnings in touched files. pnpm knip exit 0. custom-nodes-e2e-core passed on this exact head in 16m4s. For a −11,186 diff, deletion mechanics are the part everyone assumes is broken, and here they are not.
Three things I checked at symbol level rather than by grep, so I will state them as facts:
- Zero dangling references. Every export of every deleted module has zero references at head, including
stalenessCheckedKeys,uploadRunMedia,hashSinkPayloads,compareOutputHashes,seedFirebaseAuthUser,seedSmokeAuth, and the removed manifest helpers. The orphan-export set at head is a strict subset of base's, so nothing is newly stranded. EXPECTED_TESTS: 34is right by proof, not measurement.allNodes.spec.tsloses two declarations, but both sit in theunjoined yaml packsblock gated onunjoinedYamlPacks.length > 0, and basemanifest.ts:389returns[]unless the env is cloud. They were never collected on the core gate. The 5 deleted*.pure.spec.tscarry no@custom-nodestag, so they were never in the project either.- The emptied ledgers are correctly empty. All 3
AUTO_RUN_UNSTABLE_NODESentries and all 6 removedROUNDTRIP_VALUE_ALLOWLISTblocks were keyed by pack ids absent from the core manifest, sopackLedgerFor()already returned{}.
The stalenessCheckedKeys(entry, X) → Object.keys(X) substitution at four sites looks risky and is not: a CoreManifestEntry has no disabledNodes, so disabled = {} and pinSkewed is false, and the deleted function returned Object.keys(ledger) verbatim. Behaviour-preserving for core, exactly.
The rebase is the real risk, and 13 conflicts understates it
allNodes.spec.ts is the problem. #15155 structurally rewrote it into per-tier runners with a fresh page per tier; this PR's ~15 edits were written against the pre-split combined test. Git reports 3 hunks there and silently auto-merges the rest.
Take --theirs wholesale on that file and re-apply the cut by hand. This inverts the usual rule about whole-file resolution dropping code, and it is right here because the sides are asymmetric: #15155's change cannot be re-derived, while this PR's is a mechanical, fully enumerable list — drop the cloudExclusions import block; drop four helpers from the manifest import; AUTO_RUN_UNSTABLE_NODES → {}; drop 6 cloud packs from ROUNDTRIP_VALUE_ALLOWLIST; loadAllManifestPackNames() → loadManifest().map(e => e.pack); delete the disabledHarness const and the unjoined-yaml describe; stalenessCheckedKeys → Object.keys at 4 sites; drop the customNodesEnv() !== 'cloud' timeout guards; 'pin' in entry ? … : entry.deployRef → entry.pin; fix the gen:cloud-manifest reference in the uncovered-pack error.
The resolution is forced, not a preference. The merged record-custom-nodes-geometry.yaml:116 greps "all nodes by tier @custom-nodes.*S14:", a title that exists only in #15155's split file. Resolving toward the pre-split structure makes the recorder match zero tests and if-no-files-found: error fires.
Rest of the table:
| File | Take | Why |
|---|---|---|
the 5 cloud workflows / manifests / cloudCannotRunAlone.json |
ours, delete | ADR-001. Also closes #15163, whose only file this is. |
scripts/playwright-cloud-trace.test.ts |
ours, delete | below |
firebaseAuthStorage.ts |
theirs, KEEP — reverse the deletion | see the firebase.ts thread |
ci-tests-custom-nodes.yaml |
theirs, minus the two S15 lines | #15155's expression branches only on enable_s14, so it is already S15-agnostic |
autoRun.ts, autoRun.pure.spec.ts, manifest.pure.spec.ts |
ours | #15155's additions here are all cloud-pack entries and cloud-manifest assertions; they would fail against the pruned ledger |
custom-nodes-summary.py |
theirs, minus S15 and the env indirection | keep the tier buckets and [tier-pack] parsing; the CUSTOM_NODES_RESULTS_FILE/_LOG_FILE/_SUMMARY_TITLE indirection existed only so the cloud gate could reuse the script |
custom-nodes-summary.test.ts |
keep, drop its Cloud case |
:18-25 parameterises a config with no producer after the cut |
record-custom-nodes-geometry.yaml, the 4 detection-proof patches, firebaseConstants.ts |
keep |
Re-derive the count after resolving: base 34, #15155 33, this PR 34. It removes zero collected tests, so 34 is right today and 33 is right after the split.
On deleting playwright-cloud-trace.test.ts
Worth framing as principle rather than convenience. Two of its six tests assert on cloud config branches; one parses a workflow file that will not exist; the remaining three are change-detectors on source text. #15155's new test at :279 string-matches five literal source fragments including 'test.setTimeout(1_620_000 * loadManifest().length)'. That is restating the code, which is the thing "rules over snapshots" is against.
One genuine loss to name: :239 is the only thing keeping CN_ENABLE_S14, EXPECTED_TESTS and S14_ENABLED from drifting apart. The replacement is not a new YAML-parsing test — it is to stop hardcoding EXPECTED_TESTS and derive it, which deletes the drift class instead of asserting about it.
The required-check plan is aimed at the wrong population
This is the most useful thing I have for you. All 292 runs of workflow 306306832 are on 27 branches of this programme — 170 on nathaniel/custom-node-e2e-suite, 54 on nathaniel/detection-proof, 24 on nathaniel/custom-node-tier-isolation, the rest benjcooley/*, verify/*, bisect/*. Zero runs on an unrelated PR.
So 28.6% is the failure rate of a suite while it was being written, measured against itself. It is neither evidence for nor against promotion, and a burn-in week on these same branches reproduces it rather than answers it. Burn-in is a sampling requirement, not a waiting period: the gate needs runs on PRs that are not about the gate.
The bar is also higher than intuition suggests. ProtectMain sets grouping_strategy: ALLGREEN, max_entries_to_build: 5. At per-run failure rate p, a 5-entry group fails with probability 1 − (1−p)⁵ — so 28.6% gives 81.5%, 10% gives 41%, 5% gives 22.6%. Holding group failure under 10% needs p < 2.1%. Nothing measured is within an order of magnitude, and nothing measured is the right sample.
Residue
Four one-liners, and one of them is actively misleading:
connectivity.spec.ts:271— a live assertion message pointing a future maintainer at the deleted canary: "or (on floating canary defs) the pair plan reshuffled under core/pack drift". The gate is fully pinned now. This is the only survivingcanarymention in the repo.interactionProfiles.ts:6— the only survivingS15token in the repo..oxfmtrc.json:14— still listsbrowser_tests/fixtures/data/cloud/supported_nodes.yamlinignorePatternsfor a directory that is gone. oxfmt treats these as globs so a non-matching pattern is a silent no-op andformat:checkstays green. Worth noting this is the only surviving reference to any of the 26 deleted paths after a full path/basename/stem sweep — that is a good result.package.json:225—"yaml": "catalog:"is orphaned; its only three importers are all deleted. Cleanup nit, not a CI blocker: I had this reported as a knip failure and checked it rather than reasoning about it —pnpm knipandpnpm exec knip --dependenciesboth exit 0.
Also ~6 comment and test-name mentions not worth blocking on. One structural leftover with the same flavour: the case-insensitive pack-id folding at consoleErrorLedger.ts:130 and packLedger.ts:7 is justified by a comment saying "one entry covers both targets" — both targets was core plus cloud, and the lowercase dirnames came from the cloud manifest. Cheap and defensive, so keeping it is fine; the comment is now false.
Two downstream items
#13534's proof matrix includes an S15 row that this PR orphans with no replacement. #15155 already absorbed rows 01/02/03/14 and row 9 as inline perl. Its fate as a standalone PR needs a call either way. (Low confidence on its current row inventory — I confirmed the four patches present on the base branch and that no row-15 exists there, but did not read #13534's branch.)
The commit trailer. 0325ddd286 carries Co-Authored-By: Claude Fable 5. There is no check-ai-co-authors workflow in this repo, so it will not fail CI — but AGENTS.md says "Never mention Claude/AI in commits", and the trailer survives squash-merge.
What I could not verify
The .pure.spec.ts files are Playwright, not vitest — they use comfyPageFixture and need a served frontend. I confirmed collection at 116 in 14 files, matching the body, but did not execute them; the body's "116 passed" is unverified by me. I also did not typecheck or lint the merged tree, because resolving it means making the calls this review recommends. Findings about the merge queue and the changes-job gate are reasoned from the YAML and the rulesets API, not observed.
|
|
||
| const DEV_CONFIG: FirebaseOptions = { | ||
| apiKey: DEV_FIREBASE_WEB_API_KEY, | ||
| apiKey: 'AIzaSyDa_YMeyzV0SkVe92vBZ1tVikWBmOU5KVE', |
There was a problem hiding this comment.
issue: two problems meet on this line, and one of them the rebase will produce silently.
The cut is over-broad here. browser_tests/fixtures/helpers/firebaseAuthStorage.ts had three
consumers at base: smokeAuth.ts and smokeAuth.pure.spec.ts (cloud custom-node, correctly going)
and CloudAuthHelper.ts, which belongs to the pre-existing @cloud Playwright project, not to
this suite. Deleting the shared module reverts CloudAuthHelper.ts to main's version and
re-hardcodes the key in two places. If they drift, the @cloud project boots signed-out.
Net against main is zero, which is why "reverted to main's versions" reads as safe. Net against
base it discards a real improvement to a suite outside this cut. The commit that introduced the
shared export said so: "exported so test fixtures seed Firebase storage under the same key the SDK
derives its lookup keys from, instead of duplicating the literal."
And #15155 fixed the same thing properly, by adding src/config/firebaseConstants.ts. So on
rebase, keep #15155's version — it costs nothing and is strictly better.
Watch the auto-merge. Git reports no conflict on this file and produces something broken:
import { DEV_FIREBASE_WEB_API_KEY } from './firebaseConstants' // never used
...
apiKey: 'AIzaSyDa_YMeyzV0SkVe92vBZ1tVikWBmOU5KVE',Lint catches it, so it is not dangerous. It is the tell that this rebase has semantic conflicts git
cannot see, and the reason to review the clean merges too, not just the 13 marked ones.
| # ignores the pin fields in its own install step | ||
| # (ci-nightly-custom-nodes-canary.yaml); the manifest itself | ||
| # stays pinned and validating everywhere. | ||
| # mandatory here, before anything installs. |
There was a problem hiding this comment.
suggestion: four things about this workflow, in the order they will bite once it is a required check.
validate-pins is red, in two lines this PR keeps. :123 actions/cache/restore@v5 and :211
actions/cache/save@v5. .pinact.yaml:12-26 allowlists actions/cache at v5, but
actions/cache/restore and actions/cache/save are distinct names not covered by it. Worth saying
these are pre-existing at base — the PR inherits the red only because ci-validate-action-pins
triggers on paths: .github/workflows/**. Not sloppiness here, but a promotion to required cannot
land with it red. Two more ignore_actions entries is more consistent with how actions/cache is
already handled than SHA-pinning just these two.
No timeout-minutes on the gate at all, so it runs under GitHub's 360-minute default. The
geometry recorder gets this right at record-custom-nodes-geometry.yaml:29. Over the 292 measured
runs: p50 15.8, p90 31.4, p95 45.3, max 80.9 min — and 4 runs already exceed the merge queue's
60-minute check_response_timeout_minutes. Under ALLGREEN that does not fail one entry, it
invalidates the group. timeout-minutes: 50 sits above p95 and below the queue timeout.
EXPECTED_TESTS: 34 hardcoded at :259 is a merge-order hazard once required. Two PRs each
adding one @custom-nodes test both pass in isolation and the second reds on merge. Deriving the
count from --list, or asserting >=, deletes the class — and removes the last thing
scripts/playwright-cloud-trace.test.ts was still earning. (34 is correct today; this is about the
mechanism.)
A changes-job failure turns the gate green. With no terminal aggregator the required context
has to be custom-nodes-e2e-core itself, and a skipped-for-failed-dependency job reads as passing
to branch protection. Repo-wide pattern rather than a new sin — ci-tests-e2e.yaml:215 has the same
hole — but adding a fifth required check that inherits it is the moment to add an if: always()
status job asserting needs.changes.result == 'success'.
Separately, on the fork exclusion at :77-80: skipping fork PRs is right, since the job clones and
pip-installs whatever the manifest points at and setup.py runs at install time. But on a merge
queue entry github.event_name is merge_group, the first disjunct is true, and the job runs the
fork's merged code with the same clone-and-install against the shared cn-packs-v2-* cache. An
external contributor sees green on the PR and a mystery dequeue at merge. Either accept that
explicitly, handle merge_group, or put a same-origin allowlist on repo rather than only a
SHA-shape check on pin.
| // auto-run tier still waits on the WHOLE queue to go quiet before it | ||
| // measures (waitForQueueQuiet, customNodeSuite.ts), which a parallel | ||
| // shard cannot provide. | ||
| // parallel main e2e shards: the auto-run tier waits on the WHOLE queue |
There was a problem hiding this comment.
note: this project's grep is why 116 unit tests ride the required e2e gate.
custom-nodes greps /@custom-nodes/ and chromium grep-inverts it. The *.pure.spec.ts files
carry no tag, so they land in chromium and therefore in e2e-status, a required check on main,
on every PR in the repo. At this head:
--project=custom-nodes --list -> 34 tests in 6 files
--project=chromium --list -> 116 tests in 14 files
Not introduced by this PR — the count is down from 191 because the cut removed 5 of the 19 files —
so this is a note rather than a finding here. Raised properly on #13389 where the files live. Worth
knowing while sizing the required-check question, because those 116 inherit retries: 3 and are
pure functions being tested through a browser worker.
| for (const widget of blip.widgets.slice(2)) widget.dy -= 38 | ||
| blip.h -= 38 | ||
| expect(diffGeometry(baseline.nodes, measured, ledger)).toEqual([]) | ||
| const baseline = loadPackGeometry('was-node-suite-comfyui')! |
There was a problem hiding this comment.
note: the cut removes the smallest snapshot surface and keeps the two largest.
curatedOutputHashes.core.json (28 lines) goes with S15. What stays: geometry/*.json at 1.14 MB
across six files, KJNodes alone 384 KB, plus 93 KB of interaction profiles — with S13 and S14 both
still active in the gate.
Entirely consistent with this PR's scope, and not a criticism of the diff. Flagging because the
Landing Plan frames the direction as rules replacing snapshots, and this PR moves that needle by
roughly 0.2%. Worth not letting the framing outrun the code, since the rules-based replacement
currently exists in no PR.
This file is also still the change-detector Alex flagged (finding 19) — :15-56 restates every
ledger key and field path as toEqual([...]).
…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>
2 participants
Removes every live-cloud execution surface from the custom-node suite so the local hermetic suite can land on its own. All security findings in review 4922392902 live in this surface; per the landing plan it returns as separate hardened PRs.
What was cut
ci-tests-custom-nodes-cloud.yaml(cloud gate),ci-nightly-custom-nodes-canary.yaml(nightly canary),record-custom-nodes-geometry-cloud.yaml(cloud geometry recorder), plus the S15 output-hash steps andCN_ENABLE_S15/enable_s15wiring in the surviving core workflows.smokeAuth.ts,firebaseAuthStorage.ts,cloudExclusions.ts,cloudMedia.ts,outputHashes.ts, the cloud boot guard and Cloud page trace inComfyPage.ts/customNodeSuite.ts,customNodesEnv()and everyCUSTOM_NODES_ENV=cloudarm.cloud-manifest.ts,gen-cloud-manifest.tsand their tests/fixtures,playwright-cloud-trace.test.ts.customNodeManifest.cloud.json,data/cloud/*(supported_nodes.yaml, cloudCannotRunAlone, cloudExtensionSentinels, curatedCloudWorkflows),curatedOutputHashes.core.json, the cloud-pack rows in the shared value-drift/auto-run/console/connectivity ledgers, and the S15 tier incustomNode.regression.spec.ts.smokeAuth,cloudMedia,cloudExclusions,outputHashes,connectivityExpectations) and the cloud sections of the surviving pure specs.What stayed
CloudAuthHelper.tsandvite.config.mtsare reverted to main's versions; the@cloud-tagged mock-workspace tests are untouched.Validation
pnpm typecheckandpnpm typecheck:browser: 0 errorspnpm lint,pnpm knip,pnpm format:check: pass--project=chromium): 116 passed--project=custom-nodes --list: 34 tests in 6 files (matches the gate's expected count)🤖 Generated with Claude Code