review

name	review
description	Dispatch parallel codebase or architecture review agents per docs/reviewing.md

Run a full code review. Requires argument: codebase or architecture. No default. If the user invokes /review without an argument, ask which type.

Full protocol at docs/reviewing.md.

Argument: codebase

Line-level scan. 9 parallel background agents, one per scope:

Agent	Scope
irc/	lib/grappa/irc/ (parser, client, message struct)
persistence/	lib/grappa/scrollback*, lib/grappa/{accounts,networks,query_windows,user_settings,visitors}*, priv/repo/migrations/, lib/grappa/repo.ex, lib/grappa/vault.ex. Explicit SQLite angle: WAL + journal_mode + busy_timeout + synchronous configuration in config/runtime.exs; foreign_keys pragma per-connection vs migration-time; defer_foreign_keys (CP19 lesson — see feedback_* memories); index coverage vs hot read patterns (Scrollback page query, query_windows list, last_joined_channels boot read); JSON :map column patterns vs custom Ecto types (Grappa.Scrollback.Meta reference); transaction granularity (long-running writes vs Session.Server message ingest hot path); connection pool sizing for single-writer SQLite reality; schema_migrations consistency + idempotency; Cloak.Vault encrypted-at-rest column types; sqlite-specific quirks (no native datetime, ISO-8601 string convention)
lifecycle/	lib/grappa/{application,bootstrap,config,release,repo,session,version,cic,spawn_orchestrator,admission}*, lib/grappa/session/, lib/grappa/admission/
web/	lib/grappa_web/ (endpoint, router, controllers, channels, plugs, JSON views)
cicchetto/	cicchetto/src/**, cicchetto/{tsconfig.json,vite.config.ts,vitest.config.ts,biome.json,package.json,index.html}, cicchetto/public/{manifest.json,sw.js,icon*}. cicchetto/src/themes/*.css is IN scope and must be read — default.css is the single largest file in the repo (9022 lines) and was materially unreviewed for most of this skill's history: it fell inside the src/** glob while appearing nowhere in the checklist below. See the CSS lens in the cicchetto agent's list.
cross-module	Patterns across all server modules: Application.{get,put}_env outside boot path, String.to_atom/1, default arguments via \\, Logger inline-interpolation vs allowlisted KV, bare catch _, _ / rescue _, Boundary annotations + violations, migration ordering/idempotency, inter-context call discipline, @spec coverage on public context functions, structured-event PubSub topic naming consistency. NO infra in this scope — Docker/scripts split out below.
docker	DEDICATED simplification-focused review. Files: Dockerfile, compose.yaml, compose.override.yaml.example, all scripts/*.sh (especially _lib.sh, deploy.sh, deploy-cic.sh, mix.sh, iex.sh, test.sh, integration.sh, bun.sh, register-dns.sh, monitor.sh, observer.sh, healthcheck.sh, db.sh), infra/snippets/locations-api.conf, infra/freebsd/nginx.conf, infra/linux/nginx.conf, .env.example, .dockerignore. Primary lens: SIMPLIFICATION. What can be removed, collapsed, made standard? Single-stage vs multi-stage tradeoffs (CP23 S1 collapsed to single-stage); profile usage (--profile prod for the cicchetto-build oneshot — #485 dropped the nginx container, the BEAM self-serves); named volumes vs bind mounts (UID drop trap memory); image size; healthcheck adequacy; oneshot semantics (compose up --wait oneshot-exit-trap memory); cicchetto-build oneshot pipeline; deploy.sh hot-vs-cold preflight regex coverage (long-lived GenServer enumeration, defstruct detection); script duplication vs _lib.sh reuse; worktree-awareness invariants; UID/cache layout consistency across mix.sh + bun.sh; bash 4+ assumption documentation. Treat the Docker substrate as a product surface, not glue.
cross-surface	DEDICATED grappa↔cicchetto consistency review. Reads BOTH sides: lib/grappa/{accounts,networks,scrollback,query_windows,user_settings}/wire.ex + lib/grappa_web/{controllers,channels}/** + cicchetto/src/lib/{api,socket,subscribe,auth,messages,*Store,windowState,channelClient,messageClient}.ts + cicchetto/src/components/**. Look for: wire-shape drift (server Wire field exists, cic type missing or narrower); event-name divergence (server emits kind: "joined" but cic dispatcher branches on "join"); error-shape inconsistency (FallbackController response shape vs cic readError parsing); REST-vs-Channel contract divergence for the same domain entity; auth-flow seams (server admission decisions ↔ cic login UX, capability checks); time-stamp format consistency (ISO-8601 vs epoch ms); naming conventions (snake_case server JSON vs camelCase cic types — boundary clarity); version/bundle-hash protocol (server emits bundle_hash, cic compares — verify hash field shape); state-mirror discipline (cic windowState reads server window_states; CLAUDE.md "cic NEVER originates state" — flag any optimistic client state); duplicated logic (parsing/validation in cic that the server already enforces, or vice versa); things that should be SHARED (a single source of truth for the closed MessageKind set, network connection_state enum, etc.) but are restated independently. Output should highlight unification opportunities.
consistency	DEDICATED sibling-divergence + stale-premise review. Reads PATTERNS, not a directory. Every other agent reads the files in its scope and judges each one on its own terms; this agent judges files against each other, because a whole class of defect is invisible from inside any single file. Three jobs, full-repo (server + cicchetto + CSS): (1) Instance families. Enumerate every use of a shared component / helper / rule, then diff the treatment across instances. When N-1 members got a fix, an override or a guard and one did not, that one is the finding. Canonical case: three InlineConfirmButtons live in the settings drawer; quit and the vhost reconnect each got a size + colour override, the identity apply button never did, so it silently kept the compact reddish base. Each diff was correct in isolation; only the family read exposes it. (2) Stale premises. Comments that assert a fact — "users have passwords, no per-network identity editor", "a visitor is effectively single-network", "nothing publishes the uploads" — are claims with an expiry date. Re-derive each against today's code and flag the ones that stopped being true. A gate justified by a dead premise is a bug wearing a rationale. (3) Silent narrowing. Heuristics that pick one row out of N without telling the user: reduce-to-lowest-id "anchor" selections, List.first, hd/1, `

Each agent MUST read EVERY file in scope + CLAUDE.md + the active checkpoint under docs/checkpoints/ + docs/DESIGN_NOTES.md. The cicchetto/ agent additionally reads its server-side wire-shape counterparts (lib/grappa/{accounts,networks,scrollback,query_windows,user_settings}/wire.ex, lib/grappa_web/controllers/*_json.ex) so wire-shape drift is caught against the source of truth (the cross-surface agent does this exhaustively from a different angle — both lenses are intentional).

Severity gates (codebase review)

Agents report ALL severities (CRITICAL, HIGH, MEDIUM, LOW) so the triage step has full data. Triage focus per vjt direction:

• All CRITICAL — must-fix before next cluster.
• All HIGH — must-fix or formal defer-with-rationale.
• MEDIUM only when the finding is an architecture smell, maintainability hazard, best-practice gap, or evolution risk (i.e. it will hurt a future cluster). Cosmetic MEDIUMs may be noted but are not gates.
• LOW is informational only — collected for later sweep, not blocking.

The triage output (mega-cluster plan in Phase 3) MUST list every CRITICAL + HIGH and every gating MEDIUM with a bucket assignment.

Agent instructions (include in every agent prompt)

Report PROBLEMS ONLY. No praise. For each finding:

### S{N}. Short title
**File:** `path:line`
**Category:** category tag
**Severity:** CRITICAL/HIGH/MEDIUM/LOW
Description.
**Fix:** Concrete suggestion.

What to look for (server-side: irc/, persistence/, lifecycle/, web/, cross-module agents):

• Dead code (unused functions, aliases, requires, modules, unreachable clauses)
• Default arguments via \\ — only genuine config defaults are acceptable (CLAUDE.md "No default arguments via \\")
• Untyped / weakly-typed (map() where struct exists, String.t() where an atom enum exists, missing @spec, :any types, untyped Logger metadata keys not in the allowlist)
• Defensive try/rescue without recovery (CLAUDE.md "Let it crash")
• GenServer state misuse (cross-process state, large state blobs that should be in Ecto, blocking work in init/1 without {:continue, _})
• Process linkage / supervision strategy mismatches (e.g. permanent where transient is right, missing trap_exit when terminate cleanup is needed)
• Phoenix/Ecto rule violations (controllers thick with logic that belongs in contexts, raw Repo.insert/2 without changeset, sandbox not async, leaky abstractions returning map() instead of structs)
• Charset boundary violations (non-UTF-8 in domain, missing encode/decode at IRC boundary)
• IRC parser exhaustiveness (non-exhaustive prefix/command pattern matches, silent fallthrough where {:error, atom} should fire)
• PubSub topic-shape drift (topics not following the grappa:network:{net}/channel:{chan} convention)
• Logger metadata key abuse (calls with non-allowlisted keys, inline interpolation as a workaround instead of extending config/config.exs)
• Stale patterns contradicting CLAUDE.md or DESIGN_NOTES — CLAUDE.md violations are bugs even when the spec or plan asks for the pattern. The spec can inherit a bug from existing code.

What the cicchetto/ agent looks for (TypeScript/SolidJS/PWA-specific):

• SolidJS reactivity bugs: setSignal inside a tracked scope (effect loops), createResource source signal that never invalidates, untrack covering up missing reactivity, createRoot ownership leaks (effects outliving their owner), on(...) defer-flag misuse, module-singleton-signal double-init under vi.resetModules.
• TypeScript strictness: any, as casts that bypass exhaustiveness, missing unknown narrowing on JSON.parse/fetch body reads, optional-chain holes, non-exhaustive switch on closed unions (MessageKind is the canonical case), noUncheckedIndexedAccess violations, @ts-ignore / @ts-expect-error without a comment.
• Wire-shape drift: every cicchetto/src/lib/api.ts type must mirror Grappa.{Accounts,Networks,Scrollback}.Wire + GrappaWeb.*JSON. Missing fields, mismatched optionality, type narrower than server, PubSub event payload shape divergence vs GrappaWeb.GrappaChannel.
• XSS / token leakage / CSP-incompatible patterns: innerHTML, dangerouslySetInnerHTML-equivalent, eval, inline <script> injected at runtime, token in URL bar / window.history, leaked through console.log of full request objects, third-party script loads the nginx CSP rejects.
• A11y baseline: semantic HTML, ARIA roles/labels, keyboard reachability, visible focus, tap targets ≥44pt on iOS, color contrast on dark backgrounds, lang attribute, form-label association.
• Test quality: outcome assertions vs implementation details, module-boundary mocks (vi.mock on ../lib/api/../lib/socket not internal helpers), no buggy-behavior pinning, realistic mock data, production code paths called rather than re-implemented, vi.resetModules() between cases for module-singleton signals.
• PWA shell correctness: manifest.json required fields (name, start_url, display, icons 192/512); SW caching matches documented intent (shell-cache only, no API/WS); cache-bump on deploy (Cache-Control: no-cache from nginx OR versioned SW filename); index.html includes the manifest link.
• CSS (cicchetto/src/themes/*.css) — a first-class product surface, not decoration, and the least-reviewed file in the repo. Look for: rules for one surface scattered across distant bands of the file (locality, which is what lets sibling instances drift apart unnoticed); per-instance overrides re-declaring the same escape from a shared base, where one shared class belongs; controls below the iOS 44pt tap-target minimum; hardcoded colours where a theme var exists (the #75 gallery layers inline vars over these blocks, so a literal silently wins over a user's theme); !important (there are 2 in the whole file — treat any new one as a finding); dead selectors matching no markup; base rules whose semantics every consumer must override to be usable.
• Build + tooling drift: tsconfig.json strict flags pinned; package.json ↔ bun.lock sync; Vite base/root/outDir match nginx + compose.yaml (--profile prod) expectations; vite-plugin-solid is the plugin layer.

What to IGNORE: style preferences, "could be improved" without concrete impact, Phase 5+ deferred work explicitly noted in todo.md or the active checkpoint, Phase 4 UI scope (irssi-shape redesign: keyboard layout, theme system, nick lists, mode indicators, topic bar, mobile ergonomics, voice I/O — flag bugs but not "the layout is messy").

Cross-module agent additions

The cross-module agent searches the ENTIRE lib/ for:

• \\ default arguments in all function signatures
• Application.get_env/get_env!/fetch_env! outside config/ and lib/grappa/application.ex (the documented exception)
• String.to_atom/1 (atom DoS) — should be String.to_existing_atom/1 against an allowlist
• Inline string interpolation in Logger.{info,warning,error,debug} calls where structured-KV metadata would be cleaner
• Bare catch _, _ / rescue _ patterns
• Inter-context call rules (Boundary annotations — flag obvious violations now if they aren't already in mix boundary.find_violations)
• Migration drift (priv/repo/migrations/ order, idempotency, schema_migrations consistency)
• @spec coverage on public context functions; structs returned vs map() leaks
• PubSub topic-shape consistency across modules
• Cross-context import cycles + Boundary violations

The Docker agent owns infra (Dockerfile / compose / scripts / nginx / .env.example) — do NOT duplicate that scope from cross-module. The cross-surface agent owns server↔client wire/event/error/auth/state consistency — do NOT duplicate that scope either.

The consistency agent owns divergence within one side (instance families, stale premises, silent narrowing) — do NOT duplicate it from the scope agents. The split is the axis, not the files: cross-surface asks "do the two sides agree?", consistency asks "do the members of this family on ONE side agree with each other?". Both may legitimately read the same file.

Argument: architecture

Concern-based structural review. 6 parallel background agents, one per CONCERN:

Agent	Concern
Abstraction boundaries	Leaky abstractions, contexts reaching into each other's schemas, return types forcing callers to parse. Includes the server↔client boundary: does cicchetto's api.ts consume domain types or re-shape on the client?
Responsibility & cohesion	ONE job per context? God modules, feature envy, misplaced logic (controller doing IRC parsing, schema doing PubSub broadcast, cicchetto component holding domain state instead of consuming it from lib/networks.ts).
Duplication	Same problem solved differently, copy-paste with tweaks, parallel structures that drift. Canonical cases: wire-shape unification across REST/PubSub/Channel/Phase-6 listener AND across server Wire modules ↔ cicchetto api.ts types.
Dependency architecture	Dependency direction (web → contexts → schemas; cicchetto components → lib/*.ts stores → api.ts + socket.ts), import cycles (TS module cycles + Elixir Boundary), hidden coupling via Application.put_env or module-level mutable state on the client, supervision-tree ordering invariants.
Type system leverage	Atoms-or-typed-literals (CLAUDE.md "never untyped strings"), structs over maps, custom Ecto types over :map + key conventions. On the client: TS strict + noUncheckedIndexedAccess, branded/opaque types over bare string for keys (the ChannelKey pattern), exhaustive switch over closed unions, unknown narrowing instead of any. @spec discipline (Dialyxir :underspecs) on the server.
Extension & maintainability	Adding a new IRC kind = touching 15 files (server kind enum + Wire + cicchetto MessageKind + ScrollbackPane render + tests)? Adding a new context = touching the supervision tree? Config sprawl across config/*.exs AND cicchetto/{tsconfig,vite,vitest,biome}.json? Test architecture (lib/ mirror vs outcome-tested; client __tests__/ colocated).

Each agent reads files across the ENTIRE codebase (server + cicchetto) following the concern.

Agent instructions (include in every agent prompt)

Report FINDINGS, not line-level bugs. For each:

### A{N}. Short title
**Concern:** which of the 6
**Scope:** modules / files involved
**Problem:** structural issue
**Impact:** what breaks, drifts, or gets harder
**Recommendation:** concrete path forward

Severity: CRITICAL (blocks correctness/safety), HIGH (significant maintenance burden), MEDIUM (tech debt), LOW (improvement opportunity).

After all agents complete

1. Collect all findings from all agents.
2. Deduplicate (cross-module agent may overlap with scope agents).
3. Trajectory review (codebase reviews only): read the active checkpoint, docs/todo.md, README.md "Phases" section, and docs/project-story.md. Write a ## Trajectory section answering: what did we build recently, does it serve the core mission (always-on IRC bouncer + REST/WS surface + downstream IRCv3 listener), what's stalling, any observation items due, risk check, and a 2-3 sentence direction recommendation. See docs/reviewing.md section 3 for the full question list.
4. Compile into a single review document:
   • docs/reviews/codebase/YYYY-MM-DD-codebase-review.md for codebase reviews
   • docs/reviews/architecture/YYYY-MM-DD-architecture-review.md for architecture reviews
5. Summary table: severity counts by module/concern.
6. Update active checkpoint with review stats.
7. Present top findings + trajectory assessment to the user.