refactor(storage): remove the now-inert Action::Compare variant#3220
Conversation
Follow-up to #3216 (which removed the state-based Comparisons sync). After that, nothing constructs `Action::Compare` — its only producers (`compare_trees` and the `apply_action` pushes) are gone — so the variant is dead. Remove it and the ~20 defensive match arms across storage / context / node / op-adapter / runtime / tools that only existed to handle it. Wire compatibility is preserved. `Action` previously derived borsh with positional tags (Add=0, Compare=1, DeleteRef=2, Update=3). To avoid renumbering `DeleteRef`/`Update` — which would silently break every persisted and in-flight delta — `Action` now has hand-rolled `BorshSerialize`/`BorshDeserialize` that keep Add=0, DeleteRef=2, Update=3 and reject tag 1. Same approach #3216 used for `StorageDelta`. A new `action.rs` test pins the tag bytes, roundtrips each surviving variant, and asserts tag 1 is rejected. Also fixes now-stale comments (store.rs, concurrency.rs, the stale-update test) that referenced the removed `Action::Compare` push. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 2 agents | Quality score: 57% | Review time: 611.1s
💡 Suggestion (1)
1. Stale doc comment still references the removed Action::Compare variant
File: crates/storage/src/interface.rs (line 1108-1122) | Consensus: 1/2 agents
The doc comment on Interface::apply_action still says "Generates Compare action for hash verification after applying changes" and lists ActionNotAllowed as the error "if Compare action is passed directly". This PR removes the Action::Compare variant entirely (see crates/storage/src/action.rs), so both statements are now false/misleading: nothing generates a Compare action anymore, and passing one is impossible (the enum no longer has that variant, and a wire-level tag 1 is now rejected at deserialization rather than reaching apply_action as ActionNotAllowed). This is exactly the kind of comment this PR's own description says it fixed elsewhere (store.rs, tests/concurrency.rs, the stale-update test), but this occurrence was missed.
Suggested fix:
Update the doc comment, e.g. remove the "Generates Compare action..." line and drop/rephrase the `ActionNotAllowed` bullet to reflect the current error surface for apply_action.
Found by: security-reviewer
🤖 Generated by AI Code Reviewer | Review ID: review-686913a0
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 2 agents | Quality score: 56% | Review time: 645.4s
💡 Suggestion (2)
1. Stale doc comment still references the removed Action::Compare mechanism
File: crates/storage/src/interface.rs (line 1108-1108) | Consensus: 1/2 agents
The doc comment on apply_action still says "Generates Compare action for hash verification after applying changes," but Action::Compare and the compare/reconciliation mechanism it describes have been removed by this PR. This PR's own description states it fixes stale comments referencing the removed Action::Compare push in store.rs, tests/concurrency.rs, and the stale-update test, but this occurrence (and the ones below) in interface.rs were missed, leaving misleading documentation about how apply_action works.
Suggested fix:
Remove or rewrite the sentence to reflect that no Compare/hash-verification action is generated anymore.
Found by: security-reviewer
2. Stale # Errors doc entry for removed Compare rejection path
File: crates/storage/src/interface.rs (line 1122-1122) | Consensus: 1/2 agents
The # Errors section of apply_action still documents ActionNotAllowed as occurring "if Compare action is passed directly." The corresponding code (Action::Compare { .. } => return Err(StorageError::ActionNotAllowed("Compare".to_owned()))) was removed by this PR since the variant no longer exists, so this doc entry is now inaccurate and should be deleted.
Suggested fix:
Delete the stale bullet point from the `# Errors` doc list.
Found by: security-reviewer
🤖 Generated by MeroReviewer | Review ID: review-f1c3c86b
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
…pare removal Addresses the AI-review finding on #3220: the `Interface::apply_action` doc still claimed it "Generates Compare action for hash verification" and listed `ActionNotAllowed` as occurring "if Compare action is passed directly". Both are now false — the variant is gone. Drop the Compare sentence and reword the `ActionNotAllowed` bullet to the real surface (storage-type access violations, e.g. a Frozen delete or an unauthorized Shared/User write). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the AI-review findings in a9c57ae:
Non-actionable:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 2 agents | Quality score: 56% | Review time: 523.7s
🟡 Warning (1)
1. Stale comment references removed Compare variant in verify_action_update
File: crates/storage/src/interface.rs (line 3697-3705) | Consensus: 1/2 agents
The verify_action_update function's match arm has a comment // DeleteRef has its own type-match check in the main apply_action; Compare doesn't mutate. This still references Compare, which was removed by this PR. The wildcard arm now only matches DeleteRef (since Action no longer has a Compare variant), so the comment is misleading/dead documentation left over from before the removal.
Suggested fix:
Update the comment to: `// DeleteRef has its own type-match check in the main apply_action.`
Found by: patterns-reviewer
🤖 Generated by AI Code Reviewer | Review ID: review-018c5041
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 2 agents | Quality score: 90% | Review time: 562.7s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-cab32e9a
1 participant
What
Follow-up to #3216. After the state-based Comparisons sync was removed, nothing constructs
Action::Compare— its only producers (compare_treesand theapply_actionpushes) are gone. This removes the dead variant and the ~20 defensive match arms that only existed to handle it, acrossstorage,context,node,op-adapter,runtime, andtools/merodb.Wire compatibility (the one subtle part)
Actionpreviously derived borsh, giving positional tagsAdd=0, Compare=1, DeleteRef=2, Update=3. Naively deletingComparewould renumberDeleteRef2→1 andUpdate3→2 — silently corrupting every persisted and in-flight delta (the live variants), not just the dead one.So
Actionnow has a hand-rolledBorshSerialize/BorshDeserializethat keepsAdd=0, DeleteRef=2, Update=3and rejects tag 1 — byte-identical encoding for every surviving variant. Same approach #3216 used forStorageDelta. A newaction.rstest pins the tag bytes, roundtrips each variant, and asserts tag 1 is rejected.Also
Action::Comparepush (store.rs,tests/concurrency.rs, the stale-update test).payload_from_actionNonearm (kept theOptionreturn to avoid a signature change rippling to callers).Verification
cargo clippy --workspace --all-targets --features calimero-storage/testing -- -D warnings— cleancargo test -p calimero-storage --features testing— 668 passed, 0 failed (incl. the new wire-tag test)🤖 Generated with Claude Code