Repository navigation
fix(workflow): persist Cloud sidebar files and verify save identity - #1046
wealthwiselabs wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Save naming, overwrite, and command contract comfy_cli/command/workflow.py, comfy_cli/error_codes.py, CHANGELOG.md, comfy_cli/skills/comfy/SKILL.md, tests/comfy_cli/command/test_workflow_saved.py |
Save input validation handles workflow paths and overwrite conflicts. Command help, error descriptions, changelog, skill instructions, and local-save tests cover file-backed saves and overwrite behavior. |
Cloud file save and record verification comfy_cli/command/workflow.py, comfy_cli/error_codes.py, tests/comfy_cli/command/test_workflow_saved.py, tests/comfy_cli/command/test_pretty_print_sanitize.py |
Cloud saves write a userdata workflow file. Record IDs and versions are returned only after matching names and workflow content are verified. Tests cover unresolved records, warnings, and save output. |
Cloud file and record deletion comfy_cli/command/workflow.py, tests/comfy_cli/command/test_workflow_saved.py |
Cloud deletion verifies the file and record association before cleanup. Tests cover ambiguous associations, record-only workflows, and incomplete cleanup. |
Run command test fixtures
| Layer / File(s) | Summary |
|---|---|
Run workflow and object-info fixtures tests/comfy_cli/command/test_run.py, tests/comfy_cli/command/test_run_json.py, tests/comfy_cli/command/test_run_prompt.py |
Run tests now use EmptyImage workflow nodes and updated object-info fixtures. Prompt tests load frozen catalog data and build the default workflow. |
Other test updates
| Layer / File(s) | Summary |
|---|---|
Validation and certificate assertions tests/comfy_cli/output/test_error_envelope_where.py, tests/comfy_cli/test_http.py |
Validation tests mock object-info loading and check the route mode. HTTP tests compare SSL context certificates with the platform fixture. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant CLI
participant UserdataAPI
participant WorkflowAPI
CLI->>UserdataAPI: POST workflow file
UserdataAPI-->>CLI: Return file receipt
CLI->>WorkflowAPI: List records matching the filename
WorkflowAPI-->>CLI: Return candidate records
CLI->>WorkflowAPI: Fetch candidate workflow content
WorkflowAPI-->>CLI: Return workflow JSON
CLI-->>CLI: Return verified metadata or an unresolved-record warning
sequenceDiagram
participant CLI
participant UserdataAPI
participant WorkflowAPI
CLI->>WorkflowAPI: Read record and verify ownership
WorkflowAPI-->>CLI: Return record details
CLI->>UserdataAPI: Read and verify associated file
UserdataAPI-->>CLI: Return file content
CLI->>UserdataAPI: Delete verified file
UserdataAPI-->>CLI: Return file deletion result
CLI->>WorkflowAPI: Delete associated record
WorkflowAPI-->>CLI: Return record deletion result
Possibly related PRs
- Comfy-Org/comfy-cli#1008: Both changes persist Cloud workflows as files, verify corresponding workflow records, and coordinate file and record deletion.
Suggested reviewers: skishore23, mattmillerai
Merge Risk: 
45b87
-
Cloud workflow saves now write sidebar-visible files and return a record ID only after verifying it. Deletion refuses ambiguous associations. No concrete merge-blocking issue was found. The summaries note that no clean full-suite or live Cloud run is claimed, but that is normal validation uncertainty and not a specific defect.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. Architecture Decision Compliance The PR introduces a durable Cloud workflow file/record ownership model and deletion invariant in comfy_cli/command/workflow.py(the new Cloud save path at lines 1507-1520 and Cloud delete associatio…Add and accept an ADR for the Cloud workflow storage model. Document the sidebar file and mirrored record relationship, exact name/content identity checks, ownership rules, overwrite behavior, partial-cleanup semantics, backend atomicity li… -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: persisting Cloud sidebar files and verifying save identity. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 40 functions across 9 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Architecture Decision Compliance
-
Explanation
The PR introduces a durable Cloud workflow file/record ownership model and deletion invariant in
comfy_cli/command/workflow.py(the new Cloud save path at lines 1507-1520 and Cloud delete association checks at lines 1579-1644). The changed-file inventory contains no ADR addition or amendment. The description explains the backend's lack of atomic conditional deletion, but it does not explain why this durable boundary is local and reversible. No existing accepted ADR directly covers this Cloud storage model. The machine-output changes remain consistent with ADR-AUTOMATION-0003.Resolution
Add and accept an ADR for the Cloud workflow storage model. Document the sidebar file and mirrored record relationship, exact name/content identity checks, ownership rules, overwrite behavior, partial-cleanup semantics, backend atomicity limits, compatibility contract, and measurable revisit triggers.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
🎉 Thank you for your contribution, we really appreciate it! 🎉 Like many open source projects, we require contributors to sign our Contributor License Agreement (CLA). A CLA makes the ownership of contributions explicit, so contributors and the project share a clear understanding of how the code can be used. By signing, you:
CLAs are standard practice across major open source projects including those under the Apache Software Foundation and the Linux Foundation. Ours is based on the Apache Software Foundation's CLA. Most importantly, it would enable us to relicense the project under a more permissive license in the future, giving the project and its community greater flexibility. ✍ To sign, please post a new comment on this PR with exactly the following text: ✍ I have read and agree to the Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
Swarmhost agentic reviewThe detailed evaluation is available to employees in the internal Slack review thread. Evaluation failed to run ( Evaluation budget remaining for this pull request: 15 automatic and 15 manual. Updated by Swarmhost's agentic review process. |
Swarmhost agentic reviewAn internal review is in progress. Detailed evaluations are not published on public repositories. Evaluation budget remaining for this pull request: 15 automatic and 15 manual. Updated by Swarmhost's agentic review process. |
skishore23
left a comment
skishore23
left a comment
There was a problem hiding this comment.
KishoreBot (AI), reviewing at Kishore's request. Reviewed commit 1a96cf3.
[P2] Preserve JSON value types when verifying mirrored content.
In _find_mirrored_record, Python's recursive dict/list equality considers JSON true equal to 1 (and false equal to 0). For example, sidebar content {"nodes":[{"id":1,"widgets_values":[1]}]} and record content {"nodes":[{"id":1,"widgets_values":[true]}]} pass the current comparison despite different widget values/types.
With a unique same-name record owned by the current user and a stale mirror after best-effort synchronization, save can report the stale record's UUID as verified. More seriously, _cloud_delete uses this helper to establish content identity before deleting the sidebar file and the record; this mismatch passes its refusal guard. The record-ID and owner checks do not distinguish these two contents.
Please use a recursive JSON comparison that distinguishes booleans from numbers, and add nested bool/number mismatch regressions. Save should withhold the verified record ID; delete should refuse the mismatch without either DELETE request. The integrated #1008 has a type-aware _same_json helper for a similar check that may be reusable.
Evidence: static trace through save, mirrored-record lookup and delete, plus inspection of the regression tests; this reproduction was not executed against Cloud. No independent runtime tests were run. Unit/lint/validation checks currently pass, but CLA fails and build/platform checks are still pending. Requesting changes for the content-identity defect.
comfy workflow save --where cloudcreated a workflow record without the userdata file listed by the Cloud sidebar. Save now writesworkflows/<name>.jsonand returns a record ID only after unique-name and matching-content read-back. A failed/stale mirror leaves the successful file save intact with an explicit null-ID warning.--no-overwrite; documents same-name replacement and the ignored description field. Hidden names and the favorites index are refused.Backend behavior was checked read-only against the userdata and workflow service implementations. Tests cover save/list/get/delete, stale mirroring, duplicate names, another member's record, and partial failure. No live Cloud writes were performed. Filename association and deletion remain separate API calls; the backend has no atomic conditional-delete operation.
Validation
git diff --checkpass.Related skill: #1008. This fix is independent and targets
main.Monitoring follow-up (October 27)
Verify mirrored workflow JSON recursively with JSON type semantics. Boolean/number differences (
truevs1,falsevs0, including nested values) withhold the saved record ID and refuse deletion before either DELETE request. Equivalent numeric representations (1and1.0) and reordered object keys remain compatible. Twelve regressions reproduced stale IDs and unsafe deletions before the fix.The identity fix is isolated in
fe4acd5f69. A separate test-only commit45b878ef46corrects invalid LATENT-to-IMAGE success fixtures, mocks the actual catalog/loader transport boundaries, and compares actual platform trust-root sets. Validation remains active and real unexpected catalog calls fail; no production behavior, dependencies or checks were relaxed. The existing user-authored main merge is preserved.Fresh validation supersedes the earlier non-clean broad-suite snapshot above: 10,949 passed, 40 skipped, zero failures or test-file exclusions, using lockfile-matching dependencies and verified imports from this checkout. All 641 focused tests, full Ruff lint/format, ADR validation, whitespace checks and normal commit hooks pass. The exact source was frozen throughout the full run and the committed fixture patch matches its start/end checksum. CLA remains a contributor action; no authorship or allowlist changes.
Summary by CodeRabbit
--no-overwriteto prevent replacement.