Skip to content

Commit d2c46b1

Browse files
authored
fix: route raw / raw-secret / raw-public per-algorithm in subtle (#1023)
1 parent fd5e5d0 commit d2c46b1

6 files changed

Lines changed: 767 additions & 113 deletions

File tree

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
# /address-pr-feedback - Address PR Feedback
2+
3+
Fetch and address all review bot feedback on the current PR.
4+
5+
## Instructions
6+
7+
### 1. Identify the PR
8+
9+
```bash
10+
gh pr view --json number,url,state --jq '{number, url, state}'
11+
```
12+
13+
If no PR exists for the current branch, abort with a message.
14+
15+
### 2. Check that review bots are done
16+
17+
CodeRabbit reviews asynchronously. Before addressing feedback, verify it has finished.
18+
19+
```bash
20+
# Get all reviews on the PR
21+
gh pr view --json reviews --jq '.reviews[] | {author: .author.login, state: .state}'
22+
23+
# Check PR comments for bot activity (MUST use --paginate for large PRs)
24+
gh api "repos/{owner}/{repo}/pulls/{pr}/comments" --paginate \
25+
--jq '[.[] | .user.login] | unique'
26+
```
27+
28+
**CodeRabbit**: Look for a PR comment containing "Walkthrough" or a review with `coderabbitai[bot]` as author. If not present, inform the user:
29+
30+
> "CodeRabbit hasn't reviewed this PR yet. Wait for its review or run `@coderabbitai review` as a PR comment, then re-run this command."
31+
32+
**If the bot hasn't finished, stop here.** Do not proceed to fixing issues with incomplete feedback.
33+
34+
### 3. Fetch ALL review comments
35+
36+
**CRITICAL**: Always use `--paginate` with `gh api` for review comments. The default page size is 30, which is easily exceeded when CodeRabbit posts 16+ inline comments plus replies. Without `--paginate`, you will miss comments from later review passes.
37+
38+
#### 3a. Inline review comments
39+
40+
```bash
41+
# Get ALL review comments — MUST use --paginate
42+
gh api "repos/{owner}/{repo}/pulls/{pr}/comments" --paginate --jq '.[] | {
43+
id: .id,
44+
author: .user.login,
45+
path: .path,
46+
line: .line,
47+
body: .body,
48+
in_reply_to_id: .in_reply_to_id,
49+
created_at: .created_at
50+
}'
51+
```
52+
53+
To identify **new unaddressed root comments**, filter by:
54+
55+
- `in_reply_to_id == null` (root comment, not a reply)
56+
- `user.login == "coderabbitai[bot]"`
57+
- No reply from the PR author (`gh pr view --json author --jq '.author.login'`) exists with matching `in_reply_to_id`
58+
59+
Useful shortcut to see how many batches exist:
60+
61+
```bash
62+
gh api "repos/{owner}/{repo}/pulls/{pr}/comments" --paginate \
63+
--jq '.[] | select(.user.login == "coderabbitai[bot]") | select(.in_reply_to_id == null) | .created_at' \
64+
| sort | uniq -c | sort -rn
65+
```
66+
67+
Each unique timestamp cluster represents one review pass.
68+
69+
#### 3b. Outside-diff-range comments (in review body)
70+
71+
CodeRabbit posts "outside diff range" comments in the **review body**, not as inline comments. These are easy to miss.
72+
73+
```bash
74+
# Get ALL CodeRabbit reviews with non-empty bodies (includes CHANGES_REQUESTED and COMMENTED states)
75+
gh api "repos/{owner}/{repo}/pulls/{pr}/reviews" --paginate \
76+
--jq '.[] | select(.user.login == "coderabbitai[bot]") | select(.body | length > 0) | {id: .id, state: .state, submitted_at: .submitted_at}'
77+
```
78+
79+
Then fetch each review body:
80+
81+
```bash
82+
gh api "repos/{owner}/{repo}/pulls/{pr}/reviews/{review_id}" --jq '.body'
83+
```
84+
85+
Look for the `<summary>⚠️ Outside diff range comments (N)</summary>` section in the body. Parse these — they contain file paths, line numbers, and the same comment format as inline comments.
86+
87+
Also look for these sections in review bodies:
88+
89+
- **"🧹 Nitpick comments (N)"** — valid code quality items
90+
- **"♻️ Duplicate comments (N)"** — re-raised from prior reviews
91+
- **"🤖 Prompt for AI Agents"** — structured fix instructions
92+
93+
#### 3c. General PR comments
94+
95+
```bash
96+
gh api "repos/{owner}/{repo}/issues/{pr}/comments" --paginate \
97+
--jq '.[] | {id: .id, author: .user.login, body: .body, created_at: .created_at}'
98+
```
99+
100+
### 4. Identify actionable feedback
101+
102+
Collect ALL comments from:
103+
104+
- Inline review comments (3a)
105+
- Outside-diff-range / nitpick / duplicate comments from review bodies (3b)
106+
- General PR comments (3c)
107+
108+
Filter to unaddressed items from `coderabbitai[bot]`.
109+
110+
**Before applying any fix**, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped.
111+
112+
For each comment, determine:
113+
114+
1. **Valid concern** — fix it
115+
2. **False positive** — reply explaining why
116+
3. **Stale** — code was already changed/removed since the comment was posted
117+
4. **Ambiguous** — ask the user which direction to take
118+
119+
### 5. Present decisions for approval
120+
121+
**STOP and present a table** before making any changes. The user must approve the plan first.
122+
123+
| # | Source | File:Line | Comment Summary | Decision | Rationale |
124+
| --- | --------------- | -------------------- | ---------------------------- | --------------------- | ----------------- |
125+
| 1 | CR inline | `path/to/file.ts:42` | Brief summary of the comment | Fix / Dismiss / Stale | Why this decision |
126+
| 2 | CR nitpick | `path/to/file.ts:10` | Brief summary | Fix / Dismiss / Stale | Why |
127+
| 3 | CR outside-diff | `path/to/file.ts:78` | Brief summary | Fix / Dismiss / Stale | Why |
128+
129+
Wait for the user to:
130+
131+
- **Approve all** — proceed with all decisions as proposed
132+
- **Override specific rows** — change the decision for individual items (e.g., "dismiss #3 instead of fixing")
133+
- **Ask questions** — clarify any items before approving
134+
135+
**Do not proceed to step 6 until the user approves.**
136+
137+
### 6. Apply fixes locally (do NOT reply to bots yet)
138+
139+
**CRITICAL — DO NOT reply to bot threads in this step.** Bot replies must happen _after_ push so the bot can verify against the actual remote. Replying with "Fixed" before the commit is on the remote causes the bot to re-flag the comment as unfixed (it reads the remote, not your working tree).
140+
141+
For each item that needs a code change:
142+
143+
1. Read the file and understand the context around the flagged line.
144+
2. Apply the fix.
145+
146+
Do not post any thread replies, PR comments, or "Fixed" messages yet. Just edit code.
147+
148+
### 7. Run quality gates
149+
150+
After all fixes are applied:
151+
152+
```bash
153+
bun tsc
154+
```
155+
156+
All must pass before committing.
157+
158+
### 8. Commit and push
159+
160+
If any code changes were made:
161+
162+
```bash
163+
git add <specific files>
164+
git commit -m "fix: address PR review feedback from CodeRabbit"
165+
git push
166+
```
167+
168+
**Verify the push landed before moving on.** Confirm `git log origin/<branch> -1` matches your local HEAD, or check `gh api repos/{owner}/{repo}/pulls/{pr} --jq .head.sha`. The bots will read this SHA when re-evaluating, so the reply in step 9 must reference code that is actually on it.
169+
170+
### 9. Reply to bot threads
171+
172+
Now that the fix is on the remote, post replies. Reference the new commit SHA in "Fixed" replies so the bot can verify and so the timeline is auditable later.
173+
174+
**Reply rules:**
175+
176+
1. **Inline review comments (3a)** — reply on the conversation thread:
177+
178+
```bash
179+
gh api "repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies" \
180+
-X POST -f body="@coderabbitai Fixed in <sha> — <brief explanation>"
181+
```
182+
183+
2. **Review-body / outside-diff items (3b)** — no inline thread exists. Post a top-level PR comment:
184+
185+
```bash
186+
gh pr comment {pr} --body "@coderabbitai Addressed in <sha>:
187+
- Fixed: <list of fixes with file:line references>
188+
- Dismissed: <list with reasoning>"
189+
```
190+
191+
3. **General PR comments (3c)** — issue comments don't support threaded replies. Post a new PR comment referencing the original:
192+
```bash
193+
gh pr comment {pr} --body "@coderabbitai Re: comment {comment_id} — Fixed in <sha> — <explanation>"
194+
```
195+
196+
**CodeRabbit replies MUST start with `@coderabbitai`** or the bot will not see them.
197+
198+
For false positives / stale, use the same reply mechanism with an explanation instead of "Fixed":
199+
200+
```bash
201+
gh api "repos/{owner}/{repo}/pulls/{pr}/comments/{comment_id}/replies" \
202+
-X POST -f body="@coderabbitai <explanation of why this is safe / stale>"
203+
```
204+
205+
### 10. Report summary
206+
207+
Present a summary to the user:
208+
209+
- **Fixed**: List of issues that were fixed with brief descriptions
210+
- **Dismissed**: List of false positives with reasoning
211+
- **Stale**: Comments on code that was already changed/removed
212+
- **Needs input**: Any ambiguous items requiring user decision
213+
- **Quality gates**: Pass/fail status

.claude/commands/review.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ Review all commits on the current branch since diverging from main.
55
## Prerequisites
66

77
**IMPORTANT**: Before starting the review, check if this is a fresh context/session:
8+
89
- If there is prior conversation history in this session (e.g., you helped write the code being reviewed), STOP immediately
910
- Inform the user: "Code reviews should be done in a fresh context to avoid bias. Please start a new Claude Code session and run /review there."
1011
- A reviewer should not be the same "person" who wrote the code
@@ -32,3 +33,17 @@ When activated (in a fresh session), perform a full code review of the commits s
3233
- Recommended actions (if any)
3334

3435
Run `bun tsc` to verify the code compiles.
36+
37+
## Follow-up
38+
39+
After presenting the review, present a **fix plan table** for the user to approve before making any changes:
40+
41+
| # | Severity | File:Line | Issue | Proposed Action |
42+
| --- | -------- | ------------------ | ----------------- | ---------------- |
43+
| 1 | Medium | path/to/file.ts:42 | Brief description | Fix / Skip / Ask |
44+
45+
- **Fix**: Will apply the change
46+
- **Skip**: Not worth changing (explain why)
47+
- **Ask**: Ambiguous, needs user input on approach
48+
49+
**Wait for the user to approve the plan** (they may want to skip or modify items). Then apply only the approved fixes. Run `bun tsc` after all fixes are applied to verify everything is clean.

example/src/tests/subtle/encrypt_decrypt.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -603,7 +603,7 @@ test(SUITE, 'ChaCha20-Poly1305 RF C 8439 vector', async () => {
603603
const expectedTagHex = '1ae10b594f09e26a7e902ecbd0600691';
604604

605605
const key = await subtle.importKey(
606-
'raw',
606+
'raw-secret',
607607
Buffer.from(keyHex, 'hex'),
608608
{ name: 'ChaCha20-Poly1305' } as any, // eslint-disable-line @typescript-eslint/no-explicit-any
609609
true,
@@ -763,18 +763,19 @@ test(SUITE, 'ChaCha20-Poly1305 large plaintext', async () => {
763763
);
764764
});
765765

766-
test(SUITE, 'ChaCha20-Poly1305 key import/export raw', async () => {
766+
test(SUITE, 'ChaCha20-Poly1305 key import/export raw-secret', async () => {
767767
const keyMaterial = getRandomValues(new Uint8Array(32)); // 256 bits
768768

769+
// ChaCha20-Poly1305 accepts only 'raw-secret' (Node chacha20_poly1305.js:104-134)
769770
const key = await subtle.importKey(
770-
'raw',
771+
'raw-secret',
771772
keyMaterial,
772773
{ name: 'ChaCha20-Poly1305' } as any, // eslint-disable-line @typescript-eslint/no-explicit-any
773774
true,
774775
['encrypt', 'decrypt'],
775776
);
776777

777-
const exported = await subtle.exportKey('raw', key as CryptoKey);
778+
const exported = await subtle.exportKey('raw-secret', key as CryptoKey);
778779

779780
expect(Buffer.from(exported as ArrayBuffer).toString('hex')).to.equal(
780781
Buffer.from(keyMaterial as Uint8Array).toString('hex'),
@@ -1367,19 +1368,20 @@ test(SUITE, 'AES-OCB empty plaintext', async () => {
13671368
expect(decrypted.byteLength).to.equal(0);
13681369
});
13691370

1370-
// Test AES-OCB key import/export
1371-
test(SUITE, 'AES-OCB key import/export raw', async () => {
1371+
// Test AES-OCB key import/export — Node accepts only 'raw-secret' for AES-OCB
1372+
// (aes.js:243-249, webcrypto.js:550-560).
1373+
test(SUITE, 'AES-OCB key import/export raw-secret', async () => {
13721374
const keyMaterial = getRandomValues(new Uint8Array(32));
13731375

13741376
const key = await subtle.importKey(
1375-
'raw',
1377+
'raw-secret',
13761378
keyMaterial,
13771379
{ name: 'AES-OCB' },
13781380
true,
13791381
['encrypt', 'decrypt'],
13801382
);
13811383

1382-
const exported = await subtle.exportKey('raw', key as CryptoKey);
1384+
const exported = await subtle.exportKey('raw-secret', key as CryptoKey);
13831385

13841386
expect(Buffer.from(exported as ArrayBuffer).toString('hex')).to.equal(
13851387
Buffer.from(keyMaterial as Uint8Array).toString('hex'),

0 commit comments

Comments
 (0)