code-review

Code Review Skill

Use this skill when reviewing code changes, pull requests, or auditing existing code.

Review Checklist

Architecture / milestone-claim audits

Use this workflow when asked to audit an implementation against docs, roadmap claims, or shipped-milestone claims:

1. Do not modify files unless explicitly asked; run read-only inspection, builds, health checks, and DB queries only.
2. Separate claims from evidence. First extract the documented architecture/status claims from README/plan/roadmap docs, then verify against concrete code, routes, packages, DB tables, live processes, and persisted data.
3. Identify the canonical correction before auditing old plans. If the user says a topology/architecture was “corrected,” treat older docs/ADRs as potentially superseded claims. Report which assumptions remain valid vs which must be rewritten, instead of forcing all docs into one blended architecture.
4. Map subsystem by subsystem. Produce a table from requested subsystems to concrete files, HTTP routes, background jobs, profile/config files, and database tables.
5. Verify live/shipped claims with real output where possible: process manager status, public/local health endpoint, build result, database row counts, migration table, audit/activity rows, container status, and git history. Treat docs/commit messages as claims, not proof.
6. Call out drift precisely. Prefer “documented X, implemented Y” with paths and route/table names. Distinguish: implemented and verified, partially implemented, implemented differently than docs, missing, stale UI/doc text, and safety/security gaps.
7. Look for wrong-path implementation, not just stale prose. When auditing divergence from a corrected architecture, inspect whether superseded assumptions have already landed in schema, provisioning scripts, env/config, routes, or tests. Separate “docs need rewrite” from “code requires migration/deprecation.”
8. Check contracts between layers, not just files. For modular architectures, verify that documented extension contracts/manifests actually exist and are used, not bypassed via direct imports.
9. End with verification + cleanliness. Report build/health checks performed and confirm no files were modified when the task is audit-only.
10. For dual-use command-layer audits, separate UI routes, agent routes, and business commands. When asked whether a capability works for both humans and Hermes agents, map the human Studio components, admin HTTP routes, agent-gate routes, and underlying state-machine/provider functions independently. Flag duplicated body mapping, module-local schedulers, app-level review hooks, mixed handoff seams, and human-only/agent-only operations. The target architecture is one canonical command definition with thin Studio + typed-tool adapters, not two route families that happen to call the same helper.

Reference: references/caduceus-tenant-topology-audit.md captures a concrete example of auditing Caduceus docs/ADRs/code against a corrected tenant-local Portfolio OS topology. Reference: references/caduceus-dual-use-command-layer-audit.md captures the Caduceus Creative/Social/Ads/Video audit pattern for migrating rich human Studio + Hermes typed tools onto one canonical command layer. Reference: references/caduceus-social-oauth-command-parity-review.md captures the Caduceus Social high-risk review pattern: full OAuth start→state→callback→sync→publish tracing, callback-XSS probes, actual-grant verification, canonical provider-asset metadata, exact surface-bound credential selection, scheduler truth, Studio/Hermes field parity, and behavioral test requirements. Reference: references/caduceus-social-migration-publish-safety-review.md captures exact-commit review of repairable legacy Social migrations and irreversible publish safeguards: partial-target reruns, wrong nonempty mappings, intentional clears, past-due schedule activation, approved-target retry bounds, provider-write zero assertions, concurrency/TOCTOU races, acknowledgement loss, and shared-worktree discipline. Reference: references/caduceus-social-final-release-adversarial-probes.md adds final-release probes for a moving main, missing named bases, first-Meta-connection credential drift, demoted/expired retained-token readiness, multi-item Story partial-publication duplication, past-due migration activation, and malformed OAuth expiry. Reference: references/caduceus-effortless-signup-onboarding-audit.md captures the Caduceus central signup/onboarding audit map: landing Create action, spatial nodes, chat/profile state, passkeys, provisioning progress, tenant handoff, mobile behavior, project canvas reuse, and targeted tests. Reference: references/user-tier-boundary-review.md captures the Caduceus Normal/Pro/Admin tier-boundary audit pattern: role-vs-tier pitfalls, agent-key escalation, unscoped admin APIs, central handoff contract readiness, local-login fallback, and bootstrap seed hazards. Reference: references/caduceus-onboarding-canvas-account-tier-audit.md captures the Caduceus central onboarding canvas audit pattern: ephemeral preview vs tenant truth, Normal one-business semantics, Pro/Admin portfolio semantics, minimal profile handoff, Spawn provisioning status, and Hermes profile/coordinator constraints. Reference: references/caduceus-visual-onboarding-ui-review.md captures the Caduceus visual-onboarding/shared tenant UI review pattern: read untracked UI helpers, run focused central/onboarding tests plus web typecheck, mount the Vite onboarding entry for real DOM measurements, catch clipped dvh/min-height layouts under globally hidden body scroll, and probe optimistic business-name inference regexes.

1. Security First

• [ ] No hardcoded secrets, API keys, or credentials
• [ ] Input validation on all user-provided data
• [ ] SQL queries use parameterized statements (no string concatenation)
• [ ] File operations validate paths (no path traversal)
• [ ] Authentication/authorization checks present where needed

2. Error Handling

• [ ] All external calls (API, DB, file) have try/catch
• [ ] Errors are logged with context (but no sensitive data)
• [ ] User-facing errors are helpful but don't leak internals
• [ ] Resources are cleaned up in finally blocks or context managers

3. Code Quality

• [ ] Functions do one thing and are reasonably sized (<50 lines ideal)
• [ ] Variable names are descriptive (no single letters except loops)
• [ ] No commented-out code left behind
• [ ] Complex logic has explanatory comments
• [ ] No duplicate code (DRY principle)

4. Testing Considerations

• [ ] Edge cases handled (empty inputs, nulls, boundaries)
• [ ] Happy path and error paths both work
• [ ] New code has corresponding tests (if test suite exists)

5. Policy/registry/data-contract reviews

Use this extra pass when reviewing manifests, registries, product policy maps, configuration schemas, or account-boundary code:

• [ ] Probe negative cases, not just shipped tests. Run small read-only smoke/probe scripts to see whether invalid data is accepted (unknown provider ids, duplicate entries, unknown fields, alternate secret/customer key names, non-JSON values).
• [ ] Probe sanitizer failures on error paths. For final security approval, include malformed inputs where unknown property names themselves look like secrets (sk-..., bearer/token-shaped keys) at top-level and nested schema levels. Validation should reject them without echoing the raw key name/value in error.message; allowlist errors often run before secret scanners and can accidentally leak the property label.
• [ ] For final approval after raw/header-normalization hardening, probe both representations. Do not rely only on tests that count duplicate normalized headers. Build a small read-only probe that passes Express/Node-style { headers, rawHeaders }, asserts matching raw+normalized values succeed, and asserts mismatched raw Authorization or product header values reject generically without leaking either token/product string. Include rawHeaders edge cases: missing required raw entry when rawHeaders is present, duplicate raw entries with different casing, odd-length arrays, oversized arrays, and non-string names/values.
• [ ] Prefer allowlists over denylists for policy schemas. If a manifest claims it excludes credentials/pricing/customer identity but accepts arbitrary extra fields, flag it: alternate names like billing, customer.id, tenant.id, or apiKeys can bypass exact-key denylists.
• [ ] Check immutability semantics end-to-end. If public reads are documented as immutable, verify both returned copies and internal stored references. Flag registries that validate an object once but keep/return the same mutable reference.
• [ ] For final approval after an immutability/aliasing fix, re-run the exact attack class. Do not rely only on the new unit test. Write a small read-only probe (often node --input-type=module) that mutates returned arrays/objects, checks sibling references, calls the factory again, and covers every relevant branch (e.g. default and optional-tool/browser-enabled config paths). Include source-descriptor mutation probes when functions close over module constants rather than public descriptor fields.
• [ ] For final approval after public-projection hardening, probe the projection directly. Build an object with allowed fields plus plausible credential/account/billing/raw/internal/arbitrary fields, call the public projector, and assert the exact returned key list. Also assert nested provider projections omit credential refs and that returned objects/arrays are detached/frozen.
• [ ] Verify narrow import boundaries, not just behavior. For platform contract modules that should be side-effect-free, inspect source/imports and run import smokes proving they use narrow package subpaths only and do not pull root packages, Docker/process adapters, live DB files, S3/SSH/PM2, or server entrypoints.
• [ ] Separate adjacent operational configs from product registry scope. If a product list is intentionally limited, explicitly grep/review neighboring deploy broker, hostname, tenant alias, or customer-specific configs and prove excluded tenants/domains are not accidentally represented as first-class products/profiles.
• [ ] Validate referenced ids against source-of-truth registries. String-array policy fields should reject unknown ids and usually duplicates; non-empty string checks alone are not enough for provider/tool/profile policy.
• [ ] Probe exact-set requirements across every policy dimension, not just entity ids. When a registry/config must contain exactly a known set of product/service ids, provider ids, tool ids, modes, profiles, or lifecycle values, inject read-only dummy registries/configs with all expected values plus one extra (e.g. evil-provider) and verify the system fails closed. Superset checks (expected.every(id => set.has(id))) miss taxonomy drift even when request-time allowlists still reject some extras.
• [ ] Test derived contracts. Compose helpers the way consumers will (e.g. profile containerPrefix + containerName()) to catch contract drift that isolated field tests miss.
• [ ] Keep audit-only reviews read-only. Use probes via node --input-type=module, tests, import smoke checks, git diff/status; do not patch code unless explicitly asked.
• [ ] For user-tier/account-boundary reviews, distinguish role from tier. A lower-tier owner may still have role=admin within their own workspace; audit every adminOnly/requireAdmin route for missing account-type/scope checks. Direct HTTP/entity-id endpoints, agent identity creation, module routers, central handoff schema switches, legacy local-login fallback, and bootstrap seeders need server-side probes beyond UI hiding. See references/user-tier-boundary-review.md.

6. Durable executor / async checkpoint reviews

Use this extra pass when reviewing executors that accept work, persist operation state, then schedule or run side-effect adapters:

7. Tenant appliance / backup-restore security reviews

Use this extra pass when reviewing portable tenant appliance packaging, Docker/Compose runtime contracts, and backup/restore hooks. See references/caduceus-tenant-appliance-backup-review.md for a concrete Caduceus review transcript and adversarial probes. See references/hermes-spawn-caduceus-backup-review.md for Hermes Spawn host-worker probes covering predictable temp symlinks, checksum mismatches, noncanonical duplicate tar paths, and valid restore compatibility.

• [ ] When the user demands machine-readable review output, keep the final answer exactly in that shape. For JSON-only review requests, do not include markdown, prose, or extra keys. Still run the probes first; the final response should only report the verified result.
• [ ] For uncommitted-diff security/logic JSON reviews, inspect both tracked diffs and untracked source/test files. Start with git status --short, git diff --name-status, and git diff --stat; group file diffs by risk area; read relevant untracked files because git diff omits them; run git diff --check; then return only the requested JSON. See references/uncommitted-diff-security-logic-json-review.md.
• [ ] Probe predictable temp-file symlink attacks for JSON/resource writers. If a fix replaces path.with_suffix(... + ".tmp") with a safer writer, create attacker-chosen legacy temp symlinks such as resources/manifest.json.tmp and verify outside bytes are unchanged. Also probe the real target path as a symlink and require a fixed symlink/escape error.
• [ ] Verify archive payload bytes, not just manifest self-consistency. A valid aggregate checksum over files does not prove tar member contents match the advertised size/SHA. Re-open or stream every expected payload member and compare actual byte count plus SHA-256 before accepting restore input.
• [ ] Reject noncanonical archive paths before comparing member sets. Use POSIX canonicalization rules for tar member names; reject backslashes, absolute paths, empty segments, dot/dotdot segments, and aliases where posixpath.normpath(path) != path so data/x and data//x cannot bypass duplicate detection or restore semantics.
• [ ] Include a valid backup/restore compatibility probe after hardening. Negative cases are not enough: build a legitimate archive with payload data larger than the manifest read cap, read its manifest, restore it, and assert byte-for-byte payload equality so security checks do not break valid backups.
• [ ] Probe the exact tenant-root path, not only descendants. If code says CADUCEUS_TENANT_ROOT must be a real directory, test a symlink root. A common bug is resolve() before lstat(), which accepts the symlink target while claiming to reject symlink roots.
• [ ] Check tar/resource exhaustion limits. Restore must bound member count, total extracted bytes, per-member size, manifest size/file count, and compression ratio before extraction. tar.getmembers() plus unbounded copyfileobj() is not sufficient for hostile archives.
• [ ] Check crash durability, not just logical rollback. Atomic rename/os.replace is not durable unless files and parent directories are fsynced. Backup archives, SQLite snapshots, staging dirs, swap dirs, and payload-root parent dirs need explicit durability if the contract claims crash-safe restore/backup publication.
• [ ] Probe symlink TOCTOU in backup copying. lstat → open(path) → lstat can still follow a swapped symlink or changed file. Require fd-based opens with O_NOFOLLOW, fstat, and directory-fd/openat-style traversal for adversarial tenant filesystems.
• [ ] Require immutable/supply-chain-pinned appliance builds. Base images should be digest-pinned (FROM image@sha256:...) when reviewing final security approval, not tag-only (node:22-bookworm-slim).
• [ ] Verify runtime dependencies are explicit. If in-container hooks call python3, sqlite3, tar, etc., the Dockerfile should install or smoke-check them explicitly; do not rely on accidental base-image contents.
• [ ] Bind health/readiness to the full runtime identity. Image and Compose health checks should validate tenant id, operation id, and stack contract when those are part of the provisioning contract; a generic HTTP 200 is too weak for final approval.

• [ ] Audit docs/tests for stale contract names. If implementation moved from prime/default identities to required operation-bound identity, update README/env/tests together; stale docs around admin auth (ADMIN_PASSWORD_HASH vs ADMIN_PASSWORD) are security-contract drift.

• [ ] Probe post-persist acknowledgement loss at every transition boundary. Do not only test final running -> succeeded/failed acknowledgement loss. Simulate transitionOperation() committing and then throwing for early edges such as accepted -> planned and planned -> running; verify the executor re-reads durable state, continues/resumes safely, or returns a manual-recovery state. Flag code that leaves durable planned/running operations with no effects and later treats non-accepted retries as inert skips.

• [ ] Probe post-persist acknowledgement loss for per-effect claims/results, not only public status transitions. For APIs such as claimExecutionStep() and recordExecutionResult(), simulate atomic rename success followed by directory/fsync failure. Require an immediate durable re-read: an exact desired operation/effect record means success, old state follows only the explicitly bounded retry policy, and divergent or unreadable state fails closed. A test that merely retries later and observes idempotent replay is weaker and does not close the original acknowledgement window. Also probe pre-rename failure to prove old bytes remain unchanged.
• [ ] Probe initialization/plan-attachment acknowledgement loss, not only later transitions. If initializeExecution() or equivalent can both attach an execution envelope and move accepted -> planned, simulate post-rename directory/fsync failure there too. Require immediate durable re-read and continuation when exact planned + initialized state exists; otherwise an operation can be stranded with no effects and no scheduler replay.
• [ ] Validate global mutable execution history when loading durable ledgers. Fingerprints often intentionally exclude mutable apply/evidence fields, so per-effect schema validation is insufficient. For ordered forward execution, require an applied prefix followed by at most one current started/not_applied boundary and a pending suffix; execution phase/status must agree with whether forward activity exists. Reject successor applied while a predecessor is pending/started/not_applied, not_applied with later progress, premature compensation state, and terminal evidence with impossible ordering.
• [ ] Probe result-side negative bindings independently. Cover wrong fingerprint, wrong attempt, unstarted effect, public-not-running state, conflicting terminal evidence, and byte preservation on every rejection; claim-side coverage does not imply result-side safety.
• [ ] Probe scheduler hook failures after acceptance persistence. If a router persists an accepted operation and then calls an optional async scheduler/executor, wrap property lookup, invocation, and thenable handling in the decoupling boundary. A throwing getter for tenantExecutor.scheduleOperation must not turn a committed acceptance into a 5xx response or prevent replay/reconciliation from scheduling. When there are multiple scheduler hooks (for example generic tenant and specialized-product executors), run the same getter/sync-throw/rejected-promise/hostile-thenable probes for each hook, not only the first one added.
• [ ] Check replay semantics against scheduler failure modes. If idempotent replays intentionally do not reschedule, then the initial scheduling path must be impossible to fail after durable acceptance without an alternate durable outbox/reconciler. Otherwise an accepted operation can be stranded forever.
• [ ] Verify lock scope matches the mutable resource. A lock keyed by tenantId + operationId prevents duplicate execution of one operation but may allow two different operations to mutate the same tenant stack concurrently. For lifecycle/restore/upgrade/delete boundaries, probe two different operation ids against the same tenant/resource and require a resource-scoped lock with operation id as owner metadata.
• [ ] Verify adapter durability does not rely on process memory. Backup/restore/rollback adapters must survive adapter restart. Catalog allowlists, pre-snapshot maps, and ownership caches held only in process memory are not durable authority; probe backup with a fresh adapter instance before restore/rollback.
• [ ] Treat in-repo missing-module tests as blockers. If new untracked/tracked tests import modules that are absent, report the ERR_MODULE_NOT_FOUND as incomplete implementation unless the test itself is explicitly out of scope. Do not let neighboring green focused suites mask canonical-glob failures.
• [ ] Verify constructor/dependency-surface sanitization. For security-sensitive executors, probe hostile accessors on dependency objects (operationStore, effectAdapters, tenantExecutor) and ensure dependency validation reports fixed safe errors rather than leaking raw getter messages.
• [ ] Prefer small adversarial probes over test names alone. Use short read-only node --input-type=module probes that wrap stores/adapters to simulate after-commit throws, getter traps, durable re-read faults, and retries; report the actual durable state and adapter-call list.

8. OAuth provider-schema and account-token routing reviews

Use this pass for social/provider OAuth changes where external response schemas, refresh rules, permission-derived readiness, and legacy credential fallbacks meet a publishing or mutation boundary:

• [ ] Fetch the provider's current official docs and build one probe using the exact documented response envelopes; do not trust local mocks to establish provider compatibility.
• [ ] Exercise state consumption on success, denial/error callbacks, expiry, redirect drift, and replay. An early if (error) branch must not bypass state validation or leave a reusable state.
• [ ] Probe malformed persisted OAuth-state timestamps, not only normally expired states. Parse state expiry once, require Number.isFinite(), and reject before consuming state or making any provider call; Date.parse(value) <= now fails open on NaN.
• [ ] Treat publish_ready or equivalent as a server-side authorization predicate. Personal/ineligible/missing-permission accounts must be blocked before token selection and side effects.
• [ ] Probe refresh as an authorization transition: enforce provider age/validity requirements and never promote a blocked account without revalidating its granted permissions.
• [ ] Prove account-bound token selection with two accounts plus expired/missing-token cases. A global legacy token must not silently replace a direct account token unless provider flow and account ownership are explicitly bound.
• [ ] Verify token-source/Graph-origin pairing and API versioning independently for direct and legacy flows.
• [ ] Reject raw exception interpolation in OAuth callback HTML; use fixed copy or escaping even if expected provider errors are normalized.
• [ ] Require behavioral service/router tests for OAuth, refresh, readiness, and routing. Regex/source-presence assertions are supplemental only.
• [ ] Exercise the real UI-shaped publish request, not only API fixtures with hidden routing fields. Persist and approve an exact {account/provider-asset, platform} target; a publish-time body must not override the approved platform, and a direct Instagram account selected in the UI must not silently default to Facebook.
• [ ] Treat encryption-key configuration as release readiness. Production startup/readiness must reject absent or known-default token-encryption/session/admin secrets; ciphertext generated under a public fallback key is not credential secrecy.

See references/instagram-business-login-security-review.md for the current Instagram Business Login response-shape, state, refresh, readiness, credential-routing, UI-shaped publication, encryption-readiness, and adversarial-probe checklist.

9. Shared multi-tenant messaging-router reviews

Use this pass when one provider project, inbound stream, sender registry, or sidecar is shared across tenants:

• [ ] Require proof of sender/account ownership before a phone, email, or provider identity becomes routing authority; syntax validation plus first-claim-wins is not tenant isolation.
• [ ] Make sender-route ownership checks and writes one transaction. Probe concurrent claims and assert the final row equals one complete writer tuple, never a hybrid of tenant identity from one writer and URL/key/generation from another.
• [ ] Refresh operation id, destination URL, and tenant credentials on idempotent reprovision; a same-sender fast path must not retain stale authority.
• [ ] Reconcile routes against tenant lifecycle state and delete/quarantine authority and PII for disabled, deleted, or replaced tenants.
• [ ] Enforce topology promises such as DM-only routing; reject group spaces and bind the reply destination to the authenticated sender.
• [ ] Probe acknowledgement loss after tenant execution and after provider send. Durable dedup must cover both effects, and the tenant endpoint must enforce the provider message id rather than merely receiving and ignoring it.
• [ ] Correlate provisioning requests/results with a request id, generation, or fingerprint. Probe old-request/new-result races; a generic ready result must never suppress publication of a newer assignment.
• [ ] Harden secret and handoff files against permissive create-before-chmod windows, predictable-temp symlinks, non-regular files, and unbounded per-tenant scans.
• [ ] Verify the broker/sidecar is actually wired into service supervision, startup, health/readiness, credential provisioning, and restart behavior; executable scripts alone are not a deployed feature.
• [ ] In shared or actively edited worktrees, hash staged and unstaged diffs and re-check them before final output. If another process stages a fix mid-review, re-read and re-probe the new diff rather than reporting a stale finding.

See references/shared-multitenant-messaging-router-review.md for concrete race, replay, request/result correlation, nested-script, filesystem, lifecycle, and deployment probes.

Review Response Format

When providing review feedback, structure it as:

## Summary
[1-2 sentence overall assessment]

## Critical Issues (Must Fix)
- Issue 1: [description + suggested fix]
- Issue 2: ...

## Suggestions (Nice to Have)
- Suggestion 1: [description]

## Questions
- [Any clarifying questions about intent]

Common Patterns to Flag

Python

# Bad: SQL injection risk
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")

# Good: Parameterized query
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))

JavaScript

// Bad: XSS risk
element.innerHTML = userInput;

// Good: Safe text content
element.textContent = userInput;

Tone Guidelines

• Be constructive, not critical
• Explain why something is an issue, not just what
• Offer solutions, not just problems
• Acknowledge good patterns you see