Skip to content

Commit a7dee2d

Browse files
Merge branch 'stage' into feature/ADFA-5530-carousel
2 parents 1ea8713 + 53bd9c9 commit a7dee2d

8 files changed

Lines changed: 1016 additions & 12 deletions

File tree

.claude/skills/pr-review/SKILL.md

Lines changed: 429 additions & 0 deletions
Large diffs are not rendered by default.
Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
# GitHub API recipes for pr-review
2+
3+
Everything here was verified against the live API schema. Where something is unverified it
4+
says so.
5+
6+
## Contents
7+
8+
- [PR metadata](#pr-metadata)
9+
- [Posting a review with inline comments](#posting-a-review-with-inline-comments)
10+
- [File-level comments](#file-level-comments)
11+
- [Prior rounds: threads, replies, unresolve](#prior-rounds-threads-replies-unresolve)
12+
- [Submitting a verdict on its own](#submitting-a-verdict-on-its-own)
13+
- [Failure modes](#failure-modes)
14+
15+
## PR metadata
16+
17+
```bash
18+
gh pr view <n> --repo <owner/repo> \
19+
--json number,title,state,isDraft,author,headRefName,headRefOid,baseRefName,reviewDecision,mergeable
20+
```
21+
22+
`headRefOid` is the `commit_id` every comment must be posted against. `author.login`
23+
compared against `gh api user --jq .login` decides whether a verdict is even possible.
24+
25+
Existing reviews, to know whether this is round 1 or round 4:
26+
27+
```bash
28+
gh pr view <n> --repo <owner/repo> \
29+
--jq '[.reviews[] | {author: .author.login, state, submittedAt}]' --json reviews
30+
```
31+
32+
## Posting a review with inline comments
33+
34+
One call carries the comments and the event together. Build the payload in a script, never
35+
inline: bodies contain backticks, apostrophes, `$`, and `->`, each of which breaks a heredoc
36+
in its own way.
37+
38+
```python
39+
import json
40+
41+
payload = {
42+
"commit_id": "<head sha>",
43+
"body": "<review body: verdict rationale, prior-round re-check, unanchorable findings>",
44+
"event": "COMMENT", # or APPROVE / REQUEST_CHANGES
45+
"comments": [
46+
{
47+
"path": "src/main/kotlin/Foo.kt",
48+
"line": 84,
49+
"side": "RIGHT",
50+
"body": "IMPORTANT: ...",
51+
},
52+
{
53+
"path": "src/main/kotlin/Bar.kt",
54+
"line": 371,
55+
"side": "LEFT",
56+
"body": "MINOR: ...",
57+
},
58+
],
59+
}
60+
json.dump(payload, open("/tmp/review.json", "w"))
61+
```
62+
63+
```bash
64+
gh api --method POST repos/<owner>/<repo>/pulls/<n>/reviews \
65+
--input /tmp/review.json \
66+
--jq '{id, state, html_url}'
67+
```
68+
69+
**Ordering.** The `comments[]` order is the creation order, so IDs ascend with it and
70+
`GET /pulls/{n}/comments` returns them in that order. It does not affect the Files changed tab,
71+
which renders each comment at its anchor and is therefore always in diff order. Whether the
72+
Conversation tab follows array order or diff order within a single review is **unverified** -
73+
do not promise the author a severity-ordered reading surface; put the severity order in the
74+
review body, which renders above the comments in both tabs.
75+
76+
**Updating a review body after posting - unverified.**
77+
`PUT /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id}` with `{"body": "..."}` is
78+
documented as the way to rewrite a submitted review's body, which is what a body index with
79+
links to each comment needs. It has not been exercised from this skill; if it fails, keep the
80+
plain `path:line` index rather than retrying.
81+
82+
Comment fields:
83+
84+
- `line` is the line number **in the file at the state that side represents** - the
85+
right-side (post-change) number for `side: RIGHT`, the original number for `side: LEFT`.
86+
Compute both with `scripts/diff_anchors.py`; do not count hunk lines by eye.
87+
- `side` defaults to `RIGHT` when omitted. Set it explicitly anyway - a `LEFT` anchor posted
88+
without it silently lands on the wrong line.
89+
- Multi-line comments take `start_line` + `start_side` alongside `line` + `side`.
90+
- A suggestion is a fenced block inside `body`:
91+
92+
````
93+
```suggestion
94+
val tokens = contextTokens.coerceAtMost(ContextSizePolicy.MAX_CONTEXT_TOKENS)
95+
```
96+
````
97+
98+
The block replaces exactly the commented line range, so the range must be the whole thing
99+
being replaced and the replacement must be complete. Match the file's indentation
100+
character - a suggestion with spaces in a tabs file is committable and wrong.
101+
102+
Verify what landed:
103+
104+
```bash
105+
gh api "repos/<owner>/<repo>/pulls/<n>/comments?per_page=100" \
106+
--jq '[.[] | select(.pull_request_review_id==<id>) | {path, line: (.line // .original_line), side, first: (.body|split(":")[0])}]'
107+
```
108+
109+
That `first` field is worth checking every time: it is the severity token, and it proves the
110+
prefix rule held all the way through serialization.
111+
112+
## File-level comments
113+
114+
For a finding about a file as a whole (rung 3 of the anchor ladder).
115+
116+
**GraphQL - verified to exist.** `addPullRequestReviewThread` accepts
117+
`subjectType: FILE` (the enum's values are `LINE` and `FILE`) and an optional
118+
`pullRequestReviewId`, so the thread can attach to a review:
119+
120+
```bash
121+
gh api graphql -f query='
122+
mutation($prId: ID!, $path: String!, $body: String!) {
123+
addPullRequestReviewThread(input: {
124+
pullRequestId: $prId, path: $path, body: $body, subjectType: FILE
125+
}) { thread { id } }
126+
}' -f prId="<PR node id>" -f path="src/Foo.kt" -f body="NITPICK: ..."
127+
```
128+
129+
Get the PR node ID with:
130+
131+
```bash
132+
gh api graphql -f query='{repository(owner:"<o>",name:"<r>"){pullRequest(number:<n>){id}}}' \
133+
--jq .data.repository.pullRequest.id
134+
```
135+
136+
**REST - unverified.** The standalone create-review-comment endpoint documents
137+
`subject_type`, but whether the bundled `POST /pulls/{n}/reviews` `comments[]` array accepts
138+
it has not been confirmed. If a file-level comment is needed inside a bundled review, try it
139+
once and fall back to the GraphQL mutation or to ladder rung 4 rather than assuming.
140+
141+
## Prior rounds: threads, replies, unresolve
142+
143+
Thread node IDs come from GraphQL. A REST comment `id` will not work for either mutation.
144+
145+
```bash
146+
gh api graphql -f query='
147+
{ repository(owner:"<o>", name:"<r>") { pullRequest(number:<n>) {
148+
reviewThreads(first: 100) { nodes {
149+
id
150+
isResolved
151+
isOutdated
152+
path
153+
line
154+
comments(first: 1) { nodes { author { login } body } }
155+
} } } } }' \
156+
--jq '.data.repository.pullRequest.reviewThreads.nodes[] |
157+
{id, isResolved, isOutdated, path, line, first: .comments.nodes[0].body[0:160]}'
158+
```
159+
160+
Reply into a thread - verified input fields are `pullRequestReviewThreadId`, `body`,
161+
and optionally `pullRequestReviewId`:
162+
163+
```bash
164+
gh api graphql -f query='
165+
mutation($threadId: ID!, $body: String!) {
166+
addPullRequestReviewThreadReply(input: {
167+
pullRequestReviewThreadId: $threadId, body: $body
168+
}) { comment { url } }
169+
}' -f threadId="<thread node id>" -f body="IMPORTANT: still open at head - ..."
170+
```
171+
172+
Unresolve a thread whose issue is still live - verified input field is `threadId`:
173+
174+
```bash
175+
gh api graphql -f query='
176+
mutation($threadId: ID!) {
177+
unresolveReviewThread(input: {threadId: $threadId}) {
178+
thread { id isResolved }
179+
}
180+
}' -f threadId="<thread node id>"
181+
```
182+
183+
Reply first, then unresolve. A thread that pops back open with no explanation reads as noise;
184+
one that pops open under a reply reads as a finding.
185+
186+
## Submitting a verdict on its own
187+
188+
The second call of the confirmation gate: same endpoint, no `comments` array.
189+
190+
```bash
191+
gh pr review <n> --repo <owner/repo> --approve --body-file /tmp/verdict.md
192+
gh pr review <n> --repo <owner/repo> --request-changes --body-file /tmp/verdict.md
193+
```
194+
195+
`--body-file` avoids the quoting problem entirely. Confirm it landed:
196+
197+
```bash
198+
gh pr view <n> --repo <owner/repo> --json reviewDecision,reviews \
199+
--jq '{decision: .reviewDecision, approvals: [.reviews[] | select(.state=="APPROVED") | .author.login]}'
200+
```
201+
202+
`reviewDecision` can come back empty even after a successful approval - that means the repo
203+
requires a review from a specific team or `CODEOWNERS` entry that this approval does not
204+
satisfy. Report that rather than claiming the PR is green.
205+
206+
## Failure modes
207+
208+
| Symptom | Cause |
209+
|---|---|
210+
| 422 on the reviews POST | A comment's `line` is not in the diff for that `side`, or `commit_id` is not the head SHA. Re-run `diff_anchors.py --check` on every comment. |
211+
| 422 "Can not approve your own pull request" | PR author is the authenticated user. Degrade to `COMMENT`. |
212+
| Comment lands on the wrong line | `side` omitted on a `LEFT` anchor, or the line was counted by eye instead of computed. |
213+
| `bash: eval: unexpected EOF` / `bad substitution` | JSON or a body written inline in a heredoc. Use a file. |
214+
| Suggestion the author commits and it breaks the build | Suggestion covered a partial range, or used spaces in a tabs file. |
215+
| Thread mutation returns "Could not resolve to a node" | A REST comment ID was passed where a GraphQL thread node ID is required. |
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
"""Map a unified diff to the (path, side, line) anchors GitHub will accept.
2+
3+
GitHub rejects an inline PR comment whose line is not part of the diff, so every
4+
finding has to be checked against the hunks before it is posted. This also
5+
recomputes right-side line numbers, which a reviewer reading the diff by eye
6+
gets wrong often enough to matter.
7+
8+
Usage:
9+
gh pr diff <n> --repo <owner/repo> > /tmp/pr.diff
10+
11+
python diff_anchors.py /tmp/pr.diff --file Foo.kt
12+
Print every postable anchor in files whose path ends with "Foo.kt",
13+
as "SIDE line: text", so the text can be eyeballed against the claim.
14+
15+
python diff_anchors.py /tmp/pr.diff --check path/to/Foo.kt:84
16+
Exit 0 and print the line's text if that anchor is postable on RIGHT,
17+
exit 1 otherwise. Append :LEFT to check the deleted side.
18+
19+
python diff_anchors.py /tmp/pr.diff --json
20+
Emit {path: {"RIGHT": {line: text}, "LEFT": {line: text}}} for scripting.
21+
"""
22+
23+
import argparse
24+
import json
25+
import re
26+
import sys
27+
28+
HUNK = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@")
29+
30+
31+
def parse(diff_text):
32+
"""Return {path: {"RIGHT": {line: text}, "LEFT": {line: text}}} for one diff."""
33+
files = {}
34+
path = None
35+
left = right = 0
36+
for raw in diff_text.split("\n"):
37+
if raw.startswith("+++ "):
38+
target = raw[4:].strip()
39+
path = None if target == "/dev/null" else target[2:] if target.startswith("b/") else target
40+
if path:
41+
files.setdefault(path, {"RIGHT": {}, "LEFT": {}})
42+
continue
43+
if raw.startswith("--- ") or raw.startswith("diff ") or raw.startswith("index "):
44+
continue
45+
hunk = HUNK.match(raw)
46+
if hunk:
47+
left, right = int(hunk.group(1)), int(hunk.group(2))
48+
continue
49+
if path is None or not raw:
50+
continue
51+
if raw.startswith("\\"):
52+
continue
53+
text = raw[1:]
54+
if raw.startswith("+"):
55+
files[path]["RIGHT"][right] = text
56+
right += 1
57+
elif raw.startswith("-"):
58+
files[path]["LEFT"][left] = text
59+
left += 1
60+
elif raw.startswith(" "):
61+
files[path]["RIGHT"][right] = text
62+
files[path]["LEFT"][left] = text
63+
right += 1
64+
left += 1
65+
return files
66+
67+
68+
def main():
69+
ap = argparse.ArgumentParser()
70+
ap.add_argument("diff", help="path to a unified diff, or - for stdin")
71+
ap.add_argument("--file", help="only show paths ending with this")
72+
ap.add_argument("--check", help="PATH:LINE[:SIDE] - exit 0 if postable")
73+
ap.add_argument("--json", action="store_true", help="emit the full anchor map")
74+
args = ap.parse_args()
75+
76+
text = sys.stdin.read() if args.diff == "-" else open(args.diff, encoding="utf-8", errors="replace").read()
77+
files = parse(text)
78+
79+
if args.check:
80+
parts = args.check.split(":")
81+
if len(parts) < 2:
82+
sys.exit("--check wants PATH:LINE[:SIDE]")
83+
want_path, want_line = parts[0], int(parts[1])
84+
side = parts[2].upper() if len(parts) > 2 else "RIGHT"
85+
for path, sides in files.items():
86+
if path.endswith(want_path) and want_line in sides.get(side, {}):
87+
print("{} {} {}: {}".format(path, side, want_line, sides[side][want_line]))
88+
return
89+
print("not postable: {} {} {}".format(want_path, side, want_line), file=sys.stderr)
90+
sys.exit(1)
91+
92+
if args.json:
93+
json.dump(files, sys.stdout, indent=1, sort_keys=True)
94+
print()
95+
return
96+
97+
for path, sides in sorted(files.items()):
98+
if args.file and not path.endswith(args.file):
99+
continue
100+
print("== {}".format(path))
101+
for side in ("RIGHT", "LEFT"):
102+
for line in sorted(sides[side]):
103+
print("{:>5} {:>4}: {}".format(side, line, sides[side][line]))
104+
105+
106+
if __name__ == "__main__":
107+
main()

0 commit comments

Comments
 (0)