mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-08 23:36:06 +02:00
feat: Review Army — parallel specialist reviewers for /review (v0.14.3.0) (#692)
* feat: extend gstack-diff-scope with SCOPE_MIGRATIONS, SCOPE_API, SCOPE_AUTH
Three new scope signals for Review Army specialist activation:
- SCOPE_MIGRATIONS: db/migrate/, prisma/migrations/, alembic/, *.sql
- SCOPE_API: *controller*, *route*, *endpoint*, *.graphql, openapi.*
- SCOPE_AUTH: *auth*, *session*, *jwt*, *oauth*, *permission*, *role*
* feat: add 7 specialist checklist files for Review Army
- testing.md (always-on): coverage gaps, flaky patterns, security enforcement
- maintainability.md (always-on): dead code, DRY, stale comments
- security.md (conditional): OWASP deep analysis, auth bypass, injection
- performance.md (conditional): N+1 queries, bundle impact, complexity
- data-migration.md (conditional): reversibility, lock duration, backfill
- api-contract.md (conditional): breaking changes, versioning, error format
- red-team.md (conditional): adversarial analysis, cross-cutting concerns
All use standard header with JSON output schema and NO FINDINGS fallback.
* feat: Review Army resolver — parallel specialist dispatch + merge
New resolver in review-army.ts generates template prose for:
- Stack detection and specialist selection
- Parallel Agent tool dispatch with learning-informed prompts
- JSON finding collection, fingerprint dedup, consensus highlighting
- PR quality score computation
- Red Team conditional dispatch
Registered as REVIEW_ARMY in resolvers/index.ts.
* refactor: restructure /review template for Review Army
- Replace Steps 4-4.75 with CRITICAL pass + {{REVIEW_ARMY}}
- Remove {{DESIGN_REVIEW_LITE}} and {{TEST_COVERAGE_AUDIT_REVIEW}}
(subsumed into Design and Testing specialists respectively)
- Extract specialist-covered categories from checklist.md
- Keep CRITICAL + uncovered INFORMATIONAL in main agent pass
* test: Review Army — 14 diff-scope tests + 7 E2E tests
- test/diff-scope.test.ts: 14 tests for all 9 scope signals
- test/skill-e2e-review-army.test.ts: 7 E2E tests
Gate: migration safety, N+1 detection, delivery audit,
quality score, JSON findings
Periodic: red team, consensus
- Updated gen-skill-docs tests for new review structure
- Added touchfile entries and tier classifications
* docs: update SELF_LEARNING_V0.md with Release 2 status + Release 2.5
Mark Release 2 (Review Army) as in-progress. Add Release 2.5 for
deferred expansions (E1 adaptive gating, E3 test stubs, E5 cross-review
dedup, E7 specialist tracking).
* chore: bump version and changelog (v0.14.3.0)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
a0328be04c
commit
a4a181ca92
+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
|
||||
|
||||
Reference in New Issue
Block a user