Compare commits

...
Author SHA1 Message Date
Matt PocockandClaude Opus 4.8 6b8c429bc6 docs(link-skills): mark as a dev-only script
Add a header note clarifying that link-skills.sh is a dev-only script for
maintainers, not a supported installer, and that modifications or requests
for modification will not be approved.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-30 10:28:50 +01:00
Matt PocockandGitHub 7936cd4b8b Merge pull request #394 from mattpocock/review-smell-baseline
feat(review): always-on Fowler smell baseline in the Standards axis
2026-06-30 10:24:14 +01:00
Matt PocockandClaude Opus 4.8 7a4c7561d4 Add changeset for review smell baseline
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-30 10:19:49 +01:00
Matt PocockandClaude Opus 4.8 0894b3300f review: add a terse fix to each smell in the baseline
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) <noreply@anthropic.com>
2026-06-30 10:02:31 +01:00
Matt PocockandClaude Opus 4.8 cac470445b feat(review): add an always-on Fowler smell baseline to the Standards axis
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) <noreply@anthropic.com>
2026-06-30 09:58:26 +01:00
Matt PocockandClaude Opus 4.8 43ea0884b0 tdd: add tautological-test anti-pattern
Tests whose assertion is recomputed the way the code computes it pass by
construction and give zero confidence. Add it as a peer of the existing
implementation-coupling anti-pattern: a Philosophy principle, a per-cycle
checklist gate, and a BAD/GOOD example pair in tests.md. Includes a patch
changeset.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-29 21:22:50 +01:00
Matt PocockandClaude Opus 4.8 a116824938 decision-mapping: key tickets by dash-case slugs, not numeric ids
Replace numeric ticket ids (#1, #2…) with short, token-efficient
dash-case slugs (e.g. relational-db) that read as mini-titles. Update
the Structure example, the next-ticket selection rule (lowest-numbered →
first in document order), the optional-arg wording, and the Handoff
copy-paste examples accordingly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-29 16:50:46 +01:00
7 changed files with 71 additions and 12 deletions
+5
View File
@@ -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.
+5
View File
@@ -0,0 +1,5 @@
---
"mattpocock-skills": patch
---
Add the **tautological test** anti-pattern to the `tdd` skill. Tests whose assertion is recomputed the way the code computes it pass by construction and give zero confidence — distinct from the implementation-coupling anti-pattern already covered. Added as a peer at the same three sites: a Philosophy principle (expected values must come from an independent source of truth), a per-cycle checklist gate, and a BAD/GOOD example pair in `tests.md`.
+4
View File
@@ -1,6 +1,10 @@
#!/usr/bin/env bash
set -euo pipefail
# NOTE: This is a dev-only script, intended for use by maintainers of this repo.
# It is not a supported installer. Modifications to it — or requests for
# modifications — will not be approved.
#
# Links all skills in the repository into the local skill directories used by
# each agent harness:
# - ~/.claude/skills — Claude Code
+3
View File
@@ -13,6 +13,8 @@ description: Test-driven development. Use when the user wants to build features
**Bad tests** are coupled to implementation. They mock internal collaborators, test private methods, or verify through external means (like querying a database directly instead of using the interface). The warning sign: your test breaks when you refactor, but behavior hasn't changed. If you rename an internal function and tests fail, those tests were testing implementation, not behavior.
**Tautological tests** restate the implementation inside the assertion, so they pass by construction and give zero confidence. When the expected value is computed the way the code computes it — `expect(add(a, b)).toBe(a + b)`, snapshotting a figure you derived by hand the same way the code does, asserting a constant equals itself — the test can never disagree with the code: break the code wrong and the assertion breaks wrong with it. The expected value must come from an independent source of truth — a known-good literal, a worked example, the spec.
See [tests.md](tests.md) for examples and [mocking.md](mocking.md) for mocking guidelines.
## Anti-Pattern: Horizontal Slices
@@ -103,6 +105,7 @@ After all tests pass, look for [refactor candidates](refactoring.md):
[ ] Test describes behavior, not implementation
[ ] Test uses public interface only
[ ] Test would survive internal refactor
[ ] Expected values are independent literals, not recomputed from the code
[ ] Code is minimal for this test
[ ] No speculative features added
```
+16
View File
@@ -59,3 +59,19 @@ test("createUser makes user retrievable", async () => {
expect(retrieved.name).toBe("Alice");
});
```
**Tautological tests**: Expected value restates the implementation, so the test passes by construction.
```typescript
// BAD: Expected value is recomputed the way the code computes it
test("calculateTotal sums line items", () => {
const items = [{ price: 10 }, { price: 5 }];
const expected = items.reduce((sum, i) => sum + i.price, 0);
expect(calculateTotal(items)).toBe(expected);
});
// GOOD: Expected value is an independent, known literal
test("calculateTotal sums line items", () => {
expect(calculateTotal([{ price: 10 }, { price: 5 }])).toBe(15);
});
```
+16 -10
View File
@@ -14,12 +14,14 @@ Assets created during tickets should be linked to from the map, not duplicated w
### Structure
Numbered entries ("tickets"), each its own section keyed by its number:
Entries ("tickets"), each its own section keyed by a short dash-case slug that
reads as a mini-title (e.g. `relational-db`, `auth-strategy`, `cache-layer`) —
terse enough to stay token-efficient, and unique within the map.
```markdown
## #1: Relational Or Non-Relational Database?
## relational-db: Relational Or Non-Relational Database?
Blocked by: #<ticket-number>, #<ticket-number>
Blocked by: <slug>, <slug>
Status: open | in-progress | resolved
Type: Research | Prototype | Grilling
@@ -32,7 +34,11 @@ Type: Research | Prototype | Grilling
<answer-here>
```
A ticket is **unblocked** when every ticket in its `Blocked by` list is `resolved`. A session **claims** its ticket by setting `Status: in-progress` and saving the map before any work, so concurrent sessions skip it.
The slug is the canonical id, used in every `Blocked by` edge and prose
reference; the title after the colon is optional. A ticket
is **unblocked** when every ticket in its `Blocked by` list is `resolved`. A
session **claims** its ticket by setting `Status: in-progress` and saving the map
before any work, so concurrent sessions skip it.
Each ticket must be sized to one 100K token agent session.
@@ -62,10 +68,10 @@ User invokes with a loose idea.
### Work through the map
User invokes with a path to an existing map. A ticket number is **optional** — without one, you pick the next decision, not the user.
User invokes with a path to an existing map. A ticket slug is **optional** — without one, you pick the next decision, not the user.
1. Load the **whole map** as context.
2. Choose the ticket. If the user named one, use it. Otherwise pick the lowest-numbered `open` ticket that is [unblocked](#structure). [Claim it](#structure): set `Status: in-progress` and save before any work.
2. Choose the ticket. If the user named one, use it. Otherwise pick the first `open` ticket in document order that is [unblocked](#structure). [Claim it](#structure): set `Status: in-progress` and save before any work.
3. Resolve it, invoking skills as needed. If in doubt, use `/grilling` and `/domain-modeling`.
4. Record the answer in the ticket's body and set `Status: resolved`.
5. Add newly-discovered tickets with correct `Blocked by` edges. If the decisions made invalidate other parts of the map, update or delete those nodes.
@@ -79,7 +85,7 @@ End every session by clearing the context and opening one or more fresh sessions
**Open tickets remain.** List the currently-unblocked tickets, then give two copy-paste options: a bare command for one session (you pick the next ticket), and one pinned command per unblocked ticket for running them in parallel. Paste one line per fresh window — opening one, some, or all of them.
> **Next steps** — 3 tickets unblocked: #4, #5, #6.
> **Next steps** — 3 tickets unblocked: `auth-strategy`, `cache-layer`, `rate-limits`.
> Clear the context, then open fresh sessions.
>
> **One session** — resolves the next unblocked ticket:
@@ -89,9 +95,9 @@ End every session by clearing the context and opening one or more fresh sessions
>
> **Parallel** — paste one line per window, up to all 3:
> ```
> Invoke /decision-mapping with the map at <path>, ticket #4.
> Invoke /decision-mapping with the map at <path>, ticket #5.
> Invoke /decision-mapping with the map at <path>, ticket #6.
> Invoke /decision-mapping with the map at <path>, ticket auth-strategy.
> Invoke /decision-mapping with the map at <path>, ticket cache-layer.
> Invoke /decision-mapping with the map at <path>, ticket rate-limits.
> ```
**No open tickets remain.** The fog is pushed back far enough that the path to the finish line is clear — the map is done. (The initial grilling may also surface no fog at all, in which case there was never a map to build.) Recommend implementing directly, or using `/to-prd` to schedule a multi-session implementation.
+22 -2
View File
@@ -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 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. → 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
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: