13 KiB

Raw Blame History

Pre-Landing Review Checklist

Instructions

Review the git diff origin/main output for the issues listed below. Be specific — cite file:line and suggest fixes. Skip anything that's fine. Only flag real problems.

Two-pass review:

Pass 1 (CRITICAL): Run SQL & Data Safety, Race Conditions, LLM Output Trust Boundary, Shell Injection, and Enum Completeness first. Highest severity.
Pass 2 (INFORMATIONAL): Run remaining categories below. Lower severity but still actioned.
Specialist categories (handled by parallel subagents, NOT this checklist): Test Gaps, Dead Code, Magic Numbers, Conditional Side Effects, Performance & Bundle Impact, Crypto & Entropy. See review/specialists/ for these.

All findings get action via Fix-First Review: obvious mechanical fixes are applied automatically, genuinely ambiguous issues are batched into a single user question.

Output format:

Pre-Landing Review: N issues (X critical, Y informational)

**AUTO-FIXED:**
- [file:line] Problem → fix applied

**NEEDS INPUT:**
- [file:line] Problem description
  Recommended fix: suggested fix

If no issues found: Pre-Landing Review: No issues found.

Be terse. For each issue: one line describing the problem, one line with the fix. No preamble, no summaries, no "looks good overall."

Review Categories

Pass 1 — CRITICAL

SQL & Data Safety

String interpolation in SQL (even if values are .to_i/.to_f — use parameterized queries (Rails: sanitize_sql_array/Arel; Node: prepared statements; Python: parameterized queries))
TOCTOU races: check-then-set patterns that should be atomic WHERE + update_all
Bypassing model validations for direct DB writes (Rails: update_column; Django: QuerySet.update(); Prisma: raw queries)
N+1 queries: Missing eager loading (Rails: .includes(); SQLAlchemy: joinedload(); Prisma: include) for associations used in loops/views

Race Conditions & Concurrency

Read-check-write without uniqueness constraint or catch duplicate key error and retry (e.g., where(hash:).first then save! without handling concurrent insert)
find-or-create without unique DB index — concurrent calls can create duplicates
Status transitions that don't use atomic WHERE old_status = ? UPDATE SET new_status — concurrent updates can skip or double-apply transitions
Unsafe HTML rendering (Rails: .html_safe/raw(); React: dangerouslySetInnerHTML; Vue: v-html; Django: |safe/mark_safe) on user-controlled data (XSS)

LLM Output Trust Boundary

LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (EMAIL_REGEXP, URI.parse, .strip) before persisting.
Structured tool output (arrays, hashes) accepted without type/shape checks before database writes.
LLM-generated URLs fetched without allowlist — SSRF risk if URL points to internal network (Python: urllib.parse.urlparse → check hostname against blocklist before requests.get/httpx.get)
LLM output stored in knowledge bases or vector DBs without sanitization — stored prompt injection risk

Shell Injection (Python-specific)

subprocess.run() / subprocess.call() / subprocess.Popen() with shell=True AND f-string/.format() interpolation in the command string — use argument arrays instead
os.system() with variable interpolation — replace with subprocess.run() using argument arrays
eval() / exec() on LLM-generated code without sandboxing

Enum, Value & Input-Acceptance Completeness

When the diff introduces a new enum value, status string, tier name, or type constant — OR loosens what an input accepts (a newly-accepted MIME type, file extension, or flag; a widened validator/parser/guard):

Trace it through every consumer. Read (don't just grep — READ) each file that switches on, filters by, or displays that value. If any consumer doesn't handle the new value, flag it. Common miss: adding a value to the frontend dropdown but the backend model/compute method doesn't persist it.
Check allowlists/filter arrays. Search for arrays or %w[] lists containing sibling values (e.g., if adding "revise" to tiers, find every %w[quick lfg mega] and verify "revise" is included where needed).
Check case/if-elsif chains. If existing code branches on the enum, does the new value fall through to a wrong default?
Loosened acceptance breaks UNCHANGED consumers. Widening what one function accepts often breaks a different, unchanged function downstream — which never appears in the diff. When a guard/validator/parser starts accepting a new shape, grep every consumer of that same field (MIME type, extension, status, flag) and confirm each handles it. Real miss: a client isPdfFile was loosened to accept a typeless .pdf, but the unchanged submit-path updateFileMimeType still forced every typeless file to text/plain — the PDF was sent as garbage text, not natively. To do this: use Grep to find all references to the sibling values (e.g., grep for "lfg" or "mega" to find all tier consumers). Read each match. This step requires reading code OUTSIDE the diff.

Pass 2 — INFORMATIONAL

Async/Sync Mixing (Python-specific)

Synchronous subprocess.run(), open(), requests.get() inside async def endpoints — blocks the event loop. Use asyncio.to_thread(), aiofiles, or httpx.AsyncClient instead.
time.sleep() inside async functions — use asyncio.sleep()
Sync DB calls in async context without run_in_executor() wrapping

Column/Field Name Safety

Verify column names in ORM queries (.select(), .eq(), .gte(), .order()) against actual DB schema — wrong column names silently return empty results or throw swallowed errors
Check .get() calls on query results use the column name that was actually selected
Cross-reference with schema documentation when available

Dead Code & Consistency (version/changelog only — other items handled by maintainability specialist)

Version mismatch between PR title and VERSION/CHANGELOG files
CHANGELOG entries that describe changes inaccurately (e.g., "changed from X to Y" when X never existed)

Stale User-Facing Strings

When the diff changes the CONDITION that guards a user-facing string (error message, toast, label, confirmation) WITHOUT changing the string itself, re-read the string — it can silently become misleading.

A message describing "all files" still says so after the logic starts counting only some of them (real miss: a "Total file size exceeds…" message that, after the change, sums only the non-PDF files).
This is stale-comment / CHANGELOG review applied to in-code copy: the string is unchanged, so a diff-scoped pass skims right past it.

LLM Prompt Issues

0-indexed lists in prompts (LLMs reliably return 1-indexed)
Prompt text listing available tools/capabilities that don't match what's actually wired up in the tool_classes/tools array
Word/token limits stated in multiple places that could drift

Completeness Gaps

Shortcut implementations where the complete version would cost <30 minutes CC time (e.g., partial enum handling, incomplete error paths, missing edge cases that are straightforward to add)
Options presented with only human-team effort estimates — should show both human and CC+gstack time
Test coverage gaps where adding the missing tests is a "lake" not an "ocean" (e.g., missing negative-path tests, missing edge case tests that mirror happy-path structure)
Features implemented at 80-90% when 100% is achievable with modest additional code

Time Window Safety

Date-key lookups that assume "today" covers 24h — report at 8am PT only sees midnight→8am under today's key
Mismatched time windows between related features — one uses hourly buckets, another uses daily keys for the same data

Type Coercion at Boundaries

Values crossing Ruby→JSON→JS boundaries where type could change (numeric vs string) — hash/digest inputs must normalize types
Hash/digest inputs that don't call .to_s or equivalent before serialization — { cores: 8 } vs { cores: "8" } produce different hashes

View/Frontend

Inline <style> blocks in partials (re-parsed every render)
O(n*m) lookups in views (Array#find in a loop instead of index_by hash)
Ruby-side .select{} filtering on DB results that could be a WHERE clause (unless intentionally avoiding leading-wildcard LIKE)

Distribution & CI/CD Pipeline

CI/CD workflow changes (.github/workflows/): verify build tool versions match project requirements, artifact names/paths are correct, secrets use ${{ secrets.X }} not hardcoded values
New artifact types (CLI binary, library, package): verify a publish/release workflow exists and targets correct platforms
Cross-platform builds: verify CI matrix covers all target OS/arch combinations, or documents which are untested
Version tag format consistency: v1.2.3 vs 1.2.3 — must match across VERSION file, git tags, and publish scripts
Publish step idempotency: re-running the publish workflow should not fail (e.g., gh release delete before gh release create)

DO NOT flag:

Web services with existing auto-deploy pipelines (Docker build + K8s deploy)
Internal tools not distributed outside the team
Test-only CI changes (adding test steps, not publish steps)

Severity Classification

CRITICAL (highest severity):      INFORMATIONAL (main agent):      SPECIALIST (parallel subagents):
├─ SQL & Data Safety              ├─ Async/Sync Mixing             ├─ Testing specialist
├─ Race Conditions & Concurrency  ├─ Column/Field Name Safety      ├─ Maintainability specialist
├─ LLM Output Trust Boundary      ├─ Dead Code (version only)      ├─ Security specialist
├─ Shell Injection                ├─ LLM Prompt Issues             ├─ Performance specialist
└─ Enum & Value Completeness      ├─ Completeness Gaps             ├─ Data Migration specialist
                                   ├─ Time Window Safety            ├─ API Contract specialist
                                   ├─ Type Coercion at Boundaries   └─ Red Team (conditional)
                                   ├─ View/Frontend
                                   └─ Distribution & CI/CD Pipeline

All findings are actioned via Fix-First Review. Severity determines
presentation order and classification of AUTO-FIX vs ASK — critical
findings lean toward ASK (they're riskier), informational findings
lean toward AUTO-FIX (they're more mechanical).

Fix-First Heuristic

This heuristic is referenced by both /review and /ship. It determines whether the agent auto-fixes a finding or asks the user.

AUTO-FIX (agent fixes without asking):     ASK (needs human judgment):
├─ Dead code / unused variables            ├─ Security (auth, XSS, injection)
├─ N+1 queries (missing eager loading)      ├─ Race conditions
├─ Stale comments contradicting code       ├─ Design decisions
├─ Magic numbers → named constants         ├─ Large fixes (>20 lines)
├─ Missing LLM output validation           ├─ Enum completeness
├─ Version/path mismatches                 ├─ Removing functionality
├─ Variables assigned but never read       └─ Anything changing user-visible
└─ Inline styles, O(n*m) view lookups        behavior

Rule of thumb: If the fix is mechanical and a senior engineer would apply it without discussion, it's AUTO-FIX. If reasonable engineers could disagree about the fix, it's ASK.

Critical findings default toward ASK (they're inherently riskier). Informational findings default toward AUTO-FIX (they're more mechanical).

Suppressions — DO NOT flag these

"X is redundant with Y" when the redundancy is harmless and aids readability (e.g., present? redundant with length > 20)
"Add a comment explaining why this threshold/constant was chosen" — thresholds change during tuning, comments rot
"This assertion could be tighter" when the assertion already covers the behavior
Suggesting consistency-only changes (wrapping a value in a conditional to match how another constant is guarded)
"Regex doesn't handle edge case X" when the input is constrained and X never occurs in practice
"Test exercises multiple guards simultaneously" — that's fine, tests don't need to isolate every guard
Eval threshold changes (max_actionable, min scores) — these are tuned empirically and change constantly
Harmless no-ops (e.g., .reject on an element that's never in the array)
ANYTHING already addressed in the diff you're reviewing — read the FULL diff before commenting

13 KiB Raw Blame History