ci: make conflict prediction advisory - #7422
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis PR rewrites the conflict-handling script to discover overlapping same-base PRs through GitHub’s API, validate mergeability, and maintain a single managed advisory comment per PR. It also updates the workflow to call the script directly with new trigger conditions and adds unit tests for state parsing, comment updates, cleanup, and CLI behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as predict-conflicts.yml
participant Script as handle_potential_conflicts.py
participant GitHubAPI as GitHub API
participant CurrentPR as current PR
participant TargetPR as related PR
Workflow->>Script: run with --pr-number / --cleanup-closed
Script->>GitHubAPI: discover conflicts and fetch PR data
GitHubAPI-->>Script: candidate PRs and files
Script->>GitHubAPI: validate mergeability
Script->>CurrentPR: update managed outbound state
Script->>TargetPR: update inbound state or cleanup references
Script->>GitHubAPI: save or delete managed comments
Script-->>Workflow: finish execution
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🧹 Nitpick comments (1)
.github/workflows/test_handle_potential_conflicts.py (1)
116-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated manual monkeypatch boilerplate; consider
unittest.mock.patch.object.Each test manually saves the original attribute, reassigns it, and restores it in a
finallyblock. This pattern repeats ~8 times across the file. Usingunittest.mock.patch.object(handle_potential_conflicts, "name", ...)as a context manager would remove the boilerplate and guarantee restoration more idiomatically.♻️ Example refactor for one occurrence
- original_list_issue_comments = handle_potential_conflicts.list_issue_comments - handle_potential_conflicts.list_issue_comments = lambda pr_num: comments - try: - managed_comments = handle_potential_conflicts.find_managed_comments(1) - finally: - handle_potential_conflicts.list_issue_comments = original_list_issue_comments + with unittest.mock.patch.object( + handle_potential_conflicts, "list_issue_comments", lambda pr_num: comments + ): + managed_comments = handle_potential_conflicts.find_managed_comments(1)Also applies to: 133-138, 183-191, 241-255, 263-266, 292-315, 341-348
🤖 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 @.github/workflows/test_handle_potential_conflicts.py around lines 116 - 121, The tests in test_handle_potential_conflicts.py repeatedly save, replace, and restore handle_potential_conflicts attributes like list_issue_comments and get_managed_comment manually; refactor these blocks to use unittest.mock.patch.object as a context manager instead. Update each affected test case to patch the specific handle_potential_conflicts symbol inline, remove the try/finally restore boilerplate, and keep the existing assertions unchanged.
🤖 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 @.github/workflows/handle_potential_conflicts.py:
- Around line 192-210: The bot comment rendering in format_files and
format_pr_line interpolates user-controlled PR titles and filenames directly
into Markdown, so escape the rendered link text and code spans before
formatting. Update compact_title usage and the filename formatting in
format_files so Markdown metacharacters in titles, filenames, and the formatted
“Changed files” text cannot alter the GitHub Actions comment content. Keep the
existing behavior of format_pr_line and format_files, but ensure all
interpolated values are safely escaped before building the final string.
- Around line 414-419: The pre-mergeability URL built in branches_can_merge
currently interpolates raw PR labels, which can break when labels contain
reserved characters like /. Percent-encode both our_pr_label and
conflict_pr_label before constructing merge_check_url, using quote(..., safe="")
so the request always targets the correct GitHub pre_mergeable endpoint.
- Around line 139-178: normalize_state() is still allowing malformed
outbound/inbound entries through, which lets render_comment_body() later fail
when it casts item["number"] to int. Tighten normalization in normalize_state
and/or extract_state so only well-formed state entries are kept: filter outbound
to items that have a valid numeric number field and inbound to dict values that
also satisfy the expected structure before returning state. Use the existing
normalize_state, extract_state, and render_comment_body symbols to locate and
align the validation logic.
In @.github/workflows/predict-conflicts.yml:
- Around line 33-45: The workflow’s Python dependency installation is still in
the main step sequence, so a pip failure can block the job before the
advisory-only handling runs. Move the requests installation into the same
advisory-only step as validate_conflicts in predict-conflicts.yml, or otherwise
make the install step non-blocking with the same failure-tolerant behavior, so
the workflow remains advisory even if dependency setup is transiently
unavailable.
---
Nitpick comments:
In @.github/workflows/test_handle_potential_conflicts.py:
- Around line 116-121: The tests in test_handle_potential_conflicts.py
repeatedly save, replace, and restore handle_potential_conflicts attributes like
list_issue_comments and get_managed_comment manually; refactor these blocks to
use unittest.mock.patch.object as a context manager instead. Update each
affected test case to patch the specific handle_potential_conflicts symbol
inline, remove the try/finally restore boilerplate, and keep the existing
assertions unchanged.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d592229c-ebd7-4400-a59b-980d2c804496
📥 Commits
Reviewing files that changed from the base of the PR and between 4a19f6f and 6669fbd79e4f0d5d6b7e4a8d1bf44c6d0f83157e.
📒 Files selected for processing (3)
.github/workflows/handle_potential_conflicts.py.github/workflows/predict-conflicts.yml.github/workflows/test_handle_potential_conflicts.py
6669fbd to
a506e22
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/predict-conflicts.yml (1)
36-43: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid template expansion inside the
runblock (template injection).Under
pull_request_targetwithGITHUB_TOKENin scope, expanding${{ github.event.action }}and${{ github.event.pull_request.number }}directly into the shell is an injection vector. Pass them throughenvand reference the shell variables instead.🔒 Suggested hardening
- name: Validate potential conflicts and update advisory comments env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + EVENT_ACTION: ${{ github.event.action }} + PR_NUMBER: ${{ github.event.pull_request.number }} run: | - if [ "${{ github.event.action }}" = "closed" ]; then - .github/workflows/handle_potential_conflicts.py --pr-number "${{ github.event.pull_request.number }}" --cleanup-closed + if [ "$EVENT_ACTION" = "closed" ]; then + .github/workflows/handle_potential_conflicts.py --pr-number "$PR_NUMBER" --cleanup-closed else - .github/workflows/handle_potential_conflicts.py --pr-number "${{ github.event.pull_request.number }}" + .github/workflows/handle_potential_conflicts.py --pr-number "$PR_NUMBER" fi🤖 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 @.github/workflows/predict-conflicts.yml around lines 36 - 43, The workflow step in predict-conflicts.yml is expanding GitHub expressions directly inside the shell command, which creates a template injection risk. Move github.event.action and github.event.pull_request.number into env variables on the step, then update the run block to read those shell variables instead of using direct ${{ ... }} interpolation. Keep the change localized to the conflict-handling step that invokes handle_potential_conflicts.py.Source: Linters/SAST tools
🧹 Nitpick comments (1)
.github/workflows/test_handle_potential_conflicts.py (1)
119-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce repeated manual monkeypatch save/restore boilerplate.
Every test that patches module functions repeats a manual
original = ..., assign,try/finally: restorepattern.unittest.mock.patch.object(as context manager) would eliminate this duplication and reduce risk of forgetting to restore a patched attribute in future tests.♻️ Example refactor using unittest.mock.patch.object
- original_list_issue_comments = handle_potential_conflicts.list_issue_comments - handle_potential_conflicts.list_issue_comments = lambda pr_num: comments - try: - managed_comments = handle_potential_conflicts.find_managed_comments(1) - finally: - handle_potential_conflicts.list_issue_comments = original_list_issue_comments + with mock.patch.object(handle_potential_conflicts, "list_issue_comments", lambda pr_num: comments): + managed_comments = handle_potential_conflicts.find_managed_comments(1)Also applies to: 169-177, 227-241, 249-252, 270-271, 277-296, 320-327, 344-347
🤖 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 @.github/workflows/test_handle_potential_conflicts.py around lines 119 - 124, The tests in handle_potential_conflicts repeat manual save/assign/try-finally restore monkeypatching for module functions like list_issue_comments and related helpers. Refactor those blocks to use unittest.mock.patch.object as a context manager around each patched symbol so the temporary override and automatic restore are handled consistently. Apply this pattern to the repeated patching sites in the test cases for find_managed_comments and the other listed assertions to remove boilerplate and avoid missed restoration.
🤖 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.
Duplicate comments:
In @.github/workflows/predict-conflicts.yml:
- Around line 36-43: The workflow step in predict-conflicts.yml is expanding
GitHub expressions directly inside the shell command, which creates a template
injection risk. Move github.event.action and github.event.pull_request.number
into env variables on the step, then update the run block to read those shell
variables instead of using direct ${{ ... }} interpolation. Keep the change
localized to the conflict-handling step that invokes
handle_potential_conflicts.py.
---
Nitpick comments:
In @.github/workflows/test_handle_potential_conflicts.py:
- Around line 119-124: The tests in handle_potential_conflicts repeat manual
save/assign/try-finally restore monkeypatching for module functions like
list_issue_comments and related helpers. Refactor those blocks to use
unittest.mock.patch.object as a context manager around each patched symbol so
the temporary override and automatic restore are handled consistently. Apply
this pattern to the repeated patching sites in the test cases for
find_managed_comments and the other listed assertions to remove boilerplate and
avoid missed restoration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2ffaceaa-7e29-4bdb-a2eb-b5efac26c5b5
📥 Commits
Reviewing files that changed from the base of the PR and between 6669fbd79e4f0d5d6b7e4a8d1bf44c6d0f83157e and a506e22.
📒 Files selected for processing (3)
.github/workflows/handle_potential_conflicts.py.github/workflows/predict-conflicts.yml.github/workflows/test_handle_potential_conflicts.py
|
Handled the remaining CodeRabbit workflow hardening in 9f03035. What changed:
Validation:
I left the monkeypatch refactor as cleanup-only rather than mixing that into the security/advisory fix. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI note: the red The actual merge-base PR diff only changes the conflict-prediction workflow handler/tests: I also ran |
|
✅ Review complete (commit 9f03035) — PR was already merged before public review posting; local exact-SHA verification stored |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f030351b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
|
|
||
| body = comment.get("body") or "" | ||
| if COMMENT_START in body: |
There was a problem hiding this comment.
On PRs that already have comments from the previous mshick/add-pr-comment flow (message-id: conflict-prediction, with the visible conflict/no-conflict headings removed in this commit), this condition ignores them because they do not contain dash-potential-conflicts:v1. After the workflow is upgraded, the script will create/delete only the new managed comments, leaving the old visible status comment in place and potentially contradicting the new advisory state.
Useful? React with 👍 / 👎.
| our_state = normalize_state(our_managed_comment.state) | ||
| our_state["outbound"] = validated_conflicts | ||
| save_managed_comment(our_pr_num, our_state) | ||
|
|
||
| targets_to_update = previous_outbound_numbers | current_outbound_numbers |
There was a problem hiding this comment.
When a PR that previously only had an inbound warning is synchronized so it no longer overlaps/conflicts with the source PR, this keeps our_managed_comment.state["inbound"] and saves it unchanged while targets_to_update only includes outbound PRs. The job that just inspected the changed PR therefore republishes the stale “If these PRs merge first” warning and never tells the source PR to remove its outbound reference until that other PR happens to run.
Useful? React with 👍 / 👎.
…omous MnEHF signal txs 03ff1ff test: ignore MnEHF signal txs in asset lock mempool accounting (pasta) 1cea97f test: wait for every quorum commitment on the mining node (pasta) Pull request description: ## Issue being fixed or feature implemented Follow-up to #7411, which closed #7310. The same test kept failing after that closure, so the fix was only partial. Sorting the post-closure reports on #7310 by the tree they actually ran on splits them in two: - The `wait_for_quorum_list()` timeouts in `test_v24_fork` -> `mine_quorum_2_nodes` all ran on PR heads that predate #7411. #7422's head `9f030351b` and #7350's head do not contain #7411's merge `cbab2549bc4`, and the #7418 report says so explicitly. That failure mode is fixed. - The exact-mempool-count failure — `test_asset_unlocks` -> `check_mempool_result` -> `check_mempool_size` -> `AssertionError: not(1 == 0)` — still reproduces on current `develop` with #7411 present. This PR fixes the second one and the race that lets it happen. ## What was done? Two related changes. **1. `mine_quorum()` only waited for the commitment of the type it was mining.** #7411 added a wait for the mining node to hold the final commitment, but only for `llmq_type`, the type the caller asked for. The block `mine_quorum()` then generates carries a commitment for *every* LLMQ type whose mining window is open, and in regtest all test types share `dkgInterval = 24`, so they all finalize on the same block. A type whose real commitment has not reached the mining node yet is mined as a **null** commitment (`src/llmq/blockprocessor.cpp:842-846`); null commitments are accepted without being recorded as mined (`src/llmq/blockprocessor.cpp:322-331`), so that quorum is silently skipped for the whole cycle. Captured from a failing run, at final-commitment block 178, for the undriven `llmq_test`: ```text GetMineableCommitments cf height[178] content: { created nversion[3] quorumIndex[0] } ProcessCommitment -- processing commitment for block height=178, type=100, quorumHash=1ffd275a..., signers=0, validMembers=0, quorumPublicKey=000000000000...0000 ``` `created` rather than `cached` is the mining node synthesising a null commitment because it held no real one. `mine_quorum()` now waits for the mining node to hold every commitment the masternodes actually produced for that quorum hash, not just the driven type. Only commitments the masternodes already have are awaited, so a type whose DKG legitimately produced nothing cannot hold a test up. **2. `check_mempool_size()` asserted a global mempool count.** regtest's `llmqTypeMnhf` is `LLMQ_TEST` (`src/chainparams.cpp:929`), so a skipped `llmq_test` quorum defers the one-shot V24 MnEHF signal transaction to a later cycle. `CEHFSignalsHandler` submits it from the masternodes on their own (`src/llmq/ehf_signals.cpp:118`) at a moment the test does not control, while `check_mempool_size()` compared `getmempoolinfo()['size']` against `self.mempool_size`, which only ever modelled the test's own transactions: ```text node2 ... Special EHF TX is created hash=1feb94ce... node2 ... IsValidMNActivation: set MnEHF for bit=12 is valid (bit 12 = DEPLOYMENT_V24) node0 ... accepted 1feb94ce... (poolsz 1 txn, 0 kB) test ... AssertionError: not(1 == 0) ``` The count now excludes MnEHF signal transactions, so the assertion still says exactly what it said before about the transactions this test submits, and no longer depends on when the masternodes submit theirs. Deliberately not a `wait_until` on the mempool size: that would also pass if an asset-unlock transaction wrongly lingered, which is what the assertion exists to catch. `mine_cycle_quorum()` has the same gap and does not wait for commitments at all, but it drives rotated (dip0024) quorums and no failure in this family was traced to it, so it is left alone. ## How Has This Been Tested? Built from `upstream/develop` at `efe6dec7b9a` on macOS arm64, `configure --prefix=depends/aarch64-apple-darwin25.3.0 --disable-bench`, `make -j13`. Sequential control: `feature_asset_locks.py` passes in 167 s, matching CI's passing runtime. The natural rate of this flake is only ~3% locally, and it is gated by a discrete precursor rather than a timing window, so raising `--jobs` does not amplify it — at `-j20` the machine slows uniformly, which gives the `llmq_test` commitment *more* time to arrive and suppressed the precursor entirely (0/20). The before/after was therefore measured against a forced precursor: an experiment-only patch, not part of this PR, that removes the "Mine block to empty mempool" `generate()` in `test_asset_unlocks` — the block that would otherwise sweep the MnEHF transaction. Same assertion, same transaction, same mechanism. | Run | Result | | --- | --- | | 15x parallel, forced precursor, without this PR | **15/15 failed**, every one the `not(1 == 0)` signature | | 15x parallel, forced precursor, with this PR | **15/15 passed** | | 15x parallel, unforced, without this PR | 1/15 failed (the natural ~3% rate) | | 15x parallel, unforced, with this PR | 30/30 passed across 2 batches; a 3rd batch is excluded, see below | A third unforced 15x batch ran while the host was under an unrelated load spike (load average 145 on a 14-core machine, from desktop applications rather than the test run) and lost 14 of 15 copies to block-sync, mempool-sync, recovered-signature and RPC timeouts. Those are host-contention failures, not this signature, and are excluded from the table rather than counted as passes. Across all 45 unforced runs with this PR there were zero `not(1 == 0)` occurrences. In the forced-and-unfixed runs the single mempool entry was confirmed to be the MnEHF transaction, e.g. `872ca79e...` created by `CEHFSignalsHandler` and accepted on node0 at `poolsz 1 txn`. Because the framework change affects every test that mines quorums, all 15 such tests were run: `feature_dip4_coinbasemerkleroots`, `feature_llmq_connections`, `feature_llmq_data_recovery`, `feature_llmq_dkg_intake`, `feature_llmq_dkgerrors`, `feature_llmq_evo`, `feature_llmq_rotation`, `feature_llmq_signing`, `feature_llmq_simplepose` (both variants), `feature_mnehf`, `feature_notifications`, `feature_protx_version`, `p2p_instantsend`, `p2p_platform_ban`, `p2p_quorum_data`. All passed except `feature_protx_version`, which failed at `test_revoke_protx` waiting for `getconnectioncount() == 0` (feature_protx_version.py:241). That is the pre-existing #6702 flake, not a regression here: it reproduces identically on unmodified `develop` (1/8 failed at `-j8` on a clean tree vs 3/8 with this PR, the same `line 241` signature in every case), and it is unrelated to quorum commitments. `test/lint/lint-python.py` passes. ## Breaking Changes None. Test-only change. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 02358a0fc4bfcc76fed5240f288d0c278818cad1ddd01ec8181f380c7b5c8cadbdc36c2ef4559c034b15c37989ed4214987945ae5de761b32b1eacabc15cab6c
3 participants
ci: make conflict prediction advisory
Issue being fixed or feature implemented
The existing potential-conflicts workflow is hard to act on because it can fail
CI and only reports the conflict from the triggering PR's side. For review and
merge planning, maintainers need an advisory comment on both affected PRs that
explains which PR would force the other to rebase if merged first.
What was done?
Reworked the potential-conflicts workflow to:
repository-local Python handler.
API.
reporting them.
merge-order risk.
continue-on-error, so conflicts do notmake CI red.
converted to draft.
github-actions[bot]comments are treated as workflow-owned state.How Has This Been Tested?
Validated locally on macOS with:
Ran a live read-only dry-run against
dashpay/dash#7419:GITHUB_TOKEN=$(gh auth token) \ GITHUB_REPOSITORY=dashpay/dash \ .github/workflows/handle_potential_conflicts.py \ --pr-number 7419 \ --dry-rundashpay/dash#7052as the validated advisory conflictand rendered the managed comment body without writing comments.
Also ran an Opus sidecar design pass and multiple Codex review-gate passes. The
final blocker-only review returned
No issues found.Breaking Changes
None. The workflow remains advisory-only and does not fail CI when conflicts are
found or when comment updates fail.
Checklist
code-owners and collaborators only)