From c85f96c62e9d613ed904d4f83928bc586b32a7b7 Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Wed, 13 May 2026 17:30:59 +0200 Subject: [PATCH] feat(skills): improve-codebase-architecture skill adapted for template-vertical MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adapts mattpocock/skills/engineering/improve-codebase-architecture to this repo. Four files at .claude/skills/improve-codebase-architecture/: SKILL.md (104 lines): - Explore -> Present candidates -> Grilling loop process - "Hard constraints (do not propose violations)" section enumerating ADRs 006/008/010/012/013/014/015/017/020/021 that bound the design space - Repointed at docs/glossary.md (not CONTEXT.md) and docs/decisions/ (not docs/adr/) - Exploration shortcuts specific to this repo: pnpm fallow, pnpm coverage:diff, feature.manifest.ts, pnpm turbo boundaries - Grilling loop side-effects target the right glossary section and the next available ADR number (currently 022) DEEPENING.md (93 lines): - 4 dependency categories mapped to this repo's reality: Cat 1 (in-process) -> entities/use-cases/presenters Cat 2 (local-substitutable) -> our existing real + mock adapter pattern (every port has both; mocks ARE stand-ins) Cat 3 (remote but owned) -> cross-feature events via IEventBus (E0/E1 rules) Cat 4 (true external) -> Payload, Sentry/OTel, socket.io (each constrained to its vendor-isolation seam by ADR) - Seam discipline section recognises DI symbols + manifest entries as concrete seams alongside .interface.ts files - Testing strategy: replace not layer (matches ADR-020 L0 + L1) - Conformance check command list at the end (typecheck, lint, test --coverage, conformance, fallow:audit, coverage:diff) INTERFACE-DESIGN.md (66 lines): - Parallel sub-agent "Design It Twice" pattern preserved - Every sub-agent brief MUST include glossary terms + ADR constraints + manifest awareness - Output items extended with "Manifest + binder impact" and "ADR conflicts (if any)" - Comparison axes include conformance impact + coverage delta - Cross-feature moves flag release-please version-bump implications (per ADR-021 commit-path targeting) LANGUAGE.md (79 lines): - Matt's 7 abstract terms preserved (module, interface, implementation, depth, seam, adapter, leverage, locality) - New "Mapping to this repo's identifiers" table — abstract term -> concrete file shape (e.g. seam -> *.interface.ts + DI symbol + manifest entry + anchor) - Rejected framings extended with our reserved meanings ("boundary" stays the ESLint workspace-tag term; "service" stays the DI port term) Per user follow-up: vocabulary anchored so that "module" defaults to "feature" in this repo (since features are our primary unit of organisation). Abstract refactor sense survives only when the cross- scale abstraction is the point. Glossary.md updated: - "Feature" entry adds the "module = feature in refactor sense" cross-link - New "Architecture refactor vocabulary" section with 9 terms (Module, Interface (refactor sense), Implementation, Depth, Seam, Adapter, Leverage, Locality, Deletion test, Deepening) — all framed so feature is the primary instance - Flagged ambiguities entry for "module" rewritten to capture the three coexisting senses (workspace package / Node ESM / refactor vocabulary defaulting to feature); new entries for "seam" and "adapter" to prevent drift with the existing "boundary" / "service" / "scope" reservations Hooks updated: - session-start.sh skills line lists the new skill - prompt-context.sh adds a 10th keyword group firing on refactor / deepening / shallow / architecture / seam / adapter / interface design / design it twice — inject points at SKILL.md + summarises the vocabulary and hard constraints Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/hooks/prompt-context.sh | 3 + .claude/hooks/session-start.sh | 2 +- .../DEEPENING.md | 98 +++++++++++++++++ .../INTERFACE-DESIGN.md | 66 +++++++++++ .../improve-codebase-architecture/LANGUAGE.md | 78 +++++++++++++ .../improve-codebase-architecture/SKILL.md | 104 ++++++++++++++++++ docs/glossary.md | 44 +++++++- 7 files changed, 391 insertions(+), 4 deletions(-) create mode 100644 .claude/skills/improve-codebase-architecture/DEEPENING.md create mode 100644 .claude/skills/improve-codebase-architecture/INTERFACE-DESIGN.md create mode 100644 .claude/skills/improve-codebase-architecture/LANGUAGE.md create mode 100644 .claude/skills/improve-codebase-architecture/SKILL.md diff --git a/.claude/hooks/prompt-context.sh b/.claude/hooks/prompt-context.sh index 8f4b784..afa0ef6 100755 --- a/.claude/hooks/prompt-context.sh +++ b/.claude/hooks/prompt-context.sh @@ -43,6 +43,9 @@ fi if echo "$prompt" | grep -qE 'release|version|bump|semver|tag\b'; then inject+=('Releases: ADR-021 + docs/guides/releasing.md. Hybrid versioning — root template (template-v...) + 5 feature packages (auth-v..., blog-v..., etc.) version independently from 0.1.0. release-please reads Conventional Commits and opens a rolling release PR on every push to main; merging cuts per-package tags. Bump targeting is by commit-path, not (scope). Pre-1.0 policy: feat: -> patch, feat!: -> minor.') fi +if echo "$prompt" | grep -qE 'refactor|deepening|shallow|architecture|seam|adapter|interface design|design it twice'; then + inject+=('Architecture refactors: invoke the improve-codebase-architecture skill (.claude/skills/improve-codebase-architecture/SKILL.md). Vocabulary: module (= feature by default in this repo) / interface / seam / adapter / depth / leverage / locality. Process: Explore -> Present numbered candidates -> Grilling loop. Hard constraints: respect ADRs 001-021 (factory-function shape, per-feature DI, manifest-first, generator-first, boundary tags, vendor isolation). Companion files: DEEPENING.md (dependency categories), INTERFACE-DESIGN.md (parallel sub-agent design pattern), LANGUAGE.md (vocab + this-repo identifier mapping).') +fi if [ ${#inject[@]} -gt 0 ]; then echo "=== context-relevant pointers (from .claude/hooks/prompt-context.sh) ===" diff --git a/.claude/hooks/session-start.sh b/.claude/hooks/session-start.sh index 39c773b..e6ed67b 100755 --- a/.claude/hooks/session-start.sh +++ b/.claude/hooks/session-start.sh @@ -11,7 +11,7 @@ Generator-first: pnpm turbo gen beats hand-rolled scaffolding (non-negoti Conformance: pnpm conformance + pnpm fallow (5-gate drift detection) Conventional Commits (non-negotiable): (): — see CLAUDE.md Key Conventions Releases: release-please reads commits + opens rolling release PR on merge to main (ADR-021) -Skills: to-prd, grill-with-docs, grill-me, handoff (.claude/skills/) +Skills: to-prd, grill-with-docs, grill-me, handoff, improve-codebase-architecture (.claude/skills/) EOF exit 0 diff --git a/.claude/skills/improve-codebase-architecture/DEEPENING.md b/.claude/skills/improve-codebase-architecture/DEEPENING.md new file mode 100644 index 0000000..9eb41f7 --- /dev/null +++ b/.claude/skills/improve-codebase-architecture/DEEPENING.md @@ -0,0 +1,98 @@ +# Deepening + +How to deepen a cluster of shallow modules in this repo, given its dependencies. **In this repo "module" defaults to "feature"** — most deepenings operate on or within a feature (`packages//`). Narrower scopes (use case, controller, repository) follow the same dependency-category logic. Assumes the vocabulary in [LANGUAGE.md](LANGUAGE.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. + +**Where this lives in our repo:** + +- `entities/models/**` — Zod schemas + types + pure helpers +- `entities/errors/**` — domain error classes +- `application/use-cases/**` — pure orchestration (factories take ports as deps) +- `interface-adapters/controllers/**` + their colocated `presenter` functions +- Pure helpers in `core-shared/conformance/`, `core-shared/instrumentation/` (the non-OTel parts) + +For category 1 modules: **merge, then test the result through its interface**. No mocks. The test surface IS the new interface. + +### 2. Local-substitutable + +Dependencies that have local test stand-ins. **In this repo every infrastructure port already has both a real implementation and a mock side-by-side** — the mock IS the test stand-in. + +**Pattern (from ADR-012):** + +- `.repository.interface.ts` — the port (seam) +- `.repository.ts` — Payload-backed adapter (real) +- `.repository.mock.ts` — in-memory adapter (test stand-in) + +The deepened module is tested with the `.mock.ts` adapter injected directly via the factory function. No container rebinding (ADR-012). + +**Common category-2 ports in this repo:** + +- `IUsersRepository`, `IArticlesRepository`, `IMediaRepository`, etc. → Mock + Payload adapters +- `IAuthenticationService` → Mock + real adapter +- `IJobQueue` (from `core-shared/jobs/`) → `InMemoryJobQueue` + `PayloadJobQueue` +- `IEventBus` (from `core-events/`) → `InMemoryEventBus` + `PayloadJobsEventBus` +- `ITracer`, `ILogger`, `IMetrics` (from `core-shared/instrumentation/`) → `Noop*` + `Otel*` + `Recording*` (the third one lives in `core-testing` for assertions) + +If the deepening touches a category-2 port, the recommendation shape is: _"Merge X into Y, keep the port boundary at the existing `.repository.interface.ts`; both adapters survive unchanged."_ + +### 3. Remote but owned + +Our own services across a network boundary. **In this repo, cross-feature communication is already this pattern via the event bus (ADR-015).** + +The **port** is `IEventBus.publish(descriptor, payload)` + `IEventBus.subscribe(descriptor, consumerFeature, handler)`. Adapters: + +- `InMemoryEventBus` (test + dev-seed) — synchronous fan-out +- `PayloadJobsEventBus` (production) — Payload tasks fan-out durably across the network if features are split into separate deploys + +If a deepening proposal would introduce a NEW cross-feature seam, the answer is almost always "use the event bus" — don't invent a new transport. Rule E0 forbids in-feature use of the bus (in-feature reactions are direct use-case calls); E1 keeps consumer handlers private. + +If the deepening proposal would EXPOSE one feature's internals to another, that's a boundary violation — reject and suggest events instead. + +### 4. True external + +Third-party services we don't control. The deepened module takes the external dependency as an injected port; tests provide a mock adapter. + +**Where this lives in our repo:** + +- Payload CMS itself — features depend on `IXRepository`, not on `payload` directly. The real adapter (`.repository.ts`) is the only place Payload is touched. +- Sentry / OpenTelemetry exporters — features use `ITracer`/`ILogger`/`IMetrics` from `core-shared/instrumentation/`. The OTel SDK lives ONLY in `core-shared/instrumentation/otel/` (ESLint-enforced via rule R52). +- Socket.IO — features use `IRealtimeBroadcaster`; `socket.io` itself lives only in `@repo/core-realtime` (rule R2). + +If a deepening proposal would import a vendor SDK from a feature package, that's an ADR-014/ADR-016/ADR-017 violation — reject and route through the existing port. + +## 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). Single-adapter ports in this repo are usually a smell — check if the mock is missing or if the port itself is unnecessary. +- **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. +- **The DI symbol is the seam contract.** `*_SYMBOLS.IXRepository` plus `.repository.interface.ts` together define what callers depend on. Adapter swaps happen at bind time in `bind-production.ts` / `bind-dev-seed.ts`. +- **`feature.manifest.ts` is the structural-conformance seam.** If a deepening moves use cases across features, the manifests of BOTH features change — the conformance ESLint rules + `assertFeatureConformance` boot check enforce that the move is reflected in declarations, not just code. + +## Testing strategy: replace, don't layer + +- **Old unit tests on shallow modules become waste** once tests at the deepened module's interface exist — delete them. Our coverage thresholds (ADR-020) reward this: collapsing N shallow modules + their N test files into one deep module + one test file at its interface keeps coverage 100% without test bloat. +- **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** — if a test has to change when the implementation changes, it's testing past the interface. +- **L1 diff coverage (`pnpm coverage:diff`) will surface uncovered lines after the refactor** — every deepened module needs its new tests to cover the merged behaviour before the refactor PR is mergeable. + +## Conformance check (run before claiming the deepening is complete) + +After deepening, the following must all stay green: + +``` +pnpm typecheck # TS brand-slot enforcement +pnpm lint # ESLint conformance + boundaries +pnpm test --filter @repo/ -- --coverage # per-layer L0 thresholds +pnpm conformance # cross-feature event closure +pnpm fallow:audit # whole-codebase audit + dead-export sweep +pnpm coverage:diff -- --base origin/main # cover-the-diff (ADR-020 L1) +``` + +If `pnpm dev` was running, it should still boot — `assertFeatureConformance` will fail loudly on brand-slot or manifest drift if the deepening forgot a binding update. diff --git a/.claude/skills/improve-codebase-architecture/INTERFACE-DESIGN.md b/.claude/skills/improve-codebase-architecture/INTERFACE-DESIGN.md new file mode 100644 index 0000000..498f9fd --- /dev/null +++ b/.claude/skills/improve-codebase-architecture/INTERFACE-DESIGN.md @@ -0,0 +1,66 @@ +# Interface Design + +When the user wants to explore alternative interfaces for a chosen deepening candidate, use this parallel sub-agent pattern. Based on "Design It Twice" (Ousterhout) — your first idea is unlikely to be the best. + +Uses the vocabulary in [LANGUAGE.md](LANGUAGE.md) — **module** (= **feature** by default in this repo), **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](DEEPENING.md)) +- The **hard constraints from SKILL.md** that the new interface must respect (factory-function shape, per-feature DI, manifest-first, generator-first, brand wrappers, etc.) +- 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 the sub-agents work in parallel. + +### 2. Spawn sub-agents + +Spawn 3+ sub-agents in parallel using the `Agent` tool (`subagent_type=general-purpose` or a more specific type if appropriate). Each must produce a **radically different** interface for the deepened module. + +Prompt each sub-agent with a separate technical brief (file paths, coupling details, dependency category from [DEEPENING.md](DEEPENING.md), what sits behind the seam). The brief is independent of the user-facing problem-space explanation in Step 1. Give each agent a different design constraint: + +- **Agent 1**: "Minimise the interface — aim for 1–3 entry points max. Maximise leverage per entry point." +- **Agent 2**: "Maximise flexibility — support many use cases and extension." +- **Agent 3**: "Optimise for the most common caller — make the default case trivial." +- **Agent 4 (if applicable)**: "Design around ports & adapters for cross-seam dependencies." + +**Every brief MUST also include:** + +- This repo's vocabulary from [`docs/glossary.md`](../../../docs/glossary.md) (use case, manifest, feature, slice, etc.) +- The architecture vocabulary from [LANGUAGE.md](LANGUAGE.md) +- The hard constraints from [SKILL.md](SKILL.md) — sub-agents must not propose interfaces that violate ADR-006 (boundaries), ADR-008 (per-feature DI), ADR-012 (factory shape, one controller per use case), ADR-013 (schemas in use-case file), ADR-014/017 (vendor isolation), ADR-015 (events for cross-feature), ADR-020 (manifest-driven coverage bands), or ADR-021 (versioning by commit-path). +- The relevant feature `feature.manifest.ts` shape so the proposed interface aligns with manifest-first ordering. + +Each sub-agent outputs: + +1. **Interface** (types, methods, params — plus invariants, ordering, error modes, schemas if applicable) +2. **Usage example** showing how callers in this repo would use it (use real file paths and real existing feature names) +3. **What the implementation hides** behind the seam +4. **Dependency strategy and adapters** (see [DEEPENING.md](DEEPENING.md)) — which existing ports/adapters get reused, which (if any) are new +5. **Manifest + binder impact** — which `feature.manifest.ts` entries and which `bind-production.ts` / `bind-dev-seed.ts` files change +6. **Trade-offs** — where leverage is high, where it's thin +7. **ADR conflicts (if any)** — call out by ADR number with rationale, or state "none" + +### 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** (which existing `*.interface.ts` survives, which gets replaced, which is new) +- **Conformance impact** (how many manifests change, how many binders change, how many tests rewrite) +- **Coverage delta** (cumulative L0 band impact — does any layer drop below its declared 100% / 95%?) + +After comparing, give your own recommendation: which design you think is strongest and why. If elements from different designs would combine well, propose a hybrid. Be opinionated — the user wants a strong read, not a menu. + +If the chosen design crosses a feature-package boundary (e.g. moves a use case from `@repo/blog` to `@repo/media`), state explicitly: + +- Which `feature.manifest.ts` files lose / gain entries +- Which package versions will bump on the next release-please PR (per ADR-021 commit-path bump targeting) +- Whether the migration needs an intermediate compatibility seam to keep `pnpm conformance` green during the transition + +The implementation lands via the manifest-first ordering: (1) update the manifests in both packages, (2) write the new contracts in the use-case file, (3) write the failing tests, (4) implement until green. Don't skip the order even when the move feels mechanical. diff --git a/.claude/skills/improve-codebase-architecture/LANGUAGE.md b/.claude/skills/improve-codebase-architecture/LANGUAGE.md new file mode 100644 index 0000000..1393042 --- /dev/null +++ b/.claude/skills/improve-codebase-architecture/LANGUAGE.md @@ -0,0 +1,78 @@ +# Language + +Shared vocabulary for every suggestion this skill makes. Use these terms exactly — don't substitute "component," "service" (we use that narrowly for DI ports), "API," or "boundary" (overloaded with our workspace-tag enforcement). Consistent language is the whole point. + +This vocabulary is foundational for the skill's reasoning. The project's domain vocabulary lives in [`docs/glossary.md`](../../../docs/glossary.md) — terms like _use case_, _manifest_, _slice_, _binder_, _brand_, _conformance band_, _coverage layer_. Both vocabularies are in scope when proposing deepenings; see the "Mapping to this repo's identifiers" section below for how the abstract terms here land on concrete file shapes. + +## Terms + +**Module** — **in this repo, "module" defaults to "feature"** (`packages//`). The abstract definition (anything with an interface + implementation) still applies at narrower scales — a use case, controller, repository/service port, or binder can also be a module — but **whenever the refactor scope is "the whole thing", say feature**. Reach for "module" only when the abstraction across scales actually matters (e.g., comparing how a use case's depth differs from its containing feature's depth). +_Avoid_: unit, component, service (we use "service" for DI ports specifically). + +**Interface** +Everything a caller must know to use the module correctly. Includes the type signature, but also invariants, ordering constraints, error modes, required configuration, performance characteristics, **manifest declarations**, and **DI symbol contract**. +_Avoid_: API, signature (too narrow — those refer only to the type-level surface). + +**Implementation** +What's inside a module — its body of code. Distinct from **Adapter**: a thing can be a small adapter with a large implementation (a Payload-backed repository) or a large adapter with a small implementation (an in-memory mock). Reach for "adapter" when the seam is the topic; "implementation" otherwise. + +**Depth** +Leverage at the interface — the amount of behaviour a caller (or test) can exercise per unit of interface they have to learn. A module is **deep** when a large amount of behaviour sits behind a small interface. A module is **shallow** when the interface is nearly as complex as the implementation. + +**Seam** _(from Michael Feathers)_ +A place where you can alter behaviour without editing in that place. The _location_ at which a module's interface lives. Choosing where to put the seam is its own design decision, distinct from what goes behind it. +_Avoid_: boundary (this repo uses "boundary" specifically for ESLint workspace-tag rules — keep it for that meaning). + +**Adapter** +A concrete thing that satisfies an interface at a seam. Describes _role_ (what slot it fills), not substance (what's inside). In this repo every port typically has at least two adapters (real + mock); some have three (real + mock + recording). + +**Leverage** +What callers get from depth. More capability per unit of interface they have to learn. One implementation pays back across N call sites and M tests. + +**Locality** +What maintainers get from depth. Change, bugs, knowledge, and verification concentrate at one place rather than spreading across callers. Fix once, fixed everywhere. + +## Principles + +- **Depth is a property of the interface, not the implementation.** A deep module can be internally composed of small, mockable, swappable parts — they just aren't part of the interface. A module can have **internal seams** (private to its implementation, used by its own tests) as well as the **external seam** at its interface. +- **The deletion test.** Imagine deleting the module. If complexity vanishes, the module wasn't hiding anything (it was a pass-through). If complexity reappears across N callers, the module was earning its keep. +- **The interface is the test surface.** Callers and tests cross the same seam. If you want to test _past_ the interface, the module is probably the wrong shape. +- **One adapter means a hypothetical seam. Two adapters means a real one.** Don't introduce a seam unless something actually varies across it. In this repo, the typical justification is "one real adapter + one mock for tests" — that's two. +- **The manifest is a structural seam.** A feature's `feature.manifest.ts` declares its use cases / events / jobs / channels / required cores / coverage bands. Refactors that move behaviour between features MUST move manifest entries too; the conformance gates enforce this. + +## Relationships + +- A **Module** has exactly one **Interface** (the surface it presents to callers and tests). +- **Depth** is a property of a **Module**, measured against its **Interface**. +- A **Seam** is where a **Module**'s **Interface** lives. +- An **Adapter** sits at a **Seam** and satisfies the **Interface**. +- **Depth** produces **Leverage** for callers and **Locality** for maintainers. + +## Mapping to this repo's identifiers + +Abstract → concrete translation table. When proposing a deepening, name things using the right column. + +| Abstract term | Where it lands in this repo | +| ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| **Module** | **Primarily a feature** (`packages//`) — that's the canonical refactor scope. Also: a use case (`*.use-case.ts`), controller (`*.controller.ts`), repository port + adapters (`*.repository.{interface,mock,}.ts`), service port + adapters (`*.service.{interface,mock,}.ts`), binder (`bind-production.ts` / `bind-dev-seed.ts`), manifest (`feature.manifest.ts`), or a core package (`packages/core-/`) — when the refactor operates at those narrower scales. When in doubt, say "feature". | +| **Interface** | The exported types from a module file: `IXUseCase = ReturnType`, `IXController`, the `.repository.interface.ts` shape, the manifest's declared keys, the Zod input/output schemas, the DI symbol contract. | +| **Implementation** | The factory body, the adapter class body, what the binder wires. | +| **Seam** | `.repository.interface.ts`, `.service.interface.ts`, the DI symbol (`*_SYMBOLS.IXRepository`), the manifest entry, a `// ` anchor, the protocol types in `core-shared/di/bind-protocols.ts`. | +| **Adapter** | `.repository.ts` (Payload real) ↔ `.repository.mock.ts` (in-memory). For instrumentation: `Noop*` ↔ `Otel*` ↔ `Recording*` (test). For bus: `InMemoryEventBus` ↔ `PayloadJobsEventBus`. | +| **Test stand-in** | The `.mock.ts` adapter (constructed directly + injected into the factory). No container rebinding (ADR-012). | + +## Rejected framings + +- **Depth as ratio of implementation-lines to interface-lines** (Ousterhout's original metric): rewards padding the implementation. We use depth-as-leverage instead. +- **"Interface" as the TypeScript `interface` keyword or a class's public methods**: too narrow — interface here includes every fact a caller must know, including manifest entries and DI symbols. +- **"Boundary"** as a synonym for **seam**: this repo uses "boundary" specifically for ESLint workspace-tag rules (`feature` may depend on `core` + `tooling` only). Keep that meaning intact; say **seam** or **interface** when discussing features. +- **"Service" as a generic term**: in this repo, **service** = a DI-injected port for non-collection capabilities (`IAuthenticationService`, `IMailerService`). Not a generic stand-in for "feature" or "the module doing the work." +- **"Module" as the canonical noun**: avoid in everyday discourse — say **feature** (or **use case** / **controller** / **package** when narrower). "Module" is the abstract refactor vocabulary's word for the same thing, useful only when the abstraction across scales is the point. + +## Cross-references + +- [`docs/glossary.md`](../../../docs/glossary.md) — project domain vocabulary (use case, manifest, slice, brand, coverage band, etc.) +- [`docs/decisions/`](../../../docs/decisions/) — 21 ADRs that constrain the design space +- [SKILL.md](SKILL.md) — the skill's process + hard constraints +- [DEEPENING.md](DEEPENING.md) — dependency categories + seam discipline +- [INTERFACE-DESIGN.md](INTERFACE-DESIGN.md) — parallel sub-agent design exploration diff --git a/.claude/skills/improve-codebase-architecture/SKILL.md b/.claude/skills/improve-codebase-architecture/SKILL.md new file mode 100644 index 0000000..c67cc34 --- /dev/null +++ b/.claude/skills/improve-codebase-architecture/SKILL.md @@ -0,0 +1,104 @@ +--- +name: improve-codebase-architecture +description: Find deepening opportunities in this repo, informed by docs/glossary.md and the ADRs in docs/decisions/. Use when the user wants to improve architecture, find refactoring opportunities, consolidate tightly-coupled modules, or make the codebase more testable and AI-navigable. Respects the conformance system, boundary rules, and the 21 ADRs that govern shape. +--- + +# Improve Codebase Architecture + +Surface architectural friction and propose **deepening opportunities** — refactors that turn shallow modules into deep ones. The aim is testability and AI-navigability, scoped to what this template's existing rules permit. + +## Glossary + +Use these terms exactly in every suggestion. Consistent language is the point — don't drift into "component," "service" (we use that for DI ports specifically), or "boundary" (we use that for ESLint workspace-tag rules). Full definitions in [LANGUAGE.md](LANGUAGE.md). + +- **Module** — **in this repo defaults to "feature"** (`packages//`). The abstract definition (anything with interface + implementation) still applies at narrower scales — a use case, controller, repository/service port, binder, or core package can also be a module — but say "feature" whenever that's the scope. Reach for "module" only when comparing depth/leverage across scales. +- **Interface** — everything a caller must know to use the module: types, invariants, error modes, ordering, schemas, DI shape. Not just the TypeScript type signature. +- **Implementation** — the code inside. +- **Depth** — leverage at the interface: a lot of behaviour behind a small interface. **Deep** = high leverage. **Shallow** = interface nearly as complex as the implementation. +- **Seam** — where an interface lives; a place behaviour can be altered without editing in place. In this repo seams take a concrete shape: `*.interface.ts` files, DI symbols, manifest declarations, and `` anchors. +- **Adapter** — a concrete thing satisfying an interface at a seam. In this repo: `.repository.ts` (Payload real impl) vs `.repository.mock.ts` vs `Recording*` test doubles. +- **Leverage** — what callers get from depth. +- **Locality** — what maintainers get from depth: change, bugs, knowledge concentrated in one place. + +Key principles (see [LANGUAGE.md](LANGUAGE.md) for the full list): + +- **Deletion test**: imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep. +- **The interface is the test surface.** +- **One adapter = hypothetical seam. Two adapters = real seam.** + +This skill is **informed by** the project's domain model and architecture decisions. Read [`docs/glossary.md`](../../../docs/glossary.md) for project vocabulary and the relevant ADR(s) in [`docs/decisions/`](../../../docs/decisions/) before proposing anything in their territory. + +## Hard constraints (do not propose violations) + +These are settled decisions — propose deepening WITHIN them, never against them: + +- **Factory-function use cases & controllers** (ADR-012, ADR-013) — every use case is `(deps) => async (input) => output`; every controller is one verb-noun pair per file with a co-located `presenter`. +- **Schemas in the use-case file** (ADR-013) — `xInputSchema`/`xOutputSchema` colocate with the factory; don't propose moving them to a separate module. +- **Per-feature DI containers** (ADR-008) — don't propose a single global container. +- **Five boundary tags + the dependency-direction matrix** (ADR-006, ADR-010) — features may depend only on `core` + `tooling`; cross-feature reactions go through `IEventBus` (ADR-015). +- **Manifest-first ordering** (ADR-012, ADR-020) — new use cases land manifest → contracts → tests → impl; don't propose collapsing the steps. +- **Brand-based conformance** — `Instrumented` / `Captured` / `Audited` are attached at DI bind time via `withSpan` / `withCapture` / `withAudit`; don't propose moving the wrapping elsewhere. +- **Generator-first** — `pnpm turbo gen ` is the entry point for new features/events/jobs/realtime channels. Don't propose hand-rolled scaffolding. +- **Conventional Commits** (CLAUDE.md Key Conventions) — any refactor lands as conventional-commit messages. +- **Hybrid versioning** (ADR-021) — refactors that move code between feature packages have version + CHANGELOG implications. + +If a proposed deepening **would** violate an ADR, surface it explicitly with the ADR number and a "worth reopening because…" justification — but only if the friction is real enough. Most should be silently scoped out. + +## Process + +### 1. Explore + +Read [`docs/glossary.md`](../../../docs/glossary.md) and any ADRs in the area you're touching first. Then walk the codebase noting friction. The primary unit of attention is the **feature** (`packages//`); narrower units (use cases, controllers, repositories) get attention when the friction lives at that scale. + +- Where does understanding one concept require bouncing between many small files across `entities/`, `application/`, `infrastructure/`, `interface-adapters/` inside a single feature? +- Where is a feature **shallow** — its public surface (the `.` + `./ui` + `./api` exports) nearly as complex as its internal implementation? Or where inside a feature is a smaller unit shallow: + - A `service.interface.ts` with one method that wraps a single repository call. + - A `presenter` that's just `(x) => x`. + - A controller body that's just `useCase(parsed.data)` with no transformation. + - A repository wrapping another repository. + - A use case wrapping another use case. +- Where have pure functions been extracted just for testability, but the real bugs hide in how they're called (no **locality**)? +- Where do tightly-coupled features leak across their seams? (e.g. a feature reaches into another feature's internals via deep import — though ESLint should catch this.) +- Which parts are untested, or hard to test through their current interface? `pnpm coverage:diff` and `pnpm fallow` surface candidates. + +Useful exploration shortcuts in this repo: + +- `pnpm fallow` — dead exports, dupes, complexity hotspots, circular deps +- `pnpm fallow:audit` — the AI-change audit; surfaces drift across recent edits +- `git log --oneline --follow -- ` — change frequency is a depth signal +- `cat packages//src/feature.manifest.ts` — declared surface of a feature +- `pnpm turbo boundaries` — workspace dependency graph + +Apply the **deletion test** to anything you suspect is shallow: would deleting it concentrate complexity, or just move it? A "yes, concentrates" is the signal you want. + +### 2. Present candidates + +Present a numbered list of deepening opportunities. For each candidate: + +- **Files** — which files / features / smaller units are involved (give exact paths) +- **Problem** — why the current architecture is causing friction +- **Solution** — plain English description of what would change +- **Benefits** — explained in terms of **locality** and **leverage**, plus how tests would improve +- **ADR impact** — any ADR this touches. If the proposed change conflicts with a current ADR, mark it explicitly: _"contradicts ADR-NNN — worth reopening because…"_ (only when the friction warrants it). +- **Manifest impact** — if the change moves use cases / events / jobs / channels across features, the `feature.manifest.ts` of each feature involved will need an update; flag this so the user knows the conformance gates will require manifest edits before code edits (manifest-first ordering). + +**Use [`docs/glossary.md`](../../../docs/glossary.md) vocabulary for the domain (use case, manifest, slice, feature, etc.) and [LANGUAGE.md](LANGUAGE.md) vocabulary for the architecture (module, seam, adapter, depth, leverage, locality).** + +Do NOT propose interfaces yet. Ask the user: "Which of these would you like to explore?" + +### 3. Grilling loop + +Once the user picks a candidate, drop into a grilling conversation. Walk the design tree with them — constraints, dependencies, the shape of the deepened module, what sits behind the seam, what tests survive. + +Side effects happen inline as decisions crystallize: + +- **Naming a deepened module after a concept not in [`docs/glossary.md`](../../../docs/glossary.md)?** Add the term to the glossary right there — same discipline as the `grill-with-docs` skill. Pick the appropriate section (Packages / Architecture layers / Feature building blocks / Conformance / Cross-feature / Instrumentation / Workflow / Releasing). +- **Sharpening a fuzzy term during the conversation?** Update the glossary inline. +- **User rejects the candidate with a load-bearing reason?** Offer an ADR, framed as: _"Want me to record this as ADR-NNN so future architecture reviews don't re-suggest it?"_ Only offer when the reason would actually be needed by a future agent to avoid re-suggesting the same thing. The next ADR number is `001 + max(existing)` (currently `ADR-022`). Our ADR shape: `Context → Decision → Alternatives considered → Consequences → Related`. +- **Refactor will move code between feature packages?** Flag the release-please impact: both affected packages will bump versions on the next release PR. +- **Want to explore alternative interfaces for the deepened module?** See [INTERFACE-DESIGN.md](INTERFACE-DESIGN.md). + +## Related skills + +- `grill-with-docs` (`.claude/skills/grill-with-docs/`) — stress-tests plans against ADRs + glossary + manifests; share the same glossary-update discipline. +- `to-prd` — if the deepening is large enough to merit a multi-task epic, materialize it as a PRD. diff --git a/docs/glossary.md b/docs/glossary.md index ab22b97..fe9d17e 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -22,8 +22,8 @@ _Avoid:_ module, library, app (unless specifically `apps/`). The boundary classification on a package (`app`, `core`, `core-composition`, `feature`, `tooling`). Enforced by ESLint (`eslint-plugin-boundaries`) and Turborepo `boundaries`. See `AGENTS.md` → Boundary Rules. **Feature** (a.k.a. **feature package**): -A vertical slice owning its Clean Architecture layers + integrations under `packages//`. Currently: `auth`, `blog`, `media`, `marketing-pages`, `navigation`. -_Avoid:_ domain, module, vertical (use "feature" or "vertical feature"). +A vertical slice owning its Clean Architecture layers + integrations under `packages//`. Currently: `auth`, `blog`, `media`, `marketing-pages`, `navigation`. **In architecture-refactor conversations (the `improve-codebase-architecture` skill), "module" defaults to "feature" — they're the same unit at the highest level of granularity.** +_Avoid:_ domain, vertical (use "feature" or "vertical feature"). "Module" is acceptable inside the refactor skill specifically, where it abstractly covers "anything with interface + implementation" at any scale (use case, controller, repository port, or full feature). **Must-have core**: A core package the template can't run without — `core-shared`, `core-cms`, `core-api`. @@ -101,6 +101,42 @@ A feature's allowed import surface — `.` (contracts: types, errors, schemas, ` **Anchor**: A `// ` comment marking where a generator injects code (e.g. ``, ``, ``). A CI guard at `packages/core-eslint/anchors.test.js` keeps them present. +## Architecture refactor vocabulary + +Terms used by the `improve-codebase-architecture` skill (`.claude/skills/improve-codebase-architecture/`) when proposing refactors. Distinct from the everyday domain terms above — these are abstractions for reasoning about shape. Both vocabularies are in scope during refactor conversations. + +**Module** (refactor sense, **= feature by default**): +The abstract unit a refactor operates on: anything with an interface + implementation. In this repo, **"module" almost always means "feature"** (`packages//`) since that's our primary unit of organization. At narrower scales it also covers a use case, controller, repository/service port, or binder. Don't introduce "module" as a competing top-level term in domain conversations — the refactor skill is the only place this abstraction is appropriate. +_Avoid:_ using "module" outside the refactor skill (say "feature", "use case", "controller", "package" as appropriate). + +**Interface** (refactor sense): +Everything a caller must know to use a module — type signatures, invariants, ordering, error modes, Zod schemas, manifest declarations, DI symbol contract. Strictly broader than the TS `interface` keyword. + +**Implementation**: +What's inside a module. Distinct from **adapter**: an adapter is a _role_ (fills a slot at a seam); an implementation is the _substance_ (the code itself). + +**Depth**: +Leverage at the interface — behaviour per unit of interface a caller must learn. A **deep module** packs a lot of behaviour behind a small interface; a **shallow module** has an interface nearly as complex as its body. Pure factory-function use cases are usually deep; one-line wrappers and `(x) => x` presenters are usually shallow. + +**Seam** _(from Michael Feathers)_: +A place where behaviour can be altered without editing in place — the _location_ of a module's interface. Concrete seams in this repo: `*.interface.ts` files, DI symbols (`*_SYMBOLS.IX*`), `feature.manifest.ts` entries, `// ` anchors, the protocol types in `core-shared/di/bind-protocols.ts`. +_Avoid:_ "boundary" — that term is reserved in this repo for ESLint workspace-tag rules (`feature` may depend on `core` + `tooling` only). + +**Adapter**: +A concrete thing satisfying an interface at a seam. In this repo a typical port has two or three adapters: real (`.repository.ts`), mock (`.repository.mock.ts`), and sometimes recording (`Recording*` test doubles in `core-testing`). + +**Leverage**: +What callers get from depth — more capability per unit of interface they must learn. + +**Locality**: +What maintainers get from depth — change, bugs, knowledge, and verification concentrate in one place rather than spreading across callers. + +**Deletion test**: +Imagine deleting a module: if complexity vanishes, it was a pass-through; if complexity reappears across N callers, it was earning its keep. The signal for "this module is shallow" is when deletion _just moves_ the complexity rather than reducing it. + +**Deepening**: +A refactor that turns shallow modules into deeper ones — merging pass-throughs into the modules that justified them, exposing a smaller interface, or relocating the seam to a more useful place. Subject to the hard constraints in the skill's `SKILL.md` (no ADR violations, no boundary breaks, no manifest drift). + ## Conformance system **Conformance**: @@ -295,7 +331,9 @@ The shipping rhythm. One vertical slice closes one task, becomes one PR, lands a - **"bind"** — DI binding (`bind(SYMBOL).toDynamicValue(...)`); not a `Function.prototype.bind`. - **"spec"** — avoid in this template. Durable design records are **ADRs** (`docs/decisions/adr-NNN-*.md`); implementation seeds are **PRDs** (`docs/work/prds/*.prd.md`). Never use "spec" to refer to a Jest spec file either. - **"controller"** — one verb-noun pair per file; never a multi-method MVC controller. -- **"module"** — avoid for packages (use "package"); reserve for Node ESM/CJS module semantics if needed. +- **"module"** — qualify by context: **package** (workspace member under `packages/` or `apps/`) | **Node ESM/CJS module** (file-level import semantics) | **refactor-vocabulary "module"** (in the `improve-codebase-architecture` skill, defaults to **feature** — i.e., a `packages//`; can also mean a narrower unit like a use case or controller when the refactor scope is that narrow). Default: prefer "feature" for the common case; "package" for the workspace-member shell; "module" only inside refactor conversations. +- **"seam"** — refactor-vocabulary only (where a module's interface lives). Not a synonym for **boundary** (ESLint workspace-tag rule) or **scope** (Conventional Commits parenthetical). +- **"adapter"** — refactor-vocabulary only (a concrete thing satisfying an interface at a seam). Not a synonym for **port** (the interface itself) or **service** (a DI-injected port for non-collection capabilities). - **"coverage"** — qualify when ambiguous: **L0 coverage** (per-layer thresholds, test-time) | **diff coverage / L1** (cover-the-diff gate) | **aggregate coverage / L2** (committed summary.json) | **mutation coverage / L3** (Stryker mutation score, not line coverage). ## Cross-references