refactor(loop): unify tool-call dedup signature (canonical_json) [dirge-ark9] - #368
Merged
Merged
Conversation
…rge-ark9] The scavenge dedup (run.rs) used a nested canonical_json that sorts keys and normalizes numeric reprs (1 ≡ 1.0), while the storm repeat detector used raw serde_json::to_string — which only sorts keys while serde_json's preserve_order feature stays off and does NOT normalize 1 vs 1.0. So a '1' vs '1.0' duplicate could slip past storm but not scavenge. storm's own comment already flagged this and asked to 'reuse run::canonical_json'. Promote canonical_json to a shared pub(crate) fn in message.rs (the JSON-serialization module both already depend on — avoids a storm↔run circular dep) and route both sites through it. Closes the long-standing dirge-7bwx review-fix #6. Small behavior change: storm now also dedups calls that differ only by integer-vs-float encoding. Adds a test locking key-order + number stability; all 16 storm tests still pass; clean under -D warnings (default + all-features).
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.
The two tool-call dedup sites used different canonicalization:
run.rs) — a nestedcanonical_jsonthat sorts object keys and normalizes numeric reprs (1≡1.0);storm.rs) — rawserde_json::to_string, which only sorts keys whileserde_json'spreserve_orderfeature stays off and does not normalize1vs1.0.So a
1-vs-1.0duplicate could slip past storm but not scavenge. storm's own comment already flagged this (dirge-7bwxreview-fix #6) and asked to "reuserun::canonical_json".canonical_jsonto a sharedpub(crate)fn inmessage.rs— the JSON-serialization module both already depend on, which avoids astorm↔runcircular dependency.Small behavior change (the point of the issue): storm now also dedups calls that differ only by integer-vs-float encoding, and is no longer silently dependent on the
preserve_orderfeature. All 16 storm tests still pass; 2469 default + 2563 all-features; clean under-D warnings.Closes dirge-ark9 (and dirge-7bwx review-fix #6).