fix(1478): graph_load reports the workflow identity it loaded into - #1225
Draft
artokun wants to merge 10 commits into
Draft
fix(1478): graph_load reports the workflow identity it loaded into#1225artokun wants to merge 10 commits into
artokun wants to merge 10 commits into
Conversation
Defect 2 shipped in 0.14.33. Defect 1 is only mitigated by a CONDITIONAL note, because the load reply carries nothing that separates a re-minted instance from a reused one. This branch adds the workflow_uuid the orchestrator's own docblock prescribes, so the load can state what happened or claim the fence outright. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
comfyui-mcp#1478 defect 1: `panel_load_workflow` returned loaded:true and the very next graph call failed with `workflow instance mismatch`, deterministically, twice. A blank-canvas load takes the fresh-mint path, so the session's fence still names the pre-load instance. The orchestrator could only answer with a CONDITIONAL note — "an API-format load CAN re-mint the instance; if your next command is refused, that is why" — because this reply carried nothing that separated re-minted from reused. Its own docblock names the fix: give the reply a workflow_uuid, as #762/#800 did for workflow_new and workflow_save, and the load can state what happened or claim the fence from it via the refreshFenceFromOwnReply path that already exists. WHY THE REPLY AND NOT A RE-DERIVATION. An earlier attempt at this defect ran the generic rebindWorkflowFence, and review caught the P1: it adopts whatever is ACTIVE NOW with no tie to the load, so a user switching canvases in the window would stamp the session to a different workflow and the next edit would land on the wrong graph. A uuid carried in the command's own reply has no such window. The two load paths stay exactly as they were. An in-place load keeps its instance on purpose (#570 P0b — re-minting there would reject the agent's own follow-up commands mid-conversation) and will simply match the fence; a blank-canvas load mints a new one and is the reporter's case. The field only REPORTS. Shape-gated per #716: only a canonical instance uuid is published, so a routing handle or half-established value is never adopted as an identity. The read is guarded — it runs after the graph has already landed, and a throw there would turn a successful load into a failed one, which is worse than the mismatch being fixed. Verified: 4307 tests pass; 6 mutations, all applied and all killed, against a checked-GREEN baseline. One of those mutations exposed a weak test. Asserting only that the identity is read AFTER the load passes just as happily when the read reuses the PRE-load capture — the same position, the stale value. That is the worst version of this field: the orchestrator would compare a stale fence against an equally stale reply, find them equal, and conclude the instance did not move on the very load where it did. The test now pins the fresh lookup, not just its position. The executor lives in the monolith and needs a live `app`, so what is pinned here is the wiring; the field's behaviour on a real load is verified against the running ComfyUI after merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ad's Review, P1: reading the live active workflow after the await is the same wrong-graph hazard the orchestrator-side attempt was rejected for. Load starts for A, the user switches to B while it awaits, the continuation reads B, and an orchestrator that CLAIMS the fence from this reply points the session at B — the next agent edit lands on the wrong graph. A uuid that might name another canvas is worse than none, because the field is trusted enough to claim from. The reply now names a workflow only when the live one IS the object this load targeted — object identity, immune to a switch. When that cannot be proven, the field is omitted and the orchestrator keeps its conditional note. Measured afterwards on ComfyUI 0.33.1 / frontend 1.48.7: an API load into an existing active workflow keeps the same workflow object, so the identity never moves on this build and this field publishes a uuid that matches the fence. That suppresses a false alarm; it is NOT the reporter's mismatch, which I could not reproduce. Recorded rather than claimed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he reply The object-identity gate is only worth anything if the comparison is still true when the value leaves. Insert a single `await` between the check and the reply and the guarantee silently inverts: the comparison passes for A, the event loop lets the user switch to B, and the reply — already blessed — ships A's uuid for what is now B's canvas. That is the embed/swap race this fix exists to avoid, and it would leave no trace in the source shape the other tests pin. The existing tests cover WHAT is compared. This one covers WHEN, by bounding the window from the identity read to the reply and refusing an await or a deferred continuation inside it. Verified by mutation: inserting `await Promise.resolve()` into that window fails this test and only this test (7 tests, 1 fail); reverting restores green. Refs artokun/comfyui-mcp#1478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rted nothing
graph_load has two return paths. The identity field landed only on the tail one,
so the branch the reporter was actually on -- `{loaded:true, format:"api",
node_count:59}` -- returned early and published nothing. The PR claimed the
opposite: that the field removes a false alarm "on every API load".
It also had the mechanism backwards. The earlier investigation measured
`afterIsSameObject: true` on an API load and concluded the instance never moves.
Object sameness is not identity sameness. `loadApiJson` reaches the
creation-boundary wrapper WITHOUT `__cmcpKeepInstance`, so the KEEP branch is
unreachable and `shouldForkInPlaceReload({cachedUuid, incomingUuid})` decides.
API/prompt JSON carries no `extra`, so `incomingUuid` is undefined, differs from
the fenced `cachedUuid`, and the wrapper deletes the cached uuid and mints a
fresh one ONTO THE SAME OBJECT. The object is preserved while its identity
moves -- which is exactly why the mismatch was deterministic.
So: capture the target before loadApiJson and report through the same
object-identity gate the UI path uses, now extracted as one shared reader
(`loadLandedWorkflowUuid`) rather than duplicated. The gate is unchanged --
publish only while the pre-load object is still live, shape-gated to a canonical
uuid, absent on failure.
Tests: the wiring is pinned on BOTH return paths, and the root-cause claim is
pinned behaviourally against the real `shouldForkInPlaceReload` predicate rather
than asserted in prose.
Also unpins a neighbouring test from a fixed 2200-char window over the API
branch: a comment added inside the branch pushed its last assertion out of scope
and failed a wiring test that was still satisfied. It now bounds on the branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…un/comfyui-mcp-panel into fix/1478-graph-load-publishes-uuid
…fetch Merging the branch's own race test caught a real defect in the API-path read. That test pins that nothing may run between the identity check and the reply that carries its verdict. The read was placed at the natural spot -- right after loadApiJson -- but the API reply also needs readPackImportFailures, so a live `await` sat between the check and the return. The comparison passes for A, the event loop lets the user switch to B, and the reply ships A's uuid for what is now B's canvas: exactly the inversion the gate exists to prevent. The read now happens last, immediately before the reply, on both paths. The test is adapted to the refactor rather than dropped: the gate now lives in one shared reader, so it pins that the reader is synchronous AND that neither call site awaits between asking it and returning. It covers both return paths, where before it could only see one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… 900 chars Second test in this file family to fail for a reason that was not a defect. The window from `const shortfall` had about forty characters of headroom left, so the next line added anywhere in the API reply -- a comment included -- pushed the `note:` and `packs_failed_to_import` assertions out of scope while the wiring they check was untouched. It now bounds on the end of the API branch, so it measures code rather than the length of the prose around it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs comfyui-mcp#1478 (defect 1). Draft. The earlier hold reasoning was wrong on a point of fact; this describes what is now established and what still is not.
What is implemented
graph_load's reply carriesworkflow_uuid— the field the orchestrator's own docblock asks for, as #762/#800 did forworkflow_new/workflow_save— on both of its return paths, sorefreshFenceFromOwnReplycan consume a value instead of the orchestrator appending a conditional note it cannot substantiate.The identity is read through one shared gate (
loadLandedWorkflowUuid): it names a workflow only when the live one is the very object that load targeted — object identity, captured before the load, immune to a tab switch during the await. Shape-gated to a canonical instance uuid (#716). When nothing is provable (a blank-canvas load mints an object this code holds no reference to), the field is omitted and the orchestrator keeps its existing fallback. The read is the last thing before each reply, so nothing can run between the check and the value it blesses.What this round corrected
The field was on the wrong branch.
graph_loadhas two returns. The identity landed only on the UI-format tail, but the reporter's call returned{loaded:true, format:"api", node_count:59}— the API-format branch, which returns early and never reached the new code. The previous body claimed the opposite: that the field lets the orchestrator "stop emitting a false-alarm note on every API load". It published nothing there.The mechanism was backwards. The earlier investigation measured
afterIsSameObject: trueon an API load and concluded the instance identity never moves. Object sameness is not identity sameness, and the panel's own creation-boundary wrapper is precisely what separates them:loadApiJsonreaches the wrapper without__cmcpKeepInstance, so the KEEP branch is unreachable andshouldForkInPlaceReload({cachedUuid, incomingUuid})decides.extra, soincomingUuidisundefined, which differs from the fencedcachedUuid.The workflow object is preserved while its identity moves. That is why the mismatch was deterministic, and why "the object did not change" was never evidence the fence survived — the fence keys on the uuid, not the object.
The in-place UI path is the contrast that proves it: it passes
__cmcpKeepInstance, so the instance is deliberately preserved (#570 P0b) and the reported uuid simply matches the fence. On that path the field is proof that nothing moved.A race the branch's own test caught. The API reply needs
readPackImportFailures, so placing the identity read at the natural spot — right afterloadApiJson— left a liveawaitbetween the check and the reply: the comparison passes for A, the event loop lets the user switch to B, and the reply, already blessed, ships A's uuid for what is now B's canvas. The read now happens last on both paths. That test was adapted to the refactor rather than dropped, and now covers both return paths where before it could only see one.What IS established
shouldForkInPlaceReloadpredicate rather than asserted in prose — a fenced workflow plus API JSON carrying no embedded uuid re-mints; the same uuid arriving back does not.What this does NOT establish
loadApiJsondispatches through the panel-patchedapp.loadGraphData(it callsthis.loadGraphData, so it should) rather than a captured reference. Confirming that on a live ComfyUI is what would turn "explains the report" into "reproduces it".graph_load— that is a change in the other repo. The panel half is what is here.panel_get_errorsrefusal under the dirty-mutation guard is a separate fix.So #1478 stays open, but the reason is now "the other half is not written", not "we cannot explain it".
Verification
npm run test:unit: 4409 tests, 4408 pass, 0 fail, 1 todo (the todo is pre-existing). Thecheck-panel-scopegate runs first and passes.8 mutations, applied against a checked-GREEN committed baseline, all 8 killed:
workflow_uuidfrom the API-format replyworkflow_uuidfrom the UI-format replyloadApiJsoninstead of beforeshouldForkInPlaceReloadso an API load does not re-mintawaitMutation 7 is the one that matters most: it recreates the world the earlier investigation believed it was in, and the root-cause test goes red.
Two neighbouring tests were unpinned from fixed character windows
Both asserted real wiring through a fixed-width slice of the API branch (2200 and 900 characters), and both failed because a comment was added inside that branch — not because the wiring changed. The 900-char one had roughly forty characters of headroom left on
main. Both now bound on the end of the branch, so they measure code rather than the length of the prose around it.One unrelated pre-existing flake
#671 verifyInstalled … when the queue never drainsis a wall-clock budget test in the ComfyUI-Manager area. It passes in isolation and on a cleanmaincheckout, and fails only when the full suite runs it under load. Untouched by this branch.