fix: panic in p-diff-sync after diff chunking feature#669
Conversation
📝 WalkthroughWalkthroughThis PR migrates the p-diff-sync test infrastructure from TypeScript/Tryorama to Rust/sweettest by consolidating a multi-job GitHub Actions workflow into a single Rust container job, removing Nix shell configuration, deleting all TypeScript test files, and adding limited new Rust functionality including HashBroadcast signal handling and a diff-loading helper function. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/p-diff-sync-tests.yml (1)
5-11:⚠️ Potential issue | 🟠 Major
devbranch is missing from the trigger list, and nopull_requestevent is configured.This PR targets
dev, butdevis not listed underon.push.branches, so these tests won't run on merges todev. Additionally, there's nopull_requesttrigger, so the workflow won't validate PRs before merging.Proposed fix
on: push: branches: - main - staging - bootstrap-languages + - dev + pull_request: + branches: + - main + - dev
🤖 Fix all issues with AI agents
In @.github/workflows/p-diff-sync-tests.yml:
- Around line 21-24: The workflow uses deprecated action versions
actions/checkout@v3 and actions/cache@v3; update both references to
actions/checkout@v4 and actions/cache@v4 in the workflow file so the jobs (the
step named "uses: actions/checkout" and the step "Cache Rust dependencies" using
actions/cache) run on the supported Node.js runtime and avoid deprecation
warnings.
🧹 Nitpick comments (3)
.github/workflows/p-diff-sync-tests.yml (1)
48-58: Redundant cache save —actions/cachealready auto-saves on miss.
actions/cache@v3(Line 24) restores and saves (via a post-job hook) on cache miss. The explicitactions/cache/save@v3here with the same key is redundant.Either:
- Remove this step entirely and rely on the auto-save from
actions/cache, or- Change Line 24 to
actions/cache/restore@v3(restore-only) and keep this explicit save for full control over when the save happens.bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/commit.rs (1)
136-138: Pre-existing: revision equality viaDebugformat is fragile.Comparing
LocalHashReferenceinstances throughformat!("{:?}", r)relies onDebugoutput being a faithful, stable representation of all relevant fields. If theDebugimpl changes or two semantically-different values produce the same debug string, this could silently miscompare. Since this is pre-existing code (only reformatted here), it's not a blocker for this PR, but consider deriving/implementingPartialEqonLocalHashReferenceand comparing directly.bootstrap-languages/p-diff-sync/hc-dna/zomes/perspective_diff_sync/src/link_adapter/chunked_diffs.rs (1)
116-134: Unbounded recursion risk inload_diff_from_entry.
load_diff_from_entrycallsChunkedDiffs::from_entries, which in turn callsload_diff_from_entryfor each loaded entry. If a chunked entry ever (due to a bug or data corruption) points back to itself or forms a cycle, this will stack-overflow without warning. While normal operation creates at most one level of nesting, adding a recursion depth guard would make this more robust.♻️ Suggested depth-limited variant
-pub fn load_diff_from_entry<Retriever: PerspectiveDiffRetreiver>( +pub fn load_diff_from_entry<Retriever: PerspectiveDiffRetreiver>( entry: &PerspectiveDiffEntryReference, ) -> SocialContextResult<PerspectiveDiff> { + load_diff_from_entry_inner::<Retriever>(entry, 0) +} + +const MAX_CHUNK_DEPTH: u8 = 3; + +fn load_diff_from_entry_inner<Retriever: PerspectiveDiffRetreiver>( + entry: &PerspectiveDiffEntryReference, + depth: u8, +) -> SocialContextResult<PerspectiveDiff> { + if depth > MAX_CHUNK_DEPTH { + return Err(crate::errors::SocialContextError::InternalError( + "load_diff_from_entry: max chunk nesting depth exceeded", + )); + } if entry.is_chunked() { let chunk_hashes = entry.diff_chunks.as_ref().unwrap(); - debug!( - "load_diff_from_entry: Loading {} chunks", - chunk_hashes.len() - ); - let chunked_diffs = ChunkedDiffs::from_entries::<Retriever>(chunk_hashes.clone())?; + // inner from_entries would need to accept depth, or use a non-recursive path + // for chunks (which are always leaf-level) ...
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Cache Rust dependencies | ||
| uses: actions/cache@v3 |
There was a problem hiding this comment.
Upgrade actions/checkout and actions/cache from v3 to v4.
v3 of these actions uses the Node.js 16 runtime, which GitHub has deprecated. These will produce deprecation warnings and will eventually stop working.
Proposed fix
- uses: actions/checkout@v3
+ uses: actions/checkout@v4- uses: actions/cache@v3
+ uses: actions/cache@v4And on Line 50:
- uses: actions/cache/save@v3
+ uses: actions/cache/save@v4🧰 Tools
🪛 actionlint (1.7.10)
[error] 21-21: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
[error] 24-24: the runner of "actions/cache@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/p-diff-sync-tests.yml around lines 21 - 24, The workflow
uses deprecated action versions actions/checkout@v3 and actions/cache@v3; update
both references to actions/checkout@v4 and actions/cache@v4 in the workflow file
so the jobs (the step named "uses: actions/checkout" and the step "Cache Rust
dependencies" using actions/cache) run on the supported Node.js runtime and
avoid deprecation warnings.
1 participant
Most of this is cargo fmt, and also removal of old tryorama tests. CI now runs new rust based sweetest tests.
Fix: Handle nested chunked entries in from_entries()
Also: Optimized into_aggregated_diff() to eliminate unnecessary cloning.
Tests: Added 3 comprehensive tests covering nested chunks, mixed inline/chunked entries, and empty entries.
Summary by CodeRabbit
New Features
Chores