From 8929baf76a503750d5030d62c542604556c0c645 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Mon, 16 Feb 2026 23:52:42 -0800 Subject: [PATCH] feat: add review and fix-issue project commands (#104) * feat: add review and fix-issue project commands Add 4 Claude Code project commands adapted from global skills, tailored to IronClaw's build/test/lint workflow and conventions: - review-pr: Paranoid architect PR review across 6 lenses - review-crate: Deep Rust crate audit (vulnerabilities, bugs, unfinished work) - respond-pr: Triage and address PR review comments - fix-issue: End-to-end GitHub issue resolution with branch/plan/implement flow Co-Authored-By: Claude Opus 4.6 * fix: address PR review feedback on project commands - Add headRefOid to gh pr view and resolve {owner}/{repo} in review-pr.md so Step 6 line comments actually work (Gemini + Copilot) - Add --paginate to gh api calls in respond-pr.md for large PRs (Gemini + Copilot) - Use gh repo view --json defaultBranchRef instead of hardcoded main/master fallback in fix-issue.md (Gemini) - Narrow allowed-tools in all four commands to match repo convention of specific subcommands (Bash(cargo fmt:*) style) instead of broad wildcards (Copilot) - Clarify >20 files guidance in review-pr.md: read all, process in priority order (Copilot) - Make cargo audit mandatory with install hint in review-crate.md (Gemini) Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- .claude/commands/fix-issue.md | 97 ++++++++++++ .claude/commands/respond-pr.md | 81 ++++++++++ .claude/commands/review-crate.md | 245 +++++++++++++++++++++++++++++++ .claude/commands/review-pr.md | 170 +++++++++++++++++++++ 4 files changed, 593 insertions(+) create mode 100644 .claude/commands/fix-issue.md create mode 100644 .claude/commands/respond-pr.md create mode 100644 .claude/commands/review-crate.md create mode 100644 .claude/commands/review-pr.md diff --git a/.claude/commands/fix-issue.md b/.claude/commands/fix-issue.md new file mode 100644 index 00000000..a9519774 --- /dev/null +++ b/.claude/commands/fix-issue.md @@ -0,0 +1,97 @@ +--- +description: Fetch a GitHub issue, create a branch, research the codebase, plan the fix, implement with tests, and commit +disable-model-invocation: true +allowed-tools: Bash(gh issue view:*), Bash(gh repo view:*), Bash(git fetch:*), Bash(git checkout:*), Bash(git status:*), Bash(git branch:*), Bash(git add:*), Bash(git commit:*), Bash(cargo fmt:*), Bash(cargo clippy:*), Bash(cargo test:*), Read, Edit, Write, Grep, Glob +argument-hint: "" +--- + +# Fix GitHub Issue + +## Step 1: Resolve the issue + +Parse `$ARGUMENTS` to extract the issue number: +- If it's a URL like `https://github.com/owner/repo/issues/42`, extract `42`. +- If it's a bare number, use it directly. +- If empty, stop and ask the user for an issue number. + +Fetch the issue: + +``` +gh issue view {number} --json title,body,labels,assignees,comments,state +``` + +If the issue is closed, warn the user and ask if they still want to proceed. + +## Step 2: Create a branch + +Create a fresh branch off the latest main: + +1. Fetch latest: `git fetch origin` +2. Detect default branch: `gh repo view --json defaultBranchRef --jq .defaultBranchRef.name` +3. Create and switch to a new branch: `git checkout -b fix/{number}-{short-slug} origin/{default-branch}` + - `{short-slug}` is 3-5 words from the issue title, lowercase, hyphenated (e.g. `fix/42-idor-workspace-check`) + +If the working tree has uncommitted changes, warn the user and stop. Do not stash or discard their work. + +## Step 3: Understand the issue + +Summarize the issue in 2-3 sentences. Identify: +- **What's broken or missing** (the symptom or feature request) +- **Acceptance criteria** (what "done" looks like, from the issue body or comments) +- **Constraints** (mentioned technologies, backward compatibility, performance requirements) + +If the issue is unclear or ambiguous, list the open questions. These will be addressed during planning. + +## Step 4: Research the codebase + +Before planning, gather context: + +1. **Find relevant code** - Search for files, functions, types, and patterns mentioned in the issue. Read them in full. +2. **Trace the flow** - If the issue is about a specific behavior, trace the code path from the entry point (route handler, CLI command, etc.) through to the relevant logic. +3. **Check existing tests** - Find tests related to the affected code. Understand what's already covered. +4. **Check for prior art** - Look for similar patterns in the codebase that solve analogous problems. Prefer consistency with existing patterns. + +## Step 5: Enter planning mode + +Enter planning mode to design the implementation. The plan MUST cover: + +1. **Root cause** (for bugs) or **design approach** (for features) +2. **Files to modify** with specific descriptions of what changes in each +3. **New files** (if any) with justification for why they're needed +4. **Tests to add** - every code path introduced or changed needs a test: + - Happy path (expected input produces expected output) + - Error paths (invalid input, missing data, permission denied) + - Edge cases (empty collections, boundary values, concurrent access) +5. **IronClaw-specific concerns**: + - If the change touches persistence, both database backends must be updated (`postgres.rs` and `libsql_backend.rs`) + - New `Database` trait methods need implementations in both backends + - No `.unwrap()` or `.expect()` in production code + - Use `crate::` imports, not `super::` + - Error types via `thiserror` in `error.rs` +6. **Migration or compatibility concerns** (if any) + +Follow the project's CLAUDE.md guidance for architecture decisions. + +Wait for user approval before implementing. + +## Step 6: Implement + +After the plan is approved: + +1. Implement each change from the plan. +2. Write all planned tests. +3. Run IronClaw's full quality gate: + - `cargo fmt` + - `cargo clippy --all --benches --tests --examples --all-features` (zero warnings) + - `cargo test --lib` (all tests pass) +4. If any check fails, fix it before proceeding. + +Note: Integration tests (`--test workspace_integration`) require PostgreSQL and are expected to fail locally. Only `--lib` test failures are blocking. + +## Step 7: Commit and summarize + +1. Commit with a descriptive message referencing the issue (e.g. `fix: prevent IDOR in function call outputs (#42)`). +2. Summarize what was done: + - Files changed with line references + - Tests added and what they cover + - Any follow-up work or open questions diff --git a/.claude/commands/respond-pr.md b/.claude/commands/respond-pr.md new file mode 100644 index 00000000..1db91207 --- /dev/null +++ b/.claude/commands/respond-pr.md @@ -0,0 +1,81 @@ +--- +description: Respond to PR review comments — triage, plan fixes, implement after confirmation, push, and reply to reviewers +disable-model-invocation: true +allowed-tools: Bash(gh pr list:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(gh repo view:*), Bash(git branch:*), Bash(git status:*), Bash(git add:*), Bash(git commit:*), Bash(git push:*), Bash(cargo fmt:*), Bash(cargo clippy:*), Bash(cargo test:*), Read, Edit, Write, Grep, Glob +argument-hint: "[pr-number (optional, auto-detects from branch)]" +--- + +# Review and Address PR Comments + +## Step 1: Find the PR + +If `$ARGUMENTS` is provided, use that as the PR number. Otherwise, detect the PR for the current branch: + +``` +gh pr list --head $(git branch --show-current) --json number,title,url --jq '.[0]' +``` + +If no PR is found, tell the user and stop. + +## Step 2: Fetch all review comments + +Resolve the repo owner and name: + +``` +gh repo view --json owner,name --jq '"\(.owner.login)/\(.name)"' +``` + +Fetch the full set of review comments (not issue-level comments): + +``` +gh api --paginate repos/{owner}/{repo}/pulls/{number}/comments +``` + +Also fetch the review summaries: + +``` +gh api --paginate repos/{owner}/{repo}/pulls/{number}/reviews +``` + +Deduplicate comments that appear multiple times (bots sometimes post the same finding under different IDs). Group by the actual issue being raised, not by comment ID. + +## Step 3: Triage and plan + +For each unique issue raised in the comments: + +1. **Check if already addressed** - Read the current code at the referenced location. If a prior commit already fixed it, note it as "already resolved". +2. **Assess validity** - Determine if the comment identifies a real problem or is a false positive. Be honest about false positives but explain why. +3. **Classify severity** - Critical (security/data loss), High (bugs/broken behavior), Medium (correctness/robustness), Low (style/naming/nits). +4. **Plan the fix** - For each valid unresolved issue, describe the specific code change needed. + +Present the plan as a table to the user: + +| # | Issue | File:Line | Severity | Status | Planned Fix | +|---|-------|-----------|----------|--------|-------------| + +Wait for user confirmation before proceeding to implementation. + +## Step 4: Implement fixes + +After user confirms: + +1. Implement each fix in the plan. +2. Run IronClaw's quality gate to verify nothing breaks: + - `cargo fmt` + - `cargo clippy --all --benches --tests --examples --all-features` + - `cargo test --lib` +3. Commit with a descriptive message referencing the PR review. +4. Push to the branch. + +## Step 5: Reply to comments + +For each comment addressed, reply on the PR with a short message stating what was fixed and the commit SHA. For false positives or already-resolved items, reply explaining why no change was needed. + +## Rules + +- Never guess at code you haven't read. Always read the referenced file and line before assessing a comment. +- Group duplicate comments (same issue reported by multiple bots) and reply to all of them. +- Do not make changes beyond what the review comments ask for. Stay focused. +- If a comment suggests a change you disagree with, present your reasoning to the user during the planning phase rather than silently ignoring it. +- Follow IronClaw conventions: no `.unwrap()` in production code, use `crate::` imports, `thiserror` errors. +- If changes touch persistence, verify both database backends are updated. diff --git a/.claude/commands/review-crate.md b/.claude/commands/review-crate.md new file mode 100644 index 00000000..0d51c007 --- /dev/null +++ b/.claude/commands/review-crate.md @@ -0,0 +1,245 @@ +--- +description: Deep audit of the IronClaw crate for vulnerabilities, bugs, unfinished work, inconsistencies, and oversights +disable-model-invocation: true +allowed-tools: Bash(cargo fmt:*), Bash(cargo clippy:*), Bash(cargo test:*), Bash(cargo audit:*), Bash(git diff:*), Bash(git log:*), Bash(git show:*), Bash(wc:*), Read, Grep, Glob, Task +argument-hint: "[path/to/crate]" +--- + +# Rust Crate Audit + +You are performing a thorough audit of a Rust crate. Your goal is to find every vulnerability, bug, unfinished piece of work, inconsistency, and oversight before it ships. Leave no stone unturned. + +## Step 1: Locate the crate + +Parse `$ARGUMENTS`: +- If a path is provided, use it as the crate root. +- If empty, use the current working directory. + +Verify it's a valid Rust crate by checking for `Cargo.toml`. If not found, stop and ask the user. + +## Step 2: Understand the crate + +Read `Cargo.toml` to understand: +- Crate name, version, edition +- Dependencies (look for outdated, unmaintained, or suspicious crates) +- Feature flags and their implications +- Build scripts (`build.rs`) if any + +Read `CLAUDE.md`, `README.md`, or top-level documentation if present to understand intent and architecture. + +Read `src/lib.rs` or `src/main.rs` to get the module tree. Then read each module's `mod.rs` or top-level file to build a mental map of the crate's structure before diving into details. +Read all Rust files (`src/*.rs`) to make sure everything is in context when you are reasoning. + +## Step 3: Run the compiler's checks + +Run these commands and capture output. Do NOT fix anything, just collect findings: + +``` +cargo fmt --check 2>&1 +``` + +``` +cargo clippy --all --benches --tests --examples --all-features -- -W clippy::all -W clippy::pedantic -W clippy::nursery 2>&1 +``` + +``` +cargo test --lib 2>&1 +``` + +If any of these fail, record the failures as findings. If `cargo test` has ignored tests, note which ones and why. + +Note: Integration tests (`--test workspace_integration`) require a PostgreSQL database and are expected to fail locally. Only report `--lib` test failures as blocking. + +## Step 4: Scan for unfinished work + +Search the entire `src/` tree for: + +``` +todo! +unimplemented! +fixme +FIXME +TODO +HACK +XXX +SAFETY: +stub +placeholder +temporary +``` + +For each match: +- Is it in production code or test code? +- Is it a genuine incomplete feature or a deliberate placeholder? +- Is there a tracking issue referenced? +- Could this panic at runtime? + +Any `todo!()` or `unimplemented!()` in non-test code is **High severity** (runtime panic). + +## Step 5: Audit for vulnerabilities and unsafe code + +### 5a. Unsafe code + +Search for all `unsafe` blocks. For each one: +- Is the safety invariant documented with a `// SAFETY:` comment? +- Is the invariant actually upheld by the surrounding code? +- Could the unsafe block be replaced with a safe alternative? +- Are there any pointer dereferences, transmutes, or FFI calls? + +### 5b. Unwrap and panic paths + +Search for `.unwrap()`, `.expect(`, `panic!`, `unreachable!` in non-test code. For each: +- Can this actually panic in production? +- Is there a code path that reaches this with None/Err? +- Should it be replaced with proper error handling (`?`, `.ok()`, `.unwrap_or_default()`)? + +IronClaw convention: `.unwrap()` and `.expect()` are banned in production code. Any occurrence outside `#[cfg(test)]` blocks is a **High severity** finding. + +### 5c. SQL and injection vectors + +Search for string formatting used in SQL queries, shell commands, or HTML: +- `format!` used near `.execute(`, `.query(`, `Command::new(` +- String interpolation in query construction vs parameterized queries +- User input flowing into file paths (`Path::new`, `std::fs::`) + +IronClaw has two database backends (PostgreSQL and libSQL). Check both for injection vectors. + +### 5d. Cryptographic issues + +If the crate uses crypto: +- Are comparisons constant-time? (look for `==` on secrets/hashes vs `subtle::ConstantTimeEq`) +- Is randomness from `OsRng` / `thread_rng` and not a fixed seed? +- Are keys/secrets zeroized after use? (`secrecy`, `zeroize` crates) +- Are deprecated algorithms used? (MD5, SHA1 for security, RC4, DES) + +### 5e. Resource exhaustion + +- Are there unbounded allocations? (`Vec` growing from user input without limits) +- Are there unbounded loops? (retry loops without max attempts) +- Are file reads bounded? (`std::fs::read_to_string` on user-provided paths) +- Are timeouts set on all network operations? +- Are there connection/resource leaks? (opened but never closed, missing `Drop`) + +### 5f. Error handling + +- Are errors swallowed silently? (`let _ = ...`, `.ok()` discarding errors that matter) +- Do error types carry enough context to debug in production? +- Are there error type mismatches? (returning generic `anyhow::Error` where a typed error would prevent confusion) +- Is `thiserror` used consistently for error types (IronClaw convention)? + +## Step 6: Check for inconsistencies + +### 6a. Naming conventions + +- Are types, functions, modules named consistently? (e.g., mixing `get_` and `fetch_`, `create_` and `new_`) +- Do similar operations follow the same patterns? + +### 6b. Duplicate or near-duplicate code + +Look for: +- Functions that do nearly the same thing with minor variations (candidates for generics or shared helpers) +- Repeated error mapping patterns that should be extracted +- Copy-pasted SQL queries or string templates with slight differences +- Identical struct definitions or conversion logic in different modules + +### 6c. API consistency + +- Do similar functions take arguments in the same order? +- Are return types consistent? (e.g., some functions return `Option`, similar ones return `Result`) +- Are visibility modifiers consistent? (`pub` where it should be `pub(crate)`, or vice versa) + +### 6d. Dead code and unused items + +- Are there functions, structs, or modules that nothing references? +- Are there `#[allow(dead_code)]` annotations that should be investigated? +- Are there feature-gated items where the feature is never enabled? + +### 6e. Import style + +IronClaw convention: use `crate::` imports, not `super::`. Flag any `super::` imports in non-test code. + +## Step 7: Inspect for change oversights + +### 7a. Partial refactors + +- Are there old patterns coexisting with new patterns? +- Are there renamed types/functions where some call sites still use the old name via a compatibility alias? +- Are there comments referencing behavior that no longer exists? + +### 7b. Trait implementation gaps + +- If a trait is defined, do all intended types implement it? +- Are there `impl` blocks that look incomplete? +- Are `Default` implementations sensible? + +IronClaw key traits: `Database` (~60 methods), `Channel`, `Tool`, `LlmProvider`, `SuccessEvaluator`, `EmbeddingProvider`. If any new methods were added to `Database`, verify both `postgres.rs` and `libsql_backend.rs` implement them. + +### 7c. Test coverage gaps + +- Are there public functions without any test? +- Are there error paths without tests? +- Are there recently-changed functions where the tests still assert old behavior? + +### 7d. Documentation drift + +- Do doc comments match actual function behavior? +- Are examples in doc comments still valid and compilable? + +## Step 8: Dependency audit + +Review `Cargo.toml` and `Cargo.lock`: +- Are there duplicate versions of the same crate in the lock file? (potential version conflicts) +- Are there dependencies with known security advisories? Run `cargo audit` to check (install with `cargo install cargo-audit` if not present). +- Are there heavy dependencies used for trivial functionality? +- Are dependency features minimal? + +## Step 9: Present findings + +Compile all findings into a structured report. Group by severity, then by category. + +### Format + +For each finding: + +``` +### [Severity] Category: One-line summary + +**Location:** `file_path:line_number` +**Category:** Vulnerability | Bug | Unfinished | Inconsistency | Duplicate | Oversight | Style + +**Description:** +Detailed explanation of the issue, why it matters, and how it could manifest. + +**Suggested fix:** +Concrete suggestion with code if applicable. +``` + +### Severity levels + +- **Critical**: Security vulnerability, data loss, or crash in production +- **High**: Bug that causes incorrect behavior, `todo!()`/`unimplemented!()` in prod code, or missing validation on trust boundaries +- **Medium**: Inconsistency, duplicate code, incomplete error handling, missing tests for important paths +- **Low**: Naming inconsistency, unnecessary complexity, documentation drift, minor dead code +- **Nit**: Style preference, optional improvement + +### Summary table + +End with a summary table: + +| # | Severity | Category | File:Line | Finding | +|---|----------|----------|-----------|---------| + +And a final tally: X Critical, Y High, Z Medium, W Low, V Nit. + +## Rules + +- Read every file before reporting on it. Never guess about code you haven't seen. +- Be specific. "This might have issues" is worthless. "Line 42 calls `.unwrap()` on a `Result` that returns `Err` when the DB connection is dropped" is useful. +- Distinguish certainty levels: "this IS a bug" vs "this COULD be a bug if X". +- Don't invent problems to look thorough. If the code is solid, say so. +- Focus on substance over style. Don't flag formatting unless it causes real confusion. +- Respect existing project conventions (check CLAUDE.md). Don't flag patterns the project explicitly endorses. +- When in doubt about severity, round up. +- For large crates (>50 files), prioritize: core logic > public API > internal utilities > tests > examples. +- Use the Task tool to parallelize file reading across modules when the crate is large. +- Do NOT fix anything. This is a read-only audit. Report findings for the user to action. diff --git a/.claude/commands/review-pr.md b/.claude/commands/review-pr.md new file mode 100644 index 00000000..6794444f --- /dev/null +++ b/.claude/commands/review-pr.md @@ -0,0 +1,170 @@ +--- +description: Paranoid architect review of a PR — fetches diff, reads changed files, deep review across 6 lenses, posts findings as GitHub comments +disable-model-invocation: true +allowed-tools: Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh pr comment:*), Bash(gh api:*), Bash(gh repo view:*), Bash(git diff:*), Bash(git log:*), Read, Grep, Glob +argument-hint: "" +--- + +# Paranoid Architect Code Review + +You are reviewing this PR as a paranoid architect. Your job is to find every bug, vulnerability, race condition, edge case, and undocumented assumption before it ships. Assume adversarial users, concurrent access, and Murphy's law. + +## Step 1: Resolve the PR + +Parse `$ARGUMENTS` to extract the PR number: +- If it's a URL like `https://github.com/owner/repo/pull/123`, extract `123`. +- If it's a bare number, use it directly. +- If empty, stop and ask the user for a PR number. + +Fetch PR metadata (including head commit SHA for posting line comments later): + +``` +gh pr view {number} --json title,body,baseRefName,headRefName,headRefOid,files,additions,deletions +``` + +Save the `headRefOid` value, you'll need it as `commit_id` in Step 6. + +## Step 2: Load the full diff + +``` +gh pr diff {number} +``` + +Also get the list of changed files: + +``` +gh pr diff {number} --name-only +``` + +## Step 3: Read every changed file in full + +For each changed file, read the ENTIRE current file (not just the diff hunks). You need surrounding context to catch: +- Callers of modified functions that now behave differently +- Trait/interface contracts that the change may violate +- Invariants established elsewhere that the diff breaks + +If the PR touches more than 20 files, still read all of them, but process in this priority order: service logic > routes/handlers > models/types > tests > docs. Batch reads in groups of ~20 if needed. + +## Step 4: Deep review + +Go through the changes with each of these lenses. For every finding, note the file, line range, severity, and a concrete description. + +### IronClaw-specific checks + +In addition to the general lenses below, check IronClaw conventions (see CLAUDE.md): +- No `.unwrap()` or `.expect()` in production code (tests are fine) +- Use `crate::` imports, not `super::` +- Error types use `thiserror` in `error.rs` +- If the change touches persistence, verify both database backends are updated (PostgreSQL in `postgres.rs` AND libSQL in `libsql_backend.rs`) +- New tools must implement the `Tool` trait correctly and be registered in `registry.rs` +- External tool output must pass through the safety layer + +### 4a. Correctness and bugs + +- Off-by-one errors, wrong comparison operators, inverted conditions +- Unreachable code, dead branches, impossible match arms +- Type confusion (mixing up IDs, using wrong enum variant) +- Incorrect error propagation (swallowed errors, wrong error type/status code) +- Broken invariants (e.g. uniqueness assumptions violated, ordering assumptions wrong) +- Concurrency issues (TOCTOU, missing locks, race conditions between check and use) + +### 4b. Edge cases and failure handling + +- What happens with empty input, None/null, zero-length collections? +- What happens when external services fail (DB down, HTTP timeout, malformed response)? +- What happens at integer boundaries (overflow, underflow, i64::MAX)? +- What happens with malformed or adversarial input (invalid UTF-8, huge payloads, deeply nested JSON)? +- Are all error paths tested? Does every `?` propagation make sense? +- Are partial failures handled (e.g. wrote to DB but failed to emit event)? + +### 4c. Security (assume a malicious actor) + +- **Authentication/Authorization bypass**: Can an unauthenticated user reach this? Can workspace A's user access workspace B's data? Are there IDOR vulnerabilities? +- **Injection**: SQL injection via string interpolation? Command injection? Log injection? Header injection? +- **Data leakage**: Are secrets, PII, or conversation content logged? Returned in error messages? Exposed in API responses? +- **Resource exhaustion / DoS**: Can an attacker send unbounded input? Trigger expensive operations without rate limits? Cause OOM via large allocations? +- **Financial abuse**: Can tokens/credits be consumed without being tracked? Can usage limits be bypassed? +- **Replay / race conditions**: Can the same request be replayed for double-spend? Can concurrent requests bypass limits? +- **Cryptographic issues**: Timing attacks on comparisons? Weak randomness? Missing HMAC verification? + +### 4d. Test coverage + +- Is every new public function/method tested? +- Are error paths tested (not just happy paths)? +- Are edge cases covered (empty input, boundary values, concurrent access)? +- Do existing tests still make sense with the new changes, or do they assert stale behavior? +- Are there integration/e2e tests for the full flow? +- If a test is missing, describe exactly what test should be written. + +### 4e. Documentation and assumptions + +- Are new assumptions documented in comments? (e.g. "this field is always non-empty because X") +- Are non-obvious algorithms or business rules explained? +- Are API contracts (request/response shapes, error codes, status codes) documented? +- Are there TODO/FIXME/HACK comments that should be tracked as issues? + +### 4f. Architectural concerns + +- Does this change follow existing patterns in the codebase, or does it introduce a new one without justification? +- Are there unnecessary abstractions or premature generalizations? +- Is there duplicated logic that should be extracted? +- Are dependencies between modules clean, or does this create circular/tight coupling? +- Will this change make future work harder? + +## Step 5: Present findings + +Summarize findings to the user as a table: + +| # | Severity | Category | File:Line | Finding | Suggested Fix | +|---|----------|----------|-----------|---------|---------------| + +Severity levels: +- **Critical**: Security vulnerability, data loss, or financial exploit +- **High**: Bug that will cause incorrect behavior in production +- **Medium**: Robustness issue, missing validation, or incomplete error handling +- **Low**: Style, naming, documentation, or minor improvement +- **Nit**: Optional suggestion, take-it-or-leave-it + +Ask the user which findings to post as PR comments. Default: all Critical, High, and Medium. + +## Step 6: Post comments on GitHub + +Resolve the repo owner and name if not already known: + +``` +gh repo view --json owner,name --jq '"\(.owner.login)/\(.name)"' +``` + +For each approved finding, post a review comment on the PR at the specific file and line. Use the `headRefOid` from Step 1 as the `commit_id`: + +``` +gh api repos/{owner}/{repo}/pulls/{number}/comments \ + -f body="..." \ + -f path="..." \ + -f commit_id="{headRefOid}" \ + -F line=... \ + -f side="RIGHT" +``` + +For findings that span multiple locations or are architectural, post as a regular PR comment: + +``` +gh pr comment {number} --body "..." +``` + +Format each comment clearly: +- Severity tag (e.g. `**High Severity**`) +- One-line summary +- Detailed explanation of the issue +- Concrete suggestion for the fix (with code if possible) + +## Rules + +- Read every changed file in full before writing a single finding. Context matters. +- Never post a comment about code you haven't actually read. Verify line numbers against the actual file. +- Be specific. "This might have issues" is useless. "Line 42 returns 404 but should return 400 because X" is useful. +- Distinguish between "this IS a bug" and "this COULD be a bug if X". Be honest about certainty. +- Don't nitpick formatting or style unless it causes actual confusion. Focus on substance. +- If the code is good and you find nothing, say so. Don't invent problems to look thorough. +- Respect the project's CLAUDE.md privacy rules: never include customer data, secrets, or PII in comments. +- When in doubt about severity, round up. It's cheaper to dismiss a false alarm than to miss a real bug.