Compare commits

...
5 Commits
Author SHA1 Message Date
mathias 5070a738e8 docs(review): Python adaptation files for clean-code, solid and code-review
CI / Check / Test (push) Successful in 1s
CI / Tag release (push) Successful in 1s
The canonical Python stack has been pinned in ~/dev/.context/AGENT.md since
local-dev v0.3.1, and three skills still had Go examples with no Python
counterpart. Following the structure #18 settled on, and the existing
solid/references/go-adaptation.md precedent, each gets a reference file rather
than inline dilution of the primary doc.

The stack table itself is NOT copied into any of them — each cites AGENT.md as
its home. This repo spent today learning what a second copy of a fact costs.

Content is the part a linter cannot give you:

- clean-code: `raise X from e` as the analogue of `%w` error wrapping, dataclass
  over `dict[str, Any]`, and the two traps where the broken version reads
  tidier — mutable default arguments, and `if not amount:` for a value where
  zero is legitimate.
- solid: `Protocol` over `ABC`, defined at the consumer, which is what makes DIP
  real in Python; plus what does NOT carry over from the Go rewrite (return
  structs/accept interfaces, embedding, package-as-deployment-unit).
- code-review: a Python anti-pattern table parallel to the Go one, opening with
  the rule that a finding ruff or mypy already catches is not a review comment,
  it is a question about why the gate is not running.

Refs #20
Bump-Type: minor
2026-08-29 14:10:10 +02:00
mathias 8da7c9c45f docs(tdd): make the spine language-neutral, add four language references
The discipline in tdd was already language-agnostic; the checklist under it was
not. A Python user inherited a mandatory `go test -race` box they can never tick
and never got told that `mypy --strict` belongs in the red gate.

Per #18's own comment and writing-skills' Multi-Language Dilution warning, the
main SKILL.md keeps ONE canonical inline example (Go, the default stack) and the
other four languages live in reference files: python, typescript, rust, java.

The RED run drove every change. A subagent given a Python task with the Go-only
skill reported, in its own words:

- "Everything from L169 to L244 is Go-only, and the verification checklist at
  L326 silently inherits that Go-ness — which is why a Python user ends up with
  a mandatory box (-race) they can never tick and a required gate (mypy
  --strict) that isn't listed at all."
- "For a brand-new function, the first RED is always a collection-time
  ImportError... The only clean resolutions are a stub that raises
  NotImplementedError (production code before a red test?) or accepting
  ImportError as a legitimate red. The skill picks neither, and this is the
  single biggest gap." It now picks: that red is legitimate, and writing a stub
  to satisfy the runner is the Iron Law with extra steps.
- The Go table-driven exemplar branches on a `wantErr` column inside the loop.
  Translated literally that is a conditional assertion folding two behaviours
  into one test — which this skill's own quality table forbids. The agent caught
  itself mid-translation; both SKILL.md and python.md now say not to port it.

Also: the checklist names the static-analysis gate per language instead of
assuming `go vet`, and says to strike the concurrency box deliberately where no
equivalent exists — a box that cannot be ticked teaches people to skip
checklists.

Refs #18
Bump-Type: minor
2026-08-29 14:10:10 +02:00
mathias 6e5024d498 chore(release): plugin.json version 0.12.1
Skip-Release: true
2026-08-29 09:48:12 +00:00
mathias 62cfe46927 docs(superpowers): point the vendored cluster at the tools this harness has
CI / Check / Test (push) Successful in 1s
CI / Tag release (push) Successful in 0s
The vendored skills were written against a generic `Task()` and an operator
called "your human partner". Neither exists here. An agent following
dispatching-parallel-agents literally would call a tool that is not in its list.

`Task tool` becomes the `Agent` tool with `subagent_type` across all six prompt
templates and the Gemini mapping table, and the dispatch example gains the three
options that actually matter for parallel work: issuing every dispatch in ONE
message (several Agent uses in a single turn run concurrently; one per turn
serialises them), `isolation: "worktree"` so two agents cannot fight over the
same tree, and per-agent `model` override for cheap mechanical fan-out.

`Workflow` is named as the structured alternative — phases, per-finding verify,
fan-in — with its opt-in condition stated, because a skill that told an agent to
reach for it unprompted would be teaching a policy violation.

"your human partner" becomes "the operator" in five skills. Beyond phrasing, it
was factually wrong in executing-plans, which told the reader to *ask* whether
subagents are available; on this fleet Claude Code has them and Crush/Pi/OpenCode
do not, so it now says which skill to use where.

Closes #17
Bump-Type: patch
2026-08-29 11:43:46 +02:00
mathias de84800892 chore(release): plugin.json version 0.12.0
Skip-Release: true
2026-08-29 09:41:44 +00:00
26 changed files with 722 additions and 40 deletions
+1 -1
View File
@@ -1,7 +1,7 @@
{ {
"name": "skills", "name": "skills",
"description": "Mathias's engineering skills library, plus the hooks and MCP servers that make it enforceable: TDD, clean code, debugging, specs, discovery, CI, and secret hygiene.", "description": "Mathias's engineering skills library, plus the hooks and MCP servers that make it enforceable: TDD, clean code, debugging, specs, discovery, CI, and secret hygiene.",
"version": "0.11.0", "version": "0.12.1",
"author": { "author": {
"name": "Mathias", "name": "Mathias",
"url": "https://git.d-ma.be/mathias" "url": "https://git.d-ma.be/mathias"
+1
View File
@@ -281,5 +281,6 @@ During refactoring:
- `references/clean-code.md` — detailed naming rules, object calisthenics - `references/clean-code.md` — detailed naming rules, object calisthenics
- `references/code-smells.md` — comprehensive smell catalog with examples - `references/code-smells.md` — comprehensive smell catalog with examples
- `references/python-adaptation.md` — Python conventions: error chaining, dataclasses, the truthiness trap
- Load `solid` skill for SOLID principles with Go examples - Load `solid` skill for SOLID principles with Go examples
- Load `refactoring` skill for systematic refactoring techniques - Load `refactoring` skill for systematic refactoring techniques
@@ -0,0 +1,104 @@
# Clean Code: Python Adaptation
The principles in `SKILL.md` are language-agnostic. This file covers the places where
Python's idioms differ from the Go examples, and where following the Go shape produces
bad Python.
The canonical stack (ruff, mypy --strict, pytest, uv, pydantic-settings, src-layout)
lives in `~/dev/.context/AGENT.md` and is not restated here — one source of truth, and
it is not this file.
## Reveal intent with types, not comments
`mypy --strict` is the Python analogue of Go's compiler, and it is non-negotiable for
the same reason: a type is a claim the machine checks, and a comment is a claim nobody
checks. Two consequences for clean code specifically:
- **`Any` is a hole in the argument.** Each one says "the reader is on their own here".
If a value is genuinely polymorphic, say so precisely — `Protocol`, a union, a
`TypeVar` — rather than surrendering.
- **A `# type: ignore` needs the error code and a reason**: `# type: ignore[arg-type] #
boto3 stubs lag the API`. A bare ignore is an unexplained exception to a rule the rest
of the file follows.
## Errors: chain them, never swallow them
Go's `fmt.Errorf("operation: %w", err)` is `raise` with `from`:
```python
# Bad — the original cause is gone, and so is the stack that produced it
try:
row = fetch(invoice_id)
except KeyError:
raise InvoiceNotFound(invoice_id)
# Good — the chain survives; the traceback shows both
try:
row = fetch(invoice_id)
except KeyError as e:
raise InvoiceNotFound(invoice_id) from e
```
Two rules with no exceptions:
- **Never bare `except:`.** It catches `KeyboardInterrupt` and `SystemExit` too, so it
turns Ctrl-C into a swallowed error. `except Exception` if you truly mean everything.
- **Never a silent `pass`.** An exception handler that does nothing is a decision to
ignore a failure; if that is genuinely right, the handler needs a comment saying why,
and usually a log line. Most of the time it is a bug being stored for later.
## Data classes over dicts
A `dict[str, Any]` passed between functions is Go's `map[string]interface{}` — every
reader has to reconstruct the shape from the code that writes it.
```python
# Bad: shape known only by reading every producer
def total(invoice: dict[str, Any]) -> int: ...
# Good: shape declared once, checked everywhere
@dataclass(frozen=True, slots=True)
class Invoice:
id: InvoiceId
lines: tuple[Line, ...]
currency: str # ISO 4217
def total(invoice: Invoice) -> Money: ...
```
`frozen=True` where the value is conceptually immutable — it makes accidental mutation
an error rather than a surprise at a distance. Use `pydantic` models at the *boundary*
(env config, HTTP payloads) where validation is the point, and dataclasses inside,
where validation has already happened.
## Two Python-specific traps that read as clean code
**Mutable default arguments** are evaluated once, at definition:
```python
def add_line(line: Line, lines: list[Line] = []) -> list[Line]: # shared across calls
```
Use `None` and build inside. Ruff catches this (`B006`); the point of naming it here is
that the broken version looks tidier than the correct one.
**Truthiness is not presence.** `if not amount:` is true for `0`, `0.0`, `""`, `[]` and
`None` alike. For anything where zero is a legitimate value — money, counts, offsets —
write `if amount is None:`. This is the single most common way a Python function
silently mishandles a valid input.
## Structure
- `src/` layout, `pyproject.toml` only. A flat layout lets tests import the working
directory instead of the installed package, so the suite passes on a package that
cannot actually be installed.
- Module names are the unit of cohesion, as Go packages are. `utils.py` is the Python
spelling of a god package: it has no reason to change because it has no subject.
- Prefer functions to classes with one method. A class whose only job is to hold
constructor arguments and expose `run()` is a closure with ceremony.
## See also
- `references/clean-code.md` — the language-agnostic practices
- `references/code-smells.md` — the smell catalogue, all of which apply here
- `tdd/references/python.md` — the test-side conventions
+2
View File
@@ -279,6 +279,8 @@ treats `pass` as success, `fail` as failure, `skip` as neither.
## References ## References
- `references/python-adaptation.md` — the Python anti-pattern table, and what ruff/mypy
should have caught before review
- `clean-code/references/code-smells.md` — comprehensive smell catalog - `clean-code/references/code-smells.md` — comprehensive smell catalog
- Load `refactoring` skill when recommending fixes - Load `refactoring` skill when recommending fixes
- Load `test-design` skill for reviewing test quality - Load `test-design` skill for reviewing test quality
@@ -0,0 +1,60 @@
# Code Review: Python Anti-Patterns
`SKILL.md` carries the Go anti-pattern table. This is the Python one. The review
process, severity classification and output format above it are language-neutral and
apply unchanged.
Canonical stack in `~/dev/.context/AGENT.md`. Two gates decide most of this file:
`ruff` and `mypy --strict`. **If a finding is something ruff or mypy already catches,
the review comment is not "fix this line" — it is "why is the gate not running?"**
## The table
| Anti-pattern | What to look for | Why it matters |
|---|---|---|
| Bare `except:` | `except:` with no exception class | Swallows `KeyboardInterrupt` and `SystemExit`; Ctrl-C stops working |
| Silent `pass` | `except X: pass` with no comment | A decision to ignore a failure, recorded nowhere |
| Lost cause | `raise Y(...)` inside `except X as e` with no `from e` | Traceback loses the original failure — the Python analogue of a naked `fmt.Errorf` |
| Mutable default | `def f(x, acc=[])` or `={}` | Evaluated once at definition; state leaks between calls |
| Truthiness for presence | `if not amount:` where `0` is valid | Silently mishandles zero, empty string, empty list |
| `Any` as an escape | `Any` in a signature that could be `Protocol`/union/`TypeVar` | Removes exactly the checking `--strict` was adopted for |
| Unexplained ignore | bare `# type: ignore` | No error code, no reason; permanent by default |
| `assert` in production code | `assert user.is_admin` outside tests | `python -O` strips asserts — the check vanishes in a tuned deployment |
| Import-time side effects | connections, env reads, or I/O at module scope | Cannot be injected, cannot be tested, ordering-dependent |
| Sync I/O in `async def` | `requests.get`, `time.sleep`, blocking file reads | Blocks the whole event loop; looks concurrent, is not |
| Un-awaited coroutine | a call to an `async def` with no `await` | Silently does nothing; "coroutine was never awaited" is a warning, not an error |
| `float` for money | `float` amounts, `round()` on currency | `0.1 + 0.2 != 0.3`; use `Decimal` or int minor units |
| Broad `Exception` re-raise | `except Exception` around a whole function body | Turns a specific failure into an unclassifiable one |
| Mutating a shared default/global | module-level `list`/`dict` mutated by functions | The Python spelling of shared mutable state |
| `dict[str, Any]` as a domain type | dicts passed between layers | Shape known only by reading every producer; use a dataclass |
| Star imports | `from x import *` | Breaks `--strict`, hides shadowing, defeats grep |
| `os.path` string surgery | manual path joining, `+ "/"` | `pathlib` exists and is correct on every platform |
| `subprocess(..., shell=True)` with interpolation | f-strings into a shell command | Injection; pass a list, no shell |
## What to check that a linter cannot
- **Does the test suite assert the failure mode, or only the happy path?** `pytest.raises`
with no `match=` accepts any message, including one from an unrelated bug.
- **Are fixtures doing real work?** A `conftest.py` fixture with a `session` scope and
mutable state couples every test that touches it; failures then depend on ordering.
- **Is the mock mocking a boundary or a collaborator you own?** The latter tests the mock.
- **Does the type say what the code means?** `-> dict` and `-> tuple[int, int]` both pass
`--strict` and both tell the reader nothing. A named type is a review comment worth
making.
- **Would `--strict` actually run on this file?** Check for a per-module override in
`pyproject.toml` silently exempting it. An exemption nobody revisits is a hole with a
changelog entry.
## Severity in this context
- **Blocking:** anything in the table above under security or correctness — `shell=True`
interpolation, `assert` as an authorization check, `float` money, sync I/O in async.
- **Should fix:** typing escapes (`Any`, unexplained ignores), mutable defaults,
truthiness-for-presence.
- **Consider:** structural comments — dataclass instead of dict, narrower Protocol.
## See also
- `clean-code/references/python-adaptation.md` — the style-level conventions
- `solid/references/python-adaptation.md` — Protocol over ABC, DIP in Python
- `tdd/references/python.md` — the test-side conventions this review checks against
+1
View File
@@ -258,5 +258,6 @@ Domain types know nothing about HTTP, SQL, or external APIs.
- `references/solid-principles.md` — canonical SOLID reference with TypeScript examples - `references/solid-principles.md` — canonical SOLID reference with TypeScript examples
- `references/go-adaptation.md` — this workspace's Go-specific rewrite of each principle - `references/go-adaptation.md` — this workspace's Go-specific rewrite of each principle
- `references/python-adaptation.md` — the Python rewrite: Protocol over ABC, and what does not carry over from Go
- Load `clean-code` skill for naming and structure - Load `clean-code` skill for naming and structure
- Load `code-review` skill for detecting violations during review - Load `code-review` skill for detecting violations during review
@@ -0,0 +1,104 @@
# SOLID Principles: Python Adaptation
`references/go-adaptation.md` rewrites each principle for Go. This file does the same
for Python, and marks the places where the Go rewrite's advice does *not* carry over.
The canonical Python stack lives in `~/dev/.context/AGENT.md`; it is not restated here.
## The one idea that changes everything: `Protocol`
Go's interfaces are structural — a type satisfies one by having the methods, with no
declaration. Python has the same thing since 3.8, and it is the right default:
```python
from typing import Protocol
class InvoiceStore(Protocol):
def get(self, invoice_id: InvoiceId) -> Invoice: ...
def save(self, invoice: Invoice) -> None: ...
```
- **Define the Protocol where it is *consumed*, not where it is implemented.** This is
Go's "accept interfaces, return structs" and it is what makes DIP real: the high-level
module owns the abstraction, and the adapter depends on it rather than the reverse.
- **Prefer `Protocol` to `ABC`.** An ABC forces implementers to inherit, which couples
them to your package and makes a third-party class impossible to adapt without a
wrapper. Use an ABC only when you genuinely need shared implementation, not just a
contract.
- Add `@runtime_checkable` only if you actually call `isinstance` — and if you are
calling `isinstance` against a Protocol, check whether the real problem is a missing
polymorphic method (an LSP smell).
## S — Single Responsibility
Same as Go: the unit is the module, not just the class. `utils.py` is the Python god
package — it cannot have one reason to change because it has no subject.
Python-specific tell: a class whose `__init__` stores arguments and whose only other
method is `run()` or `execute()`. That is a function with ceremony. Write the function.
## O — Open/Closed
Extension in Python usually means **passing behaviour in**, not subclassing:
```python
# Closed for modification: adding a rule does not touch this function
def apply_rules(invoice: Invoice, rules: Sequence[Rule]) -> Invoice: ...
```
`functools.singledispatch` is the idiomatic open extension point when dispatch is on
type and the set of types grows outside your module. Beware the alternative that looks
Pythonic and is not: monkey-patching is modification pretending to be extension, and it
is invisible at the call site.
## L — Liskov
The Python-specific violation is **signature widening/narrowing that the type checker
would catch if you let it**. Under `mypy --strict`, an override that narrows a parameter
type or widens a return type is an error — which means most LSP violations in Python are
type errors you chose not to see. This is the strongest argument for `--strict` in a
design document rather than a style one.
Raising a *new* exception type in an override is the violation `mypy` will not catch:
callers wrote `except StoreError`, and your subclass raises `TimeoutError`. Exceptions
are part of the contract; Python just will not check them for you.
## I — Interface Segregation
Protocols make this cheap: define the narrow one you need at the call site.
```python
class InvoiceReader(Protocol):
def get(self, invoice_id: InvoiceId) -> Invoice: ...
```
A function that only reads should accept `InvoiceReader`, not the full `InvoiceStore`.
The test double then implements one method instead of eight, which is the practical
payoff and the reason this principle survives contact with real code.
## D — Dependency Inversion
Constructor injection, exactly as in Go. Two Python-specific hazards:
- **Import-time side effects are hidden dependencies.** A module that opens a database
connection or reads an env var at import has a dependency nothing can inject and no
test can replace. Configuration belongs in `pydantic-settings` objects passed in.
- **Default arguments are compile-time bindings.** `def send(client: Client = Client())`
constructs the default once, at definition, and welds it to every caller. Default to
`None` and construct inside, or require it.
## What does not carry over from the Go rewrite
- **"Return structs, accept interfaces"** — the returning half is about Go's zero values
and nil interfaces. In Python, returning a Protocol type is fine and often better.
- **Small interfaces because of embedding** — Python has no embedding; composition is
explicit delegation, which is more verbose but reads honestly.
- **Package-level responsibility as the unit of deployment** — Python packaging is much
looser. The module boundary is still the right unit for *reasoning*, but do not expect
it to be enforced.
## See also
- `references/solid-principles.md` — the canonical, language-neutral reference
- `references/go-adaptation.md` — the Go rewrite; contrast is instructive
- `clean-code/references/python-adaptation.md` — style-level Python conventions
@@ -7,7 +7,7 @@ Use this template when dispatching a spec document reviewer subagent.
**Dispatch after:** Spec document is written to docs/superpowers/specs/ **Dispatch after:** Spec document is written to docs/superpowers/specs/
``` ```
Task tool (general-purpose): Agent tool (`subagent_type: "general-purpose"`):
description: "Review spec document" description: "Review spec document"
prompt: | prompt: |
You are a spec document reviewer. Verify this spec is complete and ready for planning. You are a spec document reviewer. Verify this spec is complete and ready for planning.
@@ -68,13 +68,30 @@ Each agent gets:
### 3. Dispatch in Parallel ### 3. Dispatch in Parallel
```typescript Issue every dispatch in **one** message — several `Agent` tool uses in a single
// In Claude Code / AI environment assistant turn run concurrently; one per turn runs them in series.
Task("Fix agent-tool-abort.test.ts failures")
Task("Fix batch-completion-behavior.test.ts failures")
Task("Fix tool-approval-race-conditions.test.ts failures")
// All three run concurrently
``` ```
Agent(description: "fix abort tests", subagent_type: "general-purpose", prompt: ...)
Agent(description: "fix batch tests", subagent_type: "general-purpose", prompt: ...)
Agent(description: "fix approval tests", subagent_type: "general-purpose", prompt: ...)
```
Options that matter for this pattern:
- `run_in_background: true` (the default) returns immediately and notifies on
completion. Set it to `false` only when your very next action depends on the result.
- `isolation: "worktree"` gives an agent its own git worktree. Use it whenever two
agents might touch the same files — parallel writers to one tree is the failure this
skill's independence rule exists to avoid, and a worktree makes it structural rather
than a matter of discipline.
- `model` overrides the model per agent; cheap mechanical fan-out does not need the
same model as the reasoning that planned it.
For a fan-out with real structure — phases, a verify pass per finding, fan-in — the
`Workflow` tool expresses it directly with `agent()`, `parallel()` and `pipeline()`.
It spawns many agents and costs accordingly, so it runs **only on explicit opt-in from
the operator**; do not reach for it just because a task looks parallel.
### 4. Review and Integrate ### 4. Review and Integrate
+4 -2
View File
@@ -14,14 +14,16 @@ Load plan, review critically, execute all tasks, report when complete.
**Announce at start:** "I'm using the executing-plans skill to implement this plan." **Announce at start:** "I'm using the executing-plans skill to implement this plan."
**Note:** Tell your human partner that Superpowers works much better with access to subagents. The quality of its work will be significantly higher if run on a platform with subagent support (such as Claude Code or Codex). If subagents are available, use superpowers:subagent-driven-development instead of this skill. **Note:** This skill is the fallback for harnesses with no subagents. Claude Code has
them — the `Agent` tool — so on this fleet prefer `superpowers-subagent-driven-development`
and use this skill only where `Agent` is genuinely unavailable (Crush, Pi, OpenCode).
## The Process ## The Process
### Step 1: Load and Review Plan ### Step 1: Load and Review Plan
1. Read plan file 1. Read plan file
2. Review critically - identify any questions or concerns about the plan 2. Review critically - identify any questions or concerns about the plan
3. If concerns: Raise them with your human partner before starting 3. If concerns: Raise them with the operator before starting
4. If no concerns: Create TodoWrite and proceed 4. If no concerns: Create TodoWrite and proceed
### Step 2: Execute Tasks ### Step 2: Execute Tasks
@@ -54,7 +54,7 @@ WHY: Items may be related. Partial understanding = wrong implementation.
**Example:** **Example:**
``` ```
your human partner: "Fix 1-6" the operator: "Fix 1-6"
You understand 1,2,3,6. Unclear on 4,5. You understand 1,2,3,6. Unclear on 4,5.
❌ WRONG: Implement 1,2,3,6 now, ask about 4,5 later ❌ WRONG: Implement 1,2,3,6 now, ask about 4,5 later
@@ -63,7 +63,7 @@ You understand 1,2,3,6. Unclear on 4,5.
## Source-Specific Handling ## Source-Specific Handling
### From your human partner ### From the operator
- **Trusted** - implement after understanding - **Trusted** - implement after understanding
- **Still ask** if scope unclear - **Still ask** if scope unclear
- **No performative agreement** - **No performative agreement**
@@ -84,11 +84,11 @@ IF suggestion seems wrong:
IF can't easily verify: IF can't easily verify:
Say so: "I can't verify this without [X]. Should I [investigate/ask/proceed]?" Say so: "I can't verify this without [X]. Should I [investigate/ask/proceed]?"
IF conflicts with your human partner's prior decisions: IF conflicts with the operator's prior decisions:
Stop and discuss with your human partner first Stop and discuss with the operator first
``` ```
**your human partner's rule:** "External feedback - be skeptical, but check carefully" **the operator's rule:** "External feedback - be skeptical, but check carefully"
## YAGNI Check for "Professional" Features ## YAGNI Check for "Professional" Features
@@ -100,7 +100,7 @@ IF reviewer suggests "implementing properly":
IF used: Then implement properly IF used: Then implement properly
``` ```
**your human partner's rule:** "You and reviewer both report to me. If we don't need this feature, don't add it." **the operator's rule:** "You and reviewer both report to me. If we don't need this feature, don't add it."
## Implementation Order ## Implementation Order
@@ -123,13 +123,13 @@ Push back when:
- Violates YAGNI (unused feature) - Violates YAGNI (unused feature)
- Technically incorrect for this stack - Technically incorrect for this stack
- Legacy/compatibility reasons exist - Legacy/compatibility reasons exist
- Conflicts with your human partner's architectural decisions - Conflicts with the operator's architectural decisions
**How to push back:** **How to push back:**
- Use technical reasoning, not defensiveness - Use technical reasoning, not defensiveness
- Ask specific questions - Ask specific questions
- Reference working tests/code - Reference working tests/code
- Involve your human partner if architectural - Involve the operator if architectural
**Signal if uncomfortable pushing back out loud:** "Strange things are afoot at the Circle K" **Signal if uncomfortable pushing back out loud:** "Strange things are afoot at the Circle K"
@@ -200,7 +200,7 @@ Reviewer: "Implement proper metrics tracking with database, date filters, CSV ex
**Unclear Item (Good):** **Unclear Item (Good):**
``` ```
your human partner: "Fix items 1-6" the operator: "Fix items 1-6"
You understand 1,2,3,6. Unclear on 4,5. You understand 1,2,3,6. Unclear on 4,5.
✅ "Understand 1,2,3,6. Need clarification on 4 and 5 before implementing." ✅ "Understand 1,2,3,6. Need clarification on 4 and 5 before implementing."
``` ```
@@ -36,7 +36,9 @@ HEAD_SHA=$(git rev-parse HEAD)
**2. Dispatch code reviewer subagent:** **2. Dispatch code reviewer subagent:**
Use Task tool with `general-purpose` type, fill template at `code-reviewer.md` Use the `Agent` tool with `subagent_type: "general-purpose"`, filling the template at
`code-reviewer.md`. A reviewer needs no write access and benefits from none — if the
harness offers a read-only agent type, prefer it.
**Placeholders:** **Placeholders:**
- `{DESCRIPTION}` - Brief summary of what you built - `{DESCRIPTION}` - Brief summary of what you built
@@ -5,7 +5,7 @@ Use this template when dispatching a code reviewer subagent.
**Purpose:** Review completed work against requirements and code quality standards before it cascades into more work. **Purpose:** Review completed work against requirements and code quality standards before it cascades into more work.
``` ```
Task tool (general-purpose): Agent tool (`subagent_type: "general-purpose"`):
description: "Review code changes" description: "Review code changes"
prompt: | prompt: |
You are a Senior Code Reviewer with expertise in software architecture, You are a Senior Code Reviewer with expertise in software architecture,
@@ -14,7 +14,7 @@ Execute plan by dispatching fresh subagent per task, with two-stage review after
**Core principle:** Fresh subagent per task + two-stage review (spec then quality) = high quality, fast iteration **Core principle:** Fresh subagent per task + two-stage review (spec then quality) = high quality, fast iteration
**Continuous execution:** Do not pause to check in with your human partner between tasks. Execute all tasks from the plan without stopping. The only reasons to stop are: BLOCKED status you cannot resolve, ambiguity that genuinely prevents progress, or all tasks complete. "Should I continue?" prompts and progress summaries waste their time — they asked you to execute the plan, so execute it. **Continuous execution:** Do not pause to check in with the operator between tasks. Execute all tasks from the plan without stopping. The only reasons to stop are: BLOCKED status you cannot resolve, ambiguity that genuinely prevents progress, or all tasks complete. "Should I continue?" prompts and progress summaries waste their time — they asked you to execute the plan, so execute it.
## When to Use ## When to Use
@@ -7,7 +7,7 @@ Use this template when dispatching a code quality reviewer subagent.
**Only dispatch after spec compliance review passes.** **Only dispatch after spec compliance review passes.**
``` ```
Task tool (general-purpose): Agent tool (`subagent_type: "general-purpose"`):
Use template at requesting-code-review/code-reviewer.md Use template at requesting-code-review/code-reviewer.md
DESCRIPTION: [task summary, from implementer's report] DESCRIPTION: [task summary, from implementer's report]
@@ -3,7 +3,7 @@
Use this template when dispatching an implementer subagent. Use this template when dispatching an implementer subagent.
``` ```
Task tool (general-purpose): Agent tool (`subagent_type: "general-purpose"`):
description: "Implement Task N: [task name]" description: "Implement Task N: [task name]"
prompt: | prompt: |
You are implementing Task N: [task name] You are implementing Task N: [task name]
@@ -5,7 +5,7 @@ Use this template when dispatching a spec compliance reviewer subagent.
**Purpose:** Verify implementer built what was requested (nothing more, nothing less) **Purpose:** Verify implementer built what was requested (nothing more, nothing less)
``` ```
Task tool (general-purpose): Agent tool (`subagent_type: "general-purpose"`):
description: "Review spec compliance for Task N" description: "Review spec compliance for Task N"
prompt: | prompt: |
You are reviewing whether an implementation matches its specification. You are reviewing whether an implementation matches its specification.
@@ -24,11 +24,11 @@ When a skill says to dispatch a named agent type, use `@generalist` with the ful
| Skill instruction | Gemini CLI equivalent | | Skill instruction | Gemini CLI equivalent |
|-------------------|----------------------| |-------------------|----------------------|
| `Task tool (superpowers:implementer)` | `@generalist` with the filled `implementer-prompt.md` template | | `Agent tool (superpowers:implementer)` | `@generalist` with the filled `implementer-prompt.md` template |
| `Task tool (superpowers:spec-reviewer)` | `@generalist` with the filled `spec-reviewer-prompt.md` template | | `Agent tool (superpowers:spec-reviewer)` | `@generalist` with the filled `spec-reviewer-prompt.md` template |
| `Task tool (superpowers:code-reviewer)` | `@code-reviewer` (bundled agent) or `@generalist` with the filled review prompt | | `Agent tool (superpowers:code-reviewer)` | `@code-reviewer` (bundled agent) or `@generalist` with the filled review prompt |
| `Task tool (superpowers:code-quality-reviewer)` | `@generalist` with the filled `code-quality-reviewer-prompt.md` template | | `Agent tool (superpowers:code-quality-reviewer)` | `@generalist` with the filled `code-quality-reviewer-prompt.md` template |
| `Task tool (general-purpose)` with inline prompt | `@generalist` with your inline prompt | | `Agent tool (general-purpose)` with inline prompt | `@generalist` with your inline prompt |
### Prompt filling ### Prompt filling
@@ -111,7 +111,7 @@ Skip any step = lying, not verifying
## Why This Matters ## Why This Matters
From 24 failure memories: From 24 failure memories:
- your human partner said "I don't believe you" - trust broken - the operator said "I don't believe you" - trust broken
- Undefined functions shipped - would crash - Undefined functions shipped - would crash
- Missing requirements shipped - incomplete features - Missing requirements shipped - incomplete features
- Time wasted on false completion → redirect → rework - Time wasted on false completion → redirect → rework
@@ -7,7 +7,7 @@ Use this template when dispatching a plan document reviewer subagent.
**Dispatch after:** The complete plan is written. **Dispatch after:** The complete plan is written.
``` ```
Task tool (general-purpose): Agent tool (`subagent_type: "general-purpose"`):
description: "Review plan document" description: "Review plan document"
prompt: | prompt: |
You are a plan document reviewer. Verify this plan is complete and ready for implementation. You are a plan document reviewer. Verify this plan is complete and ready for implementation.
+2 -2
View File
@@ -246,7 +246,7 @@ Always use subagents (50-100x context savings). REQUIRED: Use [other-skill-name]
**Compress examples:** **Compress examples:**
```markdown ```markdown
# ❌ BAD: Verbose example (42 words) # ❌ BAD: Verbose example (42 words)
your human partner: "How did we handle authentication errors in React Router before?" the operator: "How did we handle authentication errors in React Router before?"
You: I'll search past conversations for React Router authentication patterns. You: I'll search past conversations for React Router authentication patterns.
[Dispatch subagent with search query: "React Router authentication error handling 401"] [Dispatch subagent with search query: "React Router authentication error handling 401"]
@@ -318,7 +318,7 @@ digraph when_flowchart {
See @graphviz-conventions.dot for graphviz style rules. See @graphviz-conventions.dot for graphviz style rules.
**Visualizing for your human partner:** Use `render-graphs.js` in this directory to render a skill's flowcharts to SVG: **Visualizing for the operator:** Use `render-graphs.js` in this directory to render a skill's flowcharts to SVG:
```bash ```bash
./render-graphs.js ../some-skill # Each diagram separately ./render-graphs.js ../some-skill # Each diagram separately
./render-graphs.js ../some-skill --combine # All diagrams in one SVG ./render-graphs.js ../some-skill --combine # All diagrams in one SVG
+37 -6
View File
@@ -100,13 +100,21 @@ go test -run TestRetryOperation_RetriesThreeTimes ./...
``` ```
Confirm: Confirm:
- Test fails (not compilation errors) - Test fails, and fails because the feature is missing
- Failure message is expected - The failure message is the one you expected
- Fails because feature is missing (not typos) - It is not failing on a typo in the test itself
**Test passes?** You're testing existing behavior. Fix test. **Test passes?** You're testing existing behavior. Fix test.
**Compilation errors?** Fix errors, re-run until it fails correctly. **Failing because the thing does not exist yet?** That is a legitimate red — a Go
compile error for an undefined function, a Python `ImportError` for a module you have
not written, a TypeScript "cannot find module". Read the message, confirm it names the
unit you are about to build, and proceed to GREEN. **Do not write a stub to make the
runner happier.** A stub is production code written before a failing test, which is the
Iron Law with extra steps.
**Failing on a typo or a broken import elsewhere?** Different thing. Fix it and re-run
until the failure is the one above.
### GREEN - Minimal Code ### GREEN - Minimal Code
@@ -166,6 +174,23 @@ Keep tests green. Don't add behavior.
Next failing test for next behavior. Next failing test for next behavior.
## Other languages
The discipline above is language-neutral; the examples below it are Go, because Go is
this fleet's default stack and one good example beats five mediocre ones.
For anything else, the per-language conventions live in reference files — load the one
you need rather than translating Go idioms by eye. Translation is where the mistakes
happen: the Go table-driven exemplar below branches on a `wantErr` column inside the
loop, which is idiomatic Go and an anti-pattern in pytest, where it produces a
conditional assertion and folds two behaviours into one test.
- `references/python.md` — pytest, `@pytest.mark.parametrize`, `mypy --strict` in the
RED gate, `pytest.raises(..., match=)`, fixtures vs helpers
- `references/typescript.md` — vitest/jest, `it.each`, `expect(...).rejects`
- `references/rust.md` — `#[cfg(test)]` modules, table-driven via arrays, `#[should_panic]`
- `references/java.md` — JUnit 5, `@ParameterizedTest`, `assertThrows`
## Go-Specific TDD Notes ## Go-Specific TDD Notes
### Table-Driven Tests (Preferred Pattern) ### Table-Driven Tests (Preferred Pattern)
@@ -329,8 +354,14 @@ Before marking work complete:
- [ ] Watched each test fail before implementing - [ ] Watched each test fail before implementing
- [ ] Each test failed for expected reason (feature missing, not typo) - [ ] Each test failed for expected reason (feature missing, not typo)
- [ ] Wrote minimal code to pass each test - [ ] Wrote minimal code to pass each test
- [ ] All tests pass: `go test ./...` - [ ] All tests pass
- [ ] Race detector clean: `go test -race ./...` - [ ] The language's static gate is clean — `go vet`, `mypy --strict`, `tsc --noEmit`,
`cargo clippy`. A red type-check is a red build; it does not wait for GREEN.
- [ ] The language's concurrency gate is clean **where one exists and applies** —
`go test -race`, `cargo test` under Miri for unsafe code. There is no equivalent
for a pure Python function, and a checklist box that can never be ticked teaches
people to skip checklists. Tick it or strike it deliberately; do not leave it
hanging.
- [ ] Tests use real code (mocks only if crossing a system boundary) - [ ] Tests use real code (mocks only if crossing a system boundary)
- [ ] Edge cases and errors covered - [ ] Edge cases and errors covered
+78
View File
@@ -0,0 +1,78 @@
# TDD in Java
The Iron Law and Red-Green-Refactor in `SKILL.md` apply unchanged. This file replaces
the Go-Specific TDD Notes for Java. JUnit 5 (Jupiter) throughout.
## The first red is a compile error, and that is correct
`cannot find symbol: method splitVat` is a legitimate red. Do not stub the method with
`throw new UnsupportedOperationException()` to make the module compile — that is
production code before a failing test.
## Table-driven: `@ParameterizedTest`
```java
@ParameterizedTest(name = "{0}")
@MethodSource("vatCases")
void roundsHalfToEven(String name, long amountMinor, BigDecimal rate, long net, long vat) {
assertEquals(new Split(net, vat), VatSplitter.split(amountMinor, rate));
}
static Stream<Arguments> vatCases() {
return Stream.of(
Arguments.of("tie rounds down to even", 14L, new BigDecimal("0.12"), 12L, 2L),
Arguments.of("tie rounds up to even", 42L, new BigDecimal("0.12"), 38L, 4L),
Arguments.of("exact division", 12500L, new BigDecimal("0.25"), 10000L, 2500L)
);
}
```
`name = "{0}"` puts the case label in the report — the `t.Run` analogue. `@CsvSource` is
terser for primitives; `@MethodSource` is the one to use as soon as a case carries a
`BigDecimal` or a domain object.
## Error cases get their own test
```java
@Test
void rejectsNegativeAmount() {
var e = assertThrows(IllegalArgumentException.class,
() -> VatSplitter.split(-1, new BigDecimal("0.25")));
assertTrue(e.getMessage().contains("non-negative"));
}
```
Assert on the message, not only the type — `IllegalArgumentException` is thrown by half
the JDK, and a test that accepts any of them will keep passing for the wrong reason.
## Money is `BigDecimal` or `long` minor units, never `double`
`0.1 + 0.2 != 0.3` is not a rounding curiosity in a payments system, it is a defect.
Where a rounding mode matters, state it: `setScale(0, RoundingMode.HALF_EVEN)`. Banker's
rounding is the default expectation in Swedish accounting and is *not* Java's default.
## Assertions
Prefer AssertJ (`assertThat(x).isEqualTo(y)`) where the project already has it: the
failure messages name the actual value. Where it does not, plain JUnit assertions are
fine — do not add a dependency for aesthetics.
`assertAll` is the closest analogue to Go's soft `assert`: it reports every failure in
the group instead of stopping at the first.
## Mocks
Mockito is standard, and easy to overuse. Per `SKILL.md`, mock only across a system
boundary. A mocked class you own tests the mock; a `@SpringBootTest` that mocks the
repository tests neither the wiring nor the query.
## Gates
```bash
./gradlew test # the failing test
./gradlew check # static gate: compiler warnings, spotless, errorprone
```
Java has no `-race`. For concurrent code the honest substitutes are a stress test with
`CountDownLatch` to force the interleaving, and jcstress for lock-free work. A pure
function should strike that checklist box deliberately.
+109
View File
@@ -0,0 +1,109 @@
# TDD in Python
The Iron Law, Red-Green-Refactor and the rationalization table in `SKILL.md` apply
unchanged. This file replaces the Go-Specific TDD Notes for Python work.
Canonical stack (`~/dev/.context/AGENT.md`): pytest + pytest-cov, `mypy --strict`, ruff,
uv, src-layout. Not restated here — that table has one home.
## The first red is an ImportError, and that is correct
For a function that does not exist yet, the first run fails at collection:
```
E ModuleNotFoundError: No module named 'invoicing.vat'
```
That is a legitimate red. Read it, confirm it names the module you are about to write,
and go to GREEN. **Do not create a stub raising `NotImplementedError` to make pytest
collect.** A stub is production code written before a failing test.
## Table-driven: `parametrize`, one behaviour per function
```python
@pytest.mark.parametrize(
("amount_minor", "rate", "expected"),
[
pytest.param(14, Decimal("0.12"), (12, 2), id="tie-rounds-down-to-even"),
pytest.param(42, Decimal("0.12"), (38, 4), id="tie-rounds-up-to-even"),
pytest.param(12500, Decimal("0.25"), (10000, 2500), id="exact-division"),
pytest.param(0, Decimal("0.25"), (0, 0), id="zero-is-not-an-error"),
],
)
def test_split_vat_rounds_half_to_even(
amount_minor: int, rate: Decimal, expected: tuple[int, int]
) -> None:
assert split_vat(amount_minor, rate) == expected
```
`id=` is the analogue of Go's `t.Run` name: it is what appears in the failure output and
what you pass to `pytest -k`.
**Do not port Go's `wantErr` column.** The Go exemplar branches inside the loop —
`if tt.wantErr { assert.Error } else { assert.NoError }`. Translated literally that
gives a conditional assertion and folds two behaviours into one test, which `SKILL.md`'s
own quality table forbids ("One thing. 'and' in name? Split it."). Error cases get
their own function:
```python
def test_split_vat_rejects_negative_amount() -> None:
with pytest.raises(ValueError, match="amount_minor must be non-negative"):
split_vat(-1, Decimal("0.25"))
```
`match=` pins the message, not just the type. Go's `assert.Error` does not, and the
stricter version is available here for free — use it.
## `mypy --strict` is part of RED, not a chore after GREEN
A red type-check is a red build.
```bash
uv run pytest tests/test_vat.py -x -q # the failing test
uv run mypy --strict src tests # same gate, same phase
```
Under `--strict` every test function needs `-> None`, or it is an untyped def and the
type-checker rejects the file — including the file that holds your failing test.
## Layout and imports
- `src/` layout, tests in a sibling `tests/`. This is the *opposite* of Go's
same-directory convention, and it buys black-box testing structurally: `from
invoicing.vat import split_vat` can only resolve against the installed package, so
the preference Go has to state explicitly is enforced here by the import system.
- No `__init__.py` in `tests/` unless you need shared test packages; pytest's rootdir
handling copes, and an accidental package shadows names.
- `conftest.py` holds fixtures shared across a directory. A fixture in a single test
file that only one test uses should be a plain function call instead — fixtures are
indirection, and indirection is only worth it at reuse.
## No `require` vs `assert`
Every pytest `assert` is fatal to its test; there is no continue-on-failure form. Where
Go would use `assert` to collect several soft failures, either split the test or use
`pytest.approx`/a single structural comparison:
```python
assert result == Invoice(id=InvoiceId("42"), net=10000, vat=2500) # one assertion, whole shape
```
Comparing whole values beats three field assertions: the failure output shows the entire
diff rather than the first field that differed.
## Fixtures vs helpers
- **Helper function** — when the setup is cheap and local. It reads top-to-bottom.
- **Fixture** — when the setup is shared, or needs teardown, or needs a scope
(`session`, `module`) because it is expensive.
- Set `__tracebackhide__ = True` in an assertion helper so failures point at the caller,
the way `t.Helper()` does in Go.
## Coverage is not the concurrency gate
`SKILL.md`'s checklist asks for the language's concurrency gate. Python has no `-race`
equivalent, and pretending `pytest --cov` fills that slot is dishonest — coverage says
which lines ran, not whether two threads can interleave badly. For a pure function,
strike the box deliberately. For genuinely concurrent code (`asyncio`, threads), the
substitute is a test that runs the interleaving you fear, plus `pytest-timeout` so a
deadlock fails rather than hangs.
+86
View File
@@ -0,0 +1,86 @@
# TDD in Rust
The Iron Law and Red-Green-Refactor in `SKILL.md` apply unchanged. This file replaces
the Go-Specific TDD Notes for Rust.
## The first red is a compile error, and that is correct
```
error[E0425]: cannot find function `split_vat` in this scope
```
Legitimate — the compiler is the test runner's first phase. Read it, confirm it names
the item you are about to write, and go to GREEN. Do not write a `todo!()` stub first;
that is production code before a failing test. (`todo!()` is fine *during* GREEN as a
branch you have not reached yet.)
## Where tests live
- **Unit tests** in the same file, in `mod tests` behind `#[cfg(test)]`. They can reach
private items — the white-box option Go spells `package foo`.
- **Integration tests** in `tests/`, which link the crate as an external consumer and see
only the public API. This is the black-box default `SKILL.md` prefers; use it unless
the behaviour under test is genuinely private.
```rust
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn tie_rounds_down_to_even() {
assert_eq!(split_vat(14, dec!(0.12)).unwrap(), (12, 2));
}
}
```
## Table-driven: an array of cases
```rust
#[test]
fn rounds_half_to_even() {
let cases = [
("tie rounds down", 14u64, dec!(0.12), (12, 2)),
("tie rounds up", 42, dec!(0.12), (38, 4)),
("exact division", 12500, dec!(0.25), (10000, 2500)),
];
for (name, amount, rate, want) in cases {
assert_eq!(split_vat(amount, rate).unwrap(), want, "case: {name}");
}
}
```
The `"case: {name}"` message is the `t.Run` analogue — without it a failure names a line
number and leaves you counting rows. If you want each case to be its own reported test,
use `rstest`'s `#[case]`; that is a dependency, so justify it before adding it.
## Error cases
`Result` makes the Go `wantErr` column unnecessary — assert on the variant:
```rust
#[test]
fn rejects_negative_amount() {
let err = split_vat_signed(-1, dec!(0.25)).unwrap_err();
assert!(matches!(err, VatError::NegativeAmount));
}
```
`#[should_panic(expected = "...")]` is for genuine panics only — invariant violations,
not expected error paths. A function whose contract includes "rejects negative input"
should return `Err`, not panic, and a test that accepts a panic hides that design choice.
## Gates
```bash
cargo test # the failing test
cargo clippy -- -D warnings # static gate, same phase as RED
cargo fmt --check
```
`cargo test` compiles in debug, so integer overflow panics rather than wrapping — a
release build will not. Where overflow matters, test the checked path explicitly
(`checked_mul`), or you are testing debug-only behaviour.
For unsafe code, `cargo +nightly miri test` is the nearest thing to Go's `-race`.
Safe-only crates have no equivalent and should strike that checklist box deliberately.
+85
View File
@@ -0,0 +1,85 @@
# TDD in TypeScript
The Iron Law and Red-Green-Refactor in `SKILL.md` apply unchanged. This file replaces
the Go-Specific TDD Notes for TypeScript.
Vitest is the default; Jest differs only in imports and a few matcher names, both noted
where it matters.
## The first red is a resolution error, and that is correct
```
Error: Failed to resolve import "./vat" from "src/vat.test.ts"
```
Legitimate. Do not create an empty module or a stub throwing `new Error("not
implemented")` to get past collection — that is production code before a failing test.
## Table-driven: `it.each`
```ts
describe('splitVat', () => {
it.each([
{ amountMinor: 14, rate: 0.12, expected: [12, 2], name: 'tie rounds down to even' },
{ amountMinor: 42, rate: 0.12, expected: [38, 4], name: 'tie rounds up to even' },
{ amountMinor: 12500, rate: 0.25, expected: [10000, 2500], name: 'exact division' },
])('$name', ({ amountMinor, rate, expected }) => {
expect(splitVat(amountMinor, rate)).toEqual(expected)
})
})
```
`$name` interpolates into the test title, which is the `t.Run` analogue. Prefer the
object form over the array form — a positional tuple with five entries is unreadable at
the point of failure.
**Do not port Go's `wantErr` column.** Error cases get their own test:
```ts
it('rejects a negative amount', () => {
expect(() => splitVat(-1, 0.25)).toThrow(/must be non-negative/)
})
it('rejects a negative amount (async)', async () => {
await expect(splitVat(-1, 0.25)).rejects.toThrow(/must be non-negative/)
})
```
The `await` on the async form is load-bearing. Without it the assertion returns a
promise nobody checks, the test passes, and it will keep passing after you break the
code. This is the single most common false-green in a TypeScript suite.
## `tsc --noEmit` is part of RED
A red type-check is a red build:
```bash
npx vitest run src/vat.test.ts # the failing test
npx tsc --noEmit # same gate, same phase
```
Vitest transpiles without type-checking, so a suite can be green while the code does not
compile. `tsc --noEmit` is not optional here — it is the half of the gate the runner
does not perform.
## Equality matchers are not interchangeable
- `toBe` — `Object.is`. For primitives and identity.
- `toEqual` — structural, and **ignores `undefined` properties**: `{a: 1, b: undefined}`
equals `{a: 1}`. That is a real hole when the bug you are hunting is a missing field.
- `toStrictEqual` — structural, respects `undefined`, and checks class identity. Default
to this one; reach for `toEqual` deliberately.
## Mocks
`vi.mock` (Jest: `jest.mock`) is hoisted above imports, so a factory referencing an
outer variable throws at runtime. Use `vi.hoisted` when the mock needs shared state.
Per `SKILL.md`, mock only across a system boundary — a mocked module you own is a test
of your mock.
## Async and fake timers
- `vi.useFakeTimers()` needs a matching `vi.useRealTimers()` in teardown, or the next
file inherits frozen time and fails somewhere unrelated.
- Prefer condition polling (`await vi.waitFor(...)`) over a fixed `setTimeout` — an
arbitrary wait is a flake with a schedule.