axoniq-framework-contribute-pr-comments

# PR Comment Processing Skill Process reviewer comments on a GitHub PR one by one, verifying each comment against the actual code before acting on it. Never blindly apply suggestions — first understand the reviewer's intent, confirm the concern is still valid in the current code, then implement only what is correct. ## Prerequisites This skill requires two CLI tools to be installed and authenticated: - **`gh`** (GitHub CLI) — used to fetch PR comments, reviews, and metadata via the GitHub API. Install: `brew install gh` / `gh auth login`. - **`git`** — used to read the current file state, find the module root, and compare the diff against the working tree. Must be run from within a checkout of the target repository. Check both before doing anything else: ```bash gh auth status git rev-parse --is-inside-work-tree ``` If either check fails, stop and tell the user what is missing before proceeding. ## Arguments - $ARGUMENTS: A GitHub PR reference — full URL (`https://github.com/owner/repo/pull/123`), short form (`owner/repo#123`), or bare number (`123`) when run from within the target repo. --- ## Phase 1: Resolve the PR Reference Determine the owner, repo, and PR number from $ARGUMENTS: ```bash # If a full URL is given, parse it: # https://github.com/AxonIQ/AxonFramework/pull/4552 → owner=AxonIQ repo=AxonFramework pr=4552 # If only a number is given, infer repo from git remote: gh repo view --json nameWithOwner -q .nameWithOwner # Fetch PR metadata (use the resolved $PR_OWNER, $PR_REPO, $PR_NUMBER): gh pr view "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" \ --json title,author,baseRefName,headRefName,headRepository,state,body ``` Extract and store: - `PR_OWNER` — repository owner - `PR_REPO` — repository name - `PR_NUMBER` — PR number - `PR_AUTHOR` — `.author.login` - `PR_BASE_BRANCH` — `.baseRefName` - `PR_HEAD_BRANCH` — `.headRefName` - `PR_HEAD_REPO` — `.headRepository.owner.login` (the fork owner; equals `PR_OWNER` when not a fork) - `PR_STATE` — `.state` (one of `OPEN`, `MERGED`, `CLOSED`) ### Check PR state If `PR_STATE` is `MERGED` or `CLOSED`, inform the user before continuing: > "This PR is already `<PR_STATE>`. You can still process its comments (e.g., to address feedback in a follow-up), but no further commits can be pushed to it directly." Then wait for the user to confirm they want to proceed. ### Verify the local repo matches the PR ```bash # Extract owner/repo from the remote URL — handles both HTTPS and SSH formats: # https://github.com/AxonIQ/AxonFramework.git → AxonIQ/AxonFramework # git@github.com:AxonIQ/AxonFramework.git → AxonIQ/AxonFramework git remote get-url origin \ | sed 's|.*github.com[:/]\(.*\)\.git|\1|; s|.*github.com[:/]\(.*\)|\1|' ``` Compare the extracted `owner/repo` string to `$PR_OWNER/$PR_REPO`. If they do not match, stop and tell the user: > "Your working directory is `<local-repo>` but the PR is on `<PR_OWNER>/<PR_REPO>`. Please navigate to the correct repository." ### Handle forked PRs If `PR_HEAD_REPO` differs from `PR_OWNER` (i.e., the PR comes from a fork), the PR branch is not automatically present locally. Before fetching it, warn the user: ```bash # Show the current branch so the user knows what will change git branch --show-current ``` Tell the user: > "This PR comes from a fork (`<PR_HEAD_REPO>`). To read the correct file versions I need to check out the PR branch locally. Your current branch is `<CURRENT_BRANCH>`. This will switch your working tree — do you want to proceed?" Wait for confirmation before running: ```bash gh pr checkout "$PR_NUMBER" --repo "$PR_OWNER/$PR_REPO" ``` After the skill session is complete, remind the user to switch back to their original branch if needed. --- ## Phase 2: Fetch All Comments Fetch all three comment sources with pagination. Never assume a single page is complete. ### 2a. Inline review comments (line-specific) — with resolved state Resolved thread state is only available via GraphQL — the REST API does not expose it. ```bash # Fetch thread resolution state via GraphQL — paginated with cursor gh api graphql -f query=' query($owner: String!, $repo: String!, $pr: Int!, $cursor: String) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr) { reviewThreads(first: 100, after: $cursor) { pageInfo { hasNextPage endCursor } nodes { id isResolved isOutdated comments(first: 100) { pageInfo { hasNextPage } nodes { databaseId } } } } } } }' -f owner="$PR_OWNER" -f repo="$PR_REPO" -F pr="$PR_NUMBER" ``` Note: `-f` passes a string value; `-F` passes a typed value (integer for the PR number). On the **first call**, omit `-f cursor=...` entirely — `$cursor` is nullable and defaults to null, which fetches from the start. On **subsequent calls**, add `-f cursor="<endCursor value from previous response>"`. If `pageInfo.hasNextPage` is true on `reviewThreads`, repeat the query passing `pageInfo.endCursor` as `$cursor` until `hasNextPage` is false. Collect all `nodes` across pages before proceeding. If any thread's `comments.pageInfo.hasNextPage` is true, that thread has more than 100 replies (extremely rare). Fetch the remaining `databaseId`s with a cursor-paginated `node(id: <thread id>) { ... on PullRequestReviewThread { comments(first: 100, after: $cursor) ... } }` query scoped to that thread, repeating until `hasNextPage` is false, and merge the extra `databaseId`s into the thread's mapping. Use the `databaseId` values to map each inline comment to its thread's `isResolved` and `isOutdated` flags. Then fetch the inline comments themselves: ```bash gh api --paginate \ "repos/$PR_OWNER/$PR_REPO/pulls/$PR_NUMBER/comments" \ --jq '[.[] | { id: .id, in_reply_to_id: .in_reply_to_id, author: .user.login, body: .body, path: .path, line: (.line // .original_line), diff_hunk: .diff_hunk, created_at: .created_at }]' ``` Merge the two results: attach `isResolved` and `isOutdated` from GraphQL to each comment using the `databaseId` → `id` mapping. ### 2b. Review bodies (top-level reviewer summaries) ```bash gh api --paginate \ "repos/$PR_OWNER/$PR_REPO/pulls/$PR_NUMBER/reviews" \ --jq '[.[] | select(.body != "" and .body != null) | { id: .id, author: .user.login, body: .body, state: .state, submitted_at: .submitted_at, source: "review_body" }]' ``` Review bodies have no file or line attached. However, a reviewer may mention specific files, classes, or line numbers in their prose (e.g., "line 87 in `Foo.java` looks wrong"). When processing a review body comment in Phase 9, scan the text for: - Explicit file names or paths (e.g., `Foo.java`, `org/axonframework/...`) - Line number references (e.g., "line 87", "L87") - Class or method names that appear in the PR's changed files If such references are found, look up the relevant code and treat the review body as if it were an inline comment on that location. If the references are ambiguous or no specific location can be inferred, present the review body as a general concern and ask the user where it applies. ### 2c. Issue comments (general conversation on the PR) ```bash gh api --paginate \ "repos/$PR_OWNER/$PR_REPO/issues/$PR_NUMBER/comments" \ --jq '[.[] | { id: .id, author: .user.login, body: .body, created_at: .created_at, source: "issue_comment" }]' ``` --- ## Phase 3: Build Comment Threads Group inline comments into threads using `in_reply_to_id`: - A comment with no `in_reply_to_id` is a **thread root**. - Comments with `in_reply_to_id` are **replies** — attach them to their root in chronological order. - Each thread carries the `isResolved` and `isOutdated` flags from Phase 2a. Each thread becomes one unit — not individual comments. Review bodies (2b) and issue comments (2c) are standalone units with no threading. --- ## Phase 4: Filter Noise Remove units that do not require action: | Filter | Rule | |---|---| | **Bot comments** | Author login ends in `[bot]` (e.g., `github-actions[bot]`, `sonarcloud[bot]`) | | **Resolved threads** | `isResolved` is true, unless a reply in the thread after the resolution expresses disagreement (e.g., "this wasn't actually addressed", "I still think…") — in that case keep the thread and mark it `needs discussion` | | **Pure praise** | Thread contains only positive sentiment with no actionable request ("LGTM", "Nice!", "+1") | **Do NOT filter:** - Resolved threads where the resolution is contested - Threads with `isOutdated: true` — flag them but keep them; the underlying concern may still apply to current code - The PR author's own replies — these are essential context for understanding the thread If after filtering there are zero actionable units, stop and report: > "No actionable comments found — all threads are resolved, bot-generated, or purely positive." --- ## Phase 5: Classify Each Thread Assign one label and one status per thread. **Classification label** (based on content of the root comment): | Label | Meaning | |---|---| | `[SUGGESTION]` | Contains a GitHub suggestion block (` ```suggestion `) — a ready-made code replacement | | `[CHANGE]` | Requests a code change, rename, refactor, or restructure | | `[NIT]` | Minor style or cosmetic issue, low priority | | `[QUESTION]` | Asks why/how something works; may or may not require a code change | | `[DOCS]` | Requests documentation or JavaDoc update | | `[TEST]` | Requests new or improved test coverage | **Status flag** (set during classification or updated in Phase 9 Step 3 after reading the code): | Status | Set when | Meaning | |---|---|---| | `valid` | Phase 5 | Concern appears actionable based on the thread content | | `stale` | Phase 5 | `isOutdated: true` from GitHub — code has changed since comment was written; verify before acting | | `questionable` | Phase 5 **or** Phase 9 Step 3 | Reviewer's premise appears to be based on a misread — either obvious from the comment text alone, or discovered after reading the actual code. Do not implement; discuss first. | | `needs discussion` | Phase 5 | `[QUESTION]` thread requiring a response rather than a code change | `questionable` can be detected in two places: in Phase 5 when the comment text contains a clearly incorrect assertion (e.g., "this method is never called" when it clearly is), or in Phase 9 Step 3 after reading the code and finding the concern does not hold. Either way, the handling is the same: do not implement, draft a reply, wait for user confirmation. A `[QUESTION]` that the reviewer answers themselves ("I see now that…") within the same thread can be downgraded to resolved and filtered in Phase 4. --- ## Phase 6: Build the Overview Do a **lightweight** pass to build the overview table — do not read every file here. That would be prohibitively slow for large PRs and will be done per-comment during Phase 9 when it is actually needed. For this pass, only extract from the data already fetched: - File path and line number from the comment - Whether the file path resolves in the local working tree: `git ls-files --error-unmatch <path> 2>/dev/null` For paths that do not resolve, set their Status column to `⚠️ path not found` in the table and add a note below the table listing the affected comment numbers and their paths: ``` ⚠️ The following paths could not be found in the local working tree. The file may have been deleted, renamed, or the wrong branch is checked out: - Comment #7: messaging/src/main/java/org/axonframework/OldClass.java ``` Output a numbered overview table, sorted by file then line number, grouped by file: ``` # PR #4552 — Review Comments Overview ## AxonIQ/AxonFramework — "Fix handler wrapper chain preservation" Total: 12 threads | 9 actionable | 2 stale | 1 resolved --- ### messaging/src/main/java/org/axonframework/.../AnnotatedEventHandlingComponent.java | # | Lines | Author | Label | Summary | Status | |---|-------|--------|-------|---------|--------| | 1 | 87–92 | reviewer-a | [CHANGE] | Replace instanceof with canHandleMessageType() | valid | | 2 | 102 | reviewer-a | [NIT] | Rename local variable for clarity | valid | | 3 | 115 | reviewer-b | [QUESTION] | Why not unwrap here? | needs discussion | ### messaging/src/test/java/org/axonframework/.../AnnotatedEventHandlingComponentTest.java | # | Lines | Author | Label | Summary | Status | |---|-------|--------|-------|---------|--------| | 4 | 44 | reviewer-a | [TEST] | Add test for wrapped handler scenario | valid | ### (General PR comments — no file) | # | Author | Label | Summary | Status | |---|--------|-------|---------|--------| | 5 | reviewer-b | [QUESTION] | Confirm this doesn't affect replay behavior | needs discussion | --- **Dependencies detected:** - Comments #1 and #4 are linked: fixing #1 (code change) requires adding test #4 to verify it. - Comment #2 is independent. ``` --- ## Phase 7: Create Todo List Use TaskCreate to add a task for each actionable thread. Format: ``` PR#<number> #<N>: [LABEL] — <brief description> (<file>:<line>) ``` Example: ``` PR#4552 #1: [CHANGE] — Replace instanceof with canHandleMessageType() (AnnotatedEventHandlingComponent.java:87) PR#4552 #2: [NIT] — Rename local variable (AnnotatedEventHandlingComponent.java:102) PR#4552 #3: [QUESTION] — Discuss: why not unwrap at line 115? PR#4552 #4: [TEST] — Add wrapped handler test (AnnotatedEventHandlingComponentTest.java:44) PR#4552 #5: [QUESTION] — Confirm no replay behavior regression ```