diff --git a/CHANGELOG.md b/CHANGELOG.md index 014d683..84e7f79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,21 @@ All notable changes to the skill-set plugin will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.12.0] - 2026-05-04 + +### Added + +- **grilling-plans**: New skill for adversarial pre-implementation interrogation of an existing plan or design — walks the decision tree one question at a time with recommended answers, prefers codebase exploration over questions, and surfaces contradictions between stated intent and actual code. Inspired by [mattpocock/skills](https://github.com/mattpocock/skills) `grill-me` / `grill-with-docs`. +- **building-shared-vocabulary**: New skill that maintains a project's domain glossary in `CONTEXT.md` and architecture decisions in `docs/adr/` as living artifacts — files are created lazily, updated inline as terms resolve and decisions crystallize. ADR creation is gated on three criteria (hard-to-reverse, surprising-without-context, real trade-off). Inspired by [mattpocock/skills](https://github.com/mattpocock/skills) `grill-with-docs` (CONTEXT.md / ADR pattern). +- **zooming-out-on-code**: New skill that draws a higher-level system map of unfamiliar code in the project's domain vocabulary — describes responsibility, callers, dependencies, and sibling modules without diving into implementation. Inspired by [mattpocock/skills](https://github.com/mattpocock/skills) `zoom-out`. +- **improving-architecture**: New skill that surfaces deep-module refactor candidates across a codebase using Ousterhout's depth/seam framing — applies the deletion test, presents candidates with locality and leverage justifications, and hands off to `grilling-plans` for the chosen candidate's design. Inspired by [mattpocock/skills](https://github.com/mattpocock/skills) `improve-codebase-architecture`. +- **/skill-set:plan:grill**: Slash command for `grilling-plans`. +- **/skill-set:code:zoom-out**: Slash command for `zooming-out-on-code`. + +### Improved + +- **developing-test-first**: Added "Anti-Pattern: Horizontal Slicing" section after the Iron Law to forbid the bulk RED→RED→RED→...→GREEN→GREEN→GREEN pattern that produces tests of imagined rather than actual behavior. Inspired by [mattpocock/skills](https://github.com/mattpocock/skills) `tdd`. + ## [1.11.1] - 2026-05-04 ### Fixed diff --git a/README.md b/README.md index a722437..3d23c87 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,8 @@ Skills are automatically available after installing the plugin. Claude will use /skill-set:ralph:execute /skill-set:pr-review:fix /skill-set:consulting:review +/skill-set:plan:grill +/skill-set:code:zoom-out ``` ## Project Structure @@ -164,6 +166,16 @@ Contributions welcome! Please see [AGENTS.md](AGENTS.md) for development guideli MIT +## Acknowledgements + +The following skills were inspired by [mattpocock/skills](https://github.com/mattpocock/skills) (MIT license): + +- `grilling-plans` ← `grill-me` / `grill-with-docs` +- `building-shared-vocabulary` ← `grill-with-docs` (CONTEXT.md / ADR pattern) +- `zooming-out-on-code` ← `zoom-out` +- `improving-architecture` ← `improve-codebase-architecture` +- `developing-test-first` (Horizontal Slicing section) ← `tdd` + ## Changelog See [CHANGELOG.md](CHANGELOG.md) for version history and migration guides. diff --git a/plugins/skill-set/.claude-plugin/plugin.json b/plugins/skill-set/.claude-plugin/plugin.json index 2a67b45..897f1f4 100644 --- a/plugins/skill-set/.claude-plugin/plugin.json +++ b/plugins/skill-set/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "skill-set", "description": "Comprehensive productivity skills and development tools for Claude Code, including git workflow automation, code context understanding, peer LLM consulting, and PR review feedback processing.", - "version": "1.11.1", + "version": "1.12.0", "author": { "name": "ether-moon" }, diff --git a/plugins/skill-set/commands/code/zoom-out.md b/plugins/skill-set/commands/code/zoom-out.md new file mode 100644 index 0000000..64eb203 --- /dev/null +++ b/plugins/skill-set/commands/code/zoom-out.md @@ -0,0 +1,12 @@ +--- +description: Draw a higher-level system map of unfamiliar code in the project's domain vocabulary. Describes the module's responsibility, callers, dependencies, and sibling modules without diving into implementation details. +--- + +Invoke the `zooming-out-on-code` skill to get oriented in an unfamiliar area of the codebase. The skill goes up one level of abstraction from the file or function in question, reads the project's `CONTEXT.md` and relevant ADRs first, and produces a four-part map: responsibility, callers, dependencies, siblings — all in domain vocabulary, not implementation language. + +**When to use this command:** +- Before changing code in an area you don't know +- When a teammate hands off a system you have no prior context on +- When you need orientation before diving deeper + +**Output is intentionally compact.** Deeper detail is a follow-up question, not part of this command's output. diff --git a/plugins/skill-set/commands/plan/grill.md b/plugins/skill-set/commands/plan/grill.md new file mode 100644 index 0000000..16f1c26 --- /dev/null +++ b/plugins/skill-set/commands/plan/grill.md @@ -0,0 +1,15 @@ +--- +description: Adversarially interrogate an existing plan or design before implementation. Walks the decision tree one question at a time with recommended answers, prefers codebase exploration over questions, and surfaces contradictions between stated intent and actual code. +--- + +Invoke the `grilling-plans` skill to stress-test the current plan, design, or proposal. The skill walks the decision tree one branch at a time, provides a recommended answer with each question, reads the codebase rather than asking when possible, and surfaces contradictions between user statements and actual code. + +**When to use this command:** +- Between `superpowers:brainstorming` (creation) and `superpowers:writing-plans` (lock-down) +- Before invoking `superpowers:executing-plans` +- Right before a PR description is finalized +- After picking a candidate from `improving-architecture` + +**Outputs:** +- A shared understanding sharp enough to feed into implementation planning +- Optionally hands off to `building-shared-vocabulary` if a domain term is sharpened or an ADR-worthy decision crystallizes diff --git a/plugins/skill-set/skills/building-shared-vocabulary/SKILL.md b/plugins/skill-set/skills/building-shared-vocabulary/SKILL.md new file mode 100644 index 0000000..c97e9b8 --- /dev/null +++ b/plugins/skill-set/skills/building-shared-vocabulary/SKILL.md @@ -0,0 +1,118 @@ +--- +name: building-shared-vocabulary +description: Maintains a project's domain glossary in CONTEXT.md and architecture decisions in docs/adr/ as living artifacts — files are created lazily, updated inline as terms resolve and decisions crystallize, and conflicts with existing entries are surfaced immediately. Use this skill whenever a domain term is being pinned down, a decision sounds ADR-worthy, or vocabulary is about to drift — phrases like "domain glossary", "context.md", "we should write this down", "let's name this", "what do we call this", "is this worth an ADR", "pin down this term" — and as a side-effect of grilling-plans when terms or decisions surface. +--- + +# Building Shared Vocabulary + +## Overview + +A project's **domain glossary** (`CONTEXT.md`) and **architecture decision records** (`docs/adr/`) are not documentation written after the fact — they are operational artifacts that the agent reads to stay aligned with the project, and updates inline as the conversation produces new shared meaning. + +**Core principle:** Vocabulary is built up one resolved term at a time, in the moment the resolution happens. Batched glossary writes go stale. + +> `CONTEXT.md` is a **domain artifact**, not an agent directive. `guarding-agent-directives` does not apply — that skill protects `CLAUDE.md` / `AGENTS.md` and the documents they reference. This skill owns `CONTEXT.md` and `docs/adr/`. + +## When to Use + +- A domain term is being pinned down during conversation (especially during grilling) +- A term in the user's plan conflicts with how the codebase uses it +- A decision that is **hard-to-reverse**, **surprising-without-context**, AND **the result of a real trade-off** has been made — record an ADR +- User explicitly asks to "build a glossary," "set up CONTEXT.md," "add an ADR" + +## Do NOT use for + +- Implementation-level documentation → README, code comments +- Process or workflow documentation → AGENTS.md / CLAUDE.md (and check `guarding-agent-directives` first) +- Library / framework reference → `understanding-code-context` +- Generating PRDs or issues from conversation → out of scope here +- Decisions that are easily reversed, obvious in hindsight, or had no real alternative → no ADR + +## Scope + +This skill supports a **single root `CONTEXT.md`** and a **single `docs/adr/` directory**. Multi-context monorepos (per-bounded-context glossaries, `CONTEXT-MAP.md` indexes) are out of scope; if a project genuinely needs them, that's a future enhancement. + +## Process + +### Lazy file creation + +Create files only on first real demand. An empty `CONTEXT.md` signals "this project has no shared vocabulary," not "no vocabulary surfaced yet" — speculative skeletons mislead future readers and the agent itself. + +| Trigger | Create | +|---|---| +| First domain term gets resolved | `CONTEXT.md` at repo root | +| First decision meets the ADR bar | `docs/adr/0001-.md` | + +If the file already exists, append to it; never overwrite or reorder existing entries silently. + +### Updating `CONTEXT.md` inline + +When a term is resolved during conversation, update `CONTEXT.md` **right away** — not at the end of the session. Use the format in `reference/context-format.md`. + +**Rules:** +- Only domain-meaningful terms. No implementation jargon, no tool names, no helper-function names. +- One canonical name per concept. If the project has been calling the same concept by two names, pick one and note the deprecated alternative under "Avoid:". +- Capture relationships between terms (e.g., "An Order has many Line Items"). +- Flag genuine ambiguity explicitly under "Flagged ambiguities". + +### Surface conflicts immediately + +When the user uses a term that conflicts with an existing `CONTEXT.md` entry, stop and surface it before continuing: + +> "`CONTEXT.md` defines **Cancellation** as the user-initiated revocation. You're using it for the system-initiated timeout. Do we update the definition, or do we need a new term?" + +Do not silently let the conflict pass. The conflict is the reason vocabulary exists. + +### Recording ADRs + +Offer an ADR **only when all three are true**: + +1. **Hard-to-reverse** — undoing the decision later costs real engineering effort +2. **Surprising-without-context** — a future reader will wonder why this choice was made +3. **The result of a real trade-off** — there were genuine alternatives, and one was picked for specific reasons + +If even one is missing, do not offer an ADR. "We picked Postgres because we know Postgres" is not an ADR. + +Use the format in `reference/adr-format.md`. Number sequentially (`0001-`, `0002-`, …). Filename slug should be a noun phrase (`0007-event-sourced-orders.md`). + +## Process Flow + +``` +term resolved during conversation + → conflict with existing CONTEXT.md entry? yes → surface, resolve + → update CONTEXT.md inline + +decision made during conversation + → hard-to-reverse? no → skip + → surprising-no-context? no → skip + → real trade-off? no → skip + → all yes → offer ADR; on accept, write docs/adr/NNNN-.md +``` + +## Reading `CONTEXT.md` and ADRs + +When this skill is invoked or when the agent enters a project for code work, read these files first if present: + +1. `CONTEXT.md` at repo root (if exists) +2. `docs/adr/*.md` in the area being touched (don't over-read; pull only ADRs whose titles match the area) + +Use the vocabulary from `CONTEXT.md` in: +- File and module names suggested in plans +- Variable and function names in implementations +- PR titles, commit messages, issue descriptions +- Conversation with the user + +## Reference + +- `reference/context-format.md` — exact format for a `CONTEXT.md` entry, with examples +- `reference/adr-format.md` — exact ADR template and the three-criterion check + +## Troubleshooting + +| Symptom | Cause | Fix | +|---|---|---| +| `CONTEXT.md` is bloated with implementation terms | Adding non-domain terms | Remove anything that wouldn't appear in a conversation with a non-engineer domain expert | +| Many ADRs over a short period | Bar set too low | Re-check each against the three criteria; demote ADRs that fail any | +| Same concept has multiple ADRs | No conflict surfacing | When updating, search ADR titles first; if related, supersede the older one explicitly | +| `CONTEXT.md` and code disagree | Code drifted, glossary didn't | Surface as a contradiction (this is exactly the signal `grilling-plans` Rule 4 catches) | +| Multi-context monorepo wants per-domain glossaries | Out of scope for this skill | Use a single root `CONTEXT.md` with section headers per bounded context as a stopgap; raise as an enhancement | diff --git a/plugins/skill-set/skills/building-shared-vocabulary/reference/adr-format.md b/plugins/skill-set/skills/building-shared-vocabulary/reference/adr-format.md new file mode 100644 index 0000000..0a69290 --- /dev/null +++ b/plugins/skill-set/skills/building-shared-vocabulary/reference/adr-format.md @@ -0,0 +1,96 @@ +# ADR Format + +Exact template for an Architecture Decision Record, plus the three-criterion check that gates whether to write one at all. + +## File location and naming + +Single directory at `/docs/adr/`. Filenames are sequential and slug-suffixed: + +``` +docs/adr/0001-event-sourced-orders.md +docs/adr/0002-postgres-for-write-model.md +docs/adr/0003-cancellation-as-line-item-operation.md +``` + +The slug is a noun phrase describing what the decision is about. Avoid verbs ("use-postgres") in favor of subjects ("postgres-for-write-model"). + +## The three-criterion gate + +**Write an ADR only if all three are true.** + +### 1. Hard-to-reverse + +Undoing this decision later costs real engineering effort. + +| Hard-to-reverse | Easy-to-reverse | +|---|---| +| Database choice | Logger choice | +| Public API shape | Internal helper signature | +| Domain model decomposition | File organization | +| Wire protocol | Local variable naming | +| Library that touches every module | Library used in one place | + +### 2. Surprising-without-context + +A future reader, looking only at the code, would wonder *why* this choice was made. + +If the code makes the reasoning obvious, no ADR is needed. ADRs exist for choices whose rationale is not visible at the call site. + +### 3. The result of a real trade-off + +There were genuine alternatives. They were considered. One was picked for specific reasons. + +"We picked X because it's the standard" is not a real trade-off. "We picked X over Y because Y's eventual consistency would break our cancellation invariant" is. + +## Template + +The minimal template: + +```markdown +# + +<1-3 sentences: what's the context, what we decided, and why.> +``` + +That's it. **An ADR can be a single paragraph.** The value is in recording *that* a decision was made and *why* — not in filling out sections. Resist the urge to bulk it up. + +## Optional sections + +Only include these when they add genuine value. Most ADRs do not need them. + +- **Status** frontmatter (`proposed | accepted | deprecated | superseded by ADR-NNNN`) — useful when decisions are revisited +- **Considered Options** — only when the rejected alternatives are worth remembering +- **Consequences** — only when non-obvious downstream effects need to be called out + +If you reach for a section, ask: would removing this section lose information a future reader would need? If not, drop it. + +## Updates + +ADRs are immutable in principle. To change a decision: + +1. Write a new ADR explaining the new decision and what changed +2. Add Status to the old ADR: `Superseded by ADR-NNNN` +3. Cross-link both directions + +Do not delete superseded ADRs. The history is the value. + +## What qualifies for an ADR + +- **Architectural shape.** "We're using a monorepo." "The write model is event-sourced; the read model is projected into Postgres." +- **Integration patterns between contexts / services.** "Ordering and Billing communicate via domain events, not synchronous HTTP." +- **Technology choices that carry lock-in.** Database, message bus, auth provider, deployment target. Not every library — just the ones that would take a quarter to swap out. +- **Boundary and scope decisions.** "Customer data is owned by the Customer context; other contexts reference it by ID only." The explicit no-s are as valuable as the yes-s. +- **Deliberate deviations from the obvious path.** "We're using manual SQL instead of an ORM because X." Anything where a reasonable reader would assume the opposite — these stop the next engineer from "fixing" something deliberate. +- **Constraints not visible in the code.** "We can't use AWS because of compliance requirements." "Response times must be under 200ms because of the partner API contract." +- **Rejected alternatives when the rejection is non-obvious.** If you considered GraphQL and picked REST for subtle reasons, record it — otherwise someone will suggest GraphQL again in six months. + +## What does NOT qualify + +- Renaming a function or file +- Picking lint / formatter rules +- Choosing a logger library (unless logging is a domain concern) +- Migrating from yarn to pnpm, or similar tool swaps +- Adding a new test framework +- Picking commit message style + +If the decision is reversible by a single PR with no migration cost, it is not ADR-worthy. diff --git a/plugins/skill-set/skills/building-shared-vocabulary/reference/context-format.md b/plugins/skill-set/skills/building-shared-vocabulary/reference/context-format.md new file mode 100644 index 0000000..11f10cc --- /dev/null +++ b/plugins/skill-set/skills/building-shared-vocabulary/reference/context-format.md @@ -0,0 +1,111 @@ +# CONTEXT.md Format + +Exact format for a project's domain glossary, with examples. + +## File location + +A single file at the repository root: `/CONTEXT.md`. + +## Top-level structure + +```markdown +# — Domain Context + +Brief one-paragraph orientation. What domain does this project serve? +What is the primary user / actor? What is the value proposition? + +## Language + +[Term entries — see "Term entry format" below] + +## Relationships + +- An **** has many **** +- A **** belongs to one **** +- [...] + +## Example dialogue + +> **Dev:** "When a **Customer** places an **Order**, do we create the **Invoice** immediately?" +> **Domain expert:** "No — an **Invoice** is only generated once a **Fulfillment** is confirmed." + +## Flagged ambiguities + +- "" — what's still unclear, what we ruled out, what's outstanding +``` + +Keep "Language" flat by default — every term sits at the same level. **Group terms under sub-headings only when natural clusters emerge** (e.g., a project with distinct sub-domains may benefit from `### Ordering` / `### Billing` groupings). If all terms belong to a single cohesive area, leave it flat. + +The **Example dialogue** is a short conversation between a developer and a domain expert that demonstrates how the canonical terms interact in practice. It clarifies boundaries between related concepts (e.g., `Order` vs. `Cart`, `Invoice` vs. `Payment`) better than definitions alone. Write it the way the conversation would actually happen — bold the canonical terms, keep it under a half-dozen exchanges. + +## Term entry format + +Each entry is a level-3 heading with the canonical name (bold), an italicized one-line definition, and an optional "_Avoid:_" line listing deprecated alternatives. + +```markdown +****: +The single-sentence definition that a domain expert would recognize. +_Avoid_: , +``` + +Example: + +```markdown +**Order**: +A confirmed customer purchase intent. Carries one or more **Line Items**, has a single **Customer**, and progresses through a fixed lifecycle. +_Avoid_: cart, basket, transaction (those mean different things — see `Cart` and `Payment Transaction`) + +**Line Item**: +A single product line within an **Order** — quantity, unit price at time of order, product reference. Cancellation operates at this level. +_Avoid_: order line, item + +**Cancellation**: +User-initiated revocation of one or more **Line Items**, possibly the whole **Order**. Distinct from system-initiated **Timeout**. +_Avoid_: refund (refund is a money-flow concept, see `Payment Reversal`) +``` + +`_Avoid_` is **optional**. Include it only when there is a real deprecated alternative the team has used or might mistakenly use. A clean, unambiguous term needs no Avoid line: + +```markdown +**Fulfillment**: +The act of preparing and shipping the goods on a confirmed **Order**. A single **Order** maps to one or more **Fulfillments** (split shipments). +``` + +If you find yourself inventing deprecated alternatives just to fill the line, drop it. + +## Rules + +### What belongs + +- Concepts a domain expert would name in conversation +- Terms that appear in user-facing copy, marketing, support +- Concepts that have a lifecycle, identity, or invariants in the system +- Roles, actors, capabilities + +### What does NOT belong + +- Implementation classes (`OrderRepository`, `OrderDTO`) +- Framework concepts (`Component`, `Reducer`, `Middleware`) +- Helper utilities, parsers, validators +- File paths, table names, environment variables +- Anything that would change if the implementation language changed + +### Naming + +- Canonical term in **bold** at first mention in any entry +- PascalCase for concepts that map to entity-like things ("Order", "Line Item") +- Plain lowercase for verbs and lifecycle states ("cancellation", "fulfilled") unless the domain itself capitalizes them + +### Conflicts + +When a new term clashes with an existing one, update the entry. Mark the resolution under "Flagged ambiguities" with a brief history line: + +```markdown +- "backlog" was previously used to mean both the *tool* hosting issues and the *body of work* inside it — resolved 2026-04: the tool is the **Issue Tracker**; "backlog" is no longer used as a domain term. +``` + +### Updates + +Each update is one entry at a time, in the moment of resolution. Do not batch. + +If you find yourself wanting to "do a vocabulary cleanup pass," that's a sign the conversation isn't surfacing terms naturally — return to grilling instead. diff --git a/plugins/skill-set/skills/developing-test-first/SKILL.md b/plugins/skill-set/skills/developing-test-first/SKILL.md index 59de7f0..8cad331 100644 --- a/plugins/skill-set/skills/developing-test-first/SKILL.md +++ b/plugins/skill-set/skills/developing-test-first/SKILL.md @@ -49,6 +49,33 @@ Write code before the test? Delete it. Start over. Implement fresh from tests. Period. +## Anti-Pattern: Horizontal Slicing + +The Iron Law forbids production code without a failing test, but it does not by itself forbid writing five tests up front and then five implementations. That sequence — **horizontal slicing** — is the most common way TDD silently fails. + +``` +WRONG (horizontal): + RED RED RED RED RED → GREEN GREEN GREEN GREEN GREEN + +RIGHT (vertical / tracer bullet): + RED → GREEN, RED → GREEN, RED → GREEN, ... +``` + +Why horizontal slicing produces bad tests: + +- **Tests written in bulk verify *imagined* behavior, not actual behavior.** The implementation hasn't been built yet, so the tests can only assert against guesses. +- **They test the *shape* of things** — data structures, function signatures, return types — instead of user-facing behavior. +- **They become insensitive to real changes.** They pass when the system breaks in ways the bulk-author didn't anticipate; they fail when behavior is fine but a signature shifted. +- **You out-run your headlights.** Committing to test structure before any implementation feedback locks you into assumptions you haven't validated. + +Why vertical slicing works: + +- Each test responds to what the previous cycle taught you +- You write the test you actually need, because you just felt the implementation +- Tests stay honest because they were written against working code, not imagination + +**Rule:** one test, one implementation, one cycle. Then the next. + ## Red-Green-Refactor ```dot diff --git a/plugins/skill-set/skills/grilling-plans/SKILL.md b/plugins/skill-set/skills/grilling-plans/SKILL.md new file mode 100644 index 0000000..fae0b45 --- /dev/null +++ b/plugins/skill-set/skills/grilling-plans/SKILL.md @@ -0,0 +1,163 @@ +--- +name: grilling-plans +description: Adversarially interrogates an existing plan, design, or proposal before implementation — walks the decision tree one question at a time, provides a recommended answer with each question, prefers codebase exploration over questions, and surfaces contradictions between stated intent and actual code. Use this skill whenever a plan, design doc, RFC, ADR draft, ticket spec, or implementation outline is shared and the user wants review, sanity check, sign-off, or asks "is this ready" — even without the word "grill". Trigger phrases include "grill me", "challenge this plan", "poke holes", "stress test". Also use before locking down a spec for implementation. +--- + +# Grilling Plans + +## Overview + +Adversarial validation mode. Take an existing plan, design, or proposal and **walk the decision tree one branch at a time**, surfacing hidden assumptions, fuzzy terminology, and contradictions before any code is written. + +**Core principle:** Misalignment is the #1 failure mode. The cure is not more brainstorming — it is forcing every implicit decision to become explicit. + +This is a discipline skill. Each rule below corresponds to a specific way grilling silently fails when the rule is dropped — keeping all six is what separates grilling from casual conversation. + +## When to Use + +**Use grilling when a plan already exists and is about to be locked:** +- Between `superpowers:brainstorming` (creation) and `superpowers:writing-plans` (lock-down) +- Before invoking `superpowers:executing-plans` +- Right before a PR description is finalized +- User says "challenge this", "poke holes", "stress test", "grill me" +- When a candidate is selected from `improving-architecture` + +**Do NOT use for:** +- Blank-slate ideas with no plan yet → `superpowers:brainstorming` +- Locked specs that just need decomposition → `superpowers:writing-plans` +- Bug investigation → `superpowers:systematic-debugging` +- Reviewing already-written code → `simplify` +- External library API questions → `understanding-code-context` + +Thinking "the plan looks good enough, skip grilling"? That is exactly when grilling matters most. + +## Domain Awareness + +Before the first question, absorb whatever domain context already exists: + +- If `CONTEXT.md` and `docs/adr/` exist (per `building-shared-vocabulary`), read them first — `CONTEXT.md` carries canonical vocabulary, ADRs in the area being grilled record decisions you should not re-litigate. If absent, infer domain vocabulary from package/module names, test descriptions, and recent commit messages. +- Walk relevant code with Grep / Glob / Read or an `Explore` subagent — see `superpowers:dispatching-parallel-agents` if multiple independent areas need pre-grilling sweeps. + +This costs a few minutes and saves a session of mis-aimed questions. It also lets Rule 5 (term sharpening) work against an actual baseline rather than guesses. + +## The Grilling Protocol + +Six rules. Each blocks a different failure mode (decision-tree drift, unanchored questions, lazy code-skipping, silent contradictions, vocabulary slop, abstract hand-waving) — weakening any one collapses grilling back into casual conversation. + +### Rule 1 — Walk the Decision Tree, One Question Per Turn + +Plans contain a tree of dependent decisions. Resolve them in dependency order, **one at a time**. Wait for the user's answer before moving to the next branch. + +Why one at a time: batched questions get answered at the same shallow level. The user spreads attention across all of them, the dependency tree collapses into a flat list, and downstream answers stop being informed by upstream ones. Single questions force depth. + +When a question opens up sub-questions, descend into the sub-tree before backing up. + +### Rule 2 — Provide a Recommended Answer With Every Question + +Every question must include the model's own recommendation, with reasoning. This forces commitment instead of polite hedging. + +``` +WRONG: "What should happen when the cache is empty?" +RIGHT: "What should happen when the cache is empty? My recommendation: + fall through to the source of truth and repopulate, because the + alternative (return empty) creates a thundering herd on miss. + Counter-argument: warmup cost. Which way?" +``` + +If you cannot recommend an answer, the question is not yet sharp enough — refine it before asking. + +### Rule 3 — Codebase Exploration Beats Asking + +If a question can be answered by reading code, **read the code instead of asking**. Use Grep, Glob, or Read mid-walk; reserve `superpowers:dispatching-parallel-agents` for the Domain Awareness pre-grilling sweep, not for questions interleaved with the user. + +``` +WRONG: "Does this project already have a retry helper?" +RIGHT: [run Grep for "retry"] → "Found `src/lib/retry.ts` with exponential + backoff. We should reuse it. Confirm?" +``` + +Only ask the user when the answer requires intent, judgment, or knowledge outside the repository. + +### Rule 4 — Surface Contradictions Immediately + +When the user's stated plan conflicts with what the code actually does, surface it the moment you see it. Do not let the contradiction silently become a future bug. + +``` +"You said partial cancellation is supported, but the code in +src/orders/cancel.ts cancels the entire Order in one transaction. +Which is the source of truth right now — the plan or the code?" +``` + +### Rule 5 — Sharpen Fuzzy Terms + +When a term is overloaded, ambiguous, or context-dependent, propose a canonical name and pin down the meaning before continuing. Vague vocabulary now becomes a wrong implementation later. + +``` +"You're saying 'account' — do you mean the Customer record, the +authenticated User, or the Billing entity? Those are three different +things in this codebase. Pick one canonical term and let's use it." +``` + +If the project has a `CONTEXT.md` glossary, check the term against it. Conflicts with the glossary are themselves a signal. + +### Rule 6 — Stress-Test With Concrete Scenarios + +When domain relationships or boundaries are being discussed, invent a specific scenario that probes the edge. Force precision instead of letting abstractions slide by. + +``` +"You said partial cancellation is supported. Concrete scenario: +Order has three Line Items, customer cancels two. The third item +ships, the first two don't. Does the Order itself get a status? +What status? What does the invoice look like?" +``` + +Edge-case scenarios surface invariants the user hadn't thought about. Pick scenarios that lie at the boundaries — empty, full, partial, simultaneous, racing, retried, reordered. The user's answer either commits to a behavior or reveals that the behavior wasn't decided yet. + +## Process Flow + +``` +receive plan + → Domain Awareness (read CONTEXT.md / ADRs / code) + → loop: + pick next undecided branch (dependency order) + → can codebase answer? yes → read, present finding + no → form question + recommendation, ask + → contradiction with code? yes → surface, then continue + → fuzzy term? yes → sharpen (canonical name), then continue + → tree complete? no → next branch + yes → plan validated + (optional: update CONTEXT.md / ADR) +``` + +## Outputs + +**Default output:** a shared understanding between user and agent. The plan is now sharp enough to feed into `superpowers:writing-plans` or `superpowers:executing-plans`. + +**Optional outputs (only if conditions are met):** + +| Trigger | Action | +|---|---| +| A fuzzy term was sharpened during grilling | Hand off to `building-shared-vocabulary` to update `CONTEXT.md` | +| A decision is **hard-to-reverse** AND **surprising-without-context** AND **the result of a real trade-off** | Offer to record an ADR via `building-shared-vocabulary` | +| The plan reveals an architectural friction worth a separate refactor | Hand off to `improving-architecture` | + +Do not produce these artifacts speculatively. Only when the bar is met. + +**ADR yes-example:** "We project the read model into Postgres while the write model is event-sourced." Hard to reverse (touches every read path), surprising (a future reader will ask why the duplication exists), real trade-off (we chose CQRS over a simpler unified model for specific consistency reasons). → ADR. + +**ADR no-example:** "We picked `pino` for logging because the team knows it." Easy to reverse (one PR), not surprising (default-ish choice), no real alternatives weighed. → No ADR, just a code comment if anything. + +## Reference + +- `reference/decision-tree-walk.md` — how to identify dependency order, when to descend vs. back up, what counts as a leaf +- `reference/codebase-cross-reference.md` — which tools to use for which kinds of contradictions, examples of high-value cross-checks + +## Troubleshooting + +| Symptom | Cause | Fix | +|---|---|---| +| User feels grilled past the useful point | Asking questions whose recommendation is obvious | Stop asking and just commit to the recommendation; surface it as a finding instead | +| User keeps saying "I don't know, you decide" | Decisions are too granular for grilling | Back up one level; the user's threshold is the right granularity | +| Questions feel disconnected from each other | Not walking the dependency tree | Re-read the plan, list undecided branches in dependency order, restart from the top | +| Same fuzzy term keeps recurring | Term not pinned down | Stop and sharpen it now; do not continue with the term still ambiguous | +| Codebase contradicts the plan in many places | Plan written without exploring the code first | Pause grilling; have user re-explore code before continuing, or adjust the plan to match code | diff --git a/plugins/skill-set/skills/grilling-plans/reference/codebase-cross-reference.md b/plugins/skill-set/skills/grilling-plans/reference/codebase-cross-reference.md new file mode 100644 index 0000000..74d229c --- /dev/null +++ b/plugins/skill-set/skills/grilling-plans/reference/codebase-cross-reference.md @@ -0,0 +1,115 @@ +# Codebase Cross-Reference + +How to use the codebase to answer questions, surface contradictions, and stress-test claims — instead of pinging the user. + +## Contents + +- [The Core Principle](#the-core-principle) +- [High-Value Cross-Checks](#high-value-cross-checks) +- [Tool Selection](#tool-selection) +- [When to Stop Reading and Start Asking](#when-to-stop-reading-and-start-asking) +- [Reporting What You Read](#reporting-what-you-read) + +## The Core Principle + +Every question to the user has a cost. Most questions about *what currently exists* have zero cost to answer from the code. The asymmetry says: read first, ask second. + +## High-Value Cross-Checks + +These are the cross-checks that catch the most defects. + +### 1. "X already exists" check + +Before proposing to build a helper, retry, parser, validator, or utility, search for it. + +``` +Tools: Grep (for keywords), Glob (for filename patterns) +Example: User plans a new `withRetry` wrapper. + → Grep "retry" in src/ + → Found `src/lib/withRetry.ts`. Reuse, don't rebuild. +``` + +### 2. Stated behavior vs. actual behavior + +When the user says "X currently does Y," check that the code agrees. + +``` +User: "The /orders endpoint returns 404 if the order is cancelled." +Read src/routes/orders.ts → endpoint returns 200 with cancelled=true. +Surface: "Code returns 200 with cancelled=true, not 404. Is the plan +relying on 404, or is the behavior changing as part of this work?" +``` + +### 3. Invariant claims + +When the user states "this never happens" or "this is always true," look for code that assumes the opposite. + +``` +User: "User always has an email at signup." +Grep for "email == null" or "!email" → found 3 sites that handle null email. +Surface: "Three call sites currently handle null email. Either they're dead +code, or your invariant is not enforced. Which?" +``` + +### 4. Dependency direction claims + +When the user describes a dependency direction ("A calls B, never the reverse"), verify with an import scan. + +``` +User: "billing/ never depends on auth/." +Grep "from.*auth" inside src/billing/ → found imports. +Surface contradiction. +``` + +### 5. Naming consistency + +When a new term is introduced, check if a different name for the same concept already exists. + +``` +User plans to add a "RevocationToken". +Grep "Cancel|Revoke|Invalidate.*Token" → found "InvalidationToken" in +src/auth/tokens.ts. Surface: "Same concept already named InvalidationToken. +Reuse the name, or rename the existing one?" +``` + +## Tool Selection + +| Question type | First tool | +|---|---| +| Does symbol X exist? | Grep with the name | +| Where is X defined? | LSP `goToDefinition` if a path is known, else Grep | +| Who calls X? | LSP `findReferences` if precise, else Grep | +| Is there code matching pattern Y? | Grep with regex | +| What files match a structure? | Glob | +| Wide unfamiliar area | Dispatch an Explore agent (`Agent` tool) | + +Use the LSP tool when symbol-precise (avoids false positives from comments, strings, similar names). Use Grep when the question is broader. + +## When to Stop Reading and Start Asking + +Reading the codebase is not free. Stop and ask the user when: + +- You have made 3-5 unsuccessful searches or 2 read-throughs without finding evidence either way +- The question requires intent (why was it built this way?) not facts (what does it do?) +- The question requires future direction not present state +- Reading would require understanding a system far outside the current scope + +The recommended-answer rule (Rule 2) still applies: if you ask after exploration, lead with what you found and what you'd recommend on that basis. + +## Reporting What You Read + +When you read code on the user's behalf, report it concisely: + +``` +WRONG (too verbose): + "I searched the codebase using Grep with the pattern 'retry' and found + several matches in src/lib/retry.ts which is a utility module that + provides exponential backoff functionality with configurable max + attempts and..." + +RIGHT (terse, evidence-first): + "Found `src/lib/withRetry.ts` (exponential backoff, configurable max + attempts). Reuse it. Confirm?" +``` + +Lead with the finding, link the file path, recommend, ask for confirmation. diff --git a/plugins/skill-set/skills/grilling-plans/reference/decision-tree-walk.md b/plugins/skill-set/skills/grilling-plans/reference/decision-tree-walk.md new file mode 100644 index 0000000..4f30577 --- /dev/null +++ b/plugins/skill-set/skills/grilling-plans/reference/decision-tree-walk.md @@ -0,0 +1,73 @@ +# Decision Tree Walk + +How to identify dependency order, when to descend, and what counts as a leaf. + +## Identifying the Tree + +Before asking the first question, read the plan and write down the **undecided branches** — points where the plan is silent, hand-wavy, or admits multiple interpretations. Each branch is a candidate question. + +A useful prompt to surface branches: + +- "What inputs does this assume exist?" +- "What outputs does this commit to?" +- "What error modes are unhandled?" +- "What invariants must hold across boundaries?" +- "What happens at the empty case, the boundary case, the failure case?" +- "What does each domain term in the plan actually mean here?" + +Branches surfaced this way are usually unordered. Order them next. + +## Dependency Order + +Two branches are **dependent** if the answer to one constrains the answer to the other. Resolve the constraining branch first. + +``` +Q: "Which datastore are we using?" ← decide first +Q: "Which transaction isolation?" ← depends on datastore +Q: "How do we test it?" ← depends on both +``` + +If two branches are independent, pick whichever the user can answer faster. Save the high-cognitive-load decisions for when context is rich. + +When you cannot tell whether two branches are dependent, ask the user — but propose your guess with reasoning (Rule 2 still applies). + +## When to Descend vs. Back Up + +After a question is answered, an answer often opens a **sub-tree**: + +``` +Q1: "Which datastore?" → "Postgres" + └─ sub-Q: "Which Postgres version?" + └─ sub-Q: "Connection pooling strategy?" + └─ sub-Q: "Migration tool?" +``` + +**Descend** when the sub-questions are tightly coupled to the answer (changing the parent invalidates them). + +**Back up** when the sub-questions are independent of the answer and would stall progress. + +A useful test: if the user says "let's come back to that," the question belongs higher in the tree, not deeper. Note it on a stack and move on. + +## What Counts as a Leaf + +A branch is a **leaf** (no further questions) when: + +- The answer is captured precisely (specific value, specific term, specific behavior) +- No reasonable reader of the plan would interpret the answer differently +- Implementation can begin from the answer alone + +Watch for **false leaves**: + +- "We'll figure that out in implementation" — not a leaf, just deferred +- "Standard approach" — not a leaf, the standard is the question +- "It depends" — not a leaf, the dependency is the question + +## When to Stop + +Grilling ends when **every branch is a leaf**. Not when the user is tired. Not when 30 minutes have elapsed. Not when the plan "feels good." + +If the user wants to stop early, surface what is still un-leafed: + +> "Three branches remain undecided: cache eviction policy, retry budget, and the meaning of 'partial success' in the response shape. Stopping now means implementation will guess at these. Continue, or accept the guesses?" + +This makes the cost of stopping explicit. The user can still choose to stop — but with eyes open. diff --git a/plugins/skill-set/skills/improving-architecture/SKILL.md b/plugins/skill-set/skills/improving-architecture/SKILL.md new file mode 100644 index 0000000..719c2e0 --- /dev/null +++ b/plugins/skill-set/skills/improving-architecture/SKILL.md @@ -0,0 +1,165 @@ +--- +name: improving-architecture +description: Surfaces deep-module refactor candidates across a codebase using domain vocabulary and Ousterhout's depth/seam framing — applies the deletion test, presents candidates with locality and leverage justifications, and hands off to the `grilling-plans` skill for the chosen candidate's design. Use this skill whenever the user mentions architecture, refactoring scope, deep/shallow modules, seams, ports/adapters, modularity, or expresses frustration with tangled code — phrases like "improve architecture", "find refactor opportunities", "deep module", "ball-of-mud area", "this code is a mess", "untangle this", "split this module", "make this testable", "extract a seam", "shallow module" — even without the word "architecture". Not for reviewing recently changed code (use `simplify`) or designing new features (use `superpowers:brainstorming`). +--- + +# Improving Architecture + +## Overview + +Surface architectural friction in a codebase and propose **deepening opportunities** — refactors that turn shallow modules into deep ones. The aim is testability and AI-navigability, not aesthetic cleanup. + +**Core principle:** A deep module hides a lot of behavior behind a small interface. A shallow module's interface is nearly as complex as its implementation. Find the shallow ones. + +## When to Use + +- The user wants to schedule architectural improvement work +- A bug fix or feature surfaces a tangled area worth improving separately +- Periodic review of an area that has accreted complexity over time +- User says "improve the architecture", "find refactor opportunities", "what should we deepen", "ball of mud" + +## Do NOT use for + +- Reviewing recently changed code → `simplify` +- Single-file cleanup (rename, dedupe, format) → just do it inline +- Bug investigation → `superpowers:systematic-debugging` +- New feature design → `superpowers:brainstorming` +- Test strategy → `driving-with-tests` + +## Glossary + +Consistent vocabulary lets two reviewers compare candidates across reviews; drift into "service" / "component" / "boundary" makes it impossible to tell whether two findings are the same or different. That is why the terms below are canonical and used as-is. + +| Term | Meaning | +|---|---| +| **Module** | Anything with an interface and an implementation — function, class, package, slice | +| **Interface** | Everything a caller must know to use the module: types, invariants, error modes, ordering, config — not just the type signature | +| **Implementation** | The code inside the module | +| **Depth** | Leverage at the interface. **Deep** = a lot of behavior behind a small interface. **Shallow** = interface nearly as complex as the implementation. | +| **Seam** | Where an interface lives — a place behavior can be altered without editing in place | +| **Adapter** | A concrete thing satisfying an interface at a seam | +| **Leverage** | What callers gain from depth | +| **Locality** | What maintainers gain from depth — change, bugs, knowledge concentrated in one place | + +Full elaboration in `reference/deep-modules.md`. The deletion test (the most useful single heuristic) lives in `reference/deletion-test.md`. + +## Process + +### 1. Orient + +If `CONTEXT.md` and `docs/adr/` exist (per `building-shared-vocabulary`), read them first — `CONTEXT.md` gives names to good seams, ADRs record decisions to not re-litigate. If absent, infer domain vocabulary from package/module names, test descriptions, and recent commit messages, and proceed. + +### 2. Explore + +Walk the codebase looking for friction. Don't apply rigid heuristics — observe organically and note where understanding is hard: + +- Where does understanding one concept require bouncing between many small files? +- Where is a module's interface nearly as complex as its implementation? (shallow) +- Where have pure functions been extracted just for testability, while the real bugs hide in *how* they're called? (no locality) +- Where do tightly-coupled modules leak across their seams? +- Which parts are untested or hard to test through their current interface? + +For broader sweeps, dispatch an `Explore` agent (`Agent` tool with `subagent_type=Explore`) to walk a directory or feature area in parallel — see `superpowers:dispatching-parallel-agents`. + +Apply the **deletion test** to anything you suspect is shallow: imagine deleting it. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep. Full procedure: `reference/deletion-test.md`. + +### 3. Present candidates + +Number them. For each: + +```text +**N. ** + +- **Files:** +- **Problem:** +- **Solution:** +- **Locality gain:** +- **Leverage gain:** +- **Test impact:** +``` + +**Do not propose specific interfaces yet.** That belongs to the next phase. + +**Vocabulary discipline:** Use `CONTEXT.md` terms for domain concepts ("the Order intake module") and the glossary above for architecture concepts ("a deep seam over the rate limiter"). Do not invent new architectural vocabulary; use the canonical terms. + +**Filled example:** + +```text +**1. Order Intake validation cluster** + +- **Files:** src/orders/intake/promotion-check.ts, inventory-check.ts, + credit-check.ts, route.ts (calls all three) +- **Problem:** Three shallow validators each export a single function; + every caller has to remember the right ordering and aggregate errors + manually. Two of three are also called by the admin Manual Order tool, + which currently re-aggregates errors with a different shape. +- **Solution:** Collapse the three validators behind a single + `validateOrder(order) → ValidationResult` interface that owns the + ordering, the aggregation, and the error shape. +- **Locality gain:** Adding a new check (e.g., fraud scoring) becomes + one edit inside the validation module; today it is three. +- **Leverage gain:** Both the HTTP route and the admin tool stop + re-implementing aggregation; both consume the same `ValidationResult`. +- **Test impact:** Existing per-validator unit tests can stay as + internal helpers; new tests assert against `validateOrder` outcomes — + closer to user-observable behavior. +``` + +**ADR conflicts:** If a candidate contradicts an existing ADR, only surface it when the friction is real enough to warrant reopening the decision. Mark it: _"Contradicts ADR-0007 — but worth reopening because…"_. Do not list every theoretical refactor an ADR forbids. + +Ask the user: "Which of these would you like to explore?" + +### 4. Classify dependencies + +Before designing a new interface, classify the candidate's dependencies (see `reference/deepening.md`): + +1. **In-process** — pure computation; merge and test directly +2. **Local-substitutable** — has a local stand-in (PGLite, in-memory FS); use it +3. **Remote but owned** — your services across a network; define a port with 2+ adapters +4. **True external** — third-party (Stripe, Twilio); inject port, mock in tests + +The category determines the seam strategy and what tests look like. + +### 5. (Optional) Design It Twice + +If interface shape is non-obvious, run a parallel sub-agent design generation pass — see `reference/interface-design.md`. Spawn 3+ agents with radically different design constraints (minimal interface / maximum flexibility / optimize-for-common-caller / ports-and-adapters), present the results sequentially, then commit to one. + +Skip this step when the right interface is already obvious. Use it when the user is unsure or when the candidate's interface shape would set a long-term direction. + +### 6. Hand off + +Hand off to `grilling-plans` to interrogate the chosen design. Do not jump to writing implementation plans directly — the candidate is still under-specified, and grilling will surface what is unclear. + +After grilling, ADRs and `CONTEXT.md` updates are owned by `building-shared-vocabulary` — this skill does not write them. + +## Process Flow + +```text +orient (read CONTEXT.md and relevant ADRs) + → explore codebase for friction (optionally via Explore subagents) + → apply deletion test to suspect shallow modules + → present numbered candidates with locality / leverage / test impact + → user picks one? no → end (note for later) + yes → classify dependencies + (in-process / local-sub / remote-owned / true-external) + → interface shape obvious? yes → hand off to grilling-plans + no → Design It Twice + → hand off to grilling-plans +``` + +## Reference + +- `reference/deep-modules.md` — full elaboration of the depth / seam / adapter / locality vocabulary, with examples of deep vs. shallow at function, module, and package scales +- `reference/deletion-test.md` — how to actually run the deletion test, what counts as "complexity reappears", common false-negatives +- `reference/deepening.md` — dependency categorization (in-process / local-substitutable / remote-owned / true-external) and the testing strategy each implies +- `reference/interface-design.md` — the "Design It Twice" parallel sub-agent pattern for generating radically different interface candidates before committing + +## Troubleshooting + +| Symptom | Cause | Fix | +|---|---|---| +| Candidate list grows past 5–7 items | Including every theoretical improvement | Cut to the friction you actually felt during exploration; reject the speculative ones. (Past ~7 dilutes user attention; pick the highest-leverage subset.) | +| User can't choose between candidates | Candidates not differentiated by impact | Rank by locality/leverage gain, not by file count or apparent size | +| Output uses generic terms ("service", "boundary", "component") | Vocabulary discipline broke | Re-edit using only the glossary terms above; the precision is the point | +| Suggesting a candidate that contradicts a recent ADR | Did not read ADRs in step 1 | Read the ADR; either drop the candidate or surface it explicitly as an ADR-reopening proposal | +| Candidates require ground-up rewrites | Bar set too high | Look for *one-step* deepenings — refactors a single PR could deliver — not architectural revolutions | diff --git a/plugins/skill-set/skills/improving-architecture/reference/deep-modules.md b/plugins/skill-set/skills/improving-architecture/reference/deep-modules.md new file mode 100644 index 0000000..8d2b1d5 --- /dev/null +++ b/plugins/skill-set/skills/improving-architecture/reference/deep-modules.md @@ -0,0 +1,129 @@ +# Deep Modules + +Full elaboration of the depth / seam / adapter / locality vocabulary, with examples at function, module, and package scales. + +Source: John Ousterhout, *A Philosophy of Software Design*. The vocabulary here adapts that book's framing to be operational for an agent. + +## Contents + +- [Module / Interface / Implementation](#module) +- [Depth — the central concept](#depth) +- [Examples by scale](#examples-by-scale) +- [Seam / Adapter](#seam) +- [Leverage / Locality](#leverage) +- [How these terms compose](#how-these-terms-compose) +- [Common shallow patterns to look for](#common-shallow-patterns-to-look-for) +- [Things that look shallow but are not](#things-that-look-shallow-but-are-not) + +## Module + +Anything with an interface and an implementation. The *scale* varies — a single function is a module, a class is a module, a package is a module, a microservice is a module. Reasoning about depth applies at every scale. + +## Interface + +Everything a caller must know to use the module. This is *not just the type signature* — it includes: + +- Type signatures (input, output, errors) +- Invariants the caller must preserve (preconditions) +- Invariants the module guarantees (postconditions) +- Ordering constraints (must call X before Y) +- Side effects (writes to disk, network, mutates input) +- Error modes (what can fail, how it surfaces, recovery options) +- Configuration (what the caller has to decide) + +Interface = the surface area of cognitive load on every caller. + +## Implementation + +The code inside the module. Everything callers do *not* need to know. + +## Depth + +The ratio between leverage gained and interface complexity carried. + +``` +deep: [ ============= IMPLEMENTATION ============= ] + [ interface ] + +shallow: [ implementation ] + [ interface ] +``` + +A **deep module** does a lot of work behind a small interface. A caller passes minimal information, gets significant behavior in return. + +A **shallow module** is one whose interface is nearly as complex as its implementation. Callers carry almost as much cognitive load as they would have without the module. + +## Examples by scale + +### Function scale + +**Deep:** `parseISODate(s: string) → Date | ParseError`. One input, one well-typed output, all the date-parsing horror inside. + +**Shallow:** `applyOrderDiscount(order, discount, taxRate, locale, customerTier, productCategory, isHoliday, ...) → Order`. The caller has to assemble all the inputs anyway — the function is barely doing anything the call site wasn't already doing. + +### Module scale + +**Deep:** A `RetryPolicy` module that exposes `withRetry(operation, policy)`. Callers don't think about exponential backoff, jitter, max attempts, error classification — those live behind the interface. + +**Shallow:** A `RetryHelper` that exposes `getBackoffMs`, `shouldRetry`, `incrementAttempt`, `resetAttempts`. Every caller has to compose these correctly. The "module" is just a namespaced bag of helpers. + +### Package scale + +**Deep:** A `billing/` package that exposes `chargeCustomer(customerId, amount, idempotencyKey)`. Inside: payment provider abstraction, retry on transient failures, idempotency checks, audit logging, ledger writes. + +**Shallow:** A `billing/` package that exposes `getPaymentProvider`, `formatAmount`, `validateAmount`, `recordCharge`, `recordRefund`, `auditLog`. Every caller has to thread these together. The package adds no leverage over a folder. + +## Seam + +A seam is a point where the interface lives — a place behavior can be altered without editing the existing code in place. + +A seam is *interesting* when there's a real reason to vary behavior at it: testing (substitute a fake), product needs (swap providers), platform constraints (switch implementations). A seam invented for "potential future flexibility" is just speculative complexity. + +> One adapter = hypothetical seam. Two adapters = real seam. + +If only one adapter exists and is the only one that ever will exist, the seam is fictional and can usually be deleted. + +## Adapter + +A concrete thing that satisfies an interface at a seam. `PostgresUserRepo` and `InMemoryUserRepo` are two adapters at the `UserRepo` seam. + +Adapters justify seams. A seam without at least two real adapters is suspicious. + +## Leverage + +The user-facing benefit of depth. What does the caller stop having to think about? What gets shorter? What can callers compose now that they couldn't before? + +## Locality + +The maintainer-facing benefit of depth. When this concept changes, where do the changes go? + +- **High locality:** changes to one concept stay in one module +- **Low locality:** changes to one concept ripple across N callers + +Locality is the property that lets a small team reason about a big system. Depth is the mechanism that produces locality. + +## How these terms compose + +> A **deep module** sits behind a **seam** with one or more **adapters**. Its **interface** carries low cognitive load relative to its **implementation**, giving callers **leverage** and concentrating maintenance in one place — that's **locality**. + +When proposing a deepening, the proposal should make explicit what changes for **leverage** (callers) and **locality** (maintainers). If both can't be named, the deepening isn't real. + +## Common shallow patterns to look for + +| Pattern | Smell | +|---|---| +| Helper module with N independent functions | Each caller has to compose them — locality wasted | +| Pure function extracted from a stateful flow for "testability" | The bugs are in the stateful flow; pure-function tests pass while the real path breaks | +| Wrapper that just renames calls | Pass-through with vocabulary churn — see deletion test | +| "Service" that exposes its CRUD | Callers know the storage shape; the seam isn't doing anything | +| Configuration object passed through three layers | Each layer is a shallow conduit; consider pushing the consumer up to the source | + +## Things that look shallow but are not + +| Looks shallow | But actually | +|---|---| +| One-line wrapper | If it consolidates an invariant in one place, it's earning its keep | +| Tiny module with one function | If two adapters exist, the seam is real | +| Module with verbose interface | Verbose ≠ shallow — a deep module can have many parameters if each one carries real semantic weight | + +The deletion test is the disambiguator. Run it when in doubt. diff --git a/plugins/skill-set/skills/improving-architecture/reference/deepening.md b/plugins/skill-set/skills/improving-architecture/reference/deepening.md new file mode 100644 index 0000000..459d3f7 --- /dev/null +++ b/plugins/skill-set/skills/improving-architecture/reference/deepening.md @@ -0,0 +1,52 @@ +# Deepening + +How to deepen a cluster of shallow modules safely, given its dependencies. Assumes the vocabulary in `deep-modules.md` — **module**, **interface**, **seam**, **adapter**. + +## Dependency categories + +When assessing a candidate for deepening, classify its dependencies. The category determines how the deepened module is tested across its seam. + +### 1. In-process + +Pure computation, in-memory state, no I/O. **Always deepenable** — merge the modules and test through the new interface directly. No adapter needed. + +### 2. Local-substitutable + +Dependencies that have local test stand-ins (PGLite for Postgres, in-memory filesystem, embedded Redis). **Deepenable if the stand-in exists.** The deepened module is tested with the stand-in running in the test suite. The seam is internal; no port at the module's external interface. + +### 3. Remote but owned (Ports & Adapters) + +Your own services across a network boundary — microservices, internal APIs, your own message queues. **Define a port (interface) at the seam.** The deep module owns the logic; the transport is injected as an **adapter**. Tests use an in-memory adapter. Production uses an HTTP / gRPC / queue adapter. + +Recommendation shape: + +> "Define a port at the seam, implement an HTTP adapter for production and an in-memory adapter for testing, so the logic sits in one deep module even though it's deployed across a network." + +### 4. True external (Mock) + +Third-party services you don't control — Stripe, Twilio, OAuth providers. **Take the external dependency as an injected port; tests provide a mock adapter.** Different from category 3 only in that you cannot control the wire-level behavior, so the mock is not a proxy for "real but cheap" — it's an explicit fiction the test owns. + +## Seam discipline + +- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a port unless at least two adapters are justified (typically production + test). A single-adapter seam is just indirection. +- **Internal seams vs. external seams.** A deep module can have internal seams (private to its implementation, used by its own tests) as well as the external seam at its interface. **Don't expose internal seams through the interface just because tests use them** — that leaks implementation into the contract. + +## Testing strategy: replace, don't layer + +When deepening shallow modules: + +- **Old unit tests on the shallow modules become candidates for removal** once tests at the deepened module's interface exist — consider deleting the now-redundant ones, but keep any that document a non-obvious invariant a future reader would need. +- **Write new tests at the deepened module's interface.** The interface is the test surface. +- **Tests assert on observable outcomes through the interface, not internal state.** +- **Tests should survive internal refactors** — they describe behavior, not implementation. If a test has to change when the implementation changes, it's testing past the interface. + +## Choosing the strategy in practice + +```text +Pure computation? → Category 1. Just merge. +Has a local stand-in? → Category 2. Use it. +Network-bounded, you own both sides? → Category 3. Port + 2 adapters. +Third-party you don't own? → Category 4. Mock at the seam. +``` + +If a single deepening candidate spans multiple categories (e.g., it computes plus calls Stripe), split the seams: pure computation stays internal, the Stripe call gets its own port + mock. diff --git a/plugins/skill-set/skills/improving-architecture/reference/deletion-test.md b/plugins/skill-set/skills/improving-architecture/reference/deletion-test.md new file mode 100644 index 0000000..da55c77 --- /dev/null +++ b/plugins/skill-set/skills/improving-architecture/reference/deletion-test.md @@ -0,0 +1,71 @@ +# The Deletion Test + +The single most useful heuristic for distinguishing deep from shallow modules. + +## The test + +Imagine deleting the module. Inline its body at every call site. + +- **If complexity vanishes:** the module was a pass-through. It added vocabulary but no leverage. *Shallow.* Delete or merge. +- **If complexity reappears across N callers:** the module was concentrating something. Each caller would now have to reproduce the work. *Deep.* Keep it. +- **If the module mostly disappears but a small invariant has to be re-asserted at each call site:** the module was earning its keep at exactly that invariant. Keep it, possibly trim the surrounding code. + +## How to actually run it + +You don't need to literally delete the file. Run it as a thought experiment in three steps: + +### Step 1 — list callers + +Use `LSP findReferences` on the public API of the module. Get the actual call sites. + +### Step 2 — for each call site, write what would need to change + +For each caller, ask: if the module disappeared, what would I have to put here? + +- Just the function body? → That's a pass-through. +- The function body plus a couple of guards / coercions / wrap-unwraps? → Borderline; think about whether those guards are the same at every site. +- The function body plus state, error handling, retries, ordering? → The module is doing real work. + +### Step 3 — count the duplication + +If the same non-trivial thing would be repeated at 3+ call sites, the module is deep enough to keep. Two callers can be a coincidence; three suggests a pattern worth concentrating in one place. + +If the "complexity" is just type coercion and could be replaced by a one-liner everywhere, it's shallow. + +## What counts as "complexity reappearing" + +| Counts | Doesn't count | +|---|---| +| Stateful logic — counters, retries, caches | Argument forwarding | +| Invariant assertions that callers would otherwise forget | Type narrowing the caller already does | +| Coordinated multi-step work | Renaming a single function call | +| Error classification with non-trivial branching | Error re-throw | +| Locking, ordering, atomicity | Plain dispatch | + +## Common false-negatives (modules that *look* shallow but pass the test) + +- **Validation modules.** Look like one function calling N small checks. But the checks together encode a domain rule that callers would otherwise duplicate poorly. The module is the *single source of truth* for the rule. +- **Idempotency wrappers.** May look like passthroughs. But the idempotency key handling, ledger lookup, and outcome caching would be a nightmare to reproduce per caller. +- **Migration shims.** Often have a tiny implementation but their value is "all the legacy handling lives here, callers stay clean." + +## Common false-positives (modules that *look* deep but fail the test) + +- **Large helper bags.** A module with ten unrelated functions can look "big" but each function is shallow on its own and callers compose them anyway. +- **Wrappers that just rename.** A `BillingService.charge()` that just calls `paymentProvider.charge()` adds vocabulary, no leverage. +- **"Service" objects with leaked internals.** If callers reach inside (`service.repo.findById(...)`), the seam was theatrical. + +## When the test is ambiguous + +Apply the **two-adapter test**: does this module have two real, used adapters? If yes, the seam is real. If no, the seam is hypothetical and can usually be deleted. + +If still ambiguous, leave it alone. Architectural review should not propose changes the team is unsure about. List it under "watch" rather than "propose." + +## What to do with the result + +| Result | Action | +|---|---| +| Clearly shallow, single caller | Inline at the call site, delete the module | +| Clearly shallow, multiple callers | Inline; if duplication appears across callers, propose a different deepening that captures the *real* invariant | +| Clearly deep | Leave alone. Possibly improve naming or trim incidental complexity. | +| Borderline | Note it; come back when you have more callers or more friction | +| Two-adapter test fails (only one adapter, no plan for another) | Propose collapsing the seam — the indirection is unjustified | diff --git a/plugins/skill-set/skills/improving-architecture/reference/interface-design.md b/plugins/skill-set/skills/improving-architecture/reference/interface-design.md new file mode 100644 index 0000000..2b8a81f --- /dev/null +++ b/plugins/skill-set/skills/improving-architecture/reference/interface-design.md @@ -0,0 +1,56 @@ +# Interface Design + +**When to skip:** if the right interface is already obvious for the chosen candidate, skip this entire reference and hand off to `grilling-plans` directly. Use this only when the interface shape would set a long-term direction and the user is unsure between alternatives. + +When the user picks a deepening candidate and you need to explore alternative interfaces for the deepened module, use this **parallel sub-agent pattern**. + +Based on Ousterhout's "Design It Twice": your first interface idea is unlikely to be the best. Force yourself to generate radically different alternatives, then choose. + +Uses the vocabulary in `deep-modules.md` — **module**, **interface**, **seam**, **adapter**, **leverage**. + +## Process + +### 1. Frame the problem space + +Before spawning sub-agents, write a user-facing explanation of the problem space for the chosen candidate: + +- The constraints any new interface would need to satisfy +- The dependencies it would rely on, and which category they fall into (see `deepening.md`) +- A rough illustrative code sketch to ground the constraints — not a proposal, just a way to make the constraints concrete + +Show this to the user, then immediately proceed to step 2. The user reads and thinks while sub-agents work in parallel. + +### 2. Spawn sub-agents + +Use the `Agent` tool with `superpowers:dispatching-parallel-agents` to spawn 3+ agents in parallel. Each must produce a **radically different** interface for the deepened module. (Ousterhout's original argument is "design it twice"; we go to three because the fourth Ports & Adapters constraint only fires for category-3/4 dependencies, so three is the realistic floor when network seams are in play.) + +Prompt each sub-agent with a separate technical brief: file paths, coupling details, dependency category from `deepening.md`, what sits behind the seam. The brief is independent of the user-facing problem-space explanation. Give each agent a different design constraint: + +- **Agent 1: Minimize the interface.** Aim for 1–3 entry points max. Maximize leverage per entry point. +- **Agent 2: Maximize flexibility.** Support many use cases and extension points. +- **Agent 3: Optimize for the most common caller.** Make the default case trivial; rare cases may carry extra ceremony. +- **Agent 4 (when applicable): Ports & Adapters.** Design around the dependency seams from `deepening.md` so adapters can vary without touching the logic. + +Include both architecture vocabulary (`deep-modules.md`) and project domain vocabulary (`CONTEXT.md`) in each brief so each sub-agent names things consistently. + +Each sub-agent outputs: + +1. **Interface** — types, methods, params, plus invariants, ordering, error modes +2. **Usage example** showing how callers use it +3. **What the implementation hides** behind the seam +4. **Dependency strategy** and adapters (per `deepening.md`) +5. **Trade-offs** — where leverage is high, where it's thin + +### 3. Present and compare + +Present designs **sequentially** so the user can absorb each one, then compare them in prose. Contrast by: + +- **Depth** — leverage at the interface +- **Locality** — where change concentrates +- **Seam placement** — what varies, what's fixed + +After comparing, give your own recommendation: which design you think is strongest and why. **Be opinionated — the user wants a strong read, not a menu.** If elements from different designs combine well, propose a hybrid. + +### 4. Hand off to grilling + +Once the user picks a design, hand off to `grilling-plans` to interrogate it before implementation. Designs always look better in the abstract; grilling forces the unspoken assumptions into the open. diff --git a/plugins/skill-set/skills/zooming-out-on-code/SKILL.md b/plugins/skill-set/skills/zooming-out-on-code/SKILL.md new file mode 100644 index 0000000..29138eb --- /dev/null +++ b/plugins/skill-set/skills/zooming-out-on-code/SKILL.md @@ -0,0 +1,109 @@ +--- +name: zooming-out-on-code +description: Draws a higher-level system map of unfamiliar internal/project code in the project's domain vocabulary — describes the module's responsibility, callers, dependencies, and sibling modules without diving into implementation details. Use this skill proactively whenever a user expresses unfamiliarity with a project code area before changing it — phrases like "zoom out", "give me the big picture", "explain this code area", "new to this code", "where does X fit", "what calls this", "tour the module", "before I change this" — even if they don't say "zoom out" explicitly. +--- + +# Zooming Out On Code + +## Overview + +When dropped into unfamiliar code, the agent's first instinct is often to read the file linearly. That produces local understanding without orientation. **Zooming out** does the opposite: go up one level of abstraction first, so the file in question is read with full system context. + +**Core principle:** Understand a module by understanding its place, not its body. + +## When to Use + +- User opens a file or function they don't recognize and asks for context +- Before proposing changes to an unfamiliar area +- When a teammate hands off a system you have no prior context on +- User says "zoom out", "explain this code area", "give me the big picture", "what does this do in context" + +## Do NOT use for + +- External library / framework documentation → `understanding-code-context` +- Reviewing changed code for quality → `simplify` +- Finding refactor opportunities → `improving-architecture` +- Debugging a specific bug → `superpowers:systematic-debugging` + +## Process + +1. **Read project context first.** If `CONTEXT.md` and `docs/adr/` exist (per `building-shared-vocabulary`), read them first. If absent, infer domain vocabulary from package/module names, public type/class names that mention business nouns, test descriptions, and recent commit messages. + +2. **Go up one level.** + - If the user asks about a function, describe its containing module + - If the user asks about a module, describe its containing package or layer + - If the user asks about a package, describe the system + - If the user asks about a top-level package or whole app, describe its role in the product/business domain — do not recurse further (recursing past the top-level package turns orientation into product strategy, which is out of scope and rarely useful for a code change) + +3. **Map four things.** These four answer "what is it for, who needs it, what does it need, what's adjacent" — together they place the module in the dependency graph without reading its body. + - **Responsibility** — what is this module's single reason to exist, in domain vocabulary? + - **Callers** — who depends on this module? Grep on the public API name; use `LSP findReferences` if an LSP is available for the language. + - **Dependencies** — what does this module depend on? Read first-level imports only; transitive imports belong to a deeper zoom, not this one. + - **Siblings** — what other modules sit at the same level of the system? Glob the parent directory. + +4. **Answer in domain language, not implementation language.** "The Order intake module validates incoming Orders against the active Promotion catalog" beats "OrderHandler calls promotionService.validate()." + +5. **Stop at the right level.** The goal is *orientation*, not *encyclopedia*. If the user wants to go deeper into a specific branch, they'll ask. + +## Output template + +``` +**** + +**Responsibility:** + +**Callers:** +- — +- — + +**Dependencies:** +- — +- — + +**Siblings (same layer):** +- — +- — + +**Where it sits in the system:** + +``` + +## Example + +User asks about `src/orders/intake/validate.ts`. + +``` +**Order Intake — Validation** + +**Responsibility:** Confirms an incoming Order satisfies promotion eligibility, +inventory availability, and customer credit before the Order is accepted. + +**Callers:** +- `src/orders/intake/route.ts` — POST /orders endpoint, calls validation before persisting +- `src/admin/manual-orders.ts` — admin tool that creates Orders on behalf of a Customer + +**Dependencies:** +- `src/promotions/catalog.ts` — looks up active promotions for the Customer +- `src/inventory/availability.ts` — confirms each Line Item is stocked +- `src/billing/credit.ts` — reads Customer credit limit + +**Siblings (same layer — `src/orders/intake/`):** +- `route.ts` — HTTP boundary +- `persist.ts` — writes accepted Orders to the write model +- `events.ts` — emits `OrderPlaced` events on success + +**Where it sits in the system:** +Validation is the gate between an HTTP request and a confirmed Order in the write +model. After it passes, persist + event emission are mechanical; before it passes, +nothing about the Order is durable. +``` + +## Troubleshooting + +| Symptom | Cause | Fix | +|---|---|---| +| Output sounds like the file's docstring | Did not actually go up a level | Re-do step 2 — describe the *containing* unit, not the unit itself | +| Output uses class/function names instead of domain terms | Skipped step 1 (didn't read CONTEXT.md) or no glossary exists | Read CONTEXT.md if present; otherwise note its absence and use the domain terms inferable from the code | +| Caller list is empty but module clearly is in use | API is exported through a barrel/index file | Search for re-exports, then trace from there | +| Output is huge | Stopped at the wrong level | Cut to the four-item template strictly; deeper detail belongs to follow-up questions |