docs: contributor review rubric, skills, PR template, and dev hooks

.githooks/pre-commit

@@ -0,0 +1,23 @@
+#!/bin/sh
+# Amber pre-commit hook: fast format check only.
+# Keep this fast — no shards install, no ameba, no specs.
+# Bypass with: SKIP_HOOKS=1 git commit ...
+# Install with: bin/setup-dev  (sets core.hooksPath to .githooks)
+
+if [ "${SKIP_HOOKS:-0}" = "1" ]; then
+  echo "pre-commit: SKIP_HOOKS=1 set, skipping format check."
+  exit 0
+fi
+
+echo "pre-commit: running 'crystal tool format --check'..."
+if ! crystal tool format --check; then
+  echo
+  echo "pre-commit: FAILED — code is not formatted."
+  echo "  Fix it with:  crystal tool format"
+  echo "  Then re-stage and commit again."
+  echo "  Bypass (not recommended):  SKIP_HOOKS=1 git commit ..."
+  exit 1
+fi
+
+echo "pre-commit: format OK."
+exit 0

.githooks/pre-push

@@ -0,0 +1,52 @@
+#!/bin/sh
+# Amber pre-push hook: mirror the per-PR CI checks locally before spending CI minutes.
+# Runs (in order): format check, ameba (if built), full spec suite (needs no DB).
+# Does NOT run the granite build spec (it needs a postgres service) — see reminder below.
+# Bypass with: SKIP_HOOKS=1 git push ...
+# Install with: bin/setup-dev  (sets core.hooksPath to .githooks)
+
+if [ "${SKIP_HOOKS:-0}" = "1" ]; then
+  echo "pre-push: SKIP_HOOKS=1 set, skipping local CI checks."
+  exit 0
+fi
+
+status=0
+
+echo "pre-push: running 'crystal tool format --check'..."
+if ! crystal tool format --check; then
+  echo "pre-push: FAILED — code is not formatted. Fix with:  crystal tool format"
+  status=1
+fi
+
+echo
+if [ -x ./bin/ameba ]; then
+  echo "pre-push: running './bin/ameba'..."
+  if ! ./bin/ameba; then
+    echo "pre-push: FAILED — ameba reported issues."
+    status=1
+  fi
+else
+  echo "pre-push: ./bin/ameba not found — run 'shards install' to get bin/ameba. Skipping lint."
+fi
+
+echo
+echo "pre-push: running 'crystal spec' (main suite — no DB required)..."
+if ! crystal spec; then
+  echo "pre-push: FAILED — specs are red."
+  status=1
+fi
+
+echo
+echo "pre-push: reminder — for the FULL CI-matching check (incl. the postgres-backed"
+echo "          granite build spec), run:  bin/amber_spec"
+
+if [ "$status" -ne 0 ]; then
+  echo
+  echo "pre-push: one or more checks failed. Push aborted."
+  echo "  Bypass (not recommended):  SKIP_HOOKS=1 git push ..."
+  exit 1
+fi
+
+echo
+echo "pre-push: all local checks passed."
+exit 0

.github/CONTRIBUTING.md

@@ -123,3 +123,19 @@ Both issue lists are sorted by total number of comments. While not perfect, numb
 * Include thoughtfully-worded, well-structured Crystal specs in the `./spec` folder.
 * Treat `describe` as a noun or situation.
 * Treat `it` as a statement about state or how an operation changes state.
+
+## Code review & quality bar
+
+A wave of AI-authored PRs taught us that the same failure shapes recur: reflected XSS, "vulnerabilities" with no reachable sink, incomplete fixes, inert dead code, and perf changes that quietly alter behavior while happy-path specs stay green. To catch these up front we maintain a shared rubric, a list of intentional design trade-offs, and two skills that walk you through self-review and maintainer review.
+
+* **[contributing/REVIEW_RUBRIC.md](./contributing/REVIEW_RUBRIC.md)** — the source of truth: the severity framework, per-change-type quality bars, and how we value enhancements vs. new features. Read this before opening or reviewing a PR.
+* **[contributing/DESIGN_DECISIONS.md](./contributing/DESIGN_DECISIONS.md)** — deliberate trade-offs reviewers should DISCOUNT (so you don't re-litigate settled decisions or flag them as bugs).
+* **[contributing/skills/pre-review/SKILL.md](./contributing/skills/pre-review/SKILL.md)** — contributor skill: self-review your own change against the rubric before you push.
+* **[contributing/skills/review/SKILL.md](./contributing/skills/review/SKILL.md)** — maintainer skill: review an incoming PR against the rubric.
+
+See **[contributing/README.md](./contributing/README.md)** for how to load the skills and run the checks.
+
+**Before you push:**
+
+* Run `bin/setup-dev` once. It installs the local git hooks and links the skills into your `.claude/` so your assistant can load them.
+* Run `bin/amber_spec` to match CI. It runs `bin/ameba`, `crystal tool format --check`, `crystal spec`, and `crystal spec ./spec/build_spec_granite.cr` — the same checks CircleCI runs (the Granite build spec needs a local PostgreSQL; CI pins Crystal 1.9.2).

.github/PULL_REQUEST_TEMPLATE

@@ -13,3 +13,35 @@
 ### Possible Drawbacks
 
 <!-- What are the possible side-effects or negative impacts of the code change? -->
+
+### Type of change
+
+<!-- Check all that apply. Pick the type that sets your correctness bar (see the rubric, linked below). -->
+
+- [ ] Bugfix
+- [ ] Security
+- [ ] Performance
+- [ ] Refactor (behavior-preserving)
+- [ ] Feature / default-change
+- [ ] Docs
+
+### Quality checklist
+
+- [ ] Ran `bin/amber_spec` locally and it passed
+- [ ] `crystal tool format --check` is clean
+- [ ] `bin/ameba` is clean
+- [ ] Added tests, including edge cases (dotfiles, empty/trailing segments, malformed input, non-development env)
+- [ ] The PR description above matches what the code actually does
+- [ ] Checked this is not a duplicate of a recently merged/open PR
+
+### If this is a security / performance / feature PR
+
+<!-- Fill in only the block(s) that apply. -->
+
+- **Security:** State the severity using the three levers — reachability (a real attacker-reachable, not-already-mitigated sink), threat model (network-facing vs local dev CLI), and environment gating (prod vs development-only). Include an **adversarial spec** that sends the malicious input and asserts it is neutralized, not just that the happy path still works.
+- **Performance:** Prove behavior is **identical** with a differential edge-case table (normal path, dotfile, trailing slash, empty segment) showing old and new agree. Green happy-path specs are not proof — add the divergent-input specs.
+- **Feature / default change:** Link the issue or discussion where this was agreed to belong in core. Risky defaults (new default middleware, on-by-default headers, changed pipeline) must be **opt-in** and ship in the CLI app template, not bolted onto existing apps — and note the semver bump (minor at minimum, major if it can break existing apps).
+
+---
+
+See [`.github/contributing/REVIEW_RUBRIC.md`](contributing/REVIEW_RUBRIC.md) for the severity framework and per-type bars, and run the pre-review skill ([`.github/contributing/skills/pre-review/SKILL.md`](contributing/skills/pre-review/SKILL.md)) before you open this PR.

.github/contributing/DESIGN_DECISIONS.md

@@ -0,0 +1,87 @@
+# Design Decisions & Known Trade-offs
+
+A living registry of **intentional** design decisions in Amber that look like bugs but are not. Reviewers should **discount** these — do not flag them as defects, and do not "fix" them in a PR without a separate, explicit discussion.
+
+This file complements the [REVIEW_RUBRIC.md](REVIEW_RUBRIC.md), which owns the severity framework and per-type bars. This file owns the *context*: the deliberate choices the rubric assumes you already know.
+
+## How to use it
+
+- **Before flagging** something as a bug, missing default, or anti-pattern, scan this list for a matching entry. If it matches, it is by design — don't flag it.
+- A finding that *contradicts* an entry here needs strong, specific evidence (a concrete reachable failure, a differential test). "Looks wrong" is not enough.
+- If an entry is genuinely wrong or stale, that is a real change: open it as its own PR/discussion, update this file in the same PR, and link the reasoning. Don't silently regress a documented decision.
+
+## How to add an entry
+
+Add a new `###` section using the template at the bottom. Each entry has three parts:
+
+1. **Decision** — what the code does, with the relevant repo-relative path.
+2. **Rationale** — why it's that way.
+3. **Don't flag** — the specific shapes of false-positive review comment this entry preempts.
+
+Keep entries short and link to source. When you add an entry, you are saving every future reviewer the trip you just took.
+
+---
+
+## Entries
+
+### App defaults live in the CLI app template, not framework core
+
+- **Decision:** Default configuration and wired-up behavior for a *new app* live in the generated app template under `src/amber/cli/templates/app/...` (e.g. `config/routes.cr.ecr`, `config/application.cr.ecr`, `config/environments/`). Framework core ships the *capability*; the template ships the *default*.
+- **Rationale:** Core stays unopinionated and embeddable. What a fresh `amber new` produces is the product surface where defaults belong, and where users can edit them.
+- **Don't flag:** "Core doesn't set a sensible default for X" — that is often correct. Check the app template first. Conversely, a PR that adds a new pipe/feature but never wires it into `config/routes.cr.ecr` (or the relevant template file) has shipped inert dead code — *that* is worth flagging.
+
+### crystal-db connection pool is already concurrency-safe
+
+- **Decision:** Database access goes through `crystal-db`'s connection pool, which is concurrency-safe on its own.
+- **Rationale:** The pool already manages checkout/checkin and fiber safety. Adding our own `Mutex` around it serializes what the pool deliberately parallelizes.
+- **Don't flag:** "Database access isn't locked / needs a mutex / has a race." Do not re-add manual locking around pool checkout. If you think there's a real race, prove it sits *outside* the pool's guarantees.
+
+### The dev error page and the bare HTML error page are two different paths
+
+- **Decision:** The rich development error page is rendered by the `exception_page` shard. The hand-written HTML 500 page in `src/amber/controller/error.cr` is the **non-development** fallback only.
+- **Rationale:** Development gets a detailed, interactive page from the shard; production gets a minimal, dependency-light page.
+- **Don't flag:** Don't critique the dev page's markup/styling against `src/amber/controller/error.cr` — that file isn't what you see in development. Conversely, *do* scrutinize `error.cr` for the production threat model: it must escape any interpolated value (e.g. an `Exception#message` that can embed a request path) because that path is reflected to non-development users. Know which page you're looking at before commenting.
+
+### Canonical shell-out is `Process.run(cmd, [args])`, no shell
+
+- **Decision:** Shelling out uses `Process.run(cmd, [args])` with an argv array and **no shell**, not `system("string ...")` or `Process.run(string, shell: true)`. See the settled pattern in `src/amber/cli/commands/exec.cr` (e.g. `Process.run("cp", [_filename, @filename])`).
+- **Rationale:** Passing an argv array avoids shell interpolation entirely, which sidesteps command injection from interpolated values.
+- **Don't flag:** Existing `Process.run(cmd, [args])` calls as "should use a shell" — they shouldn't. *Do* flag any *new* `system("#{x} ...")` or `shell: true` with interpolated input, and steer it to the argv form.
+
+### Non-clobbering header default idiom: `headers["X"] ||= value`
+
+- **Decision:** When setting a default response (or request) header, the idiom is `headers["X"] ||= value`, which only sets it if the user hasn't (e.g. `request.headers["Content-Type"] ||= "..."` in `src/amber/cli/templates/app/spec/request_helper.cr.ecr`).
+- **Rationale:** Defaults must not clobber values a user explicitly set. `||=` respects the caller's intent.
+- **Don't flag:** "This header assignment is conditional / looks like it might not run." The `||=` is intentional. *Do* flag an unconditional `headers["X"] = value` that overwrites a user-supplied default header.
+
+### Pipes are auto-required but inert until plugged into a pipeline
+
+- **Decision:** `src/amber.cr` does `require "./amber/pipes/**"`, so every pipe file compiles and is available. But a pipe does **nothing** until it's added to a pipeline (typically in the app's `config/routes.cr.ecr` via `plug`).
+- **Rationale:** Auto-require keeps pipes discoverable without manual `require` lists; pipeline wiring stays explicit and per-app so behavior is opt-in.
+- **Don't flag:** "The pipe is auto-required, so it's active" — it isn't. "The pipe exists" ≠ "the pipe runs." A PR that adds a pipe must also wire it into the relevant pipeline/template to have any effect; an unplugged pipe is dead code, not a working feature.
+
+### Single-source registries — don't hard-code duplicate lists
+
+- **Decision:** Lists that can drift are defined once and referenced everywhere. The content-type / extension registry lives in `Content::TYPE` (`src/amber/controller/helpers/responders.cr`); even the extension regex is *derived* from it (`TYPE_EXT_REGEX = /\.(#{TYPE.keys.join("|")})$/`), and the router reuses that same constant.
+- **Rationale:** One source of truth means adding/changing a type updates every consumer at once. Parallel hand-maintained lists silently diverge.
+- **Don't flag:** Code that reads from `Content::TYPE` / `TYPE_EXT_REGEX` instead of inlining a literal list — that's the right move. *Do* flag a PR that introduces a *second*, hard-coded copy of an extension/content-type list that can drift from the registry.
+
+### Crystal version policy: shard allows `>=1.0,<2`; CI pins 1.9.2 for specs
+
+- **Decision:** `shard.yml` declares `crystal: ">= 1.0.0, < 2.0"`. CI runs the spec suite on `crystallang/crystal:1.9.2` (the lint/`ameba` job uses `:latest`).
+- **Rationale:** Amber supports a broad Crystal range; specs are pinned to a known-good version for reproducible CI.
+- **Don't flag:** Code that avoids newer-stdlib-only APIs or works around an older signature — that may be deliberate range support, not a missed modernization. Conversely, *do* flag a PR that relies on behavior or APIs only present in Crystal newer than 1.9.2 (it can pass local dev yet break CI/older users) — verify against the declared range, don't assume the newest stdlib.
+
+---
+
+## Template for new entries
+
+Copy this block, fill it in, and place it in the **Entries** section.
+
+```markdown
+### <Short imperative title of the decision>
+
+- **Decision:** <What the code does + repo-relative path(s), e.g. `src/amber/...`.>
+- **Rationale:** <Why it's intentionally this way.>
+- **Don't flag:** <The specific false-positive review comment(s) this preempts. Optionally: and DO flag <the real failure shape this is sometimes confused with>.>
+```

.github/contributing/README.md

@@ -0,0 +1,47 @@
+# Contributor review infrastructure
+
+This folder holds the shared review docs and skills for Amber. They exist because multiple AI coding assistants now open PRs, and a recurring set of failure shapes (reflected XSS, "vulnerabilities" with no reachable sink, incomplete fixes, inert dead code, perf changes that silently alter behavior) need to be caught before merge — by contributors self-reviewing and by maintainers reviewing.
+
+## What's in here
+
+| File | What it is |
+| --- | --- |
+| [REVIEW_RUBRIC.md](./REVIEW_RUBRIC.md) | The source of truth: severity framework, per-change-type quality bars, and how we value enhancements vs. new features. Everything else links here instead of duplicating it. |
+| [DESIGN_DECISIONS.md](./DESIGN_DECISIONS.md) | Deliberate trade-offs reviewers should DISCOUNT — settled decisions, so you don't re-flag them as bugs. |
+| [skills/pre-review/SKILL.md](./skills/pre-review/SKILL.md) | Contributor skill. Self-review your own change against the rubric before you push. |
+| [skills/review/SKILL.md](./skills/review/SKILL.md) | Maintainer skill. Review an incoming PR against the rubric. |
+
+## Loading the skills
+
+The skills are intentionally **not committed under `.claude/`** and are **not auto-loaded** — they live here, version-controlled with the code they describe. To make your assistant pick them up, link or copy them into your personal `.claude/skills/`:
+
+* **Recommended:** run `bin/setup-dev` once from the repo root. It installs the local git hooks and symlinks the skills into `.claude/skills/`.
+* **Manual:** symlink (or copy) each skill folder into your `.claude/skills/`, for example:
+
+  ```sh
+  mkdir -p .claude/skills
+  ln -s ../../.github/contributing/skills/pre-review .claude/skills/pre-review
+  ln -s ../../.github/contributing/skills/review .claude/skills/review
+  ```
+
+`.claude/` is gitignored, so these links stay local to your checkout and are never committed.
+
+## Running the checks (match CI)
+
+CI is CircleCI (`.circleci/config.yml`). Every change runs three jobs:
+
+1. **ameba-test** — `shards install` then `bin/ameba` (lint).
+2. **amber-specs** — `shards install` then `crystal spec` (full suite, **Crystal 1.9.2**, no database services).
+3. **granite-test** — `shards install` then `crystal spec spec/build_spec_granite.cr` (needs a **PostgreSQL** service: DB `granite_test`, user `postgres`, password `postgres`).
+
+To reproduce all of it locally in one shot, run the existing script from the repo root:
+
+```sh
+bin/amber_spec
+```
+
+It runs `bin/ameba`, `crystal tool format --check`, `crystal spec`, and `crystal spec ./spec/build_spec_granite.cr`. The Granite build spec needs a local PostgreSQL running; CI pins Crystal 1.9.2 for the spec suite, so if you see version-specific failures, test against 1.9.2.
+
+## Where to start
+
+Read [REVIEW_RUBRIC.md](./REVIEW_RUBRIC.md) first, then skim [DESIGN_DECISIONS.md](./DESIGN_DECISIONS.md) so you don't re-litigate settled trade-offs.