From cac470445b2b225f42751a70017dce27d58186ac Mon Sep 17 00:00:00 2001 From: Matt Pocock Date: Tue, 30 Jun 2026 09:58:26 +0100 Subject: [PATCH 1/3] feat(review): add an always-on Fowler smell baseline to the Standards axis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds Refactoring ch.3 "Bad Smells in Code" into the in-progress `review` skill. The skill stays two-axis: smells layer into the Standards axis as a fixed, always-on baseline alongside a repo's documented standards, rather than a third axis. Because the baseline is needed on every run it lives inline in SKILL.md. Each smell is one line — its name carries the definition (the names are strong leading words) plus a diff-specific cue. Curated to ~12 high-signal smells; vague ones (Comments, Loops) and tooling-caught ones (Long Parameter List) are deliberately dropped. Two rules bind the baseline: a documented repo standard always overrides it, and every smell is reported as a judgement call, never a hard violation. The Standards sub-agent prompt now carries the baseline and these rules. Co-Authored-By: Claude Opus 4.8 (1M context) --- skills/in-progress/review/SKILL.md | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/skills/in-progress/review/SKILL.md b/skills/in-progress/review/SKILL.md index 63e71f3..77fd2bb 100644 --- a/skills/in-progress/review/SKILL.md +++ b/skills/in-progress/review/SKILL.md @@ -35,6 +35,26 @@ Look for the originating spec, in this order: Anything in the repo that documents how code should be written, such as `CODING_STANDARDS.md` or `CONTRIBUTING.md`. +On top of whatever the repo documents, the Standards axis always carries the **smell baseline** below — a fixed set of Fowler code smells (_Refactoring_, ch.3) that applies even when a repo documents nothing. Two rules bind it: + +- **The repo overrides.** A documented repo standard always wins; where it endorses something the baseline would flag, suppress the smell. +- **Always a judgement call.** Each smell is a labelled heuristic ("possible Feature Envy"), never a hard violation — and, like any standard here, skip anything tooling already enforces. + +Each name carries its own definition; match it against the diff: + +- **Mysterious Name** — a function, variable, or type whose name doesn't reveal what it does or holds. +- **Duplicated Code** — the same logic shape appears in more than one hunk or file in the change. +- **Feature Envy** — a method that reaches into another object's data more than its own. +- **Data Clumps** — the same few fields or params keep travelling together (a type wanting to be born). +- **Primitive Obsession** — a primitive or string standing in for a domain concept that deserves its own type. +- **Repeated Switches** — the same `switch`/`if`-cascade on the same type recurs across the change. +- **Shotgun Surgery** — one logical change forces scattered edits across many files in the diff. +- **Divergent Change** — one file or module is edited for several unrelated reasons. +- **Speculative Generality** — abstraction, parameters, or hooks added for needs the spec doesn't have. +- **Message Chains** — long `a.b().c().d()` navigation the caller shouldn't depend on. +- **Middle Man** — a class or function that mostly just delegates onward. +- **Refused Bequest** — a subclass or implementer that ignores or overrides most of what it inherits. + ### 4. Spawn both sub-agents in parallel Send a single message with two `Agent` tool calls. Use the `general-purpose` subagent for both. @@ -42,8 +62,8 @@ Send a single message with two `Agent` tool calls. Use the `general-purpose` sub **Standards sub-agent prompt** — include: - The full diff command and commit list. -- The list of standards-source files you found in step 3. -- The brief: "Report — per file/hunk where relevant — every place the diff violates a documented standard. Cite the standard (file + the rule). Distinguish hard violations from judgement calls. Skip anything tooling enforces. Under 400 words." +- The list of standards-source files you found in step 3, **plus the smell baseline from step 3** pasted in full — the sub-agent has no other access to it. +- The brief: "Report — per file/hunk where relevant — (a) every place the diff violates a documented standard: cite the standard (file + the rule); and (b) any baseline smell you spot: name it and quote the hunk. Distinguish hard violations from judgement calls — documented-standard breaches can be hard, but baseline smells are always judgement calls, and a documented repo standard overrides the baseline. Skip anything tooling enforces. Under 400 words." **Spec sub-agent prompt** — include: From 0894b3300f1e83cc666ed6ad075489041e3ba291 Mon Sep 17 00:00:00 2001 From: Matt Pocock Date: Tue, 30 Jun 2026 10:02:31 +0100 Subject: [PATCH 2/3] review: add a terse fix to each smell in the baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each smell now reads what-it-is → how-to-fix, telegraphic style — the fix is the actionable half a reviewer needs, kept to a clause. Co-Authored-By: Claude Opus 4.8 (1M context) --- skills/in-progress/review/SKILL.md | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/skills/in-progress/review/SKILL.md b/skills/in-progress/review/SKILL.md index 77fd2bb..02bfdc3 100644 --- a/skills/in-progress/review/SKILL.md +++ b/skills/in-progress/review/SKILL.md @@ -40,20 +40,20 @@ On top of whatever the repo documents, the Standards axis always carries the **s - **The repo overrides.** A documented repo standard always wins; where it endorses something the baseline would flag, suppress the smell. - **Always a judgement call.** Each smell is a labelled heuristic ("possible Feature Envy"), never a hard violation — and, like any standard here, skip anything tooling already enforces. -Each name carries its own definition; match it against the diff: +Each smell reads *what it is* → *how to fix*; match it against the diff: -- **Mysterious Name** — a function, variable, or type whose name doesn't reveal what it does or holds. -- **Duplicated Code** — the same logic shape appears in more than one hunk or file in the change. -- **Feature Envy** — a method that reaches into another object's data more than its own. -- **Data Clumps** — the same few fields or params keep travelling together (a type wanting to be born). -- **Primitive Obsession** — a primitive or string standing in for a domain concept that deserves its own type. -- **Repeated Switches** — the same `switch`/`if`-cascade on the same type recurs across the change. -- **Shotgun Surgery** — one logical change forces scattered edits across many files in the diff. -- **Divergent Change** — one file or module is edited for several unrelated reasons. -- **Speculative Generality** — abstraction, parameters, or hooks added for needs the spec doesn't have. -- **Message Chains** — long `a.b().c().d()` navigation the caller shouldn't depend on. -- **Middle Man** — a class or function that mostly just delegates onward. -- **Refused Bequest** — a subclass or implementer that ignores or overrides most of what it inherits. +- **Mysterious Name** — a function, variable, or type whose name doesn't reveal what it does or holds. → rename it; if no honest name comes, the design's murky. +- **Duplicated Code** — the same logic shape appears in more than one hunk or file in the change. → extract the shared shape, call it from both. +- **Feature Envy** — a method that reaches into another object's data more than its own. → move the method onto the data it envies. +- **Data Clumps** — the same few fields or params keep travelling together (a type wanting to be born). → bundle them into one type, pass that. +- **Primitive Obsession** — a primitive or string standing in for a domain concept that deserves its own type. → give the concept its own small type. +- **Repeated Switches** — the same `switch`/`if`-cascade on the same type recurs across the change. → replace with polymorphism, or one map both sites share. +- **Shotgun Surgery** — one logical change forces scattered edits across many files in the diff. → gather what changes together into one module. +- **Divergent Change** — one file or module is edited for several unrelated reasons. → split so each module changes for one reason. +- **Speculative Generality** — abstraction, parameters, or hooks added for needs the spec doesn't have. → delete it; inline back until a real need shows. +- **Message Chains** — long `a.b().c().d()` navigation the caller shouldn't depend on. → hide the walk behind one method on the first object. +- **Middle Man** — a class or function that mostly just delegates onward. → cut it, call the real target direct. +- **Refused Bequest** — a subclass or implementer that ignores or overrides most of what it inherits. → drop the inheritance, use composition. ### 4. Spawn both sub-agents in parallel From 7a4c7561d4d8d866a9ae6e0073648ac94785ac8a Mon Sep 17 00:00:00 2001 From: Matt Pocock Date: Tue, 30 Jun 2026 10:19:49 +0100 Subject: [PATCH 3/3] Add changeset for review smell baseline Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/review-smell-baseline.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/review-smell-baseline.md diff --git a/.changeset/review-smell-baseline.md b/.changeset/review-smell-baseline.md new file mode 100644 index 0000000..a44a785 --- /dev/null +++ b/.changeset/review-smell-baseline.md @@ -0,0 +1,5 @@ +--- +"mattpocock-skills": patch +--- + +Give the in-progress **`review`** skill an always-on Fowler smell baseline on its Standards axis. A curated ~12 high-signal "Bad Smells in Code" (Mysterious Name, Duplicated Code, Feature Envy, Data Clumps, Primitive Obsession, Repeated Switches, Shotgun Surgery, Divergent Change, Speculative Generality, Message Chains, Middle Man, Refused Bequest) are inlined into `SKILL.md` as a fixed baseline alongside whatever the repo documents — not a new third axis. Two binding rules keep it safe: a documented repo standard overrides the baseline, and every smell is reported as a judgement call, never a hard violation.