Repository navigation
[PILOT] v5.9.0 -- The Artist Verbs: local-first CLI + browser canvas round-trip - #98
Merged
Merged
Conversation
…cozy read/test agents Add an explicit git-prohibition clause to cozy-scout, cozy-provisioner, and cozy-crucible. These three carry a load-bearing Bash grant (probes / downloads / pytest) that the tools: frontmatter cannot narrow to "Bash minus git", so an unrestricted Bash reaches git push despite read-only prose -- the 2026-07-08 push-boundary incident. The clause documents the enforced boundary; conductor-only orchestration (ORCH.L1) is the actual control. tools: frontmatter left unchanged. Baseline: 4821 passed, 2 skipped, 0 failed (tests/ -m "not integration"). Working tree equals master @ fb50917; the bootstrap/middleware refactor is parked in a local stash per OQ-B, not committed. Miles 2-8 remain BLOCKED-BY-FREEZE pending the demo outcome / PASS 5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…k sparkline, VRAM bar, step times) Add agent/_render.py -- four stdlib-only, deterministic render functions the SEE verb will consume: braille_sparkline (dense 2x4-dot cells, optional width via nearest-neighbor resample), block_sparkline (eighth-block fallback), vram_bar (clamped [#---] usage bar), format_step_times (count / s-it avg / total). Edge cases handled explicitly: empty and all-invalid series return "", all-equal maps to a flat mid level (no divide-by-zero), None/NaN/Inf filtered, width clamped, negatives normalized or clamped. Inert by construction: lives outside agent/tools/, exports no TOOLS, absent from every registry name list -- consumed by nothing until WP-SEE-B wires it into `cozy run` post-freeze (CLI.L3). 39 tests in tests/test_render.py; collection intact at 4907. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…te (offline-first) agent/verbs/find.py -- build_models_report / build_nodes_report / command_palette with the ratified glyph legend (checkmark installed, x missing, warning attention). Listings are pure local disk reads reusing the comfy_inspect handlers; the one liveness seam (workflow missing-node check via find_missing_nodes) is loopback-only and degrades to a human-worded note when ComfyUI is down. Palette scores by prefix > substring > difflib, deterministic ordering. Plain-text renderers; structured dicts left for the CLI layer to dress up. 28 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…enderer agent/verbs/see.py -- StepTimeCollector subscribes to the TriggerRegistry singleton as a second consumer (idempotent, observe-only, fail-soft; mirrors the diagnosis subscriber), deriving step durations from event timestamps rather than the mixed-clock elapsed_ms. render_run_summary draws braille sparkline + step-time line + top slow nodes + a vram_bar from one /system_stats poll, degrading in human words when ComfyUI is down. VRAM-over-time and loss series stay CUT per the ratified WP-SEE scope. Consumes agent/_render.py as-is. 27 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…wser canvas agent/verbs/open_canvas.py -- open_canvas() checks ComfyUI liveness, pushes the session workflow through the existing push_workflow_to_canvas bridge (never re-implements the POST), then opens the local canvas via webbrowser.open on COMFYUI_URL (loopback only). Returns a structured status dict whose message carries the v1 realities -- positions re-laid-out by the frontend importer, broadcast to every open tab -- in artist words. Push failure degrades to open-with-message. First half of the canvas round-trip; ingest (pull) is Mile 6. 7 tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…wired into the CLI agent/cli.py -- five additive commands on the existing Typer app: models list / nodes list (sub-apps, lazy engine imports, degradation notes verbatim at exit 0), find (palette), open (--no-push; exit 0 iff the canvas opened), see (collector install -> execute_with_progress -> summary; partial telemetry still renders on failed runs). The 11 existing commands are untouched. Help text in artist words. agent/tools/comfy_execute.py -- one additive success-dict key (progress_log), recommended by the SEE contract so post-hoc telemetry works without re-running. 26 CLI tests; full suite 4948 passed, 0 failed (baseline 4821 held); ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…with what-changed agent/verbs/intend.py -- resolve_recipe (exact name -> trigger regex -> difflib hints, no-raise), apply_recipe_to_session routing verbatim through the existing apply_recipe handler (validated set_input patches, undo preserved, zero engine duplication), what-changed computed from before/after session snapshots so even adjust deltas report old->new honestly. Deterministic rail only -- never orchestrate/stage (CLI.L4). 23 tests; 100-test neighbor run green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…0 offsite) agent/verbs/own.py -- doctor_report (5 fixed-order checks, each a finding with artist-worded note + fix hint; ComfyUI-down is a finding, never a crash), stats_report (reuses the Mile-2 disk scan; session outcome counts; VRAM snapshot when live), search_models (fuzzy over the LOCAL scan only -- the bundled offline index stays deferred per OQ-4). Glyphs single-sourced from the find engine. 19 tests; 47-test combined run green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…odel search agent/cli.py -- --recipe/-w additive on the EXISTING run command per WP-INTEND (recipe short-circuits before any LLM/credential code: keyless rail); applied recipes validate then execute on the same telemetry rail as cozy see (shared _execute_with_telemetry helper; braille sparkline subscribes with zero extra wiring; see behavior pinned by its unmodified suite). cozy doctor (exit 0/1, CI-gateable), cozy stats, cozy model search (singular sub-app; models search additive alias). 24 CLI tests; full suite 5014 passed, 0 failed; ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alidated undoable patch agent/verbs/pull_canvas.py -- pull_canvas() folds the artist's browser graph into the session strictly per ORCH.L8: jsonpatch.make_patch(current, artist) applied through the apply_workflow_patch handler (its lock/undo/rollback reused, never re-implemented); load_workflow_from_data only on an empty session where there is no history to destroy. Adds/deletes/rewires are accepted freedom; the only refusal is a wire that goes nowhere -- caught by a pre-apply structural trace (apply_workflow_patch validates patch shape, not graph structure) that names the exact node and input BEFORE any session mutation. UI-format files convert via _ui_to_api; unmappable packs refused with names, never guessed. Deterministic sorted summaries in artist words. 16 tests incl. undo-stack growth + restore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…und-trip closes agent/cli.py -- `cozy pull [--file PATH]`: live-canvas ingest by default, saved workflow files (UI or API format) via --file. Exit 0 on applied / initial load / already-match; exit 1 with a human-worded line (always ending in the session-untouched guarantee where true) on unreachable/stale/broken-DAG. Round-trip acceptance: open -> edit in browser -> pull = 2 commands, artist node-add lands as a validated, undoable session patch. 11 CLI tests; full suite 5041 passed, 0 failed; ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ession seam pull_canvas no longer hands raw jsonpatch.make_patch output to the engine -- _build_patch_ops emits whole-value RFC6902 ops (max 3 segments, RFC6901-escaped) so rewires land as real connection changes instead of the reproduced bogus flat 'model/0' input. Defense in depth: _patches_to_mutations now returns None for any /inputs/ path deeper than 3 segments (element-level paths must not flatten), forcing the correct jsonpatch fallback for every caller. New public session seam: export_session_state()/import_session_state() -- deterministic deep-copied snapshot (history capped at 10) restored under the session lock with a fresh engine, malformed input rejected in artist words with the live session untouched. 7 engine-active regression tests incl. the exact reproduced rewire. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ompact execute_with_progress success dicts carry progress_log only when the caller passes include_progress_log=true, downsampled to the last 500 progress entries with a progress_log_truncated marker -- a long video render no longer injects hundreds of KB into every MCP tool result. Error/timeout dicts keep the full log as before. 5 gating tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… undo count find: build_models_report runs _extract_api_format first -- ui_with_api files resolve their real references, ui_only gets "re-export with Save (API Format)" instead of the reproduced false all-clear. intend: a partial recipe (error set) renders "Applied N of M steps, then stopped: <reason>" -- never the success header; multi-step recipes report their real undo depth instead of promising one undo. Recipe mutation batching into a single patch: deferred (touches agent/recipes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… + encoding guard agent/verbs/_cli_state.py -- the CLI session sidecar: persist() snapshots via the new workflow_patch seam to sessions/_cli_verbs.session.json (atomic, deterministic); restore() rehydrates it, corrupt files degrade to a human note. open/pull/see/run --recipe restore first and persist after mutations, so the canvas round-trip and the undo promise are true across separate cozy invocations -- reproduced live with two real processes. _recipe_run loads --workflow through the session seam (_load_workflow) instead of the analysis-only parser; the test that mocked the bug in is unmocked. stdout/ stderr reconfigure to utf-8/replace when the stream can't encode the glyphs -- cozy doctor > file.txt no longer dies in a UnicodeEncodeError traceback, so the CI-gating promise holds. Partial recipes exit 1 without executing; telemetry rail passes include_progress_log=true. Full suite 5083 passed, 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesThe release adds artist-facing FIND, INTEND, OPEN, OPEN-IN, SEE, and OWN CLI verbs; persistent workflow sessions; deterministic terminal telemetry rendering; safer RFC6902 patch conversion; opt-in progress logs; UTF-8 stream handling; release metadata; and read-only Git rules for agent instructions. Artist verb engines
Session and rendering support
Agent boundaries
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Artist
participant CLI
participant VerbEngine
participant SessionSidecar
participant ComfyUI
Artist->>CLI: Run artist verb
CLI->>SessionSidecar: Restore saved workflow state
CLI->>VerbEngine: Execute FIND, INTEND, OPEN, PULL, SEE, or OWN
VerbEngine->>ComfyUI: Read canvas, validate, execute, or query telemetry
VerbEngine->>SessionSidecar: Persist updated workflow state
SessionSidecar-->>CLI: Return persistence note
CLI-->>Artist: Render structured result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
agent/verbs/own.py-126-139 (1)
126-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport unreadable
Custom_Nodesseparately from a missing bridge pack.Line 128 treats permission and I/O failures as an empty directory, producing the incorrect “install the pack” remedy. Return a human-readable access error instead.
Proposed fix
try: packs = [p.name for p in cn_dir.iterdir() if p.is_dir()] - except OSError: - packs = [] + except OSError: + return _check( + "bridge_pack", + False, + f"Could not read the Custom_Nodes folder at {cn_dir}.", + "Check that the folder exists and is readable, then run `cozy doctor` again.", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/verbs/own.py` around lines 126 - 139, Update the Custom_Nodes directory handling around the packs discovery in the bridge-pack check to distinguish an unreadable directory from an empty or missing one: catch the OSError details and return a human-readable access/error result with an appropriate remedy instead of treating it as no installed pack. Preserve the existing bridge_pack success and missing-pack checks for readable directories.agent/verbs/see.py-215-236 (1)
215-236: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSynchronize the trigger lifecycle and retain failed subscriptions for retry.
Concurrent
install()calls can register two callbacks while retaining only one ID. Additionally,uninstall()clears the ID even when unregistering raises, permanently leaking the callback. Protect_trigger_idwith a lifecycle lock and clear it only after successful removal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/verbs/see.py` around lines 215 - 236, Update the SEE subscriber lifecycle in install() and uninstall() to synchronize access to _trigger_id with a lifecycle lock, ensuring concurrent installs register at most one callback and retain the active ID. In uninstall(), clear _trigger_id only after unregister succeeds; leave it unchanged when removal raises so a later call can retry.agent/tools/comfy_execute.py-877-882 (1)
877-882: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject invalid boolean values instead of enabling progress logs.
None,"invalid", lists, and dictionaries currently coerce toTrue, unexpectedly adding up to 500 log entries to the result. Accept explicit booleans and recognized true/false strings, otherwise return a human-readable validation error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/tools/comfy_execute.py` around lines 877 - 882, Update the include_progress_log parsing near _raw_log and include_progress_log to accept booleans and recognized true/false string values only; reject None, invalid strings, lists, and dictionaries with a human-readable validation error instead of enabling progress logging, while preserving the existing valid-value behavior.agent/verbs/see.py-67-78 (1)
67-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not report multi-step gaps as a single step duration.
When progress jumps from value 1 to 3, the two-step interval is appended as one step, producing an incorrect count and average. Only accept consecutive values; a gap should reseed telemetry.
Proposed fix
- if curr.node_id == prev.node_id and curr.value > prev.value: + if curr.node_id == prev.node_id and curr.value == prev.value + 1: out.append(max(0.0, curr.at_s - prev.at_s))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/verbs/see.py` around lines 67 - 78, Update _durations to accept a sample pair only when curr.value is exactly prev.value + 1, rather than any increase; when a value gap occurs, reseed telemetry so the skipped interval produces no duration and later consecutive steps remain measurable.agent/verbs/_cli_state.py-87-99 (1)
87-99: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInclude snapshot serialization in the guarded operation.
allow_nan=Falsecan raiseValueErroron workflow data before thetry, contradictingpersist()’s “Never raises” contract and skipping its user-facing warning.Proposed fix
- snapshot = export_session_state() - content = json.dumps(snapshot, indent=2, sort_keys=True, allow_nan=False) - path = sidecar_path() try: + snapshot = export_session_state() + content = json.dumps(snapshot, indent=2, sort_keys=True, allow_nan=False) + path = sidecar_path() path.parent.mkdir(parents=True, exist_ok=True) _atomic_write(path, content) - except (OSError, ValueError) as exc: + except (OSError, TypeError, ValueError) as exc:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/verbs/_cli_state.py` around lines 87 - 99, Move snapshot serialization, including json.dumps in persist(), inside the existing try block so ValueError is handled by the established warning and user-facing fallback. Preserve the current atomic write behavior and “Never raises” contract for both serialization and filesystem failures.agent/_render.py-130-142 (1)
130-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the singular “step” when
n == 1.Line 142 produces
1 steps;tests/test_render.pyLine 217 currently codifies that user-facing typo.Proposed fix
total = sum(vals) avg = total / n - return f"{n} steps · {avg:.2f} s/it avg · {total:.1f} s total" + unit = "step" if n == 1 else "steps" + return f"{n} {unit} · {avg:.2f} s/it avg · {total:.1f} s total"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/_render.py` around lines 130 - 142, Update format_step_times to use “step” when n == 1 and retain “steps” for all other counts; adjust the corresponding test expectation in tests/test_render.py.agent/_render.py-56-63 (1)
56-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the final observation when downsampling.
For
m > n,int(i * m / n)never selects the final item; a render ending in a spike can therefore hide it.Proposed fix
def _resample(values: list[float], n: int) -> list[float]: """Nearest-neighbor resample to exactly n points (deterministic).""" m = len(values) if m == 0 or n <= 0: return [] if m == n: return list(values) - return [values[min(m - 1, int(i * m / n))] for i in range(n)] + if n == 1: + return [values[m // 2]] + scale = (m - 1) / (n - 1) + return [values[int(i * scale + 0.5)] for i in range(n)]Add a regression asserting that downsampling includes the final sample.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/_render.py` around lines 56 - 63, Update _resample so downsampling when m > n always selects the final input observation while still returning exactly n points and preserving nearest-neighbor behavior. Add a regression test asserting that the final sample appears in the downsampled result, including a case where the final value is a spike.agent/cli.py-1266-1270 (1)
1266-1270: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch
UnicodeErrorin workflow loaders_read_workflow_jsonalready handlesOSError, but both it and_load_workflowstill missUnicodeErrorfromread_text(encoding="utf-8"), so non-UTF-8 files can leak a traceback. Add regression tests for the direct load path andcozy run --recipe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/cli.py` around lines 1266 - 1270, Update the workflow JSON loaders, including _read_workflow_json and _load_workflow, to catch UnicodeError alongside the existing file-read and JSON parsing exceptions when decoding UTF-8 content, preserving the current user-facing error and exit behavior. Add regression tests covering both the direct loading path and cozy run --recipe with non-UTF-8 workflow files.Source: Coding guidelines
🧹 Nitpick comments (2)
agent/verbs/intend.py (1)
40-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComplete the Python type annotations across the INTEND implementation and tests.
agent/verbs/intend.py#L40-L51: parameterize structured mappings, such asdict[str, Any], throughout the module.tests/test_verbs_intend.py#L32-L107: annotate fixture returns, nested helper parameters, fixture arguments, and test returns.As per coding guidelines, “Use Python 3.10+ type hints throughout the Python codebase.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/verbs/intend.py` around lines 40 - 51, Complete Python 3.10+ annotations throughout the INTEND implementation: update _recipe_info and other structured mapping return types in agent/verbs/intend.py to use parameterized mappings such as dict[str, Any]. In tests/test_verbs_intend.py, annotate fixture returns, nested helper parameters, fixture arguments, and test return types across the specified range; apply the required changes at both affected sites.Source: Coding guidelines
tests/test_verbs_find.py (1)
18-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Python 3.10+ annotations to the new test callables.
Annotate fixture parameters and returns, helper and nested mock signatures, and test methods with
-> None.
tests/test_verbs_find.py#L18-L67: annotate all fixtures and theirtmp_pathparameters.tests/test_verbs_find.py#L75-L389: annotate test methods and fixture parameters.tests/test_verbs_own.py#L19-L102: annotate fixtures, helpers, andfake_handle.tests/test_verbs_own.py#L110-L352: annotate test methods and fixture parameters.As per coding guidelines,
**/*.py: “Use Python 3.10+ type hints throughout the Python codebase.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_verbs_find.py` around lines 18 - 67, Add Python 3.10+ type annotations throughout the specified test callables: annotate fixture parameters such as tmp_path, fixture return values, helper and nested mock signatures, fake_handle, and all test methods with -> None where applicable. Apply these updates in tests/test_verbs_find.py ranges 18-67 and 75-389, and tests/test_verbs_own.py ranges 19-102 and 110-352; preserve the existing test behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agents/cozy-crucible.md:
- Around line 17-18: Add a conductor/harness enforcement gate that blocks
state-mutating Git commands while allowing the documented read-only inspection
commands, and align the agent wording with that enforced contract. Apply the
corresponding documentation update at .claude/agents/cozy-crucible.md lines
17-18, .claude/agents/cozy-provisioner.md lines 17-18, and
.claude/agents/cozy-scout.md lines 17-18; all three sites require the same
alignment, while the root enforcement belongs in the existing runtime validation
path.
In `@agent/_render.py`:
- Around line 24-35: Broaden the input annotations for _finite,
braille_sparkline, block_sparkline, and format_step_times from list[float] to a
sequence-based type such as Sequence[Any], reflecting their support for None and
other float()-coercible values while preserving their existing behavior.
In `@agent/cli.py`:
- Around line 155-192: Annotate the new Typer callback run in agent/cli.py with
its appropriate return type, and add -> None to every new test method and
fixture in agent/cli.py, tests/test_render.py lines 20-22,
tests/test_cli_verbs2.py lines 23-28, tests/test_cli_pull.py lines 21-26, and
tests/test_cli_verbs.py lines 21-25. Preserve all existing behavior.
In `@agent/tools/workflow_patch.py`:
- Around line 1327-1364: Validate the snapshot’s current_workflow,
base_workflow, and every history entry with the normal graph/DAG validation
before acquiring state._lock or replacing live state. Reject empty or malformed
graphs and invalid history entries using the existing validation path, while
preserving the current restore behavior only for fully valid snapshots.
- Around line 462-472: Update the path parsing in the mutation logic around
parts and mutations.setdefault so it preserves JSON Pointer token boundaries:
split the original path without stripping slashes, accept only the exact
/node/inputs/param shape, reject element-level or empty-token paths including
trailing-slash variants, and decode JSON Pointer tokens (~0 and ~1) before using
node_id and param_name.
In `@agent/verbs/find.py`:
- Around line 317-352: The workflow inventory in the on-disk indexing and
reference evaluation flow must retain each model’s existing status, including
zero-byte files, instead of storing only the model type. Update the on_disk
entries and the lookup in the refs loop so that a zero-byte model status is
propagated before applying family-mismatch logic, preserving the existing
attention status rather than reporting it as ok.
In `@agent/verbs/intend.py`:
- Around line 245-251: Update the error rendering in the result-handling block
to sanitize engine errors before interpolating them into the “Applied … then
stopped” message. Preserve known domain errors, but replace traceback-shaped or
multiline internal values with a concise human-readable fallback; keep the
existing step counts and stop-reason behavior unchanged.
- Around line 183-205: The exception path around recipe application must
distinguish decoding failures after mutation from failures before any change.
Use the existing before snapshot and _session_workflow() inside the except block
to compute landed changes; report those changes and a partial-failure status
when mutations are observable, while retaining the unchanged “nothing was
changed” response only when the diff is empty.
In `@agent/verbs/own.py`:
- Around line 237-249: Update the outcome-counting loop over *_outcomes.jsonl to
read each file incrementally line by line instead of using
path.read_text(encoding="utf-8").splitlines(). Preserve skipping blank lines,
ignoring malformed JSON, counting valid records, and continuing on OSError.
In `@agent/verbs/pull_canvas.py`:
- Around line 350-395: The pull flow in agent/verbs/pull_canvas.py, centered on
_get_state(), _build_patch_ops(), and handle("apply_workflow_patch"), must
protect the workflow snapshot, comparison, and load or patch application as one
atomic operation, or use revision-based compare-and-swap; update
agent/verbs/open_canvas.py at lines 64-72 to copy the workflow while holding
_get_state()._lock before passing it to the bridge.
- Around line 44-50: Update _normalize_graph to validate every graph entry and
reject the entire pull when any node is not a dictionary or lacks class_type,
rather than filtering malformed nodes out. Propagate the validation failure
through the complete pull flow, including the path around lines 318-329, so
malformed existing nodes cannot be interpreted as deletions.
- Around line 134-151: Restrict pull_canvas’s diff generation to existing nodes
with unchanged node IDs, types, and non-input fields: reject or fail structural
additions, removals, replacements, and node-type changes instead of emitting
whole-node operations, while retaining parameter and connection input updates.
In tests/test_verbs_pull.py:50-84, update coverage to assert structural edits
are refused while input changes succeed; in tests/test_verbs_pull.py:281-296,
remove the expectation that adding node 9 succeeds after link repair and assert
rejection instead.
- Around line 53-68: Extend graph validation beyond _broken_links to detect
cycles in the connection graph before accepting changes. Ensure both the
initial-load path and the patch-application path reject cyclic workflows before
storing the graph or creating the optional engine, while preserving existing
dangling-link validation and error reporting.
- Around line 248-254: Update the JSON loading block around Path.read_text and
json.loads to catch OSError and UnicodeError in addition to JSONDecodeError,
returning a user-readable error tuple for each file-access or decoding failure
instead of allowing raw exceptions to escape. Preserve the existing
truncated-file guidance for JSONDecodeError.
In `@agent/verbs/see.py`:
- Line 81: Complete the Python 3.10+ annotations across all specified sites: in
agent/verbs/see.py:81, update step_durations_from_progress_log to use dict[str,
Any]; in agent/tools/comfy_execute.py:363-368, parameterize workflow and result
mappings; in tests/test_verbs_see.py:64, add -> None to tests and parameterize
helper results; and in tests/test_comfy_execute.py:358-360, type the fixture
argument and add return annotations to tests.
---
Minor comments:
In `@agent/_render.py`:
- Around line 130-142: Update format_step_times to use “step” when n == 1 and
retain “steps” for all other counts; adjust the corresponding test expectation
in tests/test_render.py.
- Around line 56-63: Update _resample so downsampling when m > n always selects
the final input observation while still returning exactly n points and
preserving nearest-neighbor behavior. Add a regression test asserting that the
final sample appears in the downsampled result, including a case where the final
value is a spike.
In `@agent/cli.py`:
- Around line 1266-1270: Update the workflow JSON loaders, including
_read_workflow_json and _load_workflow, to catch UnicodeError alongside the
existing file-read and JSON parsing exceptions when decoding UTF-8 content,
preserving the current user-facing error and exit behavior. Add regression tests
covering both the direct loading path and cozy run --recipe with non-UTF-8
workflow files.
In `@agent/tools/comfy_execute.py`:
- Around line 877-882: Update the include_progress_log parsing near _raw_log and
include_progress_log to accept booleans and recognized true/false string values
only; reject None, invalid strings, lists, and dictionaries with a
human-readable validation error instead of enabling progress logging, while
preserving the existing valid-value behavior.
In `@agent/verbs/_cli_state.py`:
- Around line 87-99: Move snapshot serialization, including json.dumps in
persist(), inside the existing try block so ValueError is handled by the
established warning and user-facing fallback. Preserve the current atomic write
behavior and “Never raises” contract for both serialization and filesystem
failures.
In `@agent/verbs/own.py`:
- Around line 126-139: Update the Custom_Nodes directory handling around the
packs discovery in the bridge-pack check to distinguish an unreadable directory
from an empty or missing one: catch the OSError details and return a
human-readable access/error result with an appropriate remedy instead of
treating it as no installed pack. Preserve the existing bridge_pack success and
missing-pack checks for readable directories.
In `@agent/verbs/see.py`:
- Around line 215-236: Update the SEE subscriber lifecycle in install() and
uninstall() to synchronize access to _trigger_id with a lifecycle lock, ensuring
concurrent installs register at most one callback and retain the active ID. In
uninstall(), clear _trigger_id only after unregister succeeds; leave it
unchanged when removal raises so a later call can retry.
- Around line 67-78: Update _durations to accept a sample pair only when
curr.value is exactly prev.value + 1, rather than any increase; when a value gap
occurs, reseed telemetry so the skipped interval produces no duration and later
consecutive steps remain measurable.
---
Nitpick comments:
In `@agent/verbs/intend.py`:
- Around line 40-51: Complete Python 3.10+ annotations throughout the INTEND
implementation: update _recipe_info and other structured mapping return types in
agent/verbs/intend.py to use parameterized mappings such as dict[str, Any]. In
tests/test_verbs_intend.py, annotate fixture returns, nested helper parameters,
fixture arguments, and test return types across the specified range; apply the
required changes at both affected sites.
In `@tests/test_verbs_find.py`:
- Around line 18-67: Add Python 3.10+ type annotations throughout the specified
test callables: annotate fixture parameters such as tmp_path, fixture return
values, helper and nested mock signatures, fake_handle, and all test methods
with -> None where applicable. Apply these updates in tests/test_verbs_find.py
ranges 18-67 and 75-389, and tests/test_verbs_own.py ranges 19-102 and 110-352;
preserve the existing test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb853971-d29b-40a7-8667-63aafdf534ad
📒 Files selected for processing (30)
.claude/agents/cozy-crucible.md.claude/agents/cozy-provisioner.md.claude/agents/cozy-scout.mdCHANGELOG.mdagent/__init__.pyagent/_render.pyagent/cli.pyagent/tools/comfy_execute.pyagent/tools/workflow_patch.pyagent/verbs/__init__.pyagent/verbs/_cli_state.pyagent/verbs/find.pyagent/verbs/intend.pyagent/verbs/open_canvas.pyagent/verbs/own.pyagent/verbs/pull_canvas.pyagent/verbs/see.pytests/test_cli_pull.pytests/test_cli_state.pytests/test_cli_verbs.pytests/test_cli_verbs2.pytests/test_comfy_execute.pytests/test_render.pytests/test_verbs_find.pytests/test_verbs_intend.pytests/test_verbs_open.pytests/test_verbs_own.pytests/test_verbs_pull.pytests/test_verbs_see.pytests/test_workflow_patch.py
Comment on lines
+17
to
+18
| > **Git is conductor-only (ORCH.L1).** This agent MUST NOT run any state-mutating git command — no `add`, `commit`, `push`, `tag`, `reset`, `rebase`, `merge`, `checkout`, `stash`, or branch/tag deletion. Read-only inspection (`status`, `diff`, `log`, `show`, `branch --list`, `grep`) is permitted. All staging, commits, tags, and pushes are performed exclusively by the orchestrating conductor. Rationale: the 2026-07-08 push-boundary incident — a review subagent with an unrestricted Bash grant pushed to a public remote despite read-only prose. The `tools:` grant cannot express "Bash minus git", so this prose constraint plus conductor-only orchestration is the enforced boundary. | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files '.claude/agents/cozy-crucible.md' '.claude/agents/cozy-provisioner.md' '.claude/agents/cozy-scout.md' '.claude' | sed -n '1,200p'
printf '\n== Outline: agent files ==\n'
for f in .claude/agents/cozy-crucible.md .claude/agents/cozy-provisioner.md .claude/agents/cozy-scout.md; do
echo "--- $f ---"
wc -l "$f"
sed -n '1,120p' "$f" | nl -ba | sed -n '1,80p'
done
printf '\n== Search for conductor / git boundary enforcement ==\n'
rg -n --hidden --glob '!.git' -e 'conductor-only|state-mutating git|Bash minus git|push-boundary|read-only prose|git.*mutat|deny.*git|forbid.*git|git.*deny|shell.*git' .claude . 2>/dev/null | sed -n '1,240p'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 1040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in .claude/agents/cozy-crucible.md .claude/agents/cozy-provisioner.md .claude/agents/cozy-scout.md; do
echo "===== $f ====="
nl -ba "$f" | sed -n '1,80p'
echo
doneRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- cozy-crucible ---\n'
sed -n '1,60p' .claude/agents/cozy-crucible.md | nl -ba
printf '\n--- cozy-provisioner ---\n'
sed -n '1,60p' .claude/agents/cozy-provisioner.md | nl -ba
printf '\n--- cozy-scout ---\n'
sed -n '1,60p' .claude/agents/cozy-scout.md | nl -ba
printf '\n--- policy refs ---\n'
rg -n --hidden --glob '!.git' -e 'constitution|ORCH\.L1|Bash minus git|conductor-only|state-mutating git|read-only inspection|tools:' .claude . 2>/dev/null | sed -n '1,240p'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in .claude/agents/cozy-crucible.md .claude/agents/cozy-provisioner.md .claude/agents/cozy-scout.md; do
echo "===== $f ====="
awk 'NR>=1 && NR<=40 {printf "%4d %s\n", NR, $0}' "$f"
echo
doneRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 6850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Constitution outline =="
wc -l .claude/COZY_CONSTITUTION.md
sed -n '1,220p' .claude/COZY_CONSTITUTION.md | awk '{printf "%4d %s\n", NR, $0}'
echo
echo "== Search for enforcement / git policy =="
rg -n --hidden --glob '!.git' -e 'git|push|commit|tag|reset|rebase|merge|checkout|stash|conductor|ORCH\.L1|human_gates|_IRREVERSIBLE_ACTIONS|deny|forbid|enforce|tools:' .claude | sed -n '1,260p'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 27034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate runtime files =="
git ls-files | rg '(^|/)(agent/stage/constitution\.py|agent/stage/ratchet\.py|agent/stage/|CLAUDE\.md|\.claude/settings\.json|\.claude/dispatch-phase5b\.md)$' | sed -n '1,200p'
echo
echo "== grep for pre/post checks and git authority map =="
rg -n --hidden --glob '!.git' -e 'run_pre_checks|run_post_checks|Git Authority Map|requires per-call approval|git push|git add|git commit|Bash' agent .claude | sed -n '1,260p'
echo
echo "== any tool gate definitions mentioning Bash/Git =="
rg -n --hidden --glob '!.git' -e 'allowed_tools|tools:' .claude/agents .claude/commands .claude | sed -n '1,260p'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 10251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run_pre_checks / run_post_checks =="
sed -n '400,560p' agent/stage/constitution.py | awk '{printf "%4d %s\n", NR+399, $0}'
echo
echo "== git references in constitution.py =="
rg -n 'git|push|commit|tag|reset|rebase|merge|checkout|stash|branch' agent/stage/constitution.py | sed -n '1,200p'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 4870
Make the Git boundary real in the conductor/harness, not just in agent prose.
The current runtime checks cover role isolation, human gates, and persistence, but not Git mutation, so these three files still depend on a prose-only restriction:
.claude/agents/cozy-crucible.md#L17-L18.claude/agents/cozy-provisioner.md#L17-L18.claude/agents/cozy-scout.md#L17-L18
Add an enforcement gate for state-mutating Git commands and keep the agent wording aligned with the harness contract.
📍 Affects 3 files
.claude/agents/cozy-crucible.md#L17-L18(this comment).claude/agents/cozy-provisioner.md#L17-L18.claude/agents/cozy-scout.md#L17-L18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/agents/cozy-crucible.md around lines 17 - 18, Add a
conductor/harness enforcement gate that blocks state-mutating Git commands while
allowing the documented read-only inspection commands, and align the agent
wording with that enforced contract. Apply the corresponding documentation
update at .claude/agents/cozy-crucible.md lines 17-18,
.claude/agents/cozy-provisioner.md lines 17-18, and .claude/agents/cozy-scout.md
lines 17-18; all three sites require the same alignment, while the root
enforcement belongs in the existing runtime validation path.
Comment on lines
+24
to
+35
| def _finite(values: list[float]) -> list[float]: | ||
| """Return only the finite numeric entries, dropping None/NaN/Inf/non-numbers.""" | ||
| out: list[float] = [] | ||
| for v in values: | ||
| if v is None: | ||
| continue | ||
| try: | ||
| f = float(v) | ||
| except (TypeError, ValueError): | ||
| continue | ||
| if math.isnan(f) or math.isinf(f): | ||
| continue |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline agent/_render.py --items all --type function --view expanded
rg -n -C2 'None|non-numbers|def (_finite|braille_sparkline|block_sparkline|format_step_times)' agent/_render.pyRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 2300
🏁 Script executed:
sed -n '1,220p' agent/_render.py
printf '\n--- tests ---\n'
rg -n -C3 'braille_sparkline|block_sparkline|format_step_times|_finite|_resample' -g '*test*' -g '*.py'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 5134
🏁 Script executed:
rg -n -C2 '\b(_finite|braille_sparkline|block_sparkline|format_step_times)\b' agent . --glob '*.py'Repository: JosephOIbrahim/Comfy-Cozy
Length of output: 15893
Broaden the renderer helper annotations. These helpers accept None and other float()-coercible values at runtime, so list[float] understates the contract. Use a broader input type like Sequence[Any] across _finite, braille_sparkline, block_sparkline, and format_step_times.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/_render.py` around lines 24 - 35, Broaden the input annotations for
_finite, braille_sparkline, block_sparkline, and format_step_times from
list[float] to a sequence-based type such as Sequence[Any], reflecting their
support for None and other float()-coercible values while preserving their
existing behavior.
Source: Coding guidelines
Comment on lines
155
to
192
| def run( | ||
| session: str = typer.Option( | ||
| None, "--session", "-s", | ||
| None, | ||
| "--session", | ||
| "-s", | ||
| help="Named session for memory persistence", | ||
| ), | ||
| verbose: bool = typer.Option( | ||
| False, "--verbose", "-v", | ||
| False, | ||
| "--verbose", | ||
| "-v", | ||
| help="Show debug logging (API timing, tool execution, context management)", | ||
| ), | ||
| model: str = typer.Option( | ||
| None, "--model", | ||
| None, | ||
| "--model", | ||
| help="Model alias or id to use (e.g. 'nemotron', 'claude'). See list_models_available.", | ||
| ), | ||
| provider: str = typer.Option( | ||
| None, "--provider", | ||
| None, | ||
| "--provider", | ||
| help="LLM provider: anthropic|openai|gemini|ollama|nvidia|custom", | ||
| ), | ||
| recipe: str = typer.Option( | ||
| None, | ||
| "--recipe", | ||
| help=( | ||
| "Apply a named look ('dreamier', 'sharper', 'faster'...) to the workflow " | ||
| "and run it -- no chat, no API key. 'list' shows every recipe." | ||
| ), | ||
| ), | ||
| workflow: str = typer.Option( | ||
| None, | ||
| "--workflow", | ||
| "-w", | ||
| help="With --recipe: the workflow JSON file to load and run the recipe on.", | ||
| ), | ||
| ): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep run --pattern 'def $FUNC($$$): $$$' --lang python \
agent/cli.py tests/test_render.py tests/test_cli_verbs2.py \
tests/test_cli_pull.py tests/test_cli_verbs.pyRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 50382
🏁 Script executed:
sed -n '140,210p' agent/cli.py
printf '\n---\n'
sed -n '1,120p' tests/test_render.py
printf '\n---\n'
sed -n '1,80p' tests/test_cli_verbs.py
printf '\n---\n'
sed -n '1,80p' tests/test_cli_pull.py
printf '\n---\n'
sed -n '1,80p' tests/test_cli_verbs2.pyRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 15315
🏁 Script executed:
ast-grep outline agent/cli.py --view expanded
printf '\n---\n'
ast-grep outline tests/test_render.py --view expanded
printf '\n---\n'
ast-grep outline tests/test_cli_verbs.py --view expanded
printf '\n---\n'
ast-grep outline tests/test_cli_pull.py --view expanded
printf '\n---\n'
ast-grep outline tests/test_cli_verbs2.py --view expandedRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 8481
🏁 Script executed:
sed -n '380,520p' agent/cli.py
printf '\n---\n'
sed -n '520,760p' agent/cli.py
printf '\n---\n'
sed -n '760,980p' agent/cli.py
printf '\n---\n'
sed -n '980,1160p' agent/cli.py
printf '\n---\n'
sed -n '1160,1510p' agent/cli.pyRepository: JosephOIbrahim/Comfy-Cozy
Length of output: 42222
Add the missing return annotations
- Add return annotations to the new Typer callbacks in
agent/cli.py. - Add
-> Noneto the new test methods and fixtures intests/test_render.py,tests/test_cli_verbs.py,tests/test_cli_pull.py, andtests/test_cli_verbs2.py.
📍 Affects 5 files
agent/cli.py#L155-L192(this comment)tests/test_render.py#L20-L22tests/test_cli_verbs2.py#L23-L28tests/test_cli_pull.py#L21-L26tests/test_cli_verbs.py#L21-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/cli.py` around lines 155 - 192, Annotate the new Typer callback run in
agent/cli.py with its appropriate return type, and add -> None to every new test
method and fixture in agent/cli.py, tests/test_render.py lines 20-22,
tests/test_cli_verbs2.py lines 23-28, tests/test_cli_pull.py lines 21-26, and
tests/test_cli_verbs.py lines 21-25. Preserve all existing behavior.
Source: Coding guidelines
Comment on lines
462
to
472
| parts = path.strip("/").split("/") | ||
| if len(parts) < 3 or parts[1] != "inputs": | ||
| return None | ||
| if len(parts) > 3: | ||
| # Element-level paths (e.g. /2/inputs/model/0) must not flatten | ||
| # into a bogus literal param name — force the jsonpatch fallback. | ||
| return None | ||
|
|
||
| node_id = parts[0] | ||
| param_name = "/".join(parts[2:]) # Handle nested paths | ||
| param_name = parts[2] | ||
| mutations.setdefault(node_id, {})[param_name] = value |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve JSON Pointer token boundaries.
strip("/") makes /3/inputs/model/ indistinguishable from /3/inputs/model, so an element-level path with an empty final token can incorrectly overwrite the whole input and report success. Split without stripping, require the exact /node/inputs/param shape, and decode ~0/~1 tokens.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/tools/workflow_patch.py` around lines 462 - 472, Update the path
parsing in the mutation logic around parts and mutations.setdefault so it
preserves JSON Pointer token boundaries: split the original path without
stripping slashes, accept only the exact /node/inputs/param shape, reject
element-level or empty-token paths including trailing-slash variants, and decode
JSON Pointer tokens (~0 and ~1) before using node_id and param_name.
Comment on lines
+1327
to
+1364
| current = snapshot.get("current_workflow") | ||
| if current is not None and not isinstance(current, dict): | ||
| return "That session snapshot is damaged: the workflow inside it isn't a graph." | ||
| base = snapshot.get("base_workflow") | ||
| if base is not None and not isinstance(base, dict): | ||
| return "That session snapshot is damaged: its original workflow isn't a graph." | ||
| history = snapshot.get("history", []) | ||
| if not isinstance(history, list) or any(not isinstance(h, dict) for h in history): | ||
| return "That session snapshot is damaged: its undo history isn't readable." | ||
|
|
||
| state = _get_state() | ||
| with state._lock: | ||
| if current is None: | ||
| state["current_workflow"] = None | ||
| state["base_workflow"] = None | ||
| state["history"] = deque(maxlen=_MAX_HISTORY) | ||
| state["validated_since_mutation"] = False | ||
| _set_engine(None) | ||
| return None | ||
|
|
||
| state["current_workflow"] = copy.deepcopy(current) | ||
| state["base_workflow"] = ( | ||
| copy.deepcopy(base) if base is not None else copy.deepcopy(current) | ||
| ) | ||
| restored = deque(maxlen=_MAX_HISTORY) | ||
| for entry in history: | ||
| restored.append(copy.deepcopy(entry)) | ||
| state["history"] = restored | ||
| state["validated_since_mutation"] = bool( | ||
| snapshot.get("validated_since_mutation", False) | ||
| ) | ||
|
|
||
| # Mirror load_workflow_from_data: fresh engine from the restored graph. | ||
| try: | ||
| _set_engine(_create_engine(state["current_workflow"])) | ||
| except Exception as exc: # import already succeeded, engine is optional | ||
| log.warning("Could not create engine on import: %s. Engine disabled.", exc) | ||
| _set_engine(None) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate every restored graph before replacing live state.
A schema-1 snapshot with current_workflow: {} or malformed base_workflow/history dictionaries passes these checks. State is replaced, engine failure is suppressed, and restore reports success; malformed history can likewise surface during undo. Run the normal graph/DAG validation over current, base, and every history entry before acquiring the mutation lock.
As per coding guidelines, “Never delete all nodes, replace an entire workflow JSON, or modify node types; only change inputs and connections, and refuse changes that break the DAG.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/tools/workflow_patch.py` around lines 1327 - 1364, Validate the
snapshot’s current_workflow, base_workflow, and every history entry with the
normal graph/DAG validation before acquiring state._lock or replacing live
state. Reject empty or malformed graphs and invalid history entries using the
existing validation path, while preserving the current restore behavior only for
fully valid snapshots.
Source: Coding guidelines
Comment on lines
+53
to
+68
| def _broken_links(graph: dict) -> list[str]: | ||
| """Return human-worded descriptions of connections that break the DAG. | ||
|
|
||
| A connection whose source node id is absent from the graph is a dangling | ||
| link — executing it would fail, so ingest refuses and explains. Uses the | ||
| same connection tracer the validator uses; output order is deterministic. | ||
| """ | ||
| problems: list[str] = [] | ||
| for conn in _trace_connections(graph): | ||
| if str(conn["from_node"]) not in graph: | ||
| problems.append( | ||
| f"{conn['to_class']} (node {conn['to_node']}) input " | ||
| f"'{conn['to_input']}' points at node {conn['from_node']}, " | ||
| "which is not in the graph" | ||
| ) | ||
| return problems |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate cycles before accepting the graph.
_broken_links only checks whether source IDs exist. A cycle such as 1 → 2 → 1 passes, and the initial-load branch stores it before optional engine creation can reject it. Add explicit cycle detection before either load or patch application.
As per coding guidelines, workflow changes must be refused when they break the DAG.
Also applies to: 332-356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/verbs/pull_canvas.py` around lines 53 - 68, Extend graph validation
beyond _broken_links to detect cycles in the connection graph before accepting
changes. Ensure both the initial-load path and the patch-application path reject
cyclic workflows before storing the graph or creating the optional engine, while
preserving existing dangling-link validation and error reporting.
Source: Coding guidelines
Comment on lines
+134
to
+151
| for nid in sorted(set(current) | set(artist)): | ||
| if nid not in current: | ||
| ops.append({"op": "add", "path": f"/{_esc(nid)}", "value": artist[nid]}) | ||
| continue | ||
| if nid not in artist: | ||
| ops.append({"op": "remove", "path": f"/{_esc(nid)}"}) | ||
| continue | ||
| cur_node, art_node = current[nid], artist[nid] | ||
| if cur_node == art_node: | ||
| continue | ||
| cur_rest = {k: v for k, v in cur_node.items() if k != "inputs"} | ||
| art_rest = {k: v for k, v in art_node.items() if k != "inputs"} | ||
| cur_in = cur_node.get("inputs") | ||
| art_in = art_node.get("inputs") | ||
| if cur_rest != art_rest or not isinstance(cur_in, dict) or not isinstance(art_in, dict): | ||
| # class_type (or any non-input member) changed, or inputs aren't | ||
| # a plain dict on one side — swap the whole node in one honest op. | ||
| ops.append({"op": "replace", "path": f"/{_esc(nid)}", "value": art_node}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The implementation and tests permit structural edits outside the allowed workflow mutation contract.
agent/verbs/pull_canvas.py#L134-L151: reject node-ID, node-type, and non-input changes instead of emitting whole-node operations.tests/test_verbs_pull.py#L50-L84: test parameter and connection updates separately from structural edits, which should be refused.tests/test_verbs_pull.py#L281-L296: remove the expectation that adding node9succeeds after repairing its link.
As per coding guidelines, “Never delete all nodes, replace an entire workflow JSON, or modify node types; only change inputs and connections.”
📍 Affects 2 files
agent/verbs/pull_canvas.py#L134-L151(this comment)tests/test_verbs_pull.py#L50-L84tests/test_verbs_pull.py#L281-L296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/verbs/pull_canvas.py` around lines 134 - 151, Restrict pull_canvas’s
diff generation to existing nodes with unchanged node IDs, types, and non-input
fields: reject or fail structural additions, removals, replacements, and
node-type changes instead of emitting whole-node operations, while retaining
parameter and connection input updates. In tests/test_verbs_pull.py:50-84,
update coverage to assert structural edits are refused while input changes
succeed; in tests/test_verbs_pull.py:281-296, remove the expectation that adding
node 9 succeeds after link repair and assert rejection instead.
Source: Coding guidelines
Comment on lines
+248
to
+254
| try: | ||
| data = json.loads(Path(file).read_text(encoding="utf-8")) | ||
| except json.JSONDecodeError: | ||
| return None, ( | ||
| f"{file} isn't valid JSON — it may be truncated. " | ||
| "Re-save the workflow from ComfyUI and try again." | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Translate file-access and decoding failures into user-readable errors.
read_text can raise OSError or UnicodeError after validation—for example, if the file disappears, lacks permission, or is not UTF-8. Those currently escape as raw tracebacks.
Proposed fix
try:
data = json.loads(Path(file).read_text(encoding="utf-8"))
except json.JSONDecodeError:
return None, (
f"{file} isn't valid JSON — it may be truncated. "
"Re-save the workflow from ComfyUI and try again."
)
+ except UnicodeError:
+ return None, f"{file} isn't valid UTF-8 text."
+ except OSError as exc:
+ return None, f"Couldn't read {file}: {exc}"As per coding guidelines, “Never expose raw tracebacks; translate errors into human-readable messages.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| data = json.loads(Path(file).read_text(encoding="utf-8")) | |
| except json.JSONDecodeError: | |
| return None, ( | |
| f"{file} isn't valid JSON — it may be truncated. " | |
| "Re-save the workflow from ComfyUI and try again." | |
| ) | |
| try: | |
| data = json.loads(Path(file).read_text(encoding="utf-8")) | |
| except json.JSONDecodeError: | |
| return None, ( | |
| f"{file} isn't valid JSON — it may be truncated. " | |
| "Re-save the workflow from ComfyUI and try again." | |
| ) | |
| except UnicodeError: | |
| return None, f"{file} isn't valid UTF-8 text." | |
| except OSError as exc: | |
| return None, f"Couldn't read {file}: {exc}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/verbs/pull_canvas.py` around lines 248 - 254, Update the JSON loading
block around Path.read_text and json.loads to catch OSError and UnicodeError in
addition to JSONDecodeError, returning a user-readable error tuple for each
file-access or decoding failure instead of allowing raw exceptions to escape.
Preserve the existing truncated-file guidance for JSONDecodeError.
Source: Coding guidelines
Comment on lines
+350
to
+395
| current = workflow_patch._get_state()["current_workflow"] | ||
|
|
||
| # No session workflow: initial ingest. There is no undo history, base, or | ||
| # consent flag to destroy — loading here is legitimate (and the only path). | ||
| if current is None: | ||
| origin = "<canvas pull>" if source == "canvas" else str(file) | ||
| load_err = workflow_patch.load_workflow_from_data(artist_graph, source=origin) | ||
| if load_err: | ||
| return _result( | ||
| ok=False, | ||
| pulled=True, | ||
| applied=False, | ||
| source=source, | ||
| node_count=node_count, | ||
| message=f"Couldn't start a session from the pulled graph: {load_err}", | ||
| ) | ||
| return _result( | ||
| ok=True, | ||
| pulled=True, | ||
| applied=True, | ||
| initial_load=True, | ||
| source=source, | ||
| node_count=node_count, | ||
| message=( | ||
| f"No session workflow was loaded, so your graph ({node_count} nodes) " | ||
| "just became the session baseline. Nothing was overwritten — the " | ||
| "session simply starts from what you built." | ||
| ), | ||
| ) | ||
|
|
||
| # Live session: edits re-enter ONLY as a validated patch (ORCH.L8), | ||
| # built at whole-value granularity — never element-level (defect A1). | ||
| patch_ops = _build_patch_ops(current, artist_graph) | ||
| if not patch_ops: | ||
| return _result( | ||
| ok=True, | ||
| pulled=True, | ||
| applied=False, | ||
| source=source, | ||
| node_count=node_count, | ||
| message="Canvas and session already match — nothing to pull.", | ||
| ) | ||
|
|
||
| summary = _summarize_changes(current, artist_graph) | ||
| apply_result = json.loads( | ||
| workflow_patch.handle("apply_workflow_patch", {"patches": patch_ops}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Session workflow reads and writes are not consistently protected by the session lock.
agent/verbs/pull_canvas.py#L350-L395: make snapshot, comparison, and load/patch application one atomic operation or use revision-based compare-and-swap.agent/verbs/open_canvas.py#L64-L72: copy the workflow under_get_state()._lockbefore passing it to the bridge.
As per coding guidelines, “Protect shared mutable state with threading.Lock in workflow patching [and] orchestration.”
📍 Affects 2 files
agent/verbs/pull_canvas.py#L350-L395(this comment)agent/verbs/open_canvas.py#L64-L72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/verbs/pull_canvas.py` around lines 350 - 395, The pull flow in
agent/verbs/pull_canvas.py, centered on _get_state(), _build_patch_ops(), and
handle("apply_workflow_patch"), must protect the workflow snapshot, comparison,
and load or patch application as one atomic operation, or use revision-based
compare-and-swap; update agent/verbs/open_canvas.py at lines 64-72 to copy the
workflow while holding _get_state()._lock before passing it to the bridge.
Source: Coding guidelines
| return out | ||
|
|
||
|
|
||
| def step_durations_from_progress_log(entries: list[dict]) -> list[float]: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete the new Python type annotations across this layer.
agent/verbs/see.py#L81-L81: parameterize progress-log mappings withdict[str, Any].agent/tools/comfy_execute.py#L363-L368: parameterize workflow and result mappings.tests/test_verbs_see.py#L64-L64: add-> Noneto tests and parameterize helper results.tests/test_comfy_execute.py#L358-L360: type the fixture argument and add test return annotations.
As per coding guidelines, “Use Python 3.10+ type hints throughout the Python codebase.”
📍 Affects 4 files
agent/verbs/see.py#L81-L81(this comment)agent/tools/comfy_execute.py#L363-L368tests/test_verbs_see.py#L64-L64tests/test_comfy_execute.py#L358-L360
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/verbs/see.py` at line 81, Complete the Python 3.10+ annotations across
all specified sites: in agent/verbs/see.py:81, update
step_durations_from_progress_log to use dict[str, Any]; in
agent/tools/comfy_execute.py:363-368, parameterize workflow and result mappings;
in tests/test_verbs_see.py:64, add -> None to tests and parameterize helper
results; and in tests/test_comfy_execute.py:358-360, type the fixture argument
and add return annotations to tests.
Source: Coding guidelines
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
1 participant
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.
v5.9.0 — The Artist Verbs: local-first CLI surface + browser canvas round-trip
Executes the ratified CLI-ORCH harness end-to-end: five artist verbs, all
offline for core operation (loopback-only to the local ComfyUI, zero accounts,
zero API keys on any verb rail), plus the canvas round-trip that hands the
graph to the artist in the browser and takes their edits back as validated,
undoable patches.
The surface
cozy models list·cozy nodes list·cozy findcozy run --recipe dreamier|sharper|…cozy seecozy doctor·cozy stats·cozy model searchcozy open·cozy pullA cross-process session sidecar makes the round-trip real across separate
cozyinvocations: workflow, undo history, and baseline survive betweencommands via an atomic snapshot.
Verification
tests across engines, CLI wiring, and regression guards). Ruff clean.
finding): 22 raw findings → 10 confirmed (8 distinct, 4 critical), all with
live repros — then fixed in 4 file-disjoint lanes and re-verified 8/8
dead by re-running the original repros. Highlights: element-level patch
paths flattening into bogus inputs (rewire corruption), process-lifetime
session state (round-trip broken across invocations), analysis-only loads on
the recipe rail, UnicodeEncodeError on piped output.
agent/stage/**untouched.Deliberate deferrals
model searchis local-disk only.agent/recipes).--steps 4unicode image preview (SEE's remaining slice).16 commits, engine-per-lane history. Harness:
HARNESS_CLI_ORCH_20260718.md(local control doc); run ledger in
tooling/harness/cliorch/LEDGER.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ln2LbRfjDzDgE5Ab6x69Un
Summary by CodeRabbit