fix(jobs): scope jobs ls state-file rows to the resolved --where target (add --all) - #582
fix(jobs): scope jobs ls state-file rows to the resolved --where target (add --all)#582mattmillerai wants to merge 2 commits into
jobs ls state-file rows to the resolved --where target (add --all)#582Conversation
…rget
`--where` only routed the SERVER query: `_gather_local_state_files` read
every state file in the jobs state dir regardless of each state's `where`,
so `comfy --json --where local jobs ls` kept surfacing cloud jobs from
previous runs (state files never expire). Agents wrapping the CLI with a
pinned `--where local` were getting cloud rows indefinitely.
- `_gather_local_state_files` takes a `where` filter; a missing/empty
`state.where` reads as "local". `None` keeps the unfiltered union view.
- `jobs ls` resolves the target once and scopes state rows to it; the new
`--all` flag restores the union. `--orphaned` stays unfiltered — watcher
cleanup is where-agnostic.
- `--watch` threads the same scope so the live table matches `jobs ls`.
- Payload gains `scope` ("local" / "cloud" / "all") so callers can see
which view they got. Additive; envelope/1 is unchanged.
- `server_rows` are deliberately not filtered: they are already scoped by
whichever backend was queried.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 1 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
The `--watch` branch returned before `if orphaned: local_only = True`, and `_watch_ls` never received `orphaned`, so `jobs ls --watch --orphaned` still queried the server, listed every state file instead of only crashed-watcher rows, and scoped state files to the resolved target instead of the union — diverging from the one-shot behavior this PR documents. Hoist the `orphaned` handling and the `state_where` decision above the `--watch` branch and thread `orphaned_only` through `_watch_ls` into `_gather_local_state_files`, so the live table applies exactly the same filters as `jobs ls`. Tests cover the kwargs the watch path is driven with (default / --all / --where cloud / --orphaned) plus `_watch_ls` actually forwarding them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ELI-5
comfy jobs lshas two sources: the ComfyUI/cloud server it queries, and thejob state files this CLI wrote to disk when you submitted something.
--whereonly ever routed the server half — the state-file half read every file
regardless of which target the job ran on. So
comfy --json --where local jobs lshappily listed cloud jobs from last week, forever (state files never expire).
Now the state-file rows are scoped to the resolved
--wheretarget too, and theunion view is one flag away:
--all.What changed
_gather_local_state_files(..., where=...)— new keyword filter. Rowswhose
state.wheredoesn't match are skipped; a missing/emptystate.wherereads as
"local"(a defensive default — see the note below).where=None(the default) keeps the unfiltered union view, so every existing caller is
behaviourally unchanged.
jobs lsresolves the target once (--whereflag >COMFY_WHEREenv —which is how the top-level
comfy --wherearrives > config default) andscopes state rows to it. New
--allflag restores the union.--orphanedstays unfiltered. Watcher cleanup is where-agnostic — youwant to see a crashed cloud watcher from a default (local) shell. The
wherefilter is also applied after the stale-watcher reap, so reapingkeeps working the same in every view.
--watchthreads the same scope, so the live table matchesjobs ls.server_rowsare deliberately not filtered — they're already scoped bywhichever backend was queried.
scope("local"/"cloud"/"all") so agents can seewhich view they got. Additive,
envelope/1unchanged; documented incomfy_cli/schemas/jobs.jsonand the shippedcomfyskill doc.Behaviour change worth flagging
This narrows the default
jobs lsview. Anything relying on "one call showseverything I ever submitted via this CLI" must now pass
--all. That's theintent — the pretty header already renders a single target's host, so a scoped
list is what the UI claimed to show — but it is a visible change and
scopeisthere so callers can detect it.
Downstream:
comfy-local-mcp'sget_queuealways passes--where localand isdocumented local-only, so it gets correct behaviour for free once this ships.
Verification
uv sync --frozen --extra dev, then:pytest tests/— 2776 passed, 37 skipped.ruff check ./ruff format --check .— unchanged frommain(the 15UP038findings and the one unformatted file,tests/comfy_cli/command/github/test_pr.py,are pre-existing on
main; this diff adds none).New tests (
tests/comfy_cli/jobs/test_jobs.py): a seeded temp state dir with alocal, acloudand a legacy row, covering default/--where cloud/--all/--orphanedat both the gather level and throughls_cmd's emitted envelope,plus
--allin the machine-readable help contract.End-to-end, against the real CLI with a seeded state dir (
HOMEpointed ata temp tree holding one local + one cloud + one legacy state file):
No capability is removed — the union view was empirically re-attempted after
the change and is fully reachable via
--all(second command above).Judgment calls
where" don't actually exist in a readable form.JobState.wherehas been a required, non-defaulted field since the statefile was introduced, so
jobs_state.read()already rejects a file that omitsthe key entirely (returns
None) — it never reaches the filter. Thereachable legacy shape is an explicitly null/empty value, which the
state.where or "local"default handles. Both are pinned by tests. I didnot give
JobState.wherea default: that's a shared-dataclass changewell outside this ticket, and it would resurrect files today treated as
truncated.
scopeis"all"for--orphaned, not the target. The ticket saysscopeis"all"or the target; since--orphanedgathers unfiltered, thetruthful value is
"all".scopeis derived from the same variable passedto the gatherer, so it can't drift from the actual view.
whereto"local"for a null/empty statewhere(JobRow.whereis typedstrand already defaults to"local"), soa row scoped as local can't report
where: null. Nil blast radius —jobs_state.new()always writes a target._is_cloud(where)calls inls_cmdwith thesingle resolved
target_where. Same semantics (and still monkeypatchable),but it guarantees the filter, the emitted
where, and the pretty header cannever disagree.
comfyskill doc describedjobs lsas "local state files + server queue", which is now incomplete foragents. One short paragraph + an example line were added.
Pre-existing issue observed, not fixed (out of scope)
While verifying the cloud path live:
jobs ls --where cloudwith credentialspresent but the cloud API unreachable raises an uncaught
URLErrorinstead ofdegrading to the state-file view its own comment promises (only
typer.Exitiscaught). And when the preflight fails, its error envelope wins the
one-envelope-per-process guard, so the state-only fallback never reaches a JSON
caller. Both predate this change and are untouched here; the cloud scoping is
covered by unit tests instead.