mirror of
https://github.com/cirruslabs/softnet.git
synced 2026-09-29 19:41:10 +02:00
fix: address PR review feedback
- simplify allow/block resolution without mem::take (refs #2661210452) - use dns.google test domain with stable A records (refs #2661236654) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.5
parent
50b5e2af2d
commit
1ccb0ee4b4
@@ -0,0 +1,163 @@
|
||||
---
|
||||
description: "Address PR review comments, make fixes, reply, and resolve"
|
||||
argument-hint: "<pr-number>"
|
||||
model: claude-opus-4-5-20251101
|
||||
---
|
||||
|
||||
# Address PR Review Comments
|
||||
|
||||
Automates addressing pull request review feedback: analyze comments, make fixes, reply to explain changes, resolve threads, and request re-review from @codex.
|
||||
|
||||
## Prerequisites
|
||||
|
||||
- Must have `gh` CLI authenticated
|
||||
- Must be in a git repository with a GitHub remote
|
||||
|
||||
## Step 1: Validate Input
|
||||
|
||||
**If $ARGUMENTS is empty:**
|
||||
- Use AskUserQuestion to ask for the PR number
|
||||
- Validate it's a number
|
||||
|
||||
**If $ARGUMENTS is provided:**
|
||||
- Extract PR number from $ARGUMENTS
|
||||
- Validate it's a valid number
|
||||
|
||||
## Step 2: Fetch PR Review Threads
|
||||
|
||||
Run this GraphQL query to get all unresolved review threads:
|
||||
|
||||
```bash
|
||||
gh api graphql -f query='
|
||||
query {
|
||||
repository(owner: "{owner}", name: "{repo}") {
|
||||
pullRequest(number: PR_NUMBER) {
|
||||
id
|
||||
reviewThreads(first: 100) {
|
||||
nodes {
|
||||
id
|
||||
isResolved
|
||||
path
|
||||
line
|
||||
comments(first: 10) {
|
||||
nodes {
|
||||
id
|
||||
databaseId
|
||||
body
|
||||
author {
|
||||
login
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}'
|
||||
```
|
||||
|
||||
Filter to only unresolved threads (`isResolved: false`).
|
||||
|
||||
If there are no unresolved threads, inform the user and exit.
|
||||
|
||||
## Step 3: Analyze and Categorize Comments
|
||||
|
||||
For each unresolved thread:
|
||||
|
||||
1. Read the comment body to understand the feedback
|
||||
2. Identify the file path and line number
|
||||
3. Read the relevant file section to understand the context
|
||||
4. Categorize the type of feedback:
|
||||
- **Code change needed** - requires file modification
|
||||
- **Documentation** - needs comment/doc update
|
||||
- **Question** - requires explanation only, no code change
|
||||
- **Disagree/Won't fix** - ASK USER before responding
|
||||
|
||||
Present a summary to the user showing:
|
||||
- Number of comments found
|
||||
- File paths affected
|
||||
- Brief description of each comment
|
||||
|
||||
**IMPORTANT**: For any comment where you disagree or think "won't fix" is appropriate, use AskUserQuestion to get user confirmation before replying. Never auto-resolve disagreements.
|
||||
|
||||
## Step 4: Address Each Comment
|
||||
|
||||
For each comment requiring action:
|
||||
|
||||
### 4a. Read the relevant file
|
||||
|
||||
Use the Read tool to understand the context around the specified line.
|
||||
|
||||
### 4b. Make the fix
|
||||
|
||||
Use the Edit tool to make the necessary changes based on the feedback.
|
||||
|
||||
### 4c. Reply to the comment
|
||||
|
||||
Use REST API to reply to the comment explaining what was done:
|
||||
|
||||
```bash
|
||||
gh api --method POST \
|
||||
repos/{owner}/{repo}/pulls/PR_NUMBER/comments/COMMENT_DB_ID/replies \
|
||||
-f body='Fixed: [explanation of what was changed and why]'
|
||||
```
|
||||
|
||||
Keep replies concise but informative. Explain WHAT was changed and WHY.
|
||||
|
||||
### 4d. Resolve the thread
|
||||
|
||||
Use GraphQL mutation to resolve the thread:
|
||||
|
||||
```bash
|
||||
gh api graphql -f query='
|
||||
mutation {
|
||||
resolveReviewThread(input: {threadId: "THREAD_NODE_ID"}) {
|
||||
thread { isResolved }
|
||||
}
|
||||
}'
|
||||
```
|
||||
|
||||
## Step 5: Commit Changes
|
||||
|
||||
After all fixes are made:
|
||||
|
||||
1. Stage all changed files with `git add`
|
||||
2. Create a commit with a descriptive message following conventional commits format:
|
||||
|
||||
```bash
|
||||
git commit -m "fix: address PR review feedback
|
||||
|
||||
- [list each fix made]
|
||||
- [reference comment IDs if helpful]
|
||||
|
||||
🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
||||
|
||||
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
3. Push the changes to the remote branch
|
||||
|
||||
## Step 6: Request Re-Review
|
||||
|
||||
Add a comment to the PR requesting re-review from @codex:
|
||||
|
||||
```bash
|
||||
gh pr comment PR_NUMBER --body '@codex review'
|
||||
```
|
||||
|
||||
## Step 7: Summary
|
||||
|
||||
Present a summary to the user:
|
||||
|
||||
- Number of comments addressed
|
||||
- Files modified
|
||||
- Commit hash created
|
||||
- Any comments that were NOT addressed (with reasons)
|
||||
- Link to the PR
|
||||
|
||||
## Error Handling
|
||||
|
||||
- If `gh` CLI is not authenticated, instruct user to run `gh auth login`
|
||||
- If PR number is invalid, show error and ask for correct number
|
||||
- If a thread fails to resolve, log the error but continue with other threads
|
||||
- If commit fails, show the error and suggest manual resolution
|
||||
+30
-17
@@ -210,12 +210,24 @@ fn try_main() -> anyhow::Result<()> {
|
||||
// [2]: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/kqueue.2.html
|
||||
unsafe { signal(Signal::SIGINT, SigHandler::SigIgn) }?;
|
||||
|
||||
let mut args: Args = Args::parse();
|
||||
let Args {
|
||||
vm_fd,
|
||||
vm_mac_address,
|
||||
vm_net_type,
|
||||
bootpd_lease_time,
|
||||
user,
|
||||
group,
|
||||
allow,
|
||||
block,
|
||||
expose,
|
||||
sudo_escalation_probing,
|
||||
sudo_escalation_done,
|
||||
} = Args::parse();
|
||||
|
||||
// No need to run anything, just return
|
||||
// so that the invoker process knows we
|
||||
// can be invoked in Sudo as root
|
||||
if args.sudo_escalation_probing {
|
||||
if sudo_escalation_probing {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -231,7 +243,7 @@ fn try_main() -> anyhow::Result<()> {
|
||||
|
||||
// Ensure we are running as root
|
||||
if get_effective_uid() != 0 {
|
||||
if sudo_escalation_works() && !args.sudo_escalation_done {
|
||||
if sudo_escalation_works() && !sudo_escalation_done {
|
||||
let exe = std::env::current_exe().unwrap();
|
||||
let args = std::env::args().skip(1);
|
||||
|
||||
@@ -253,28 +265,28 @@ fn try_main() -> anyhow::Result<()> {
|
||||
));
|
||||
}
|
||||
|
||||
let allow = resolve_allow_block_entries("allow", std::mem::take(&mut args.allow))?;
|
||||
let block = resolve_allow_block_entries("block", std::mem::take(&mut args.block))?;
|
||||
let allow = resolve_allow_block_entries("allow", allow)?;
|
||||
let block = resolve_allow_block_entries("block", block)?;
|
||||
|
||||
// Set bootpd(8) min/max lease time while still having the root privileges
|
||||
set_bootpd_lease_time(args.bootpd_lease_time);
|
||||
set_bootpd_lease_time(bootpd_lease_time);
|
||||
|
||||
// Initialize the proxy while still having the root privileges
|
||||
let mut proxy = Proxy::new(
|
||||
args.vm_fd as RawFd,
|
||||
args.vm_mac_address,
|
||||
args.vm_net_type,
|
||||
vm_fd as RawFd,
|
||||
vm_mac_address,
|
||||
vm_net_type,
|
||||
PrefixSet::from_iter(allow),
|
||||
PrefixSet::from_iter(block),
|
||||
args.expose,
|
||||
expose,
|
||||
)
|
||||
.context("failed to initialize proxy")?;
|
||||
|
||||
// Drop effective privileges to the user
|
||||
// and group which have had invoked us
|
||||
PrivDrop::default()
|
||||
.user(args.user.unwrap_or(current_user_name))
|
||||
.group(args.group.unwrap_or(current_group_name))
|
||||
.user(user.unwrap_or(current_user_name))
|
||||
.group(group.unwrap_or(current_group_name))
|
||||
.apply()
|
||||
.context("failed to drop privileges")?;
|
||||
|
||||
@@ -325,15 +337,16 @@ mod tests {
|
||||
use std::net::Ipv4Addr;
|
||||
|
||||
#[test]
|
||||
fn resolve_domain_to_ipv4_nets_example_com() {
|
||||
fn resolve_domain_to_ipv4_nets_dns_google() {
|
||||
let nets = resolve_allow_block_entries(
|
||||
"allow",
|
||||
vec![AllowBlockEntry::Domain("example.com".to_string())],
|
||||
vec![AllowBlockEntry::Domain("dns.google".to_string())],
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
assert!(nets.contains(&Ipv4Net::from(Ipv4Addr::new(
|
||||
93, 184, 216, 34
|
||||
))));
|
||||
assert!(
|
||||
nets.contains(&Ipv4Net::from(Ipv4Addr::new(8, 8, 8, 8)))
|
||||
|| nets.contains(&Ipv4Net::from(Ipv4Addr::new(8, 8, 4, 4)))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user