rebuild only when needed, demote noisy log - #1197
Conversation
Signed-off-by: Roland Kuhn <rk@rkuhn.info>
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughPeer snapshot artifacts now use ChangesPeer snapshot build flow
Worktree snapshot tooling
Stake-distribution conformance tests
Build and observability hygiene
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Cargo
participant build_script
participant OUT_DIR
participant GitHub_API
Cargo->>build_script: watch staged inputs
build_script->>OUT_DIR: read cached snapshots
build_script->>GitHub_API: fetch missing snapshots
GitHub_API-->>build_script: return snapshot bytes and commit metadata
build_script->>OUT_DIR: write resolved artifacts
build_script-->>Cargo: provide generated build inputs
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru-node/build/peer_snapshot.rs (1)
170-196: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDiscard stale output bytes when the resolved SHA changes.
If
sha_changedis true anddownload_snapshotfails, the previous file atdestremains.prepare_peer_snapshotsthen treats that file as present and emits the newCONFIGS_COMMIT, even though the embedded bytes came from the previous SHA.Remove the previous output before the download attempt when the SHA changes. This lets the staged-file fallback replace a failed download. Otherwise, fail the build instead of embedding stale bytes.
Proposed fix
for network in peer_snapshot_network_names() { let dest = out_dir_snapshot_path(out_dir, network); + if sha_changed && dest.is_file() { + fs::remove_file(&dest) + .with_context(|| format!("remove stale snapshot {}", dest.display()))?; + } if !sha_changed && dest.is_file() { continue; }🤖 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 `@crates/amaru-node/build/peer_snapshot.rs` around lines 170 - 196, In prepare_peer_snapshots, remove each existing dest output before attempting download_snapshot whenever sha_changed is true. Preserve existing files when the SHA is unchanged, and ensure a failed download cannot leave stale bytes to be embedded; allow the existing staged-file fallback to replace the removed output or propagate a build failure when no replacement is available.
🧹 Nitpick comments (1)
crates/amaru/build/stake_distribution.rs (1)
90-96: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftMake snapshot refreshes machine-checkable.
available_ledger_snapshot_epochs(&ledger_dir)still contributes tostake_distribution_test_cases_source, which writesOUT_DIR/stake_distribution_{network}_test_cases.rs. After the ledger rerun trigger is removed, importing a new snapshot does not refresh this file by itself. A missed fixture-touch orcargo clean -p amarustep can silently omit an epoch from conformance tests. If the snapshot import workflow does not enforce that step, add a watched snapshot manifest or a CI freshness check.🤖 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 `@crates/amaru/build/stake_distribution.rs` around lines 90 - 96, Make snapshot refreshes machine-checkable for stake_distribution_test_cases_source and its generated OUT_DIR/stake_distribution_{network}_test_cases.rs: either watch a snapshot manifest that changes on every import or add a CI freshness check that regenerates the test-case list and fails when it differs. Ensure newly imported ledger epochs cannot be silently omitted without detecting the stale generated file.
🤖 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 `@scripts/setup-worktree-snapshots.sh`:
- Around line 21-22: Update the primary worktree extraction assigned to
main_root in the setup script to remove only the “worktree ” prefix from the
complete porcelain line, preserving spaces in the path; keep selecting the first
worktree entry and leave the subsequent path checks and linking behavior
unchanged.
---
Outside diff comments:
In `@crates/amaru-node/build/peer_snapshot.rs`:
- Around line 170-196: In prepare_peer_snapshots, remove each existing dest
output before attempting download_snapshot whenever sha_changed is true.
Preserve existing files when the SHA is unchanged, and ensure a failed download
cannot leave stale bytes to be embedded; allow the existing staged-file fallback
to replace the removed output or propagate a build failure when no replacement
is available.
---
Nitpick comments:
In `@crates/amaru/build/stake_distribution.rs`:
- Around line 90-96: Make snapshot refreshes machine-checkable for
stake_distribution_test_cases_source and its generated
OUT_DIR/stake_distribution_{network}_test_cases.rs: either watch a snapshot
manifest that changes on every import or add a CI freshness check that
regenerates the test-case list and fails when it differs. Ensure newly imported
ledger epochs cannot be silently omitted without detecting the stale generated
file.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d78ac8f-ea54-4b78-a80e-83a3559d0c49
📒 Files selected for processing (10)
.gitignorecrates/amaru-ledger/src/governance/ratification.rscrates/amaru-node/build/build.rscrates/amaru-node/build/peer_snapshot.rscrates/amaru-node/config/peer-snapshots/README.mdcrates/amaru/build/build.rscrates/amaru/build/stake_distribution.rsscripts/hooks/post-checkoutscripts/setup-hooks.shscripts/setup-worktree-snapshots.sh
| # First entry of `git worktree list` is always the primary worktree. | ||
| main_root="$(git worktree list --porcelain | awk '/^worktree / { print $2; exit }')" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve spaces in the primary worktree path.
Line 22 uses awk field $2, which truncates a worktree path that contains spaces. The script then tests and links the wrong src path. Remove only the worktree prefix from the complete line.
Proposed fix
- main_root="$(git worktree list --porcelain | awk '/^worktree / { print $2; exit }')"
+ main_root="$(git worktree list --porcelain | awk '/^worktree / { sub(/^worktree /, ""); print; exit }')"📝 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.
| # First entry of `git worktree list` is always the primary worktree. | |
| main_root="$(git worktree list --porcelain | awk '/^worktree / { print $2; exit }')" | |
| # First entry of `git worktree list` is always the primary worktree. | |
| main_root="$(git worktree list --porcelain | awk '/^worktree / { sub(/^worktree /, ""); print; exit }')" |
🤖 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 `@scripts/setup-worktree-snapshots.sh` around lines 21 - 22, Update the primary
worktree extraction assigned to main_root in the setup script to remove only the
“worktree ” prefix from the complete porcelain line, preserving spaces in the
path; keep selecting the first worktree entry and leave the subsequent path
checks and linking behavior unchanged.
It is useful; this is how we trigger the epoch boundary tests. In principle this only happens once: if the folders don't exists, they are created and then, the build is skipped. Of course, it also happens if the content of the ledger.db changes, because that means new/different e2e snapshot tests are available. |
|
@KtorZ thanks for the explanation! Could you be more specific about what exactly is required to make this useful? I’d like to avoid that |
|
Discussion notes:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/amaru/tests/summary.rs (1)
89-100: 🗄️ Data Integrity & Integration | 🔵 TrivialVerify that CI exposes successful soft-skips.
When the snapshot is missing, this function returns
Ok(()), so libtest reports the comparison as passed. For a successful test, libtest captures theeprintln!warning unless the run uses--nocapture. If CI uses the default command, the suite can pass without comparing any snapshot. Ensure CI enables--nocaptureor records and checks the soft-skip count.This follows from the
Ok(())branch incrates/amaru/tests/summary.rsand the documented test command incrates/amaru/tests/conformance/stake-distributions/README.md.🤖 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 `@crates/amaru/tests/summary.rs` around lines 89 - 100, Update the CI test invocation for the stake-distribution comparison to use --nocapture, or otherwise record and validate the soft-skip count, so the missing-snapshot warning from the Ok(()) branch in the summary test is visible and actionable. Use the documented command in the stake-distributions README as the reference and preserve normal successful comparisons.
🤖 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 `@crates/amaru/tests/conformance/stake-distributions/README.md`:
- Around line 37-41: Update the command heading in the stake-distributions
README to remove the claim that the commands target a given network. State that
cargo test lists or runs all generated comparison tests, unless the commands are
extended with a supported network filter.
---
Nitpick comments:
In `@crates/amaru/tests/summary.rs`:
- Around line 89-100: Update the CI test invocation for the stake-distribution
comparison to use --nocapture, or otherwise record and validate the soft-skip
count, so the missing-snapshot warning from the Ok(()) branch in the summary
test is visible and actionable. Use the documented command in the
stake-distributions README as the reference and preserve normal successful
comparisons.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf8531ec-76c0-4b23-9841-0d1d055527af
📒 Files selected for processing (4)
CHANGELOG.mdcrates/amaru/build/stake_distribution.rscrates/amaru/tests/conformance/stake-distributions/README.mdcrates/amaru/tests/summary.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru/build/stake_distribution.rs
| To list or run the generated comparison tests for a given network: | ||
|
|
||
| ```console | ||
| AMARU_NETWORK=preview cargo test -p amaru --test summary -- --list | ||
| AMARU_NETWORK=preview cargo test -p amaru --test summary | ||
| cargo test -p amaru --test summary -- --list | ||
| cargo test -p amaru --test summary --nocapture |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the network scope in the command heading.
The commands are network-independent, but the heading says “for a given network”. Both commands run the summary test binary without a network selector. State that they list or run all generated comparison tests, or document a supported network filter.
Suggested wording
- To list or run the generated comparison tests for a given network:
+ To list or run all generated comparison tests:This follows from the fixture scan over tests/conformance/stake-distributions/<network>/ and the commands shown in this section.
📝 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.
| To list or run the generated comparison tests for a given network: | |
| ```console | |
| AMARU_NETWORK=preview cargo test -p amaru --test summary -- --list | |
| AMARU_NETWORK=preview cargo test -p amaru --test summary | |
| cargo test -p amaru --test summary -- --list | |
| cargo test -p amaru --test summary --nocapture | |
| To list or run all generated comparison tests: | |
🤖 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 `@crates/amaru/tests/conformance/stake-distributions/README.md` around lines 37
- 41, Update the command heading in the stake-distributions README to remove the
claim that the commands target a given network. State that cargo test lists or
runs all generated comparison tests, unless the commands are extended with a
supported network filter.
amaru Signed-off-by: Roland Kuhn <rk@rkuhn.info>
2 participants
amaru, which I don’t think is useful -- please correct me if wrongamaru-node, which was incorrect (moved that file to $out_dir)Skip-changelog
Stack created with GitHub Stacks CLI • Give Feedback 💬
Summary by CodeRabbit
Developer Experience
Tests
Documentation
Observability