ci(protocol-watch): show what changed, not just which files - #155
Merged
Conversation
…le of one protocol-watch fired on issue #46: features.macro changed on rippled develop. The change is one line - LendingProtocolV1_1 moved from Supported::No to Supported::Yes in "feat: Enable LendingProtocolV1_1 amendment" (#8125), 2026-08-26. That line is cited in this script as the example of a name that must not go into EXTRA_FEATURES, on two grounds: Supported::No on develop, and absent from the release macro. Both have stopped being true. develop marked it supported, and the CI stand moved to 3.3.0, which declares it - the comment still said 3.2.0. So the example now argues the opposite of what it was written to show: by the stated rule, both binaries know the name, which is exactly what makes an entry admissible. Rewritten as history rather than replaced, because no amendment fits that shape today: checked every Supported::No name on develop against 3.3.0 and all of them are declared there. Saying so is more useful than picking a new example that does not hold. No config change, and none is possible: the generator selects Supported::Yes names, and the nightly pin is 3.4.0~b0+202608111815.26cc683e - built 15 days before the flip, so its binary still has the amendment as Supported::No. Regenerating from that ref, which is the only ref allowed to match the pin, produces what is already committed. Verified rather than assumed. Regenerating both stands from their own refs - 26cc683e for nightly, 3.3.0 for CI - reproduces the committed files byte for byte, 47 amendments each. Generating into a throwaway copy from develop yields 48, with LendingProtocolV1_1 in [features] and in [amendments] with its hash: the entry arrives on its own when the pin moves, which nightly-pin-watch will propose within the day - the pin is 20 days old against its 21-day allowance.
The watch tells the operator that a protocol file moved and links to its history. Naming the file still leaves the reading of the change to a human who must fetch two revisions and compare them by hand - which is the work the watch exists to save. The last firing was one line of features.macro flipping LendingProtocolV1_1 from Supported::No to Supported::Yes, and seeing that took several minutes of manual blob fetching. Both blob shas are already in hand at that point: the baseline in the tracking issue holds the old one, the fresh snapshot the new. So fetch the two blobs by sha and render a unified diff inline, per file, folded into a <details> block so a wide change does not bury the comment. Rendering the diff is best-effort while the notification is not. A blob fetch that fails leaves the file listed with its links and a note rather than aborting a run whose only job is to tell somebody that something changed. Long diffs are cut at 80 lines per file with the true total stated, keeping the comment far below GitHub's 65536-character limit.
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe pull request updates amendment validation comments and extends protocol-watch notifications with per-file unified diffs. The workflow handles added, removed, renamed, and unavailable files, and truncates long diffs. ChangesProtocol watch updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds best-effort unified diffs to protocol-watch notifications and updates an amendment example; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merged
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.
Closes the follow-up from the protocol-watch firing tracked in #46.
Why
The watch tells the operator that a protocol file moved on
XRPLF/rippleddevelopand links to its history. Naming the file still leaves the reading of the change to a human, who has to fetch two revisions and compare them by hand — which is the work the watch exists to save.The last firing was one line of
features.macroflippingLendingProtocolV1_1fromSupported::NotoSupported::Yes. Finding that out took several minutes of manual blob fetching.What changed
Both blob shas are already in hand at the point the comment is built: the baseline stored in the tracking issue holds the old one, the fresh snapshot the new. So the two blobs are fetched by sha and diffed directly, and each changed file gets a unified diff folded into a
<details>block under its links.The comment for the last real firing now renders as:
Rendering a diff is best-effort while the notification is not: a blob fetch that fails leaves the file listed with its links and a note, rather than aborting a run whose only job is to tell somebody that something changed. Long diffs are cut at 80 lines per file with the true total stated.
The second commit is unrelated housekeeping from the same firing:
generate-amendments.shusedLendingProtocolV1_1as its example of an amendment the script has to cope with, and that amendment is no longer an example of one.Verification
The comment-building logic was run outside Actions against the real shas from #46 (
jqis absent locally, so its calls were replaced with the values it would return), covering all three paths:truncated at 4 of 7 linesactionlintreports the same three pre-existing SC2016 findings before and after — no new ones.Summary by CodeRabbit
Improvements
Documentation