mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-01 19:25:10 +02:00
Garry Tan
126 lines
6.7 KiB
Markdown
126 lines
6.7 KiB
Markdown
# 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 and LLM Output Trust Boundary first. These can block `/ship`.
|
|
- **Pass 2 (INFORMATIONAL):** Run all remaining categories. These are included in the PR body but do not block.
|
|
|
|
**Output format:**
|
|
|
|
```
|
|
Pre-Landing Review: N issues (X critical, Y informational)
|
|
|
|
**CRITICAL** (blocking /ship):
|
|
- [file:line] Problem description
|
|
Fix: suggested fix
|
|
|
|
**Issues** (non-blocking):
|
|
- [file:line] Problem description
|
|
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 `sanitize_sql_array` or Arel)
|
|
- TOCTOU races: check-then-set patterns that should be atomic `WHERE` + `update_all`
|
|
- `update_column`/`update_columns` bypassing validations on fields that have or should have constraints
|
|
- N+1 queries: `.includes()` missing for associations used in loops/views (especially avatar, attachments)
|
|
|
|
#### Race Conditions & Concurrency
|
|
- Read-check-write without uniqueness constraint or `rescue RecordNotUnique; retry` (e.g., `where(hash:).first` then `save!` without handling concurrent insert)
|
|
- `find_or_create_by` on columns 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
|
|
- `html_safe` on user-controlled data (XSS) — check any `.html_safe`, `raw()`, or string interpolation into `html_safe` output
|
|
|
|
#### 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.
|
|
|
|
### Pass 2 — INFORMATIONAL
|
|
|
|
#### Conditional Side Effects
|
|
- Code paths that branch on a condition but forget to apply a side effect on one branch. Example: item promoted to verified but URL only attached when a secondary condition is true — the other branch promotes without the URL, creating an inconsistent record.
|
|
- Log messages that claim an action happened but the action was conditionally skipped. The log should reflect what actually occurred.
|
|
|
|
#### Magic Numbers & String Coupling
|
|
- Bare numeric literals used in multiple files — should be named constants documented together
|
|
- Error message strings used as query filters elsewhere (grep for the string — is anything matching on it?)
|
|
|
|
#### Dead Code & Consistency
|
|
- Variables assigned but never read
|
|
- 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)
|
|
- Comments/docstrings that describe old behavior after the code changed
|
|
|
|
#### 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
|
|
|
|
#### Test Gaps
|
|
- Negative-path tests that assert type/status but not the side effects (URL attached? field populated? callback fired?)
|
|
- Assertions on string content without checking format (e.g., asserting title present but not URL format)
|
|
- `.expects(:something).never` missing when a code path should explicitly NOT call an external service
|
|
- Security enforcement features (blocking, rate limiting, auth) without integration tests verifying the enforcement path works end-to-end
|
|
|
|
#### Crypto & Entropy
|
|
- Truncation of data instead of hashing (last N chars instead of SHA-256) — less entropy, easier collisions
|
|
- `rand()` / `Random.rand` for security-sensitive values — use `SecureRandom` instead
|
|
- Non-constant-time comparisons (`==`) on secrets or tokens — vulnerable to timing attacks
|
|
|
|
#### 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`)
|
|
|
|
---
|
|
|
|
## Gate Classification
|
|
|
|
```
|
|
CRITICAL (blocks /ship): INFORMATIONAL (in PR body):
|
|
├─ SQL & Data Safety ├─ Conditional Side Effects
|
|
├─ Race Conditions & Concurrency ├─ Magic Numbers & String Coupling
|
|
└─ LLM Output Trust Boundary ├─ Dead Code & Consistency
|
|
├─ LLM Prompt Issues
|
|
├─ Test Gaps
|
|
├─ Crypto & Entropy
|
|
├─ Time Window Safety
|
|
├─ Type Coercion at Boundaries
|
|
└─ View/Frontend
|
|
```
|
|
|
|
---
|
|
|
|
## 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
|