Repository navigation
fix(tests): follow command literals through forwarding helpers in the emit-command ratchet - #1018
Conversation
`test_every_emitted_command_registers_a_schema` only recorded a `command=` value when it was an `ast.Constant` at the `.emit` call itself. Every structured workflow edit passes its command name to `workflow_edit._finish`, which emits `command=command` — an `ast.Name` — so the literal sits one frame up at the call site and the ratchet saw nothing. The guard has been blind for its whole existence. Teach the collector to follow a literal through a forwarder: pass 1 finds functions in `comfy_cli/` that take a `command` parameter and forward that same name into `.emit(command=...)`; pass 2 extracts the string literal each call site passes in that position, by keyword or positionally, falling back to a literal signature default. It stays a static scan — nothing imports or executes `comfy_cli` — and stays conservative: a non-literal argument is skipped rather than guessed, and forwarders are followed only through bare-name calls. Also register the two commands the blind spot was hiding.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @christian-byrne.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/6 reviewers contributed findings.
`delete-node` set `renderer.command = "workflow delete-node"` but passed `"workflow delete"` to `_finish`, so the SUCCESS envelope named a different, real command — the cloud/local CRUD `comfy workflow delete` (workflow.py) — while the error path fell back to the correct name. The registration ratchet could not catch it twice over: it could not see the literal at all (it is forwarded through `_finish`), and even once it could, `comfy workflow delete` is itself registered, so the check passes on a name that only looks right. Agents reading the success envelope were routed to the wrong command and schema. `comfy workflow delete-node` is already in COMMAND_SCHEMAS, so correcting the literal resolves to the existing `workflow` schema with no new entry. Pinned by a test asserting the success and error envelopes agree.
Review follow-ups on the forwarder pass. Key forwarders by `(defining module, function name)` and resolve a call site only when the forwarder is defined in that module or explicitly imported into it. Bare-name keying failed open in both directions: two same-named forwarders marked each other ambiguous and silently dropped every call site of both, and a same-named non-forwarder had the forwarder's default attributed to it. Stop using `ast.walk` to decide whether a function forwards — it descends into nested functions, lambdas and classes, so an outer function was marked a forwarder when an inner closure had its own `command`. Walk only the function's own body, and skip any function that rebinds `command` before the emit, since a transformed value is not the caller's literal. Narrow the starred-argument guard: a positional splat cannot bind a keyword-only `command`, and one appearing after `command`'s index does not affect that index. Only skip when a splat sits at or before it. Add a ratchet over all five known forwarders. Only `_finish`/`_emit_op` have forwarder-only commands to assert on, so `_emit_terminal`, `emit_selected` and `validate_api_workflow` could otherwise stop resolving with nothing failing. Narrow the docstring to the one untransformed hop actually covered and name the known gap: `deploy promote`/`deploy rollback` build their names with an f-string two hops down and stay unchecked.
Swarmhost agentic reviewThe detailed evaluation is available to employees in the internal Slack review thread. Evaluation budget remaining for this pull request: 14 automatic and 15 manual. Updated by Swarmhost's agentic review process. |
comfy-greenlight-bot
left a comment
There was a problem hiding this comment.
This pull request at head commit ae605a9 has been reviewed and approved by Swarmhost's agentic review process.
The detailed evaluation is available to employees in the internal Slack review thread.
The blind spot
tests/comfy_cli/output/test_discovery.py::test_every_emitted_command_registers_a_schemais the ratchet that stops a command from emitting anenvelope/1without a schema registered inCOMMAND_SCHEMAS. Its collector recorded acommand=value only when it was anast.Constantat the.emitcall itself.Every structured workflow edit routes through a local forwarder:
The literal lives one frame up, at the call site (
_finish(..., "workflow clear")). So the scanner saw aName, recorded nothing, and the ratchet never had an opinion about any of it. The guard has been blind for its entire existence.Blast radius
The forwarder pattern is not confined to
_finish. A repo-wide scan for "takes acommandparameter and forwards that same name into.emit(command=...)" finds five forwarders across four files:_finishcomfy_cli/command/workflow_edit.py_emit_opcomfy_cli/command/workflow_edit.py_emit_terminalcomfy_cli/command/jobs.pyemit_selectedcomfy_cli/selector.pyvalidate_api_workflowcomfy_cli/command/workflow.pyTogether they hid 10 command literals from the ratchet:
jobs watch,validate,workflow validate,workflow add-node,workflow clear,workflow connect,workflow insert-workflow,workflow reset-doc,workflow set-node-field,workflow set-widget.The two gaps it hid
Eight of those ten happened to be registered already. Two were not, and were not in
LEGACY_UNREGISTEREDeither (that allowlist contains no workflow entries at all):comfy workflow clearcomfy workflow reset-docBoth are registered here as
"workflow", which is what every other_finish-based command maps to — they emit the identical_finishop envelope (workflow/op/base_version/version/wrote) as the already-registeredcomfy workflow delete. Per the test's own docstring they are registered rather than allowlisted;LEGACY_UNREGISTEREDdoes not grow.The fix
Two static passes, in the test file:
comfy_cli/that take acommandparameter and forward that same name into.emit(command=...). Record the parameter's positional index and any literal default.Deliberately conservative, because a ratchet that guesses is worse than one that abstains:
comfy_cliobj.emit_selected(...)is never assumed to be the same function*args/**kwargssplats suppress positional extraction and the default fallbackHow the positive control proves it
A guard nobody has watched fail is not known to work, so this PR demonstrates the failure both ways.
Red — scanner fix applied, the
workflow reset-docregistration reverted:Before the scanner change that same state was green — which is the defect in one line.
Green — both halves applied:
20 passed.Six new tests encode it so it cannot rot back:
commandbut never forwards it into.emitis not treated as a forwardertest_workflow_edit_commands_are_reachable_only_through_the_forwarder, a ratchet over the real source tree listing seven commands reachable only via_finish/_emit_op. If the forwarder pass ever regresses, this fails loudly before the registration check can go quiet again.Testing
tests/comfy_cli/output/test_discovery.py— 20 passedtests/suite —15 failed, 10102 passed, 33 skippedmain—15 failed, 10096 passed, 33 skippedZero failure delta; +6 passing tests. The 15 failures are pre-existing on
mainand untouched by this change (code_search,logs,node_deps,agent/test_command,build_digest_cache, and twotest_file_utilsumask assertions sensitive to the runner's umask).ruff checkandruff format --checkboth clean.Note
workflow disconnectis a ninth_finishcall site arriving in #923 and is deliberately untouched here. Once both land, the strengthened scanner covers it automatically — and would have caught it had it gone unregistered.