From cbb4d35792e83847a6b7cbe5fd4aee8d69417f2b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 28 Aug 2026 01:45:53 +0000 Subject: [PATCH] =?UTF-8?q?feat(review):=20simplification=20specialist=20?= =?UTF-8?q?=E2=80=94=20advisory=20over-engineering=20lens=20with=20ponytai?= =?UTF-8?q?l's=20tag=20vocabulary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- review/checklist.md | 8 +- review/sections/review-army.md | 26 ++++- review/specialists/simplification.md | 49 ++++++++ scripts/resolvers/review-army.ts | 26 ++++- ship/sections/review-army.md | 26 ++++- test/fixtures/review-army-lean-complete.js | 28 +++++ test/fixtures/review-army-overbuild.js | 68 +++++++++++ test/gen-skill-docs.test.ts | 27 +++++ test/helpers/touchfiles-data.ts | 4 + test/skill-e2e-review-army.test.ts | 125 +++++++++++++++++++++ 10 files changed, 369 insertions(+), 18 deletions(-) create mode 100644 review/specialists/simplification.md create mode 100644 test/fixtures/review-army-lean-complete.js create mode 100644 test/fixtures/review-army-overbuild.js diff --git a/review/checklist.md b/review/checklist.md index 16aa111bb..27d7ba2e4 100644 --- a/review/checklist.md +++ b/review/checklist.md @@ -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 diff --git a/review/sections/review-army.md b/review/sections/review-army.md index bfcd5a8ba..05aea20cd 100644 --- a/review/sections/review-army.md +++ b/review/sections/review-army.md @@ -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"}` diff --git a/review/specialists/simplification.md b/review/specialists/simplification.md new file mode 100644 index 000000000..2b6110799 --- /dev/null +++ b/review/specialists/simplification.md @@ -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 (`` 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 diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index 029367deb..3e936751b 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -67,6 +67,7 @@ Based on the scope signals above, select which specialists to dispatch. 5. **Data Migration** — if SCOPE_MIGRATIONS=true. Read \`${ctx.paths.skillRoot}/review/specialists/data-migration.md\` 6. **API Contract** — if SCOPE_API=true. Read \`${ctx.paths.skillRoot}/review/specialists/api-contract.md\` 7. **Design** — if SCOPE_FRONTEND=true. Use the existing design review checklist at \`${ctx.paths.skillRoot}/review/design-checklist.md\` +8. **Simplification** — if DIFF_LINES > 100. Read \`${ctx.paths.skillRoot}/review/specialists/simplification.md\`. Advisory-only lens: hunts unrequested structure (hand-rolled stdlib, one-implementation abstractions, dependencies duplicating platform features), never coverage. ### Adaptive gating @@ -76,7 +77,7 @@ For each conditional specialist that passed scope gating, check the \`gstack-spe - 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)."`; @@ -167,8 +168,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. @@ -178,7 +185,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] @@ -186,12 +194,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 ${fixFirstRef} alongside ${critPassRef}. -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 ${persistRef}. -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"}\` diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index 247c6b2f0..669da2ba9 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -183,6 +183,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 @@ -192,7 +193,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)." @@ -277,8 +278,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. @@ -288,7 +295,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] @@ -296,12 +304,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 the Fix-First flow (item 4) alongside the checklist pass (Step 9). -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 persist. -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"}` diff --git a/test/fixtures/review-army-lean-complete.js b/test/fixtures/review-army-lean-complete.js new file mode 100644 index 000000000..3810dfa14 --- /dev/null +++ b/test/fixtures/review-army-lean-complete.js @@ -0,0 +1,28 @@ +// Lean-and-complete fixture: the false-flag precision case for the +// simplification specialist. This is an ETHOS "choose A" diff — small, +// covers the error path and edge cases, carries its own check. There is +// nothing here to cut; a correct simplification pass returns NO FINDINGS. + +function parsePort(value) { + if (value === null || value === undefined || value === '') { + throw new Error(`parsePort: missing value`); + } + const port = Number(value); + if (!Number.isInteger(port) || port < 1 || port > 65535) { + throw new Error(`parsePort: expected integer in 1-65535, got ${JSON.stringify(value)}`); + } + return port; +} + +// Self-check: the smallest thing that fails if the logic breaks. +function testParsePort() { + const assert = require('node:assert'); + assert.strictEqual(parsePort('8080'), 8080); + assert.strictEqual(parsePort(443), 443); + assert.throws(() => parsePort(''), /missing value/); + assert.throws(() => parsePort('0'), /1-65535/); + assert.throws(() => parsePort('65536'), /1-65535/); + assert.throws(() => parsePort('abc'), /1-65535/); +} + +module.exports = { parsePort, testParsePort }; diff --git a/test/fixtures/review-army-overbuild.js b/test/fixtures/review-army-overbuild.js new file mode 100644 index 000000000..eecd8f5d9 --- /dev/null +++ b/test/fixtures/review-army-overbuild.js @@ -0,0 +1,68 @@ +// Planted over-engineering fixture for the simplification specialist. +// Three deliberate invitations: a hand-rolled date formatter the platform +// ships (native:), an abstract layer with exactly one implementation +// (speculative:), and a config block nothing reads (delete:). + +// INVITATION 1 (native:): Intl.DateTimeFormat does all of this in one line. +class DateFormatter { + constructor(locale) { + this.locale = locale || 'en-US'; + this.monthNames = [ + 'January', 'February', 'March', 'April', 'May', 'June', + 'July', 'August', 'September', 'October', 'November', 'December', + ]; + } + + pad(n) { + return n < 10 ? '0' + n : String(n); + } + + formatLong(date) { + const month = this.monthNames[date.getMonth()]; + return `${month} ${date.getDate()}, ${date.getFullYear()}`; + } + + formatShort(date) { + return `${this.pad(date.getMonth() + 1)}/${this.pad(date.getDate())}/${date.getFullYear()}`; + } + + formatTime(date) { + const hours = date.getHours() % 12 || 12; + const suffix = date.getHours() >= 12 ? 'PM' : 'AM'; + return `${hours}:${this.pad(date.getMinutes())} ${suffix}`; + } +} + +// INVITATION 2 (speculative:): abstract base with a single implementation. +class AbstractItemStore { + save(item) { + throw new Error('not implemented'); + } + load(id) { + throw new Error('not implemented'); + } +} + +class MemoryItemStore extends AbstractItemStore { + constructor() { + super(); + this.items = new Map(); + } + save(item) { + this.items.set(item.id, item); + return item; + } + load(id) { + return this.items.get(id) || null; + } +} + +// INVITATION 3 (delete:): configuration nothing in this file (or repo) reads. +const FORMATTER_CONFIG = { + enableLegacyMode: false, + cacheSize: 128, + strictParsing: true, + fallbackLocale: 'en-GB', +}; + +module.exports = { DateFormatter, MemoryItemStore, FORMATTER_CONFIG }; diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index 47a94b466..be9226581 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -943,6 +943,7 @@ describe('TEST_COVERAGE_AUDIT placeholders', () => { 'performance.md', 'data-migration.md', 'api-contract.md', + 'simplification.md', 'red-team.md', ]; for (const f of expected) { @@ -950,6 +951,32 @@ describe('TEST_COVERAGE_AUDIT placeholders', () => { } }); + // Regression pins for the simplification specialist (advisory carve-out edits + // the pre-existing quality_score instruction, so the rendered contract is + // pinned statically — the carve-out and the early-out line must both survive + // regeneration verbatim). + test('simplification advisory carve-out and early-out render into review docs', () => { + const reviewArmySection = fs.readFileSync( + path.join(ROOT, 'review', 'sections', 'review-army.md'), + 'utf-8', + ); + expect(reviewArmySection).toContain('"advisory": true'); + expect(reviewArmySection).toContain('quality score over NON-advisory findings only'); + expect(reviewArmySection).toContain('Simplification: lean already — nothing to cut.'); + expect(reviewArmySection).toContain('net: -N lines possible'); + expect(reviewArmySection).toContain('--simplification'); + // The specialist itself must never carry a verdict-shaped zero-findings line. + const spec = fs.readFileSync( + path.join(ROOT, 'review', 'specialists', 'simplification.md'), + 'utf-8', + ); + expect(spec).toContain('NO FINDINGS'); + expect(spec).not.toContain('Lean already. Ship.'); + // Closed tag vocabulary: the disavowed yagni: frame must not appear. + expect(spec).toContain('speculative'); + expect(spec.toLowerCase()).not.toContain('"yagni"'); + }); + test('each specialist file has standard header with scope and output format', () => { const specDir = path.join(ROOT, 'review', 'specialists'); const files = fs.readdirSync(specDir).filter(f => f.endsWith('.md')); diff --git a/test/helpers/touchfiles-data.ts b/test/helpers/touchfiles-data.ts index 90b8877a4..7d67be823 100644 --- a/test/helpers/touchfiles-data.ts +++ b/test/helpers/touchfiles-data.ts @@ -66,6 +66,8 @@ export const E2E_TOUCHFILES: Record = { 'review-army-quality-score': ['review/**', 'scripts/resolvers/review-army.ts'], 'review-army-json-findings': ['review/**', 'scripts/resolvers/review-army.ts'], 'review-army-red-team': ['review/**', 'scripts/resolvers/review-army.ts'], + 'review-army-simplification': ['review/**', 'scripts/resolvers/review-army.ts'], + 'review-army-simplification-precision': ['review/**', 'scripts/resolvers/review-army.ts'], 'review-army-consensus': ['review/**', 'scripts/resolvers/review-army.ts'], // Office Hours @@ -489,6 +491,8 @@ export const E2E_TIERS: Record = { 'review-army-json-findings': 'gate', // JSON schema compliance 'review-army-red-team': 'periodic', // Multi-agent coordination 'review-army-consensus': 'periodic', // Multi-specialist agreement + 'review-army-simplification': 'periodic', // Advisory lens quality benchmark + 'review-army-simplification-precision': 'periodic', // False-flag noise benchmark // Office Hours 'office-hours-spec-review': 'gate', diff --git a/test/skill-e2e-review-army.test.ts b/test/skill-e2e-review-army.test.ts index 0bbe74a07..a256fa187 100644 --- a/test/skill-e2e-review-army.test.ts +++ b/test/skill-e2e-review-army.test.ts @@ -588,6 +588,131 @@ Write findings to ${dir}/review-output.md`, }, 210_000); }); +// --- Review Army: Simplification specialist (activation) --- + +describeIfSelected('Review Army: Simplification activation', ['review-army-simplification'], () => { + let dir: string; + + beforeAll(() => { + const repo = setupRepo('army-simplification'); + dir = repo.dir; + + fs.writeFileSync(path.join(dir, 'app.js'), '// base\n'); + repo.run('git', ['add', '.']); + repo.run('git', ['commit', '-m', 'initial']); + + repo.run('git', ['checkout', '-b', 'feature/date-utils']); + const overbuild = fs.readFileSync( + path.join(ROOT, 'test', 'fixtures', 'review-army-overbuild.js'), 'utf-8' + ); + fs.writeFileSync(path.join(dir, 'date_utils.js'), overbuild); + repo.run('git', ['add', '.']); + repo.run('git', ['commit', '-m', 'add date utils']); + + copyReviewFiles(dir); + }); + + afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} }); + + testConcurrentIfSelected('review-army-simplification', async () => { + const result = await runSkillTest({ + prompt: `You are in a git repo on a feature branch that adds a JS utility file. +Read review-SKILL.md for instructions. Also read review-checklist.md. +The specialist checklists are in review-specialists/ (testing.md, simplification.md, etc.). + +Skip the preamble, lake intro, telemetry sections. +Run Step 4.5 (Review Army) only. +The base branch is main. The diff is over 100 lines, so the Simplification specialist should activate. + +For the specialist dispatch, read review-specialists/simplification.md and apply it against the diff. + +Write your findings to ${dir}/review-output.md`, + workingDirectory: dir, + maxTurns: 20, + timeout: 180_000, + testName: 'review-army-simplification', + runId, + }); + + logCost('/review army simplification', result); + recordE2E(evalCollector, '/review army simplification detection', 'Review Army', result); + expect(result.exitReason).toBe('success'); + + const outputPath = path.join(dir, 'review-output.md'); + expect(fs.existsSync(outputPath)).toBe(true); + const content = fs.readFileSync(outputPath, 'utf-8').toLowerCase(); + // At least one planted invitation caught, expressed through the closed + // tag vocabulary or its obvious phrasing. + const hasStructureFinding = + content.includes('native') || + content.includes('stdlib') || + content.includes('speculative') || + content.includes('intl') || + content.includes('one implementation') || + content.includes('single implementation'); + expect(hasStructureFinding).toBe(true); + // Advisory findings must not read as defects: the disavowed frame stays out. + expect(content).not.toContain('lean already. ship.'); + }, 210_000); +}); + +// --- Review Army: Simplification specialist (false-flag precision) --- + +describeIfSelected('Review Army: Simplification precision', ['review-army-simplification-precision'], () => { + let dir: string; + + beforeAll(() => { + const repo = setupRepo('army-simplification-lean'); + dir = repo.dir; + + fs.writeFileSync(path.join(dir, 'app.js'), '// base\n'); + repo.run('git', ['add', '.']); + repo.run('git', ['commit', '-m', 'initial']); + + repo.run('git', ['checkout', '-b', 'feature/parse-port']); + const lean = fs.readFileSync( + path.join(ROOT, 'test', 'fixtures', 'review-army-lean-complete.js'), 'utf-8' + ); + fs.writeFileSync(path.join(dir, 'parse_port.js'), lean); + repo.run('git', ['add', '.']); + repo.run('git', ['commit', '-m', 'add parsePort with self-check']); + + copyReviewFiles(dir); + }); + + afterAll(() => { try { fs.rmSync(dir, { recursive: true, force: true }); } catch {} }); + + testConcurrentIfSelected('review-army-simplification-precision', async () => { + const result = await runSkillTest({ + prompt: `You are in a git repo on a feature branch that adds one small, complete utility (validation + error path + self-check). +Read review-specialists/simplification.md and apply it against the diff of the current branch vs main (git diff main). + +Write the specialist's raw output to ${dir}/review-output.md — either the finding JSON lines or the exact NO FINDINGS sentinel.`, + workingDirectory: dir, + maxTurns: 12, + timeout: 150_000, + testName: 'review-army-simplification-precision', + runId, + }); + + logCost('/review army simplification precision', result); + recordE2E(evalCollector, '/review army simplification precision', 'Review Army', result); + expect(result.exitReason).toBe('success'); + + const outputPath = path.join(dir, 'review-output.md'); + expect(fs.existsSync(outputPath)).toBe(true); + const content = fs.readFileSync(outputPath, 'utf-8'); + // Precision: a lean, complete diff yields no simplification findings. + // The specialist must not flag the error path or the self-check for + // deletion — that is the noise failure mode this case pins. + const flaggedTestOrErrorPath = + /"category"\s*:\s*"(delete|shrink|stdlib|native|speculative)"/i.test(content) && + /(testparseport|self-check|assert|throw)/i.test(content); + expect(flaggedTestOrErrorPath).toBe(false); + expect(content.toUpperCase()).toContain('NO FINDINGS'); + }, 180_000); +}); + // Finalize eval collector afterAll(async () => { await finalizeEvalCollector(evalCollector);