From 6da378442b3beb60ec46a05447a73c4d4544a4ea Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Wed, 21 Oct 2026 10:25:48 -0700 Subject: [PATCH] fix(inspector): check only the negative the legacy parse read for a zero-out Review I1/M2/M3/M5 of the zero-out legacy-fill guard. The guard decided over every node with a `negative` input, so a graph whose read sampler encodes "blurry" lost that legacy-filled negative when another branch zeroed its own (fewer than the legacy pages: below the parity floor). - comfy-metadata.js gains `legacyNegativeSource`, which returns the graph and sampler `negative` link that `extractGenerationParams` reads from. It reuses the parser's own sampler pick (`selectSampler`, factored out of `fromPromptGraph` without changing any reading). The workspace checks only that link: a direct zero-out shows the zeroed empty negative, a zero-out further up the path leaves it unfilled, and anything else keeps the legacy fill. - Workflow-only files go through the parser's own workflow conversion, so bypassed (mode 4) and muted (mode 2) zero-outs and unconnected ones behave as the legacy reading does. A subgraph instance whose definition runs a zero-out (directly or nested) leaves the negative unfilled. - One bounded walk with an index pointer replaces a walk per negative input with queue.shift(). - An ambiguous fact that the legacy parse knows (a grid's per-branch model) is still filled, documented: the legacy pages show it and this page never shows less. Tests: the reversed mixed graph shows "blurry"; the mixed graph whose read sampler is zeroed, the zero-out-only grid and a direct workflow zero-out show the zeroed empty negative; a combined zero-out and a zero-out subgraph instance stay unfilled; bypassed and unconnected workflow zero-outs keep the legacy reading. An API-graph Reroute in front of a zero-out gives the legacy parse no negative, so the server answer stays (that expectation changed from zeroed). --- tests/js/inspection-semantics-sync.test.js | 2 +- tests/js/workspace-inspection-state.test.js | 97 ++++++++++++++++----- web/js/comfy-metadata.js | 33 ++++++- web/js/workspace/inspection-state.mjs | 59 ++++++++----- 4 files changed, 141 insertions(+), 50 deletions(-) diff --git a/tests/js/inspection-semantics-sync.test.js b/tests/js/inspection-semantics-sync.test.js index bbbbd38..bd9f094 100644 --- a/tests/js/inspection-semantics-sync.test.js +++ b/tests/js/inspection-semantics-sync.test.js @@ -30,7 +30,7 @@ test("facts guider providers are guiders in the semantics table", () => { }); test("the facts tables are not part of the legacy page contract", () => { - assert.deepStrictEqual(Object.keys(metadata).filter(key => !key.startsWith("_")).sort(), ["PLACEHOLDERS", "extractGenerationFacts", "extractGenerationParams"]); + assert.deepStrictEqual(Object.keys(metadata).filter(key => !key.startsWith("_")).sort(), ["PLACEHOLDERS", "extractGenerationFacts", "extractGenerationParams", "legacyNegativeSource"]); assert.ok(Object.isFrozen(metadata._factsTables)); }); diff --git a/tests/js/workspace-inspection-state.test.js b/tests/js/workspace-inspection-state.test.js index 975bfa9..6fca178 100644 --- a/tests/js/workspace-inspection-state.test.js +++ b/tests/js/workspace-inspection-state.test.js @@ -573,44 +573,99 @@ test('a zeroed negative is never filled with the positive text from the legacy p const record = undecided(zeroedGridGraph()); assert.deepEqual(record.prompts.positive, { status: 'resolved', text: 'a woman holding a coffee cup', source: 'graph-inferred' }); assert.deepEqual(record.prompts.negative, { status: 'resolved', text: '', source: 'zeroed-conditioning' }); - // A Reroute between the sampler and the zero-out is still a zeroed negative. - const rerouted = zeroedGridGraph(2); + // Behind an API-graph Reroute the legacy parse reads no negative, so there is nothing + // to correct: the server's answer stays. + const rerouted = zeroedGridGraph(1); rerouted[7] = { class_type: 'Reroute', inputs: { value: ['5', 0] } }; rerouted[20].inputs.negative = ['7', 0]; - assert.deepEqual(undecided(rerouted).prompts.negative, { status: 'resolved', text: '', source: 'zeroed-conditioning' }); + assert.equal(parser.legacyNegativeSource({ prompt: rerouted }), null); + assert.deepEqual(undecided(rerouted).prompts.negative, { status: 'ambiguous', text: null, source: null }); }); -test('a negative whose zero-out path cannot be decided stays as the server left it (#249)', t => { - globalThis.ComfyMetadata = require('../../web/js/comfy-metadata.js'); +test('the zero-out check follows the sampler the legacy parse read (#249)', t => { + const parser = require('../../web/js/comfy-metadata.js'); + globalThis.ComfyMetadata = parser; t.after(() => { delete globalThis.ComfyMetadata; }); - const ambiguous = { status: 'ambiguous', text: null, source: null }; - // One branch zeroes its negative, another encodes a text: which one the image used is open. + const zeroed = { status: 'resolved', text: '', source: 'zeroed-conditioning' }; + // The legacy parse reads the first sampler (20): its negative is the zero-out. const mixed = zeroedGridGraph(2); mixed[8] = { class_type: 'CLIPTextEncode', inputs: { text: 'blurry', clip: ['2', 0] } }; mixed[21].inputs.negative = ['8', 0]; - assert.deepEqual(undecided(mixed).prompts.negative, ambiguous); + assert.deepEqual(undecided(mixed).prompts.negative, zeroed); + // Reversed: the sampler it reads encodes "blurry", which it showed before and still does. + const reversed = zeroedGridGraph(2); + reversed[8] = { class_type: 'CLIPTextEncode', inputs: { text: 'blurry', clip: ['2', 0] } }; + reversed[20].inputs.negative = ['8', 0]; + assert.equal(parser.extractGenerationParams({ prompt: reversed }).negativePrompt, 'blurry'); + assert.deepEqual(undecided(reversed).prompts.negative, { status: 'resolved', text: 'blurry', source: 'graph-inferred' }); + // The positive fills from the legacy parse in both cases. + assert.equal(undecided(mixed).prompts.positive.text, 'a woman holding a coffee cup'); + assert.equal(undecided(reversed).prompts.positive.text, 'a woman holding a coffee cup'); +}); + +test('a zero-out mixed into the read negative leaves it as the server left it (#249)', t => { + globalThis.ComfyMetadata = require('../../web/js/comfy-metadata.js'); + t.after(() => { delete globalThis.ComfyMetadata; }); // A zero-out combined with another conditioning is not an empty negative. const combined = zeroedGridGraph(1); combined[8] = { class_type: 'CLIPTextEncode', inputs: { text: 'blurry', clip: ['2', 0] } }; combined[9] = { class_type: 'ConditioningCombine', inputs: { conditioning_1: ['5', 0], conditioning_2: ['8', 0] } }; combined[20].inputs.negative = ['9', 0]; - assert.deepEqual(undecided(combined).prompts.negative, ambiguous); - // The positive still fills from the legacy parse in both cases. - assert.equal(undecided(mixed).prompts.positive.text, 'a woman holding a coffee cup'); - // A workflow-only file holding a zero-out never fills the negative either. + assert.deepEqual(undecided(combined).prompts.negative, { status: 'ambiguous', text: null, source: null }); +}); + +function workflowOnly(workflow) { const entry = image(), data = envelope(entry); data.prompts = { positive: { status: 'unresolved', text: null, source: null }, negative: { status: 'unresolved', text: null, source: null } }; - const workflow = { nodes: [ - { id: 4, type: 'CLIPTextEncode', inputs: [], outputs: [{ links: [1, 2] }], widgets_values: ['a woman holding a coffee cup'] }, - { id: 5, type: 'ConditioningZeroOut', inputs: [{ name: 'conditioning', link: 1 }], outputs: [{ links: [3] }] }, - { id: 1, type: 'UNETLoader', inputs: [], outputs: [{ links: [4] }], widgets_values: ['base.safetensors', 'default'] }, - { id: 3, type: 'KSampler', inputs: [{ name: 'model', link: 4 }, { name: 'positive', link: 2 }, { name: 'negative', link: 3 }], widgets_values: [42, 'fixed', 8, 1, 'euler', 'simple', 1] }, - ], links: [[1, 4, 0, 5, 0, 'CONDITIONING'], [2, 4, 0, 3, 1, 'CONDITIONING'], [3, 5, 0, 3, 2, 'CONDITIONING'], [4, 1, 0, 3, 0, 'MODEL']] }; - assert.equal(globalThis.ComfyMetadata.extractGenerationParams({ workflow }).negativePrompt, 'a woman holding a coffee cup', 'the legacy parse would fill it'); Object.assign(data.metadata, { raw: { workflow: JSON.stringify(workflow) }, record_formats: { workflow: 'reserialized-json' }, prompt_graph: { status: 'absent', value: null }, workflow_graph: { status: 'parsed', value: workflow } }); - const record = contract('toInspectionRecord')(data, entry, extractGenerationFacts); - assert.deepEqual(record.prompts.negative, { status: 'unresolved', text: null, source: null }); + return contract('toInspectionRecord')(data, entry, extractGenerationFacts); +} + +// A UI workflow: encode 4 -> zero-out 5 -> sampler 3's negative, encode 4 -> positive. +function zeroedWorkflow({ zeroMode = 0, extraNodes = [], extraLinks = [], negativeLink = 3 } = {}) { + return { nodes: [ + { id: 4, type: 'CLIPTextEncode', inputs: [], outputs: [{ links: [1, 2] }], widgets_values: ['a woman holding a coffee cup'] }, + { id: 5, type: 'ConditioningZeroOut', mode: zeroMode, inputs: [{ name: 'conditioning', type: 'CONDITIONING', link: 1 }], outputs: [{ links: [3] }] }, + { id: 1, type: 'UNETLoader', inputs: [], outputs: [{ links: [4] }], widgets_values: ['base.safetensors', 'default'] }, + { id: 3, type: 'KSampler', inputs: [{ name: 'model', link: 4 }, { name: 'positive', link: 2 }, { name: 'negative', link: negativeLink }], widgets_values: [42, 'fixed', 8, 1, 'euler', 'simple', 1] }, + ...extraNodes, + ], links: [[1, 4, 0, 5, 0, 'CONDITIONING'], [2, 4, 0, 3, 1, 'CONDITIONING'], [3, 5, 0, 3, 2, 'CONDITIONING'], [4, 1, 0, 3, 0, 'MODEL'], ...extraLinks] }; +} + +test('workflow-only files follow the legacy reading of their zero-out (#249)', t => { + const parser = require('../../web/js/comfy-metadata.js'); + globalThis.ComfyMetadata = parser; + t.after(() => { delete globalThis.ComfyMetadata; }); + // A connected zero-out on the read sampler's negative is the zeroed empty negative. + const direct = zeroedWorkflow(); + assert.equal(parser.extractGenerationParams({ workflow: direct }).negativePrompt, 'a woman holding a coffee cup', 'the legacy parse would fill it'); + assert.deepEqual(workflowOnly(direct).prompts.negative, { status: 'resolved', text: '', source: 'zeroed-conditioning' }); + // A bypassed zero-out (mode 4) passes its conditioning through: the legacy reading is right. + const bypassed = workflowOnly(zeroedWorkflow({ zeroMode: 4 })); + assert.deepEqual(bypassed.prompts.negative, { status: 'resolved', text: 'a woman holding a coffee cup', source: 'graph-inferred' }); + // An unconnected zero-out beside a text negative changes nothing. + const unconnected = workflowOnly(zeroedWorkflow({ negativeLink: 6, + extraNodes: [{ id: 6, type: 'CLIPTextEncode', inputs: [], outputs: [{ links: [6] }], widgets_values: ['blurry'] }], + extraLinks: [[6, 6, 0, 3, 2, 'CONDITIONING']] })); + assert.deepEqual(unconnected.prompts.negative, { status: 'resolved', text: 'blurry', source: 'graph-inferred' }); +}); + +test('a negative read through a subgraph holding a zero-out is not filled (#249)', t => { + const parser = require('../../web/js/comfy-metadata.js'); + globalThis.ComfyMetadata = parser; + t.after(() => { delete globalThis.ComfyMetadata; }); + // The sampler's negative comes from a subgraph instance whose definition zeroes it. + const definition = 'aaaaaaaa-0000-4000-8000-000000000001'; + const workflow = zeroedWorkflow({ negativeLink: 7, + extraNodes: [{ id: 9, type: definition, inputs: [{ name: 'conditioning', type: 'CONDITIONING', link: 8 }], outputs: [{ links: [7] }] }], + extraLinks: [[7, 9, 0, 3, 2, 'CONDITIONING'], [8, 4, 0, 9, 0, 'CONDITIONING']] }); + workflow.definitions = { subgraphs: [{ id: definition, nodes: [{ id: 1, type: 'ConditioningZeroOut', inputs: [], outputs: [] }], links: [] }] }; + assert.equal(parser.extractGenerationParams({ workflow }).negativePrompt, 'a woman holding a coffee cup', 'the legacy parse would fill it'); + assert.deepEqual(workflowOnly(workflow).prompts.negative, { status: 'unresolved', text: null, source: null }); + // The same instance of a definition without a zero-out keeps the legacy fill. + workflow.definitions.subgraphs[0].nodes[0].type = 'Reroute'; + assert.equal(workflowOnly(workflow).prompts.negative.text, 'a woman holding a coffee cup'); }); test('callers without the browser global pass the legacy parser explicitly', () => { diff --git a/web/js/comfy-metadata.js b/web/js/comfy-metadata.js index f216646..c505ea7 100644 --- a/web/js/comfy-metadata.js +++ b/web/js/comfy-metadata.js @@ -212,12 +212,17 @@ return undefined; } + /** The sampler the legacy parse reads: a custom sampler's guider, else the first sampler. */ + function selectSampler(ctx, nodes) { + const custom = nodes.find(isCustomSampler); + const [, guider] = custom ? enter(ctx, custom.inputs.guider, new Set()) : []; + return { custom, sampler: isSampler(guider) ? guider : nodes.find(isSampler) }; + } + function fromPromptGraph(graph, workflow) { const ctx = createContext(graph, workflow); const nodes = Object.values(graph); - const custom = nodes.find(isCustomSampler); - const [, guider] = custom ? enter(ctx, custom.inputs.guider, new Set()) : []; - const sampler = isSampler(guider) ? guider : nodes.find(isSampler); + const { custom, sampler } = selectSampler(ctx, nodes); if (!sampler) { const loose = nodes.find((n) => n && n.inputs && /ksampler/i.test(n.class_type || "")); const inputs = (loose && loose.inputs) || {}; @@ -402,6 +407,26 @@ return merged; } + /** + * Where `extractGenerationParams` reads its negative from (#249): the graph it used + * (the API graph, else the workflow converted the same way) and the read sampler's + * `negative` link, or null when it found no negative. It changes no reading. + */ + function legacyNegativeSource(comfyData) { + const data = comfyData || {}; + const sources = []; + if (data.prompt && typeof data.prompt === "object") sources.push([data.prompt, data.workflow]); + if (data.workflow) sources.push([workflowToGraph(data.workflow), null]); + for (const [graph, workflow] of sources) { + const ctx = createContext(graph, workflow); + const { sampler } = selectSampler(ctx, Object.values(graph)); + if (sampler && hasValue(traceConditioning(ctx, sampler.inputs.negative, "negative"))) { + return { graph, link: sampler.inputs.negative }; + } + } + return null; + } + const FACT_FIELDS = ["seed", "steps", "cfg", "sampler", "scheduler", "model"]; const WORKFLOW_PASSTHROUGH = Symbol("inspection workflow passthrough"); // These tables must equal inspection-semantics.json: SAMPLERS its `samplers` section @@ -761,7 +786,7 @@ return factSet(metadataStatus === "available" || metadataStatus === "absent" ? "not-recorded" : "unresolved"); } - const api = { extractGenerationParams, extractGenerationFacts, PLACEHOLDERS }; + const api = { extractGenerationParams, extractGenerationFacts, legacyNegativeSource, PLACEHOLDERS }; // For the semantics sync test only; not part of the legacy page contract. Object.defineProperty(api, "_factsTables", { value: Object.freeze({ diff --git a/web/js/workspace/inspection-state.mjs b/web/js/workspace/inspection-state.mjs index 3f8932c..1a2fdbe 100644 --- a/web/js/workspace/inspection-state.mjs +++ b/web/js/workspace/inspection-state.mjs @@ -175,45 +175,52 @@ function comfyMetadata() { } // #249: the legacy parse follows ConditioningZeroOut back to its input, so it reads the -// positive text as the negative. The workspace shows the server's zeroed state instead -// when every sampler's negative is a zero-out, and never fills the negative from the -// legacy parse when a zero-out sits anywhere else in a negative path. +// positive text as the negative. Only the negative it actually read is checked: a direct +// zero-out shows the server's zeroed state; a zero-out elsewhere on +// that path, or a subgraph instance whose definition holds one, leaves the negative +// unfilled. Any other negative keeps the legacy fill, so this page never shows less. const ZERO_OUT = 'ConditioningZeroOut'; const ZEROED_NEGATIVE = Object.freeze({ status: 'resolved', text: '', source: 'zeroed-conditioning' }); const MAX_NEGATIVE_VISITS = 2000; +const MODE_MUTED = 2, MODE_BYPASSED = 4; const isLink = value => Array.isArray(value) && value.length === 2 && ['string', 'number'].includes(typeof value[0]); -/** 'zeroed', 'undecided' (do not fill), or null (no zero-out: the legacy fill applies). */ -function zeroOutNegative(graph, workflow) { - if (!graph) return Array.isArray(workflow?.nodes) && workflow.nodes.some(node => node?.type === ZERO_OUT) ? 'undecided' : null; - const nodes = Object.values(graph).filter(object); - if (!nodes.some(node => node.class_type === ZERO_OUT)) return null; - const kinds = nodes.filter(node => object(node.inputs) && isLink(node.inputs.negative)).map(node => negativeKind(graph, node.inputs.negative)); - if (kinds.length && kinds.every(kind => kind === 'zeroed')) return 'zeroed'; - return kinds.every(kind => kind === 'plain') && kinds.length ? null : 'undecided'; +/** Subgraph definition IDs that run a zero-out, directly or through a nested instance. */ +function zeroOutSubgraphs(workflow) { + const definitions = Array.isArray(workflow?.definitions?.subgraphs) ? workflow.definitions.subgraphs.filter(object) : []; + const active = item => Array.isArray(item.nodes) ? item.nodes.filter(node => object(node) && ![MODE_MUTED, MODE_BYPASSED].includes(node.mode)) : []; + const found = new Set(); + for (let changed = true; changed;) { + changed = false; + for (const item of definitions) { + if (found.has(item.id) || !active(item).some(node => node.type === ZERO_OUT || found.has(node.type))) continue; + found.add(item.id); changed = true; + } + } + return found; } -function negativeKind(graph, link) { - let node = graph[String(link[0])]; - for (let hops = 0; object(node) && node.class_type === 'Reroute' && hops < MAX_NEGATIVE_VISITS; hops++) { - const next = object(node.inputs) ? node.inputs.value : undefined; - node = isLink(next) ? graph[String(next[0])] : undefined; - } +/** 'zeroed', 'undecided' (do not fill), or null (the legacy fill applies). */ +function zeroOutNegative(source, workflow) { + if (!source || !object(source.graph) || !isLink(source.link)) return null; + const { graph } = source, subgraphs = zeroOutSubgraphs(workflow); + const zeroing = node => node.class_type === ZERO_OUT || subgraphs.has(node.class_type); + if (!Object.values(graph).some(node => object(node) && zeroing(node))) return null; + const node = graph[String(source.link[0])]; if (!object(node)) return 'undecided'; if (node.class_type === ZERO_OUT) return 'zeroed'; - // Any zero-out further up (combined, averaged, ...) leaves the negative undecided. + // One bounded walk upstream; an index pointer keeps it linear in the nodes visited. const queue = [node], seen = new Set(); - while (queue.length) { - const current = queue.shift(); - if (current.class_type === ZERO_OUT) return 'undecided'; - if (seen.size >= MAX_NEGATIVE_VISITS) return 'undecided'; + for (let index = 0; index < queue.length; index++) { + const current = queue[index]; + if (zeroing(current) || seen.size >= MAX_NEGATIVE_VISITS) return 'undecided'; for (const value of object(current.inputs) ? Object.values(current.inputs) : []) { const id = isLink(value) ? String(value[0]) : null; if (id === null || seen.has(id) || !object(graph[id])) continue; seen.add(id); queue.push(graph[id]); } } - return 'plain'; + return null; } function withWorkflowParams(record, parser = comfyMetadata()) { @@ -226,7 +233,9 @@ function withWorkflowParams(record, parser = comfyMetadata()) { const found = key => typeof params[key] === 'string' || typeof params[key] === 'number' ? String(params[key]) !== '' && params[key] !== parser.PLACEHOLDERS?.[key] : false; const prompts = { ...record.prompts }; - const zeroed = prompts.negative?.status === 'resolved' ? null : zeroOutNegative(graph, workflow); + const read = prompts.negative?.status === 'resolved' || typeof parser.legacyNegativeSource !== 'function' ? null : + parser.legacyNegativeSource({ prompt: graph, workflow }); + const zeroed = zeroOutNegative(read, workflow); for (const [role, key] of [['positive', 'positivePrompt'], ['negative', 'negativePrompt']]) { const current = prompts[role]; if (current?.status === 'resolved') continue; @@ -240,6 +249,8 @@ function withWorkflowParams(record, parser = comfyMetadata()) { record.prompts = prompts; for (const [field, key] of LEGACY_FACTS) { // Only open answers are filled: a known value or a definite "not recorded" stays. + // An ambiguous field (e.g. a compare grid's per-branch model) is filled too, on + // purpose: the legacy pages show this value, and this page never shows less. if (!found(key) || !['unresolved', 'ambiguous'].includes(record.generation[field]?.status)) continue; record.generation[field] = { status: 'known', value: String(params[key]), source: graph ? 'api-graph' : 'workflow', evidence: 'configuration', reason: null, sampler_id: null }; }