Skip to content

Add PPL bugfix skill for Claude Code#5307

Open
qianheng-aws wants to merge 9 commits intoopensearch-project:mainfrom
qianheng-aws:bugfix-skill
Open

Add PPL bugfix skill for Claude Code#5307
qianheng-aws wants to merge 9 commits intoopensearch-project:mainfrom
qianheng-aws:bugfix-skill

Conversation

@qianheng-aws
Copy link
Copy Markdown
Collaborator

@qianheng-aws qianheng-aws commented Apr 3, 2026

Summary

Adds a /ppl-bugfix slash command for Claude Code that automates PPL bug fixing end-to-end: triage, classification, TDD-style fix, testing, and PR creation — all driven by a structured harness.

Who is this for?

Developers working on the OpenSearch SQL plugin who use Claude Code as their coding assistant. The command turns a GitHub issue number into a draft PR with fix, tests, and decision log.

How it works

/ppl-bugfix #1234                    # Fix a single issue
/ppl-bugfix PR#5293                  # Follow up on an existing PR (CI failures, review feedback, merge conflicts)
/ppl-bugfix #1234 #5678 PR#9012     # Process multiple in parallel

Each issue/PR dispatches an autonomous subagent in an isolated git worktree (no interference with your working directory). The subagent follows a structured harness:

  1. Phase 0 — Reproduce bug, classify layer (Grammar / AST / Type System / Optimizer / Execution / DI), route to fix path
  2. Phase 1 — Implement fix following layer-specific guidance with historical commit references
  3. Phase 2 — TDD: write failing test first, then fix, then complete coverage (unit + integration + YAML REST)
  4. Phase 3 — Verify (spotless + full test suite), commit with DCO, create draft PR, post Decision Log

Permission modes control how much autonomy the subagent has:

  • --yolo (default) — bypassPermissions, zero prompts, fastest
  • --safeacceptEdits only, Bash commands require manual approval

Files

File Purpose
.claude/commands/ppl-bugfix.md Slash command definition — input parsing, issue/PR resolution, subagent dispatch
.claude/harness/ppl-bugfix-harness.md Full bugfix workflow (Phase 0-4) with fix paths, test templates, case index
.claude/harness/ppl-bugfix-followup.md Post-PR follow-up: review feedback, CI failures, merge conflicts, commit cleanup
.claude/settings.json Pre-approved tool permissions (gradle, gh, git commands)
CLAUDE_GUIDE.md User-facing documentation for all slash commands

Example PRs produced by this command

Test plan

  • Run /ppl-bugfix #<issue> on a real open issue — verify subagent dispatches in worktree, follows harness phases, creates draft PR
  • Run /ppl-bugfix PR#<number> on an existing PR with review comments — verify follow-up agent addresses feedback
  • Run /ppl-bugfix #1234 #5678 — verify parallel dispatch of multiple subagents
  • Run /ppl-bugfix #1234 --safe — verify acceptEdits mode (Bash commands prompt for approval)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 42caa29.

PathLineSeverityDescription
.claude/commands/ppl-bugfix.md21mediumbypassPermissions is the default mode for subagents ('--yolo'). Combined with the allowed gh and git push permissions, a prompt-injected subagent (e.g. via malicious issue content fetched from GitHub) could push branches, create PRs, or modify issues without any human approval prompt.
.claude/settings.json21mediumBash(git push --force-with-lease:*) is explicitly allowed. Force-push capability is granted unconditionally to all subagents with no branch filter, meaning a compromised or misbehaving agent could overwrite history on any branch including main without a manual approval gate.
.claude/harness/ppl-bugfix-harness.md261lowThe fallback fork owner is hardcoded as 'qianheng-aws'. If this username does not belong to an authorized team member, the harness would push fix branches to a third-party fork, potentially exfiltrating in-progress code or creating PRs from an uncontrolled account.
.claude/settings.json5lowBroad gh issue:*, gh pr:*, and gh run:* permissions allow subagents to create, edit, and close GitHub issues and PRs, and re-trigger CI runs, all without manual approval. A prompt-injected agent could abuse these to manipulate project state (e.g. close legitimate issues, spam comments, or trigger repeated CI runs).

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 2 | Low: 2


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

PR Reviewer Guide 🔍

(Review updated until commit 42caa29)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
The harness instructs the subagent to infer the fork owner from the git user name and fall back to a hardcoded username (qianheng-aws). If the git config contains a real developer's username, it could be used to push to that person's fork without explicit confirmation. Additionally, bypassPermissions mode (the default) allows the subagent to execute arbitrary gh and git commands — including creating PRs and pushing branches — without any human approval, which could expose internal branch names, commit history, or draft PR content prematurely.

✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add PPL bugfix slash command and harness files

Relevant files:

  • .claude/commands/ppl-bugfix.md
  • .claude/harness/ppl-bugfix-harness.md
  • .claude/harness/ppl-bugfix-followup.md
  • .claude/settings.json

Sub-PR theme: Update documentation for PPL bugfix command

Relevant files:

  • CLAUDE.md
  • CLAUDE_GUIDE.md

⚡ Recommended focus areas for review

Overly Broad Permissions

The git reset --soft:* permission is very broad and could allow resetting to arbitrary refs, potentially causing data loss. Similarly, git push --force-with-lease:* and git cherry-pick:* with wildcard arguments could be misused. Consider restricting these to specific safe patterns or documenting the risk explicitly.

"Bash(git reset --soft:*)"
Hardcoded Fork Owner

The fallback fork owner is hardcoded as "qianheng-aws" in the push instructions. This is a project-specific developer name that will be wrong for any other contributor using this command, potentially causing pushes to fail or go to the wrong repository.

# Use the git user name to infer the fork owner, or fall back to "qianheng-aws"
Missing Error Handling

The merge conflict resolution section runs ./gradlew test && ./gradlew :integ-test:integTest inline without any guidance on what to do if tests fail after the merge. There is no fallback or escalation path defined for post-merge test failures, which could leave the PR in a broken state.

git fetch origin && git merge origin/main  # Resolve conflicts
./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest  # Re-verify
git commit -s -m "Resolve merge conflicts with main"
git push -u fork <branch_name>

</details>

<details><summary><a href='https://github.com/opensearch-project/sql/pull/5307/files#diff-342b7659b9763f77dd1212d00dc7430e3849578bc01a738b77fdbec1c180bd50R37-R37'><strong>Default Bypass Permissions</strong></a>

The default permission mode is `bypassPermissions` (yolo), meaning any invocation without an explicit flag runs with full autonomy. While the worktree isolation mitigates some risk, a user unfamiliar with the default could unknowingly trigger unrestricted bash execution including git pushes and PR creation without any approval prompts.
</summary>

```markdown
| _(no flag)_ | `bypassPermissions` (default) |

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

PR Code Suggestions ✨

Latest suggestions up to 42caa29

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Remove hardcoded username fallback for fork owner

Hardcoding a specific GitHub username (qianheng-aws) as a fallback fork owner is a
critical bug — this will cause pushes to fail or push to the wrong fork for any
other contributor. The fallback should instead prompt the user or fail with a clear
error message rather than silently using a hardcoded username.

.claude/harness/ppl-bugfix-harness.md [241]

-# Use the git user name to infer the fork owner, or fall back to "qianheng-aws"
+# Use the git user name to infer the fork owner; if unknown, prompt the user for the fork owner before proceeding
Suggestion importance[1-10]: 6

__

Why: Hardcoding qianheng-aws as a fallback fork owner is a real issue that would cause failures or incorrect pushes for any other contributor. The suggestion to prompt the user instead is valid, though the improved code only changes a comment rather than adding actual resolution logic.

Low
Resolve unresolved fork owner placeholder

The <fork_owner> placeholder is never resolved in this file — there is no instruction on how to
determine the fork owner in the follow-up harness, unlike the main harness which at
least mentions inferring from git user name. This will cause the command to fail
literally. Add explicit instructions to resolve the fork owner (e.g., from gh api
user --jq .login) before running this command.

.claude/harness/ppl-bugfix-followup.md [20]

-git remote add fork https://github.com/<fork_owner>/sql.git
+# Resolve fork owner from GitHub CLI
+FORK_OWNER=$(gh api user --jq '.login')
+git remote add fork https://github.com/${FORK_OWNER}/sql.git
Suggestion importance[1-10]: 5

__

Why: The <fork_owner> placeholder in the follow-up harness is indeed unresolved with no instructions on how to determine it. Using gh api user --jq '.login' is a concrete and practical solution, though this is a documentation/instruction file so the agent is expected to interpret placeholders contextually.

Low
General
Use dynamic fork owner resolution via CLI

The fork owner resolution is vague — "use the git user name" doesn't specify which
git config key to read, and the fallback is a hardcoded username. Replace this with
a concrete command to resolve the fork owner dynamically using the GitHub CLI, which
is already an allowed tool.

.claude/harness/ppl-bugfix-harness.md [240-241]

-git remote add fork https://github.com/<fork_owner>/sql.git
-# Use the git user name to infer the fork owner, or fall back to "qianheng-aws"
+FORK_OWNER=$(gh api user --jq '.login')
+git remote add fork https://github.com/${FORK_OWNER}/sql.git
Suggestion importance[1-10]: 5

__

Why: Replacing the vague "use the git user name" instruction and hardcoded fallback with gh api user --jq '.login' is more concrete and portable. This overlaps significantly with suggestion 2 but provides a more actionable implementation.

Low
Security
Tighten overly broad git push permission

The permission Bash(git push -u:) is overly broad and would also match git push -u
fork main which could push to unintended branches. More importantly, git push
without --force-with-lease is missing as a standalone permission, but the current
pattern git push -u:
won't match plain git push fork calls used in the harness.
Verify that all git push variants used in the harness are covered and that the
patterns are as restrictive as possible.

.claude/settings.json [21-22]

-"Bash(git push -u:*)",
+"Bash(git push -u fork:*)",
+"Bash(git push fork:*)",
 "Bash(git push --force-with-lease:*)",
Suggestion importance[1-10]: 4

__

Why: The concern about git push -u:* being overly broad has some merit, but the pattern matching in Claude's permission system may work differently than standard glob matching. The suggestion to add Bash(git push fork:*) is reasonable but the security impact is limited since the harness already runs in isolated worktrees.

Low

Previous suggestions

Suggestions up to commit 45bbb51
CategorySuggestion                                                                                                                                    Impact
Security
Remove hardcoded username fallback for fork remote

Hardcoding a specific GitHub username (qianheng-aws) as a fallback for the fork
owner is a security and correctness concern — it could cause code to be pushed to
the wrong repository if the git user name cannot be resolved. This fallback should
be removed or replaced with a prompt to the user to provide the correct fork owner.

.claude/harness/ppl-bugfix-harness.md [241]

-# Use the git user name to infer the fork owner, or fall back to "qianheng-aws"
+# Use the git user name to infer the fork owner; if unknown, ask the user before proceeding
Suggestion importance[1-10]: 6

__

Why: Hardcoding qianheng-aws as a fallback fork owner is a real concern — it could cause pushes to the wrong repository for other contributors. Replacing it with a prompt to ask the user is a meaningful correctness and security improvement.

Low
General
Add conflict check before committing merge resolution

The merge conflict resolution runs all integration tests unconditionally, which can
be very slow and may not be necessary for simple conflicts. More critically, if git
merge results in conflicts that need manual resolution, the subsequent commands will
fail or commit an incomplete state. Consider adding a conflict-check step before
committing.

.claude/harness/ppl-bugfix-followup.md [80-83]

 git fetch origin && git merge origin/main  # Resolve conflicts
+# Check for unresolved conflicts before proceeding
+git diff --check
 ./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest  # Re-verify
 git commit -s -m "Resolve merge conflicts with main"
-git push -u fork <branch_name>
+git push fork <branch_name>
Suggestion importance[1-10]: 5

__

Why: Adding git diff --check before committing is a reasonable safeguard to detect unresolved merge conflicts before they get committed. This is a valid improvement to the harness workflow that could prevent committing broken state.

Low
Possible issue
Fix overly restrictive git push permission patterns

The git push -u permission pattern may not match git push -u fork commands as
intended, since the -u flag is part of the command arguments rather than the command
name. Consider using a broader git push pattern or verifying that the permission
syntax correctly matches the actual commands used in the harness. Additionally, git
push --force-with-lease is a separate pattern that may not cover all force-push
variants used in the follow-up harness.

.claude/settings.json [21-22]

-"Bash(git push -u:*)",
-"Bash(git push --force-with-lease:*)",
+"Bash(git push:*)",
Suggestion importance[1-10]: 4

__

Why: The concern about git push -u pattern matching is valid since permission patterns in Claude settings may need to match the full command string. However, broadening to Bash(git push:*) could be too permissive, and the current patterns may already work correctly depending on how the permission system parses them.

Low
Suggestions up to commit ce54987
CategorySuggestion                                                                                                                                    Impact
General
Scope post-conflict re-verification to affected tests

After resolving merge conflicts, the harness runs the full integration test suite
(integTest) without scoping it to the relevant test class. This can be very slow and
may time out. Consider scoping the re-verification to the affected test class first,
consistent with how Phase 3.1 in the main harness structures verification steps.

.claude/harness/ppl-bugfix-followup.md [81]

-./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest  # Re-verify
+./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest -Dtests.class="*<YourIT>"  # Re-verify affected IT
+./gradlew :integ-test:integTest  # Full IT regression if time permits
Suggestion importance[1-10]: 5

__

Why: Running the full integTest suite after every merge conflict resolution is indeed slow and could time out. Scoping to the affected test class first is a practical improvement consistent with the main harness's approach in Phase 3.1, making this a reasonable maintainability suggestion.

Low
Ensure all git push variants are permitted

The git push -u permission pattern may not match git push -u fork correctly
depending on how Claude Code parses permission patterns. Additionally, a plain git
push (without flags) is not covered, which could block legitimate push operations.
Consider adding "Bash(git push:*)" to ensure all push variants are permitted.

.claude/settings.json [21-22]

-"Bash(git push -u:*)",
-"Bash(git push --force-with-lease:*)",
+"Bash(git push:*)",
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about git push variants not being covered, but the existing patterns Bash(git push -u:*) and Bash(git push --force-with-lease:*) cover the specific push commands used in the harness. Adding a blanket Bash(git push:*) would be more permissive than needed, and the improved_code removes the existing specific patterns rather than adding to them.

Low
Add fork owner fallback in follow-up harness

The fork owner placeholder <fork_owner> is hardcoded as a template variable with no fallback
logic in the follow-up harness, but the main harness (ppl-bugfix-harness.md)
provides a fallback to "qianheng-aws". The follow-up harness should include the same
fallback instruction so agents don't get stuck when the fork owner cannot be
inferred.

.claude/harness/ppl-bugfix-followup.md [20]

 git remote add fork https://github.com/<fork_owner>/sql.git
+# Use the git user name to infer the fork owner, or fall back to "qianheng-aws"
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies an inconsistency between the follow-up harness and the main harness regarding the <fork_owner> fallback. However, the improved_code only adds a comment rather than substantive logic, making this a minor documentation/consistency improvement.

Low
Suggestions up to commit 6b29b60
CategorySuggestion                                                                                                                                    Impact
Possible issue
Include merged/closed PRs in issue resolution lookup

These gh pr list searches only look at open PRs by default. A PR that was already
merged or closed would not be found, causing the agent to incorrectly classify a
follow-up as an "Initial Fix" and create a duplicate PR. Add --state all to each
search to cover merged and closed PRs as well.

.claude/commands/ppl-bugfix.md [47-49]

-gh pr list --search "Resolves #<issue_number>" --json number,url,state --limit 5
-gh pr list --search "Fixes #<issue_number>" --json number,url,state --limit 5
-gh pr list --search "Closes #<issue_number>" --json number,url,state --limit 5
+gh pr list --search "Resolves #<issue_number>" --json number,url,state --state all --limit 5
+gh pr list --search "Fixes #<issue_number>" --json number,url,state --state all --limit 5
+gh pr list --search "Closes #<issue_number>" --json number,url,state --state all --limit 5
Suggestion importance[1-10]: 7

__

Why: This is a valid and important issue — without --state all, merged PRs won't be found, potentially causing the agent to create duplicate PRs for already-fixed issues. The improved_code correctly adds --state all to each search command.

Medium
Add missing plain git push permission

The permission Bash(git push -u:) only matches git push -u as a prefix, but git
push --force-with-lease and plain git push are separate patterns. More critically,
git push without -u or --force-with-lease is not covered, which may block the
subagent when pushing without those flags. Consider adding a plain Bash(git push:
)
entry or verifying the pattern matching behavior of the tool.

.claude/settings.json [21-22]

 "Bash(git push -u:*)",
 "Bash(git push --force-with-lease:*)",
+"Bash(git push:*)",
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that git push without -u or --force-with-lease flags isn't covered, which could block the subagent in some scenarios. However, the harness files consistently use either git push -u or git push --force-with-lease, so the practical impact may be limited.

Low
General
Handle cherry-pick conflicts before amending commit

After git cherry-pick, the working tree may have conflicts that need resolution
before git commit --amend is valid. Additionally, if the cherry-pick produces a
merge commit or fails, the subsequent amend will operate on the wrong commit. Add a
conflict-check step or note between cherry-pick and amend to handle this case.

.claude/harness/ppl-bugfix-followup.md [52-55]

 git checkout -B clean-branch origin/main
 git cherry-pick <fix_commit_sha>
+# Resolve any conflicts before amending
 git commit --amend -s -m "<updated message>"
 git push <your_fork_remote> clean-branch:<pr_branch> --force-with-lease
Suggestion importance[1-10]: 2

__

Why: The suggestion only adds a comment to the existing code without any functional change, making the existing_code and improved_code effectively equivalent in behavior. This is documentation-level guidance rather than a real fix.

Low
Suggestions up to commit 638f0ce
CategorySuggestion                                                                                                                                    Impact
Possible issue
Include merged/closed PRs in issue resolution lookup

These searches only look for open PRs by default. A closed or merged PR that
resolves the issue would not be found, potentially causing the agent to create a
duplicate PR for an already-fixed issue. Add --state all to each search to catch
merged or closed PRs.

.claude/commands/ppl-bugfix.md [47-49]

-gh pr list --search "Resolves #<issue_number>" --json number,url,state --limit 5
-gh pr list --search "Fixes #<issue_number>" --json number,url,state --limit 5
-gh pr list --search "Closes #<issue_number>" --json number,url,state --limit 5
+gh pr list --search "Resolves #<issue_number>" --json number,url,state --state all --limit 5
+gh pr list --search "Fixes #<issue_number>" --json number,url,state --state all --limit 5
+gh pr list --search "Closes #<issue_number>" --json number,url,state --state all --limit 5
Suggestion importance[1-10]: 6

__

Why: This is a valid functional concern — without --state all, the agent could miss already-merged PRs and create duplicate work. The improved_code accurately reflects the change needed.

Low
Security
Restrict push permissions to fork remote only

The git push permissions are overly broad — git push -u: and git push
--force-with-lease:
allow pushing to any remote and branch. A compromised or
misbehaving subagent could force-push to upstream branches. Consider restricting to
known fork remotes or at minimum documenting that origin should never point to
upstream.

.claude/settings.json [21-22]

-"Bash(git push -u:*)",
-"Bash(git push --force-with-lease:*)",
+"Bash(git push -u <your_fork_remote>:*)",
+"Bash(git push --force-with-lease <your_fork_remote>:*)",
Suggestion importance[1-10]: 3

__

Why: While the security concern about overly broad push permissions is valid, the improved_code uses a literal placeholder <your_fork_remote> which is not a valid permission pattern in settings.json — the actual remote name would need to be hardcoded, which isn't practical for a shared config file.

Low
General
Handle cherry-pick conflicts before amending commit

After git cherry-pick, if the cherry-pick itself creates a commit, running git
commit --amend will amend that cherry-pick commit correctly. However, if the
cherry-pick fails due to conflicts, the subsequent git commit --amend will fail or
produce unexpected results. Add a conflict check or note that conflicts must be
resolved before amending.

.claude/harness/ppl-bugfix-followup.md [52-55]

 git checkout -B clean-branch origin/main
 git cherry-pick <fix_commit_sha>
-git commit --amend -s -m "<updated message>"
+# Resolve any conflicts before proceeding
+git commit --amend -s --no-edit -m "<updated message>"
 git push <your_fork_remote> clean-branch:<pr_branch> --force-with-lease
Suggestion importance[1-10]: 3

__

Why: The suggestion adds a comment about resolving conflicts, but the improved_code also changes git commit --amend -s -m to add --no-edit while still keeping -m, which is contradictory — --no-edit and -m together would conflict. The improvement is marginal and the code change is slightly inaccurate.

Low
Clarify remote naming to avoid push/sync confusion

The git merge origin/main step merges from origin, but the harness instructs pushing
to a personal fork remote (not origin). If origin points to the upstream repo, this
is correct for syncing, but the inconsistency with the push step (which uses <your_fork_remote>) may
confuse the agent. Clarify that origin here refers to the upstream remote, distinct
from the fork remote used for pushing.

.claude/harness/ppl-bugfix-harness.md [235-238]

-# Sync with main (merge, not rebase — PRs use squash-merge)
+# Sync with upstream main (origin = upstream; PRs use squash-merge, so merge not rebase)
 git fetch origin && git merge origin/main
 # Always re-run tests after merge — even a trivial merge can introduce regressions
 ./gradlew test && ./gradlew :integ-test:integTest -Dtests.class="*<YourIT>"
Suggestion importance[1-10]: 2

__

Why: The suggestion only adds a comment clarifying that origin is the upstream remote, which is a documentation-only change with minimal functional impact. The existing_code and improved_code differ only in a comment.

Low
Suggestions up to commit 57bfdb0
CategorySuggestion                                                                                                                                    Impact
Security
Restrict broad git push permissions

The wildcard git push:* permission allows force-pushing to any remote, including
upstream. The harness instructs agents to push to a personal fork, but this broad
permission could allow accidental or malicious force-pushes to the upstream origin.
Consider restricting to --force-with-lease only or scoping to known fork remotes.

.claude/settings.json [21]

-"Bash(git push:*)"
+"Bash(git push --force-with-lease:*)",
+"Bash(git push -u:*)"
Suggestion importance[1-10]: 6

__

Why: The wildcard git push:* permission is overly broad and could allow force-pushing to upstream remotes. The harness explicitly instructs agents to push to a personal fork, but the permission doesn't enforce this boundary, making accidental upstream pushes possible.

Low
Restrict destructive git reset permissions

The git reset --hard command can permanently destroy uncommitted work, and git reset
with broad wildcard permissions allows any variant including destructive ones.
Consider restricting to only the safe soft-reset variant or removing this permission
entirely, since the harness only documents git reset --soft usage in a specific
cherry-pick scenario.

.claude/settings.json [24]

-"Bash(git reset:*)"
+"Bash(git reset --soft:*)"
Suggestion importance[1-10]: 5

__

Why: While git reset --hard can be destructive, the subagents run in isolated worktrees which limits the blast radius. The suggestion is valid but the risk is somewhat mitigated by the worktree isolation design. Restricting to --soft only would be safer but may break legitimate use cases.

Low
General
Deduplicate overlapping PR search results

These three gh pr list searches run sequentially but could return overlapping
results if a PR body contains multiple closing keywords. The results should be
deduplicated by PR number before routing to Step 2A or 2B, otherwise the same PR
could be dispatched as multiple subagents.

.claude/commands/ppl-bugfix.md [47-49]

 gh pr list --search "Resolves #<issue_number>" --json number,url,state --limit 5
 gh pr list --search "Fixes #<issue_number>" --json number,url,state --limit 5
 gh pr list --search "Closes #<issue_number>" --json number,url,state --limit 5
+# Deduplicate results by PR number before routing
Suggestion importance[1-10]: 3

__

Why: The deduplication concern is valid but the improved_code only adds a comment rather than actual deduplication logic. Since this is a markdown instruction file for an AI agent, the agent should be capable of deduplicating results itself, making this a low-impact suggestion.

Low
Guard commit behind passing test verification

The merge conflict resolution flow commits and pushes without verifying that the
tests actually passed. If ./gradlew test or integTest fails, the agent should stop
and report rather than committing a broken state. Add an explicit failure check or
note before the commit step.

.claude/harness/ppl-bugfix-followup.md [72-74]

-./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest  # Re-verify
+./gradlew spotlessApply && ./gradlew test && ./gradlew :integ-test:integTest  # Re-verify — stop and report if any step fails
+# Only proceed if all tests pass
 git commit -s -m "Resolve merge conflicts with main"
 git push <your_fork_remote> <branch_name>
Suggestion importance[1-10]: 3

__

Why: The && chaining in the existing command already ensures that subsequent steps only run if previous ones pass. The improved_code only adds a comment rather than substantive logic change, making this a minimal improvement.

Low

- ppl-bugfix-harness.md: systematic bugfix SOP distilled from 15+
  historical commits, covering triage, TDD-style fix paths (A-E by
  bug layer), test templates (UT/IT/YAML), verification, PR creation,
  decision log, and post-PR follow-up
- .claude/commands/ppl-bugfix.md: slash command that auto-resolves
  issue↔PR, dispatches subagent with worktree isolation and scoped
  permissions, handles both initial fix and follow-up flows
- CLAUDE.md: reference to /ppl-bugfix as the entry point for PPL bugs
- .gitignore: allow .claude/commands/ and settings.json to be tracked

Signed-off-by: Heng Qian <qianheng@amazon.com>
… reorg

- Add --safe/--yolo permission mode flags (default: bypassPermissions)
- Support multiple issue/PR references for parallel processing
- Move harness files to .claude/harness/ directory
- Add ppl-bugfix-followup.md for post-PR follow-up workflow
- Add .claude/settings.json with pre-approved tool permissions
- Add CLAUDE_GUIDE.md documenting all slash commands

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws qianheng-aws added the maintenance Improves code quality, but not the product label Apr 3, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit d4b9b01

The Reconstruct Context section now explicitly loads all PR comments
(bot and human) and categorizes bot suggestions as actionable review
feedback, not just CI checks and human reviews.

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit b9584ee

- Add missing git merge permission to settings.json (used in harness merge steps)
- Search all PR closing keyword variants (Resolves/Fixes/Closes) for issue-PR linkage
- Replace grep -oP with portable grep -oE for macOS compatibility
- Use --force-with-lease instead of --force in cherry-pick cleanup flow
- Make post-merge test re-run mandatory with explicit command
- Add --repo opensearch-project/sql to gh pr create for correct targeting

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit 57bfdb0

- Replace broad `git push:*` with `git push -u:*` and `git push --force-with-lease:*`
  to prevent accidental force-pushes to upstream
- Restrict `git reset:*` to `git reset --soft:*` since the harness only uses soft reset
- Update followup harness push commands to use `-u` flag consistently

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit 638f0ce

Followup agent now reflects on each feedback comment to identify
gaps in the harness that should have prevented the issue, and
fixes them in the same commit.

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit 6b29b60

- Add fork remote discovery step to both harness and followup
  (replace <your_fork_remote> placeholder with concrete instructions)
- Remove redundant follow-up type from command dispatch — subagent
  discovers it from PR state via followup harness categorization table
- Fix Phase 0.1: remove fake REST API call, use integration test only
- Add no-coauthor rule to followup harness (was only in harness)
- Add --repo flag to gh pr checkout in followup for worktree context

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit ce54987

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit 45bbb51

Subagent was ignoring the "stop and report back" instruction and still
creating regression tests + PRs for bugs that no longer reproduce.

- Harness: clarify definition of "not reproducible", replace soft
  "stop and report back" with explicit HARD STOP + forbidden actions
- Skill: add CRITICAL instruction in subagent dispatch prompt so it
  sees the constraint before even reading the harness

Signed-off-by: Heng Qian <qianheng@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2026

Persistent review updated to latest commit 42caa29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Improves code quality, but not the product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant