diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000000..ea2c451613 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,16 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "node \"$CLAUDE_PROJECT_DIR\"/.claude/skills/pr-self-review/scripts/gate.mjs", + "timeout": 30 + } + ] + } + ] + } +} diff --git a/.claude/skills/README.md b/.claude/skills/README.md index c4b79c5413..9237c7ef72 100644 --- a/.claude/skills/README.md +++ b/.claude/skills/README.md @@ -7,6 +7,7 @@ Reusable AI skills that provide specialized knowledge and workflows. Canonical l | Skill | Scope | Description | |-------|-------|-------------| | [fastify-best-practices](fastify-best-practices/SKILL.md) | Backend | Fastify routes, plugins, JSON-schema validation, error handling | +| [onion-architecture](onion-architecture/SKILL.md) | Backend | Layering for server/: ports & adapters, Container DI, dependency-cruiser gate | | [drizzle-orm-patterns](drizzle-orm-patterns/SKILL.md) | Backend | Drizzle schema, queries, relations, transactions, migrations | | [postgresql-table-design](postgresql-table-design/SKILL.md) | Backend | Postgres schema design, data types, indexing, constraints | | [next-best-practices](next-best-practices/SKILL.md) | Frontend | Next.js App Router, RSC boundaries, data fetching, optimization | @@ -16,6 +17,8 @@ Reusable AI skills that provide specialized knowledge and workflows. Canonical l | [typescript-expert](typescript-expert/SKILL.md) | Full-stack | Type-level programming, performance, tooling, migrations | | [security](security/SKILL.md) | Full-stack | OWASP Top 10:2025, auth, injection, uploads, secrets | | [mermaid-diagram](mermaid-diagram/SKILL.md) | Shared | Mermaid diagrams in markdown (flowcharts, sequence, ERD, …) | +| [engineering-insights](engineering-insights/SKILL.md) | Shared | Append non-obvious lessons to the module's INSIGHTS.md | +| [pr-self-review](pr-self-review/SKILL.md) | Shared | Pre-PR gate: routes local changes to the skills above, verifies criticals, blocks `gh pr create/merge` (v1.0.0) | ## What Are Skills? diff --git a/.claude/skills/engineering-insights/SKILL.md b/.claude/skills/engineering-insights/SKILL.md new file mode 100644 index 0000000000..e18d354066 --- /dev/null +++ b/.claude/skills/engineering-insights/SKILL.md @@ -0,0 +1,15 @@ +--- +name: engineering-insights +description: Records non-obvious engineering lessons into the INSIGHTS.md of the module the work touched (client, server, reviewer-core, e2e; root INSIGHTS.md if cross-package). Use when a fix is confirmed after a surprising cause, an approach turned out to be a dead end, a library/tool behaved unexpectedly, the same error recurred, an undocumented convention was discovered, or a question stayed unresolved — and when the user says "wrap up", "what did we learn", or "/engineering-insights". +--- + +# Engineering insights + +1. File: `/INSIGHTS.md` of the code you changed or debugged; root `INSIGHTS.md` only if the lesson spans packages. Read it first — never duplicate an existing entry. +2. Section: What Works · What Doesn't Work · Codebase Patterns · Tool & Library Notes · Recurring Errors & Fixes · Session Notes · Open Questions. Never skip What Doesn't Work — dead ends are the most valuable. +3. Filter — write only if a future session would act differently because of it (saves 5+ min). Skip: generic knowledge, one-off fixes, anything already in code/README/CLAUDE.md, secrets. Record only after the fix is confirmed; unverified hunches go to Open Questions. +4. One insight per bullet, actionable cold — what to do/avoid, where, why: + ❌ "Promises can be tricky" ✅ "Promise.all() on ingest times out after ~30 items — use Promise.allSettled() in batches of 10" +5. Format: `- YYYY-MM-DD · — ()`. Session Notes: one dated line per session. +6. Append-only: new entries go at the top of their section; never edit or delete old ones. A wrong entry gets a new dated one: "supersedes YYYY-MM-DD …". If the file passes ~100 entries, tell the user it needs a consolidation pass — don't prune yourself. +7. Tell the user in one line what you added and where. diff --git a/.claude/skills/onion-architecture/SKILL.md b/.claude/skills/onion-architecture/SKILL.md new file mode 100644 index 0000000000..8b220ad1a7 --- /dev/null +++ b/.claude/skills/onion-architecture/SKILL.md @@ -0,0 +1,181 @@ +--- +name: onion-architecture +description: "Onion / Clean (ports & adapters) architecture for the backend (server/: Fastify 5 + Drizzle + Postgres/pgvector + zod): which layer owns a piece of code (domain, application service, ports, inbound/outbound adapters, composition root), which direction imports may point, where Fastify / Drizzle / zod / SDK (octokit, openai, anthropic, simple-git) imports are allowed, repositories + mappers, Unit of Work for transactions, domain errors vs HTTP errors, constructor injection via the Container, and the dependency-cruiser gate (pnpm arch:check). Use this skill whenever you add or change a server module, route, service, repository, adapter, job handler or port, decide 'where should this go?', wire a dependency, review a backend PR for structure, or refactor server/ — even if the user only says 'add an endpoint', 'extract this', 'add a table and expose it' or 'is this in the right place?'. Not for Fastify/Drizzle/zod API details or schema design (see fastify-best-practices, drizzle-orm-patterns, zod, postgresql-table-design)." +metadata: + version: "1.0.0" + updated: "2026-10-18" +--- + +# Onion Architecture — backend (`server/`) + +Keep business rules in the middle, frameworks at the edge, and make every +import point **inward**. Then a use case can be read, tested and changed +without Fastify, Postgres or an LLM key, and an adapter can be swapped without +touching a rule. Sources and rationale: [SOURCES.md](SOURCES.md). + +## Scope — what this skill is and isn't + +| Question | Skill | +|----------|-------| +| *Which layer* owns code, *where* the file lives, *which way* it may import, *how* deps are wired | **this skill** | +| Fastify API mechanics (hooks, plugins, schemas, serialization) | `fastify-best-practices` | +| Drizzle query/relations/migration syntax | `drizzle-orm-patterns` | +| Table/index design | `postgresql-table-design` · validation details: `zod` | + +On a *placement/dependency* question this skill wins; on a *how-to-write-it* +question the sibling wins. Repo rules in `CLAUDE.md` files win over both. + +## Before you start + +1. Read `server/CLAUDE.md` and `server/INSIGHTS.md` (root CLAUDE.md requires it). +2. Open 2–3 sibling modules in `server/src/modules/` and match their naming. +3. Run `cd server && pnpm arch:check` before and after your change. + +## Adoption policy (decided 2026-10-18) + +- **New code is strict**: every rule below applies to new modules, new files + and new functions. +- **Legacy is fixed on touch**: when you substantially change a legacy file, + move the part you touch toward the target shape — don't rewrite modules you + are not otherwise changing, and don't refactor "while you're there" beyond + the function you edit. Known legacy violations are baselined + (`server/.dependency-cruiser-known-violations.json`); never re-baseline to + hide a new one. Playbook: [references/migration.md](references/migration.md). +- **Domain layer only where it pays**: a `domain.ts` exists only when the + module has real invariants/state rules (e.g. reviews, repo-intel). CRUD + modules go `routes → service → repository` with no empty domain folder. +- **DI stays our own `Container`** (`src/platform/container.ts`) — no Awilix. +- **Domain errors without HTTP codes** is a rule for new code; existing + `AppError(statusCode)` stays until a module is touched. + +## The rings, mapped to this repo + +``` + ┌──────────── composition root: platform/container.ts, modules//compose.ts + │ ┌───────── adapters: routes.ts (in) · repository.ts, src/adapters/* (out) + │ │ ┌────── application: service.ts + ports.ts + │ │ │ ┌─── domain: domain.ts (optional) + │ │ │ │ + imports point → → → inward only +``` + +| Ring | Contains | Files | May import | +|------|----------|-------|------------| +| **Domain** | Entities/value objects, invariants, state transitions, domain errors (`code`, no HTTP status). Pure TS. | `modules//domain.ts` or `domain/` | own `constants.ts`/`types.ts`; `import type` from `@devdigest/shared` | +| **Application** | Use cases (one public method per user intent), orchestration, transactions via a port, **ports** = interfaces it needs | `service.ts`, `ports.ts`, `helpers.ts` (pure) | domain, own ports, `@devdigest/shared` types, `platform/errors` (legacy) | +| **Inbound adapters** | Fastify routes, job handlers, SSE: parse → call use case → map result/HTTP | `routes.ts` | application, `_shared/context`, zod contracts | +| **Outbound adapters** | Drizzle repositories + mappers; LLM/GitHub/git/embedder clients | `repository.ts`, `repository/*.repo.ts`, `mappers.ts`, `src/adapters/**` | ports they implement, `drizzle-orm`, `src/db/**`, SDKs | +| **Composition root** | Construct concrete objects and inject them | `platform/container.ts`, `modules//compose.ts` | everything | + +Ports used by **more than one package** (LLM, Git, GitHub, Secrets…) stay in +`src/vendor/shared/adapters.ts` (and its client twin). Ports used by one module +live in that module's `ports.ts` — the consumer owns the abstraction. + +## The rules (each one is checked or reviewable) + +1. **Dependencies point inward.** Domain imports nothing outer; services never + import routes, repositories, `src/db`, `drizzle-orm`, `fastify` or SDKs. + *Why:* the core then compiles and tests without infrastructure — the defining + property of onion (Palermo, tenet 4). +2. **Services receive narrow deps through the constructor.** No `Container` + parameter (service locator), no `new XRepository(db)` inside a service. + *Why:* explicit deps document the use case and make fakes trivial. +3. **Only composition code touches the Container**: `modules//compose.ts` + builds the service from `container.*`; `routes.ts` calls the factory. + Shared repositories/facades are exposed as `container.` getters + (existing pattern: `agentsRepo`, `reviewRepo`, `repoIntel`). +4. **Drizzle stays in repositories.** `drizzle-orm`, `src/db/schema`, + `src/db/rows`, `$inferSelect` and `tx` appear only in repository/mapper + files. A repository returns a domain object or a read model declared in + `ports.ts`/`domain.ts` — not a row type. Details: + [references/drizzle.md](references/drizzle.md). +5. **Fastify stays in `routes.ts`** (+ `_shared/context.ts`). No + `FastifyRequest`/`reply` below the route. Details: + [references/fastify.md](references/fastify.md). +6. **Parse at the edge, trust inside.** Routes declare zod + `params`/`body`/`response` (repo rule); services take typed inputs and don't + re-validate wire shapes. Domain never imports runtime zod. Details: + [references/zod-boundaries.md](references/zod-boundaries.md). +7. **Map at every boundary.** Wire DTO → service input → domain → row and back. + The `toXDto` mapper (domain/read model → contract) lives in `helpers.ts`; + row ↔ domain mapping lives next to the repository. +8. **Transactions through a port.** A use case that writes >1 table calls + `uow.run(async (tx) => …)` on a `UnitOfWork` port; the Drizzle adapter wraps + `db.transaction` and hands back tx-bound repositories. Never pass a Drizzle + `tx` into a service. +9. **Domain errors carry a `code`, not a status.** New code throws module + errors (`class RepoAlreadyArchived extends DomainError`); the route (or a + module `toHttpError`) maps them to `AppError` subclasses so the global + `setErrorHandler` renders them. +10. **External systems only via ports.** Modules never import + `src/adapters/**` concretions; they get `GitClient`, `LLMProvider`, … from + the container. Secrets are read by adapters/the container via + `SecretsProvider`, never by a service. Details: + [references/external-adapters.md](references/external-adapters.md). +11. **Modules don't reach into each other.** Never import another module's + repository/service/routes; use its container facade, its `constants.ts` / + `types.ts`, or move the shared piece to `modules/_shared/`. +12. **Behaviour lives with data.** Put invariant checks and state transitions + in `domain.ts` functions/classes, not scattered `if`s in services — but + don't invent a domain model for plain CRUD. + +## Where does X go? + +| You're adding… | Put it in… | +|----------------|-----------| +| A new HTTP endpoint | `modules//routes.ts` (+ register in `modules/index.ts` if new module) | +| Request/response shape | zod contract in `src/vendor/shared/contracts/*` **and** the client copy | +| A use case | a method on `Service` in `service.ts` (split into `.service.ts` only when `service.ts` gets unwieldy) | +| An interface the service needs (store, queue, clock, gateway) | `modules//ports.ts` | +| Business invariant / state transition | `modules//domain.ts` (create it when the first one appears) | +| Pure transform, DTO mapper, parsing helper | `modules//helpers.ts` | +| SQL / Drizzle query | `modules//repository.ts` (or `repository/.repo.ts`) | +| Row ↔ domain mapping | inside the repository file or `modules//mappers.ts` | +| New table | `src/db/schema/*` → `pnpm db:generate` (never hand-edit migrations) | +| A client for a new external system | port in `vendor/shared/adapters.ts` (both copies) or `ports.ts`; impl in `src/adapters//`; wire in `container.ts`; mock in `adapters/mocks.ts` | +| Wiring service + its deps | `modules//compose.ts` | +| Background job handler | registered from `compose.ts`/`routes.ts`; handler body calls a service method | +| Literals / job kinds | `modules//constants.ts` | +| Code used by ≥2 modules | `modules/_shared/` (logic) or a container getter (stateful repo/facade) | + +Full module template, with code: [references/layers-and-placement.md](references/layers-and-placement.md). + +## Module template (new module) + +``` +src/modules// +├── routes.ts # inbound adapter: zod schemas, getContext, call service, status codes +├── compose.ts # createXService(container) — the only non-route file that sees Container +├── service.ts # use cases; constructor(deps: XDeps) +├── ports.ts # XStore, read models, other interfaces the service needs +├── domain.ts # OPTIONAL — invariants, transitions, domain errors +├── repository.ts # implements XStore with Drizzle; maps rows ↔ read models +├── helpers.ts # pure functions, toXDto +└── constants.ts +``` + +Tests: service with in-memory fakes of `ports.ts` (unit), repository against +testcontainers (`*.it.test.ts`), route via `app.inject` + `ContainerOverrides`. +See [references/testing.md](references/testing.md). + +## Enforcement + +`server/.dependency-cruiser.cjs` encodes rules 1, 2, 4, 5, 6 (domain), 10, 11. +`pnpm arch:check` fails on new violations; it runs in CI (`server-unit.yml`, +typecheck job). Rule names, how to read a failure, and when re-baselining is +legitimate: [references/enforcement.md](references/enforcement.md). + +## Review checklist + +- [ ] `pnpm arch:check` is clean (no new violations, baseline not regenerated to hide one). +- [ ] New service takes explicit deps in its constructor; no `Container`, no `new …Repository`. +- [ ] No `drizzle-orm` / `src/db` / row types outside repository & mapper files. +- [ ] No `fastify` types below `routes.ts`; handlers stay thin (parse → call → map). +- [ ] Repositories return read models/domain objects; DTO mapping in `helpers.ts`. +- [ ] Multi-table writes go through a `UnitOfWork` port. +- [ ] New errors carry a `code`; HTTP status assigned at the edge. +- [ ] External calls go through a port; mock added to `adapters/mocks.ts`. +- [ ] No import into another module's internals. +- [ ] `domain.ts` only where invariants exist; no empty scaffolding. +- [ ] Contract change applied to both `vendor/shared` copies. +- [ ] Use case has a unit test with fakes; DB code has an `*.it.test.ts`. diff --git a/.claude/skills/onion-architecture/SOURCES.md b/.claude/skills/onion-architecture/SOURCES.md new file mode 100644 index 0000000000..45ff59c4dc --- /dev/null +++ b/.claude/skills/onion-architecture/SOURCES.md @@ -0,0 +1,92 @@ +# Sources + +All links verified (HTTP 200) on 2026-10-18. Each line: what we took from it. + +## Onion / Clean / Hexagonal — canonical + +- Jeffrey Palermo — The Onion Architecture + [Part 1](https://jeffreypalermo.com/2008/07/the-onion-architecture-part-1/) · + [Part 2](https://jeffreypalermo.com/2008/07/the-onion-architecture-part-2/) · + [Part 3](https://jeffreypalermo.com/2008/08/the-onion-architecture-part-3/) · + [Part 4 — after four years](https://jeffreypalermo.com/2013/08/onion-architecture-part-4-after-four-years/) + — all coupling toward the center; inner layers define interfaces; the core + compiles and runs without infrastructure. +- [Alistair Cockburn — Hexagonal Architecture](https://alistair.cockburn.us/hexagonal-architecture/) + — ports owned by the app; adapters (HTTP, DB, tests) are interchangeable. +- [Robert C. Martin — The Clean Architecture](https://blog.cleancoder.com/uncle-bob/2012/08/13/the-clean-architecture.html) + — the Dependency Rule; simple data crosses boundaries. +- [Herberto Graça — Explicit Architecture](https://herbertograca.com/2017/11/16/explicit-architecture-01-ddd-hexagonal-onion-clean-cqrs-how-i-put-it-all-together/) + — one map of onion + hexagonal + clean + CQRS. +- Microsoft Learn — + [DDD-oriented microservice](https://learn.microsoft.com/en-us/dotnet/architecture/microservices/microservice-ddd-cqrs-patterns/ddd-oriented-microservice) · + [Common web application architectures](https://learn.microsoft.com/en-us/dotnet/architecture/modern-web-apps-azure/common-web-application-architectures) · + [Infrastructure persistence layer](https://learn.microsoft.com/en-us/dotnet/architecture/microservices/microservice-ddd-cqrs-patterns/infrastructure-persistence-layer-design) + — core holds interfaces, infrastructure references core; repository per aggregate; UoW. +- Martin Fowler — + [DIP in the Wild](https://martinfowler.com/articles/dipInTheWild.html) (client owns the abstraction) · + [Repository](https://martinfowler.com/eaaCatalog/repository.html) · + [Unit of Work](https://martinfowler.com/eaaCatalog/unitOfWork.html) · + [Anemic Domain Model](https://martinfowler.com/bliki/AnemicDomainModel.html) +- [Mark Seemann — Composition Root](https://blog.ploeh.dk/2011/07/28/CompositionRoot/) + — compose the object graph in one place near the entry point. + +## TypeScript / Node + +- Khalil Stemmler — + [Organizing App Logic](https://khalilstemmler.com/articles/software-design-architecture/organizing-app-logic/) · + [Clean Node.js Architecture](https://khalilstemmler.com/articles/enterprise-typescript-nodejs/clean-nodejs-architecture/) · + [Repository, DTO, Mapper](https://khalilstemmler.com/articles/typescript-domain-driven-design/repository-dto-mapper/) · + [Entities](https://khalilstemmler.com/articles/typescript-domain-driven-design/entities/) · + [Make illegal states unrepresentable](https://khalilstemmler.com/articles/typescript-domain-driven-design/make-illegal-states-unrepresentable/) +- [Sairyss/domain-driven-hexagon](https://github.com/Sairyss/domain-driven-hexagon) — full TS reference (ports, adapters, mappers, domain errors). +- [marcoturi/fastify-boilerplate](https://github.com/marcoturi/fastify-boilerplate) — Fastify 5 + clean layering + dependency-cruiser in CI. +- [practicajs/practica](https://github.com/practicajs/practica) — entry-points → domain → data-access starter. +- [jbuget/nodejs-clean-architecture-app](https://github.com/jbuget/nodejs-clean-architecture-app) — swappable in-memory/SQL adapters. +- Node.js Best Practices — + [Structure by components](https://github.com/goldbergyoni/nodebestpractices/blob/master/sections/projectstructre/breakintcomponents.md) · + [Layer your components](https://github.com/goldbergyoni/nodebestpractices/blob/master/sections/projectstructre/createlayers.md) + +## Fastify + +- [Encapsulation](https://fastify.dev/docs/latest/Reference/Encapsulation/) · + [Plugins](https://fastify.dev/docs/latest/Reference/Plugins/) · + [Plugins Guide](https://fastify.dev/docs/latest/Guides/Plugins-Guide/) · + [fastify-plugin](https://github.com/fastify/fastify-plugin) · + [Decorators](https://fastify.dev/docs/latest/Reference/Decorators/) +- [Type Providers](https://fastify.dev/docs/latest/Reference/Type-Providers/) · + [fastify-type-provider-zod](https://github.com/turkerdev/fastify-type-provider-zod) +- [Errors](https://fastify.dev/docs/latest/Reference/Errors/) · + [Server (setErrorHandler)](https://fastify.dev/docs/latest/Reference/Server/) · + [Testing (inject)](https://fastify.dev/docs/latest/Guides/Testing/) +- Considered and not chosen (we keep our Container): + [@fastify/awilix](https://github.com/fastify/fastify-awilix) · [Awilix](https://github.com/jeffijoe/awilix) + +## Drizzle + +- [Transactions](https://orm.drizzle.team/docs/transactions) · + [Schema declaration](https://orm.drizzle.team/docs/sql-schema-declaration) · + [Goodies ($inferSelect)](https://orm.drizzle.team/docs/goodies) · + [Vector similarity search](https://orm.drizzle.team/docs/guides/vector-similarity-search) +- [Sentry — Atomic Repositories in Clean Architecture and TypeScript](https://blog.sentry.io/atomic-repositories-in-clean-architecture-and-typescript/) + — transactional repositories without leaking the ORM into use cases. + +## zod / parsing + +- [Alexis King — Parse, don't validate](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) +- [Zod v3 docs](https://v3.zod.dev/) + +## Testing & enforcement + +- [Testcontainers for Node](https://node.testcontainers.org/) · + [PostgreSQL module](https://node.testcontainers.org/modules/postgresql/) · + [nodejs-integration-tests-best-practices](https://github.com/testjavascript/nodejs-integration-tests-best-practices) +- Martin Fowler — + [Mocks Aren't Stubs](https://martinfowler.com/articles/mocksArentStubs.html) · + [TestDouble](https://martinfowler.com/bliki/TestDouble.html) · + [InMemoryTestDatabase](https://martinfowler.com/bliki/InMemoryTestDatabase.html) +- [dependency-cruiser](https://github.com/sverweij/dependency-cruiser) · + [rules reference](https://github.com/sverweij/dependency-cruiser/blob/main/doc/rules-reference.md) · + [rules tutorial](https://github.com/sverweij/dependency-cruiser/blob/main/doc/rules-tutorial.md) +- Alternatives (not used): [eslint-plugin-boundaries](https://github.com/javierbrea/eslint-plugin-boundaries) · + [no-restricted-imports](https://eslint.org/docs/latest/rules/no-restricted-imports) · + [import/no-restricted-paths](https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-restricted-paths.md) diff --git a/.claude/skills/onion-architecture/references/drizzle.md b/.claude/skills/onion-architecture/references/drizzle.md new file mode 100644 index 0000000000..c04802d254 --- /dev/null +++ b/.claude/skills/onion-architecture/references/drizzle.md @@ -0,0 +1,112 @@ +# Drizzle as an outbound adapter + +Drizzle is infrastructure. The schema (`pgTable`), the query builder, inferred +row types and transactions belong to the outer ring. The application sees +**ports** (`NoteStore`, `UnitOfWork`, `SimilaritySearch`) and **read models**. + +## Rules + +1. `drizzle-orm`, `src/db/schema*`, `src/db/rows.ts`, `src/db/client.ts` may be + imported only by repository files (`repository.ts`, `repository/*.repo.ts`, + `mappers.ts`) and by `src/db/**`, `src/platform/**`, `src/adapters/**`. + Enforced by `drizzle-only-in-repositories` / `service-no-infrastructure`. +2. **One repository per aggregate/table cluster**, owned by one module. Other + modules reach it through a `Container` getter, never by import + (`no-cross-module-internals`). +3. **Return read models, not rows.** Define the shape in `ports.ts`; map with + a `toRecord(row)` function in the repository. Row types (`$inferSelect`) are + private to the adapter. + - Legacy: many repositories return `XRow` from `src/db/rows.ts`. When you + touch such a method, return a read model for *new* methods; convert an + existing method only if you are changing its callers anyway. +4. **Queries are workspace-scoped** in the repository (existing tenancy rule) — + the service passes `workspaceId`, the repository enforces it. +5. **No business decisions in SQL-building code** beyond filtering/sorting the + use case asked for. "Can this transition happen?" is a domain question. +6. **Schema changes:** edit `src/db/schema/*`, run `pnpm db:generate`, never + edit `src/db/migrations/**` (root CLAUDE.md). Migrations are not applied on + boot — `pnpm db:migrate`. +7. **Read-side shortcuts are allowed in the adapter**: a repository method may + join several tables and return a purpose-built view model (CQRS-lite) — it + is still behind a port, and the service still doesn't see Drizzle. + +## Unit of Work (transactions) + +The codebase has no transactions yet. When a use case must write several tables +atomically, introduce a port — don't pass `db`/`tx` into the service. + +```ts +// modules//ports.ts +export interface NoteTxRepos { + notes: NoteStore; + audit: AuditStore; +} +export interface UnitOfWork { + run(work: (repos: NoteTxRepos) => Promise): Promise; +} +``` + +```ts +// modules//repository.ts (adapter side) +import type { Db } from '../../db/client.js'; + +/** Db or a transaction handle — repositories accept either. */ +type DbOrTx = Db | Parameters[0]>[0]; + +export class NoteRepository implements NoteStore { + constructor(private readonly db: DbOrTx) {} + // … +} + +export class DrizzleNotesUnitOfWork implements UnitOfWork { + constructor(private readonly db: Db) {} + run(work: (repos: NoteTxRepos) => Promise): Promise { + return this.db.transaction((tx) => + work({ notes: new NoteRepository(tx), audit: new AuditRepository(tx) }), + ); + } +} +``` + +```ts +// service.ts +await this.deps.uow.run(async ({ notes, audit }) => { + const n = await notes.insert({ … }); + await audit.record({ noteId: n.id, action: 'created' }); +}); +``` + +- Throwing inside `work` rolls back (Drizzle rejects the transaction promise). +- Keep external calls (LLM, GitHub, git) **outside** the transaction — never + hold a DB transaction open across a network call. +- If the `DbOrTx` alias is needed by a second module, move it to + `src/db/client.ts` (it's infrastructure, not a module type). +- Pattern source: Sentry, *Atomic Repositories in Clean Architecture and + TypeScript*; Fowler, *Unit of Work*. + +In unit tests the fake UoW just calls `work(fakeRepos)`. + +## pgvector / similarity search + +Vector search is a capability, not a table detail. Put it behind a port: + +```ts +export interface SimilaritySearch { + nearest(workspaceId: string, embedding: number[], k: number): Promise<{ id: string; score: number }[]>; +} +``` + +The adapter owns the operator (`cosineDistance`, `<=>`), index hints and the +`vector` column; the service owns *what* to search for and what to do with hits. +The embedding itself comes from the `Embedder` port (`container.embedder()`), +which may throw when embeddings are disabled — callers degrade gracefully +(existing convention in `container.ts`). + +## Legacy patterns you'll meet (don't copy) + +| Where | Pattern | On touch | +|-------|---------|----------| +| `pulls/routes.ts`, `workspace/routes.ts`, `settings/routes.ts`, `polling/routes.ts` | Drizzle queries in route handlers | move the touched query into a repository method + service call | +| `repos/helpers.ts` `toRepoDto(row: $inferSelect)` | DTO mapper takes a row | introduce a read model; mapper takes it | +| `reviews/service.ts`, `run-executor.ts` → `src/db/rows.ts` | row types in application code | new methods return read models | +| `*/service.ts` `new XRepository(container.db)` | service builds its own repository | inject via constructor from `compose.ts` | diff --git a/.claude/skills/onion-architecture/references/enforcement.md b/.claude/skills/onion-architecture/references/enforcement.md new file mode 100644 index 0000000000..7507031618 --- /dev/null +++ b/.claude/skills/onion-architecture/references/enforcement.md @@ -0,0 +1,61 @@ +# Enforcement — dependency-cruiser + +Rules that live only in prose erode. The layer rules are checked mechanically +with dependency-cruiser (already a server dependency). + +| File | Purpose | +|------|---------| +| `server/.dependency-cruiser.cjs` | the `forbidden` rules | +| `server/.dependency-cruiser-known-violations.json` | baseline of legacy violations (36 on 2026-10-18) | +| `pnpm arch:check` | `depcruise src --config .dependency-cruiser.cjs --ignore-known --output-type err` | +| `pnpm arch:baseline` | `depcruise-baseline src --config .dependency-cruiser.cjs` | +| CI | `.github/workflows/server-unit.yml` → typecheck job → "Architecture boundaries" | + +## Rules + +| Rule | Forbids | Skill rule | +|------|---------|------------| +| `domain-is-pure` | `domain.ts`/`domain/**` → db, drizzle, fastify, zod, SDKs, adapters, platform, other modules (own `constants.ts`/`types.ts` OK) | 1, 6 | +| `domain-contracts-type-only` | domain → runtime import of `@devdigest/shared` (type-only OK) | 6 | +| `ports-are-pure` | `ports.ts` → db, fastify, SDKs, adapters, container | 1 | +| `service-no-infrastructure` | `service.ts`/`*.service.ts` → db, drizzle, fastify, SDKs, `src/adapters` | 1, 4, 10 | +| `service-no-container` | service → `platform/container.ts` (even `import type`) | 2 | +| `service-no-concrete-repository` | service → repository/mapper files | 2, 4 | +| `drizzle-only-in-repositories` | any module file except repository/mappers → drizzle, `src/db` | 4 | +| `fastify-only-in-routes` | any module file except `routes.ts`, `_shared/` → fastify packages | 5 | +| `no-cross-module-internals` | module A → module B's repository/service/routes/compose/run-executor | 11 | +| `adapters-know-no-modules` | `src/adapters/**` → modules, container | 10 | +| `modules-no-concrete-adapters` | module files (except `compose.ts`/`index.ts`) → `src/adapters/**` (mocks OK) | 10 | + +File-role detection is **by file name** (`service.ts`, `routes.ts`, +`repository.ts`, `*.repo.ts`, `mappers.ts`, `ports.ts`, `domain.ts`, +`compose.ts`) — follow the naming in the module template or the rules can't see +your layers. `tsPreCompilationDeps: true` means type-only imports count. + +## Reading a failure + +``` +error service-no-container: src/modules/notes/service.ts → src/platform/container.ts +``` + +Fix the dependency, don't silence it: inject the dep through the constructor +and wire it in `compose.ts`. Each rule's `comment` in the config says what to +do instead. `--output-type text` or `dot` helps to see the graph. + +## Baseline policy + +- `pnpm arch:baseline` is for **after you removed legacy violations** (to shrink + the file) — commit the smaller baseline in the same PR as the fix. +- Never re-baseline to make a new violation pass. A reviewer should reject a PR + where the known-violations file grows. +- Baseline entries contain resolved npm paths (e.g. + `drizzle-orm@0.38.4_postgres@3.4.9`). After bumping such a dependency, old + violations reappear as "new" — re-baseline in the bump PR, verifying the + count did not grow. +- To see everything, including baselined: `pnpm exec depcruise src --config .dependency-cruiser.cjs --output-type err`. + +## Changing the rules + +Tighten freely; loosening a rule needs a reason in the PR description and a +matching edit to `SKILL.md`. Rules reference: dependency-cruiser +`doc/rules-reference.md`. diff --git a/.claude/skills/onion-architecture/references/external-adapters.md b/.claude/skills/onion-architecture/references/external-adapters.md new file mode 100644 index 0000000000..0ea2460783 --- /dev/null +++ b/.claude/skills/onion-architecture/references/external-adapters.md @@ -0,0 +1,61 @@ +# External systems: LLM, GitHub, git, embeddings, code index + +Every external system is reached through a **port** the application owns and +an **adapter** in `src/adapters//` that implements it. This is the +original Hexagonal idea (Cockburn): the app doesn't know whether it talks to +OpenAI, a mock, or a test fake. + +## Where things are today + +| Port (interface) | Declared in | Adapters | Container accessor | +|------------------|-------------|----------|--------------------| +| `LLMProvider` | `vendor/shared/adapters.ts` | `adapters/llm/{openai,anthropic}.ts`, `OpenRouterProvider` (reviewer-core) | `await container.llm(id)` | +| `GitHubClient` | `vendor/shared/adapters.ts` | `adapters/github/octokit.ts` | `await container.github()` | +| `GitClient` | `vendor/shared/adapters.ts` | `adapters/git/simple-git.ts` | `container.git` | +| `CodeIndex` | `vendor/shared/adapters.ts` | `adapters/codeindex/ripgrep.ts` | `container.codeIndex` | +| `Embedder` | `vendor/shared/adapters.ts` | `adapters/embedder/openai.ts` | `await container.embedder()` | +| `SecretsProvider`, `AuthProvider` | `vendor/shared/adapters.ts` | `adapters/secrets/local.ts`, `adapters/auth/local.ts` | `container.secrets`, `container.auth` | +| `DepGraph`, `Tokenizer` | `adapters/depgraph`, `adapters/tokenizer` (legacy: port next to impl) | same files | `container.depgraph`, `container.tokenizer` | + +Mocks for all of them: `src/adapters/mocks.ts`. + +## Rules + +1. **Modules import ports, never concretions** (`modules-no-concrete-adapters`). + Only `platform/container.ts` (and `compose.ts`) construct adapters. +2. **Adapters don't import modules or the Container** (`adapters-know-no-modules`). + If an adapter needs a constant from a module, the constant belongs to the + port's side (vendor/shared or a neutral constants file). Legacy: + `adapters/{astgrep,depgraph}` → `repo-intel/constants.ts`. +3. **Anti-corruption mapping in the adapter.** Map SDK responses (Octokit, + OpenAI) to port types inside the adapter; no SDK types leak into services. + Wrap SDK failures in `ExternalServiceError` (or a port-level error) there. +4. **Secrets only via `SecretsProvider`**, resolved in the container/adapter + (`~/.devdigest/secrets.json`) — never in services, DB, git or `.env` + (root CLAUDE.md). +5. **Pure helpers are not adapters.** Functions like `parseUnifiedDiff`, + `extract` (codeindex) or ast-grep queries that are pure transforms can be + used by modules — but then they belong in a neutral place + (`modules/_shared/` or reviewer-core), not under `src/adapters/`. Legacy + imports of those from modules are baselined; when touching, prefer to call + them through the port or move them. +6. **Ports shared across packages** (server ↔ client ↔ reviewer-core) live in + `vendor/shared/adapters.ts` — both copies. A port used by one module lives + in that module's `ports.ts`. +7. **Review-engine logic belongs in reviewer-core**; `platform/{grounding,prompt,structured}.ts` + are re-export shims (server CLAUDE.md). Never weaken `INJECTION_GUARD` or the + grounding gate. +8. **Resilience at the edge.** Retries/timeouts/backoff (`platform/resilience.ts`, + `p-queue`) wrap adapter calls; the service decides what a failure *means* + (degrade, fail the run, skip). + +## Adding a new external system — steps + +1. Write the port from the use case's point of view (`ports.ts` or vendor/shared ×2). +2. Implement `src/adapters//.ts` with SDK → port mapping. +3. Add a mock to `src/adapters/mocks.ts`. +4. Add a lazy getter + `ContainerOverrides` field in `platform/container.ts`; + resolve any key via `SecretsProvider` there. +5. Inject into the service from `compose.ts`. +6. Unit-test the service with the mock; test the adapter with a recorded or + local fake, never a live paid API in CI. diff --git a/.claude/skills/onion-architecture/references/fastify.md b/.claude/skills/onion-architecture/references/fastify.md new file mode 100644 index 0000000000..f2e2d1f61b --- /dev/null +++ b/.claude/skills/onion-architecture/references/fastify.md @@ -0,0 +1,66 @@ +# Fastify as the inbound adapter + +Fastify is a delivery mechanism. A route translates HTTP into a use-case call +and the result back into HTTP — nothing else. If you deleted Fastify and drove +the service from a CLI or a job, no business rule should be lost. + +## What a handler does (and only this) + +1. Validate input — declaratively, via the route `schema` (`params`, `body`, + `querystring`, `response`) with `fastify-type-provider-zod`. Never + `Schema.parse(req.body)` in the handler (repo rule). +2. Resolve request context — `getContext(app.container, req)` → + `{ workspaceId, userId }`. +3. Call **one** service method with plain typed arguments (not `req`). +4. Map the outcome — status code (`reply.status(201)`), `undefined` → 404, + domain error → `AppError` (module `toHttpError`). +5. Return the DTO (serialized by the `response` schema when declared). + +Smell: a handler longer than ~15 lines, any `if` about business state, any +`container.db`, any loop that persists things → move it into the service. + +## Plugins, encapsulation, registration + +- Each module is one Fastify plugin (`modules//routes.ts`) — encapsulation + gives a natural per-module boundary (Fastify docs: *Encapsulation*). +- Registration is **static** in `modules/index.ts` — do not switch to + `@fastify/autoload` (repo rule; works under tsx, bundler and vitest). +- Build the service **once per plugin** (`const service = createXService(app.container)`), + not per request. +- Cross-cutting HTTP concerns (CORS, helmet, rate limit, error handler, + validator/serializer compilers) are configured in `src/app.ts`, not in modules. + +## DI with the Container (our choice — no Awilix) + +- `app.decorate('container', container)` in `app.ts` makes the composition root + available as `app.container`. +- Only `routes.ts` and `compose.ts` read it. Services get narrow deps. +- Tests swap implementations with `ContainerOverrides` (`buildApp({ overrides })`) + — the reason services must not `new` their own adapters. +- Don't add per-module `app.decorate(...)` for services; keep wiring in one + place (Seemann, *Composition Root*). + +## Errors + +- One global `setErrorHandler` (`src/app.ts`) renders `{ error: { code, message, details } }`: + zod validation → 422, `AppError` → its `statusCode`, else 500. +- New code: domain/application errors carry a `code` only; the route maps them + to `AppError` subclasses (see `layers-and-placement.md` → `http-errors.ts`). + Status codes are an HTTP concern. +- Don't `try/catch` just to log-and-rethrow; let the global handler do it. + +## Jobs and SSE are inbound adapters too + +- `container.jobs.register(KIND, handler)`: the handler parses/casts the payload + and calls a service method — same thinness as a route. Register once from + `routes.ts` / `compose.ts` (existing pattern in `repos/routes.ts`). +- SSE (`fastify-sse-v2`, `runBus`): the route subscribes and streams; the + service publishes domain events through a port (e.g. `RunEvents.publish`), + not by importing the SSE plugin. + +## Testing the adapter + +`app.inject({ method, url, payload })` exercises routing, validation, +serialization and error mapping without a port (Fastify docs: *Testing*). +Use mock adapters from `src/adapters/mocks.ts` via `ContainerOverrides`. +Business branches are tested at the service level, not through HTTP. diff --git a/.claude/skills/onion-architecture/references/layers-and-placement.md b/.claude/skills/onion-architecture/references/layers-and-placement.md new file mode 100644 index 0000000000..248f86ce40 --- /dev/null +++ b/.claude/skills/onion-architecture/references/layers-and-placement.md @@ -0,0 +1,274 @@ +# Layers & placement — a full module, end to end + +Worked example: a new `notes` module (CRUD + one real invariant), written the +target way. Use it as the template; copy the *shape*, not the names. + +## File by file + +### `ports.ts` — what the service needs (owned by the application) + +```ts +import type { NoteStatus } from './domain.js'; + +/** Read model the service works with — NOT a Drizzle row. */ +export interface NoteRecord { + id: string; + workspaceId: string; + body: string; + status: NoteStatus; + createdAt: Date; +} + +export interface NoteStore { + get(workspaceId: string, id: string): Promise; + list(workspaceId: string): Promise; + insert(note: Omit): Promise; + updateStatus(workspaceId: string, id: string, status: NoteStatus): Promise; +} + +/** Narrow slice of JobRunner the service actually uses. */ +export interface JobQueue { + enqueue(workspaceId: string, kind: string, payload: unknown): Promise; +} +``` + +Rule of thumb: a port is shaped by **what the use case needs**, not by what the +table or SDK offers (Fowler, *DIP in the Wild*). Don't mirror every column or +every SDK method. + +### `domain.ts` — only because there IS an invariant + +```ts +export type NoteStatus = 'draft' | 'published' | 'archived'; + +export class DomainError extends Error { + constructor(readonly code: string, message: string) { + super(message); + this.name = new.target.name; + } +} +export class NoteTransitionError extends DomainError { + constructor(from: NoteStatus, to: NoteStatus) { + super('note_invalid_transition', `Cannot move a note from ${from} to ${to}`); + } +} + +const ALLOWED: Record = { + draft: ['published', 'archived'], + published: ['archived'], + archived: [], +}; + +/** The rule lives here, once — services call it, they don't re-implement it. */ +export function transition(from: NoteStatus, to: NoteStatus): NoteStatus { + if (!ALLOWED[from].includes(to)) throw new NoteTransitionError(from, to); + return to; +} +``` + +No zod, no Drizzle, no `AppError` (it carries an HTTP status), no platform +imports. If a second module needs `DomainError`, promote it to +`modules/_shared/domain-error.ts` — still framework-free. + +A CRUD-only module would **not** have this file. + +### `service.ts` — use cases, explicit deps + +```ts +import type { Note } from '@devdigest/shared'; +import type { NoteStore, JobQueue } from './ports.js'; +import { transition, type NoteStatus } from './domain.js'; +import { toNoteDto } from './helpers.js'; +import { NOTE_PUBLISHED_JOB } from './constants.js'; + +export interface NotesDeps { + store: NoteStore; + jobs: JobQueue; +} + +export class NotesService { + constructor(private readonly deps: NotesDeps) {} + + async list(workspaceId: string): Promise { + return (await this.deps.store.list(workspaceId)).map(toNoteDto); + } + + async create(workspaceId: string, body: string): Promise { + const note = await this.deps.store.insert({ workspaceId, body, status: 'draft' }); + return toNoteDto(note); + } + + /** Returns undefined when the note does not exist — the route maps that to 404. */ + async changeStatus(workspaceId: string, id: string, to: NoteStatus): Promise { + const note = await this.deps.store.get(workspaceId, id); + if (!note) return undefined; + const status = transition(note.status, to); // throws a DomainError + await this.deps.store.updateStatus(workspaceId, id, status); + if (status === 'published') { + await this.deps.jobs.enqueue(workspaceId, NOTE_PUBLISHED_JOB, { id }); + } + return toNoteDto({ ...note, status }); + } +} +``` + +Note what is absent: `Container`, `db`, `drizzle-orm`, `fastify`, +`new NoteRepository(...)`, secrets. + +Not-found handling: either return `undefined` (as above) and let the route +decide, or throw a domain `NotFound`-style error with a `code`. Legacy services +throw `NotFoundError` from `platform/errors` — acceptable when editing them, not +for new modules. + +### `helpers.ts` — pure mapping + +```ts +import type { Note } from '@devdigest/shared'; +import type { NoteRecord } from './ports.js'; + +export function toNoteDto(n: NoteRecord): Note { + return { + id: n.id, + workspace_id: n.workspaceId, + body: n.body, + status: n.status, + created_at: n.createdAt.toISOString(), + }; +} +``` + +Takes the **read model**, not `typeof t.notes.$inferSelect` (that is the legacy +shape — see `repos/helpers.ts`). + +### `repository.ts` — outbound adapter + +```ts +import { and, eq } from 'drizzle-orm'; +import type { Db } from '../../db/client.js'; +import * as t from '../../db/schema.js'; +import type { NoteRecord, NoteStore } from './ports.js'; +import type { NoteStatus } from './domain.js'; + +type NoteRow = typeof t.notes.$inferSelect; + +const toRecord = (r: NoteRow): NoteRecord => ({ + id: r.id, + workspaceId: r.workspaceId, + body: r.body, + status: r.status as NoteStatus, + createdAt: r.createdAt, +}); + +export class NoteRepository implements NoteStore { + constructor(private readonly db: Db) {} + + async get(workspaceId: string, id: string) { + const [row] = await this.db + .select() + .from(t.notes) + .where(and(eq(t.notes.workspaceId, workspaceId), eq(t.notes.id, id))); + return row && toRecord(row); + } + // list / insert / updateStatus … +} +``` + +`implements NoteStore` makes the compiler check the adapter against the port. +Every query stays workspace-scoped (tenancy guard, existing convention). + +### `compose.ts` — module composition root + +```ts +import type { Container } from '../../platform/container.js'; +import { NotesService } from './service.js'; +import { NoteRepository } from './repository.js'; + +export function createNotesService(c: Container): NotesService { + return new NotesService({ + store: new NoteRepository(c.db), + jobs: c.jobs, + }); +} +``` + +If another module needs the store, promote it to a lazy getter on `Container` +(as `agentsRepo` / `reviewRepo` already are) instead of importing this file +from the other module. + +### `routes.ts` — inbound adapter + +```ts +import type { FastifyInstance } from 'fastify'; +import type { ZodTypeProvider } from 'fastify-type-provider-zod'; +import { NoteInput, NoteStatusInput } from '@devdigest/shared'; +import { getContext } from '../_shared/context.js'; +import { IdParams } from '../_shared/schemas.js'; +import { NotFoundError } from '../../platform/errors.js'; +import { createNotesService } from './compose.js'; +import { toHttpError } from './http-errors.js'; + +export default async function notesRoutes(appBase: FastifyInstance) { + const app = appBase.withTypeProvider(); + const service = createNotesService(app.container); + + app.post('/notes', { schema: { body: NoteInput } }, async (req, reply) => { + const { workspaceId } = await getContext(app.container, req); + reply.status(201); + return service.create(workspaceId, req.body.body); + }); + + app.post( + '/notes/:id/status', + { schema: { params: IdParams, body: NoteStatusInput } }, + async (req) => { + const { workspaceId } = await getContext(app.container, req); + try { + const note = await service.changeStatus(workspaceId, req.params.id, req.body.status); + if (!note) throw new NotFoundError('Note not found'); + return note; + } catch (err) { + throw toHttpError(err); + } + }, + ); +} +``` + +`http-errors.ts` (module-local, inbound side): + +```ts +import { AppError } from '../../platform/errors.js'; +import { DomainError } from './domain.js'; + +const STATUS: Record = { note_invalid_transition: 409 }; + +export function toHttpError(err: unknown): unknown { + if (err instanceof DomainError) { + return new AppError(err.code, err.message, STATUS[err.code] ?? 400); + } + return err; +} +``` + +Then register the plugin in `modules/index.ts` (static registration — repo +rule). + +## Dependency graph of the module + +``` +routes.ts ──► compose.ts ──► repository.ts ──► drizzle-orm, src/db + │ │ │ + │ ▼ ▼ implements + └──────► service.ts ──────► ports.ts ◄── domain.ts (types) + │ + └──────► domain.ts, helpers.ts +``` + +Nothing points from `service.ts`/`domain.ts` back out. + +## Small-module shortcut + +For a tiny read-only module (one or two queries, no rules) it is fine to have +`routes.ts → service.ts → repository.ts` with the store interface declared at +the top of `service.ts` instead of a separate `ports.ts`. What is *not* fine, +even for small modules: Drizzle in `routes.ts`, or the service taking `Container`. diff --git a/.claude/skills/onion-architecture/references/migration.md b/.claude/skills/onion-architecture/references/migration.md new file mode 100644 index 0000000000..1c1df296f1 --- /dev/null +++ b/.claude/skills/onion-architecture/references/migration.md @@ -0,0 +1,51 @@ +# Migrating legacy code — "fix on touch" + +Policy (2026-10-18): new code strict; legacy moves toward the target **only in +code you are already changing**. No big-bang rewrites, no drive-by refactors. +Each step should keep tests green and shrink the baseline a little. + +## Known legacy shapes (from the 2026-10-18 baseline) + +| Smell | Where | Target | +|-------|-------|--------| +| Service takes `Container` (service locator) | `agents`, `repos`, `reviews`, `repo-intel` services | `constructor(deps: XDeps)` + `compose.ts` | +| Service does `new XRepository(container.db)` | same | injected `XStore` port | +| Drizzle in routes | `pulls`, `workspace`, `settings`, `polling` routes; `settings/feature-models.ts` | repository method + service | +| Row types in application code | `reviews/service.ts`, `run-executor.ts`, `diff-loader.ts`, `repos/helpers.ts` | read models in `ports.ts` | +| Modules import concrete adapters / pure helpers from `src/adapters` | `reviews/diff-loader.ts` (`diff-parser`), `repo-intel/**` (`extract`, `astgrep`, `tokenizer`) | port via container, or move the pure helper out of `adapters/` | +| Adapters import a module | `adapters/{astgrep,depgraph}` → `repo-intel/constants.ts` | move constants next to the port | +| `AppError` with HTTP status thrown from services | everywhere | domain error with `code`; map at the route | + +## Recipes + +**Service locator → explicit deps** (smallest useful step) + +1. Add `export interface XDeps { … }` listing only what the service actually + uses (`store`, `jobs`, `git`, `secrets`…). +2. Change the constructor to `constructor(private deps: XDeps)`; replace + `this.container.jobs` with `this.deps.jobs`, etc. +3. Create `compose.ts` with `createXService(c: Container)`; update `routes.ts` + (and the container getter, if the service is exposed there). +4. Declare the store interface in `ports.ts`, `implements` it in the repository. +5. `pnpm arch:check`, tests, then `pnpm arch:baseline` to drop the fixed entries. + +**Drizzle in a route → repository** + +1. Move the query verbatim into a repository method (create `repository.ts` if + the module has none), scoped by `workspaceId`. +2. Add a service method the route calls; the route keeps only parse → call → map. +3. Return a read model; map to DTO in `helpers.ts`. + +**Row type leaking → read model** + +1. Define `XRecord` in `ports.ts` with the fields the caller uses. +2. Add `toRecord(row)` in the repository; return it from the touched method. +3. Change the DTO mapper to take `XRecord`. + +## Don'ts + +- Don't convert a module you're not otherwise changing. +- Don't add `domain.ts` to CRUD modules during migration. +- Don't change public HTTP contracts as part of an architectural refactor — + keep the wire shape identical and prove it with existing route tests. +- Don't regenerate the baseline unless it got **smaller**. diff --git a/.claude/skills/onion-architecture/references/testing.md b/.claude/skills/onion-architecture/references/testing.md new file mode 100644 index 0000000000..f133aa5660 --- /dev/null +++ b/.claude/skills/onion-architecture/references/testing.md @@ -0,0 +1,51 @@ +# Testing by ring + +Onion makes each ring testable with the cheapest tool that can break it. +`TESTING.md` (root) is the source of truth for suites, naming and CI; this file +only says which ring gets which test. + +| Ring | Test type | Tools | File | +|------|-----------|-------|------| +| Domain | unit, pure | vitest, no setup | `test/-domain.test.ts` | +| Application (service) | unit with **fakes of ports** | vitest, hand-written in-memory fakes or `adapters/mocks.ts` | `test/-service.test.ts` | +| Outbound adapter (repository) | integration, real Postgres + pgvector | testcontainers (`test/helpers/pg.ts`) | **must** be `*.it.test.ts` | +| Inbound adapter (routes) | HTTP-level | `app.inject` + `ContainerOverrides` | smoke in unit lane; DB-backed in `*.it.test.ts` | +| Whole flow | e2e | agent-browser (`e2e/`) | — | + +## Fakes over mocks for ports + +A fake is a tiny working implementation (Fowler, *Mocks Aren't Stubs*): + +```ts +class InMemoryNoteStore implements NoteStore { + private notes = new Map(); + async get(ws: string, id: string) { + const n = this.notes.get(id); + return n?.workspaceId === ws ? n : undefined; + } + async insert(n: Omit) { + const rec = { ...n, id: crypto.randomUUID(), createdAt: new Date() }; + this.notes.set(rec.id, rec); + return rec; + } + // … +} + +const jobs = { enqueue: vi.fn(async () => ({})) }; +const service = new NotesService({ store: new InMemoryNoteStore(), jobs }); +``` + +Assert on **outcomes** (returned DTO, store state, enqueued job), not on call +order. Because `implements NoteStore` is type-checked, a fake can't silently +drift from the port. + +## Rules + +- A service test must not need Docker, network or keys. If it does, a port is + missing. +- Repository tests prove the SQL, mapping and tenancy scoping against a real + DB — don't fake Drizzle. +- Don't re-test business branches through HTTP; route tests cover status + codes, validation (422), serialization and error mapping. +- The fake UoW runs `work(fakeRepos)` directly; rollback is covered by one + `*.it.test.ts` against real Postgres. diff --git a/.claude/skills/onion-architecture/references/zod-boundaries.md b/.claude/skills/onion-architecture/references/zod-boundaries.md new file mode 100644 index 0000000000..0b8f0a5190 --- /dev/null +++ b/.claude/skills/onion-architecture/references/zod-boundaries.md @@ -0,0 +1,35 @@ +# zod at the boundaries + +"Parse, don't validate" (Alexis King): turn untrusted input into a precise type +**once, at the edge**; everything inside trusts the type and never re-checks. + +## Three kinds of shapes — keep them apart + +| Shape | Lives in | Owned by | zod? | +|-------|----------|----------|------| +| **Wire contract** (request/response DTO, snake_case) | `src/vendor/shared/contracts/*` — **two copies** (server + client), change both | API ↔ client agreement | yes — schemas + `z.infer` types | +| **Read model / domain type** (camelCase, `Date`, unions) | `ports.ts` / `domain.ts` | the module | no — plain TS types | +| **Row** (`$inferSelect`) | private to the repository | Drizzle | n/a | + +Mapping: contract → service input (route) · read model ↔ row (repository) · +read model → contract (`toXDto` in `helpers.ts`). + +## Rules + +1. **Routes declare schemas**; the type provider parses before the handler + runs. No `parse` inside handlers (repo rule). +2. **Services accept typed arguments**, not `unknown`. They don't call + `Contract.parse` on data a route already parsed. +3. **Domain never imports runtime zod or runtime contracts** — + `domain-is-pure` / `domain-contracts-type-only`. `import type { Provider } from '@devdigest/shared'` + is fine for shared enums/unions. +4. **Parse every other untrusted edge too**, in the adapter that receives it: + LLM structured output (reviewer-core already does this), GitHub/SDK payloads + you depend on, job payloads read back from the DB, jsonb columns + (default missing fields on read — see `server/INSIGHTS.md`, `getRunTrace`). +5. **Don't make the contract your domain model.** When the wire shape and the + business shape diverge (naming, optionality, `string` dates vs `Date`), map; + don't bend the domain to snake_case. +6. **Value objects** that need validation (e.g. a repo URL) get a factory in + `domain.ts`/`helpers.ts` (`parseRepoUrl` → typed result or domain error) — + plain TS, optionally a branded type. zod stays at the edge. diff --git a/.claude/skills/pr-self-review/README.md b/.claude/skills/pr-self-review/README.md new file mode 100644 index 0000000000..417f0f8bac --- /dev/null +++ b/.claude/skills/pr-self-review/README.md @@ -0,0 +1,164 @@ +# pr-self-review + +**Version:** 1.0.0 · **Updated:** 2026-10-18 · **Scope:** whole repo (client, server, reviewer-core, e2e) + +A local review gate that runs before a pull request exists. It reviews every +local change, applies each project skill only to the files that skill owns, +verifies critical findings, and blocks `gh pr create|ready|merge` while any +critical remains. Run it manually with `/pr-self-review`. Claude also triggers +it before opening a PR, and the hook forces it when it hasn't run. + +Claude reads `SKILL.md`. This README is the design (the plan) for humans. + +## Contents + +| File | Purpose | Loaded by Claude | +|---|---|---| +| [SKILL.md](SKILL.md) | Orchestration: steps, flags, how to talk about the verdict | when the skill triggers | +| [routing.json](routing.json) | Glob → skill routes, excludes, precedence, scope defaults | read by scripts | +| [references/severity.md](references/severity.md) | Shared severity scale + mapping of every skill's own scale | by each reviewer | +| [references/reviewer-prompt.md](references/reviewer-prompt.md) | Prompt template, one subagent per (skill, file group) | when spawning reviewers | +| [references/verifier-prompt.md](references/verifier-prompt.md) | Adversarial prompt that tries to refute a critical | when spawning verifiers | +| [references/guard-integrity.md](references/guard-integrity.md) | Built-in reviewer for INJECTION_GUARD + grounding gate | as a reviewer "skill" | +| [suppressions.json](suppressions.json) | Repo-wide false-positive suppressions (non-critical only) | read by scripts | +| `scripts/prepare.mjs` | Collect changes, fingerprint, route, chunk, cache lookup | step 1 | +| `scripts/checks.mjs` | Project rules + typecheck / arch:check (/ tests) | step 2 | +| `scripts/finalize.mjs` | Grounding, new-only filter, verification, dedup, waivers, cache, verdict, report | steps 4–5 | +| `scripts/gate.mjs` | PreToolUse hook / `--cli` gate, reads the verdict only | hook | +| `scripts/post-status.mjs` | Optional GitHub commit status `devdigest/self-review` | `--post-status` | +| `scripts/selftest.mjs` | Regression test of all deterministic behaviour (no LLM) | maintenance | + +## Pipeline + +``` +prepare ──► checks ──► reviewers (parallel, 1 per skill-group) ──► finalize --stage candidates + │ + verifiers (parallel, 1 per critical) ◄──┘ + │ + finalize ──► verdict ──► gate hook +``` + +1. **Collect.** Diff against `merge-base(origin/main, HEAD)`, covering + committed, staged, unstaged and untracked files (respecting `.gitignore`). + Renames count as delete + add. +2. **Fingerprint.** A hash of the merge-base plus `(path, content hash)` for + every changed file. Committing reviewed work keeps it. Any edit or a new + base invalidates it, so the gate can never accept a stale review. +3. **Route.** Each reviewable file is matched against `routing.json`. + - Client files go to the frontend skills, server files to onion / fastify / + drizzle / postgres, contracts to zod. + - security covers server, reviewer-core, the API gateway, workflows and + `package.json`. + - reviewer-core's guard files also go to the built-in `@guard-integrity` + reviewer. + - Code that no route claims falls back to `typescript-expert`. + - Lockfiles, generated migrations and binaries are excluded from LLM + review (migrations still have a deterministic rule). + - Skills that exist in `.claude/skills/` but aren't routed produce a + warning. If the catalog gives them a scope, they get that scope's + default globs, so a new skill is never silently ignored. +4. **Deterministic checks.** Rules from root `CLAUDE.md`, enforced as + critical with no LLM involved: + - migration changed without a schema change; + - contract changed in only one of the two `vendor/shared` copies; + - key-shaped secret or a `.env` added; + - `INJECTION_GUARD` usage removed. + + Plus `typecheck` for every touched package and `arch:check` for server. + `--with-tests` adds unit tests. +5. **Review.** One clean-context subagent per (skill, ≤40-file group). It reads + only its own skill, its own diff and the module's `CLAUDE.md` / + `INSIGHTS.md`, and writes JSON findings. +6. **Ground + new-only.** See improvements 1 and 2 below. +7. **Verify.** Each fresh critical gets an adversarial verifier that tries + to refute it. Refuted → major. Unverified → stays critical (fail-safe). +8. **Verdict.** `block` if any unwaived critical, any reviewer without output, + or checks didn't run. Otherwise `pass`. `--quick` gives `quick-pass`, which + never unlocks the gate. + +## Severity + +One scale: critical / major / minor / nit, plus `info` for pre-existing issues. +**Critical means introduced by this diff** and either a demonstrable bug, a +traced vulnerability, a breach of a root-`CLAUDE.md` rule, or a new +dependency-direction violation. The full mapping of each skill's own scale is +in [references/severity.md](references/severity.md). + +## Improvements built in (beyond the first plan) + +1. **Only new problems block.** A finding on a line the diff didn't change is + downgraded to `info` ("pre-existing in a touched file"). Without this, every + PR touching an old file would be blocked by legacy issues, and people + would stop using the gate. +2. **Grounding.** Every finding must quote the code verbatim (`evidence`). + `finalize` searches the current file, or the removed side of the diff for + `"side":"removed"`. A finding whose quote isn't there is dropped and listed + under *Filtered out*. One that points at the wrong line is re-anchored to + the real one. This is the same idea as reviewer-core's grounding gate, + applied to our own reviewers. +3. **Incremental re-runs.** Results are cached per + `(skill, skill-file hash + prompt/severity hash, path, file diff hash)` in + `.git/devdigest-self-review/cache/`. After fixing one critical, only the + edited files are reviewed again. Editing a skill or the reviewer prompt + invalidates its cache automatically. +4. **Skill precedence.** `routing.json → precedence` decides which finding is + primary when several skills report the same `file:line`. The others are + kept as "also reported by". It also settles known conflicts, e.g. + `react-best-practices` vs `frontend-ui-architecture` + (`docs/improvement-plan.md` Q12). +5. **Suppressions for false positives.** Two ways, for non-critical findings + only: + - inline `// self-review-ignore ` on the line or up to two + lines above; + - `suppressions.json` entries with a reason and expiry. + + Criticals can only be waived per run. +6. **PR body section.** `pr-body-section.md` records the verdict, reviewers, + checks, waived criticals and open majors. + +## Decisions taken (defaults — change here if the team disagrees) + +| Question | Decision | +|---|---| +| Escape hatch for a critical? | Yes. `--waive ="reason"` (≥10 chars), and only with the reason the user gave themselves. It is recorded in the verdict and the PR body. Claude never waives on its own. | +| Publish to GitHub? | Opt-in `--post-status`. It sets commit status `devdigest/self-review` on HEAD and refuses if the working tree is dirty. To make it actually block merges, add it as a required check in `main` branch protection. | +| Run tests? | Off by default (typecheck + arch:check only). `--with-tests` adds unit tests. | +| Checklist items | major by default. critical only for dependency direction, contracts and project rules. | +| `git push` | Not gated. Pushing WIP is fine; opening, readying or merging a PR is what's gated. | + +## Enforcement layers + +1. **Claude Code hook** (enabled): `.claude/settings.json` → PreToolUse on Bash + → `scripts/gate.mjs`. + - It reacts only to `gh pr create|ready|merge`, honours `--base`/`-B`, and + exits 2 with the reason. + - It costs one `git diff --name-status` + file hashes, with no LLM. + - `gh pr merge ` is judged by the *current branch's* verdict, so merging + someone else's PR from your branch needs layer 3. +2. **git pre-push** (opt-in, works outside Claude): + ```bash + mkdir -p .githooks && printf '#!/bin/sh\nexec node .claude/skills/pr-self-review/scripts/gate.mjs --cli\n' > .githooks/pre-push + chmod +x .githooks/pre-push && git config core.hooksPath .githooks + ``` + This blocks every push without a passing review, which is stricter than + layer 1, so enable it only if the team wants that. +3. **GitHub required check** (opt-in): `--post-status` + branch protection. + The status is set from a developer machine, so it rests on trust. The + fully enforced version is running this review in CI, a natural fit for the + future agent-runner lesson. + +## Maintaining + +- **New skill in `.claude/skills/`:** add a route to `routing.json` or list it + in `notReviewers`. Until then `prepare` warns on every run. +- **Changed scripts:** run `node .claude/skills/pr-self-review/scripts/selftest.mjs`. + It covers 31 scenarios: routing, gate, grounding, cache, waivers and project + rules. +- **State** lives in `.git/devdigest-self-review/{runs,verdicts,cache}`. It is + never committed and safe to delete. + +## Later (not built yet) + +`--fix` mode for major/minor findings, a "new use case without a test" check, +per-skill noise metrics (findings vs. suppressions), and replacing the +subagents with DevDigest's own `reviewer-core` once the agent-runner exists. diff --git a/.claude/skills/pr-self-review/SKILL.md b/.claude/skills/pr-self-review/SKILL.md new file mode 100644 index 0000000000..ae4bbc0783 --- /dev/null +++ b/.claude/skills/pr-self-review/SKILL.md @@ -0,0 +1,158 @@ +--- +name: pr-self-review +description: "Local pre-PR gate for DevDigest: reviews ALL local changes (unmerged commits + staged + unstaged + untracked) by routing each changed file to the project skills that own it (frontend skills on client/, onion-architecture/fastify/drizzle on server/, zod/security/typescript where they apply), runs typecheck + arch:check + CLAUDE.md project rules, verifies every critical finding, and BLOCKS opening or merging the PR while any critical remains. Use this skill before every `gh pr create` / `gh pr ready` / `gh pr merge`, whenever the user says 'open a PR', 'create a pull request', 'ready for review', 'self-review', 'review my changes', 'check my diff before PR', 'перевір зміни', 'відкрий PR', 'чи можна мержити', or runs /pr-self-review — and whenever the PreToolUse gate blocked a PR command." +disable-model-invocation: true +argument-hint: "[--base ] [--quick] [--with-tests] [--no-cache] [--waive =\"reason\"] [--post-status]" +metadata: + version: "1.0.0" + updated: "2026-10-18" +--- + +# PR Self-Review + +A review gate that runs **before** a pull request exists. It looks at everything +that would go into the PR, applies each project skill only where it is the +right lens, and refuses to let a PR open while a verified critical finding +remains. Design and rationale: [README.md](README.md). + +The scripts do the deterministic parts: diff collection, routing, grounding, +caching and the verdict. You orchestrate the reviewers and talk to the user. +Run every command from the repo root. `S=.claude/skills/pr-self-review/scripts`. + +## Arguments + +| Flag | Effect | +|---|---| +| `--base ` | PR target (default `origin/main`). Use the same base as `gh pr create --base`. | +| `--quick` | Deterministic checks only. Fast, but its verdict does **not** unlock the PR. | +| `--with-tests` | Also run unit tests of touched packages. | +| `--no-cache` | Re-review every file, ignoring cached per-file results. | +| `--waive ="reason"` | Waive one critical. **Only when the user gave this id and reason themselves.** | +| `--post-status` | After the verdict, publish it as GitHub commit status `devdigest/self-review`. | + +## Workflow + +### 1. Collect and route + +```bash +node $S/prepare.mjs [--base ] [--no-cache] +``` + +This prints the fingerprint, a REVIEW or cached line per reviewer group, any +files no skill covers, warnings, and `RUN_DIR=…`. Read +`$RUN_DIR/manifest.json` for the groups. + +- `NO_CHANGES` → tell the user there is nothing to review and stop. +- Pass on every `WARN` line to the user. A skill that is not routed, or a route + to a missing skill, means `routing.json` needs an update. + +### 2. Deterministic checks + +```bash +node $S/checks.mjs --run $RUN_DIR [--with-tests] +``` + +Runs project rules (migrations, contract twins, secrets, INJECTION_GUARD) and +typecheck/arch:check for touched packages. A failure becomes a critical with no +LLM involved. If `--quick`, jump to step 5 with `--quick`. + +### 3. Reviewers, all in parallel + +For each group in `manifest.groups` with a non-empty `files` array, build a +prompt from [references/reviewer-prompt.md](references/reviewer-prompt.md): + +- `{{skillPath}}`, `{{diffPath}}`, `{{outputPath}}`, `{{groupId}}`, `{{skill}}`: from the group; +- `{{severityPath}}`: `.claude/skills/pr-self-review/references/severity.md`; +- `{{moduleDocs}}`: the group's `moduleDocs`; +- `{{base}}`: the manifest base; +- `{{fileList}}`: one line per file, as `path — changed lines `. + +Spawn **all** of them in a single message with the Agent tool +(`subagent_type: "general-purpose"`, description `review:`). Each one +works in a clean context with one skill and only its own files. That is what +keeps frontend rules off backend files and stops one skill's rules from +leaking into another's findings. Groups with only `cachedFiles` are not run; +their earlier results are replayed. + +When an agent finishes without writing its output file, re-run that single +group once. If it fails again, leave it: finalize marks the review +*incomplete*, which blocks. A skipped reviewer must never turn into a silent +pass. + +### 4. Verify criticals + +```bash +node $S/finalize.mjs --run $RUN_DIR --stage candidates +``` + +This grounds every finding first. A finding whose quote does not exist in the +code is dropped, and one on an unchanged line becomes `info`. Then it lists the +fresh critical candidates with their ids and `verify output` paths. For each +candidate, spawn one verifier from +[references/verifier-prompt.md](references/verifier-prompt.md), all in one +message. Fill the placeholders like this: + +- `{{findingJson}}`, `{{id}}`, `{{file}}`: the candidate's object and fields + in `$RUN_DIR/candidates.json`; +- `{{mergeBase}}`: from the manifest; +- `{{skillPath}}`: from the group whose `skill` equals the candidate's `source`; +- `{{verifyPath}}`: the printed `verify output` path. + +The verifier's job is to *refute*. A refuted critical becomes major. A +critical that was never verified stays critical (fail-safe). Skip this step +when there are 0 candidates. + +### 5. Verdict + +```bash +node $S/finalize.mjs --run $RUN_DIR [--quick] [--waive ="reason" ...] +``` + +This prints the report and `VERDICT=PASS|BLOCK|QUICK-PASS`, and writes the +verdict the gate reads, the per-file cache, `report.md` and +`pr-body-section.md`. + +### 6. Tell the user + +Lead with the verdict. Then: +- list every critical with its id, `file:line`, why and fix; +- give majors as a short list; +- give counts for the rest, and mention anything dropped, uncovered or + incomplete. + +Don't paste the whole report when it is long. Point to `$RUN_DIR/report.md`. + +- **BLOCK:** say plainly that the PR cannot be opened or merged until the + criticals are fixed, then offer to fix them. After fixes, re-run from step + 1. Unchanged files come from the cache, so only touched groups run again. +- **Waivers are the user's call, not yours.** Never add `--waive` on your own + initiative or to get past the gate. If the user wants to accept a critical, + ask them for the reason and pass exactly that. It ends up in the verdict and + in `pr-body-section.md`. +- **PASS**, and the user asked to open a PR: append + `$RUN_DIR/pr-body-section.md` to the PR body, then run `gh pr create`. The + gate hook lets it through only for the exact fingerprint that passed. Any + later edit needs a new review. +- `--post-status`: confirm with the user first (it publishes to GitHub). + Then commit and push, and run `node $S/post-status.mjs [--base …]`. + +## The gate + +`.claude/settings.json` registers `node $S/gate.mjs` as a PreToolUse hook on +Bash. It blocks `gh pr create|ready|merge` unless the current fingerprint has a +full `pass` verdict. When it blocks, run this skill. Don't work around the +hook: no rewording the command, no other tool to create the PR. For pushes +outside Claude Code, `node $S/gate.mjs --cli` works as a git `pre-push` hook. +The README explains how to enable it. + +## Gotchas + +- The fingerprint covers the content of every changed file against the + merge-base. Committing reviewed changes keeps it valid. Editing a file, + rebasing onto a new base, or changing `--base` invalidates it. +- The cache key includes the skill file and the prompt/severity references. + Editing a skill re-reviews its files automatically. +- `next-best-practices` is `user-invocable: false`, which doesn't matter here: + reviewers read the SKILL.md file directly. +- Routing lives in [routing.json](routing.json). When adding a skill to + `.claude/skills/`, add its route there or list it in `notReviewers`. diff --git a/.claude/skills/pr-self-review/evals/evals.json b/.claude/skills/pr-self-review/evals/evals.json new file mode 100644 index 0000000000..bb5288ad4d --- /dev/null +++ b/.claude/skills/pr-self-review/evals/evals.json @@ -0,0 +1,29 @@ +{ + "skill_name": "pr-self-review", + "evals": [ + { + "id": 1, + "prompt": "я закінчив з фічею в server/src/modules/repos, відкрий PR в main", + "expected_output": "Runs the full self-review before opening the PR; server files go to onion-architecture/fastify/security only (no frontend skills); arch:check failures surface as critical; the PR is not opened while a critical remains; the user is offered fixes.", + "files": [] + }, + { + "id": 2, + "prompt": "/pr-self-review --base l-01", + "expected_output": "Uses l-01 as base; reviewer groups spawned in one message; criticals verified before the verdict; reports the verdict, criticals with ids and fixes, and the coverage table.", + "files": [] + }, + { + "id": 3, + "prompt": "the PR command got blocked by some hook, just get the PR open please", + "expected_output": "Explains the gate and runs the self-review instead of bypassing it; never adds --waive on its own; asks the user for a reason if they want to accept a critical.", + "files": [] + }, + { + "id": 4, + "prompt": "поміняв тільки client/src/app/agents/_components/AgentCard, глянь чи норм перед PR, тести теж прожени", + "expected_output": "Uses --with-tests; routes only to frontend-ui-architecture, react-best-practices, next-best-practices (under app/) and react-testing-library for the test file; no onion/fastify reviewers.", + "files": [] + } + ] +} diff --git a/.claude/skills/pr-self-review/references/guard-integrity.md b/.claude/skills/pr-self-review/references/guard-integrity.md new file mode 100644 index 0000000000..9d0ce961d8 --- /dev/null +++ b/.claude/skills/pr-self-review/references/guard-integrity.md @@ -0,0 +1,34 @@ +# @guard-integrity — built-in reviewer (not a standalone skill) + +Routed to `reviewer-core/src/prompt.ts`, `reviewer-core/src/grounding.ts` and +`reviewer-core/src/review/**`. Root `CLAUDE.md` says: *"`INJECTION_GUARD` and the +grounding gate in reviewer-core — never weaken them, never replace them with +keyword filtering."* This reviewer enforces exactly that and nothing else. + +Read `reviewer-core/CLAUDE.md` and `reviewer-core/INSIGHTS.md` first, then the +current `prompt.ts` and `grounding.ts` in full, and work out what the guard and +the gate protect against today. + +## Critical (any of these, when introduced by the diff) + +- `INJECTION_GUARD` text removed, shortened, made conditional, moved after + untrusted content, or no longer included in a prompt path that carries PR + content. +- PR/diff/file content concatenated into the system prompt or instructions + instead of being delimited as untrusted data. +- The grounding gate skipped, short-circuited, or loosened: findings accepted + without matching a real diff line, tolerance widened, a new `kind` or flag + that bypasses line grounding when the LLM can set it, or trusted-only kinds + that can now come from LLM output. +- The injection defence replaced with keyword/regex filtering of the input. +- Tests that cover the guard or the gate deleted or weakened (assertion removed, + `.skip`, expectations inverted). + +## Major + +- Refactors that keep the behaviour but make the guard easier to drop by + accident (e.g. guard moved into an optional parameter with a default). +- New prompt path without a test that asserts the guard is present. + +Anything else in these files belongs to the other reviewers (typescript-expert, +zod, security), so do not report it here. diff --git a/.claude/skills/pr-self-review/references/reviewer-prompt.md b/.claude/skills/pr-self-review/references/reviewer-prompt.md new file mode 100644 index 0000000000..78bfab8393 --- /dev/null +++ b/.claude/skills/pr-self-review/references/reviewer-prompt.md @@ -0,0 +1,77 @@ +# Reviewer subagent prompt + +Fill the `{{…}}` placeholders from one entry of `manifest.groups` and pass the +result as the Agent prompt (`subagent_type: general-purpose`). One agent per +group, all spawned in a single message. + +--- + +You are a code reviewer applying ONE skill to part of a local diff, before a +pull request is opened. A finding of severity `critical` blocks the PR, so +precision matters more than volume. + +**Skill to apply:** read `{{skillPath}}` in full. Read files it links to only +when a rule you are checking points there. Review strictly through this skill's +lens. Other reviewers cover the other skills, so ignore issues outside it. + +**Severity scale:** read `{{severityPath}}` and use only +`critical | major | minor | nit`. + +**Project context:** read {{moduleDocs}}. Treat `INSIGHTS.md` entries as +high-confidence project conventions. + +**What changed:** `{{diffPath}}` is the unified diff for your files (base +`{{base}}`). Your files and their changed (new-side) line numbers: + +{{fileList}} + +Read the current version of each file for context. Read neighbouring files only +when you need to confirm an import target or a caller. + +## Rules + +1. Report only problems **introduced or made worse by this diff**. The finding + must sit on a changed line from the list above. If a deleted line is the + problem, set `"side": "removed"`. A problem on an unchanged line belongs + in the report only if this diff makes it reachable or worse. The scripts + downgrade unchanged-line findings to `info` anyway. +2. **Every finding quotes the code.** `evidence` is the exact line from the + file (or from the `-` side of the diff for `"side": "removed"`), copied + verbatim. Findings whose quote does not exist in the code are dropped + automatically, so do not paraphrase. +3. `critical` needs a concrete failure scenario in `why`: which input or + state leads to which wrong behaviour, or which rule in root `CLAUDE.md` / + which architecture rule it breaks. If you cannot state one, it is `major` + at most. +4. `rule` is a short stable id: `/`, e.g. + `onion/no-drizzle-in-service`, `fe-arch/no-fetch-outside-gateway`. + Reuse the same id for the same problem in different places. +5. Do not edit any source file. Your only write is the output file below. +6. Zero findings is a valid, good result. Do not invent issues to look useful. + +## Output + +Write this JSON to `{{outputPath}}` (create it even when `findings` is empty): + +```json +{ + "group": "{{groupId}}", + "skill": "{{skill}}", + "reviewed": [""], + "findings": [ + { + "severity": "critical", + "file": "server/src/modules/repos/service.ts", + "line": 42, + "side": "added", + "rule": "onion/no-drizzle-in-service", + "title": "Service queries the DB directly", + "evidence": "import { eq } from \"drizzle-orm\";", + "why": "Application service imports drizzle-orm, so the import points outward (onion rule 2); the use case can no longer be tested with fakes.", + "fix": "Move the query into ReposRepository and inject the repository port." + } + ] +} +``` + +Then reply with one line: `: findings ( critical)`. diff --git a/.claude/skills/pr-self-review/references/severity.md b/.claude/skills/pr-self-review/references/severity.md new file mode 100644 index 0000000000..5325b63eca --- /dev/null +++ b/.claude/skills/pr-self-review/references/severity.md @@ -0,0 +1,41 @@ +# Severity scale (shared by every reviewer) + +The skills we route to grade things differently. `react-best-practices` uses +CRITICAL/HIGH/MEDIUM, `security` grades confidence, and the architecture skills +only have checklists. Map everything onto this one scale. **Critical blocks the +PR**, so a wrong critical costs a person real time and a missed one ships a +defect. Be calibrated, not dramatic. + +| Severity | Meaning | Blocks PR | +|---|---|---| +| **critical** | Introduced by this diff, and one of: (a) a demonstrable bug — wrong result, crash, data loss, broken contract at runtime; (b) a vulnerability with an attacker-reachable input you traced; (c) a breach of a project rule from root `CLAUDE.md` "Do not touch" / "Cross-package rules"; (d) a new dependency-direction violation (onion rules 1/2/4/5/6/10/11; frontend "imports never point up or sideways"). | yes | +| **major** | A clear violation of a rule in the skill that will make the code harder to change, test or reason about, but nothing is broken today. | no | +| **minor** | Worth fixing in this PR if cheap; a weaker version of a skill rule, naming, small duplication. | no | +| **nit** | Taste. Only report if the skill states it explicitly. | no | +| **info** | Set by the scripts, not by you: an issue on a line this diff did not change. | no | + +## Mapping each skill's own scale + +| Skill | Its label | Our severity | +|---|---|---| +| react-best-practices | CRITICAL (reconciliation bugs, broken hooks rules) | critical only if it causes a real bug in this code path, else major | +| react-best-practices | HIGH / MEDIUM | major / minor | +| security | HIGH confidence (pattern + attacker-controlled input confirmed) | critical | +| security | MEDIUM confidence (input source unclear) | major — say what to verify | +| security | LOW | do not report | +| onion-architecture | checklist item breaking import direction, framework/ORM leak into domain or service, cross-module internals import | critical | +| onion-architecture | other checklist items (missing UoW, error without `code`, mapping in wrong file, missing test) | major | +| frontend-ui-architecture | import pointing up/sideways, `fetch` outside the gateway | critical | +| frontend-ui-architecture | other checklist items (placement, thin routes, barrels, state copies) | major | +| next-best-practices, fastify, drizzle, postgres, zod, typescript-expert, react-testing-library | rule that causes a runtime/type-safety bug in this diff | critical | +| same | other rule violations | major / minor by impact | +| @guard-integrity | any weakening of INJECTION_GUARD or the grounding gate | critical | + +## Rules of thumb + +- "Could be a problem if…" is not critical. Critical needs the concrete failing + scenario: input/state → wrong output. +- Style, size, and "I would have done it differently" are never critical. +- When two rules from different skills disagree (e.g. `react-best-practices` + size rules vs `frontend-ui-architecture`), the skill earlier in + `routing.json → precedence` wins; report the conflict as minor at most. diff --git a/.claude/skills/pr-self-review/references/verifier-prompt.md b/.claude/skills/pr-self-review/references/verifier-prompt.md new file mode 100644 index 0000000000..0174a3ee07 --- /dev/null +++ b/.claude/skills/pr-self-review/references/verifier-prompt.md @@ -0,0 +1,38 @@ +# Verifier subagent prompt + +One agent per entry in `candidates.json` (`subagent_type: general-purpose`), all +spawned in one message. The verifier gets only the finding, not the reviewer's +reasoning, so it judges the code fresh. + +--- + +A reviewer claims the following **critical** issue in a local diff. A critical +blocks the pull request. Your job is to try to **refute** it: find out whether +it is real, introduced by this diff, and actually critical under +`{{severityPath}}`. + +Finding: + +```json +{{findingJson}} +``` + +Diff for the file: run `git diff {{mergeBase}} -- {{file}}`. If that prints +nothing, the file is untracked and the whole file is new. Read the current +file, and follow imports/callers as far as needed. If the finding cites a +skill rule, read the rule in `{{skillPath}}`. + +Decide: +- `refuted`: the quoted code does not do what is claimed, or the problem existed + before this diff, or the failure scenario cannot happen (input is trusted, + path unreachable, guarded elsewhere), or it is a real issue but below + critical (style, maintainability only). +- `confirmed`: you can state the concrete failing scenario yourself. + +Do not edit any source file. Write to `{{verifyPath}}`: + +```json +{ "id": "{{id}}", "verdict": "confirmed", "reason": "" } +``` + +Reply with one line: `: confirmed|refuted`. diff --git a/.claude/skills/pr-self-review/routing.json b/.claude/skills/pr-self-review/routing.json new file mode 100644 index 0000000000..48d13b60e2 --- /dev/null +++ b/.claude/skills/pr-self-review/routing.json @@ -0,0 +1,118 @@ +{ + "$comment": "Single source of truth for which skill reviews which changed file. Globs are repo-relative. A file can be routed to several skills. See README.md §Routing.", + "version": 1, + "maxFilesPerGroup": 40, + "exclude": [ + "**/pnpm-lock.yaml", + "**/package-lock.json", + "server/src/db/migrations/**", + "**/*.{png,jpg,jpeg,gif,svg,ico,webp,woff,woff2,ttf,pdf}", + "**/*.snap", + "**/*.tsbuildinfo", + "**/.dependency-cruiser-known-violations.json" + ], + "precedence": [ + "project-rules", + "@guard-integrity", + "onion-architecture", + "frontend-ui-architecture", + "security", + "next-best-practices", + "react-best-practices", + "fastify-best-practices", + "drizzle-orm-patterns", + "postgresql-table-design", + "zod", + "react-testing-library", + "typescript-expert" + ], + "routes": [ + { + "skill": "frontend-ui-architecture", + "include": ["client/src/**/*.{ts,tsx}"], + "exclude": ["client/src/vendor/**", "client/src/**/*.test.{ts,tsx}"] + }, + { + "skill": "react-best-practices", + "include": ["client/src/**/*.tsx", "client/src/**/use-*.ts", "client/src/lib/hooks/**/*.ts"], + "exclude": ["client/src/vendor/**", "client/src/**/*.test.tsx"] + }, + { + "skill": "next-best-practices", + "include": [ + "client/src/app/**/*.{ts,tsx}", + "client/src/middleware.ts", + "client/next.config.{js,mjs,ts}" + ] + }, + { + "skill": "react-testing-library", + "include": ["client/src/**/*.test.{ts,tsx}"] + }, + { + "skill": "onion-architecture", + "include": ["server/src/**/*.ts"], + "exclude": ["server/src/vendor/**", "server/src/**/*.test.ts"] + }, + { + "skill": "fastify-best-practices", + "include": ["server/src/**/routes.ts", "server/src/**/routes/**/*.ts", "server/src/platform/**/*.ts", "server/src/app.ts", "server/src/server.ts"] + }, + { + "skill": "drizzle-orm-patterns", + "include": ["server/src/db/**/*.ts", "server/src/**/*repository*.ts", "server/src/**/repository/**/*.ts"] + }, + { + "skill": "postgresql-table-design", + "include": ["server/src/db/schema.ts", "server/src/db/schema/**/*.ts"] + }, + { + "skill": "zod", + "include": [ + "server/src/vendor/shared/**/*.ts", + "client/src/vendor/shared/**/*.ts", + "**/*schema*.ts", + "**/contracts/**/*.ts", + "reviewer-core/src/**/*.ts" + ], + "exclude": ["server/src/db/**"] + }, + { + "skill": "security", + "include": [ + "server/src/**/*.ts", + "reviewer-core/src/**/*.ts", + "client/src/lib/api*.ts", + "client/src/app/api/**/*.ts", + "client/src/middleware.ts", + ".github/workflows/**", + "**/package.json", + ".claude/settings*.json", + "**/.env.example", + "scripts/**" + ], + "exclude": ["server/src/**/*.test.ts", "server/src/vendor/**"] + }, + { + "skill": "@guard-integrity", + "reference": "references/guard-integrity.md", + "include": ["reviewer-core/src/prompt.ts", "reviewer-core/src/grounding.ts", "reviewer-core/src/review/**/*.ts"] + }, + { + "skill": "typescript-expert", + "include": ["reviewer-core/src/**/*.ts", "e2e/**/*.ts"] + } + ], + "fallback": { + "$comment": "Applied only to code files that no route above picked up.", + "skill": "typescript-expert", + "include": ["**/*.{ts,tsx,mts,cts,js,mjs,cjs}"] + }, + "scopeDefaults": { + "$comment": "Used for skills found in .claude/skills/README.md catalog but not routed here — so a newly added skill is never silently ignored.", + "Backend": ["server/src/**/*.ts"], + "Frontend": ["client/src/**/*.{ts,tsx}"], + "Full-stack": ["server/src/**/*.ts", "client/src/**/*.{ts,tsx}", "reviewer-core/src/**/*.ts"] + }, + "notReviewers": ["mermaid-diagram", "engineering-insights", "pr-self-review"] +} diff --git a/.claude/skills/pr-self-review/scripts/checks.mjs b/.claude/skills/pr-self-review/scripts/checks.mjs new file mode 100644 index 0000000000..012401f0bc --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/checks.mjs @@ -0,0 +1,133 @@ +#!/usr/bin/env node +// Step 2: deterministic gates — no LLM. Project rules from the root CLAUDE.md +// plus typecheck / arch:check (and tests with --with-tests) for touched packages. +// +// node checks.mjs --run [--with-tests] [--skip-commands] +// +// Writes /checks.json. Every finding here is a fact, so it skips the +// LLM verification step. +import { spawnSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; +import { parseArgs, readJson, writeJson } from "./lib.mjs"; + +const args = parseArgs(process.argv.slice(2)); +if (typeof args.run !== "string") throw new Error("Usage: checks.mjs --run "); +const manifest = readJson(join(args.run, "manifest.json")); +const facts = readJson(join(args.run, "files.json")); +const root = manifest.root; +const changed = manifest.files.map((f) => f.path); +const has = (prefix) => changed.some((p) => p.startsWith(prefix)); +const findings = []; +const add = (f) => findings.push({ source: "project-rules", severity: "critical", line: 1, ...f }); + +// ---- 1. Migrations are generated: never hand-edited without a schema change ---- +const migrations = changed.filter((p) => p.startsWith("server/src/db/migrations/")); +const schemaChanged = changed.some((p) => p === "server/src/db/schema.ts" || p.startsWith("server/src/db/schema/")); +if (migrations.length && !schemaChanged) { + for (const p of migrations) { + add({ + file: p, + rule: "project/migrations-are-generated", + title: "Migration changed without a schema change", + evidence: p, + why: "server/src/db/migrations/** is generated (root CLAUDE.md, Do not touch). Edit the schema and run `pnpm db:generate`.", + fix: "Revert the migration edit; change server/src/db/schema* and regenerate.", + }); + } +} + +// ---- 2. Zod contracts live twice: a change must land in both copies ---- +const SHARED = ["server/src/vendor/shared/", "client/src/vendor/shared/"]; +for (const p of changed) { + const side = SHARED.findIndex((s) => p.startsWith(s)); + if (side === -1) continue; + const twin = SHARED[1 - side] + p.slice(SHARED[side].length); + if (!changed.includes(twin)) { + const firstAdded = facts[p]?.added?.[0]; + add({ + file: p, + line: firstAdded ?? 1, + rule: "project/contract-twin", + title: "Shared contract changed in only one copy", + evidence: facts[p]?.addedText?.[0] ?? p, + why: `Contracts exist twice (root CLAUDE.md). ${twin} was not changed in this diff.`, + fix: `Apply the same change to ${twin}.`, + }); + } +} + +// ---- 3. Secrets: never in git ---- +const SECRET_PATTERNS = [ + [/sk-ant-[A-Za-z0-9_-]{20,}/, "Anthropic API key"], + [/\bsk-(?:proj-)?[A-Za-z0-9_-]{32,}/, "OpenAI-style API key"], + [/\bgh[pousr]_[A-Za-z0-9]{36,}/, "GitHub token"], + [/\bgithub_pat_[A-Za-z0-9_]{50,}/, "GitHub fine-grained token"], + [/\bAKIA[0-9A-Z]{16}\b/, "AWS access key"], + [/\bxox[baprs]-[A-Za-z0-9-]{10,}/, "Slack token"], + [/-----BEGIN [A-Z ]*PRIVATE KEY-----/, "Private key"], +]; +for (const [path, f] of Object.entries(facts)) { + const base = path.split("/").pop(); + if (/^\.env(\..+)?$/.test(base) && !/\.(example|sample|template)$/.test(base) && f.status !== "D") { + add({ file: path, rule: "project/no-committed-env", title: "Env file added to the repo", evidence: path, + why: "Secrets go through SecretsProvider (~/.devdigest/secrets.json) only — never a committed .env.", fix: "Remove the file and add it to .gitignore." }); + } + f.addedText.forEach((text, i) => { + const hit = SECRET_PATTERNS.find(([re]) => re.test(text)); + if (hit) { + add({ file: path, line: f.added[i], rule: "project/no-secrets", title: `${hit[1]} in diff`, evidence: text.trim().slice(0, 200), + why: "Secrets go through SecretsProvider only — never DB, git or a committed file.", fix: "Remove it, rotate the credential, load it via SecretsProvider." }); + } + }); +} + +// ---- 4. INJECTION_GUARD / grounding gate must never be weakened ---- +for (const [path, f] of Object.entries(facts)) { + if (!path.startsWith("reviewer-core/")) continue; + const removed = f.removedText.filter((t) => /INJECTION_GUARD/.test(t)); + const readded = f.addedText.filter((t) => /INJECTION_GUARD/.test(t)); + if (removed.length > readded.length) { + add({ file: path, line: f.added[0] ?? 1, side: "removed", rule: "project/injection-guard", title: "INJECTION_GUARD usage removed", + evidence: removed[0].trim(), why: "INJECTION_GUARD must never be weakened or removed (root CLAUDE.md, Do not touch).", fix: "Restore the guard." }); + } +} + +// ---- 5. Package commands for touched packages ---- +const commands = []; +if (!args["skip-commands"]) { + const plan = []; + if (has("server/")) { + plan.push(["server: typecheck", "server", "pnpm", ["typecheck"]]); + plan.push(["server: arch:check", "server", "pnpm", ["arch:check"]]); + if (args["with-tests"]) plan.push(["server: unit tests", "server", "pnpm", ["exec", "vitest", "run", "--exclude", "**/*.it.test.ts"]]); + } + if (has("client/")) { + plan.push(["client: typecheck", "client", "pnpm", ["typecheck"]]); + if (args["with-tests"]) plan.push(["client: tests", "client", "pnpm", ["test"]]); + } + if (has("reviewer-core/")) { + plan.push(["reviewer-core: typecheck", "reviewer-core", "npm", ["run", "typecheck"]]); + if (args["with-tests"]) plan.push(["reviewer-core: tests", "reviewer-core", "npm", ["test"]]); + } + for (const [name, dir, bin, argv] of plan) { + const cwd = join(root, dir); + if (!existsSync(join(cwd, "node_modules"))) { + commands.push({ name, cmd: `${bin} ${argv.join(" ")}`, cwd: dir, ok: null, skipped: "node_modules missing — install deps to run it" }); + continue; + } + const t0 = Date.now(); + const r = spawnSync(bin, argv, { cwd, encoding: "utf8", maxBuffer: 64 * 1024 * 1024, timeout: 10 * 60 * 1000 }); + const out = `${r.stdout ?? ""}${r.stderr ?? ""}`.trim(); + const ok = r.status === 0; + commands.push({ name, cmd: `${bin} ${argv.join(" ")}`, cwd: dir, ok, durationMs: Date.now() - t0, outputTail: out.split("\n").slice(-40).join("\n") }); + if (!ok) { + add({ file: dir, rule: `command/${name.replace(/[^a-z0-9]+/gi, "-").toLowerCase()}`, title: `${name} failed`, + evidence: `${bin} ${argv.join(" ")}`, why: out.split("\n").slice(-15).join("\n"), fix: "Fix the errors above and re-run the self-review." }); + } + } +} + +writeJson(join(args.run, "checks.json"), { commands, findings }); +for (const c of commands) console.log(`${c.ok === true ? "PASS" : c.ok === false ? "FAIL" : "SKIP"} ${c.name}${c.skipped ? ` (${c.skipped})` : ""}`); +console.log(`project-rules findings: ${findings.length}${findings.length ? ` (${findings.map((f) => f.rule).join(", ")})` : ""}`); diff --git a/.claude/skills/pr-self-review/scripts/finalize.mjs b/.claude/skills/pr-self-review/scripts/finalize.mjs new file mode 100644 index 0000000000..f73532f75f --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/finalize.mjs @@ -0,0 +1,300 @@ +#!/usr/bin/env node +// Steps 5–7: turn reviewer output into a verdict. +// +// node finalize.mjs --run --stage candidates +// Grounds + filters findings, writes candidates.json (fresh criticals that +// need an adversarial verifier). Prints their ids and verify paths. +// node finalize.mjs --run [--quick] [--waive = ...] +// Applies verifications, suppressions, dedup and waivers, writes the cache, +// report.md, pr-body-section.md and the verdict the gate reads. +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { + SEVERITIES, SKILL_DIR, findingId, matchesAny, normalize, parseArgs, readJson, + stateDir, verdictPath, writeJson, +} from "./lib.mjs"; + +const args = parseArgs(process.argv.slice(2)); +if (typeof args.run !== "string") throw new Error("Usage: finalize.mjs --run [--stage candidates]"); +const runDir = args.run; +const manifest = readJson(join(runDir, "manifest.json")); +const facts = readJson(join(runDir, "files.json")); +const routing = readJson(join(SKILL_DIR, "routing.json")); +const root = manifest.root; +const state = stateDir(root); +const quick = Boolean(args.quick); +const rank = (s) => SEVERITIES.indexOf(s); +const precedence = (s) => { + const i = routing.precedence.indexOf(s); + return i === -1 ? routing.precedence.length : i; +}; +const fileLines = new Map(); +const linesOf = (p) => { + if (!fileLines.has(p)) { + const abs = join(root, p); + fileLines.set(p, existsSync(abs) ? readFileSync(abs, "utf8").split("\n") : []); + } + return fileLines.get(p); +}; + +// ---------------------------------------------------------------- collect +const incomplete = []; +const dropped = []; +const fresh = []; // grounded findings from reviewers that ran in this run +const cached = []; // findings replayed from cache (already grounded + verified) +const reviewedFresh = new Map(); // groupId -> files actually reviewed this run + +for (const g of manifest.groups) { + for (const c of g.cachedFiles) { + const entry = readJson(join(state, "cache", `${c.cacheKey}.json`), null); + if (entry) cached.push(...entry.findings.map((f) => ({ ...f, cached: true }))); + } + if (g.files.length === 0 || quick) continue; + const out = readJson(g.outputPath, null); + if (!out || !Array.isArray(out.findings)) { + incomplete.push(g.id); + continue; + } + const inGroup = new Set(g.files.map((f) => f.path)); + reviewedFresh.set(g.id, g); + for (const raw of out.findings) { + const f = normalizeFinding(raw, g.skill); + if (!inGroup.has(f.file)) { + dropped.push({ ...f, dropReason: "file not in this reviewer's group" }); + continue; + } + const grounded = ground(f); + if (grounded.dropReason) dropped.push(grounded); + else fresh.push(grounded); + } +} + +const checks = readJson(join(runDir, "checks.json"), null); +if (!checks) incomplete.push("checks (checks.mjs did not run)"); +const ruleFindings = (checks?.findings ?? []).map((f) => ({ ...f, id: findingId(f), deterministic: true })); + +// ------------------------------------------------------------- candidates +if (args.stage === "candidates") { + const candidates = fresh.filter((f) => f.severity === "critical"); + writeJson(join(runDir, "candidates.json"), candidates); + console.log(`Grounded findings: ${fresh.length} fresh, ${cached.length} cached, ${dropped.length} dropped as ungrounded.`); + if (incomplete.length) console.log(`INCOMPLETE groups (no valid output): ${incomplete.join(", ")}`); + console.log(`Critical candidates needing verification: ${candidates.length}`); + for (const c of candidates) { + console.log(` ${c.id} ${c.file}:${c.line} [${c.source}] ${c.rule} — ${c.title}`); + console.log(` verify output: ${join(runDir, "verify", `${c.id}.json`)}`); + } + process.exit(0); +} + +// ---------------------------------------------------------- verification +for (const f of fresh) { + if (f.severity !== "critical") continue; + const v = readJson(join(runDir, "verify", `${f.id}.json`), null); + if (!v) f.notes.push("not verified — kept as critical (fail-safe)"); + else if (v.verdict === "refuted") { + f.severity = "major"; + f.notes.push(`downgraded from critical by verifier: ${v.reason ?? "refuted"}`); + } else f.notes.push(`verified: ${v.reason ?? "confirmed"}`); +} + +// ------------------------------------------------------------------ cache +for (const g of reviewedFresh.values()) { + for (const file of g.files) { + const own = fresh.filter((f) => f.source === g.skill && f.file === file.path); + writeJson(join(state, "cache", `${file.cacheKey}.json`), { skill: g.skill, file: file.path, createdAt: new Date().toISOString(), findings: own }); + } +} + +// ----------------------------------------------------------- suppressions +const suppressionsFile = readJson(join(SKILL_DIR, "suppressions.json"), { suppressions: [] }); +const today = new Date().toISOString().slice(0, 10); +const activeSuppressions = suppressionsFile.suppressions.filter((s) => !s.expires || s.expires >= today); +const suppressed = []; +const all = [...ruleFindings, ...fresh, ...cached].filter((f) => { + const why = suppressionFor(f); + if (why) suppressed.push({ ...f, suppressedBy: why }); + return !why; +}); + +// ------------------------------------------------------------------ dedup +const byLocation = new Map(); +for (const f of all.sort((a, b) => rank(a.severity) - rank(b.severity) || precedence(a.source) - precedence(b.source))) { + const key = `${f.file}:${f.line}`; + const primary = byLocation.get(key); + if (!primary) byLocation.set(key, { ...f, alsoReportedBy: [] }); + else primary.alsoReportedBy.push({ source: f.source, severity: f.severity, rule: f.rule, title: f.title }); +} +const final = [...byLocation.values()]; + +// ---------------------------------------------------------------- waivers +const waiverArgs = [].concat(args.waive ?? []).filter((w) => typeof w === "string"); +const waiverWarnings = []; +for (const w of waiverArgs) { + const [id, reason] = w.split(/=(.*)/s); + const target = final.find((f) => f.id === id && f.severity === "critical"); + if (!target) waiverWarnings.push(`waiver "${id}" matches no critical finding — ignored`); + else if (!reason || reason.trim().length < 10) waiverWarnings.push(`waiver "${id}" needs a real reason (≥10 chars) — ignored`); + else target.waived = reason.trim(); +} + +// ---------------------------------------------------------------- verdict +const criticals = final.filter((f) => f.severity === "critical"); +const blocking = criticals.filter((f) => !f.waived); +const status = blocking.length || incomplete.length ? "block" : quick ? "quick-pass" : "pass"; +const counts = Object.fromEntries(SEVERITIES.map((s) => [s, final.filter((f) => f.severity === s).length])); +const verdict = { + fingerprint: manifest.fingerprint, + status, + mode: quick ? "quick" : "full", + base: manifest.base, + mergeBase: manifest.mergeBase, + head: manifest.head, + createdAt: new Date().toISOString(), + counts, + criticals: criticals.map(({ id, file, line, rule, title, source, waived }) => ({ id, file, line, rule, title, source, waived: waived ?? null })), + incomplete, + reportPath: join(runDir, "report.md"), +}; +writeJson(verdictPath(state, manifest.fingerprint), verdict); +writeJson(join(runDir, "findings.final.json"), { verdict, findings: final, dropped, suppressed }); + +const report = renderReport(); +writeFileSync(join(runDir, "report.md"), report); +writeFileSync(join(runDir, "pr-body-section.md"), renderPrSection()); +console.log(report); +console.log(`\nVERDICT=${status.toUpperCase()} REPORT=${join(runDir, "report.md")}`); + +// ========================================================================= +function normalizeFinding(raw, skill) { + const sevMap = { high: "major", medium: "minor", low: "nit", blocker: "critical", error: "major", warning: "minor" }; + let severity = String(raw.severity ?? "minor").toLowerCase(); + severity = SEVERITIES.includes(severity) ? severity : sevMap[severity] ?? "minor"; + const f = { + source: skill, + severity, + file: String(raw.file ?? "").replace(/^\.\//, ""), + line: Number.parseInt(raw.line, 10) || 1, + side: raw.side === "removed" ? "removed" : "added", + rule: String(raw.rule ?? `${skill}/unspecified`), + title: String(raw.title ?? "").trim(), + evidence: String(raw.evidence ?? ""), + why: String(raw.why ?? ""), + fix: String(raw.fix ?? ""), + notes: [], + }; + f.id = findingId(f); + return f; +} + +/** Grounding gate: the quote must exist in the code; findings are re-anchored to the real line. */ +function ground(f) { + const quote = f.evidence.split("\n").map(normalize).find((l) => l.length >= 4); + if (!quote) return { ...f, dropReason: "no evidence quote" }; + const fileFacts = facts[f.file]; + if (f.side === "removed") { + return fileFacts?.removedText.some((t) => normalize(t).includes(quote)) + ? f + : { ...f, dropReason: "quoted removed line not found in diff" }; + } + const lines = linesOf(f.file); + let best = -1; + lines.forEach((l, i) => { + if (normalize(l).includes(quote) && (best === -1 || Math.abs(i + 1 - f.line) < Math.abs(best + 1 - f.line))) best = i; + }); + if (best === -1) return { ...f, dropReason: "evidence quote not found in file" }; + const line = best + 1; + if (line !== f.line) f.notes.push(`re-anchored from line ${f.line}`); + const isNew = fileFacts?.added.includes(line); + if (!isNew) { + f.notes.push(`pre-existing (line ${line} not changed in this diff) — was ${f.severity}`); + return { ...f, line, severity: "info", preExisting: true }; + } + return { ...f, line }; +} + +function suppressionFor(f) { + if (f.severity === "critical") return null; // criticals are only waived explicitly, never suppressed + const lines = linesOf(f.file); + for (let i = Math.max(0, f.line - 3); i < Math.min(lines.length, f.line); i++) { + const m = /self-review-ignore[:\s]+([\w@/.*-]+)/.exec(lines[i]); + if (m && (m[1] === f.rule || m[1] === f.source || m[1] === "*")) return `inline comment line ${i + 1}`; + } + const s = activeSuppressions.find( + (s) => (!s.rule || s.rule === f.rule) && (!s.source || s.source === f.source) && (!s.path || matchesAny(f.file, [].concat(s.path))), + ); + return s ? `suppressions.json: ${s.reason}` : null; +} + +function renderFinding(f) { + const out = [`- **\`${f.id}\`** \`${f.file}:${f.line}\` · ${f.source} · \`${f.rule}\`${f.waived ? " · **WAIVED**" : ""}`]; + if (f.title) out.push(` **${f.title}**`); + if (f.evidence) out.push(` > ${f.evidence.split("\n")[0].trim().slice(0, 200)}`); + if (f.why) out.push(` Why: ${f.why.replace(/\n+/g, " ").slice(0, 600)}`); + if (f.fix) out.push(` Fix: ${f.fix.replace(/\n+/g, " ").slice(0, 400)}`); + if (f.waived) out.push(` Waiver reason: ${f.waived}`); + for (const n of f.notes ?? []) out.push(` _${n}_`); + for (const a of f.alsoReportedBy ?? []) out.push(` _also: ${a.source} (${a.severity}) ${a.title}_`); + return out.join("\n"); +} + +function renderReport() { + const r = []; + const icon = { block: "BLOCK", pass: "PASS", "quick-pass": "QUICK PASS (does not unlock PR)" }[status]; + r.push(`# PR self-review — ${icon}`); + r.push(""); + r.push(`fingerprint \`${manifest.fingerprint}\` · base \`${manifest.base}\` · HEAD \`${manifest.head.slice(0, 8)}\` · ${verdict.mode} · ${verdict.createdAt}`); + r.push(""); + r.push(`critical **${counts.critical}** (blocking ${blocking.length}) · major ${counts.major} · minor ${counts.minor} · nit ${counts.nit} · pre-existing ${counts.info}`); + if (incomplete.length) r.push(`\n**Incomplete — verdict cannot pass:** ${incomplete.join(", ")}`); + for (const sev of ["critical", "major", "minor", "nit"]) { + const list = final.filter((f) => f.severity === sev); + if (!list.length) continue; + r.push(`\n## ${sev[0].toUpperCase()}${sev.slice(1)} (${list.length})\n`); + r.push(list.map(renderFinding).join("\n")); + } + const pre = final.filter((f) => f.severity === "info"); + if (pre.length) { + r.push(`\n## Pre-existing in touched files (${pre.length}, never blocking)\n`); + r.push(pre.map((f) => `- \`${f.file}:${f.line}\` · ${f.source} · ${f.title}`).join("\n")); + } + r.push("\n## Deterministic checks\n"); + if (!checks?.commands?.length) r.push("- no package commands ran"); + for (const c of checks?.commands ?? []) r.push(`- ${c.ok === true ? "PASS" : c.ok === false ? "FAIL" : "SKIP"} ${c.name} (\`${c.cmd}\`)${c.skipped ? ` — ${c.skipped}` : ""}`); + r.push("\n## Coverage\n"); + r.push("| reviewer | fresh files | cached files |"); + r.push("|---|---|---|"); + const cov = new Map(); + for (const g of manifest.groups) { + const c = cov.get(g.skill) ?? { fresh: 0, cached: 0 }; + c.fresh += quick ? 0 : g.files.length; + c.cached += g.cachedFiles.length; + cov.set(g.skill, c); + } + for (const [s, c] of cov) r.push(`| ${s} | ${c.fresh} | ${c.cached} |`); + if (manifest.unrouted.length) r.push(`\nNot reviewed by any skill: ${manifest.unrouted.map((p) => `\`${p}\``).join(", ")}`); + for (const w of [...manifest.warnings, ...waiverWarnings]) r.push(`\n> WARN ${w}`); + if (dropped.length || suppressed.length) { + r.push(`\n## Filtered out\n`); + for (const d of dropped) r.push(`- dropped (${d.dropReason}): ${d.source} \`${d.file}:${d.line}\` ${d.title}`); + for (const s of suppressed) r.push(`- suppressed (${s.suppressedBy}): ${s.source} \`${s.file}:${s.line}\` ${s.title}`); + } + return r.join("\n"); +} + +function renderPrSection() { + const r = ["## Self-review", ""]; + r.push(`\`/pr-self-review\` **${status.toUpperCase()}** on fingerprint \`${manifest.fingerprint}\` — critical ${counts.critical}, major ${counts.major}, minor ${counts.minor}.`); + r.push(`Reviewers: ${[...new Set(manifest.groups.map((g) => g.skill))].join(", ") || "none"}; checks: ${(checks?.commands ?? []).map((c) => `${c.name} ${c.ok ? "✓" : c.ok === false ? "✗" : "–"}`).join(", ") || "none"}.`); + const waived = criticals.filter((f) => f.waived); + if (waived.length) { + r.push("", "**Waived criticals:**"); + for (const f of waived) r.push(`- \`${f.file}:${f.line}\` ${f.title} — ${f.waived}`); + } + const majors = final.filter((f) => f.severity === "major"); + if (majors.length) { + r.push("", "**Open majors:**"); + for (const f of majors) r.push(`- \`${f.file}:${f.line}\` ${f.title}`); + } + return `${r.join("\n")}\n`; +} diff --git a/.claude/skills/pr-self-review/scripts/gate.mjs b/.claude/skills/pr-self-review/scripts/gate.mjs new file mode 100644 index 0000000000..465a986e35 --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/gate.mjs @@ -0,0 +1,79 @@ +#!/usr/bin/env node +// The gate. Allows a PR action only if the current local changes have a full +// self-review verdict of PASS. Never runs an LLM — it only reads the verdict. +// +// Claude Code PreToolUse hook (Bash): reads the hook JSON from stdin, acts only +// on `gh pr create|ready|merge`, exits 2 with a reason to block. +// node gate.mjs +// Any other caller (git pre-push hook, CI script, a human): +// node gate.mjs --cli [--base ] exit 0 = allowed, 1 = blocked +import { readFileSync } from "node:fs"; +import { changedFiles, fingerprint, parseArgs, readJson, repoRoot, resolveBase, stateDir, verdictPath } from "./lib.mjs"; + +const args = parseArgs(process.argv.slice(2)); +const cli = Boolean(args.cli); +let requestedBase = typeof args.base === "string" ? args.base : undefined; +let cwd = process.cwd(); + +if (!cli) { + let input; + try { + input = JSON.parse(readFileSync(0, "utf8") || "{}"); + } catch { + process.exit(0); // not our business — never break unrelated tool calls + } + const command = String(input?.tool_input?.command ?? ""); + // Only a real invocation: `gh` must start a shell segment (start, after ; && || | ( or newline, + // optionally behind env assignments). A command that merely mentions the text — echo, grep, + // a heredoc body, a JSON string — must pass untouched. + const invocation = /(?:^|[;&|(\n]|\$\()\s*(?:[A-Za-z_]\w*=\S*\s+)*gh\s+pr\s+(?:create|ready|merge)\b[^;&|\n]*/m.exec(command); + if (!invocation) process.exit(0); + const m = /(?:--base|-B)[=\s]+["']?([\w./-]+)/.exec(invocation[0]); + if (m) requestedBase = m[1]; + if (input.cwd) cwd = input.cwd; +} + +function block(message) { + if (cli) { + console.error(message); + process.exit(1); + } + process.stderr.write(`${message}\n`); + process.exit(2); +} + +let root; +let fp; +try { + root = repoRoot(cwd); + const { mergeBase } = resolveBase(root, requestedBase); + const files = changedFiles(root, mergeBase); + if (files.length === 0) process.exit(0); + fp = fingerprint(root, mergeBase, files); +} catch (e) { + block(`pr-self-review gate could not compute the change fingerprint: ${e.message}`); +} + +const verdict = readJson(verdictPath(stateDir(root), fp), null); +const base = requestedBase ? ` --base ${requestedBase}` : ""; +if (!verdict) { + block( + `PR blocked: no self-review for the current changes (fingerprint ${fp}). ` + + `Run /pr-self-review${base} first — any edit after a review needs a new one.`, + ); +} +if (verdict.status !== "pass") { + const list = verdict.criticals + .filter((c) => !c.waived) + .map((c) => ` - [${c.id}] ${c.file}:${c.line} ${c.title} (${c.source})`) + .join("\n"); + const why = + verdict.status === "quick-pass" + ? "only a --quick review exists; run the full /pr-self-review." + : verdict.incomplete.length + ? `review incomplete (${verdict.incomplete.join(", ")}).` + : `${verdict.criticals.filter((c) => !c.waived).length} unresolved critical finding(s):\n${list}`; + block(`PR blocked by /pr-self-review (fingerprint ${fp}): ${why}\nReport: ${verdict.reportPath}`); +} +if (cli) console.log(`pr-self-review: PASS (fingerprint ${fp})`); +process.exit(0); diff --git a/.claude/skills/pr-self-review/scripts/lib.mjs b/.claude/skills/pr-self-review/scripts/lib.mjs new file mode 100644 index 0000000000..12bb73e364 --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/lib.mjs @@ -0,0 +1,214 @@ +// Shared helpers for the pr-self-review scripts. Node >= 22, no dependencies. +import { execFileSync, spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { dirname, join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +export const SKILL_DIR = resolve(dirname(fileURLToPath(import.meta.url)), ".."); +export const SEVERITIES = ["critical", "major", "minor", "nit", "info"]; + +export function git(args, opts = {}) { + return execFileSync("git", args, { + encoding: "utf8", + maxBuffer: 256 * 1024 * 1024, + stdio: ["ignore", "pipe", "pipe"], + ...opts, + }); +} + +/** git that tolerates non-zero exit (diff --no-index exits 1 when files differ). */ +export function gitLoose(args, opts = {}) { + const r = spawnSync("git", args, { encoding: "utf8", maxBuffer: 256 * 1024 * 1024, ...opts }); + return r.stdout ?? ""; +} + +export function repoRoot(cwd = process.cwd()) { + return git(["rev-parse", "--show-toplevel"], { cwd }).trim(); +} + +/** Lives inside .git (works for worktrees too) so verdicts and cache are never committed. */ +export function stateDir(root) { + const rel = git(["rev-parse", "--git-path", "devdigest-self-review"], { cwd: root }).trim(); + const dir = resolve(root, rel); + mkdirSync(dir, { recursive: true }); + return dir; +} + +export function refExists(root, ref) { + return spawnSync("git", ["rev-parse", "--verify", "--quiet", `${ref}^{commit}`], { cwd: root }).status === 0; +} + +/** Default target: origin/main, else main. Returns { base, mergeBase }. */ +export function resolveBase(root, requested) { + const candidates = requested + ? [requested, `origin/${requested}`] + : ["origin/main", "main"]; + const base = candidates.find((ref) => refExists(root, ref)); + if (!base) throw new Error(`Base branch not found (tried ${candidates.join(", ")}). Pass --base .`); + const mergeBase = git(["merge-base", base, "HEAD"], { cwd: root }).trim(); + return { base, mergeBase }; +} + +const splitZ = (s) => s.split("\0").filter(Boolean); + +/** + * Every local change relative to the merge-base: committed-but-unmerged, + * staged, unstaged and untracked (respecting .gitignore). Renames are split + * into delete + add so the fingerprint does not depend on rename detection. + */ +export function changedFiles(root, mergeBase) { + const files = new Map(); + const tokens = splitZ(git(["diff", "--name-status", "-z", "--no-renames", mergeBase], { cwd: root })); + for (let i = 0; i < tokens.length; i += 2) { + const status = tokens[i]; + const path = tokens[i + 1]; + if (path) files.set(path, { path, status: status[0], untracked: false }); + } + for (const path of splitZ(git(["ls-files", "--others", "--exclude-standard", "-z"], { cwd: root }))) { + files.set(path, { path, status: "A", untracked: true }); + } + return [...files.values()].sort((a, b) => a.path.localeCompare(b.path)); +} + +export function sha256(data) { + return createHash("sha256").update(data).digest("hex"); +} + +export function readFileOrNull(root, path) { + const abs = join(root, path); + return existsSync(abs) ? readFileSync(abs) : null; +} + +/** + * Identity of "what is being reviewed": merge-base + (path, content) of every + * changed file. Committing reviewed changes keeps the fingerprint; editing any + * changed file, or changing the base, produces a new one. + */ +export function fingerprint(root, mergeBase, files) { + const h = createHash("sha256"); + h.update(`base:${mergeBase}\n`); + for (const f of files) { + const content = f.status === "D" ? null : readFileOrNull(root, f.path); + h.update(`${f.path}\0${content === null ? "deleted" : sha256(content)}\n`); + } + return h.digest("hex").slice(0, 16); +} + +/** Unified diff of one file against the merge-base (untracked files diffed against /dev/null). */ +export function fileDiff(root, mergeBase, file, context = 3) { + if (file.untracked) { + return gitLoose(["diff", "--no-index", `-U${context}`, "--", "/dev/null", file.path], { cwd: root }); + } + return git(["diff", "--no-renames", `-U${context}`, mergeBase, "--", file.path], { cwd: root }); +} + +/** Parses a unified diff into added line numbers (new side) and removed line texts. */ +export function parseDiff(diffText) { + const added = []; + const addedText = []; + const removedText = []; + let newLine = 0; + let inHunk = false; + for (const line of diffText.split("\n")) { + const m = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (m) { + newLine = Number(m[1]); + inHunk = true; + continue; + } + if (!inHunk || line.startsWith("\\")) continue; + if (line.startsWith("+")) { + added.push(newLine); + addedText.push(line.slice(1)); + newLine++; + } else if (line.startsWith("-")) { + removedText.push(line.slice(1)); + } else { + newLine++; + } + } + return { added, addedText, removedText }; +} + +/** Compresses sorted line numbers into "3-7,12" style ranges for prompts. */ +export function toRanges(lines) { + const out = []; + for (const n of lines) { + const last = out[out.length - 1]; + if (last && n === last[1] + 1) last[1] = n; + else out.push([n, n]); + } + return out.map(([a, b]) => (a === b ? `${a}` : `${a}-${b}`)).join(","); +} + +/** Minimal glob: **, *, ?, {a,b}. Paths are repo-relative with forward slashes. */ +export function globToRegExp(glob) { + let re = ""; + for (let i = 0; i < glob.length; i++) { + const c = glob[i]; + if (c === "*") { + if (glob[i + 1] === "*") { + const slash = glob[i + 2] === "/"; + re += slash ? "(?:.*/)?" : ".*"; + i += slash ? 2 : 1; + } else re += "[^/]*"; + } else if (c === "?") re += "[^/]"; + else if (c === "{") { + const end = glob.indexOf("}", i); + re += `(?:${glob.slice(i + 1, end).split(",").map(escapeRe).join("|")})`; + i = end; + } else re += escapeRe(c); + } + return new RegExp(`^${re}$`); +} +const escapeRe = (s) => s.replace(/[.+^$()|[\]\\]/g, "\\$&"); + +const globCache = new Map(); +export function matchesAny(path, globs = []) { + return globs.some((g) => { + if (!globCache.has(g)) globCache.set(g, globToRegExp(g)); + return globCache.get(g).test(path); + }); +} + +export function readJson(path, fallback) { + if (!existsSync(path)) { + if (fallback !== undefined) return fallback; + throw new Error(`Missing file: ${path}`); + } + return JSON.parse(readFileSync(path, "utf8")); +} + +export function writeJson(path, data) { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, `${JSON.stringify(data, null, 2)}\n`); +} + +export function parseArgs(argv) { + const args = { _: [] }; + for (let i = 0; i < argv.length; i++) { + const a = argv[i]; + if (!a.startsWith("--")) { + args._.push(a); + continue; + } + const [key, inline] = a.slice(2).split(/=(.*)/s); + const next = argv[i + 1]; + const value = inline ?? (next !== undefined && !next.startsWith("--") ? (i++, next) : true); + if (key in args) args[key] = [].concat(args[key], value); + else args[key] = value; + } + return args; +} + +/** Stable short id so waivers and verifications can reference a finding across runs. */ +export function findingId(f) { + return sha256(`${f.source}|${f.file}|${f.rule}|${normalize(f.evidence ?? "")}`).slice(0, 8); +} + +export const normalize = (s) => s.replace(/\s+/g, " ").trim(); + +export function verdictPath(state, fp) { + return join(state, "verdicts", `${fp}.json`); +} diff --git a/.claude/skills/pr-self-review/scripts/post-status.mjs b/.claude/skills/pr-self-review/scripts/post-status.mjs new file mode 100644 index 0000000000..0414b2ad6a --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/post-status.mjs @@ -0,0 +1,42 @@ +#!/usr/bin/env node +// Optional: publish the verdict as a GitHub commit status on HEAD so branch +// protection can require it (context "devdigest/self-review"). Outward-facing — +// run only when the user asked for it. +// +// node post-status.mjs [--base ] [--dry-run] +// +// Refuses when the working tree has uncommitted changes: the status is attached +// to a commit, so HEAD must contain exactly what was reviewed. +import { execFileSync } from "node:child_process"; +import { changedFiles, fingerprint, git, parseArgs, readJson, repoRoot, resolveBase, stateDir, verdictPath } from "./lib.mjs"; + +const args = parseArgs(process.argv.slice(2)); +const root = repoRoot(); +const { mergeBase } = resolveBase(root, typeof args.base === "string" ? args.base : undefined); + +const dirty = git(["status", "--porcelain", "--untracked-files=normal"], { cwd: root }).trim(); +if (dirty) { + console.error("Refusing: uncommitted or untracked changes. Commit (and push) first so HEAD is what was reviewed."); + process.exit(1); +} +const fp = fingerprint(root, mergeBase, changedFiles(root, mergeBase)); +const verdict = readJson(verdictPath(stateDir(root), fp), null); +if (!verdict) { + console.error(`No verdict for fingerprint ${fp}. Run /pr-self-review first.`); + process.exit(1); +} +const sha = git(["rev-parse", "HEAD"], { cwd: root }).trim(); +const state = verdict.status === "pass" ? "success" : "failure"; +const description = + verdict.status === "pass" + ? `PASS · ${verdict.counts.major} major, ${verdict.counts.minor} minor` + : `BLOCK · ${verdict.criticals.filter((c) => !c.waived).length} critical`; +const cmd = ["api", "-X", "POST", `repos/{owner}/{repo}/statuses/${sha}`, + "-f", `state=${state}`, "-f", "context=devdigest/self-review", "-f", `description=${description.slice(0, 140)}`]; + +if (args["dry-run"]) { + console.log(`gh ${cmd.join(" ")}`); + process.exit(0); +} +execFileSync("gh", cmd, { cwd: root, stdio: "inherit" }); +console.log(`Posted devdigest/self-review=${state} on ${sha.slice(0, 8)}.`); diff --git a/.claude/skills/pr-self-review/scripts/prepare.mjs b/.claude/skills/pr-self-review/scripts/prepare.mjs new file mode 100644 index 0000000000..ae042ab0af --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/prepare.mjs @@ -0,0 +1,192 @@ +#!/usr/bin/env node +// Step 1: collect every local change, fingerprint it, route files to skills, +// split into reviewer groups, and skip files whose review is cached. +// +// node prepare.mjs [--base ] [--no-cache] +// +// Writes /runs//{manifest.json,files.json,groups/*.diff} +// and prints a summary ending with RUN_DIR=. +import { existsSync, readdirSync, readFileSync, rmSync, writeFileSync, mkdirSync } from "node:fs"; +import { join } from "node:path"; +import { + SKILL_DIR, changedFiles, fileDiff, fingerprint, git, matchesAny, parseArgs, parseDiff, + readJson, repoRoot, resolveBase, sha256, stateDir, toRanges, writeJson, +} from "./lib.mjs"; + +const args = parseArgs(process.argv.slice(2)); +const root = repoRoot(); +const state = stateDir(root); +const routing = readJson(join(SKILL_DIR, "routing.json")); +const { base, mergeBase } = resolveBase(root, typeof args.base === "string" ? args.base : undefined); +const head = git(["rev-parse", "HEAD"], { cwd: root }).trim(); + +const files = changedFiles(root, mergeBase); +const fp = fingerprint(root, mergeBase, files); +const runDir = join(state, "runs", fp); +rmSync(runDir, { recursive: true, force: true }); +mkdirSync(join(runDir, "groups"), { recursive: true }); +mkdirSync(join(runDir, "findings"), { recursive: true }); +mkdirSync(join(runDir, "verify"), { recursive: true }); + +// ---- per-file diff facts (used by reviewers and by finalize's grounding) ---- +const fileFacts = {}; +for (const f of files) { + const excluded = matchesAny(f.path, routing.exclude); + const diff = f.status === "D" || excluded ? "" : fileDiff(root, mergeBase, f); + const { added, addedText, removedText } = parseDiff(diff); + fileFacts[f.path] = { ...f, excluded, added, addedText, removedText, diffHash: sha256(diff), diff }; +} + +// ---- which skills exist, and which of them review code ---- +const skillsDir = join(root, ".claude", "skills"); +const installed = readdirSync(skillsDir, { withFileTypes: true }) + .filter((d) => d.isDirectory() && existsSync(join(skillsDir, d.name, "SKILL.md"))) + .map((d) => d.name); +const catalogScope = parseCatalogScopes(join(skillsDir, "README.md")); +const routes = routing.routes.filter((r) => r.skill.startsWith("@") || installed.includes(r.skill)); +const warnings = []; +for (const r of routing.routes) { + if (!r.skill.startsWith("@") && !installed.includes(r.skill)) warnings.push(`routing.json names "${r.skill}" but .claude/skills/${r.skill}/SKILL.md does not exist — route ignored.`); +} +const routed = new Set(routing.routes.map((r) => r.skill)); +const unmappedSkills = installed.filter((s) => !routed.has(s) && !routing.notReviewers.includes(s)); +for (const s of unmappedSkills) { + const scope = catalogScope[s]; + const globs = scope && routing.scopeDefaults[scope]; + if (globs) { + routes.push({ skill: s, include: globs, viaScopeDefault: scope }); + warnings.push(`Skill "${s}" is not in routing.json; using the ${scope} default globs. Add an explicit route.`); + } else { + warnings.push(`Skill "${s}" is not in routing.json and has no catalog scope — it reviews nothing. Add a route or list it in notReviewers.`); + } +} + +// ---- route files to skills ---- +const bySkill = new Map(); +const reviewable = files.filter((f) => f.status !== "D" && !fileFacts[f.path].excluded); +const unrouted = []; +for (const f of reviewable) { + let hit = false; + for (const r of routes) { + if (matchesAny(f.path, r.include) && !matchesAny(f.path, r.exclude)) { + if (!bySkill.has(r.skill)) bySkill.set(r.skill, { route: r, files: [] }); + bySkill.get(r.skill).files.push(f.path); + hit = true; + } + } + if (!hit && routing.fallback && matchesAny(f.path, routing.fallback.include)) { + const s = routing.fallback.skill; + if (!bySkill.has(s)) bySkill.set(s, { route: { skill: s }, files: [] }); + bySkill.get(s).files.push(f.path); + hit = true; + } + if (!hit) unrouted.push(f.path); +} + +// ---- groups (chunked), cache lookup, diff bundles ---- +const promptVersion = sha256( + ["reviewer-prompt.md", "severity.md"].map((n) => readFileSync(join(SKILL_DIR, "references", n), "utf8")).join("\n"), +); +const groups = []; +for (const [skill, { route, files: paths }] of bySkill) { + const skillPath = route.reference + ? join(SKILL_DIR, route.reference) + : join(skillsDir, skill, "SKILL.md"); + const skillHash = sha256(readFileSync(skillPath, "utf8") + promptVersion); + const pending = []; + const cached = []; + for (const p of paths) { + const cacheKey = sha256(`${skill}|${skillHash}|${p}|${fileFacts[p].diffHash}`); + const hit = !args["no-cache"] && existsSync(join(state, "cache", `${cacheKey}.json`)); + (hit ? cached : pending).push({ path: p, cacheKey }); + } + const chunks = []; + for (let i = 0; i < pending.length; i += routing.maxFilesPerGroup) chunks.push(pending.slice(i, i + routing.maxFilesPerGroup)); + if (chunks.length === 0) chunks.push([]); + chunks.forEach((chunk, idx) => { + const id = chunks.length > 1 ? `${slug(skill)}--${idx + 1}` : slug(skill); + const diffPath = join(runDir, "groups", `${id}.diff`); + writeFileSync(diffPath, chunk.map((c) => fileFacts[c.path].diff).join("\n")); + groups.push({ + id, + skill, + skillPath, + viaScopeDefault: route.viaScopeDefault ?? null, + files: chunk.map((c) => ({ path: c.path, cacheKey: c.cacheKey, changedLines: toRanges(fileFacts[c.path].added) })), + cachedFiles: idx === 0 ? cached : [], + diffPath, + outputPath: join(runDir, "findings", `${id}.json`), + moduleDocs: moduleDocs(chunk.map((c) => c.path)), + }); + }); +} + +const manifest = { + fingerprint: fp, + base, + mergeBase, + head, + createdAt: new Date().toISOString(), + root, + runDir, + noCache: Boolean(args["no-cache"]), + files: files.map((f) => ({ + path: f.path, + status: f.untracked ? "untracked" : f.status, + excluded: fileFacts[f.path].excluded, + changedLines: toRanges(fileFacts[f.path].added), + })), + groups, + unrouted, + warnings, +}; +writeJson(join(runDir, "manifest.json"), manifest); +writeJson( + join(runDir, "files.json"), + Object.fromEntries(Object.entries(fileFacts).map(([p, f]) => [p, { added: f.added, addedText: f.addedText, removedText: f.removedText, status: f.status, untracked: f.untracked }])), +); + +// ---- summary ---- +const toReview = groups.filter((g) => g.files.length > 0); +const log = []; +log.push(`Fingerprint ${fp} (base ${base} @ ${mergeBase.slice(0, 8)}, HEAD ${head.slice(0, 8)})`); +log.push(`Changed files: ${files.length} (${reviewable.length} reviewable, ${files.length - reviewable.length} deleted/excluded)`); +if (files.length === 0) log.push("NO_CHANGES — nothing to review."); +for (const g of groups) { + const cachedNote = g.cachedFiles.length ? `, ${g.cachedFiles.length} cached` : ""; + log.push(` ${g.files.length ? "REVIEW" : "cached"} ${g.id}: ${g.files.length} file(s)${cachedNote}`); +} +const unroutedDocs = unrouted.filter((p) => p.endsWith(".md")); +const unroutedOther = unrouted.filter((p) => !p.endsWith(".md")); +if (unroutedOther.length) log.push(`Not covered by any skill: ${unroutedOther.join(", ")}`); +if (unroutedDocs.length) log.push(`Not covered (markdown, expected): ${unroutedDocs.length} file(s)`); +for (const w of warnings) log.push(`WARN ${w}`); +log.push(`Reviewer groups to run: ${toReview.length}`); +log.push(`RUN_DIR=${runDir}`); +console.log(log.join("\n")); + +// --------------------------------------------------------------------------- +function slug(s) { + return s.replace(/^@/, "").replace(/[^a-z0-9-]+/gi, "-"); +} + +function moduleDocs(paths) { + const mods = new Set(paths.map((p) => p.split("/")[0])); + const docs = []; + for (const m of mods) { + for (const n of ["CLAUDE.md", "INSIGHTS.md"]) { + if (existsSync(join(root, m, n))) docs.push(`${m}/${n}`); + } + } + return docs; +} + +function parseCatalogScopes(readmePath) { + if (!existsSync(readmePath)) return {}; + const out = {}; + for (const line of readFileSync(readmePath, "utf8").split("\n")) { + const m = /^\|\s*\[([^\]]+)\]\([^)]*\)\s*\|\s*([^|]+?)\s*\|/.exec(line); + if (m) out[m[1]] = m[2]; + } + return out; +} diff --git a/.claude/skills/pr-self-review/scripts/selftest.mjs b/.claude/skills/pr-self-review/scripts/selftest.mjs new file mode 100644 index 0000000000..650d91028f --- /dev/null +++ b/.claude/skills/pr-self-review/scripts/selftest.mjs @@ -0,0 +1,161 @@ +#!/usr/bin/env node +// Regression test for the deterministic half of pr-self-review: routing, +// fingerprint, grounding, new-only filter, verification, waivers, cache, gate. +// Builds a throwaway git repo, plays the reviewer/verifier roles with canned +// JSON, and asserts verdicts. No LLM, no network. +// +// node selftest.mjs (exit 0 = all scenarios pass) +import { execFileSync, spawnSync } from "node:child_process"; +import { cpSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { SKILL_DIR, readJson } from "./lib.mjs"; + +const repo = mkdtempSync(join(process.env.TMPDIR ?? tmpdir(), "pr-self-review-test-")); +const S = join(repo, ".claude/skills/pr-self-review/scripts"); +let failures = 0; + +const sh = (cmd, args, opts = {}) => execFileSync(cmd, args, { cwd: repo, encoding: "utf8", ...opts }); +const g = (...a) => sh("git", a); +const write = (p, c) => { + mkdirSync(dirname(join(repo, p)), { recursive: true }); + writeFileSync(join(repo, p), c); +}; +const node = (script, ...a) => sh("node", [join(S, script), ...a]); +const gate = (command) => + spawnSync("node", [join(S, "gate.mjs")], { cwd: repo, encoding: "utf8", input: JSON.stringify({ tool_input: { command }, cwd: repo }) }); +const check = (name, cond, detail = "") => { + console.log(`${cond ? "ok " : "FAIL"} ${name}${!cond && detail ? `\n ${detail}` : ""}`); + if (!cond) failures++; +}; +const prepare = (...a) => { + const out = node("prepare.mjs", ...a); + const runDir = /RUN_DIR=(.*)/.exec(out)[1].trim(); + return { out, runDir, manifest: readJson(join(runDir, "manifest.json")) }; +}; +const review = (manifest, skill, findings) => { + for (const grp of manifest.groups.filter((x) => x.skill === skill && x.files.length)) { + writeFileSync(grp.outputPath, JSON.stringify({ group: grp.id, skill, reviewed: grp.files.map((f) => f.path), findings })); + } +}; +const reviewAllEmpty = (manifest) => { + for (const grp of manifest.groups.filter((x) => x.files.length)) review(manifest, grp.skill, []); +}; +const finalize = (runDir, ...a) => { + const out = node("finalize.mjs", "--run", runDir, ...a); + return { out, verdict: /VERDICT=(\S+)/.exec(out)?.[1] }; +}; + +try { + // ---------------------------------------------------------------- fixture + g("init", "-q", "-b", "main"); + g("config", "user.email", "t@t"); + g("config", "user.name", "t"); + cpSync(SKILL_DIR, join(repo, ".claude/skills/pr-self-review"), { recursive: true }); + for (const s of ["onion-architecture", "frontend-ui-architecture", "security", "zod", "typescript-expert", "mermaid-diagram", "brand-new-skill"]) { + write(`.claude/skills/${s}/SKILL.md`, `---\nname: ${s}\n---\n# ${s}\n`); + } + write(".claude/skills/README.md", "| Skill | Scope | Description |\n|---|---|---|\n| [brand-new-skill](brand-new-skill/SKILL.md) | Backend | test |\n"); + write("server/CLAUDE.md", "# server\n"); + write("server/src/modules/repos/service.ts", "export class ReposService {\n list() {\n return [];\n }\n}\n"); + write("server/src/vendor/shared/contracts/a.ts", "export const A = 1;\n"); + write("client/src/vendor/shared/contracts/a.ts", "export const A = 1;\n"); + write("client/src/app/page.tsx", "export default function Page() {\n return null;\n}\n"); + write("docs/readme.md", "hello\n"); + g("add", "-A"); + g("commit", "-qm", "base"); + g("checkout", "-qb", "feature"); + + // ------------------------------------------------- 1. docs-only change + write("docs/readme.md", "hello world\n"); + check("gate blocks gh pr create before any review", gate("gh pr create --fill").status === 2); + check("gate ignores unrelated commands", gate("git status").status === 0); + check("gate ignores commands that only mention the text", gate(`echo '{"command":"gh pr create --fill"}' | jq . && grep -r "gh pr merge" docs`).status === 0); + check("gate catches gh after && and env assignments", gate("cd /tmp && GH_PROMPT_DISABLED=1 gh pr merge 12 --squash").status === 2); + check("gate catches gh inside $( )", gate('url=$(gh pr create --fill)').status === 2); + let r = prepare(); + check("docs-only change has no reviewer groups", r.manifest.groups.length === 0, JSON.stringify(r.manifest.groups)); + node("checks.mjs", "--run", r.runDir, "--skip-commands"); + check("docs-only change passes", finalize(r.runDir).verdict === "PASS"); + check("gate allows after PASS", gate("gh pr create --fill").status === 0); + g("commit", "-qam", "docs"); + check("committing reviewed changes keeps the verdict valid", gate("gh pr create --fill").status === 0); + + // ------------------------------------------------- 2. routing + criticals + write( + "server/src/modules/repos/service.ts", + 'import { eq } from "drizzle-orm";\nexport class ReposService {\n list() {\n return [];\n }\n}\n', + ); + write("client/src/app/page.tsx", "export default function Page() {\n fetch('/api/x');\n return null;\n}\n"); + check("gate blocks after an edit invalidates the fingerprint", gate("gh pr create").status === 2); + r = prepare(); + const skillsFor = (p) => r.manifest.groups.filter((x) => x.files.some((f) => f.path === p)).map((x) => x.skill).sort(); + check("server file → onion + security, no frontend skills", JSON.stringify(skillsFor("server/src/modules/repos/service.ts")) === JSON.stringify(["brand-new-skill", "onion-architecture", "security"]), skillsFor("server/src/modules/repos/service.ts").join()); + check("client file → frontend-ui-architecture, no onion", skillsFor("client/src/app/page.tsx").includes("frontend-ui-architecture") && !skillsFor("client/src/app/page.tsx").includes("onion-architecture"), skillsFor("client/src/app/page.tsx").join()); + check("unrouted skill with catalog scope gets a warning + default route", r.manifest.warnings.some((w) => w.includes("brand-new-skill"))); + check("notReviewers skill never reviews", !r.manifest.groups.some((x) => x.skill === "mermaid-diagram")); + node("checks.mjs", "--run", r.runDir, "--skip-commands"); + reviewAllEmpty(r.manifest); + review(r.manifest, "onion-architecture", [ + { severity: "critical", file: "server/src/modules/repos/service.ts", line: 9, rule: "onion/no-drizzle-in-service", title: "drizzle in service", evidence: 'import { eq } from "drizzle-orm";', why: "x" }, + { severity: "major", file: "server/src/modules/repos/service.ts", line: 3, rule: "onion/fake", title: "made-up quote", evidence: "this line does not exist anywhere" }, + { severity: "critical", file: "server/src/modules/repos/service.ts", line: 4, rule: "onion/old-code", title: "pre-existing", evidence: "return [];" }, + ]); + const cand = node("finalize.mjs", "--run", r.runDir, "--stage", "candidates"); + const candidates = readJson(join(r.runDir, "candidates.json")); + check("only the grounded, new-line critical is a candidate", candidates.length === 1 && candidates[0].rule === "onion/no-drizzle-in-service", cand); + check("finding is re-anchored to the real line", candidates[0]?.line === 1); + let f = finalize(r.runDir); + const final = readJson(join(r.runDir, "findings.final.json")); + check("ungrounded finding is dropped", final.dropped.some((d) => d.rule === "onion/fake")); + check("finding on unchanged line becomes info", final.findings.some((x) => x.rule === "onion/old-code" && x.severity === "info")); + check("unverified critical blocks (fail-safe)", f.verdict === "BLOCK"); + check("gate blocks on BLOCK verdict", gate("gh pr create").status === 2); + + writeFileSync(join(r.runDir, "verify", `${candidates[0].id}.json`), JSON.stringify({ id: candidates[0].id, verdict: "refuted", reason: "test" })); + f = finalize(r.runDir); + check("refuted critical is downgraded → PASS", f.verdict === "PASS", f.out.slice(0, 400)); + + writeFileSync(join(r.runDir, "verify", `${candidates[0].id}.json`), JSON.stringify({ id: candidates[0].id, verdict: "confirmed", reason: "test" })); + check("confirmed critical blocks", finalize(r.runDir).verdict === "BLOCK"); + check("waiver without a real reason is ignored", finalize(r.runDir, "--waive", `${candidates[0].id}=ok`).verdict === "BLOCK"); + check("waiver with a reason passes", finalize(r.runDir, "--waive", `${candidates[0].id}=accepted by the team for this release`).verdict === "PASS"); + check("quick verdict never unlocks the gate", finalize(r.runDir, "--quick").verdict === "QUICK-PASS" && gate("gh pr create").status === 2); + + // ------------------------------------------------- 3. cache + finalize(r.runDir, "--waive", `${candidates[0].id}=accepted by the team for this release`); + const r2 = prepare(); + check("unchanged files are served from cache", r2.manifest.groups.every((x) => x.files.length === 0), r2.out); + node("checks.mjs", "--run", r2.runDir, "--skip-commands"); + check("cached confirmed critical still blocks without the waiver", finalize(r2.runDir).verdict === "BLOCK"); + write("client/src/app/page.tsx", "export default function Page() {\n return
;\n}\n"); + const r3 = prepare(); + check("only the edited file is re-reviewed", r3.manifest.groups.filter((x) => x.files.length).every((x) => x.files.every((ff) => ff.path === "client/src/app/page.tsx")), r3.out); + + // ------------------------------------------------- 4. project rules + write("server/src/vendor/shared/contracts/a.ts", "export const A = 2;\n"); + write("server/src/db/migrations/0001_x.sql", "select 1;\n"); + // Built at runtime so this file itself never contains a key-shaped literal. + write("server/src/config.ts", `const k = "${["sk", "ant", "api03", "abcdefghijklmnopqrstuvwxyz0123"].join("-")}";\n`); + r = prepare(); + node("checks.mjs", "--run", r.runDir, "--skip-commands"); + const rules = readJson(join(r.runDir, "checks.json")).findings.map((x) => x.rule); + check("contract changed in one copy → critical", rules.includes("project/contract-twin"), rules.join()); + check("migration without schema change → critical", rules.includes("project/migrations-are-generated")); + check("secret in diff → critical", rules.includes("project/no-secrets")); + check("migrations are excluded from LLM review", !r.manifest.groups.some((x) => x.files.some((ff) => ff.path.includes("migrations")))); + reviewAllEmpty(r.manifest); + check("project-rule criticals block", finalize(r.runDir).verdict === "BLOCK"); + + // ------------------------------------------------- 5. incomplete review + r = prepare("--no-cache"); + node("checks.mjs", "--run", r.runDir, "--skip-commands"); + check("missing reviewer output → BLOCK (incomplete)", finalize(r.runDir).verdict === "BLOCK"); +} catch (e) { + failures++; + console.error(e.stdout ?? "", e.stderr ?? "", e); +} finally { + rmSync(repo, { recursive: true, force: true }); +} +console.log(failures ? `\n${failures} check(s) failed` : "\nall checks passed"); +process.exit(failures ? 1 : 0); diff --git a/.claude/skills/pr-self-review/suppressions.json b/.claude/skills/pr-self-review/suppressions.json new file mode 100644 index 0000000000..9947b15ec5 --- /dev/null +++ b/.claude/skills/pr-self-review/suppressions.json @@ -0,0 +1,4 @@ +{ + "$comment": "Repo-wide suppressions for known false positives. Each needs a reason; add `expires` (YYYY-MM-DD) so they are revisited. Never applies to critical findings — those can only be waived per run with --waive. Fields: rule?, source? (skill), path? (glob or list), reason, expires?", + "suppressions": [] +} diff --git a/.github/workflows/server-unit.yml b/.github/workflows/server-unit.yml index 96da8b9c58..f63ff1d05e 100644 --- a/.github/workflows/server-unit.yml +++ b/.github/workflows/server-unit.yml @@ -65,6 +65,12 @@ jobs: - run: pnpm typecheck + # Onion-architecture boundaries (.dependency-cruiser.cjs). Known legacy + # violations are baselined in .dependency-cruiser-known-violations.json; + # only NEW ones fail. Inlined for the same reason as vitest below. + - name: Architecture boundaries + run: pnpm exec depcruise src --config .dependency-cruiser.cjs --ignore-known --output-type err + unit: name: tests runs-on: ubuntu-latest diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..e975a7e8dd --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,65 @@ +# DevDigest + +Local-first AI pull-request review. Course starter: each lesson (L01–L08) adds +one feature as a new self-contained module — keep changes inside that module. + +## Stack +- Node ≥22 · pnpm ≥10 · TypeScript 5.7 strict (`noUncheckedIndexedAccess`) · ESM +- server: Fastify 5 · Drizzle 0.38 · Postgres + pgvector · zod 3 +- client: Next.js 15 (App Router) · React 19 · TanStack Query 5 · Tailwind 4 +- reviewer-core: pure TS + zod, LLM via injected provider +- tests: vitest 2 · testcontainers · agent-browser (e2e, no LLM) + +## Commands +- Full stack: `./scripts/dev.sh` (Postgres → migrate → seed → API :3001 + web :3000) +- server unit: `cd server && pnpm exec vitest run --exclude '**/*.it.test.ts'` +- server integration (needs Docker): `cd server && pnpm exec vitest run .it.test` +- client: `cd client && pnpm typecheck && pnpm test` +- reviewer-core: `cd reviewer-core && npm run typecheck && npm test` (npm, not pnpm) +- e2e: `cd e2e && npm run e2e:hermetic` (npm, not pnpm) + +## Map +- `server/` API, DB, adapters, repo-intel indexer → server/CLAUDE.md +- `client/` Next.js studio UI → client/CLAUDE.md +- `reviewer-core/` pure review engine (prompt → LLM → grounding) → reviewer-core/CLAUDE.md +- `e2e/` deterministic browser flows → e2e/CLAUDE.md +- `docs/agent-prompts/` system prompts of the built-in agents +- Root `docs/` · `specs/` · `INSIGHTS.md` — only for things that span packages. + +Every module has: `README.md` (source of truth) · `CLAUDE.md` (this kind of map) +· `INSIGHTS.md` (lessons log) · `docs/` (deep dives) · `specs/` (feature specs). + +## Cross-package rules (non-default) +- NO monorepo workspace. Packages share code only via tsconfig `paths` + (source imports, no build). Do not add workspace deps or publish packages. +- Zod contracts exist TWICE: `server/src/vendor/shared` and + `client/src/vendor/shared`. A contract change must be applied to both. +- Secrets go through `SecretsProvider` (`~/.devdigest/secrets.json`) only — + never DB, git, or a committed `.env`. +- Migrations are NOT applied on boot; run `pnpm db:migrate` in `server/`. +- Code comments mentioning an "agent-runner (CI)" refer to a package that does + not exist yet (future lesson) — don't look for it. + +## Do not touch +- `INJECTION_GUARD` and the grounding gate in reviewer-core — never weaken them, + never replace them with keyword filtering. +- `server/src/db/migrations/**` — generated; edit the schema, then `pnpm db:generate`. +- Empty "future lesson" tables, contracts and `client/src/vendor/ui` widgets — + intentional, do not delete as dead code. + +## Read when +- Before changing anything that crosses packages → read README.md §Architecture. +- Before writing, moving or splitting a test → read TESTING.md. +- Before editing a built-in agent's prompt → read docs/agent-prompts/README.md. +- Before implementing a feature → read its spec in `/specs/` (or root + `specs/` if it spans packages). +- Before editing files in a module → read `/CLAUDE.md` and + `/INSIGHTS.md` explicitly (subdirectory auto-load is unreliable in + the VS Code extension). + +## Insights +- Read: before working in a module, read its `INSIGHTS.md` and treat entries + as high-confidence guidance. +- Capture: found a non-obvious lesson, or finished a task that produced one? + Use the `engineering-insights` skill (append-only, module's `INSIGHTS.md`). +- Ask before promoting an insight into a CLAUDE.md. diff --git a/INSIGHTS.md b/INSIGHTS.md new file mode 100644 index 0000000000..df7e2c1f13 --- /dev/null +++ b/INSIGHTS.md @@ -0,0 +1,20 @@ +# Insights — cross-package + + +## What Works + +## What Doesn't Work + +## Codebase Patterns + +## Tool & Library Notes +- 2026-10-19 · Running ANY `pnpm