Skip to content

#155 comfyui workflow config precedence: per-source documented order#172

Merged
realproject7 merged 1 commit into
mainfrom
task/155-config-precedence
Jul 14, 2026
Merged

#155 comfyui workflow config precedence: per-source documented order#172
realproject7 merged 1 commit into
mainfrom
task/155-config-precedence

Conversation

@realproject7

Copy link
Copy Markdown
Owner

Closes #155.

Problem

resolveComfyUIConfig (packages/providers/src/comfyui-config.ts) coalesced the env path, the config-file path, AND the workspace path into one workflowPath value, checked BEFORE the config-file inline graph:

const workflowPath = nonEmpty(env.TOONY_COMFYUI_WORKFLOW) ?? nonEmpty(fromFile.workflowPath) ?? nonEmpty(ws?.workflow);
if (overrides.workflow )  else if (workflowPath ) load(workflowPath) else if (fromFile.workflow ) 

So a stale workspace .toony/config.json workflow path (documented LOWEST) silently beat the config file's inline graph (documented HIGHER) — generation ran the wrong workflow with no warning. Severity: MEDIUM.

Fix

Check each source in the documented order, no coalescing:
overrides.workflow → env path → config-file (fromFile.workflow inline ?? fromFile.workflowPath) → ws.workflow path → bundled default. The config-file inline graph is now selected before any lower-precedence PATH. The module header already documented this order (override > env > config-file > workspace); behaviour now matches it. No docs change needed beyond the corrected inline comment.

Tests — precedence matrix (all four sources)

packages/providers/src/__tests__/comfyui-config.test.ts (+6): each source is marked with a unique workflow class_type and the winner asserted:

  • inline override beats every lower source;
  • env path beats config-file + workspace;
  • config-file inline beats a stale workspace path with no config path — the headline bug (pre-fix this returned the workspace path);
  • config-file inline beats config-file path + workspace path;
  • config-file path beats workspace path;
  • workspace path used when nothing higher is set.
    (The bundled-default case was already covered.)

EPIC Alignment

  • EPIC EPIC: Build Toony MVP #1 (Build Toony MVP) — predictable provider configuration: the workflow the operator pins is the one that runs, so generation is trustworthy and matches the documented contract.
  • Ticket [audit] providers: workflow config precedence disagrees with the documented order #155 (Batch 22-approved): the per-source restructure the audit proposed, with the full four-source test matrix.
  • Contracts respected: precedence = inline override > env path > config-file (inline ?? path) > workspace path; docs and behaviour agree; test matrix covers all four sources. Out of scope: any new config source or provider feature.

Self-Verification

  • AC — precedence matrix covers all four sources: 6 tests above, including the config-inline-vs-workspace-path regression. ✅
  • AC — docs and behaviour agree: the module header's documented order (override > env > config-file > workspace) now matches the code path-for-path. ✅
  • AC — pnpm check + pnpm test green: uncached (turbo … --force) on Node 20.20.2 AND 24.15.0check exit 0; 17/17 tasks, 0 fail (@toony/providers 50 → 56).
  • Invariants: no new deps; no stubs; no new config source; not visible UI (provider config logic) → no Design Fidelity.

Out of scope

Any new config source or provider feature.

🤖 Generated with Claude Code

…rder

`resolveComfyUIConfig` coalesced env-path, config-file-path, AND workspace-path
into one `workflowPath` value checked BEFORE the config-file INLINE graph — so a
stale workspace `.toony/config.json` workflow PATH silently beat the config
file's inline graph, contradicting the documented order.

Restructured to check each source in order: inline override > env path >
config-file (inline graph ?? path) > workspace path > bundled default. The
config-file inline graph is now selected before any config/workspace PATH.

Test matrix (6 cases) covers all four sources, including the headline bug
(config-file inline beats a stale workspace path) and config-inline-vs-config-path.
Zero change to the documented header. check + test green uncached Node 20.20.2 +
24.15.0 (@toony/providers 50 -> 56).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@project7-interns project7-interns left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE

Epic Alignment: PASS

#155 makes the actual ComfyUI workflow selection match the documented operator-facing precedence, preventing a stale lower-priority workspace path from silently winning.

Checked (evidence)

  • Structural gate: live PR body has filled ## EPIC Alignment and ## Self-Verification; no Design Fidelity table is required for provider config logic.
  • Contract: live issue #155 requires override > env path > config-file (inline ?? path) > workspace path.
  • Implementation: packages/providers/src/comfyui-config.ts:205-229 evaluates each source in exactly that sequence, with config inline checked before the config-file path and workspace path.
  • Regression matrix: packages/providers/src/__tests__/comfyui-config.test.ts:173-280 covers override, env, config inline, config path, workspace, and the original inline-versus-stale-workspace failure.
  • Riskiest part: config-file inline/path ordering; the explicit branches make the intended nullish priority auditable and remove the prior coalesced-path bypass.
  • Kill-list: complete two-file diff scanned - clean; no new source, dependency, stub, or scope creep.
  • CI: gh pr checks 172 -> Node 20 PASS (1m37s); Node 24 PASS (1m2s).

Findings

  • None.

Decision

The code, documentation, and tests now agree on the required precedence order, including the reported regression path.

@realproject7

Copy link
Copy Markdown
Owner Author

[@re2 REVIEW — APPROVE] (marked comment — @re2 formal gh pr review is rejected on own-identity PRs; this comment IS the approval)

Review of PR #172 (#155), commit f165478 vs origin/main (238f408). Verdict: APPROVE / TICKET-OK.

Checked (evidence)

  • Precedence now matches the documented order exactly (comfyui-config.ts:205-229), each source checked IN ORDER: overrides.workflow (1) → envWorkflowPath (2) → fromFile.workflow inline (3) → fileWorkflowPath (4) → wsWorkflowPath (5) → bundled default (6) = override > env path > config-file (inline ?? path) > workspace path > default.
  • Root-cause fix confirmed: the old code coalesced env ?? fromFile.workflowPath ?? ws.workflow into one workflowPath checked BEFORE fromFile.workflow, so a lower-precedence workspace PATH silently beat the config-file INLINE graph. Splitting the sources and ordering the branches fixes it — the inline graph (step 3) is now reached before the workspace path (step 5).
  • nonEmpty gating is sound: nonEmpty returns undefined for empty/non-string, so the !== undefined checks correctly skip empty values (the dropped .length > 0 was redundant).
  • 6-case precedence matrix with distinct class_type markers unambiguously proves each rung: override-wins, env-path-wins, config-inline beats ws path (the bug), config-path beats ws path, ws-path-when-sole-source, and the isolated headline (config-inline vs stale ws path, no config path → CONFIG_INLINE; pre-fix returned WS_PATH). Plus the existing default-graph case.
  • Reproduced UNCACHED on Node 24.15.0: @toony/providers 56 pass / 0 fail (50→56); all six precedence tests confirmed by name.
  • Scope/invariants: change is comfyui-config.ts + its test only; no new deps, no stubs, no visible UI.

Clean to merge on my side. No further changes.

@realproject7
realproject7 merged commit b118d0f into main Jul 14, 2026
2 checks passed
@realproject7 realproject7 mentioned this pull request Jul 14, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[audit] providers: workflow config precedence disagrees with the documented order

2 participants