ci: move conflict comment state after advisory#7453
Conversation
The hidden state block was rendered first, so raw notification previews of the managed conflict comment started with an opaque base64 blob instead of the advisory text. Render it after the advisory instead. Reading it back can no longer take the first marker in the body, since advisory content (a PR title, say) may contain marker-like text ahead of the real block. Anchor on the last marker instead, unless the body starts with one, which is how comments in the old marker-first layout are still read.
0b199db to
e86bacf
Compare
| - name: Test workflow scripts | ||
| run: | | ||
| # The conflict handler and its tests import requests, which the lint container does not ship. | ||
| python3 -m pip install --quiet requests | ||
| python3 .github/workflows/test_handle_potential_conflicts.py | ||
| shell: bash | ||
|
|
There was a problem hiding this comment.
@thepastaclaw seems as irrelevant changes ; why did you change this?
There was a problem hiding this comment.
It is meant to make the tests for this change non-dead. Before this PR, .github/workflows/test_handle_potential_conflicts.py was not invoked by any workflow; predict-conflicts.yml only runs the handler itself. Without this step, the new legacy-layout and marker-in-title regressions would only be run manually.
I put it in the existing lint job because that job checks out the PR head, and the requests install is needed by the handler/test imports in that container.
One caveat I verified while answering: the current PR pull_request_target run used the base revision of lint.yml, so the new step did not execute on this PR; once merged, it will enforce the focused tests for future PR heads. So it is related CI coverage, but I agree it broadens the PR. I can split/remove the workflow wiring if you prefer.
There was a problem hiding this comment.
Agreed — removed this from the lint workflow. The unit test remains in the PR but is no longer run as part of linting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe conflict workflow now anchors managed state extraction to the first marker only when the comment begins with it, otherwise using the last marker occurrence. Comment rendering places the advisory content before the encoded managed state block. Tests cover marker-like content in titles, delimiter placement, state round-tripping, and legacy comments with the marker segment first. Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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 |
|
🔍 Review in progress — actively reviewing now (commit 754ffa6) |
4 participants
Issue being fixed or feature implemented
Conflict advisory comments begin with a hidden HTML block containing base64-encoded state. GitHub hides this block in the web UI, but Slack's raw comment preview shows it before the useful conflict information.
What was done?
How Has This Been Tested?
python3 .github/workflows/test_handle_potential_conflicts.py(15 tests)python3 -m py_compile .github/workflows/handle_potential_conflicts.py .github/workflows/test_handle_potential_conflicts.pytest/lint/lint-python.pytest/lint/lint-whitespace.py.github/workflows/lint.ymlgit diff --check upstream/develop...HEADBreaking Changes
None.
Checklist: