Compare commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
c377c23e21 | ||
|
|
8f9d5dd8bd | ||
|
|
7ba01abdbe | ||
|
|
9a81a707af | ||
|
|
5070a738e8 | ||
|
|
8da7c9c45f | ||
|
|
6e5024d498 | ||
|
|
62cfe46927 | ||
|
|
de84800892 | ||
|
|
51a2d396fd | ||
|
|
bb912e5594 |
@@ -1,7 +1,7 @@
|
||||
{
|
||||
"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.",
|
||||
"version": "0.10.2",
|
||||
"version": "0.13.1",
|
||||
"author": {
|
||||
"name": "Mathias",
|
||||
"url": "https://git.d-ma.be/mathias"
|
||||
|
||||
@@ -12,5 +12,22 @@
|
||||
"type": "http",
|
||||
"url": "https://koala.tail30472e.ts.net:8444/mcp"
|
||||
}
|
||||
},
|
||||
"_parked": {
|
||||
"_reason": "cad and gitea-mcp are proxied, reachable and AUTHENTICATING: tailscale serve 8445 -> 127.0.0.1:31877 and 8446 -> 127.0.0.1:30340; an MCP initialize with the correct bearer returns 200 from both (serverInfo gitea-mcp 0.1.0, cad aed2aaf). They stay parked only because nothing puts those two tokens into the session environment. Both live in the 1Password AgentSecrets vault, and by deliberate design (infra#224, closed) no single identity on koala reads both vaults: koala-homelab is HomeLab-only and resolves ~/.op-env, koala-agentsecrets-write is AgentSecrets-only and lives in ~/.op-agent-write.env. Adding an AgentSecrets reference to ~/.op-env does NOT degrade gracefully -- `op run` fails wholesale and every other token, BRAIN_MCP_TOKEN included, goes unresolved with it. Enabling needs a read-scoped AgentSecrets identity in the session-env path; see mathias/skills#32, #33.",
|
||||
"cad": {
|
||||
"type": "http",
|
||||
"url": "https://koala.tail30472e.ts.net:8445/mcp",
|
||||
"headers": {
|
||||
"Authorization": "Bearer ${CAD_MCP_TOKEN}"
|
||||
}
|
||||
},
|
||||
"gitea": {
|
||||
"type": "http",
|
||||
"url": "https://koala.tail30472e.ts.net:8446/mcp",
|
||||
"headers": {
|
||||
"Authorization": "Bearer ${GITEA_MCP_STATIC_TOKEN}"
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -281,5 +281,6 @@ During refactoring:
|
||||
|
||||
- `references/clean-code.md` — detailed naming rules, object calisthenics
|
||||
- `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 `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
|
||||
@@ -279,6 +279,8 @@ treats `pass` as success, `fail` as failure, `skip` as neither.
|
||||
|
||||
## 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
|
||||
- Load `refactoring` skill when recommending fixes
|
||||
- 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
|
||||
@@ -32,6 +32,90 @@ A feature spec is the implementation-level contract for one feature or component
|
||||
2. **An "Out of scope" section is mandatory.** If you do not draw the boundary, the implementer (you, future-you, or another agent) will guess wrong.
|
||||
3. **Every technical decision in "Technical approach" must have a rationale.** A decision is any choice where a reasonable alternative existed and was not taken. "Use Postgres" is not a decision; "use Postgres because the rest of the stack uses it and we need transactions across user + invoice" is.
|
||||
|
||||
## EARS-lite acceptance criteria
|
||||
|
||||
Measurable criteria answer "is it done?". They do not always answer "what should it do
|
||||
when X happens?" — and behavioural requirements written as prose are where ambiguity
|
||||
survives review. For any criterion that is conditional or event-driven, use EARS-lite
|
||||
instead of prose:
|
||||
|
||||
```
|
||||
WHEN <trigger> THE SYSTEM SHALL <observable response>
|
||||
WHILE <state> THE SYSTEM SHALL <observable response>
|
||||
IF <unwanted condition> THEN THE SYSTEM SHALL <observable response>
|
||||
```
|
||||
|
||||
There is a fourth, unconditional form. Use it for invariants — the properties that must
|
||||
hold with no trigger at all, which are usually the ones that matter most:
|
||||
|
||||
```
|
||||
THE SYSTEM SHALL <observable response>
|
||||
```
|
||||
|
||||
Worked, deliberately from a domain unlike whatever you are specifying:
|
||||
|
||||
```
|
||||
THE SYSTEM SHALL reject a firmware image whose signature does not verify
|
||||
WHEN a sensor reports a temperature above the shutdown limit
|
||||
THE SYSTEM SHALL open the contactor within 200ms
|
||||
WHILE the contactor is open
|
||||
THE SYSTEM SHALL refuse every start command
|
||||
IF a sensor reading is missing or stale by more than one interval
|
||||
THEN THE SYSTEM SHALL treat it as above the limit
|
||||
```
|
||||
|
||||
The example is off-domain on purpose. An on-domain example gets copied: a spec written
|
||||
against an invoice-flavoured example inherited a requirement about extraction confidence
|
||||
that its feature request never mentioned, and the author only noticed on review. Read
|
||||
the shapes, not the content.
|
||||
|
||||
Each conditional line names a trigger and one observable response, which usually maps
|
||||
onto a single test. The `WHILE` and unconditional forms are universally quantified —
|
||||
"every start command", "a firmware image" — so they map onto a property or a set of
|
||||
cases, not one assertion. That is still worth writing: it closes the requirements half
|
||||
of the audit trail, which is otherwise issue-body prose.
|
||||
|
||||
**This is an option, not a replacement.** Threshold criteria ("p99 < 200ms under 100
|
||||
RPS") stay as they are — EARS phrasing adds nothing to a number. Use it for conditional
|
||||
behaviour, error handling, and state-dependent rules.
|
||||
|
||||
## Context snapshot
|
||||
|
||||
The executor of this spec will not see the conversation that produced it, and often
|
||||
cannot see the architecture document either — a different agent, a fresh session, a
|
||||
different machine. A spec that says "see the architecture doc" hands them a lookup they
|
||||
may not be able to perform.
|
||||
|
||||
So inline the specific facts this spec depends on, not the whole document:
|
||||
|
||||
```markdown
|
||||
## Context snapshot
|
||||
Taken from <source> @ <commit-or-date>. Re-check before implementing if stale.
|
||||
|
||||
- The mailbox poller writes rows to `invoices` with `state = 'received'`.
|
||||
- The payment-file writer reads `invoices` and emits one file per batch.
|
||||
- Money is int64 minor units plus an ISO-4217 code; float is banned repo-wide.
|
||||
```
|
||||
|
||||
Three rules keep this from becoming a second source of truth, which is the real
|
||||
objection to inlining and a good one:
|
||||
|
||||
1. **Snapshot what the implementer cannot look up and cannot proceed without.** That is
|
||||
wider than "facts this spec's decisions rest on" — a table name may underpin no
|
||||
decision here and still be the thing that blocks writing the migration. It is still
|
||||
narrower than the whole document: if a fact changes nothing and blocks nothing, leave
|
||||
it out, and say what you left out so the omission reads as deliberate.
|
||||
2. **Always cite source and version.** A snapshot without provenance cannot be
|
||||
revalidated, and an unrevalidatable copy is exactly the drift everyone fears.
|
||||
3. **If you could not read the source either, say so in those words.** Writing a spec
|
||||
against a document you cannot open is common and is not a reason to skip the section
|
||||
— it is a reason to mark the whole block `UNVERIFIED — reconstructed, not read`, and
|
||||
to make confirming it the implementer's first task, before the first failing test.
|
||||
An unmarked guess is the only genuinely dangerous version.
|
||||
4. **The snapshot is evidence, not authority.** Open the section with one line to that
|
||||
effect — "where this disagrees with the source, the source wins and this spec is
|
||||
stale" — rather than annotating each fact.
|
||||
|
||||
## Spec Template
|
||||
|
||||
```markdown
|
||||
@@ -42,12 +126,21 @@ A feature spec is the implementation-level contract for one feature or component
|
||||
What problem does this feature solve? For whom? Why now?
|
||||
(2-4 sentences. If you cannot answer "why now," ask before writing the rest.)
|
||||
|
||||
## Context snapshot
|
||||
[Facts the implementer cannot look up, with source and version. See "Context snapshot" above.]
|
||||
|
||||
## Success Criteria
|
||||
|
||||
- [ ] Criterion 1 — measurable and verifiable
|
||||
- [ ] Criterion 2 — measurable and verifiable
|
||||
- [ ] Criterion 3 — measurable and verifiable
|
||||
|
||||
### Behavioural criteria (EARS-lite)
|
||||
|
||||
[Conditional and event-driven behaviour, one trigger and one observable response per
|
||||
line. Not checkboxes — these are properties, and several need more than one test each.
|
||||
Omit the subsection entirely if the feature has no conditional behaviour.]
|
||||
|
||||
## Constraints
|
||||
|
||||
Non-negotiable requirements the solution must satisfy.
|
||||
|
||||
@@ -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/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 `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
|
||||
@@ -200,6 +200,56 @@ Dev: [full command]
|
||||
[Anything unresolved that needs input before implementation begins]
|
||||
```
|
||||
|
||||
## EARS-lite acceptance criteria
|
||||
|
||||
Measurable criteria answer "is it done?". They do not always answer "what should it do
|
||||
when X happens?" — and behavioural requirements written as prose are where ambiguity
|
||||
survives review. For any criterion that is conditional or event-driven, use EARS-lite
|
||||
instead of prose:
|
||||
|
||||
```
|
||||
WHEN <trigger> THE SYSTEM SHALL <observable response>
|
||||
WHILE <state> THE SYSTEM SHALL <observable response>
|
||||
IF <unwanted condition> THEN THE SYSTEM SHALL <observable response>
|
||||
```
|
||||
|
||||
There is a fourth, unconditional form. Use it for invariants — the properties that must
|
||||
hold with no trigger at all, which are usually the ones that matter most:
|
||||
|
||||
```
|
||||
THE SYSTEM SHALL <observable response>
|
||||
```
|
||||
|
||||
Worked, deliberately from a domain unlike whatever you are specifying:
|
||||
|
||||
```
|
||||
THE SYSTEM SHALL reject a firmware image whose signature does not verify
|
||||
WHEN a sensor reports a temperature above the shutdown limit
|
||||
THE SYSTEM SHALL open the contactor within 200ms
|
||||
WHILE the contactor is open
|
||||
THE SYSTEM SHALL refuse every start command
|
||||
IF a sensor reading is missing or stale by more than one interval
|
||||
THEN THE SYSTEM SHALL treat it as above the limit
|
||||
```
|
||||
|
||||
The example is off-domain on purpose. An on-domain example gets copied: a spec written
|
||||
against an invoice-flavoured example inherited a requirement about extraction confidence
|
||||
that its feature request never mentioned, and the author only noticed on review. Read
|
||||
the shapes, not the content.
|
||||
|
||||
Each conditional line names a trigger and one observable response, which usually maps
|
||||
onto a single test. The `WHILE` and unconditional forms are universally quantified —
|
||||
"every start command", "a firmware image" — so they map onto a property or a set of
|
||||
cases, not one assertion. That is still worth writing: it closes the requirements half
|
||||
of the audit trail, which is otherwise issue-body prose.
|
||||
|
||||
**This is an option, not a replacement.** Threshold criteria ("p99 < 200ms under 100
|
||||
RPS") stay as they are — EARS phrasing adds nothing to a number. Use it for conditional
|
||||
behaviour, error handling, and state-dependent rules.
|
||||
|
||||
For the executor-facing handoff, `feature-spec` also carries a **Context snapshot**
|
||||
section — inline the architecture facts the implementer cannot look up.
|
||||
|
||||
## Reframing Vague Requirements
|
||||
|
||||
When you receive a vague requirement, translate it into specific success criteria before writing any spec content:
|
||||
|
||||
@@ -7,7 +7,7 @@ Use this template when dispatching a spec document reviewer subagent.
|
||||
**Dispatch after:** Spec document is written to docs/superpowers/specs/
|
||||
|
||||
```
|
||||
Task tool (general-purpose):
|
||||
Agent tool (`subagent_type: "general-purpose"`):
|
||||
description: "Review spec document"
|
||||
prompt: |
|
||||
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
|
||||
|
||||
```typescript
|
||||
// In Claude Code / AI environment
|
||||
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
|
||||
Issue every dispatch in **one** message — several `Agent` tool uses in a single
|
||||
assistant turn run concurrently; one per turn runs them in series.
|
||||
|
||||
```
|
||||
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
|
||||
|
||||
|
||||
@@ -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."
|
||||
|
||||
**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
|
||||
|
||||
### Step 1: Load and Review Plan
|
||||
1. Read plan file
|
||||
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
|
||||
|
||||
### Step 2: Execute Tasks
|
||||
|
||||
@@ -54,7 +54,7 @@ WHY: Items may be related. Partial understanding = wrong implementation.
|
||||
|
||||
**Example:**
|
||||
```
|
||||
your human partner: "Fix 1-6"
|
||||
the operator: "Fix 1-6"
|
||||
You understand 1,2,3,6. Unclear on 4,5.
|
||||
|
||||
❌ 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
|
||||
|
||||
### From your human partner
|
||||
### From the operator
|
||||
- **Trusted** - implement after understanding
|
||||
- **Still ask** if scope unclear
|
||||
- **No performative agreement**
|
||||
@@ -84,11 +84,11 @@ IF suggestion seems wrong:
|
||||
IF can't easily verify:
|
||||
Say so: "I can't verify this without [X]. Should I [investigate/ask/proceed]?"
|
||||
|
||||
IF conflicts with your human partner's prior decisions:
|
||||
Stop and discuss with your human partner first
|
||||
IF conflicts with the operator's prior decisions:
|
||||
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
|
||||
|
||||
@@ -100,7 +100,7 @@ IF reviewer suggests "implementing 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
|
||||
|
||||
@@ -123,13 +123,13 @@ Push back when:
|
||||
- Violates YAGNI (unused feature)
|
||||
- Technically incorrect for this stack
|
||||
- Legacy/compatibility reasons exist
|
||||
- Conflicts with your human partner's architectural decisions
|
||||
- Conflicts with the operator's architectural decisions
|
||||
|
||||
**How to push back:**
|
||||
- Use technical reasoning, not defensiveness
|
||||
- Ask specific questions
|
||||
- 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"
|
||||
|
||||
@@ -200,7 +200,7 @@ Reviewer: "Implement proper metrics tracking with database, date filters, CSV ex
|
||||
|
||||
**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.
|
||||
✅ "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:**
|
||||
|
||||
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:**
|
||||
- `{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.
|
||||
|
||||
```
|
||||
Task tool (general-purpose):
|
||||
Agent tool (`subagent_type: "general-purpose"`):
|
||||
description: "Review code changes"
|
||||
prompt: |
|
||||
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
|
||||
|
||||
**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
|
||||
|
||||
|
||||
@@ -7,7 +7,7 @@ Use this template when dispatching a code quality reviewer subagent.
|
||||
**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
|
||||
|
||||
DESCRIPTION: [task summary, from implementer's report]
|
||||
|
||||
@@ -3,7 +3,7 @@
|
||||
Use this template when dispatching an implementer subagent.
|
||||
|
||||
```
|
||||
Task tool (general-purpose):
|
||||
Agent tool (`subagent_type: "general-purpose"`):
|
||||
description: "Implement Task N: [task name]"
|
||||
prompt: |
|
||||
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)
|
||||
|
||||
```
|
||||
Task tool (general-purpose):
|
||||
Agent tool (`subagent_type: "general-purpose"`):
|
||||
description: "Review spec compliance for Task N"
|
||||
prompt: |
|
||||
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 |
|
||||
|-------------------|----------------------|
|
||||
| `Task 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 |
|
||||
| `Task 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 |
|
||||
| `Task tool (general-purpose)` with inline prompt | `@generalist` with your inline prompt |
|
||||
| `Agent tool (superpowers:implementer)` | `@generalist` with the filled `implementer-prompt.md` template |
|
||||
| `Agent tool (superpowers:spec-reviewer)` | `@generalist` with the filled `spec-reviewer-prompt.md` template |
|
||||
| `Agent tool (superpowers:code-reviewer)` | `@code-reviewer` (bundled agent) or `@generalist` with the filled review prompt |
|
||||
| `Agent tool (superpowers:code-quality-reviewer)` | `@generalist` with the filled `code-quality-reviewer-prompt.md` template |
|
||||
| `Agent tool (general-purpose)` with inline prompt | `@generalist` with your inline prompt |
|
||||
|
||||
### Prompt filling
|
||||
|
||||
|
||||
@@ -111,7 +111,7 @@ Skip any step = lying, not verifying
|
||||
## Why This Matters
|
||||
|
||||
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
|
||||
- Missing requirements shipped - incomplete features
|
||||
- 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.
|
||||
|
||||
```
|
||||
Task tool (general-purpose):
|
||||
Agent tool (`subagent_type: "general-purpose"`):
|
||||
description: "Review plan document"
|
||||
prompt: |
|
||||
You are a plan document reviewer. Verify this plan is complete and ready for implementation.
|
||||
|
||||
@@ -246,7 +246,7 @@ Always use subagents (50-100x context savings). REQUIRED: Use [other-skill-name]
|
||||
**Compress examples:**
|
||||
```markdown
|
||||
# ❌ 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.
|
||||
[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.
|
||||
|
||||
**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
|
||||
./render-graphs.js ../some-skill # Each diagram separately
|
||||
./render-graphs.js ../some-skill --combine # All diagrams in one SVG
|
||||
|
||||
+37
-6
@@ -100,13 +100,21 @@ go test -run TestRetryOperation_RetriesThreeTimes ./...
|
||||
```
|
||||
|
||||
Confirm:
|
||||
- Test fails (not compilation errors)
|
||||
- Failure message is expected
|
||||
- Fails because feature is missing (not typos)
|
||||
- Test fails, and fails because the feature is missing
|
||||
- The failure message is the one you expected
|
||||
- It is not failing on a typo in the test itself
|
||||
|
||||
**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
|
||||
|
||||
@@ -166,6 +174,23 @@ Keep tests green. Don't add 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
|
||||
|
||||
### Table-Driven Tests (Preferred Pattern)
|
||||
@@ -329,8 +354,14 @@ Before marking work complete:
|
||||
- [ ] Watched each test fail before implementing
|
||||
- [ ] Each test failed for expected reason (feature missing, not typo)
|
||||
- [ ] Wrote minimal code to pass each test
|
||||
- [ ] All tests pass: `go test ./...`
|
||||
- [ ] Race detector clean: `go test -race ./...`
|
||||
- [ ] All tests pass
|
||||
- [ ] 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)
|
||||
- [ ] Edge cases and errors covered
|
||||
|
||||
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user