mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-31 02:10:51 +02:00
feat(review): simplification specialist — advisory over-engineering lens with ponytail's tag vocabulary
New 8th Review Army specialist (DIFF_LINES > 100, --simplification force flag) hunting unrequested STRUCTURE only: delete/stdlib/native/speculative/shrink closed tags, one-line findings, lines_removable field. speculative: replaces ponytail's yagni: tag — we import the lens, not the posture; coverage stays sacred (Completeness Gaps owns it, suppressions inlined, shrink needs >=5 lines). Advisory carve-out in the merge step: advisory findings are excluded from quality_score and the findings-count header, render with an [ADVISORY] label, and are ASK-only in Fix-First. Zero-findings case prints the lens-scoped 'Simplification: lean already — nothing to cut.' from the PARENT (the specialist keeps the exact NO FINDINGS contract); with findings, the parent prints 'net: -N lines possible' summed from lines_removable. Tests: static pins for the carve-out + early-out contract (gen-skill-docs), two periodic e2e cases with planted fixtures — activation (over-build traps: hand-rolled Intl, one-impl abstract, dead config) and false-flag precision (a lean ETHOS 'choose A' diff must yield NO FINDINGS). Inspired by dietrichgebert/ponytail's /ponytail-review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Fable 5
Claude Fable 5
parent
848973007c
commit
cbb4d35792
+5
-3
@@ -7,7 +7,9 @@ Review the `git diff origin/main` output for the issues listed below. Be specifi
|
||||
**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.
|
||||
- **Specialist categories (handled by parallel subagents, NOT this checklist):** Test Gaps, Dead Code, Magic Numbers, Conditional Side Effects, Performance & Bundle Impact, Crypto & Entropy, Simplification (unrequested structure). See `review/specialists/` for these.
|
||||
|
||||
Completeness Gaps and Simplification are orthogonal, not contradictory: Completeness pushes coverage UP (tests, edge cases, error paths), Simplification pushes unrequested structure DOWN (one-implementation abstractions, hand-rolled stdlib, dead flexibility). The same diff can legitimately receive both.
|
||||
|
||||
All findings get action via Fix-First Review: obvious mechanical fixes are applied automatically,
|
||||
genuinely ambiguous issues are batched into a single user question.
|
||||
@@ -129,8 +131,8 @@ CRITICAL (highest severity): INFORMATIONAL (main agent): SPECIALIST (p
|
||||
├─ 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
|
||||
├─ Type Coercion at Boundaries ├─ Simplification (advisory)
|
||||
├─ View/Frontend └─ Red Team (conditional)
|
||||
└─ Distribution & CI/CD Pipeline
|
||||
|
||||
All findings are actioned via Fix-First Review. Severity determines
|
||||
|
||||
@@ -51,6 +51,7 @@ Based on the scope signals above, select which specialists to dispatch.
|
||||
5. **Data Migration** — if SCOPE_MIGRATIONS=true. Read `~/.claude/skills/gstack/review/specialists/data-migration.md`
|
||||
6. **API Contract** — if SCOPE_API=true. Read `~/.claude/skills/gstack/review/specialists/api-contract.md`
|
||||
7. **Design** — if SCOPE_FRONTEND=true. Use the existing design review checklist at `~/.claude/skills/gstack/review/design-checklist.md`
|
||||
8. **Simplification** — if DIFF_LINES > 100. Read `~/.claude/skills/gstack/review/specialists/simplification.md`. Advisory-only lens: hunts unrequested structure (hand-rolled stdlib, one-implementation abstractions, dependencies duplicating platform features), never coverage.
|
||||
|
||||
### Adaptive gating
|
||||
|
||||
@@ -60,7 +61,7 @@ For each conditional specialist that passed scope gating, check the `gstack-spec
|
||||
- If tagged `[GATE_CANDIDATE]` (0 findings in 10+ dispatches): skip it. Print: "[specialist] auto-gated (0 findings in N reviews)."
|
||||
- If tagged `[NEVER_GATE]`: always dispatch regardless of hit rate. Security and data-migration are insurance policy specialists — they should run even when silent.
|
||||
|
||||
**Force flags:** If the user's prompt includes `--security`, `--performance`, `--testing`, `--maintainability`, `--data-migration`, `--api-contract`, `--design`, or `--all-specialists`, force-include that specialist regardless of gating.
|
||||
**Force flags:** If the user's prompt includes `--security`, `--performance`, `--testing`, `--maintainability`, `--data-migration`, `--api-contract`, `--design`, `--simplification`, or `--all-specialists`, force-include that specialist regardless of gating.
|
||||
|
||||
Note which specialists were selected, gated, and skipped. Print the selection:
|
||||
"Dispatching N specialists: [names]. Skipped: [names] (scope not detected). Gated: [names] (0 findings in N+ reviews)."
|
||||
@@ -145,8 +146,14 @@ Group findings by fingerprint. For findings sharing the same fingerprint:
|
||||
- Confidence 3-4: move to appendix (suppress from main findings)
|
||||
- Confidence 1-2: suppress entirely
|
||||
|
||||
**Advisory carve-out (simplification specialist):**
|
||||
Findings with `"advisory": true` are excluded from BOTH the quality_score
|
||||
summation and the findings-count header below — they are structure suggestions,
|
||||
not defects, and must not make "5 findings … 10/10" look contradictory. In
|
||||
Fix-First they are ASK-only: NEVER auto-applied, even when mechanical.
|
||||
|
||||
**Compute PR Quality Score:**
|
||||
After merging, compute the quality score:
|
||||
After merging, compute the quality score over NON-advisory findings only:
|
||||
`quality_score = max(0, 10 - (critical_count * 2 + informational_count * 0.5))`
|
||||
Cap at 10. Log this in the review result at the end.
|
||||
|
||||
@@ -156,7 +163,8 @@ Present the merged findings in the same format as the current review:
|
||||
```
|
||||
SPECIALIST REVIEW: N findings (X critical, Y informational) from Z specialists
|
||||
|
||||
[For each finding, in order: CRITICAL first, then INFORMATIONAL, sorted by confidence descending]
|
||||
[For each finding, in order: CRITICAL first, then INFORMATIONAL, sorted by confidence descending;
|
||||
advisory findings last, each rendered with an [ADVISORY] label in place of the severity]
|
||||
[SEVERITY] (confidence: N/10, specialist: name) path:line — summary
|
||||
Fix: recommended fix
|
||||
[If MULTI-SPECIALIST CONFIRMED: show confirmation note]
|
||||
@@ -164,12 +172,20 @@ SPECIALIST REVIEW: N findings (X critical, Y informational) from Z specialists
|
||||
PR Quality Score: X/10
|
||||
```
|
||||
|
||||
**Simplification footer (after the score line):**
|
||||
- If the simplification specialist was dispatched and returned findings, sum
|
||||
their `lines_removable` values and print: `net: -N lines possible` (omit
|
||||
findings without the field from the sum).
|
||||
- If it was dispatched and returned NO FINDINGS, print:
|
||||
`Simplification: lean already — nothing to cut.`
|
||||
- If it was not dispatched, print neither line.
|
||||
|
||||
These findings flow into Step 5 Fix-First alongside the CRITICAL pass findings from Step 4.
|
||||
The Fix-First heuristic applies identically — specialist findings follow the same AUTO-FIX vs ASK classification.
|
||||
The Fix-First heuristic applies identically — specialist findings follow the same AUTO-FIX vs ASK classification (except advisory findings, which are ASK-only per the carve-out above).
|
||||
|
||||
**Compile per-specialist stats:**
|
||||
After merging findings, compile a `specialists` object for the review-log entry in Step 5.8.
|
||||
For each specialist (testing, maintainability, security, performance, data-migration, api-contract, design, red-team):
|
||||
For each specialist (testing, maintainability, security, performance, data-migration, api-contract, design, simplification, red-team):
|
||||
- If dispatched: `{"dispatched": true, "findings": N, "critical": N, "informational": N}`
|
||||
- If skipped by scope: `{"dispatched": false, "reason": "scope"}`
|
||||
- If skipped by gating: `{"dispatched": false, "reason": "gated"}`
|
||||
|
||||
@@ -0,0 +1,49 @@
|
||||
# Simplification Specialist Review Checklist
|
||||
|
||||
Scope: Conditional (DIFF_LINES > 100). This lens hunts unrequested *structure* only: abstractions with one implementation, hand-rolled stdlib, dependencies duplicating platform features, dead flexibility. Coverage gaps are out of scope — the Completeness Gaps checklist category owns those. Never flag a test, an error path, or an edge-case branch for deletion.
|
||||
Output: JSON objects, one finding per line. Schema:
|
||||
{"severity":"INFORMATIONAL","confidence":N,"path":"file","line":N,"category":"delete|stdlib|native|speculative|shrink","summary":"...","fix":"...","lines_removable":N,"advisory":true,"fingerprint":"path:line:category","specialist":"simplification"}
|
||||
Required: severity (always INFORMATIONAL), confidence, path, category, summary, advisory (always true), specialist.
|
||||
Optional: line, fix, fingerprint, evidence, lines_removable (the net lines deleted if the fix is applied — the merge step sums this for the `net:` footer).
|
||||
If no findings: output `NO FINDINGS` and nothing else.
|
||||
|
||||
Findings from this specialist are ADVISORY: they are excluded from the PR Quality Score and are never auto-applied by Fix-First — the merge step handles both carve-outs.
|
||||
|
||||
---
|
||||
|
||||
## The five tags (closed vocabulary — every finding uses exactly one as its `category`)
|
||||
|
||||
- `delete:` dead code, unused flexibility, speculative feature. Replacement: nothing.
|
||||
- `stdlib:` hand-rolled thing the standard library ships. Name the function.
|
||||
- `native:` dependency or code doing what the platform already does. Name the feature.
|
||||
- `speculative:` abstraction with one implementation, config nobody sets, layer with one caller.
|
||||
- `shrink:` same logic, fewer lines — only when the reduction is ≥5 lines. Show the shorter form.
|
||||
|
||||
## Finding style — one line each, location + what to cut + what replaces it
|
||||
|
||||
❌ "This EmailValidator class might be more complex than necessary, have you
|
||||
considered whether all these validation rules are needed at this stage?"
|
||||
|
||||
✅ `{"severity":"INFORMATIONAL","confidence":8,"path":"lib/email.ts","line":12,"category":"stdlib","summary":"27-line validator class — '@' in email covers it; real validation is the confirmation mail","fix":"replace class with a one-line includes('@') check","lines_removable":26,"advisory":true,"specialist":"simplification"}`
|
||||
|
||||
✅ `{"severity":"INFORMATIONAL","confidence":9,"path":"app/dates.ts","line":4,"category":"native","summary":"moment.js imported for one format call","fix":"Intl.DateTimeFormat, 0 deps","lines_removable":3,"advisory":true,"specialist":"simplification"}`
|
||||
|
||||
✅ `{"severity":"INFORMATIONAL","confidence":8,"path":"repo.py","line":88,"category":"speculative","summary":"AbstractRepository with one implementation","fix":"inline it until a second implementation exists","lines_removable":41,"advisory":true,"specialist":"simplification"}`
|
||||
|
||||
✅ `{"severity":"INFORMATIONAL","confidence":7,"path":"sync.ts","line":52,"category":"delete","summary":"retry wrapper around an idempotent local call","fix":"nothing replaces it","lines_removable":19,"advisory":true,"specialist":"simplification"}`
|
||||
|
||||
## What to hunt
|
||||
|
||||
- Dependencies the stdlib or platform already ships (`<input type="date">` over a picker lib, CSS over JS, DB constraint over app code)
|
||||
- Single-implementation interfaces, factories with one product, wrappers that only delegate
|
||||
- Files exporting one thing, dead flags and config, hand-rolled stdlib
|
||||
- Manual loops that a built-in expresses in one line (≥5 lines saved only)
|
||||
|
||||
## Suppressions — DO NOT flag these (inherited from the main checklist, binding here)
|
||||
|
||||
- "X is redundant with Y" when the redundancy is harmless and aids readability
|
||||
- Consistency-only changes (wrapping a value in a conditional to match how another constant is guarded)
|
||||
- Tests, error paths, edge-case branches, input validation, security measures, accessibility — NEVER deletion targets; coverage is the Completeness Gaps category's job, and the house rule is "If A is 70 lines more, choose A" (ETHOS.md)
|
||||
- A single smoke test or assert-based self-check — that is the completeness minimum, not bloat
|
||||
- Deliberate `gstack-shortcut(dec-*)` markers — already acknowledged debt with a ledger entry
|
||||
- ANYTHING already addressed in the diff you're reviewing — read the FULL diff before commenting
|
||||
Reference in New Issue
Block a user