Files
astrolabe/.claude/skills/eng-council/SKILL.md
T

126 lines
8.8 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
---
name: eng-council
description: Convene the engineering council — an evidence-grounded review of codebase structure, consistency, layering altitude, and subtraction (what should be deleted). Modes - whole-codebase sweep (milestone boundary or backfill), refactor review, new-functionality review, and a pre-build consult. Auto-invoke only the consult mode - before introducing a new module/store/modal/service/hook shape, ask the council for the canonical shape and what already exists to reuse. All other modes run on demand.
disable-model-invocation: false
---
# Engineering Council
The structural counterpart to `/council`. The design council guards what the user sees;
the engineering council guards the shape of the codebase — structure, consistency,
altitude, and above all **net growth**. `/alignment` reviews a diff in isolation and can't
see cross-cutting drift: parallel patterns forming, near-duplicate helpers, modules
outgrowing their responsibility. This skill exists to see that whole picture.
**Seats are evidence lenses, not personas.** The design council works because every seat
quotes a real corpus. There is no external canon for software structure, so a seat here is
defined by _what it measures and lines up_ — different seats run different queries over
the codebase. A seat that produces an opinion without evidence (file:line, tool output, or
a side-by-side of instances) has not spoken.
## The deletion rules (output format, non-negotiable)
LLMs accrete code; aspirational "prefer removal" instructions wash out. So the bias is
encoded in the report format instead:
1. **Every finding states its estimated net LOC delta** (e.g. `120`, `+15`). The report
ends with the total if all findings were accepted.
2. **"Remove" is a mandatory report section.** It may be empty, but emptiness must be
argued ("knip clean, no single-implementation abstractions found"), not skipped.
3. **Any proposal that adds an abstraction must name ≥ 2 existing call sites** that would
use it _today_. No speculative generality.
4. **A refactor that adds a new way to do something already done elsewhere is a finding,
not progress** — even if the new way is better, until the old instances are migrated
and the old way deleted.
## The seats
Run **sequentially, in this order, in this session** — no subagent fan-out. Later seats
consume earlier findings (Subtraction needs Consistency's verdict on which variant is
canonical before deciding which duplicate dies).
| Seat | Question | Evidence it gathers |
| --------------- | --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **Structure** | Is the dependency picture still the documented one? | `npx madge --circular --extensions ts,tsx src` (cycles); `npx madge --json src` for fan-in/fan-out outliers; grep for layer leaks (`docs/architecture/00-overview.md` defines the layers — core imports nothing from app; only infrastructure touches browser APIs). Oversized modules: files ≫ their peers' size doing > 1 job. |
| **Consistency** | How many shapes does each _kind_ of thing have? | Line up all instances of a kind (modals, stores, services, hooks, confirm flows, persistence subscribers) side by side. Count distinct shapes; name the canonical one; list divergers. Divergence is only visible in the line-up — never judge an instance alone. |
| **Altitude** | Is logic at the layer where it's cheapest to test? | Testable logic stuck in components that belongs in `src/core/` or stores (the testing philosophy in `AGENTS.md` is the canon: core hardest, components lightest). Spec operations outside core. Duplicated derivations that should be a store selector. |
| **Subtraction** | What can be deleted? | `npx knip` (dead exports, unused files/deps); `npx jscpd src --min-tokens 50` (duplication); grep for abstractions with a single implementation, re-implementations of an existing utility, props/options/branches no caller exercises, spec'd-then-abandoned remnants. Consumes all prior seats' findings. |
Tool notes: `madge`, `knip`, `jscpd` are **not** project dependencies — run via `npx`,
treat output as evidence, not verdict (knip false-positives on entry points and dynamic
imports; verify a symbol is truly dead before deleting). If a tool fails or is
unavailable, the seat still sits — grep is the fallback evidence.
## Modes
Pick the mode from what's in front of you; say which mode is running.
The two review modes are also executed by a **clean-context subagent** at session
wrap-up, after `/doc-update` and the alignment pass (see CLAUDE.md → Session wrap-up
protocol). When running as that subagent: you deliberately have no session context —
judge the diff against the codebase's own population of shapes, and return the report as
your final message so the session agent can relay and arbitrate it. A deliberate-looking
divergence whose rationale is recorded nowhere is itself a finding.
### Sweep — whole codebase
For: milestone boundaries, the one-off backfill, "how healthy are we?" on demand.
**Never on a schedule.** Scope: all of `src/` (+ `styles/` for the Structure seat). All
four seats. Output: the full report (format below) **plus a metrics snapshot row**
appended to `docs/codebase-metrics.md` (create on first sweep):
```
| date | src files | src LOC | core LOC | app LOC | deps | knip dead exports | jscpd dup % |
```
The trend line is the point — accretion becomes visible instead of felt.
### Refactor review
For: a landed or in-progress refactor (`git diff` / `git diff --staged`). Scope: changed
files **plus their one-hop import neighborhood** (importers and imports) — drift is only
visible relative to neighbors. Seats: Consistency and Subtraction always; Structure if
imports moved. Center question: did this refactor _reduce_ the number of shapes, or add
variant N+1? (Deletion rule 4 applies with full force.)
### New-functionality review
For: a new feature in the diff. Scope: changed files + one-hop neighborhood. Seats:
Consistency (does it follow the canonical shape for its kind?), Altitude (is the logic in
core/stores, not the component?), Subtraction (does it re-implement an existing utility?
does every prop/option/branch have a caller?), Structure if a new module or dependency
appeared (a new dependency needs the same justification `/alignment` demands).
### Consult — before writing code
The cheapest and most preventive mode; the only one that auto-fires. Before building a
new instance of a kind (a modal, store, service, hook, persistence path): line up the
existing instances, name the canonical shape, list what already exists to reuse, and flag
what the new work might make deletable. Two paragraphs, no report scaffolding.
## Report format
For sweep and review modes:
1. **Verdict** — two or three sentences: overall shape, the one thing to act on first.
2. **Findings** — per seat, each finding: `file:line`, the evidence, the proposed fix,
net LOC delta. Skip seats with nothing to say in one line, not zero.
3. **Remove** — the mandatory section (deletion rule 2).
4. **Net delta** — total LOC delta if all findings were accepted.
5. **Rules discovered** — recurring patterns worth making law (see Close the loop).
**Fix vs. propose:** mechanical, behavior-preserving removals (dead exports, unused files,
unreferenced props) — fix directly, keep `npm run typecheck` + `npm test` green.
Structural findings (merge two shapes, move logic across layers, kill an abstraction) —
propose with the evidence; they're the user's call. Out-of-scope observations get a
`// TODO:` breadcrumb at the code site, same rule as `/alignment`.
## Close the loop
A council that re-finds the same drift every sweep has failed. When a finding reveals a
_recurring_ rule (not a one-off): capture it into the relevant `docs/architecture/` doc
via `/doc-update`, and if it's checkable in a diff, **add it as a numbered check to
`/alignment`** so pointwise review enforces it from then on. The council discovers
systemic rules; `/alignment` keeps them honest. Sweeps should get quieter over time —
that, plus the metrics trend, is how you know it's working.