ci-code-review

Provide a code review for the given pull request. Follow these steps: 1. **Eligibility Check** (Sonnet): Check if the PR (a) is closed, (b) is automated/trivial. If so, stop. 2. **PR Analysis** (Fable): View the PR and return: - Summary of the change and its purpose - List of new functions, types, enums, or abstractions introduced - For each new abstraction: its name, stated purpose (from comments/docs), and intended usage contract 3. **Parallel Deep Review** (4 Fable agents): Pass the PR summary and new abstractions list to each agent. a. **Semantic Consistency Agent**: For each new function/type/enum introduced: - Read its definition, documentation, and any comments describing when/how it should be used - Find ALL usages of that abstraction within the PR - Verify each usage matches the documented intent - Flag misuse: e.g., error-handling functions called for wrong error types, validation functions bypassed, enums used inconsistently - Pay special attention to: panic/error functions (when should they trigger?), unsafe blocks, security-sensitive operations b. **Deep Bug Analysis Agent**: Read the full context of modified files (not just diff lines). - Understand the data flow and control flow around changes - Check for logic errors, edge cases, off-by-one errors, resource leaks - Verify error handling is appropriate for each failure mode - Check that invariants are maintained across the changes c. **Tech Debt Removal Agent**: - Understand new abstractions that are introduced, new functions/types/enums - Identify possible future usecases for these things and understand whether abstractions meet future requirements - See which paradigms of Rust (or other language) development are used and whether they apply here - Identify possible improvements that would benefit long term maintainability of the code - Be an enjoyer of abstractions: generics, traits, dyn, enums, etc. d. **Security Reviewer Agent**: - Identify whether changes to the protocol do not break soundness - Identify possible attack vectors that are introduced with these changes - Check for input validation gaps at trust boundaries (user input, network data, file I/O, IPC) - Verify authentication/authorization checks are not bypassed or weakened - Look for injection risks: SQL, command, path traversal, template injection, deserialization - Check cryptographic usage: hardcoded secrets, weak algorithms, nonce reuse, timing side-channels - Verify resource limits: unbounded allocations, missing timeouts, denial-of-service vectors - Check concurrency: TOCTOU races, lock ordering, shared mutable state without synchronization 4. **Validate Issues** (MANDATORY — do not skip): After collecting all issues from the 4 agents, validate every issue scored >= 50. For each issue from the agents: - For complex logic/semantic issues, reason through whether the bug is real and exploitable. - For issues you can verify mechanically (e.g., a failing test), prefer direct verification (run the test). Score each issue 0-100 AFTER validation: - 0: False positive, doesn't stand up to scrutiny, or pre-existing issue - 25: Might be real, but couldn't verify. Stylistic issues without explicit guidance. - 50: Verified real issue, but minor/nitpick. Not important relative to PR scope. - 75: Verified real issue that will impact functionality. Insufficient existing approach. - 100: Confirmed real issue that will happen frequently. Direct evidence confirms it. 5. **Post comments to PR**: Always post a single review to the PR so the author has confirmation that the review ran, regardless of whether issues were found. - When there are validated issues with score >= 50, include them in the `comments` array. - When there are none, post the review with an empty `comments` array and a body that briefly states no issues were found (1-2 sentences, same tone as the comment guidelines below — concise, senior-engineer voice, no scores/severity labels/ceremony). Runs in CI — do not pause, do not ask for user confirmation, do not list issues for approval. Build a JSON file and use the GitHub review API: ```bash # Write review JSON to a unique temp file (use $$ for PID to avoid collisions). # With issues: populate the comments array. Without issues: leave it empty ([]) and # write a short "nothing actionable" body. cat > "/tmp/pr-review-${PR_NUMBER}-$$.json" << 'EOF' { "commit_id": "<HEAD_SHA>", "event": "COMMENT", "body": "Short review summary (1-2 sentences). When no issues were found, state that plainly.", "comments": [ { "path": "relative/path/to/file.rs", "line": 42, "body": "Comment text — see tone guidelines below" } ] } EOF # Post the review (all comments appear as a single review). Always run this call, # even when "comments" is []. gh api repos/{owner}/{repo}/pulls/{number}/reviews --method POST --input "/tmp/pr-review-${PR_NUMBER}-$$.json" ``` Key points: - Use a heredoc with `'EOF'` (quoted) to prevent shell interpolation of `$`, backticks, etc. - The `line` field refers to the NEW file line number (right side of diff) for added/modified lines. - Get the head SHA via `gh api repos/{owner}/{repo}/pulls/{number} --jq '.head.sha'`. - Get changed files via `gh api repos/{owner}/{repo}/pulls/{number}/files`. - Do NOT use `--raw-field` for the comments array — it doesn't handle nested JSON. Always use `--input` with a file. **Comment tone**: Write like a senior engineer — concise, direct, no ceremony. - NO scores, severity labels, or `**[Score X]**` prefixes - NO "Title + Explanation" structure — just say the thing - Lead with what's wrong or what to consider, then why in 1-2 sentences max - When the fix is obvious, use a GitHub suggestion block in the comment body. Pick the lines to replace via `line` (end) and `start_line` (start) on the comment object, then put a ` ```suggestion ` fenced block in the body — its content replaces those lines: ``` Poisoned mutex will panic all future callers. ```suggestion _guard: mutex.lock().unwrap_or_else(|e| e.into_inner()), ``` ``` For single-line suggestions, omit `start_line` (defaults to `line`). - Examples of good comments: - `"If only one of the three openings is missing, the dummy all-zero r_address hits this assert before take_missing_opening_error() runs. Consider a fallible check here."` - `"This leaves the original opening accessible after recording MalformedProof. Removing it too would prevent the verifier from using a stale claim within the stage."` - `"Poisoned mutex will panic all future callers."` (with a ` ```suggestion ` block for the fix) ## False Positives (skip these) - Pre-existing issues not introduced by this PR - Issues on lines not modified in the PR - Things linters/compilers catch (imports, types, formatting) - Pedantic nitpicks a senior engineer wouldn't flag - Intentional functionality changes related to PR purpose - General quality concerns (test coverage, docs) unless explicitly required ## Notes - Do NOT run builds/tests - CI handles that - Use `gh` for all GitHub interaction