cxc-dev-code-reviewer

Dev-Code-Reviewer — Code Review Guide

C0/C1 work (small local patches): See dev §0.0 Work Classifier + §0.1 Patch Fast-Path before reading references.

dev is canonical: dev §0.2 Rule Classes, §3 Verification Gate, and §5 Safety Rules apply to all work governed by this skill. Read the dev skill first for project-wide conventions before applying review rules.

Systematic code review patterns for finding real issues, not bikeshedding. This skill activates by change-surface for review requests, pre-merge checks, or independent audit passes.

Review Posture (REVIEW-POSTURE-01)

Review as a skeptical, independent outsider. Executor claims, passing tests, AI summaries, and user-facing "done" prose are untrusted until you confirm them yourself — assume the work may have failed and look for the regression or false-confidence test that proves it. Inspect artifacts before believing them; a green run you did not read is not evidence.

Modular References

dev-testing owns test adequacy and QA execution. dev-debugging owns RCA when review discovers a runtime failure. dev-architecture owns coupling and boundary placement.

External/current review evidence

For dependency CVEs, release-note claims, package maintainer/source checks, provider behavior, or other current/public evidence used in a review, read the active search skill and follow its query-rewrite, source-fetch, and evidence-status rules. Browser fetch/open/text/get-dom/snapshot is downstream verification after candidate URLs exist, not a raw-query search substitute.

1. Code Review Process

Automated Pre-Scan

Run repo-native lint, type checks, and tests first.

1. Errors block review readiness.
2. Warnings are non-blocking but must be reported.
3. Tool findings appear before manual findings.
4. Unavailable tools are skipped with the gap stated; absence is non-blocking.

Routing: linters catch style, imports, and simple bugs; type checkers catch type and null-safety errors; SAST catches common injection/auth patterns; dependency audits catch known CVEs. Errors block, warnings inform. Manual review remains responsible for architecture, correctness, business intent, and cross-file impact.

Review Order (by impact, not preference)

1. Architecture — Does the approach make sense? Right layer? Right abstraction? Is this the right place for this code?
2. Correctness — Logic errors, edge cases, off-by-one, null/undefined handling, error paths
3. Security — Input validation, injection risks, auth checks, secrets exposure
4. Performance — N+1 queries, unbounded collections, missing indexes, unnecessary computation
5. Maintainability — Naming, structure, complexity, test coverage, documentation
6. Style — Last priority. Don't bikeshed formatting when there are real issues.

Delegation: coupling classification belongs to dev-architecture §3; boundary and validation-location findings belong to dev-architecture §4.

Review Mindset

• Be specific. "This could fail" → "This throws if user is null on line 42"
• Suggest, don't demand. Unless it's a security or correctness issue.
• Explain why. Not just "change X to Y" but "X causes N+1 queries because..."
• Acknowledge good work. If a complex problem is solved elegantly, say so briefly.

Output Contract (REVIEW-OUTPUT-01)

Tool findings go first (Automated Pre-Scan item 3); then manual findings sorted Critical > High > Medium > Low > Style; then a dedicated blocking_issues block; verdict last. For dispatched plan-audit (PABCD A-gate) reviews the verdict is additionally machine-scannable: end the reply with a final line VERDICT: PASS, VERDICT: GO-WITH-FIXES (blockers=N), or VERDICT: FAIL (mapping: Approve -> PASS; Approve-with-suggestions -> GO-WITH-FIXES; Request-changes / Block -> FAIL). The dispatching agent's exit rule is AUDIT-LOOP-01 (cxc-pabcd §A): FAIL always triggers another round. Every finding carries a concrete trigger, impact, and path:line (FAMILY-CITE-01) — no finding on a hunch. Do not file pre-existing debt unless the patch worsened it. When a change introduces a value/type/message crossing a module boundary, trace the consumer side before declaring it correct, rather than reviewing the emitting hunk alone.

Compact finding example:

severity: High
title: Missing ownership check permits cross-account access
location: src/routes/accounts.ts:42
trigger: Authenticated caller supplies another account ID
impact: Caller can read another user's account data
evidence: Handler loads by ID without constraining owner_id
remediation: Scope the query to the authenticated owner
verification: verified

Regression & false-confidence tests (REVIEW-REGRESS-01)

Run a dedicated pass: what previously-working behavior can now break, and do the tests cover that surface? Flag deletion-only "fixes", tautological tests, tests that merely mirror the implementation, and scope-drift abstractions added beyond the request.

Pre-Review Checklist

• Build passes.
• Tests pass.
• The change explains what changed and why.
• The diff is small and structured enough to review.

2. Quality Thresholds

Severity Definitions

Flag these during review:

File Size Guidance

Canonical rule imported from dev-architecture §1: >400 LOC -> split (DEFAULT).

Review Verdict

Deterministic blocker semantics (REVIEW-BLOCK-01): any unresolved Critical or High blocks the merge. Medium findings should be fixed before merge. When explicitly marked non-blocking by the author with a stated reason, Medium may pass with a tracked follow-up. Style never affects the verdict.

3. Common Antipatterns

Structural

Dead Code

Dead code is a maintenance tax — remove rather than comment out.

Logic

Security

This section owns the mandatory review pre-scan; dev-security owns security policy and deep analysis. Use this checklist for hardcoded secrets, injection, validation, auth, authorization, and logging findings.

3.5 Security Review Quick-Check

For every review, scan these OWASP-aligned red flags.

Must-Check Every PR

Conditional Checks

Load dev-security for deep analysis. Security routing is required when the diff touches auth, credentials, untrusted input, sensitive data, dependencies, agent tools, or trust boundaries.

3.6 Performance Review Quick-Check

Scan every PR for these common performance pitfalls:

Database & API

Frontend-Specific

General

4. Receiving Code Review

Read all feedback, restate the technical requirement, and verify it against the codebase before accepting it. Evaluate stack fit, existing architecture, tests, and actual usage rather than treating reviewer claims as authority.

Clarify ambiguous items first, then handle blockers, simple fixes, and complex changes in that order. Implement one item at a time and test each change.

Push back with concrete tests, code, or documented decisions when advice breaks behavior, lacks context, violates YAGNI, is technically incorrect, or conflicts with established architecture. Respond with the verified result and avoid performative agreement.

5. Requesting Code Review

Request review before merging, after major features, and before large refactors; also request it for complex fixes or when the approach is uncertain. Small, non-impactful config/docs changes may skip review.

A review request must include passing build/tests, the base-to-head diff range, a concise change and behavior summary, and requested focus areas. Keep diffs under 500 changed lines or split them into reviewable units.

Fix Critical findings immediately and re-request review; fix High before other work and Medium before merge. Low and Style findings follow impact and team conventions.

Reviewing AI-Generated Code

Run this in addition to the normal review when the diff is substantially AI-generated:

6. Subagent Review Mode

Parallelize review only when domain breadth exceeds one reviewer's context (e.g., frontend + backend + infra in a single diff, or when the diff spans too many unrelated domains for a single pass). Each subagent receives its file subset, the review process from sections 1-5, and outputs structured findings. The main agent deduplicates, normalizes severity, and presents a unified review.

AI Tool Integration Awareness

Read references/ai-assisted-review.md for tool coordination, AI-generated-code checks, re-review policy, and agentic security triggers. Focus manual review on architecture, intent, and cross-system impact. STRICT (REVIEW-AI-EVIDENCE-01): de-duplicate, reproduce, and severity-normalize AI findings; they are evidence to inspect, not authority.

AI Slop Cleanup Checklist (REVIEW-SLOP-01)

Activate for explicit slop cleanup or >=3 slop findings. Lock behavior with green tests before deletion.

REVIEW-GUARD-REMOVAL-01 (DEFAULT). Deleting input validation or error handling at a trust boundary requires a regression test that actually EXERCISES the deleted path. For an input-validation guard that means malformed/hostile input; for an error handler it means injecting the fault that reaches it — network timeout, connection reset, filesystem I/O failure, subprocess failure. Attaching an unrelated input test to satisfy the form does not meet this bar. Without it the deletion is a High blocker: row 2 (Over-defense) above is not by itself grounds for calling a boundary guard unnecessary. Trust boundaries are where external input first lands: hook stdin, CLI arguments, file parsing, network responses, subagent output.

REVIEW-REMOVED-BACKEND-01 (DEFAULT). A change touching search/SKILL.md gets checked for removed search backends creeping back in as if they were available: progrok, web-AI, Grok Expert, GPT Pro, Exa, Tavily, Perplexity, Brave. These names may appear only in non-goal or historical framing.

There is no automated check, and there cannot be a useful one: codexclaw owns no registry of available backends to compare the prose against — web_search is host-provided. The scan that used to guard this read one prose file for phrase existence, broke on rewording, and proved nothing, so it was deleted with the protection routed here on purpose (plugins/codexclaw/test/manifest-policy.test.mjs, the TEST-PROMPT-SEAM-01 comment). This is a reviewer's read, not a contract. If a backend registry ever lands in code, revisit whether a real two-source check is possible.

Judge by deletion kind. A replacing/relocating deletion (the check moved elsewhere) must stay GREEN after the old guard is removed, and go RED only when the surviving boundary check is removed too. For a non-replacing deletion, a regression that goes RED proves the guard is load-bearing — do not approve the deletion.

Test adequacy for prose-wording changes follows dev-testing's TEST-PROMPT-SEAM-01; that skill owns test adequacy (see §"Scope" above) and this rule does not restate it.

Changed-File Coverage Ledger (REVIEW-COVERAGE-01, DEFAULT)

Account for every changed file as reviewed, skipped (reason), or out-of-scope (reason) before verdict. Generated, lock, vendored, binary, and outside-domain files may be skipped only with an explicit reason. Any unaccounted file makes the verdict incomplete.

Finding Falsification (REVIEW-FALSIFY-01, DEFAULT)

Before reporting a finding:

1. State it as a testable claim.
2. Search tests, guards, caller context, and docs for contradictory evidence.
3. Downgrade or retract the claim when contradictory evidence disproves it.
4. Retain the claim when it survives the falsification attempt.

Every finding includes verification: verified|unverified. Use unverified when the falsification attempt could not be completed or evidence is incomplete.

Interdiff Re-Review (REVIEW-INTERDIFF-01, DEFAULT)

Anchor the re-review to both the previous reviewed commit/range and the new head; record those anchors in the review. Review only the interdiff, preserve unresolved findings, verify each claimed fix, and process new findings normally. Revisit unchanged code when a cross-file dependency changed. If either anchor is missing, history was rewritten ambiguously, or the interdiff cannot be trusted, fall back to a full review of the current base-to-head diff.

REVIEW-WORKTREE-01 (DEFAULT). Never check out another review ref in the worktree you were handed. If the review target is already checked out in the worktree the dispatcher assigned, review it there; otherwise create or attach a dedicated named worktree and run the checkout, tests and QA in that one. Record pwd -P and the target HEAD alongside the anchors above.

The condition is observable state, not ownership: a reviewer — often a subagent — cannot know which branch the parent session "owns", but it can always see what is checked out where it stands.