mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-08 23:36:06 +02:00
Merge branch 'main' into garrytan/team-supabase-store
Brings in 48 commits from main (v0.15.7–v0.15.16): deterministic slugs, TabSession refactor, pair-agent tunnel fix, content security layers, community security wave, team-friendly install, interactive snapshots. Conflict resolution: - .gitignore: merged both sides (kept .factory/ + added .kiro/.opencode/ .slate/.cursor/.openclaw/ from main) - open-gstack-browser/SKILL.md: accepted main (renamed from .factory/) - setup-team-sync/SKILL.md: regenerated via gen:skill-docs - test/fixtures/golden/*: updated golden baselines for ship SKILL.md - codex-ship-SKILL.md: accepted main (renamed from .factory/) - package.json version: synced to VERSION (0.15.16.0) - bin/gstack-uninstall: check settings file exists before claiming SessionStart hook removal (fixes false positive on clean systems) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Garry Tan
+660
-331
File diff suppressed because it is too large
Load Diff
+25
-53
@@ -6,7 +6,7 @@ description: |
|
||||
Pre-landing PR review. Analyzes diff against the base branch for SQL safety, LLM trust
|
||||
boundary violations, conditional side effects, and other structural issues. Use when
|
||||
asked to "review this PR", "code review", "pre-landing review", or "check my diff".
|
||||
Proactively suggest when the user is about to merge or land code changes.
|
||||
Proactively suggest when the user is about to merge or land code changes. (gstack)
|
||||
allowed-tools:
|
||||
- Bash
|
||||
- Read
|
||||
@@ -37,43 +37,10 @@ You are running the `/review` workflow. Analyze the current branch's diff agains
|
||||
|
||||
---
|
||||
|
||||
## Step 1.5: Scope Drift Detection
|
||||
|
||||
Before reviewing code quality, check: **did they build what was requested — nothing more, nothing less?**
|
||||
|
||||
1. Read `TODOS.md` (if it exists). Read PR description (`gh pr view --json body --jq .body 2>/dev/null || true`).
|
||||
Read commit messages (`git log origin/<base>..HEAD --oneline`).
|
||||
**If no PR exists:** rely on commit messages and TODOS.md for stated intent — this is the common case since /review runs before /ship creates the PR.
|
||||
2. Identify the **stated intent** — what was this branch supposed to accomplish?
|
||||
3. Run `git diff origin/<base>...HEAD --stat` and compare the files changed against the stated intent.
|
||||
{{SCOPE_DRIFT}}
|
||||
|
||||
{{PLAN_COMPLETION_AUDIT_REVIEW}}
|
||||
|
||||
4. Evaluate with skepticism (incorporating plan completion results if available):
|
||||
|
||||
**SCOPE CREEP detection:**
|
||||
- Files changed that are unrelated to the stated intent
|
||||
- New features or refactors not mentioned in the plan
|
||||
- "While I was in there..." changes that expand blast radius
|
||||
|
||||
**MISSING REQUIREMENTS detection:**
|
||||
- Requirements from TODOS.md/PR description not addressed in the diff
|
||||
- Test coverage gaps for stated requirements
|
||||
- Partial implementations (started but not finished)
|
||||
|
||||
5. Output (before the main review begins):
|
||||
```
|
||||
Scope Check: [CLEAN / DRIFT DETECTED / REQUIREMENTS MISSING]
|
||||
Intent: <1-line summary of what was requested>
|
||||
Delivered: <1-line summary of what the diff actually does>
|
||||
[If drift: list each out-of-scope change]
|
||||
[If missing: list each unaddressed requirement]
|
||||
```
|
||||
|
||||
6. This is **INFORMATIONAL** — does not block the review. Proceed to Step 2.
|
||||
|
||||
---
|
||||
|
||||
## Step 2: Read the checklist
|
||||
|
||||
Read `.claude/skills/review/checklist.md`.
|
||||
@@ -104,12 +71,14 @@ Run `git diff origin/<base>` to get the full diff. This includes both committed
|
||||
|
||||
---
|
||||
|
||||
## Step 4: Two-pass review
|
||||
{{LEARNINGS_SEARCH}}
|
||||
|
||||
Apply the checklist against the diff in two passes:
|
||||
## Step 4: Critical pass (core review)
|
||||
|
||||
1. **Pass 1 (CRITICAL):** SQL & Data Safety, Race Conditions & Concurrency, LLM Output Trust Boundary, Enum & Value Completeness
|
||||
2. **Pass 2 (INFORMATIONAL):** Conditional Side Effects, Magic Numbers & String Coupling, Dead Code & Consistency, LLM Prompt Issues, Test Gaps, View/Frontend, Performance & Bundle Impact
|
||||
Apply the CRITICAL categories from the checklist against the diff:
|
||||
SQL & Data Safety, Race Conditions & Concurrency, LLM Output Trust Boundary, Shell Injection, Enum & Value Completeness.
|
||||
|
||||
Also apply the remaining INFORMATIONAL categories that are still in the checklist (Async/Sync Mixing, Column/Field Name Safety, LLM Prompt Issues, Type Coercion, View/Frontend, Time Window Safety, Completeness Gaps, Distribution & CI/CD).
|
||||
|
||||
**Enum & Value Completeness requires reading code OUTSIDE the diff.** When the diff introduces a new enum value, status, tier, or type constant, use Grep to find all files that reference sibling values, then Read those files to check if the new value is handled. This is the one category where within-diff review is insufficient.
|
||||
|
||||
@@ -122,21 +91,11 @@ Takes seconds, prevents recommending outdated patterns. If WebSearch is unavaila
|
||||
|
||||
Follow the output format specified in the checklist. Respect the suppressions — do NOT flag items listed in the "DO NOT flag" section.
|
||||
|
||||
---
|
||||
|
||||
## Step 4.5: Design Review (conditional)
|
||||
|
||||
{{DESIGN_REVIEW_LITE}}
|
||||
|
||||
Include any design findings alongside the findings from Step 4. They follow the same Fix-First flow in Step 5 — AUTO-FIX for mechanical CSS fixes, ASK for everything else.
|
||||
{{CONFIDENCE_CALIBRATION}}
|
||||
|
||||
---
|
||||
|
||||
## Step 4.75: Test Coverage Diagram
|
||||
|
||||
{{TEST_COVERAGE_AUDIT_REVIEW}}
|
||||
|
||||
This step subsumes the "Test Gaps" category from Pass 2 — do not duplicate findings between the checklist Test Gaps item and this coverage diagram. Include any coverage gaps alongside the findings from Step 4 and Step 4.5. They follow the same Fix-First flow — gaps are INFORMATIONAL findings.
|
||||
{{REVIEW_ARMY}}
|
||||
|
||||
---
|
||||
|
||||
@@ -144,7 +103,7 @@ This step subsumes the "Test Gaps" category from Pass 2 — do not duplicate fin
|
||||
|
||||
**Every finding gets action — not just critical ones.**
|
||||
|
||||
Output a summary header: `Pre-Landing Review: N issues (X critical, Y informational)`
|
||||
{{CROSS_REVIEW_DEDUP}}
|
||||
|
||||
### Step 5a: Classify each finding
|
||||
|
||||
@@ -152,6 +111,14 @@ For each finding, classify as AUTO-FIX or ASK per the Fix-First Heuristic in
|
||||
checklist.md. Critical findings lean toward ASK; informational findings lean
|
||||
toward AUTO-FIX.
|
||||
|
||||
**Test stub override:** Any finding that has a `test_stub` field (generated by a specialist)
|
||||
is reclassified as ASK regardless of its original classification. When presenting the ASK
|
||||
item, show the proposed test file path and the test code. The user approves or skips the
|
||||
test creation. If approved, write the fix + test file. Derive the test file path from
|
||||
the finding's `path` using project conventions (`spec/` for RSpec, `__tests__/` for
|
||||
Jest/Vitest, `test_` prefix for pytest, `_test.go` suffix for Go). If the test file
|
||||
already exists, append the new test. Output: `[FIXED + TEST] [file:line] Problem -> fix + test at [test_path]`
|
||||
|
||||
### Step 5b: Auto-fix all AUTO-FIX items
|
||||
|
||||
Apply each fix directly. For each one, output a one-line summary:
|
||||
@@ -262,7 +229,7 @@ recognize that Eng Review was run on this branch.
|
||||
Run:
|
||||
|
||||
```bash
|
||||
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"commit":"COMMIT"}'
|
||||
~/.claude/skills/gstack/bin/gstack-review-log '{"skill":"review","timestamp":"TIMESTAMP","status":"STATUS","issues_found":N,"critical":N,"informational":N,"quality_score":SCORE,"specialists":SPECIALISTS_JSON,"findings":FINDINGS_JSON,"commit":"COMMIT"}'
|
||||
```
|
||||
|
||||
Substitute:
|
||||
@@ -271,8 +238,13 @@ Substitute:
|
||||
- `issues_found` = total remaining unresolved findings
|
||||
- `critical` = remaining unresolved critical findings
|
||||
- `informational` = remaining unresolved informational findings
|
||||
- `quality_score` = the PR Quality Score computed in Step 4.6 (e.g., 7.5). If specialists were skipped (small diff), use `10.0`
|
||||
- `specialists` = the per-specialist stats object compiled in Step 4.6. Each specialist that was considered gets an entry: `{"dispatched":true/false,"findings":N,"critical":N,"informational":N}` if dispatched, or `{"dispatched":false,"reason":"scope|gated"}` if skipped. Include Design specialist. Example: `{"testing":{"dispatched":true,"findings":2,"critical":0,"informational":2},"security":{"dispatched":false,"reason":"scope"}}`
|
||||
- `findings` = array of per-finding records from Step 5. For each finding (from critical pass and specialists), include: `{"fingerprint":"path:line:category","severity":"CRITICAL|INFORMATIONAL","action":"ACTION"}`. ACTION is `"auto-fixed"` (Step 5b), `"fixed"` (user approved in Step 5d), or `"skipped"` (user chose Skip in Step 5c). Suppressed findings from Step 5.0 are NOT included (they were already recorded in a prior review entry).
|
||||
- `COMMIT` = output of `git rev-parse --short HEAD`
|
||||
|
||||
{{LEARNINGS_LOG}}
|
||||
|
||||
If the review exits early before a real review completes (for example, no diff against the base branch), do **not** write this entry.
|
||||
|
||||
## Important Rules
|
||||
|
||||
+12
-52
@@ -5,8 +5,9 @@
|
||||
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. Highest severity.
|
||||
- **Pass 2 (INFORMATIONAL):** Run all remaining categories. Lower severity but still actioned.
|
||||
- **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.
|
||||
@@ -76,42 +77,21 @@ To do this: use Grep to find all references to the sibling values (e.g., grep fo
|
||||
- Check `.get()` calls on query results use the column name that was actually selected
|
||||
- Cross-reference with schema documentation when available
|
||||
|
||||
#### 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
|
||||
#### 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)
|
||||
- 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
|
||||
|
||||
#### 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
|
||||
|
||||
#### 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
|
||||
@@ -125,23 +105,6 @@ To do this: use Grep to find all references to the sibling values (e.g., grep fo
|
||||
- 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`)
|
||||
|
||||
#### Performance & Bundle Impact
|
||||
- New `dependencies` entries in package.json that are known-heavy: moment.js (→ date-fns, 330KB→22KB), lodash full (→ lodash-es or per-function imports), jquery, core-js full polyfill
|
||||
- Significant lockfile growth (many new transitive dependencies from a single addition)
|
||||
- Images added without `loading="lazy"` or explicit width/height attributes (causes layout shift / CLS)
|
||||
- Large static assets committed to repo (>500KB per file)
|
||||
- Synchronous `<script>` tags without async/defer
|
||||
- CSS `@import` in stylesheets (blocks parallel loading — use bundler imports instead)
|
||||
- `useEffect` with fetch that depends on another fetch result (request waterfall — combine or parallelize)
|
||||
- Named → default import switches on tree-shakeable libraries (breaks tree-shaking)
|
||||
- New `require()` calls in ESM codebases
|
||||
|
||||
**DO NOT flag:**
|
||||
- devDependencies additions (don't affect production bundle)
|
||||
- Dynamic `import()` calls (code splitting — these are good)
|
||||
- Small utility additions (<5KB gzipped)
|
||||
- Server-side-only dependencies
|
||||
|
||||
#### 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
|
||||
@@ -159,18 +122,15 @@ To do this: use Grep to find all references to the sibling values (e.g., grep fo
|
||||
## Severity Classification
|
||||
|
||||
```
|
||||
CRITICAL (highest severity): INFORMATIONAL (lower severity):
|
||||
├─ SQL & Data Safety ├─ Conditional Side Effects
|
||||
├─ Race Conditions & Concurrency ├─ Magic Numbers & String Coupling
|
||||
├─ LLM Output Trust Boundary ├─ Dead Code & Consistency
|
||||
└─ Enum & Value Completeness ├─ LLM Prompt Issues
|
||||
├─ Test Gaps
|
||||
├─ Completeness Gaps
|
||||
├─ Crypto & Entropy
|
||||
├─ Time Window Safety
|
||||
├─ Type Coercion at Boundaries
|
||||
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
|
||||
├─ Performance & Bundle Impact
|
||||
└─ Distribution & CI/CD Pipeline
|
||||
|
||||
All findings are actioned via Fix-First Review. Severity determines
|
||||
|
||||
@@ -58,6 +58,8 @@ Design Review: N issues (X auto-fixable, Y need input, Z possible)
|
||||
- [file:line] Possible issue — verify with /design-review
|
||||
```
|
||||
|
||||
Optional: `test_stub` — skeleton test code for this finding using the project's test framework.
|
||||
|
||||
If no issues found: `Design Review: No issues found.`
|
||||
|
||||
If no frontend files changed: skip silently, no output.
|
||||
|
||||
@@ -0,0 +1,49 @@
|
||||
# API Contract Specialist Review Checklist
|
||||
|
||||
Scope: When SCOPE_API=true
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"api-contract","summary":"...","fix":"...","fingerprint":"path:line:api-contract","specialist":"api-contract"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
## Categories
|
||||
|
||||
### Breaking Changes
|
||||
- Removed fields from response bodies (clients may depend on them)
|
||||
- Changed field types (string → number, object → array)
|
||||
- New required parameters added to existing endpoints
|
||||
- Changed HTTP methods (GET → POST) or status codes (200 → 201)
|
||||
- Renamed endpoints without maintaining the old path as a redirect/alias
|
||||
- Changed authentication requirements (public → authenticated)
|
||||
|
||||
### Versioning Strategy
|
||||
- Breaking changes made without a version bump (v1 → v2)
|
||||
- Multiple versioning strategies mixed in the same API (URL vs header vs query param)
|
||||
- Deprecated endpoints without a sunset timeline or migration guide
|
||||
- Version-specific logic scattered across controllers instead of centralized
|
||||
|
||||
### Error Response Consistency
|
||||
- New endpoints returning different error formats than existing ones
|
||||
- Error responses missing standard fields (error code, message, details)
|
||||
- HTTP status codes that don't match the error type (200 for errors, 500 for validation)
|
||||
- Error messages that leak internal implementation details (stack traces, SQL)
|
||||
|
||||
### Rate Limiting & Pagination
|
||||
- New endpoints missing rate limiting when similar endpoints have it
|
||||
- Pagination changes (offset → cursor) without backwards compatibility
|
||||
- Changed page sizes or default limits without documentation
|
||||
- Missing total count or next-page indicators in paginated responses
|
||||
|
||||
### Documentation Drift
|
||||
- OpenAPI/Swagger spec not updated to match new endpoints or changed params
|
||||
- README or API docs describing old behavior after changes
|
||||
- Example requests/responses that no longer work
|
||||
- Missing documentation for new endpoints or changed parameters
|
||||
|
||||
### Backwards Compatibility
|
||||
- Clients on older versions: will they break?
|
||||
- Mobile apps that can't force-update: does the API still work for them?
|
||||
- Webhook payloads changed without notifying subscribers
|
||||
- SDK or client library changes needed to use new features
|
||||
@@ -0,0 +1,48 @@
|
||||
# Data Migration Specialist Review Checklist
|
||||
|
||||
Scope: When SCOPE_MIGRATIONS=true
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"data-migration","summary":"...","fix":"...","fingerprint":"path:line:data-migration","specialist":"data-migration"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
## Categories
|
||||
|
||||
### Reversibility
|
||||
- Can this migration be rolled back without data loss?
|
||||
- Is there a corresponding down/rollback migration?
|
||||
- Does the rollback actually undo the change or just no-op?
|
||||
- Would rolling back break the current application code?
|
||||
|
||||
### Data Loss Risk
|
||||
- Dropping columns that still contain data (add deprecation period first)
|
||||
- Changing column types that truncate data (varchar(255) → varchar(50))
|
||||
- Removing tables without verifying no code references them
|
||||
- Renaming columns without updating all references (ORM, raw SQL, views)
|
||||
- NOT NULL constraints added to columns with existing NULL values (needs backfill first)
|
||||
|
||||
### Lock Duration
|
||||
- ALTER TABLE on large tables without CONCURRENTLY (PostgreSQL)
|
||||
- Adding indexes without CONCURRENTLY on tables with >100K rows
|
||||
- Multiple ALTER TABLE statements that could be combined into one lock acquisition
|
||||
- Schema changes that acquire exclusive locks during peak traffic hours
|
||||
|
||||
### Backfill Strategy
|
||||
- New NOT NULL columns without DEFAULT value (requires backfill before constraint)
|
||||
- New columns with computed defaults that need batch population
|
||||
- Missing backfill script or rake task for existing records
|
||||
- Backfill that updates all rows at once instead of batching (locks table)
|
||||
|
||||
### Index Creation
|
||||
- CREATE INDEX without CONCURRENTLY on production tables
|
||||
- Duplicate indexes (new index covers same columns as existing one)
|
||||
- Missing indexes on new foreign key columns
|
||||
- Partial indexes where a full index would be more useful (or vice versa)
|
||||
|
||||
### Multi-Phase Safety
|
||||
- Migrations that must be deployed in a specific order with application code
|
||||
- Schema changes that break the current running code (deploy code first, then migrate)
|
||||
- Migrations that assume a deploy boundary (old code + new schema = crash)
|
||||
- Missing feature flag to handle mixed old/new code during rolling deploy
|
||||
@@ -0,0 +1,46 @@
|
||||
# Maintainability Specialist Review Checklist
|
||||
|
||||
Scope: Always-on (every review)
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"maintainability","summary":"...","fix":"...","fingerprint":"path:line:maintainability","specialist":"maintainability"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
## Categories
|
||||
|
||||
### Dead Code & Unused Imports
|
||||
- Variables assigned but never read in the changed files
|
||||
- Functions/methods defined but never called (check with Grep across the repo)
|
||||
- Imports/requires that are no longer referenced after the change
|
||||
- Commented-out code blocks (either remove or explain why they exist)
|
||||
|
||||
### Magic Numbers & String Coupling
|
||||
- Bare numeric literals used in logic (thresholds, limits, retry counts) — should be named constants
|
||||
- Error message strings used as query filters or conditionals elsewhere
|
||||
- Hardcoded URLs, ports, or hostnames that should be config
|
||||
- Duplicated literal values across multiple files
|
||||
|
||||
### Stale Comments & Docstrings
|
||||
- Comments that describe old behavior after the code was changed in this diff
|
||||
- TODO/FIXME comments that reference completed work
|
||||
- Docstrings with parameter lists that don't match the current function signature
|
||||
- ASCII diagrams in comments that no longer match the code flow
|
||||
|
||||
### DRY Violations
|
||||
- Similar code blocks (3+ lines) appearing multiple times within the diff
|
||||
- Copy-paste patterns where a shared helper would be cleaner
|
||||
- Configuration or setup logic duplicated across test files
|
||||
- Repeated conditional chains that could be a lookup table or map
|
||||
|
||||
### Conditional Side Effects
|
||||
- Code paths that branch on a condition but forget a side effect on one branch
|
||||
- Log messages that claim an action happened but the action was conditionally skipped
|
||||
- State transitions where one branch updates related records but the other doesn't
|
||||
- Event emissions that only fire on the happy path, missing error/edge paths
|
||||
|
||||
### Module Boundary Violations
|
||||
- Reaching into another module's internal implementation (accessing private-by-convention methods)
|
||||
- Direct database queries in controllers/views that should go through a service/model
|
||||
- Tight coupling between components that should communicate through interfaces
|
||||
@@ -0,0 +1,52 @@
|
||||
# Performance Specialist Review Checklist
|
||||
|
||||
Scope: When SCOPE_BACKEND=true OR SCOPE_FRONTEND=true
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"performance","summary":"...","fix":"...","fingerprint":"path:line:performance","specialist":"performance"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
## Categories
|
||||
|
||||
### N+1 Queries
|
||||
- ActiveRecord/ORM associations traversed in loops without eager loading (.includes, joinedload, include)
|
||||
- Database queries inside iteration blocks (each, map, forEach) that could be batched
|
||||
- Nested serializers that trigger lazy-loaded associations
|
||||
- GraphQL resolvers that query per-field instead of batching (check for DataLoader usage)
|
||||
|
||||
### Missing Database Indexes
|
||||
- New WHERE clauses on columns without indexes (check migration files or schema)
|
||||
- New ORDER BY on non-indexed columns
|
||||
- Composite queries (WHERE a AND b) without composite indexes
|
||||
- Foreign key columns added without indexes
|
||||
|
||||
### Algorithmic Complexity
|
||||
- O(n^2) or worse patterns: nested loops over collections, Array.find inside Array.map
|
||||
- Repeated linear searches that could use a hash/map/set lookup
|
||||
- String concatenation in loops (use join or StringBuilder)
|
||||
- Sorting or filtering large collections multiple times when once would suffice
|
||||
|
||||
### Bundle Size Impact (Frontend)
|
||||
- New production dependencies that are known-heavy (moment.js, lodash full, jquery)
|
||||
- Barrel imports (import from 'library') instead of deep imports (import from 'library/specific')
|
||||
- Large static assets (images, fonts) committed without optimization
|
||||
- Missing code splitting for route-level chunks
|
||||
|
||||
### Rendering Performance (Frontend)
|
||||
- Fetch waterfalls: sequential API calls that could be parallel (Promise.all)
|
||||
- Unnecessary re-renders from unstable references (new objects/arrays in render)
|
||||
- Missing React.memo, useMemo, or useCallback on expensive computations
|
||||
- Layout thrashing from reading then writing DOM properties in loops
|
||||
- Missing loading="lazy" on below-fold images
|
||||
|
||||
### Missing Pagination
|
||||
- List endpoints that return unbounded results (no LIMIT, no pagination params)
|
||||
- Database queries without LIMIT that grow with data volume
|
||||
- API responses that embed full nested objects instead of IDs with expansion
|
||||
|
||||
### Blocking in Async Contexts
|
||||
- Synchronous I/O (file reads, subprocess, HTTP requests) inside async functions
|
||||
- time.sleep() / Thread.sleep() inside event-loop-based handlers
|
||||
- CPU-intensive computation blocking the main thread without worker offload
|
||||
@@ -0,0 +1,45 @@
|
||||
# Red Team Review
|
||||
|
||||
Scope: When diff > 200 lines OR security specialist found CRITICAL findings. Runs AFTER other specialists.
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"red-team","summary":"...","fix":"...","fingerprint":"path:line:red-team","specialist":"red-team"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
This is NOT a checklist review. This is adversarial analysis.
|
||||
|
||||
You have access to the other specialists' findings (provided in your prompt). Your job is to find what they MISSED. Think like an attacker, a chaos engineer, and a hostile QA tester simultaneously.
|
||||
|
||||
## Approach
|
||||
|
||||
### 1. Attack the Happy Path
|
||||
- What happens when the system is under 10x normal load?
|
||||
- What happens when two requests hit the same resource simultaneously?
|
||||
- What happens when the database is slow (>5s query time)?
|
||||
- What happens when an external service returns garbage?
|
||||
|
||||
### 2. Find the Silent Failures
|
||||
- Error handling that swallows exceptions (catch-all with just a log)
|
||||
- Operations that can partially complete (3 of 5 items processed, then crash)
|
||||
- State transitions that leave records in inconsistent states on failure
|
||||
- Background jobs that fail without alerting anyone
|
||||
|
||||
### 3. Exploit Trust Assumptions
|
||||
- Data validated on the frontend but not the backend
|
||||
- Internal APIs called without authentication (assuming "only our code calls this")
|
||||
- Configuration values assumed to be present but not validated
|
||||
- File paths or URLs constructed from user input without sanitization
|
||||
|
||||
### 4. Break the Edge Cases
|
||||
- What happens with the maximum possible input size?
|
||||
- What happens with zero items, empty strings, null values?
|
||||
- What happens on the first run ever (no existing data)?
|
||||
- What happens when the user clicks the button twice in 100ms?
|
||||
|
||||
### 5. Find What the Other Specialists Missed
|
||||
- Review each specialist's findings. What's the gap between their categories?
|
||||
- Look for cross-category issues (e.g., a performance issue that's also a security issue)
|
||||
- Look for issues at integration boundaries (where two systems meet)
|
||||
- Look for issues that only manifest in specific deployment configurations
|
||||
@@ -0,0 +1,61 @@
|
||||
# Security Specialist Review Checklist
|
||||
|
||||
Scope: When SCOPE_AUTH=true OR (SCOPE_BACKEND=true AND diff > 100 lines)
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"security","summary":"...","fix":"...","fingerprint":"path:line:security","specialist":"security"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
This checklist goes deeper than the main CRITICAL pass. The main agent already checks SQL injection, race conditions, LLM trust, and enum completeness. This specialist focuses on auth/authz patterns, cryptographic misuse, and attack surface expansion.
|
||||
|
||||
## Categories
|
||||
|
||||
### Input Validation at Trust Boundaries
|
||||
- User input accepted without validation at controller/handler level
|
||||
- Query parameters used directly in database queries or file paths
|
||||
- Request body fields accepted without type checking or schema validation
|
||||
- File uploads without type/size/content validation
|
||||
- Webhook payloads processed without signature verification
|
||||
|
||||
### Auth & Authorization Bypass
|
||||
- Endpoints missing authentication middleware (check route definitions)
|
||||
- Authorization checks that default to "allow" instead of "deny"
|
||||
- Role escalation paths (user can modify their own role/permissions)
|
||||
- Direct object reference vulnerabilities (user A accesses user B's data by changing an ID)
|
||||
- Session fixation or session hijacking opportunities
|
||||
- Token/API key validation that doesn't check expiration
|
||||
|
||||
### Injection Vectors (beyond SQL)
|
||||
- Command injection via subprocess calls with user-controlled arguments
|
||||
- Template injection (Jinja2, ERB, Handlebars) with user input
|
||||
- LDAP injection in directory queries
|
||||
- SSRF via user-controlled URLs (fetch, redirect, webhook targets)
|
||||
- Path traversal via user-controlled file paths (../../etc/passwd)
|
||||
- Header injection via user-controlled values in HTTP headers
|
||||
|
||||
### Cryptographic Misuse
|
||||
- Weak hashing algorithms (MD5, SHA1) for security-sensitive operations
|
||||
- Predictable randomness (Math.random, rand()) for tokens or secrets
|
||||
- Non-constant-time comparisons (==) on secrets, tokens, or digests
|
||||
- Hardcoded encryption keys or IVs
|
||||
- Missing salt in password hashing
|
||||
|
||||
### Secrets Exposure
|
||||
- API keys, tokens, or passwords in source code (even in comments)
|
||||
- Secrets logged in application logs or error messages
|
||||
- Credentials in URLs (query parameters or basic auth in URL)
|
||||
- Sensitive data in error responses returned to users
|
||||
- PII stored in plaintext when encryption is expected
|
||||
|
||||
### XSS via Escape Hatches
|
||||
- Rails: .html_safe, raw() on user-controlled data
|
||||
- React: dangerouslySetInnerHTML with user content
|
||||
- Vue: v-html with user content
|
||||
- Django: |safe, mark_safe() on user input
|
||||
- General: innerHTML assignment with unsanitized data
|
||||
|
||||
### Deserialization
|
||||
- Deserializing untrusted data (pickle, Marshal, YAML.load, JSON.parse of executable types)
|
||||
- Accepting serialized objects from user input or external APIs without schema validation
|
||||
@@ -0,0 +1,46 @@
|
||||
# Testing Specialist Review Checklist
|
||||
|
||||
Scope: Always-on (every review)
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"CRITICAL|INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"testing","summary":"...","fix":"...","fingerprint":"path:line:testing","specialist":"testing"}
|
||||
Optional: line, fix, fingerprint, evidence, test_stub.
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
---
|
||||
|
||||
## Categories
|
||||
|
||||
### Missing Negative-Path Tests
|
||||
- New code paths that handle errors, rejections, or invalid input with NO corresponding test
|
||||
- Guard clauses and early returns that are untested
|
||||
- Error branches in try/catch, rescue, or error boundaries with no failure-path test
|
||||
- Permission/auth checks that are asserted in code but never tested for the "denied" case
|
||||
|
||||
### Missing Edge-Case Coverage
|
||||
- Boundary values: zero, negative, max-int, empty string, empty array, nil/null/undefined
|
||||
- Single-element collections (off-by-one on loops)
|
||||
- Unicode and special characters in user-facing inputs
|
||||
- Concurrent access patterns with no race-condition test
|
||||
|
||||
### Test Isolation Violations
|
||||
- Tests sharing mutable state (class variables, global singletons, DB records not cleaned up)
|
||||
- Order-dependent tests (pass in sequence, fail when randomized)
|
||||
- Tests that depend on system clock, timezone, or locale
|
||||
- Tests that make real network calls instead of using stubs/mocks
|
||||
|
||||
### Flaky Test Patterns
|
||||
- Timing-dependent assertions (sleep, setTimeout, waitFor with tight timeouts)
|
||||
- Assertions on ordering of unordered results (hash keys, Set iteration, async resolution order)
|
||||
- Tests that depend on external services (APIs, databases) without fallback
|
||||
- Randomized test data without seed control
|
||||
|
||||
### Security Enforcement Tests Missing
|
||||
- Auth/authz checks in controllers with no test for the "unauthorized" case
|
||||
- Rate limiting logic with no test proving it actually blocks
|
||||
- Input sanitization with no test for malicious input
|
||||
- CSRF/CORS configuration with no integration test
|
||||
|
||||
### Coverage Gaps
|
||||
- New public methods/functions with zero test coverage
|
||||
- Changed methods where existing tests only cover the old behavior, not the new branch
|
||||
- Utility functions called from multiple places but tested only indirectly
|
||||
Reference in New Issue
Block a user