Add release checksum bundle#102
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDeterministic release checksum bundle support: new generator/check CLI, comprehensive tests, excluded-bundle integration with artifact comparison, Makefile/CI/check-script wiring, committed checksum artifacts, and updated docs/ops traceability. ChangesRelease checksum bundle implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review Please review the release checksum bundle with special attention to:
|
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
release-artifacts/README.md (1)
8-13: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winClarify the workflow scope or make it complete.
The workflow shown here includes
generate_release_checksums.pybut omits the deployment manifest and address book generation steps. Per lines 155-159 ofdocs/tooling.md, the checksum generator coversdeployments/config/,deployments/examples/,deployments/address-books/, anddeployments/schema/.Users following only this workflow might run checksum generation over stale deployment artifacts, producing incorrect checksums.
Consider either:
- Adding the missing steps to show the complete workflow (matching
docs/tooling.mdlines 116-121):python scripts/generate_deployment_manifest.py python scripts/generate_address_books.py python scripts/generate_release_checksums.py- Or clarifying that this is a partial workflow for release-artifact-only changes, with a note that deployment artifact changes require additional steps before checksum regeneration.
🤖 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 `@release-artifacts/README.md` around lines 8 - 13, Update the README workflow to prevent stale checksum generation by either (A) expanding the shown steps to the full sequence used in docs/tooling.md — include running python scripts/generate_deployment_manifest.py and python scripts/generate_address_books.py before python scripts/generate_release_checksums.py — or (B) explicitly mark the shown sequence as a partial workflow for release-artifact-only changes and add a note instructing users to run scripts/generate_deployment_manifest.py and scripts/generate_address_books.py first when deployments/config/, deployments/examples/, deployments/address-books/, or deployments/schema/ were modified; reference the scripts generate_release_checksums.py, generate_deployment_manifest.py, and generate_address_books.py and align the README text with docs/tooling.md.
🧹 Nitpick comments (1)
scripts/generate_release_checksums.py (1)
187-188: 💤 Low valueConsider adding path traversal validation.
The current validation rejects absolute paths and backslashes, but doesn't reject paths containing
"..". While the threat model is limited (requires commit access), adding a check likeif ".." in relative_pathwould provide defense-in-depth against potential information disclosure if a malicious checksum file is committed.🛡️ Proposed defense-in-depth enhancement
if relative_path.startswith("/") or "\\" in relative_path: raise ChecksumError(f"malformed checksum line {line_number}: invalid path") + if ".." in relative_path: + raise ChecksumError(f"malformed checksum line {line_number}: path traversal not allowed") entries.append((digest, relative_path))🤖 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/generate_release_checksums.py` around lines 187 - 188, The validation for checksum file paths currently only rejects absolute paths and backslashes; update the validation in the same block handling relative_path (used with line_number and ChecksumError) to also reject path traversal by checking for parent references (e.g., if ".." in relative_path) or by normalizing the path with pathlib.Path(relative_path).parts and ensuring no part equals ".."; if a traversal is detected raise ChecksumError with the same malformed checksum line message.
🤖 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.
Outside diff comments:
In `@release-artifacts/README.md`:
- Around line 8-13: Update the README workflow to prevent stale checksum
generation by either (A) expanding the shown steps to the full sequence used in
docs/tooling.md — include running python scripts/generate_deployment_manifest.py
and python scripts/generate_address_books.py before python
scripts/generate_release_checksums.py — or (B) explicitly mark the shown
sequence as a partial workflow for release-artifact-only changes and add a note
instructing users to run scripts/generate_deployment_manifest.py and
scripts/generate_address_books.py first when deployments/config/,
deployments/examples/, deployments/address-books/, or deployments/schema/ were
modified; reference the scripts generate_release_checksums.py,
generate_deployment_manifest.py, and generate_address_books.py and align the
README text with docs/tooling.md.
---
Nitpick comments:
In `@scripts/generate_release_checksums.py`:
- Around line 187-188: The validation for checksum file paths currently only
rejects absolute paths and backslashes; update the validation in the same block
handling relative_path (used with line_number and ChecksumError) to also reject
path traversal by checking for parent references (e.g., if ".." in
relative_path) or by normalizing the path with pathlib.Path(relative_path).parts
and ensuring no part equals ".."; if a traversal is detected raise ChecksumError
with the same malformed checksum line message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2a096ec-dd17-4ea7-955a-7e9d9826398f
📒 Files selected for processing (18)
.github/workflows/ci.ymlMakefileREADME.mddeployments/README.mddocs/deployment.mddocs/status.mddocs/tooling.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdrelease-artifacts/README.mdrelease-artifacts/latest/SHA256SUMSrelease-artifacts/latest/release-checksums.jsonscripts/check.ps1scripts/check.shscripts/generate_release_artifacts.pyscripts/generate_release_checksums.pyscripts/test_release_artifacts.pyscripts/test_release_checksums.py
|
@coderabbitai review Addressed the PR #102 findings on latest head
Validation after the review fix:
|
|
✅ Action performedReview finished.
|
|
Merge evidence for PR #102:
|
1 participant
Summary
release-artifacts/latest/SHA256SUMSandrelease-artifacts/latest/release-checksums.jsonrelease-artifacts/latest/--checkintomake check, Linux/Windows wrappers, and CI, and update release/deployment docs plus roadmap/run-state traceabilityCloses #101
Validation
python scripts\test_release_checksums.pypython scripts\generate_release_checksums.py --checkpython scripts\test_release_artifacts.pypython scripts\generate_release_artifacts.py --checkpython -m py_compile scripts\generate_release_artifacts.py scripts\test_release_artifacts.py scripts\check_abi_compatibility.py scripts\test_abi_compatibility.py scripts\generate_deployment_manifest.py scripts\test_deployment_manifest.py scripts\generate_address_books.py scripts\test_address_books.py scripts\generate_release_checksums.py scripts\test_release_checksums.pybash -n scripts/check.shscripts\check.ps1release-artifacts\latest\release-checksums.jsonandrelease-artifacts\latest\SHA256SUMSmake checkpowershell -ExecutionPolicy Bypass -File scripts\check.ps1rgtraceability check forP1-RELEASE-004,generate_release_checksums,SHA256SUMS, and release-signature boundary wordinggit diff --checkreported only line-ending warnings for touched scriptsSummary by CodeRabbit
New Features
Tests
Documentation
CI/Build