Jcdendrite Claude Config Code Review

Review the code that was just written or modified. Act as a principal engineer reviewing a junior engineer's work. Be thorough but not pedantic.

Step 0 — Detect changed domains

Before reviewing, determine which files were changed (from context, git diff, or the conversation). Classify each changed file into one or more domains:

• Infrastructure: .github/, *.tf, Dockerfile, docker-compose*, CI/CD configs
• Data infrastructure: **/migrations/**, *.sql (outside warehouse-model paths), schema definitions, CDC / change-stream config, ETL/ELT pipeline code, warehouse ingestion connector configs
• Analytics modeling: models/**/*.sql, models/**/*.yml, dbt_project.yml, macros/**, tests/** (dbt-style), seeds/**, scheduled-query / view definitions in warehouse-config repos, semantic-layer files
• Frontend: *.tsx, *.jsx, *.css, src/components/**, src/pages/**
• Backend: edge functions, API routes, server-side utilities, *.go, *.py, NoSQL access-pattern code, schema design files (Prisma, ORM models, NoSQL ODM models)
• Claude Code config: .claude/**
• Lovable config: .lovable/**

Apply the Base checklist always. Apply each Domain checklist only when at least one changed file matches that domain.

Step 0.5 — Load project-specific layer

If a project-specific layer exists for this skill, load it now. Glob for .claude/skills/code-review-*/SKILL.md from the repo root (resolved via git rev-parse --show-toplevel); if exactly one matches, read it with the Read tool and merge its checklist into the items below. If multiple match, list them and stop — that's a config error in the project, not something this review resolves. If none match, proceed without a layer.

Step 0.6 — Pre-judgment table check

Before forming any ripple judgment in Step 1 or the checklist, enumerate every row in the Change-type table (under Ripple effect triage) that matches the diff. Hold the list in working memory through the rest of the review — each row is an anchor the spawn decision must address. This step exists because the table is the institutional memory; the parent's first-pass judgment is not. Form judgment against the table, not in lieu of it. For compound diffs that match multiple rows, narrow the diff scope per spawn — pass each specialist only the file(s) within their lane, not the full diff, so their findings stay in-scope.

Step 1 — Implementation-fitness gate

Before reviewing for gaps, answer: is the implementation appropriately sized for what the change needed to accomplish? Gap-finding on an over-elaborate change elaborates it further, and the checklist won't surface "the whole implementation is the wrong shape."

Markers of over-elaboration:

• Captured outputs / artifacts / fields with no reader downstream.
• Layers that duplicate a higher-level abstraction already running the same work.
• Defensive paths against attack scenarios that haven't been observed.
• Granularity exceeding the actual consumer's need.
• Conditional logic for future phases that may not arrive.

If over-elaborated: stop. Surface the simpler implementation as the primary review output before any checklist item. Otherwise proceed to the checklist.

Question implementation choices, not feature scope. Feature scope is fixed by the ticket; implementation choice is reviewer-tunable.

Base checklist

Evaluate the code against each item. Only flag items where there is a concrete issue — do not flag items just to show you checked them.

Read every changed file fully, including generated ones. Generated files (Supabase types, OpenAPI clients, GraphQL codegen) need the same scrutiny — runners like Vitest+esbuild strip types without validating, so npm warnings or build noise can leak into the file head undetected. Check head -5 of generated files even when tests are green.

Correctness

1. API misuse — Are libraries, frameworks, and language APIs used as designed? Flag any reliance on accidental or undocumented behavior (passing invalid arguments that happen to work, using internal methods, relying on side effects of unrelated calls).

2. Error handling changes — Are there catch blocks, fallback defaults, or error handlers that hide failures the caller would want to know about? Empty catches, catch-and-return-null, and catch-and-log-only are all suspects. If this change modifies error/fallback behavior, trace the error paths — most production incidents come from changed catch blocks and silent fallbacks, not happy-path logic.

3. Race conditions — Is shared mutable state accessed concurrently without synchronization? Check module-level variables, singletons, caches, and lazy-init patterns.

4. Silent defaults for unexpected values — Does the code silently substitute a default for an unexpected value (unknown enum variant, unrecognized config key)? In infrastructure and test code, prefer throwing over guessing.

5. Feature flag coverage — If the change adds or modifies feature flags or conditional rollout logic, are all flag states tested? Check for stale flags that are always-on/always-off and should be removed; verify the default-off state doesn't break existing behavior.

Hygiene

6. Dead exports — Are there exported types, functions, or constants not imported by any other file? Check with grep before flagging.

7. Unnecessary wrappers — Are there functions that simply delegate to another function without adding logic, type narrowing, or meaningful naming? The same principle applies at module level: if a file's entire post-refactor content is re-export statements from a single other module (and it no longer holds any unique declarations), suggest deleting the shim and updating importers directly. This is not a breaking change if all callers are in the same repo.

8. Inline business logic where a library method exists — Is there hand-rolled logic (regex parsing, string manipulation, date math, data structure ops) where the project's existing dependencies already provide a tested function for the same thing?

9. Repeated in-house logic that should be extracted — Is the same non-trivial logic block (≥~5 lines, or any block with semantic identity beyond formatting) repeated in three or more sites in this codebase, where a single shared helper would carry the same behavior? The default threshold is three instances, but it's a default, not a rule: a 2-instance extraction can be right when the logic is obviously a single domain concept (Stripe error shaping, retry with backoff, auth-error mapping) and a 5-instance repetition can be wrong to extract when the bodies are coincidentally similar but semantically distinct (each one will diverge under its own pressure). Suggest the extraction site (_shared/<helper>.ts, lib/<helper>.ts, or the codebase's established shared-utility location) and name what the helper should encapsulate, not just "DRY this up." Carve-out: production code is DRY; test code is DAMP — repeated arrange/assert blocks in tests often aid readability over abstraction, so do not flag test-file duplication unless the repeated block is large enough to obscure the test's intent.

9a. Repeated domain discriminants without a shared type — Is the same string literal representing a domain concept (tier, role, status, event type) used as a discriminator at 3 or more sites without a shared named type? Flag and suggest the language's idiomatic named-type construct (TypeScript string-literal union, Java/Kotlin/C# enum, Python Literal[...] or Enum, Rust enum, Go typed-string constant set) declared at the canonical module for that domain, with call sites replaced by the named type.

9b. Unnamed semantic bounds in slicing/truncation — Does the code pass a bare numeric literal as a length, prefix, or offset bound when slicing, truncating, or windowing a string or collection in non-test code? If the number represents a semantic bound (prefix length for redaction, maximum field width, ID segment length), it should be a named constant expressing the intent, not a bare number. Suggest a named constant at the shared-utility level if the bound is used across files, or at the top of the file in the language's idiomatic constant declaration if local.

Clarity

10. Undocumented limitations — Does the code make assumptions or have known constraints invisible to future readers (only handling the first element, assuming single-tenant, ignoring edge cases by design)?

11. Misleading names — Do function or variable names promise more or less than they deliver? A validateUser that only checks one field, an allItems that holds a filtered subset.

12. Stripped WHY comments — In modified files, were comments documenting a non-obvious constraint, subtle invariant, bug workaround, or surprising behavior deleted? Stripping these regresses documentation that the original author judged worth keeping. The "default to no comments" rule applies to adding; it does not authorize bulk removal during unrelated edits. Apply the same WHY test to existing comments: keep if it meets the standard, remove only if it restates WHAT the code does. Check git diff for deleted comment lines in changed hunks.

Security

13. Test adequacy for security controls — For code enforcing security invariants (access control, input validation, privilege boundaries), are there tests for both allow and deny paths? This overrides the general "add tests" exclusion — untested security controls are indistinguishable from absent ones. Check: for each boundary, is the unauthorized caller rejected AND the authorized caller accepted?

Scope discipline

14. Pre-existing issues in unchanged code — If you notice issues in code NOT written or modified in this change, flag them in a separate "Pre-existing issues" section. Do NOT fix them — informational only, out of scope.

Domain: Infrastructure

15. Concurrency and parallelism scoping — Do concurrency groups, mutex locks, or job dependencies match their intended scope? A workflow-level concurrency group affects all jobs, including no-op or unrelated ones — check that cancel-in-progress won't kill an important job due to an unrelated trigger.

16. Secret exposure — Are secrets used in contexts that could log them? Check for secrets in run: commands that echo or pipe output, in env: blocks visible to steps that don't need them, and in artifact uploads. Ensure secrets are not passed as command-line arguments (visible in process lists).

17. Permissions least privilege — Are workflow permissions, IAM roles, or service accounts scoped to what's actually needed? Flag contents: write when only read is required, admin when write suffices, or wildcard permissions.

18. Idempotency — Is the workflow/script safe to re-run? Check for unconditional creates (without "if not exists"), non-atomic operations that leave partial state on failure, and missing cleanup on retry.

19. Trigger-condition alignment — Do trigger filters (branch, path, actor, event type) match the job's purpose? A job intended only for bot commits but triggered on all pushes is a mismatch even if individual steps have if guards.

Domain: Data

20. Migration reversibility — Destructive operations (DROP COLUMN, type narrowing, table drops) need a backup or reversal path.

21. Index coverage — New WHERE/JOIN/ORDER BY columns and foreign keys need supporting indexes, especially on growing tables.

22. Lock safety — Flag long-locking DDL on large tables — ALTER with defaults, CREATE INDEX without CONCURRENTLY, in-migration backfills.

23. RLS and access control on new tables — New tables exposed via auto-generated APIs (PostgREST, Hasura, generated resolvers) need row-security enabled with policies.

Domain: Frontend

24. Accessibility — Do interactive elements have accessible names (aria-label, visible label, alt text)? Are click handlers on non-button elements keyboard-accessible? Check for missing focus management in modals and drawers.

25. Render performance — Are there new inline object/array/function literals in JSX props that would cause child re-renders on every parent render? Check for missing key props on list items and expensive computations not wrapped in useMemo/useCallback where the component re-renders frequently.

26. Bundle impact — Does the change add a large new dependency where a smaller alternative or existing utility exists? Flag full-library imports (e.g., all of lodash) when only one function is used.

27. State-dependent rendering coverage — Does the change modify which UI state a component enters (conditional branches, state machines, context-driven rendering)? If so, check whether tests verify the affected states render correctly. For each new or changed condition, is there a test that the component renders the expected output for each branch?

Domain: Backend

28. Auth boundary coverage — Every new endpoint or RPC needs both authentication (who) and authorization (can they) — and the check must not be bypassable by hitting the endpoint outside the UI flow.

29. Input validation at system boundaries — Validate/sanitize user input before SQL, shell, file paths, or outbound API calls — framework parameterization counts, string concatenation does not.

30. Error response leakage — Error responses must not expose internal details (stack traces, internal IDs, DB error text, file paths) — log server-side, return generic to the client.

31. Dependency upgrades — Does the change upgrade a runtime or build dependency? Read the changelog for breaking changes. Check for peer dependency conflicts and, for frontend deps, bundle size regression.

32. Third-party API integration — Does the change add or modify a third-party API call? Verify retry/timeout behavior, credential scoping (least-privilege keys), and failure modes (API down, unexpected data).

33. Sensitive data in logs — Does the change add or modify logging? Verify logs do not include tokens, credentials, PII, or full API responses from auth/OAuth endpoints. Extract only the fields needed for debugging.

34. Performance-sensitive code paths — Does the change modify a hot path (queries in loops, N+1, cache read/write, large list ops)? Verify with representative data volumes, not just test fixtures.

34a. Untyped error raised where the file uses typed-error dispatch — If the file defines or imports typed error variants and dispatches on them by type (catch-arm narrowing, instanceof / isinstance / errors.As, sum-type pattern match), any untyped/generic error raised for a parallel failure path will bypass the typed arm and fall to the generic handler. Flag any generic-error raise, throw, or error-return in the same file that shares the logical shape of an existing typed variant. The fix is a typed variant — not widening the dispatch.

Domain: Claude Code config

For .claude/skills/**/SKILL.md review, invoke skill-review. For claude/.claude/agents/*.md and plugins/*/agents/*.md review, invoke agent-review. Each owns frontmatter contract, trigger design, voice, length, behavior test, cross-reference vs duplication, and behavioral-equivalence audit on compressions for its file type — do not assert behavioral equivalence on prose compressions yourself; that audit belongs to the dispatched skill.

35. Permission scope — Do permissions.allow rules in settings.json follow least-privilege? Flag blanket allows ("Bash") where scoped ("Bash(git:*)") would suffice. If permissions.allow rules were added or modified, invoke /review-permissions for deep security analysis.

For hook reviews (claude/.claude/hooks/*.sh, hook entries in settings.json), invoke the claude-hook-review skill.

Domain: Lovable config

Apply when changed files match .lovable/**. Invoke the lovable-cloud-knowledge skill for the review checklist (perspective, specificity, scope split between project/workspace knowledge, char budget, sync status).

Exclusions — do NOT flag these

• Issues that a linter, typechecker, or compiler would catch (imports, type errors, formatting)
• Stylistic nitpicks in unchanged code (naming conventions, whitespace, comment style)
• Generic improvement suggestions ("add tests," "add docs," "improve error messages") not tied to a specific finding from the checklist above, except for security controls (see item 13)
• Domain checklist items for domains where no files were changed

Output format

Start with a one-line summary of which domains were detected (e.g., "Domains: Infrastructure, Backend").

Follow with a mandatory Spawn decisions: line. For every Change-type table row that matched the diff (per Step 0.6's enumeration), list it with a one-line verdict:

• spawned: <agent> for <specific question> — the question must be a concrete sentence, not the row's title.
• skipped: <row> — <reason> — name the row by its left-column shorthand and give a one-sentence rationale.

When no Change-type rows match the diff, write: "Spawn decisions: no Change-type rows matched the diff." This makes the absence affirmative, not silent. Empty rationale is the under-spawn failure mode this format closes; visible rationale is reviewable.

Project-layer scope: the mandatory Spawn decisions format is base-skill-only. Project-layer skills (code-review-*/SKILL.md, loaded at Step 0.5) compose by extending checklists — they do not override the output format. A project layer that wants to add fields should do so via an additive section, not by replacing the base format.

For each finding, state:

1. Which checklist item (by number and name)
2. File and line
3. What the issue is (one sentence)
4. Why it matters (one sentence)
5. Suggested fix (concrete, not "consider improving")

If no issues are found, say: "No issues found" — do not pad with praise or generic observations.

Ripple effect triage

Spawn every Change-type table row that matches the diff (per Step 0.6's enumeration). To skip a row, write the skip rationale into the Spawn decisions: output (one sentence per skipped row). High-stakes rows — data migration, row-security policy changes, auth model changes, billing or payment, access-control restrictions, re-pointed reads on shared data — are near-mandatory: skip only when the change is declared dev-only or internal-only with no production-reachable surface, and name the surface in the output. The escape-hatch criteria below remain available but become reasons to skip named in the output, not reasons to never enumerate the row.

This is the last gate before ship — a specialist miss here ships as a regression. Lean conservative; misses are not recoverable downstream the way they are at plan-review. The criteria below anchor when a small diff still fires and when a matched row can be skipped with rationale:

• Contract blast radius — not LOC. A small diff that re-points a shared internal contract (a helper read by many callsites, a config consumed by many flows, a rename that propagates to many policies/functions) fires the relevant Change-type rows even when the line count is small. Count consumers of the contract, not changed lines. A 10-line helper rename consumed by 30+ row-security policies is a high-stakes data-model change, not a hygiene refactor.
• Below-staff-level depth. Specialized kernels (query-planner internals, cryptography primitives, fine-grained a11y, kernel-level concurrency) — not "general knowledge in domain X."
• High-stakes boundary. RLS, auth, billing, payment, data migration, privileged ops — a specialist's eyes are worth it even at generalist depth.
• Holistic-reasoning overload. Multi-domain ripple you can't hold in working memory at once.
• Convergence-as-design-tell from a prior round (see Reconciliation).
• Explicit user request.

Always spawn ciso-reviewer when the change touches auth/authz, secrets, tokens, data exposure, sensitive-data logging, third-party data sharing, or infra permissions — high-stakes-boundary case is non-optional, unless the change is declared dev-only or internal-only (no privilege boundary crossed) (e.g., a dev-only flow with no production reachability, or an internal-only path where engineers themselves are the only callers and the change crosses no privilege boundary they shouldn't cross). When skipping on those grounds, name the surface in the review output — never silently skip. The ciso-reviewer rule is one instance of the general default-fire pattern above, not the exception. Always spawn staff-product-engineer when the change alters any end-user-visible surface: user interface, transactional or lifecycle email, push notification, SMS, in-app notification, billing artifact, exported file, webhook payload to a customer integration, OAuth consent screen, embedded widget or iframe surface, or end-user-visible log/audit entry. The trigger fires on the channel, not on the file-path domain — a backend-only change that ships a new email body still alters user-facing behavior. Indirect channel effects count: a data or logic change that determines which channel fires or what it contains (a new user-status enum value that triggers a different lifecycle email, a field added to a user record that an existing template reads) alters user-facing behavior even when the diff contains no channel template file.

Spawn per question (not per file-path domain) — "change touches .github/" isn't enough; the question needs a specific shape. When you spawn: spawn on the CODE, not on this review's output (each subagent reads the diff fresh); pick the specialist that serves the question (table is reference, not roster); pass diff scope, specific question, AND — for re-review — prior findings + what's been applied. Reviewers without prior context re-discover; that's wasted spawn.

The Change type column keys on what the change does for an operator or consumer, not on file types — a markdown-only diff can cross a runtime-config or CI/CD boundary if it restructures the taxonomy operators use to provision secrets, identify deploy targets, or reason about config layering.

Report every matched row's verdict via the Spawn decisions: line in the Output format section above. Empty rationale is the under-spawn failure mode the format closes — write the read, don't omit it.

When you do spawn a specialist, be specific. "Spawn ciso-reviewer" is useless; "Spawn ciso-reviewer and ask it to verify the checkout flow in CheckoutPage.tsx still enforces ownership after the new validation" is actionable.

When spawning staff-backend-engineer, pass findings_path: agent-reviews/staff-backend-engineer-<epoch>-<slug>.md in the prompt, where epoch = $(date +%s) and slug = $(git rev-parse --abbrev-ref HEAD | tr '/' '-' | cut -c1-20). Spawn synchronously (not run_in_background) — the mandatory read-back step must execute in the same turn; a background spawn lets the review be silently skipped. The agent writes structured Markdown findings to that path and returns inline only: path, one-sentence summary, finding count. After it returns: if the agent reported a successful write, Read the findings file — Recommendations section first, full file when count > 0; if the agent reported a write failure and fell back to inline findings, use those inline findings directly instead. Before the first spawn, add agent-reviews/ to $(git rev-parse --git-dir)/info/exclude idempotently (grep-check before appending) so findings files can't be accidentally staged; cleanup is automatic with the worktree. The agent-reviews/ directory does not need to be pre-created — the agent's Write call creates it. Canary scope: pass findings_path only to reviewers that have the Write tool and a file-based-output section; today that is staff-backend-engineer alone. Extending findings_path to any other reviewer without first adding Write + the section to that agent re-arms the heredoc-abort-on-large-findings bug.

Other spawned specialists must return ≤2K tokens of structured findings (checklist-item-keyed bullets), not narrative prose; if they exceed the budget, prioritize by severity and explicitly note that lower-severity items were omitted. Include this constraint in each agent's prompt.

Reconciliation

After spawned reviewers return findings, pause if findings concentrate on a single surface — the same feature, implementation detail, or design choice attracting multiple gaps. If two specialists flag the same file:line with the same root cause, present the finding once with both reviewer attributions rather than as duplicate findings. Two readings:

• Implementation-wrong-shape. The surface is the wrong abstraction; gaps will keep multiplying as you patch. Replace, don't patch-by-patch.
• Prompt-overlap artifact. Reviewers given similar prompts produce N voices of the same observation. Convergence looks like signal but is framing-induced.

You judge which applies. Don't treat convergence as automatic authority for "patch each gap." If implementation-wrong-shape, replace the surface and re-run Step 1. If prompt-overlap, apply the underlying finding once, skip duplicates, and note the overlap so the next spawn uses tighter prompts.

Finding disposition

After Reconciliation, before producing the recommendation, walk every reviewer-spawned finding and tag it ADDRESS or DEFER. ADDRESS is the default and needs no rationale; DEFER requires a named criterion from the closed list below.

Default ADDRESS is grounded in opportunistic-refactoring discipline (Fowler, Opportunistic Refactoring): code already in the diff, especially when covered by tests already running in this PR's verification, is the cheapest place a finding will ever be fixed. DEFER is the exception.

Format: inline ADDRESS: / DEFER (<criterion>): tags when there are ≤2 findings; a four-column table — Finding | Source | Disposition | Rationale — when there are 3+. The orchestrator's recommendation that follows can still propose grouping fixes into this PR vs follow-ups, but every finding has been seen, tagged, and either addressed or justified against the closed list.

DEFER criteria (closed list). A finding may be tagged DEFER only when it matches one of:

1. Orthogonal scope — addresses code or a concern truly unrelated to this change and not covered by tests already running in this PR's verification.
2. Coordinated multi-PR effort, with design-tell test passed — the fix requires changes across repos, services, or sequenced migrations that exceed this PR's coordination boundary. Before tagging DEFER here, run the design-tell test: if the coordination requirement signals the current PR's design is wrong-shape, re-run Step 1 instead of deferring. If it's pre-existing structural debt, DEFER is valid only when a ticket is filed for the remediation and the issue is surfaced to the human in the recommendation.
3. Gold-plating beyond declared user surface — Step 1's implementation-fitness gate established the user surface and threat model; the finding adds a layer beyond it.
4. Contract pinned at another layer — test coverage suggested for a contract already enforced by another layer's test or invariant.
5. Edge case below current scale — failure mode that doesn't fire at the system's operating scale; mark the boundary with an in-code comment so the future re-evaluation has a hook.

Invalid DEFER rationales. These look like dispositions but aren't — do not use them:

• "Informational only" / "FYI" — reviewer severity labels are triage signals, not dispositions. The orchestrator runs its own test against the five criteria above. A reviewer's "FYI: X is stale" produces orchestrator disposition ADDRESS unless one of the criteria genuinely applies.
• "No correctness impact" — stale comments, misleading names, and outdated docs cost future readers (often future agent sessions priming on the comment to build context). Mechanical fixes are ADDRESS.
• "General skill guidance, not PR scope" — narrowing the PR boundary to dismiss a finding is scope-shrinking, not disposition. If the guidance has a durable home (header comment, migration note, runbook line in the file), ADDRESS it there.
• "Pre-existing gap" — pre-existing is a fact about the gap's age, not a disposition. A pre-existing gap that closes a correctness or security-invariant hole is ADDRESS; pre-existing alone is not a criterion.

If you find yourself tagging 3+ findings DEFER in a single review, re-read the criteria — the default is ADDRESS, and a heavy DEFER list is usually a sign the criteria are being stretched.

Item ownership

Routes each checklist item to the reviewer subagent(s) that file findings on it. Bold shorthands match titles above; numbers are the dispatcher's primary key. Primary owner files findings; co-owners are spawned where the item touches their turf. When in doubt, this table wins over inline mentions.

The dispatcher fires reviewers per file-path domain detection. Each agent self-scopes against the diff and returns early ("No X concerns") when out of lane.

Step — Record review completion

If the review is clean (no blockers, no unresolved critical findings, and you reviewed the currently staged changes), record it by running this command exactly once:

~/.claude/scripts/marker.sh write code-review

This writes the hash of the currently staged diff into a per-session marker keyed by <repo-hash>.<session-id>. The pre-commit hook reads the same session-id from its JSON payload and compares the staged-diff hash against THIS session's marker — match means the commit is allowed through. Per-session keying prevents two parallel sessions in the same worktree from overwriting each other's markers. Re-staging any change invalidates the marker automatically.

If the chain fails (empty SESSION_ID, etc.), the capture-session-id.sh SessionStart hook didn't run — abort and report; do not proceed without the marker, since git commit will be blocked by the gate.

Do NOT write the marker if:

• The review found blockers or unresolved critical findings
• You reviewed a different state than what is currently staged
• The user asked you to present findings without committing
• You are not in a git repository

If you skip it, say so explicitly so the user knows the commit gate will block until issues are fixed and the review is re-run on the final staged state.

This marker must be written only by this skill. Never compute and write it manually — that satisfies the hook's shell check but forges the gate. If the hook blocks a commit and you cannot invoke this skill, spawn a subagent that can; do not hand-write the marker path.

Source: jcdendrite/claude-config — distributed by TomeVault.