*: add skill for cherry-pick PR repair#10313
Conversation
Signed-off-by: okjiang <819421878@qq.com>
ti-chi-bot
bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
needs-cherry-pick-release-8.5
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded documentation: a new AGENTS.md entry registering the fix-cherry-pick-pr skill and a comprehensive skill guide at .agents/skills/fix-cherry-pick-pr/SKILL.md describing step-by-step procedures to repair and verify cherry-pick PRs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10313 +/- ##
==========================================
+ Coverage 78.78% 78.85% +0.07%
==========================================
Files 527 527
Lines 70916 70920 +4
==========================================
+ Hits 55870 55926 +56
+ Misses 11026 10975 -51
+ Partials 4020 4019 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: okjiang <819421878@qq.com>
okJiang
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/pd-fix-cherry-pick-pr/SKILL.md:
- Around line 35-39: The current pipeline in SKILL.md which runs "gh pr diff
<source-pr> --repo tikv/pd | rg '^[+-][^+-]' | sort" loses filename context and
can report false parity when identical line changes occur in different files;
update the recipe so file identity is preserved and comparisons are done
per-file (e.g., capture the per-file diff headers from the gh pr diff output and
group/compare +/− hunks for each filename, or produce a patch per file and diff
corresponding files) instead of globally sorting all added/removed lines; locate
the existing command in SKILL.md and replace it with a per-file-aware comparison
pipeline that retains the diff filenames and aligns changes by file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af4a820a-0de4-475f-96da-9dcfa721d5b2
📒 Files selected for processing (2)
.agents/skills/pd-fix-cherry-pick-pr/SKILL.mdAGENTS.md
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/skills/fix-cherry-pick-pr/SKILL.md (1)
95-99: Always run failpoint cleanup even on test failure.This recipe only disables failpoints if the test command completes normally. If
go testfails or is interrupted, the working tree can stay dirty and skew the later parity checks. Consider documenting it with a shelltrapso cleanup is guaranteed.Based on learnings, the skill should document its workflow and constraints clearly for reuse under `.agents/skills/**/*.md`.Suggested doc update
-```bash -make failpoint-enable -go test ./server/cluster -run 'TestCheckCache|TestStoreLimitChangeRefreshLimiter' -count=1 -v -timeout=2m -make failpoint-disable -``` +```bash +make failpoint-enable +trap 'make failpoint-disable' EXIT +go test ./server/cluster -run 'TestCheckCache|TestStoreLimitChangeRefreshLimiter' -count=1 -v -timeout=2m +trap - EXIT +make failpoint-disable +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/fix-cherry-pick-pr/SKILL.md around lines 95 - 99, Update the SKILL documentation (in .agents/skills/fix-cherry-pick-pr/SKILL.md) to ensure failpoint cleanup always runs by wrapping the test invocation with a shell trap that calls make failpoint-disable on EXIT, and explicitly reset/remove the trap after tests complete; also add a short section describing the workflow and constraints (why failpoints must be disabled, when to run the recipe, and expected repo state) so future users follow the guaranteed-cleanup pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/skills/fix-cherry-pick-pr/SKILL.md:
- Around line 95-99: Update the SKILL documentation (in
.agents/skills/fix-cherry-pick-pr/SKILL.md) to ensure failpoint cleanup always
runs by wrapping the test invocation with a shell trap that calls make
failpoint-disable on EXIT, and explicitly reset/remove the trap after tests
complete; also add a short section describing the workflow and constraints (why
failpoints must be disabled, when to run the recipe, and expected repo state) so
future users follow the guaranteed-cleanup pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e82a671-f45c-40a1-912b-58569b067ce6
📒 Files selected for processing (2)
.agents/skills/fix-cherry-pick-pr/SKILL.mdAGENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
|
/retest |
|
@okJiang: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| - Keep the current worktree clean before switching. | ||
| - Fetch the cherry-pick head branch from its actual remote, often `tichi` or `ti-chi-bot`. | ||
| - Create a local `codex/...` branch that tracks the cherry-pick head. |
There was a problem hiding this comment.
Codex-specific logic; consider changing it.
|
|
||
| ```bash | ||
| git fetch tichi cherry-pick-10131-to-release-8.5 | ||
| git switch -c codex/pr-10301-fix-conflict-markers --track tichi/cherry-pick-10131-to-release-8.5 |
| git commit -s -m 'storelimit: resolve cherry-pick markers in #10301' | ||
| ``` | ||
|
|
||
| - Push back to the cherry-pick head branch if you have permission. |
There was a problem hiding this comment.
In the previous requirement, we actually created a separate local branch to track the actual cherry-pick branch. Here, the phrasing “push back” seems ambiguous and can easily cause the agent to deviate during execution—for example, pushing to a newly created branch on origin.
| make failpoint-disable | ||
| ``` | ||
|
|
||
| - If you need broader signal, run `make basic-test`. |
There was a problem hiding this comment.
Running make basic-test does not cover the tests directory; if the related fix is in there, it seems running it doesn’t help.
|
Maybe we should bypass the CI requirement for doc-only PR's like this. |
|
/retest |
|
/ok-to-retest |
Signed-off-by: okjiang <819421878@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.agents/skills/fix-cherry-pick-pr/SKILL.md (3)
63-72: Consider generalizing or documenting the Codex-specific remote names.Lines 63 and 70 reference
tichiandti-chi-botas the remotes for cherry-pick branches. These appear specific to PingCAP's Codex bot infrastructure. For broader applicability, consider either:
- Using more generic language: "Fetch the cherry-pick head branch from its bot remote (commonly
tichiorti-chi-botin tikv/pd)"- Adding a note that this workflow is specific to tikv/pd's automation setup
This would help future maintainers understand whether these remote names are tikv/pd-specific conventions or general guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/fix-cherry-pick-pr/SKILL.md around lines 63 - 72, The doc currently names specific bot remotes "tichi" and "ti-chi-bot" in the examples and prose; update the SKILL.md text around the git fetch/git switch examples to use generic wording (e.g., "bot remote (commonly `tichi`/`ti-chi-bot` in tikv/pd)") or add a short note clarifying these are Codex/PingCAP-specific remotes rather than universal names; update the example lines that show git fetch and git switch to either use a placeholder like `<bot-remote>` or include a parenthetical explaining the remote is example-specific so readers know to replace it with their own remote.
101-102: Clarify testing coverage for thetests/directory.The guidance mentions running "the narrowest additional package or integration test target" but doesn't explicitly address a common pitfall: package-level test commands like
make basic-testdo not cover integration tests in thetests/directory.Consider adding an explicit note after line 101:
Suggested addition
- If you need broader signal, run the narrowest additional package or integration test target that covers the touched files. +- Note: package-level targets like `make basic-test` do not cover the top-level `tests/` directory. If the fix touches integration tests in `tests/`, run those tests explicitly (e.g., `make integration-test` or targeted tests in that directory). - If that extra verification fails in unrelated packages, separate that from the cherry-pick fix. Report the failing package and test name rather than attributing it to the cherry-pick automatically.This helps prevent agents from claiming verification is complete when integration tests were never run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/fix-cherry-pick-pr/SKILL.md around lines 101 - 102, Add an explicit note after the paragraph that includes "If you need broader signal, run the narrowest additional package or integration test target that covers the touched files" clarifying that package-level test commands (for example "make basic-test") do not execute integration tests located in the tests/ directory; instruct agents to run the integration test targets explicitly (e.g., run the tests under tests/ or the project's integration-test target) and to report missing integration-test verification if they only ran package-level commands.
121-122: Provide explicit git push command to avoid ambiguity.Line 121 says "Push
HEADback to the existing cherry-pick head branch" but doesn't show the exact command. Given the concern about agents potentially pushing to the wrong location, consider showing the explicit command:Suggested clarification
-- Push `HEAD` back to the existing cherry-pick head branch if you have permission. +- Push to the tracked upstream branch (the remote cherry-pick branch you fetched in step 3): + +```bash +git push +``` + +This works because the local branch was created with `--track` pointing to the cherry-pick head. If push access is denied, the command will fail with a clear error. + - If push fails and the user did not specify a fallback, stop and report the push error instead of inventing a new branch plan.Since the local branch tracks the remote cherry-pick branch (per line 71), a plain
git pushwill push to the correct upstream without ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/fix-cherry-pick-pr/SKILL.md around lines 121 - 122, Replace the vague instruction "Push `HEAD` back to the existing cherry-pick head branch" with an explicit instruction to run a simple push to the branch that the local branch is tracking (i.e., use a plain git push relying on the earlier created tracking branch), and keep the existing behavior that if that push fails and no fallback was provided the agent must stop and report the push error instead of inventing a new branch plan; reference the phrasing "Push `HEAD` back to the existing cherry-pick head branch" and the note that the local branch was created with --track so the plain push targets the correct remote.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/skills/fix-cherry-pick-pr/SKILL.md:
- Around line 63-72: The doc currently names specific bot remotes "tichi" and
"ti-chi-bot" in the examples and prose; update the SKILL.md text around the git
fetch/git switch examples to use generic wording (e.g., "bot remote (commonly
`tichi`/`ti-chi-bot` in tikv/pd)") or add a short note clarifying these are
Codex/PingCAP-specific remotes rather than universal names; update the example
lines that show git fetch and git switch to either use a placeholder like
`<bot-remote>` or include a parenthetical explaining the remote is
example-specific so readers know to replace it with their own remote.
- Around line 101-102: Add an explicit note after the paragraph that includes
"If you need broader signal, run the narrowest additional package or integration
test target that covers the touched files" clarifying that package-level test
commands (for example "make basic-test") do not execute integration tests
located in the tests/ directory; instruct agents to run the integration test
targets explicitly (e.g., run the tests under tests/ or the project's
integration-test target) and to report missing integration-test verification if
they only ran package-level commands.
- Around line 121-122: Replace the vague instruction "Push `HEAD` back to the
existing cherry-pick head branch" with an explicit instruction to run a simple
push to the branch that the local branch is tracking (i.e., use a plain git push
relying on the earlier created tracking branch), and keep the existing behavior
that if that push fails and no fallback was provided the agent must stop and
report the push error instead of inventing a new branch plan; reference the
phrasing "Push `HEAD` back to the existing cherry-pick head branch" and the note
that the local branch was created with --track so the plain push targets the
correct remote.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0ff8595-6909-48f8-ad17-17a938dbf044
📒 Files selected for processing (1)
.agents/skills/fix-cherry-pick-pr/SKILL.md
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: ref #10159
What is changed and how does it work?
This adds a reusable agent skill at
.agents/skills/fix-cherry-pick-prand registers it inAGENTS.md.The skill documents a repeatable workflow for repairing broken cherry-pick PRs in PD:
The goal is to make cherry-pick PR repair and parity verification consistent for future maintenance work.
Check List
Tests
Manual test steps:
.agents/skills/fix-cherry-pick-pr/SKILL.mdAGENTS.mdlinks the new skill from the Agent Skills section.agents/skills/fix-cherry-pick-prCode changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit