Skip to main content

Codebase Review

Deep-review an existing repo's architecture, security, and hardening posture (building on the spec→repo build). Use when asked to review an existing codebase (not a PR diff) — a private repo, a freshly-shared project, or a deployed app. Goal: a confident, evidence-backed verdict on architecture and security, with real build/test output as proof.

Workflow​

  1. Verify access first. For a repo just shared with you, run gh auth status, gh api user --jq '.login', then find the repo (gh repo list <owner> / gh search repos). Confirm permission level via gh api repos/<owner>/<repo> --jq '.permissions'. Report access before diving into code.
  2. Clone (gh repo clone), then map the tree: git log --oneline -15, find . -type f -not -path './.git/*', current branch + remote branches.
  3. Read config + entrypoints first: package.json (scripts, deps, engines, overrides), README.md, AGENTS.md/CONTRIBUTING.md (repo-enforced workflow rules), wrangler.jsonc/tsconfig*.
  4. Read the architecture spine: worker entry (worker.ts), MCP/tool surface, service layer, repository layer, domain logic. Read these in parallel batches.
  5. Spend the most effort on the security-sensitive layer. For a Supabase/DB-backed app that is the SQL migrations and RPC functions (see checklist). Read these before general code.
  6. Verify claims with real tool output. npm install then npm run check, npm run build, npm test. Report actual pass/fail counts. For dependency health run npm audit and classify severity + whether the fix is in/out of the pinned range.
  7. Deliver a structured verdict: architecture summary, "done well" list, observations table (severity-ranked), bottom line, and 2-4 optional next actions.

Security review checklist (backend / DB-backed / MCP)​

  • Audited-RPC-only mutation pattern: direct INSERT/UPDATE/DELETE revoked from app roles (incl. service_role) so the only write path is a security definer function that also writes an audit_events row. Verify revoke statements exist and grants are narrow (grant execute ... to service_role only).
  • set search_path = '' on every security-definer function (prevents search-path hijacking). Grep for functions missing it.
  • Row-Level Security enabled on every user-owned table + owner policies ((select auth.uid()) = user_id).
  • Tenant-safe composite FKs: (user_id, id) foreign keys that prevent cross-tenant reference attacks.
  • Destructive ops = two-step approval: request token (short TTL enforced both in app AND a DB check constraint), then confirm. Confirmation locks the row FOR UPDATE, revalidates ownership, acts, marks consumed, and audits atomically. Single-use under concurrency.
  • Sensitive identifiers hashed in audit log (e.g. sha256(approval_token) not plaintext), with a backfill migration to re-hash historical values.
  • OAuth hardening (Cloudflare Workers OAuth): PKCE S256 enforced (plain rejected), exact-scope check, __Host- cookie HttpOnly/Secure/SameSite=Lax, CSRF state bound to browser session, single-use auth codes claimed atomically via Durable Object transaction, rate-limited dynamic client registration.
  • Money as integer cents; strict real-date validation.
  • Crypto/constant-time signature helpers: review any safeEqual/timingSafeEqual/HMAC-compare helper carefully. A hash-then-compare helper that compares two **ArrayBuffer**s with === is reference equality, not content equality — it returns false for every two distinct buffers, silently rejecting all valid signatures. Also grep for signature/verification helpers that are defined but not wired (dead stubs returning 501/NOT_IMPLEMENTED): a webhook or token gate that "looks implemented" but routes through a placeholder is a real gap even in a pilot.
  • Unescaped user/tenant-controlled strings in templated output (email HTML, files, shell): a value like <img src=x onerror=...> in a name/vendor/subject interpolated into an HTML template is an injection → phishing via your own sending domain. Check every templates.ts-style builder.
  • Rate limiting + request-size caps: verify the spec's promised throttling (e.g. n/min/user) and body-size limits are actually implemented, not just documented. Missing throttling on unauthenticated signup/onboarding routes is a free-trial-abuse vector; unbounded request bodies are a DoS vector.
  • Look for this across every migration — many small sequential migrations capturing hardening steps is a strong-signal pattern, not a smell.

Pitfalls​

  • read_file redacts literal credential-looking strings. In source review, an Authorization: Bearer ${...} header may render as Authorization: *** ${...} — that's the reader redacting, NOT a bug. Verify with awk 'NR==<line>' src/file | od -c before flagging.
  • Don't flag nested-capacity quirks as bugs. A greedy allocation that lets projectedRemainingCents go negative may be an intentional projection choice. Note it as a low-severity enhancement, not a defect.
  • npm audit high-severity findings often live in the dev toolchain (wrangler→miniflare→undici), not the runtime path — classify severity and whether the fix is outside the pinned range before alarming.

Support files​

  • references/security-backend-checklist.md — expanded domain notes: the audited-RPC + approval-token security model in detail.
  • references/crypto-and-hardening-review-examples.md — worked examples from a CF-Worker+Supabase SaaS review: the hash-then-=== constant-time bug (breaks every signature), dead-but-present signature helpers, unescaped template strings, and throttling/size-cap gaps.

Supporting files: this skill's supporting files are held in the docsite at docs/15-skills/_support/software-development/codebase-review/ — fetch them fresh from jknash/docsite main alongside this page. Source: jknash/hermes-shared-skills · branch hermes-jkdev001 @ 1d0d545c3970 · skills/software-development/codebase-review/ · view source · Imported 2026-10-04. Supporting files (references, scripts) remain in the source repository.

Published by Muse · 2026-10-04.