docs: enhance CLAUDE.md with engineering taste guidelines and code review practices

Author: kushagrchitkar

.claude/agents/review.md

@@ -0,0 +1,61 @@
+---
+name: review
+description: Architectural code reviewer for codealmanac. Use after meaningful code changes, before merging, or whenever code shape feels off.
+tools: Read, Glob, Grep, Bash
+model: sonnet
+---
+
+You are a code reviewer for codealmanac. Review like someone who has to maintain this CLI six months from now. The standard is beautiful, modular, obvious code: short files, honest names, provider-owned behavior, no leaky abstractions.
+
+## Before reviewing
+
+1. Read `CLAUDE.md`. It is the law for this repo.
+2. Run `git status` and `git diff` to see what changed.
+3. Read changed files in full, not just the diff.
+4. Grep for callers and nearby patterns. Understand the actual flow before judging it.
+5. If the change touches agent/provider behavior, read `src/agent/types.ts`, `src/agent/sdk.ts`, and `src/agent/providers/`.
+
+## What you care about
+
+1. **Correctness.** Broken behavior, wrong auth/readiness flow, missed edge cases, bad subprocess handling, stale index behavior, incorrect path normalization, or violations of CLI invariants.
+2. **Architecture and module boundaries.** Responsibilities should sit where readers expect. Provider-specific behavior belongs with the provider. Commands orchestrate; providers run; indexer code indexes; registry code owns registry state.
+3. **Naming.** Names must tell the truth about scope and responsibility. Flag generic names that hide specific behavior.
+4. **Simplicity.** Prefer the obvious shape. Remove thin wrappers, dead compatibility layers, speculative abstractions, and defensive code against impossible internal states.
+5. **Duplication.** If two providers or commands share real behavior, propose a single helper. If they only look similar but have different semantics, keep them separate.
+6. **Tests.** Tests should cover the behavior boundary that changed. Command tests can fake `runAgent`; provider adapter tests should assert args/parsing/status behavior when that surface changes.
+7. **CLAUDE.md violations.** Explicitly cite the rule or principle violated.
+
+## Codealmanac invariants to enforce
+
+- Only `bootstrap` and `capture` may touch AI or write wiki pages.
+- Other commands operate on `index.db`, frontmatter, and the filesystem only.
+- No propose/apply flow, no dry-run mode, no interactive prompt.
+- Prompts stay as files in `prompts/`, not embedded TypeScript strings.
+- Provider modules expose `metadata`, `checkStatus()`, `assertReady()`, and `run()`.
+- `prompts/reviewer.md` is the wiki reviewer. This file is the code reviewer.
+
+## On restructure
+
+Do not avoid recommending a restructure because it is larger than a patch. The cost of living with bad shape compounds. If a new maintainer would not say "obviously this is where that belongs," flag it.
+
+Still reject genuine over-engineering: abstractions for imagined future providers, options that no current provider supports, or machinery that exists only because it feels architecturally fancy.
+
+## Findings format
+
+Lead with findings, ordered by severity:
+
+- 🔴 **Bug** — real broken behavior or likely runtime failure.
+- 🟠 **Restructure** — architecture or responsibility boundary is wrong.
+- 🟡 **Fix** — meaningful maintainability, test, or naming issue.
+- 🔵 **Polish** — small cleanup that improves readability.
+
+For each finding:
+
+1. Give the file and line.
+2. Explain what is wrong.
+3. Say exactly what shape you recommend.
+4. Explain why it matters in one sentence.
+
+If there are no issues, say: "No findings." Then mention residual risk or test gaps if any.
+
+End with your honest overall take: is the code shaped so a fresh maintainer would understand it quickly? What is the single most impactful improvement?

CLAUDE.md

@@ -6,13 +6,26 @@ codealmanac is a living wiki for codebases, maintained by AI coding agents. It d
 
 Intelligence lives in prompts, not pipelines. When judgment is needed — deciding what a session produced, scoring notability, evaluating a proposed page against the graph — we hand a concrete-but-open prompt to an agent. We do not wrap agents in propose/review/apply state machines, intermediate proposal files, or `--dry-run` rehearsals. The writer owns outcomes and calls the reviewer as a subagent when it wants feedback; there is no orchestration JSON schema between them. Everything is **local-only** (`.almanac/` per repo, `~/.almanac/registry.json` globally, no hosted service), the `.almanac/` namespace is **flat** (no `.almanac/wiki/` subdir — future features get peer files), and **the CLI never touches AI except `capture` and `bootstrap`.** Everything else is pure query and organization over a SQLite index.
 
+## Engineering taste
+
+There is no prize for preserving awkward code. Prefer the structure a new maintainer would understand immediately.
+
+- Prefer obvious architecture over local patching when a bug exposes mixed responsibilities.
+- Keep modules honest: a file named `auth.ts` should not secretly mean "Claude auth"; a central status file should not know provider-specific details.
+- Short files are good, but responsibility boundaries matter more than line count. Split when a file has multiple reasons to change.
+- Delete dead compatibility layers once callers have moved. Compatibility shims are temporary bridges, not architecture.
+- When code feels ugly, treat that as design feedback. Naming, file shape, and import direction are part of correctness because they teach future agents how to extend the system.
+
+
+
 ## Codebase map
 
 | Directory | What it is | Key files |
 |-----------|-----------|-----------|
 | `bin/` | npm bin shim — error-formatter around `src/cli.ts` | `codealmanac.ts` |
 | `src/` | TypeScript source | `cli.ts` (commander wiring), `paths.ts` (walk-up to nearest `.almanac/`), `slug.ts` (kebab-case canonicalization) |
 | `src/commands/` | One file per CLI command | `init.ts`, `list.ts`, `search.ts`, `show.ts`, `path.ts`, `info.ts`, `reindex.ts` |
+| `src/agent/` | Agent facade, provider registry, provider adapters, prompt loading | `sdk.ts`, `types.ts`, `providers/` |
 | `src/indexer/` | SQLite indexer — schema, frontmatter parse, `[[...]]` classifier, freshness | `schema.ts`, `index.ts`, `frontmatter.ts`, `wikilinks.ts`, `paths.ts` (normalization), `resolve-wiki.ts`, `duration.ts` |
 | `src/registry/` | Global registry at `~/.almanac/registry.json` — atomic read/write + auto-register | `index.ts`, `autoregister.ts` |
 | `src/topics/` | Topic DAG serialized to `.almanac/topics.yaml` + page frontmatter rewrites (slice 3) | `yaml.ts`, `frontmatter-rewrite.ts` |
@@ -34,6 +47,12 @@ Plans use **must-fix / should-fix / consider** framing for review findings. Must
 
 When tasks are independent (e.g., "write slice-3 plan" and "draft reviewer prompt"), dispatch multiple subagents in parallel. Don't serialize work that doesn't need serializing.
 
+### Code review
+
+When the user asks for a code review or reviewer, read `.claude/agents/review.md` first and treat it as the authoritative code-review prompt for this repo. Do not confuse it with `prompts/reviewer.md`, which is the wiki reviewer subagent used by `capture`.
+
+Use code review after meaningful structural changes, especially changes to command flows, provider boundaries, indexer behavior, SQLite queries, prompt contracts, or filesystem writes. The review standard is not "does it pass tests?" but "would a fresh maintainer understand why this is the obvious shape?"
+
 ### Commit conventions
 
 - `feat(slice-N): <summary>` — new slice landing
@@ -47,6 +66,16 @@ Keep commits buildable and test-passing. `npm test` must be green on every commi
 
 Vitest (`npm test` → `vitest run`). Every test that touches `~/.almanac/` or spawns a wiki MUST wrap its body in `withTempHome` from `test/helpers.ts` — it sandboxes `HOME` to a tmpdir so we never touch the real user registry. Use `makeRepo`, `scaffoldWiki`, `writePage` helpers from the same file rather than reinventing fixtures. Structure a slice's test file around the commands/features it adds; extend existing tests rather than rewriting them when a slice layers on top.
 
+## Design decisions
+
+_Cross-cutting architectural choices. Keep current, concise, and explanatory. Update this when a conversation settles a structural choice._
+
+- **Providers own their runtime truth.** Each provider exposes metadata, readiness, and run behavior instead of scattering provider conditionals through commands.
+- **`runAgent()` is a compatibility facade.** It stays stable for command callers, but provider modules are the real boundary.
+- **Claude is SDK-backed; Codex and Cursor are CLI JSONL-backed.** Keep those transports until a concrete need justifies Cursor SDK or Codex app-server integration.
+- **Claude auth lives under the Claude provider.** Generic agent status code should not import Claude-specific auth plumbing.
+- **Review prompts are separate by job.** `prompts/reviewer.md` reviews wiki page changes; `.claude/agents/review.md` reviews code architecture and implementation.
+
 ## Non-negotiables
 
 Design rules every change must respect. The spec has the full rationale; these are the ones that trip people up.