From 728e400f46150b738e860a47c28ee164051c5685 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 10 Aug 2026 17:59:27 -0700 Subject: [PATCH 1/4] fix(review-pr): confirm a Copilot request via GraphQL, not REST The re-trigger itself was fine. What was broken is the check that follows it: REST requested_reviewers lists Users only, and Copilot is a Bot, so it never appears there however the request was made. The documented grep therefore reported failure on every successful request, which makes a working trigger look broken and sends the loop hunting for a replacement it does not need. Confirmation now reads GraphQL reviewRequests, where the bot does appear - without the `[bot]` suffix it carries on the review it later submits, so each form is matched where it actually occurs. Verified on a live PR: `gh pr edit --add-reviewer @copilot` moves reviewRequests from empty to copilot-pull-request-reviewer while REST requested_reviewers stays empty throughout. Also records in efficient-orchestration that a subagent can go idle without returning anything. Silence is a failure to be retried once and then taken back in-house, not an empty result: treating it as "nothing found" reports a clean sweep over work that was never done. --- .antigravity/skills/efficient-orchestration/SKILL.md | 2 ++ .antigravity/skills/review-pr/SKILL.md | 10 +++++++++- .claude/commands/efficient-orchestration.md | 2 ++ .claude/commands/review-pr.md | 10 +++++++++- .codex/skills/efficient-orchestration/SKILL.md | 2 ++ .codex/skills/review-pr/SKILL.md | 10 +++++++++- .copilot/skills/efficient-orchestration/SKILL.md | 2 ++ .copilot/skills/review-pr/SKILL.md | 10 +++++++++- .kimi-code/skills/efficient-orchestration/SKILL.md | 2 ++ .kimi-code/skills/review-pr/SKILL.md | 10 +++++++++- 10 files changed, 55 insertions(+), 5 deletions(-) diff --git a/.antigravity/skills/efficient-orchestration/SKILL.md b/.antigravity/skills/efficient-orchestration/SKILL.md index 453d903..4c3c3b3 100644 --- a/.antigravity/skills/efficient-orchestration/SKILL.md +++ b/.antigravity/skills/efficient-orchestration/SKILL.md @@ -49,6 +49,8 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. + ## Guardrails - Don't delegate a blocker your next step needs. diff --git a/.antigravity/skills/review-pr/SKILL.md b/.antigravity/skills/review-pr/SKILL.md index a7dbad5..69c7f80 100644 --- a/.antigravity/skills/review-pr/SKILL.md +++ b/.antigravity/skills/review-pr/SKILL.md @@ -83,7 +83,15 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: + + ```bash + gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + ``` + + Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. - App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. - **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. diff --git a/.claude/commands/efficient-orchestration.md b/.claude/commands/efficient-orchestration.md index 06e3546..f6285bf 100644 --- a/.claude/commands/efficient-orchestration.md +++ b/.claude/commands/efficient-orchestration.md @@ -48,6 +48,8 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. + ## Guardrails - Don't delegate a blocker your next step needs. diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md index 8c2f846..325498c 100644 --- a/.claude/commands/review-pr.md +++ b/.claude/commands/review-pr.md @@ -83,7 +83,15 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: + + ```bash + gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + ``` + + Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. - App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. - **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. diff --git a/.codex/skills/efficient-orchestration/SKILL.md b/.codex/skills/efficient-orchestration/SKILL.md index 453d903..4c3c3b3 100644 --- a/.codex/skills/efficient-orchestration/SKILL.md +++ b/.codex/skills/efficient-orchestration/SKILL.md @@ -49,6 +49,8 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. + ## Guardrails - Don't delegate a blocker your next step needs. diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md index a7dbad5..69c7f80 100644 --- a/.codex/skills/review-pr/SKILL.md +++ b/.codex/skills/review-pr/SKILL.md @@ -83,7 +83,15 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: + + ```bash + gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + ``` + + Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. - App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. - **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. diff --git a/.copilot/skills/efficient-orchestration/SKILL.md b/.copilot/skills/efficient-orchestration/SKILL.md index 453d903..4c3c3b3 100644 --- a/.copilot/skills/efficient-orchestration/SKILL.md +++ b/.copilot/skills/efficient-orchestration/SKILL.md @@ -49,6 +49,8 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. + ## Guardrails - Don't delegate a blocker your next step needs. diff --git a/.copilot/skills/review-pr/SKILL.md b/.copilot/skills/review-pr/SKILL.md index a7dbad5..69c7f80 100644 --- a/.copilot/skills/review-pr/SKILL.md +++ b/.copilot/skills/review-pr/SKILL.md @@ -83,7 +83,15 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: + + ```bash + gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + ``` + + Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. - App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. - **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. diff --git a/.kimi-code/skills/efficient-orchestration/SKILL.md b/.kimi-code/skills/efficient-orchestration/SKILL.md index 453d903..4c3c3b3 100644 --- a/.kimi-code/skills/efficient-orchestration/SKILL.md +++ b/.kimi-code/skills/efficient-orchestration/SKILL.md @@ -49,6 +49,8 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. + ## Guardrails - Don't delegate a blocker your next step needs. diff --git a/.kimi-code/skills/review-pr/SKILL.md b/.kimi-code/skills/review-pr/SKILL.md index a7dbad5..69c7f80 100644 --- a/.kimi-code/skills/review-pr/SKILL.md +++ b/.kimi-code/skills/review-pr/SKILL.md @@ -83,7 +83,15 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login can exit 0 having requested nothing. Confirm Copilot specifically, not just that some reviewer is pending: `gh api repos/{owner}/{repo}/pulls/{PR_NUMBER} --jq '.requested_reviewers[].login' | grep -qiE '^(Copilot|copilot-pull-request-reviewer\[bot\])$'`. A miss means it did not take, and the poll below would burn its full timeout waiting. Match both spellings: `requested_reviewers` returns the login as `Copilot`, while the review it later submits carries `copilot-pull-request-reviewer[bot]`, so checking only the `[bot]` form reports failure on every successful request. +- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: + + ```bash + gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + ``` + + Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. - App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. - **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. From eaf00b2b47b38395e3e786e251b1be5fd124fe1a Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 10 Aug 2026 18:06:43 -0700 Subject: [PATCH 2/4] refactor(review-pr): tighten the prompt without dropping any rule Cuts roughly a sixth of the file with no loss of instruction. The step 2 GraphQL query collapses to one line, matching the style of the other queries in the file, and the prose sheds hedging and restatement: "ALWAYS re-fetch" where "re-fetch" carries it, "Do NOT re-request a bot review while threads are still open; process existing feedback first" where the first clause says it. The Copilot note added in the previous commit was the worst offender - it explained the REST/GraphQL trap three times over. One statement of it is enough, and the code block below carries the rest. Every rule, trap and command survives: the pagination caveats, the empty-set poll guard, the one-shell requirement, the classification criteria, and the never-guess-a-mention-string rule are all still here, just shorter. --- .antigravity/skills/review-pr/SKILL.md | 51 ++++++++++---------------- .claude/commands/review-pr.md | 51 ++++++++++---------------- .codex/skills/review-pr/SKILL.md | 51 ++++++++++---------------- .copilot/skills/review-pr/SKILL.md | 51 ++++++++++---------------- .kimi-code/skills/review-pr/SKILL.md | 51 ++++++++++---------------- 5 files changed, 100 insertions(+), 155 deletions(-) diff --git a/.antigravity/skills/review-pr/SKILL.md b/.antigravity/skills/review-pr/SKILL.md index 69c7f80..eaf0833 100644 --- a/.antigravity/skills/review-pr/SKILL.md +++ b/.antigravity/skills/review-pr/SKILL.md @@ -25,44 +25,33 @@ Run `gh pr checks`. On failure: `gh run view --log-failed`, fix, commit ### 2. Fetch unresolved threads -ALWAYS re-fetch fresh each iteration. Use `gh api graphql --paginate --slurp` with `$endCursor`, then pipe to `jq` (`--slurp` can't be combined with `--jq`): +Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to `jq`: ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { - repository(owner:$owner,name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100,after:$endCursor) { - pageInfo { hasNextPage endCursor } - nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } - } - } - } - }' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` -**Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. +**Auto-resolve:** first comment body matching an `$IGNORED_FILE` entry (`grep -qxF`) → resolve via `resolveReviewThread`, no classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. +Threads remain → step 3. Never re-request a bot while threads are open. Zero unresolved → step 5. ### 3. Classify and resolve -Read referenced file + context for each remaining thread, then classify: +Read the referenced file and its context, then classify: -- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes -Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Loop back to step 2. +Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Back to step 2. ### 5. Ensure bot review covers latest commit -Each bot's latest review, and the commit it covers: - ```bash head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') @@ -73,9 +62,9 @@ latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/revi stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. +`/reviews` identifies the review bots; CI and deploy bots never appear there. Pipe `--slurp` to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a bot twice. -Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. +Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re-review a push on their own. | Bot | Login | Re-trigger with | | --- | --- | --- | @@ -83,7 +72,7 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -91,17 +80,17 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' ``` - Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. -- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. -- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. +- App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. +- **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: ```bash triggered="$stale" # minus any bot you could not trigger -# Never poll on an empty set: comm would report nothing pending and the loop -# would break on the first pass, declaring success without waiting. +# Never poll an empty set: comm reports nothing pending, so the loop breaks on +# the first pass and declares success without waiting. [ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 @@ -113,8 +102,8 @@ while [ $SECONDS -lt $end ]; do done ``` -Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. +Both blocks in one shell: `head_sha` and `latest` don't survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command. -Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. +Timeout → name the bots still pending, tell the user to re-run, stop. Success → back to step 2. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md index 325498c..a42710a 100644 --- a/.claude/commands/review-pr.md +++ b/.claude/commands/review-pr.md @@ -25,44 +25,33 @@ Run `gh pr checks`. On failure: `gh run view --log-failed`, fix, commit ### 2. Fetch unresolved threads -ALWAYS re-fetch fresh each iteration. Use `gh api graphql --paginate --slurp` with `$endCursor`, then pipe to `jq` (`--slurp` can't be combined with `--jq`): +Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to `jq`: ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { - repository(owner:$owner,name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100,after:$endCursor) { - pageInfo { hasNextPage endCursor } - nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } - } - } - } - }' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` -**Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. +**Auto-resolve:** first comment body matching an `$IGNORED_FILE` entry (`grep -qxF`) → resolve via `resolveReviewThread`, no classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. +Threads remain → step 3. Never re-request a bot while threads are open. Zero unresolved → step 5. ### 3. Classify and resolve -Read referenced file + context for each remaining thread, then classify: +Read the referenced file and its context, then classify: -- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes -Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Loop back to step 2. +Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Back to step 2. ### 5. Ensure bot review covers latest commit -Each bot's latest review, and the commit it covers: - ```bash head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') @@ -73,9 +62,9 @@ latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/revi stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. +`/reviews` identifies the review bots; CI and deploy bots never appear there. Pipe `--slurp` to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a bot twice. -Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. +Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re-review a push on their own. | Bot | Login | Re-trigger with | | --- | --- | --- | @@ -83,7 +72,7 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -91,17 +80,17 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' ``` - Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. -- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. -- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. +- App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. +- **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: ```bash triggered="$stale" # minus any bot you could not trigger -# Never poll on an empty set: comm would report nothing pending and the loop -# would break on the first pass, declaring success without waiting. +# Never poll an empty set: comm reports nothing pending, so the loop breaks on +# the first pass and declares success without waiting. [ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 @@ -113,8 +102,8 @@ while [ $SECONDS -lt $end ]; do done ``` -Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. +Both blocks in one shell: `head_sha` and `latest` don't survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command. -Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. +Timeout → name the bots still pending, tell the user to re-run, stop. Success → back to step 2. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md index 69c7f80..eaf0833 100644 --- a/.codex/skills/review-pr/SKILL.md +++ b/.codex/skills/review-pr/SKILL.md @@ -25,44 +25,33 @@ Run `gh pr checks`. On failure: `gh run view --log-failed`, fix, commit ### 2. Fetch unresolved threads -ALWAYS re-fetch fresh each iteration. Use `gh api graphql --paginate --slurp` with `$endCursor`, then pipe to `jq` (`--slurp` can't be combined with `--jq`): +Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to `jq`: ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { - repository(owner:$owner,name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100,after:$endCursor) { - pageInfo { hasNextPage endCursor } - nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } - } - } - } - }' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` -**Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. +**Auto-resolve:** first comment body matching an `$IGNORED_FILE` entry (`grep -qxF`) → resolve via `resolveReviewThread`, no classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. +Threads remain → step 3. Never re-request a bot while threads are open. Zero unresolved → step 5. ### 3. Classify and resolve -Read referenced file + context for each remaining thread, then classify: +Read the referenced file and its context, then classify: -- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes -Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Loop back to step 2. +Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Back to step 2. ### 5. Ensure bot review covers latest commit -Each bot's latest review, and the commit it covers: - ```bash head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') @@ -73,9 +62,9 @@ latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/revi stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. +`/reviews` identifies the review bots; CI and deploy bots never appear there. Pipe `--slurp` to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a bot twice. -Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. +Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re-review a push on their own. | Bot | Login | Re-trigger with | | --- | --- | --- | @@ -83,7 +72,7 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -91,17 +80,17 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' ``` - Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. -- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. -- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. +- App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. +- **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: ```bash triggered="$stale" # minus any bot you could not trigger -# Never poll on an empty set: comm would report nothing pending and the loop -# would break on the first pass, declaring success without waiting. +# Never poll an empty set: comm reports nothing pending, so the loop breaks on +# the first pass and declares success without waiting. [ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 @@ -113,8 +102,8 @@ while [ $SECONDS -lt $end ]; do done ``` -Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. +Both blocks in one shell: `head_sha` and `latest` don't survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command. -Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. +Timeout → name the bots still pending, tell the user to re-run, stop. Success → back to step 2. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.copilot/skills/review-pr/SKILL.md b/.copilot/skills/review-pr/SKILL.md index 69c7f80..eaf0833 100644 --- a/.copilot/skills/review-pr/SKILL.md +++ b/.copilot/skills/review-pr/SKILL.md @@ -25,44 +25,33 @@ Run `gh pr checks`. On failure: `gh run view --log-failed`, fix, commit ### 2. Fetch unresolved threads -ALWAYS re-fetch fresh each iteration. Use `gh api graphql --paginate --slurp` with `$endCursor`, then pipe to `jq` (`--slurp` can't be combined with `--jq`): +Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to `jq`: ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { - repository(owner:$owner,name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100,after:$endCursor) { - pageInfo { hasNextPage endCursor } - nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } - } - } - } - }' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` -**Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. +**Auto-resolve:** first comment body matching an `$IGNORED_FILE` entry (`grep -qxF`) → resolve via `resolveReviewThread`, no classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. +Threads remain → step 3. Never re-request a bot while threads are open. Zero unresolved → step 5. ### 3. Classify and resolve -Read referenced file + context for each remaining thread, then classify: +Read the referenced file and its context, then classify: -- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes -Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Loop back to step 2. +Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Back to step 2. ### 5. Ensure bot review covers latest commit -Each bot's latest review, and the commit it covers: - ```bash head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') @@ -73,9 +62,9 @@ latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/revi stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. +`/reviews` identifies the review bots; CI and deploy bots never appear there. Pipe `--slurp` to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a bot twice. -Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. +Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re-review a push on their own. | Bot | Login | Re-trigger with | | --- | --- | --- | @@ -83,7 +72,7 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -91,17 +80,17 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' ``` - Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. -- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. -- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. +- App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. +- **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: ```bash triggered="$stale" # minus any bot you could not trigger -# Never poll on an empty set: comm would report nothing pending and the loop -# would break on the first pass, declaring success without waiting. +# Never poll an empty set: comm reports nothing pending, so the loop breaks on +# the first pass and declares success without waiting. [ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 @@ -113,8 +102,8 @@ while [ $SECONDS -lt $end ]; do done ``` -Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. +Both blocks in one shell: `head_sha` and `latest` don't survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command. -Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. +Timeout → name the bots still pending, tell the user to re-run, stop. Success → back to step 2. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. diff --git a/.kimi-code/skills/review-pr/SKILL.md b/.kimi-code/skills/review-pr/SKILL.md index 69c7f80..eaf0833 100644 --- a/.kimi-code/skills/review-pr/SKILL.md +++ b/.kimi-code/skills/review-pr/SKILL.md @@ -25,44 +25,33 @@ Run `gh pr checks`. On failure: `gh run view --log-failed`, fix, commit ### 2. Fetch unresolved threads -ALWAYS re-fetch fresh each iteration. Use `gh api graphql --paginate --slurp` with `$endCursor`, then pipe to `jq` (`--slurp` can't be combined with `--jq`): +Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to `jq`: ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { - repository(owner:$owner,name:$repo) { - pullRequest(number:$pr) { - reviewThreads(first:100,after:$endCursor) { - pageInfo { hasNextPage endCursor } - nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } - } - } - } - }' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` -**Auto-resolve:** If a thread's first comment body matches any `$IGNORED_FILE` entry (`grep -qxF`), resolve via `resolveReviewThread` mutation without classifying. +**Auto-resolve:** first comment body matching an `$IGNORED_FILE` entry (`grep -qxF`) → resolve via `resolveReviewThread`, no classifying. -If unresolved threads remain → step 3. Do NOT re-request a bot review while threads are still open; process existing feedback first. Only when zero unresolved threads remain → step 5. +Threads remain → step 3. Never re-request a bot while threads are open. Zero unresolved → step 5. ### 3. Classify and resolve -Read referenced file + context for each remaining thread, then classify: +Read the referenced file and its context, then classify: -- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply with brief explanation if inaccurate). -- **Valid fix**: implement minimal change. Must meet ALL: (1) fixes a real bug (wrong behavior, data loss, security, crash, or race condition); (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing. Includes: style preferences not enforced by linter, docstring suggestions on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append body to `$IGNORED_FILE`, reply with one-line rationale, resolve. +- **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes -Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Loop back to step 2. +Stage, commit (`fix:`/`refactor:`/etc.), push, verify CI green, resolve fixed threads. Back to step 2. ### 5. Ensure bot review covers latest commit -Each bot's latest review, and the commit it covers: - ```bash head_sha=$(gh pr view {PR_NUMBER} --json commits --jq '.commits[-1].oid') @@ -73,9 +62,9 @@ latest() { gh api --paginate --slurp repos/{owner}/{repo}/pulls/{PR_NUMBER}/revi stale=$(latest | grep -v " $head_sha$" | cut -d' ' -f1 | sort -u) ``` -`/reviews` alone identifies the review bots; CI and deploy bots never appear there. `--slurp` piped to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a long-running PR's bots twice. +`/reviews` identifies the review bots; CI and deploy bots never appear there. Pipe `--slurp` to `jq`, not `--jq`: under `--paginate` a `--jq` filter runs per page, so `max_by` returns a per-page max and lists a bot twice. -Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise re-trigger each login in `stale`; they do not re-review a push on their own. +Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re-review a push on their own. | Bot | Login | Re-trigger with | | --- | --- | --- | @@ -83,7 +72,7 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Pass the literal `@copilot`; its raw `[bot]` login cannot be resolved (`--add-reviewer Copilot` fails with `Could not resolve user with login 'copilot'`). Confirm Copilot specifically, not just that some reviewer is pending — but **confirm through GraphQL `reviewRequests`, never REST `requested_reviewers`.** Copilot is a Bot, and the REST field lists Users only, so a Bot reviewer never appears there no matter how the request was made. A REST-based check therefore reports failure on every *successful* request, which makes a working trigger look broken and sends you hunting for a replacement that was never needed: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -91,17 +80,17 @@ Empty `stale` → every bot already covers `head_sha`, success, stop. Otherwise --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' ``` - Note the pattern: `reviewRequests` drops the `[bot]` suffix the table above lists, so grepping the table's login here silently never matches. A genuine miss means the request did not take, and the poll below would burn its full timeout waiting. -- App-based bots (CodeRabbit, Greptile) cannot be requested as reviewers at all; a mention is their only trigger. `@coderabbitai full review` re-reviews the whole diff rather than just new commits. -- **Bot not in the table, or none found** → ask the user for the exact trigger. Never guess a mention string: a wrong one posts a visible no-op comment. + `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. +- App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. +- **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line, dropping any you could not trigger: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: ```bash triggered="$stale" # minus any bot you could not trigger -# Never poll on an empty set: comm would report nothing pending and the loop -# would break on the first pass, declaring success without waiting. +# Never poll an empty set: comm reports nothing pending, so the loop breaks on +# the first pass and declares success without waiting. [ -n "$triggered" ] || { echo "nothing was triggered"; exit 1; } end=$((SECONDS+900)); sleep 480 @@ -113,8 +102,8 @@ while [ $SECONDS -lt $end ]; do done ``` -Run both blocks in one shell: `head_sha` and `latest` do not survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command rather than sleeping between tool calls. +Both blocks in one shell: `head_sha` and `latest` don't survive separate tool calls. If your harness blocks foreground `sleep`, run the whole wait as one backgrounded command. -Timeout → name the bots still pending, tell user to re-run this command, stop. Success → go back to step 2. +Timeout → name the bots still pending, tell the user to re-run, stop. Success → back to step 2. Stop at iteration 5. Report: threads resolved, fixes made, threads auto-ignored, threads remaining, CI status. From 968b2fba77d5189a8d0c2430b37c3f4a8916e26e Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 10 Aug 2026 18:15:24 -0700 Subject: [PATCH 3/4] style: drop em dashes and restore the one-per-line poll note Replaces the em dashes this branch introduced with colons and semicolons; the rest of the repo uses none. Also restores the instruction that `triggered` holds one login per line: printf feeds comm line by line, so a space-separated list would never match a covered bot and the poll would silently run out its full timeout before reporting a false failure. --- .antigravity/skills/efficient-orchestration/SKILL.md | 2 +- .antigravity/skills/review-pr/SKILL.md | 8 ++++---- .claude/commands/efficient-orchestration.md | 2 +- .claude/commands/review-pr.md | 8 ++++---- .codex/skills/efficient-orchestration/SKILL.md | 2 +- .codex/skills/review-pr/SKILL.md | 8 ++++---- .copilot/skills/efficient-orchestration/SKILL.md | 2 +- .copilot/skills/review-pr/SKILL.md | 8 ++++---- .kimi-code/skills/efficient-orchestration/SKILL.md | 2 +- .kimi-code/skills/review-pr/SKILL.md | 8 ++++---- 10 files changed, 25 insertions(+), 25 deletions(-) diff --git a/.antigravity/skills/efficient-orchestration/SKILL.md b/.antigravity/skills/efficient-orchestration/SKILL.md index 4c3c3b3..70f1393 100644 --- a/.antigravity/skills/efficient-orchestration/SKILL.md +++ b/.antigravity/skills/efficient-orchestration/SKILL.md @@ -49,7 +49,7 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. -**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result: ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found"; that reads as a clean result when nothing was actually checked. ## Guardrails diff --git a/.antigravity/skills/review-pr/SKILL.md b/.antigravity/skills/review-pr/SKILL.md index eaf0833..40a8eaa 100644 --- a/.antigravity/skills/review-pr/SKILL.md +++ b/.antigravity/skills/review-pr/SKILL.md @@ -43,8 +43,8 @@ Threads remain → step 3. Never re-request a bot while threads are open. Zero u Read the referenced file and its context, then classify: - **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). -- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug: wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing: style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes @@ -72,7 +72,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -84,7 +84,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re - App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. - **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line: ```bash triggered="$stale" # minus any bot you could not trigger diff --git a/.claude/commands/efficient-orchestration.md b/.claude/commands/efficient-orchestration.md index f6285bf..ea18d3d 100644 --- a/.claude/commands/efficient-orchestration.md +++ b/.claude/commands/efficient-orchestration.md @@ -48,7 +48,7 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. -**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result: ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found"; that reads as a clean result when nothing was actually checked. ## Guardrails diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md index a42710a..7968d2a 100644 --- a/.claude/commands/review-pr.md +++ b/.claude/commands/review-pr.md @@ -43,8 +43,8 @@ Threads remain → step 3. Never re-request a bot while threads are open. Zero u Read the referenced file and its context, then classify: - **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). -- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug: wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing: style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes @@ -72,7 +72,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -84,7 +84,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re - App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. - **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line: ```bash triggered="$stale" # minus any bot you could not trigger diff --git a/.codex/skills/efficient-orchestration/SKILL.md b/.codex/skills/efficient-orchestration/SKILL.md index 4c3c3b3..70f1393 100644 --- a/.codex/skills/efficient-orchestration/SKILL.md +++ b/.codex/skills/efficient-orchestration/SKILL.md @@ -49,7 +49,7 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. -**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result: ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found"; that reads as a clean result when nothing was actually checked. ## Guardrails diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md index eaf0833..40a8eaa 100644 --- a/.codex/skills/review-pr/SKILL.md +++ b/.codex/skills/review-pr/SKILL.md @@ -43,8 +43,8 @@ Threads remain → step 3. Never re-request a bot while threads are open. Zero u Read the referenced file and its context, then classify: - **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). -- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug: wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing: style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes @@ -72,7 +72,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -84,7 +84,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re - App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. - **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line: ```bash triggered="$stale" # minus any bot you could not trigger diff --git a/.copilot/skills/efficient-orchestration/SKILL.md b/.copilot/skills/efficient-orchestration/SKILL.md index 4c3c3b3..70f1393 100644 --- a/.copilot/skills/efficient-orchestration/SKILL.md +++ b/.copilot/skills/efficient-orchestration/SKILL.md @@ -49,7 +49,7 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. -**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result: ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found"; that reads as a clean result when nothing was actually checked. ## Guardrails diff --git a/.copilot/skills/review-pr/SKILL.md b/.copilot/skills/review-pr/SKILL.md index eaf0833..40a8eaa 100644 --- a/.copilot/skills/review-pr/SKILL.md +++ b/.copilot/skills/review-pr/SKILL.md @@ -43,8 +43,8 @@ Threads remain → step 3. Never re-request a bot while threads are open. Zero u Read the referenced file and its context, then classify: - **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). -- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug: wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing: style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes @@ -72,7 +72,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -84,7 +84,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re - App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. - **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line: ```bash triggered="$stale" # minus any bot you could not trigger diff --git a/.kimi-code/skills/efficient-orchestration/SKILL.md b/.kimi-code/skills/efficient-orchestration/SKILL.md index 4c3c3b3..70f1393 100644 --- a/.kimi-code/skills/efficient-orchestration/SKILL.md +++ b/.kimi-code/skills/efficient-orchestration/SKILL.md @@ -49,7 +49,7 @@ Pin an explicit model on every spawn, preferring your harness's stable aliases ( Reports are leads, not facts. Before acting on a high-impact finding, opening a PR, or claiming done: reopen key cited files, confirm line refs and failures, review the final diff, and resolve subagent disagreements yourself. For non-trivial completed work, spawn a fresh-context verifier on your tier that only tries to refute the claim (rerun the tests, drive the affected flow, probe edge cases) and never fixes anything; independent refutation beats self-review. -**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result — ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found" — that reads as a clean result when nothing was actually checked. +**A subagent can finish without reporting.** Going idle is not the same as returning findings: an agent may signal it is done and hand back nothing. Treat a silent completion as one failure, not as an empty result: ask for the report once, naming the format you expected and telling it to state which objectives it did *not* finish rather than padding. If the second attempt is also silent, take the work back rather than spawning a replacement; a task cheap enough to delegate is usually cheap enough to run inline, and you have already spent the delegation's wall-clock twice. Never treat silence as "nothing found"; that reads as a clean result when nothing was actually checked. ## Guardrails diff --git a/.kimi-code/skills/review-pr/SKILL.md b/.kimi-code/skills/review-pr/SKILL.md index eaf0833..40a8eaa 100644 --- a/.kimi-code/skills/review-pr/SKILL.md +++ b/.kimi-code/skills/review-pr/SKILL.md @@ -43,8 +43,8 @@ Threads remain → step 3. Never re-request a bot while threads are open. Zero u Read the referenced file and its context, then classify: - **Already addressed / Informational / Inaccurate**: append body to `$IGNORED_FILE`, resolve (reply briefly if inaccurate). -- **Valid fix**: implement minimally. Must meet ALL: (1) real bug — wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. -- **Nitpick / Low-value**: resolve WITHOUT implementing — style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. +- **Valid fix**: implement minimally. Must meet ALL: (1) real bug: wrong behavior, data loss, security, crash, race; (2) net-simpler or complexity-neutral; (3) concrete, not speculative. +- **Nitpick / Low-value**: resolve WITHOUT implementing: style not enforced by a linter, docstrings on clear code, subjective renames, unnecessary defensive checks, premature abstraction, "consider X instead of Y" where both work, type annotations beyond codebase norms. Append to `$IGNORED_FILE`, reply with a one-line rationale, resolve. ### 4. Push fixes @@ -72,7 +72,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests` — **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: ```bash gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ @@ -84,7 +84,7 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re - App bots (CodeRabbit, Greptile) can't be requested as reviewers; a mention is the only trigger. `@coderabbitai full review` covers the whole diff, not just new commits. - **Bot not in the table** → ask the user for the trigger. Never guess a mention string: a wrong one posts a visible no-op comment. -Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired: +Poll until every triggered bot covers `head_sha`. Set `triggered` to the logins you actually fired, one per line: ```bash triggered="$stale" # minus any bot you could not trigger From dced7a21c473e1dd157818410bcf5d3ed23fb4d7 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Mon, 10 Aug 2026 18:46:33 -0700 Subject: [PATCH 4/4] refactor(review-pr): reformat GraphQL queries multi-line, document the gh export gap gh pr view --json reviewRequests looks like a cleaner Copilot confirm but has the REST hole one layer up: gh's export switch (api/export_pr.go) serializes only User and Team requested reviewers, so a requested Bot prints as an empty array and a successful request reads as failed. Verified live on PR #22 with gh 2.97.0: the human-readable view shows 'Copilot (AI) (Requested)' while the JSON output shows []. Record that trap next to the REST one and keep the raw query, expanded to multi-line so it reads as structure instead of a 300-character string. The unused __typename field is dropped. --- .antigravity/skills/review-pr/SKILL.md | 27 ++++++++++++++++++++++---- .claude/commands/review-pr.md | 27 ++++++++++++++++++++++---- .codex/skills/review-pr/SKILL.md | 27 ++++++++++++++++++++++---- .copilot/skills/review-pr/SKILL.md | 27 ++++++++++++++++++++++---- .kimi-code/skills/review-pr/SKILL.md | 27 ++++++++++++++++++++++---- 5 files changed, 115 insertions(+), 20 deletions(-) diff --git a/.antigravity/skills/review-pr/SKILL.md b/.antigravity/skills/review-pr/SKILL.md index 40a8eaa..07a7916 100644 --- a/.antigravity/skills/review-pr/SKILL.md +++ b/.antigravity/skills/review-pr/SKILL.md @@ -29,7 +29,16 @@ Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to ` ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100, after:$endCursor) { + pageInfo { hasNextPage endCursor } + nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } + } + } + } + }' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` @@ -72,12 +81,22 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via raw GraphQL `reviewRequests` only. **Never REST `requested_reviewers` (lists Users only) and never `gh pr view --json reviewRequests` (serializes only Users and Teams): in both, a requested Bot prints as empty and a successful request reads as failed.** ```bash - gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + gh api graphql \ + -f query='query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewRequests(first:20) { + nodes { requestedReviewer { ... on Bot { login } ... on User { login } } } + } + } + } + }' \ -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ - --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' \ + | grep -q '^copilot-pull-request-reviewer$' ``` `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md index 7968d2a..028baee 100644 --- a/.claude/commands/review-pr.md +++ b/.claude/commands/review-pr.md @@ -29,7 +29,16 @@ Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to ` ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100, after:$endCursor) { + pageInfo { hasNextPage endCursor } + nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } + } + } + } + }' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` @@ -72,12 +81,22 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via raw GraphQL `reviewRequests` only. **Never REST `requested_reviewers` (lists Users only) and never `gh pr view --json reviewRequests` (serializes only Users and Teams): in both, a requested Bot prints as empty and a successful request reads as failed.** ```bash - gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + gh api graphql \ + -f query='query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewRequests(first:20) { + nodes { requestedReviewer { ... on Bot { login } ... on User { login } } } + } + } + } + }' \ -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ - --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' \ + | grep -q '^copilot-pull-request-reviewer$' ``` `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md index 40a8eaa..07a7916 100644 --- a/.codex/skills/review-pr/SKILL.md +++ b/.codex/skills/review-pr/SKILL.md @@ -29,7 +29,16 @@ Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to ` ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100, after:$endCursor) { + pageInfo { hasNextPage endCursor } + nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } + } + } + } + }' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` @@ -72,12 +81,22 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via raw GraphQL `reviewRequests` only. **Never REST `requested_reviewers` (lists Users only) and never `gh pr view --json reviewRequests` (serializes only Users and Teams): in both, a requested Bot prints as empty and a successful request reads as failed.** ```bash - gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + gh api graphql \ + -f query='query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewRequests(first:20) { + nodes { requestedReviewer { ... on Bot { login } ... on User { login } } } + } + } + } + }' \ -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ - --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' \ + | grep -q '^copilot-pull-request-reviewer$' ``` `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. diff --git a/.copilot/skills/review-pr/SKILL.md b/.copilot/skills/review-pr/SKILL.md index 40a8eaa..07a7916 100644 --- a/.copilot/skills/review-pr/SKILL.md +++ b/.copilot/skills/review-pr/SKILL.md @@ -29,7 +29,16 @@ Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to ` ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100, after:$endCursor) { + pageInfo { hasNextPage endCursor } + nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } + } + } + } + }' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` @@ -72,12 +81,22 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via raw GraphQL `reviewRequests` only. **Never REST `requested_reviewers` (lists Users only) and never `gh pr view --json reviewRequests` (serializes only Users and Teams): in both, a requested Bot prints as empty and a successful request reads as failed.** ```bash - gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + gh api graphql \ + -f query='query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewRequests(first:20) { + nodes { requestedReviewer { ... on Bot { login } ... on User { login } } } + } + } + } + }' \ -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ - --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' \ + | grep -q '^copilot-pull-request-reviewer$' ``` `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting. diff --git a/.kimi-code/skills/review-pr/SKILL.md b/.kimi-code/skills/review-pr/SKILL.md index 40a8eaa..07a7916 100644 --- a/.kimi-code/skills/review-pr/SKILL.md +++ b/.kimi-code/skills/review-pr/SKILL.md @@ -29,7 +29,16 @@ Re-fetch fresh each iteration. `--slurp` can't combine with `--jq`, so pipe to ` ```bash gh api graphql --paginate --slurp \ - -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewThreads(first:100,after:$endCursor){pageInfo{hasNextPage endCursor} nodes{id isResolved comments(first:100){nodes{databaseId body path line author{login}}}}}}}}' \ + -f query='query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewThreads(first:100, after:$endCursor) { + pageInfo { hasNextPage endCursor } + nodes { id isResolved comments(first:100){nodes{databaseId body path line author{login}}} } + } + } + } + }' \ -f owner="{owner}" -f repo="{repo}" -F pr={PR_NUMBER} \ | jq '[.[].data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved==false)]' ``` @@ -72,12 +81,22 @@ Empty `stale` → success, stop. Otherwise re-trigger each login; bots do not re | CodeRabbit | `coderabbitai[bot]` | `gh pr comment {PR_NUMBER} --body "@coderabbitai review"` | | Greptile | `greptile-apps[bot]`, `greptileai[bot]` | `gh pr comment {PR_NUMBER} --body "@greptileai review"` | -- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via GraphQL `reviewRequests`, **never REST `requested_reviewers`, which lists Users only, so a Bot never appears there and a successful request reads as failed**: +- Copilot takes the literal `@copilot` (`--add-reviewer Copilot` fails to resolve). Confirm via raw GraphQL `reviewRequests` only. **Never REST `requested_reviewers` (lists Users only) and never `gh pr view --json reviewRequests` (serializes only Users and Teams): in both, a requested Bot prints as empty and a successful request reads as failed.** ```bash - gh api graphql -f query='query($owner:String!,$repo:String!,$pr:Int!){repository(owner:$owner,name:$repo){pullRequest(number:$pr){reviewRequests(first:20){nodes{requestedReviewer{__typename ... on Bot{login} ... on User{login}}}}}}}' \ + gh api graphql \ + -f query='query($owner:String!,$repo:String!,$pr:Int!) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$pr) { + reviewRequests(first:20) { + nodes { requestedReviewer { ... on Bot { login } ... on User { login } } } + } + } + } + }' \ -f owner={owner} -f repo={repo} -F pr={PR_NUMBER} \ - --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' | grep -q '^copilot-pull-request-reviewer$' + --jq '.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login' \ + | grep -q '^copilot-pull-request-reviewer$' ``` `reviewRequests` drops the `[bot]` suffix the table lists. A real miss means the request failed, and the poll below would time out waiting.