gstack-review

name: gstack-reviewdescription: "Use before merging a branch or PR: review the diff for bugs that pass CI but break production, fix the obvious ones and ask about the rest. (gstack)"

Review: find what passes CI but breaks in production

Start here. In the repository (cd into it first: the shell starts in the workspace root), run with execute (pass the base branch if you know it):

sh skills/gstack-review/scripts/diff-base.sh <base>

It prints the base, the merge base, the commits since the base and the diff stat, then NEXT: git diff <merge base>; run that and read the whole diff and every untracked file before you comment (on UNTRACKED_MORE:, list the rest with the command it names). On NOTHING_TO_REVIEW, say so and stop. On BASE: none, ask the user for the base branch or review the uncommitted changes it shows.

QM fills in the absolute path of the folder its skill tool synced (it prints that path too). If the script is missing, run this instead:

git merge-base origin/main HEAD
git diff --stat <merge-base>

How to review

1. Read the full diff, then the unchanged callers of anything it changes.
2. Apply the CRITICAL categories, then the INFORMATIONAL ones, below. Respect the suppressions.
3. Classify each finding with the Fix-First Heuristic: fix AUTO-FIX items directly; for ASK items, ask the user in one message (a numbered list, each with a recommended fix) and stop until they answer.
4. Verify each claim (below), then report: CRITICAL and INFORMATIONAL lists, one line problem and one line fix each, with file:line, then what you fixed. NO FINDINGS is a complete result.

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 Completeness

When the diff introduces a new enum value, status string, tier name, or type constant, or loosens what an input accepts (a new MIME type, extension or flag, or 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. Grep every consumer of the same field and confirm each handles the newly accepted shape; the broken code is usually outside the diff. 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

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)
• 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

Timing-Dependent Tests

• A diff that raises an existing timing bound or adds a retry to make a flake pass: the bound is not the cause; find its cause first (gstack-investigate)

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)

Fix-First Heuristic

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

Zero findings is a valid outcome. Flag only what a senior engineer on this team would change in review; if the diff is clean, say so.

• "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
• 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
• "Consider adding error handling" on a call whose errors the caller or framework already owns (error middleware, an error boundary, a top-level try/catch, an upstream .catch)
• "Function too long" for an exhaustive switch, a config object, a test table or generated code — length is not complexity
• "Possible null dereference" when a type guard or if check in scope already narrows the value — trace the type flow
• "N+1 query" on a loop with a small fixed cardinality, or on a path that is already batched
• "Hardcoded value" in test fixtures, example code or docs — tests are supposed to hardcode their expectations
• Math.random() in a non-cryptographic context (jitter, sampling, animation) — security-sensitive randomness is still flagged

Verification of claims

Before final output, cite the line proving a safety claim, read and cite any handling code you rely on, and name the test file and method for coverage claims. Verify claims or flag them as unknown; "this looks fine" is not evidence.

Important Rules

• Read the FULL diff before commenting. Do not flag issues already addressed in the diff.
• Fix-first, not read-only. AUTO-FIX items are applied directly, ASK items only after user approval. Never commit, push, or create PRs — leave that to the user.
• Be terse. One line problem, one line fix. No preamble.
• Only flag real problems. Skip anything that's fine.