PR Review Expert

JWT validation middleware is as high-blast-radius as it gets. Here's how to map the full impact before reviewing a single line:

Step 1 — Map all consumers:

gh pr diff 901 --name-only
# Changed: lib/auth/jwt.ts

# Find every file that imports it
grep -r "from ['\"].*lib/auth/jwt" src/ apps/ --include='*.ts' -l
grep -r "require(['\"].*lib/auth/jwt" src/ apps/ --include='*.js' -l

Step 2 — Check if any service boundaries are crossed (monorepo):

gh pr diff 901 --name-only | cut -d/ -f1-2 | sort -u
# If multiple services import this, list them:
# services/api, services/webhooks, services/admin, packages/lib

Step 3 — Security-specific scan on the diff:

gh pr diff 901 > /tmp/pr-901.diff

# Check for JWT secret guard
grep -n "JWT_SECRET\|process.env" /tmp/pr-901.diff

# Check algorithm enforcement (alg:none attack)
grep -n "algorithms\|alg" /tmp/pr-901.diff

# Check for timing-safe comparison
grep -n "timingSafeEqual\|===" /tmp/pr-901.diff | grep -i "token\|hash"

# Check expiry validation
grep -n "expiresIn\|exp\|verify" /tmp/pr-901.diff

Blast Radius Assessment:

Level	Scope	Impact
CRITICAL	lib/auth/jwt.ts	All routes using this middleware will be affected
HIGH	14 route files importing requireAuth	Auth bypass or 401 on all protected endpoints
HIGH	3 services in monorepo	Cross-service authentication breaks
MEDIUM	8 integration tests	Tests may pass with old behavior, miss regressions

Minimum review requirements for this PR:

• Run npm test -- --coverage locally against the diff
• Auth integration tests must pass at 100%
• Verify algorithms: ['HS256'] is still explicitly set (prevents alg:none)
• Verify expiresIn check is not accidentally removed

PR Review: Feature flag integration (#1043)

Blast Radius: MEDIUM — 3 new services dependencies Security: 1 finding (medium) Breaking Changes: YES — missing env vars will crash the service in production

---

MUST FIX (Blocking)

[BREAKING] 3 new env vars with no fallback or startup guard

grep "^+" /tmp/pr-1043.diff | grep -oE "process\.env\.[A-Z_]+" | sort -u
# FEATURE_FLAGS_API_KEY
# FEATURE_FLAGS_URL  
# FEATURE_FLAGS_TIMEOUT

If FEATURE_FLAGS_API_KEY is undefined, what happens? Check the code:

// Current code (risky):
const client = new FeatureFlagsClient(process.env.FEATURE_FLAGS_API_KEY)
// If FEATURE_FLAGS_API_KEY is undefined, client silently uses 'undefined' as key

Required fix — startup validation:

// Validate at startup, not at runtime
const requiredEnvVars = ['FEATURE_FLAGS_API_KEY', 'FEATURE_FLAGS_URL']
for (const envVar of requiredEnvVars) {
  if (!process.env[envVar]) {
    throw new Error(`Required environment variable ${envVar} is not set`)
  }
}

Or make the feature flag client fail gracefully when unconfigured (all flags default to false) so deployment can proceed before the vars are added.

---

SHOULD FIX (Non-blocking)

[MEDIUM] FEATURE_FLAGS_TIMEOUT has no default parseInt(process.env.FEATURE_FLAGS_TIMEOUT) returns NaN if unset. Add: || 3000

---

REQUIRED DEPLOYMENT STEPS (block merge until added to PR description)

• Add FEATURE_FLAGS_API_KEY to AWS Secrets Manager + ECS task definition
• Add FEATURE_FLAGS_URL to environment config for staging and production
• Add FEATURE_FLAGS_TIMEOUT=3000 (optional, has default after your fix)
• Update .env.example with all three vars documented
• Deploy env vars to staging before merging to main

Reviewing a Database Migration PR on 15 Million Rows

A migration that renames columns across 3 tables, adds indexes, and drops a table on 15 million rows is one of the highest-risk things that goes through a PR review. The code might be two lines. The blast radius is enormous.

---

The Core Risk: Table Locks on Large Tables

Every DDL operation acquires a lock. On a live database with 15M rows:

• ALTER TABLE ... RENAME COLUMN acquires an ACCESS EXCLUSIVE lock — it blocks all reads and writes for the duration. On 15M rows this is typically milliseconds for the rename itself (it's metadata-only in most databases), but if there are long-running transactions already holding locks, your rename waits — and everything else queues behind it.
• CREATE INDEX without CONCURRENTLY also takes an ACCESS EXCLUSIVE lock and scans the entire table. On 15M rows this could be 30 seconds to several minutes of complete table unavailability.
• DROP TABLE takes an ACCESS EXCLUSIVE lock. Check for any long-running transactions that might be using the legacy table.

Questions to ask in the PR:

• What is the expected transaction duration on these tables in production? Are there any batch jobs or long-running queries that could be holding locks when this runs?
• Has this been tested against a production-sized dataset on staging?

---

The Zero-Downtime Problem: Direct Rename Is Not Safe

A direct RENAME COLUMN user_id TO account_id is not zero-downtime safe if the application code is deployed simultaneously. During a rolling deploy, some app instances will reference user_id and some will reference account_id — and the wrong one gets a column not found error.

Check if the migration follows the expand/contract pattern:

1. Expand: Add the new column account_id, populate it (backfill or trigger-based sync), keep user_id intact
2. Deploy app: Update application code to write to both columns, read from account_id
3. Contract: Drop user_id in a subsequent migration after all instances are updated

A direct rename skips expand/contract entirely. If the PR deploys the rename and the app code change simultaneously, ask whether the deploy is atomic (all instances swap simultaneously with zero traffic during the swap) or rolling. Rolling deploys with a direct rename will cause errors during the transition window.

---

Rollback Analysis

Rolling back a column rename on 15M rows:

• If rename is metadata-only (PostgreSQL), rollback is also metadata-only — fast
• However, if any data was written under the new column name before rollback, those writes are lost or need manual reconciliation
• If the app was writing to account_id before rollback, rolling the schema back leaves orphaned data

Ask the PR author: What is the rollback procedure? Has it been tested? Is there a checkpoint in the deploy sequence where rollback is safe vs. unsafe?

---

Index Creation: The CONCURRENTLY Flag

For PostgreSQL, adding an index without CONCURRENTLY locks the table for the full index build duration. On 15M rows this is a production outage.

Check the migration file for:

-- Safe (non-blocking, slower):
CREATE INDEX CONCURRENTLY idx_account_id ON table_name(account_id);

-- Unsafe on live table (blocks all reads/writes during build):
CREATE INDEX idx_account_id ON table_name(account_id);

CONCURRENTLY builds the index without holding an exclusive lock, but it requires running outside a transaction block — it cannot be wrapped in BEGIN/COMMIT. If the migration framework wraps all migrations in a transaction (which many do by default), CONCURRENTLY will fail. Confirm that the migration runner is configured to allow non-transactional DDL for this migration.

---

Dropped Table: Verification Before Merging

Before a table drop goes to production:

1. Code search: Has every reference to the legacy table been removed from the application codebase? Check ORM models, raw queries, stored procedures, views, and any analytics/reporting queries.
2. Dependency scan: Are there any foreign keys from other tables pointing to the legacy table? DROP TABLE with dependent objects will fail (or cascade-drop things you didn't intend).
3. Archived data concern: Does the legacy table contain data that is not fully migrated to its replacement? Confirm the backfill is 100% complete before dropping.
4. Retention policy: Is any data in this table subject to a legal, compliance, or audit retention requirement?

---

Comment Template for This PR

Add these as inline comments and a PR-level summary comment:

[BLOCKING] Index creation without CONCURRENTLY will lock `table_name` for the
full build duration. On 15M rows this is a production outage. Use CREATE INDEX
CONCURRENTLY, and ensure the migration runner does not wrap this in a transaction.

[BLOCKING] Direct column rename is not zero-downtime safe on a rolling deploy.
Confirm the deploy sequence is atomic, or use expand/contract (add column →
backfill → swap reads → drop old column in separate migration).

[REQUIRED] Add a verified rollback procedure to the PR description. What happens
if we need to revert after traffic has written to `account_id`?

[REQUIRED] Confirm all application code references to the legacy table have been
audited and removed before merging the DROP TABLE migration.

[RECOMMENDED] Run this against a staging environment with production-scale data
and measure actual lock wait times and index build duration before merging.