Fix revert-pr to amend commit message for UPSTREAM convention#336
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR updates documentation for the revert-pr skill and command with expanded guidance on UPSTREAM convention detection and commit message amendments. Version bumps from 0.0.7 to 0.0.8 across plugin manifests accompany these documentation changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/lgtm Needs a plugin version bump |
openshift-ci
Bot
added
lgtm
petr-muller
force-pushed
the
fix-revert-pr-upstream-commit-message
branch
from
8c6dcce to
cbd21d1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/ci/commands/revert-pr.md (1)
62-62:⚠️ Potential issue | 🟡 MinorInconsistent UPSTREAM convention terminology.
Line 62 still uses the old specific format
UPSTREAM: <carry>:while line 122 uses the generalizedUPSTREAM: <tag>:format. For consistency with the PR's objective to generalize the documentation, this should use the same terminology.📝 Proposed fix for consistency
- - Detecting commit message conventions (e.g., `UPSTREAM: <carry>:` prefix) + - Detecting commit message conventions (e.g., `UPSTREAM: <tag>:` prefix)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/commands/revert-pr.md` at line 62, Replace the legacy specific term in the commit convention example: change the string "UPSTREAM: <carry>:" to the generalized "UPSTREAM: <tag>:" in the revert-pr.md documentation (the commit message conventions line currently using UPSTREAM: <carry>: should be updated to match the other occurrence that uses UPSTREAM: <tag>: so both use the same generalized terminology).plugins/ci/skills/revert-pr/SKILL.md (1)
147-159:⚠️ Potential issue | 🟡 MinorInconsistent PR title format between Step 6 and Step 9.
Line 153 shows the UPSTREAM revert format as:
UPSTREAM: <carry>: Revert "ORIGINAL_TITLE" because REASONHowever, the actual PR title format defined in Step 9 (line 303) and the example (line 305) omit the
because REASONclause:{JIRA}: UPSTREAM: <tag>: Revert "{ORIGINAL_TITLE}"The format in Step 6 should match the actual template used in Step 9 for consistency.
📝 Proposed fix to align Step 6 with Step 9
- The revert commit message and PR title should use:
- UPSTREAM: : Revert "ORIGINAL_TITLE" because REASON
- UPSTREAM: : Revert "ORIGINAL_TITLE"
For example:
- UPSTREAM: : Revert "UPSTREAM: 12345: Fix kubelet crash" because it broke e2e-aws jobs
- UPSTREAM: : Revert "UPSTREAM: 12345: Fix kubelet crash"
Note: The "because REASON" context belongs in the PR body's `{CONTEXT}` section, not in the title. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plugins/ci/skills/revert-pr/SKILL.mdaround lines 147 - 159, Update the
UPSTREAM revert guidance in Step 6 to match the PR title template in Step 9:
remove the trailing "because REASON" from the title format shown asUPSTREAM: <carry>: Revert "ORIGINAL_TITLE"and update the exampleUPSTREAM: <carry>: Revert "UPSTREAM: 12345: Fix kubelet crash"to match; ensure the note directs
authors to put the rationale in the PR body{CONTEXT}(not the title) so the
Step 6 format aligns with the{JIRA}: UPSTREAM: <tag>: Revert "{ORIGINAL_TITLE}"template in Step 9.</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@plugins/ci/skills/revert-pr/SKILL.md`: - Line 296: Add language identifiers to the fenced code blocks containing the revert heading examples: locate the code blocks with the text snippet `{JIRA}: Revert #{PR_NUMBER} "{ORIGINAL_TITLE}"` and `{JIRA}: UPSTREAM: <tag>: Revert "{ORIGINAL_TITLE}"` and change their fences from plain ``` to ```text so the blocks are annotated for static analysis and readability. - Line 197: Add language identifiers to the fenced code blocks mentioned (the code blocks currently containing the revert lines at lines 197 and 201 in SKILL.md) so they render with syntax highlighting; update each triple-backtick fence from ``` to ```text and adjust the inner content to include the "UPSTREAM: <tag>:" prefix where shown in the proposed fix, ensuring both code fences are changed consistently (the block with Revert "Merge pull request `#638` from author/branch" and the other block at line 201). - Around line 179-204: The sed command currently inserts the literal "<tag>"; capture the detected tag into a variable (e.g., upstream_tag) when you detect it in Step 6 and then use that variable when building the amended message (replace sed '1s/^/UPSTREAM: <tag>: /' with a shell-expanded form like sed "1s/^/UPSTREAM: ${upstream_tag}: /" or prepend the prefix into amended_msg before git commit --amend) so the actual tag value (carry/drop/number) is inserted into the commit message. --- Outside diff comments: In `@plugins/ci/commands/revert-pr.md`: - Line 62: Replace the legacy specific term in the commit convention example: change the string "UPSTREAM: <carry>:" to the generalized "UPSTREAM: <tag>:" in the revert-pr.md documentation (the commit message conventions line currently using UPSTREAM: <carry>: should be updated to match the other occurrence that uses UPSTREAM: <tag>: so both use the same generalized terminology). In `@plugins/ci/skills/revert-pr/SKILL.md`: - Around line 147-159: Update the UPSTREAM revert guidance in Step 6 to match the PR title template in Step 9: remove the trailing "because REASON" from the title format shown as `UPSTREAM: <carry>: Revert "ORIGINAL_TITLE"` and update the example `UPSTREAM: <carry>: Revert "UPSTREAM: 12345: Fix kubelet crash"` to match; ensure the note directs authors to put the rationale in the PR body `{CONTEXT}` (not the title) so the Step 6 format aligns with the `{JIRA}: UPSTREAM: <tag>: Revert "{ORIGINAL_TITLE}"` template in Step 9.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
plugins/ci/commands/revert-pr.mdplugins/ci/skills/revert-pr/SKILL.md
| ``` | ||
|
|
||
| This transforms the commit message from: | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to code blocks.
The fenced code blocks on lines 197 and 201 should have language identifiers for proper syntax highlighting and better readability.
📝 Proposed fix
This transforms the commit message from:
-```
+```text
Revert "Merge pull request `#638` from author/branch"to:
- +text
UPSTREAM: : Revert "Merge pull request #638 from author/branch"
Also applies to: 201-201
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 197-197: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ci/skills/revert-pr/SKILL.md` at line 197, Add language identifiers
to the fenced code blocks mentioned (the code blocks currently containing the
revert lines at lines 197 and 201 in SKILL.md) so they render with syntax
highlighting; update each triple-backtick fence from ``` to ```text and adjust
the inner content to include the "UPSTREAM: <tag>:" prefix where shown in the
proposed fix, ensuring both code fences are changed consistently (the block with
Revert "Merge pull request `#638` from author/branch" and the other block at line
201).
cbd21d1 to
5a3c4c5
Compare
The revert-pr skill creates the revert commit with `git revert -m1 --no-edit`, which produces a default `Revert "..."` message. In repos that enforce the `UPSTREAM: <tag>:` commit message convention via a commitchecker CI job, this causes verify-commits to fail. Add an explicit step to amend the revert commit message with the appropriate `UPSTREAM: <tag>:` prefix when the convention is detected. Also emphasize that convention detection must always be performed, fix a stale step reference, and generalize the documentation beyond openshift/kubernetes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
petr-muller
force-pushed
the
fix-revert-pr-upstream-commit-message
branch
from
5a3c4c5 to
0d64154
Compare
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)
plugins/ci/skills/revert-pr/SKILL.md (1)
293-343:⚠️ Potential issue | 🟠 MajorPR title instructions and command are currently inconsistent.
Lines 293-306 define a convention-dependent title, but Line 341 still hardcodes the standard non-UPSTREAM format. This can generate incorrect titles for UPSTREAM repos.
🛠️ Proposed fix
+if [ -n "$upstream_tag" ]; then + pr_title_rendered="$jira: UPSTREAM: <${upstream_tag}>: Revert \"$pr_title\"" +else + pr_title_rendered="$jira: Revert #$pr_number \"$pr_title\"" +fi + gh pr create \ --repo "$owner/$repo" \ --base "$base_branch" \ --head "$gh_user:$revert_branch" \ - --title "$jira: Revert #$pr_number \"$pr_title\"" \ + --title "$pr_title_rendered" \ --body "$rendered_body"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/revert-pr/SKILL.md` around lines 293 - 343, The PR title guidance and the gh pr create invocation are inconsistent: the "PR Title Format" section defines different title formats for standard vs UPSTREAM carry repositories but the gh pr create command still hardcodes the standard format via --title "$jira: Revert #$pr_number \"$pr_title\"". Fix by computing the correct title beforehand (e.g., build a $title or $rendered_title variable based on the commit-convention detected in Step 6) and then pass that variable to gh pr create (replace --title "$jira: Revert #$pr_number \"$pr_title\"" with --title "$title"); ensure the UPSTREAM branch path uses the UPSTREAM template with <tag> when constructing $title.
♻️ Duplicate comments (2)
plugins/ci/skills/revert-pr/SKILL.md (2)
197-203:⚠️ Potential issue | 🟡 MinorAdd language identifiers to these fenced blocks.
Lines 197 and 201 should use language-tagged fences (e.g.,
text) to satisfy MD040 and keep rendering consistent.📝 Proposed fix
-``` +```text Revert "Merge pull request `#638` from author/branch"to:
-+text
UPSTREAM: : Revert "Merge pull request#638from author/branch"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/revert-pr/SKILL.md` around lines 197 - 203, Add language identifiers (e.g., text) to the two fenced code blocks that contain the revert lines in SKILL.md so they satisfy MD040 and render consistently: locate the fenced blocks that wrap `Revert "Merge pull request `#638` from author/branch"` and `UPSTREAM: <tag>: Revert "Merge pull request `#638` from author/branch"` (the blocks around those exact strings) and change the opening backticks to include a language tag (for example, ```text) for both blocks.
179-194:⚠️ Potential issue | 🔴 CriticalUse the detected UPSTREAM tag variable, not a literal
<tag>.Line 190 prepends a literal placeholder, so the amended commit message still won’t match the repository convention. Capture the detected tag in Step 6 and interpolate it in Step 7.
🔧 Proposed fix
Store the detected convention for use in Steps 7 and 9. +```bash +# Example: set during Step 6 detection +upstream_tag="carry" +``` @@ # Get the current commit message current_msg=$(git log -1 --format=%B) # Prepend the UPSTREAM: <tag>: prefix to the first line -amended_msg=$(echo "$current_msg" | sed '1s/^/UPSTREAM: <tag>: /') +amended_msg=$(echo "$current_msg" | sed "1s/^/UPSTREAM: <${upstream_tag}>: /")#!/bin/bash # Verify the docs do not still use a literal placeholder in the sed replacement. rg -n 'sed .*UPSTREAM: <tag>:' plugins/ci/skills/revert-pr/SKILL.md # Verify a stored tag variable is documented for reuse across steps. rg -n 'upstream_tag|detected.*tag' plugins/ci/skills/revert-pr/SKILL.mdExpected result:
- First command: no matches after fix.
- Second command: at least one match in Step 6/Step 7 flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ci/skills/revert-pr/SKILL.md` around lines 179 - 194, The sed replacement in Step 7 currently inserts the literal placeholder "<tag>"; capture the detected tag in Step 6 into a variable (e.g., upstream_tag) and interpolate that variable when building amended_msg so the commit message becomes "UPSTREAM: <carry>:" (or other detected tag) instead of "UPSTREAM: <tag>:"; update the example to show upstream_tag being set during detection and change amended_msg to use double-quoted sed invocation that expands ${upstream_tag} (referencing upstream_tag and amended_msg) so the amend step uses the correct, detected tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/ci/skills/revert-pr/SKILL.md`:
- Around line 293-343: The PR title guidance and the gh pr create invocation are
inconsistent: the "PR Title Format" section defines different title formats for
standard vs UPSTREAM carry repositories but the gh pr create command still
hardcodes the standard format via --title "$jira: Revert #$pr_number
\"$pr_title\"". Fix by computing the correct title beforehand (e.g., build a
$title or $rendered_title variable based on the commit-convention detected in
Step 6) and then pass that variable to gh pr create (replace --title "$jira:
Revert #$pr_number \"$pr_title\"" with --title "$title"); ensure the UPSTREAM
branch path uses the UPSTREAM template with <tag> when constructing $title.
---
Duplicate comments:
In `@plugins/ci/skills/revert-pr/SKILL.md`:
- Around line 197-203: Add language identifiers (e.g., text) to the two fenced
code blocks that contain the revert lines in SKILL.md so they satisfy MD040 and
render consistently: locate the fenced blocks that wrap `Revert "Merge pull
request `#638` from author/branch"` and `UPSTREAM: <tag>: Revert "Merge pull
request `#638` from author/branch"` (the blocks around those exact strings) and
change the opening backticks to include a language tag (for example, ```text)
for both blocks.
- Around line 179-194: The sed replacement in Step 7 currently inserts the
literal placeholder "<tag>"; capture the detected tag in Step 6 into a variable
(e.g., upstream_tag) and interpolate that variable when building amended_msg so
the commit message becomes "UPSTREAM: <carry>:" (or other detected tag) instead
of "UPSTREAM: <tag>:"; update the example to show upstream_tag being set during
detection and change amended_msg to use double-quoted sed invocation that
expands ${upstream_tag} (referencing upstream_tag and amended_msg) so the amend
step uses the correct, detected tag.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
.claude-plugin/marketplace.jsondocs/data.jsonplugins/ci/.claude-plugin/plugin.jsonplugins/ci/commands/revert-pr.mdplugins/ci/skills/revert-pr/SKILL.mdplugins/testing/commands/mutation-test.md
✅ Files skipped from review due to trivial changes (1)
- docs/data.json
|
Thank you... |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller, stbenjam The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
revert-prskill creates revert commits withgit revert -m1 --no-edit, producing a defaultRevert "..."message. In repos enforcing theUPSTREAM: <tag>:commit convention viacommitchecker(e.g.,openshift/operator-framework-operator-controller), this causesverify-commitsCI to fail.UPSTREAM: <tag>:prefix when the convention is detected.openshift/kubernetes.Context: openshift/operator-framework-operator-controller#647 failed
verify-commitsbecause the revert commit lacked theUPSTREAM: <carry>:prefix.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Chores