Menu
Review pull requests for code quality, security, and correctness
Install with Codex or Claude Copy this prompt, paste it into Codex, Claude, or another assistant, and let it review the skill page and install it for you.
Based on SOC occupation classification
| name | pr-review |
| version | 2.0.0 |
| standalone | true |
| description | Review pull requests for code quality, security, and correctness |
| uses | ["development/git","core/memory","core/repo-intelligence"] |
| requires | {"tools":["git","gh"],"env":[]} |
| security | {"risk_level":"write","capabilities":["git:read","github:pr:read","github:pr:comment"],"requires_approval":false} |
Thorough code review for pull requests. Handles the full review process including context loading, analysis, inline commenting, and verdict submission.
Standalone: This skill works without agent-coordinator. Steps marked (framework only) can be skipped when running standalone in Claude Code.
Every PR gets TWO independent review passes. Only publish a verdict after both.
Round 1 — Code Reviewer: Run references/checklist.md CRITICAL tier.
Focus on correctness, security, test coverage, design consistency.
Round 2 — Silent Failure Hunter: Re-read the FULL diff with a different
mental model. Hunt for what Round 1 missed: swallowed exceptions, race
conditions, data loss on edge inputs, missing error paths, state not cleaned up.
Prefix all Round 2 findings with [R2].
Verdict rules:
review_changes_requestedreview_approvedWhen the Agent tool is available (SDK wrapper or interactive mode), spawn specialized subagents for each round instead of doing both yourself:
Agent(description="Code reviewer", prompt="Review PR #{pr_number}. Run checklist CRITICAL tier. Return findings.")
Agent(description="Silent failure hunter", prompt="Hunt for silent failures in PR #{pr_number}. Prefix findings with [R2].")
Launch both in parallel for faster reviews. Combine their findings for the verdict.
Optional third agent for large diffs (200+ lines):
Agent(description="Security auditor", prompt="Security audit PR #{pr_number}. Check OWASP top 10, auth bypass, injection.")
Scale review effort based on diff size:
| Diff Size | Review Depth |
|---|---|
| < 50 lines | Single-pass checklist (no subagents) |
| 50-199 lines | Two rounds (code reviewer + silent failure hunter) |
| 200+ lines | Three rounds (+ security auditor subagent) |
| 500+ lines | Flag for split — PR may be too large for effective review |
To check diff size: gh pr diff {pr_number} --stat | tail -1
Auto-fix mechanical issues. Ask only for judgment calls.
| Auto-fixable (commit directly) | NOT auto-fixable (report as finding) |
|---|---|
| Dead imports / unused variables | Security design decisions |
| Formatting / whitespace | Architecture changes |
| Typos in comments | API contract changes |
| Stale comments that don't match code | Performance trade-offs |
| Missing type annotations | Business logic changes |
When auto-fixing: create a fixup commit on the PR branch, push, note in review comment.
Platform note: Uses GitHub (gh CLI). The review methodology is
platform-agnostic; only the tool commands are GitHub-specific.
Before reviewing code quality, check if the PR does what it claims:
Scope drift is a HIGH finding — it means either the PR is incomplete or contains unintended changes.
Do NOT begin reviewing until you have read every section below. Each section has a purpose. The lifecycle steps at the end (Section 6) are MANDATORY — skipping them leaves the task in limbo and the developer is never notified of your verdict.
If you catch yourself thinking any of these, STOP. These are the shortcuts that lead to rubber-stamp reviews. Every one has caused a real bug to ship.
| Rationalization | Why It's Wrong | Required Action |
|---|---|---|
| "Small PR, quick review" | Heartbleed was a 2-line change. Size ≠ risk. | Full methodology, every step. |
| "This pattern looks safe, I've seen it before" | Each context has different callers, state, and trust boundaries. | Verify THIS specific instance. |
| "The tests pass, so it's correct" | Tests only cover what the developer thought to test. Missing tests are invisible. | Check what's NOT tested. |
| "I'll flag it as Medium, not blocking" | If it causes data loss or silent failure in production, it's Critical regardless of how minor the code change looks. | Assess actual production impact, not code-change size. |
| "The developer probably thought about this" | You are the safety net. If you assume the developer handled it, nobody checked. | Verify explicitly — read the code path. |
| "This is just a UI change, low risk" | DOM state bugs, XSS, error swallowing, and accessibility failures all live in UI code. | Apply full checklist including Silent Failure Hunting. |
| "Skipping full verification for efficiency" | Partial analysis is worse than no analysis — it creates false confidence. | Complete every applicable checklist item. |
| "Similar code elsewhere does this" | Similar code elsewhere may also be buggy. Don't inherit assumptions. | Verify independently. |
| "Just a refactor, no security impact" | Refactors break invariants. Changed code paths need the same scrutiny as new code. | Analyze as HIGH risk until proven LOW. |
| "It's documented in the README" | Developers don't read docs under deadline pressure. Security must be enforced in code, not docs. | Verify the code enforces the documented behavior. |
MANDATORY: Read BOTH files in
references/before starting:
references/common-misses.md— 7 patterns reviewers consistently missreferences/checklist.md— structured 2-tier checklist (CRITICAL + INFORMATIONAL)If you skip these, you WILL miss real bugs.
These are the most commonly skipped steps. If your context window is running low, prioritize these over adding more inline comments:
references/common-misses.md AND references/checklist.md — the patterns and checks you're most likely to missgh api (Section 6) — a summary-only review is INCOMPLETEThis is a builtin skill with per-tool handlers. When pr_diff is invoked,
the executor runs gh pr diff. When pr_comment is invoked, the executor posts
the review comment. The agent applies the methodology between tool calls.
pr_created message arrives on the busreview_request message arrives (developer addressed comments)Before reviewing, claim the task so other reviewers don't duplicate work:
COORDINATOR="${AC_COORDINATOR_URL:-http://localhost:9889}"
TASK_ID="{task_id}" # from the bus message payload
AGENT_NAME="{your-agent-name}"
# Claim the task (JSON body, not query param)
curl -s -X POST "${COORDINATOR}/tasks/${TASK_ID}/claim" \
-H "Content-Type: application/json" \
-d "{\"agent_id\": \"${AGENT_NAME}\"}"
# Expected: {"task_id": N, "claimed": true, ...}
# If 409 → another reviewer already claimed it. STOP — do not review.
Checkpoint:
curl -s "${COORDINATOR}/tasks/${TASK_ID}"showsagent_idis your name.
Before reviewing, use the query_repo_context tool to load intelligence:
CRITICAL — Resolve PR Number First: Issue numbers and PR numbers are DIFFERENT namespaces.
A bus message with issue_id: 473 may correspond to PR #474. Never assume they are the same.
Never look at local branches to find the PR. Always use gh to resolve the actual PR number.
If the bus message contains pr_number: use it directly.
If the bus message contains only issue_id (no pr_number):
# Method 1: Search by linked issue (most reliable)
gh pr list --state open --search "closes:#<issue_id>" --json number,title,headRefName
# Method 2: Match branch name pattern (e.g. fix/<issue_id>-description)
gh pr list --state open --json number,title,headRefName | grep "<issue_id>"
# Method 3: List all open PRs and inspect
gh pr list --state open --json number,title,headRefName,url
Verify the resolved PR number before proceeding:
gh pr view <pr_number> --json number,title,state,body
Checkpoint: You have a confirmed integer PR number. Write it down — every subsequent step uses this number. If you cannot resolve it, STOP and notify via bus before proceeding.
Use the pr_diff tool to get the full diff. Also use feedback_fetch to check existing comments and CI status. Gather:
gh pr list --state openBefore diving into code details, assess the overall design. This is the most important review dimension — if the design is wrong, code quality doesn't matter.
PR Splittability Assessment (flag if ANY apply):
Anti-Over-Engineering Check (LLM-generated code is especially prone to these):
You are an LLM reviewing code likely written by an LLM. Both of you share the same tendencies. Actively watch for these and flag them:
_deprecated_ wrappers when the code is internal and can just be changed.# increment counter above counter += 1). Good comments explain why.Checkpoint: You can articulate in one sentence what this PR does and why the chosen approach is (or isn't) the right one.
Before reviewing individual files, trace the complete functional flow of the change:
Map the flow end-to-end before diving into line-by-line review. This catches architectural issues that per-file review misses (e.g., a function that writes to disk but has no cleanup on failure, or a bus message published before the DB transaction commits).
Checkpoint: You can draw the flow: trigger → processing → side effects → output, and you know what happens on the error path at each stage.
For each file, run this checklist. These items apply to every review regardless of reviewer specialization.
Security (OWASP-aligned, always check first):
Correctness:
if value: must not lose 0, False, [], {}; use
if value is not None: or isinstance() checks when the value domain includes falsiesPath.unlink(missing_ok=True) only suppresses
file-not-found, NOT parent-dir-deleted — check parent.is_dir() firstSilent Failure Hunting (MANDATORY — this catches the bugs that pass all other checks):
For EVERY error handling path in the diff — try/catch, except, if err, .catch(),
return codes, exit codes — ask: "What happens when this fails? Does the user know?
Does the developer have breadcrumbs to debug it?"
Quick scan for these high-signal antipatterns by domain (section 4B error-path-analysis
has full details, fix guidance, and additional items beyond this summary).
Skip language sections not present in the PR diff — e.g., if the PR is Python-only,
skip the Go/Rust/Java/JS sections:
Any language:
catch (_) / except Exception with no logging (High/Medium)Web/Frontend (JS/TS):
res.ok ? data : [] — server errors look like empty data (Critical)response.json().catch(() => response.text()) — body stream consumed twice (High)Python/Backend:
except: — swallows everything including KeyboardInterrupt/SystemExit (Critical)except Exception: pass — swallows all application errors silently (High)subprocess.run() without check=True and no manual returncode check (High)requests.get()/httpx.get() without timeout= — hangs forever on unresponsive server (High)session.close() (High)async for / async with resource not cleaned up on CancelledError (High)CLI/Infrastructure:
os.system() or subprocess return value unchecked (High)finally or context manager (High)Go:
result, _ := doSomething() — error return discarded (High)go func() without context/cancellation — goroutine leak (High)defer f.Close() before nil check on f — panic on error (High)Rust:
.unwrap() in non-test library code — panic with no recovery (High)let _ = fallible_fn() — #[must_use] Result silently discarded (High)Java/Kotlin:
catch (Exception e) { /* ignore */ } — especially swallowed InterruptedException (High)Database/Data:
Configuration/YAML/JSON:
yes/no/on/off parsed as boolean instead of string (Medium)Note: These apply to catches on reachable error paths (I/O, network, disk, DB). If a catch guards pure logic with provably constrained inputs, it may be unnecessary — see the Anti-Over-Engineering Check (Section 3) resolution rule.
PR Description Verification (MANDATORY):
Performance:
subprocess.Popen, open(), mkdir(), and
file locks in async def or FastAPI routes MUST use asyncio.to_thread()time.sleep() > 1 second should use time.monotonic() loop for interruptibilityTesting:
AI-Generated Code Patterns (LLM-written code has predictable failure modes):
Code generated by LLMs (including this project's agents) fails in specific, predictable ways. Check explicitly for these:
Note: Unnecessary error handling for impossible cases is covered under Anti-Over-Engineering Check (Section 3). Do not duplicate findings here.
Observability (can you debug this in production?):
console.log or print() debugging left in production code pathsFramework Compliance (MANDATORY for this project):
.os/ files were modified, were src/os/templates/ updated FIRST
and synced? Direct .os/ edits without template changes are a Critical issue.claude_wrapper.py was changed, does agent_runner.py
need the same change (or vice versa)? If only one path was changed, verify the PR
description explains WHY the other path is unaffected..os/agents/*/memory/ files from the PR AUTHOR should NOT
be in the PR diff — these are local agent state, not project code. Flag as Medium if present.
Exception: reviewer's own memory commits intentionally added to the branch are expected.Documentation:
Before starting this section, check your review_focus from identity.yaml.
Apply ONLY the dimension checklists that match your focus tags. Skip the rest.
If you do not have a review_focus list, apply ALL dimension checklists below.
When submitting your review via pr_comment, you MUST include a review_checklist
parameter — a JSON object where each key is one of your review_focus tags and
each value is "checked", "not_applicable", or "skipped_with_reason:...".
The tool will reject your review if any focus area is missing.
async-correctnesssubprocess.Popen, open(), os.mkdir(),
file locks in async def MUST use asyncio.to_thread()asyncio.shield() used where needed, cleanup runs on cancelasync def but never await are misleadingasyncio.Lock held across await points minimizedtime.sleep() never used in async code — use asyncio.sleep() insteadplatform-safetyos.killpg(pid, sig) MUST verify os.getpgid(pid) == pid
first — killing a non-leader's group kills the parent process (server)Path.replace() and os.rename() fail with PermissionError when
destination is open — provide fallback to direct writefh.close() can raise PermissionError under contention —
wrap cleanup in try/exceptPath objects, not string concatenation with /shlex.quote() for Unix, avoid shell=True on both platformsresource-lifecyclesubprocess.Popen has a corresponding kill/wait path on erroropen() uses with or has explicit close() in finallyasyncio.create_task() is awaited or tracked for cancellationtempfile.mkstemp or NamedTemporaryFile has cleanup in finallyconcurrency-safetyINSERT...SELECT for DB claims, os.rename for file swapsstate-machine-integrityidle→working→done)data-integrityif value: doesn't silently lose 0, False, "", [], {}str(None) producing "None" strings in datadefensive-coding"unknown", "none") where None or enum should be usedcross-module-impactgrep all callers — do they still work?unbounded-growtherror-path-analysisThis is the authoritative checklist for error-path bugs. Section 4A's Silent Failure Hunting is a quick-scan summary — this section has the full details and fix guidance. Trace each error path through callers, not just the immediate diff. Read only the "Any language" section plus sections matching the PR's languages. Skip language sections not present in the diff to conserve context window.
Any language / cross-cutting:
.catch(() => genericMsg), catch(e) { showGeneric() },
except Exception: return default where the error is never logged or surfaced. Every
catch must log the error and show specifics. Flag as High.catch (_) {}, catch {}, except Exception: with the
error deliberately unnamed/unused. Flag as High if the catch body does not clearly
indicate intentional handling. Flag as Medium if the catch body handles the case
correctly but does not log for debugging. Fix: name it, log it.None — API handler returns 200 with empty data, never
knowing the DB is down. Flag as High. Check whether the caller distinguishes
"no data" from "error fetching data." If the caller handles the None/default as an
error condition, the local catch may be intentional — only flag when the error is truly
swallowed and the caller cannot distinguish failure from empty.
Fix: let the error propagate; handle where the user gets actionable feedback.obj?.prop?.method() silently produces undefined
when obj is unexpectedly null. In Python: getattr(obj, 'prop', None). If the value
should always exist, optional chaining masks the bug. Flag as High if the value
is required by downstream logic (silent undefined propagates). Flag as Medium if
the value is sometimes-absent by design.
Fix: explicit null check with error for required values; ?. only for genuinely optional.Web/Frontend (JS/TS):
res.ok ? data : [] or res.ok ? data : {} makes server
errors (500, 502, 404) indistinguishable from "no data." User sees empty list, assumes
nothing exists. Every fetch must have a distinct error state with HTTP status shown.
Flag as Critical.response.json().catch(() => response.text()).
In browser Fetch, calling .json() consumes the ReadableStream body. If .json()
rejects, a subsequent .text() resolves to empty string because the stream is drained.
(Node.js undici may differ.) Flag as High.
Fix: const text = await response.text(); const data = JSON.parse(text);try { data = await res.json() } catch { data = {} } —
parse failure on a success response silently produces empty data. Downstream code gets
undefined for expected fields. At minimum: console.warn. Flag as Medium.finally.Python/Backend:
except: catches everything including BaseException
subclasses (KeyboardInterrupt, SystemExit, CancelledError in Python 3.9+) — the
process cannot be interrupted. Flag as Critical.
except Exception: pass does NOT catch KeyboardInterrupt/SystemExit but still
silently swallows all application errors. Flag as High.
Fix: catch specific exceptions, or at minimum except Exception as e: logger.error(e).subprocess.run() or os.system() without
check=True and no manual returncode check. The command fails, the code continues
as if it succeeded. Flag as High.
Fix: use subprocess.run(..., check=True) or check result.returncode != 0.requests.get(url) or httpx.get(url) without
timeout= parameter. Hangs forever if the server is unresponsive. In async code,
asyncio.wait_for() or httpx.AsyncClient(timeout=...). Flag as High.
Fix: always set explicit timeout. Default to 30s for API calls, 5s for health checks.session.add() / session.execute()
but session.commit() missing or only in the happy path. On exception, session.close()
auto-rolls-back and changes are silently lost. Flag as High.
Fix: use with session.begin(): context manager or explicit commit in try + rollback
in except.async with or async for resource cleanup skipped
when asyncio.CancelledError propagates. The resource (file handle, DB connection,
subprocess) leaks. Flag as High.
Fix: use try/finally or ensure __aexit__ handles cancellation.logging.exception() outside except block: Calling logging.exception() outside
an except block produces "NoneType" in the traceback. Flag as Medium.
Fix: use logging.error() with explicit exception, or only call .exception() in except.CLI/Infrastructure:
os.system(cmd) return value unchecked, or subprocess.call()
result discarded. Script reports success when the underlying command failed.
Flag as High. Fix: check return code, propagate non-zero as error.atexit AND signal handlers, or use context managers.fcntl.flock() or msvcrt.locking()
without try/finally or context manager. Process crash = permanent lock.
Flag as High. Fix: use with statement or finally block. Consider lease-based
locks with expiry for distributed systems.Database/Data:
ALTER TABLE DROP COLUMN or
ALTER TABLE RENAME COLUMN without a data migration step. Existing data is silently
lost or inaccessible. Flag as Critical.
Fix: add data migration, or use add-new → migrate-data → drop-old pattern.IF EXISTS patterns.UPDATE ... WHERE version = expected.Go:
result, _ := doSomething() or doSomething() without
capturing the error return. The function fails, the code continues with zero-value result.
Flag as High when the function performs I/O or has runtime-dependent failure modes.
If the error is provably impossible for the given inputs (e.g., strconv.Atoi on a
compile-time constant), the discard may be intentional — verify before flagging.
Fix: result, err := doSomething(); if err != nil { return err }.go func() launched without cancellation path — if the goroutine
blocks on a channel or I/O, it never exits. Flag as High.
Fix: pass context.Context, select on ctx.Done().f, err := os.Open(...); defer f.Close() before
checking err. If err != nil, f is nil and defer f.Close() panics.
Flag as High. Fix: defer after the nil check.Rust:
.unwrap() or .expect() in non-test, non-main code.
Panics crash the caller with no recovery. Flag as High when the Result/Option
depends on runtime input. Unwrapping a value just validated by a match/if-let or
produced by an infallible path is idiomatic — verify before flagging.
Fix: return Result<T, E> and use ? operator.let _ = fallible_fn(); — the #[must_use] warning is suppressed
and the error is silently discarded. Flag as High if the function has side effects..collect::<Vec<_>>() on an iterator of unknown size — OOM on
large inputs. Flag as Medium. Fix: use bounded buffer or streaming.Java/Kotlin:
catch (Exception e) { /* ignore */ } — especially
InterruptedException which clears the interrupt flag silently. Flag as High.
Fix: re-throw, or at minimum Thread.currentThread().interrupt() for InterruptedException.InputStream, Connection, PreparedStatement opened without
try-with-resources. Leaked on exception. Flag as High.
Fix: use try-with-resources (try (var rs = ...)) or Kotlin's .use {}.@Nullable without
null check. Compiles fine, NPE at runtime. Flag as High.Configuration/YAML/JSON:
yes/no/on/off parsed as boolean, not string.
port: 8080 parsed as int, port: 08080 parsed as octal (some parsers). Norway
country code NO parsed as false. Flag as Medium.
Fix: quote strings in YAML, use explicit types, validate after parse.os.environ['KEY'] or env::var('KEY').unwrap() without
fallback — crashes on missing env. But os.getenv('KEY', '') silently uses empty string.
Flag as Medium. Fix: validate env vars at startup, fail with descriptive error.test-depthtime.sleep(10) — use short timeouts or mockstype-designWhen a PR introduces or modifies types (classes, dataclasses, Pydantic models, TypedDict,
interfaces, structs). If you flag a construction validation issue here, do not also flag
it under error-path-analysis or insecure-defaults — one finding per root cause.
Task, Agent, Decision).
Fix: move validation and state-change logic into methods on the type.@property with defensive copying.__init__ / constructor. Make illegal states unrepresentable.__init__ or factory accepts raw user input
without validating constraints. Invalid instances circulate through the system.
Flag as High. Fix: validate at construction, raise on invalid input.adversary-modelingFor security-relevant code (auth, API endpoints, config, user input, permissions), consider three adversary types (adapted from Trail of Bits' sharp-edges methodology):
The Scoundrel (malicious actor — attacker controlling input or config):
verify_ssl: false, auth: off)The Lazy Developer (copy-pastes examples, skips docs, deadline pressure):
The Confused Developer (misunderstands the API contract):
(key, nonce) vs (nonce, key))0, "", null, or [] for a security parameter disable the check?Flag findings with the adversary type: "High [Scoundrel]: attacker can set
timeout=0 to bypass rate limiting."
insecure-defaultsAdapted from Trail of Bits' insecure-defaults skill. Check for fail-open patterns:
env.get('SECRET') or 'default-key' — app runs with weak
secret if env var missing. Flag as Critical if it reaches auth/crypto paths.if not config.get('auth_required'): pass — missing config
disables security instead of enforcing it. Secure default = deny.timeout=0, max_retries=0,
rate_limit=0? Does 0 mean "infinite" or "disabled" or "immediate"?debug=True, skip_validation=True,
allow_all_origins=True — if the default is the insecure value, flag as High.test/, __tests__/, or .example files. Flag as Critical.Access-Control-Allow-Origin: * combined
with credentials: true. Flag as Critical.Every finding you report MUST include a confidence score. This prevents false positives from wasting the developer's time and ensures Critical findings are backed by evidence, not pattern-matching hunches.
Scale (0–100):
| Score | Meaning | Evidence Required |
|---|---|---|
| 90–100 | Certain — you traced the data flow and confirmed the bug | Full code path traced, specific trigger described |
| 70–89 | High confidence — code clearly has the issue but you haven't traced every caller | Issue visible in diff, checked immediate context |
| 50–69 | Moderate — pattern matches a known antipattern but context might make it safe | Pattern identified, but mitigating code may exist elsewhere |
| 30–49 | Low — suspicious but could be intentional or guarded upstream | Flagging for developer to confirm, not asserting a bug |
| 0–29 | Speculative — do NOT report these | Suppress — not enough evidence |
Filtering rules:
**Critical [confidence: 55, unverified]**: ... — needs developer confirmation.
A false positive on a Critical issue wastes 5 minutes of developer time. A false
negative on a Critical issue ships a security vulnerability. The cost is asymmetric.Include the score in each inline comment: **High [confidence: 85]**: ...
Before reporting any Critical or High finding, verify it is a true positive —
not just pattern matching. For findings reported via the 4C escape hatch ([unverified]),
complete as many verification steps as possible and document which steps you could not
complete. The [unverified] tag signals that full verification was not achievable.
For each Critical/High finding, answer these questions:
file.py:42, when X happens, Y occurs
because Z." If you can't restate it clearly, it's probably not real.Rationalizations to reject (from Trail of Bits):
| Rationalization | Why It's Wrong |
|---|---|
| "This pattern looks dangerous, so it's a vulnerability" | Pattern recognition is not analysis. Complete data flow tracing first. |
| "Similar code was vulnerable elsewhere" | Each context has different validation, callers, and protections. |
| "This is clearly critical" | LLMs are biased toward seeing bugs and overrating severity. Prove it. |
| "Skipping verification for efficiency" | Unverified findings waste more time than thorough analysis. |
If a finding survives verification, include the evidence in your comment:
"Critical [confidence: 92]: deleteTask at line 47 — when server returns 500,
response.json() consumes the body stream. The subsequent .text() fallback returns
empty string. Traced: no upstream guard exists. Trigger: any 500 from /tasks/{id}."
This is what separates a good review from a superficial one. Do NOT just review the diff in isolation. Assess system-wide impact:
Dependency tracing (concrete techniques):
grep -rn "function_name" --include="*.py" or use query_repo_contextIntegration points:
grep -rn "bus_publish\|publishes\|listens_to" --include="*.py" --include="*.yaml"grep -rn "config_key_name" --include="*.py" --include="*.yaml"Backward compatibility:
Cross-PR conflicts:
Checkpoint: For each changed file, you can explain what OTHER code in the repository is affected by this change and why it is safe.
While reviewing, watch for code that should be removed. LLM-generated PRs frequently add new code paths without cleaning up the old ones they replace.
grep to verify zero callers.fetchData() and fetchDataV2() exist, but only V2 is called).X and its code path."catch(() => []) fallback is left in place. The fallback now masks the
new error handling. Flag as Medium.Report removal candidates in the review summary section, not as blocking issues
(unless the dead code creates confusion or maintenance burden). Use the format:
"Removal candidate: functionName in file.py — zero callers after this PR."
You MUST post inline comments on specific lines where issues exist. A single consolidated summary comment is NOT sufficient. Each issue MUST be tied to the exact file and line where it occurs. Only post a summary comment AFTER all inline comments are submitted.
Every file in the diff must be EXAMINED. For files with no issues, note them in the summary section — do not post noise comments just to fill a checkbox.
Use the GitHub reviews API to submit a single review with inline comments:
# Get HEAD commit SHA and repo info
HEAD_SHA=$(gh api repos/{owner}/{repo}/pulls/{number} --jq '.head.sha')
Build a review JSON payload with a comments array. Each comment needs:
path: file path from the diffposition: line number within the diff hunk (not the file line number)body: review comment with severity label and analysis# Submit review with inline comments in a single API call
gh api repos/{owner}/{repo}/pulls/{number}/reviews \
--method POST \
-f event="COMMENT" \
-f body="Summary of review" \
--input payload.json
Where payload.json contains:
{
"event": "COMMENT",
"body": "## Review Summary\n...",
"comments": [
{"path": "src/file.py", "position": 42, "body": "**High**: Issue description..."}
]
}
For each inline comment include:
VERIFICATION GATE: Before proceeding to Step 7, confirm:
gh api (not just gh pr review)Use the pr_comment tool to submit the final verdict:
event="request-changes" with summaryevent="comment" with summaryevent="approve" with summary of what was checkedMANDATORY: If your identity.yaml defines review_focus, you MUST include a
review_checklist parameter when calling pr_comment. This is a JSON object mapping
each of your focus tags to its status:
{
"review_checklist": {
"async-correctness": "checked",
"platform-safety": "checked",
"defensive-coding": "not_applicable",
"data-integrity": "checked",
"cross-module-impact": "skipped_with_reason: no cross-module changes in this PR"
}
}
The tool will reject your review if any focus area is missing from the checklist.
This is the most commonly skipped step. If you skip it, the developer is never notified and the task stays in review limbo.
COORDINATOR="${AC_COORDINATOR_URL:-http://localhost:9889}"
TASK_ID="{task_id}"
AGENT_NAME="{your-agent-name}"
# If approving:
curl -s -X POST "${COORDINATOR}/tasks/${TASK_ID}/status?new_status=approved&agent_id=${AGENT_NAME}"
# If requesting changes:
curl -s -X POST "${COORDINATOR}/tasks/${TASK_ID}/status?new_status=changes_requested&agent_id=${AGENT_NAME}"
Publish to bus (use the bus_publish MCP tool):
If approving:
{
"channel": "work",
"type": "review_approved",
"message": {
"pr_number": "{pr_number}",
"task_id": "{task_id}",
"verdict": "approved",
"summary": "{brief summary of review}",
"reviewer": "{your-agent-name}"
}
}
If requesting changes:
{
"channel": "work",
"type": "review_completed",
"message": {
"pr_number": "{pr_number}",
"task_id": "{task_id}",
"verdict": "changes_requested",
"summary": "{brief summary of issues}",
"reviewer": "{your-agent-name}"
}
}
Update repo intelligence:
After updating your memory files, commit them to the reviewed PR's branch so they ship with that PR merge. NEVER create a separate PR for reviewer memory updates. Separate memory PRs conflict with main and create noise.
# Get the reviewed PR's branch name
BRANCH=$(gh pr view {pr_number} --json headRefName --jq '.headRefName')
# Checkout the branch, commit memory, push
git fetch origin
git checkout "$BRANCH"
git add .os/agents/reviewer/memory/
git commit -m "chore(reviewer): update memory with PR #{pr_number} review notes"
git push origin "$BRANCH"
# Return to your working branch
git checkout main
If the PR is already merged (branch deleted): create a short-lived feature branch, commit memory there, and push. The coordinator will handle it. Do NOT push directly to main.
git checkout main && git pull
git checkout -b chore/reviewer-memory-pr{pr_number}
git add .os/agents/reviewer/memory/
git commit -m "chore(reviewer): update memory with PR #{pr_number} review notes"
git push origin chore/reviewer-memory-pr{pr_number}
Checkpoint: Memory is committed. No new PR was created for the memory update.
Checkpoint: Task status is
approvedorchanges_requested. Areview_approvedorreview_completedmessage exists on the work channel.
## PR Review: #[N] — [Title]
### Verdict: [Approved / Changes Requested]
### Design Assessment
[Is the overall approach sound? One paragraph.]
### End-to-End Impact
[What other code/systems are affected by this change? Any backward compat concerns?]
### Statistics
- Critical: [count]
- High: [count]
- Medium: [count]
- Low: [count]
- Total comments: [count]
### Critical Issues
1. [file:line] — [description] — [impact on callers/system]
### High Priority
1. [file:line] — [description] — [impact on callers/system]
### Summary
- Design: [sound / N concerns]
- Security: [pass / N issues]
- Correctness: [pass / N issues]
- Performance: [pass / N issues]
- Testing: [pass / N issues]
- Backward Compatibility: [safe / N concerns]
### Recommendation
[Approve / Request changes — specific actions needed]
Before posting your review, verify you completed every review dimension:
SELF-AUDIT (Universal):
- [ ] Functional flow: I traced trigger → processing → side effects → output → error path
- [ ] Design: I assessed the overall approach, not just code details
- [ ] Security: I ran the full OWASP-aligned checklist (11 items)
- [ ] Impact: I traced callers/consumers for every changed function/API
- [ ] Backward compat: I checked if existing consumers break
- [ ] Cross-PR: I checked for conflicting open PRs
- [ ] Framework: Templates updated before .os/? Both execution paths checked?
- [ ] No DEVELOPER memory files (.os/agents/*/memory/) leaked into the PR? (Reviewer's own memory commits are intentional — not a leak.)
- [ ] Lifecycle: I will update task status AND publish bus notification
SELF-AUDIT (Review Profile — one per review_focus tag):
- [ ] Each review_focus tag from my identity.yaml has been checked
- [ ] review_checklist JSON object is complete with all focus tags
- [ ] No focus tag left unaddressed (use "not_applicable" if genuinely N/A)
If any box is unchecked, go back and complete it before submitting.
Reviewing only the diff, not the context — Consequence: miss regressions in callers, break integration points, approve changes that look correct in isolation but fail system-wide. Fix: Read the full function/class around each change. Search for callers with grep. Check API consumers.
Skipping design review — Consequence: approve a fundamentally wrong approach that passes all code-level checks. Harder to fix later. Fix: Always start with Section 3 (Design Review). Ask: "Is this the right approach?"
Superficial review — Consequence: bugs and security issues reach production. Fix: Go deep on every file. Check logic, security, edge cases.
Severity inflation — Consequence: developers learn to ignore your reviews. Fix: Reserve Critical for security vulns and data loss. Style nits are "Nit:" prefix.
No fix suggestions — Consequence: developer doesn't know how to fix the issue. Fix: Always include a specific code suggestion or approach.
Skipping security checklist — Consequence: vulnerabilities sneak through. Fix: Run the full 11-item security checklist for every PR, even small ones.
Not checking backward compatibility — Consequence: deployed change breaks existing API consumers, config files, or dependent agents. Fix: For every changed interface, ask "who calls this?" and "will they break?"
Not loading repo context — Consequence: miss context-specific issues.
Fix: Use query_repo_context before starting the review.
Wrong bus type on completion — Consequence: developer handles review_completed (changes requested) differently from review_approved. Wrong type = wrong developer action.
Fix: Use review_approved for approvals, review_completed for changes requested.
Not updating task status — Consequence: task stays in review forever. Developer never gets notified. Task lifecycle is broken.
Fix: Always call POST /tasks/{task_id}/status with approved or changes_requested.
Creating a separate PR for reviewer memory updates — Consequence: separate chore(reviewer): update memory PRs conflict with main and accumulate, requiring manual cleanup.
Fix: Commit memory to the reviewed PR's branch BEFORE submitting the verdict (see Section 8 Memory Commit step). If the branch is already merged, create a short-lived chore/reviewer-memory-pr{N} branch and push — never push directly to main.
A PR review is complete when ALL of these are true:
approved or changes_requested)Review pull requests for code quality, security, and correctness
End-to-end shipping workflow — tests, PR creation, review request. Unified pr-create + pr-complete.
Periodic cross-agent learning consolidation — review what worked, what didn't, propagate insights
Break down complex problems through structured analytical frameworks
Persist and recall context across sessions — critical for continuity
Session lifecycle — checkpoint, restore, handoff, sleep/wake