diff --git a/.githooks/pre-commit b/.githooks/pre-commit new file mode 100755 index 000000000..3c25eb34b --- /dev/null +++ b/.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 diff --git a/.githooks/pre-push b/.githooks/pre-push new file mode 100755 index 000000000..14bd044ed --- /dev/null +++ b/.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 diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 6d1c39d2a..13aa76231 100644 --- a/.github/CONTRIBUTING.md +++ b/.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). diff --git a/.github/PULL_REQUEST_TEMPLATE b/.github/PULL_REQUEST_TEMPLATE index c97ebc981..dadb23133 100644 --- a/.github/PULL_REQUEST_TEMPLATE +++ b/.github/PULL_REQUEST_TEMPLATE @@ -13,3 +13,35 @@ ### Possible Drawbacks + +### Type of change + + + +- [ ] 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 + + + +- **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. diff --git a/.github/contributing/DESIGN_DECISIONS.md b/.github/contributing/DESIGN_DECISIONS.md new file mode 100644 index 000000000..e07a7d713 --- /dev/null +++ b/.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 +### + +- **Decision:** +- **Rationale:** +- **Don't flag:** .> +``` diff --git a/.github/contributing/README.md b/.github/contributing/README.md new file mode 100644 index 000000000..66a551b71 --- /dev/null +++ b/.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. diff --git a/.github/contributing/REVIEW_RUBRIC.md b/.github/contributing/REVIEW_RUBRIC.md new file mode 100644 index 000000000..7d57d78bc --- /dev/null +++ b/.github/contributing/REVIEW_RUBRIC.md @@ -0,0 +1,144 @@ +# Amber Review Rubric + +The canonical, single source of truth for how we judge a change to Amber. Everything else (the pre-review skill, the review skill, the PR template, `.github/CONTRIBUTING.md`) links here and adds only its own lens. If those documents and this one ever disagree, **this document wins** — fix the other. + +## 1. Purpose & how to use + +Multiple AI coding assistants now open PRs against Amber. A 2026-06-15 wave surfaced a small set of repeatable failure shapes (over-stated severities, incomplete fixes, "perf" changes that quietly altered behavior, dead/dangerous defaults). This rubric exists so both sides catch them before they cost review cycles. + +- **Contributors — run this before opening a PR.** Load the pre-review skill (`.github/contributing/skills/pre-review/SKILL.md`), self-grade against this rubric, fix what you find, and fill the PR template honestly. The cheapest review note is the one you never trigger. +- **Maintainers — run this before merging.** Load the review skill (`.github/contributing/skills/review/SKILL.md`) and grade the diff against the matching PR-type bar (§3), apply the discount levers (§2) before you assign a severity, and verify mergeability (§6) plus local/CI checks (§7). + +Use second person, cite the actual file and line, and prefer "here is the reachable sink" over "this looks scary." + +## 2. Severity framework — "what is important" + +Assign a tier to every issue you raise. Then **discount it** with the three levers below. A raw finding is a hypothesis; the severity is what survives the levers. + +### Tiers + +| Tier | Meaning | Concrete Amber example (2026-06-15 wave) | +|---|---|---| +| **Critical** | Remote, unauthenticated, no preconditions: RCE, auth bypass, secret/data disclosure at scale. | (None confirmed in the wave — reserve this tier; it triggers an immediate fix/release, not a normal review.) | +| **High** | Real exploit with realistic preconditions, or in a default-on/network-facing path. | Reflected XSS: `src/amber/controller/error.cr` interpolates an unescaped `@ex.message` into the HTML 500 page (`RouteNotFound` embeds the request path). Network-facing — **but see Environment gating below**, which is why it lands High, not Critical. | +| **Medium** | Exploitable only with significant preconditions, or limited blast radius, or needs an unusual config. | A malformed-cookie code path that raises an unexpected exception type and produces a confusing 500, where the request is already authenticated/contained. | +| **Low** | Real but minor: needs local access, the operator is the attacker, or impact is cosmetic/hardening. | Command injection in `src/amber/cli/commands/encrypt.cr` — `system("#{options.editor} #{unencrypted_file}")` runs a shell. Real, but it is a **local dev CLI**: the operator already controls `$EDITOR` and the filesystem. | +| **Informational** | Style, naming, doc nits, defense-in-depth suggestions with no live vector. | "Consider escaping here too even though this branch is dev-only." Note it; do not block on it. | + +### The three discount levers (apply before finalizing severity) + +**(a) Reachability — is there a real attacker-reachable sink, and is it already mitigated?** +Trace the finding to a sink an attacker can actually drive, *then* check existing mitigations on the path before you assign severity. +- Worked example (the "DoS" that was not one): `MessageVerifier` / `MessageEncryptor` raise `IndexError` on malformed cookies. Sounds like a DoS — but every attacker-reachable caller already rescues it (`SignedStore#verify`, `EncryptedStore#verify_and_decrypt`), and the top-level `Pipe::Error` catches the rest. Reachable sink with full existing mitigation -> **downgrade to Informational/Low**, not High. +- Checklist: Can an unauthenticated request reach it? Does the immediate caller `rescue`? Is there a top-level `Pipe::Error` (or framework handler) that already contains it? If yes, say so explicitly and discount. + +**(b) Threat model — local dev CLI vs network-facing.** +Who is the attacker relative to who already has control? A network endpoint trusts no one; a `bin/amber`/CLI command already runs as the operator. +- The `encrypt.cr` command injection is real code-smell but **Low**, because the person who can set `$EDITOR` and write the YAML file is the same person the shell would run as. Network-facing equivalents jump several tiers. + +**(c) Environment gating — production vs development-only.** +Where the dangerous branch only executes outside `development`, severity rises in prod and the dev path is fine — and vice versa. +- The `error.cr` XSS lives in the non-development branch (`else "...
#{@ex.message}
..."`); the development branch renders the safe `Amber::Exceptions::Page`. So it is a **production** bug (High), not a dev annoyance. Gating can cut both ways: a finding that only fires in `development?` is usually Low/Informational because production never runs it. + +## 3. Correctness bars by PR type + +Pick the type that matches the change. State which one you are reviewing/submitting against. The bar is **evidence**, not vibes. + +### Bugfix +- A failing spec that **reproduces the bug** (red before the fix), now green. +- The fix targets the root cause, not the symptom; no unrelated drive-by edits. +- A note on blast radius: what else calls this path? + +### Security +A security PR must clear **all** of: +- **Real + reachable + not-already-mitigated.** Apply §2(a)–(c). If a top-level `Pipe::Error` or caller `rescue` already contains it, say why your fix still matters. +- **Complete fix — no leftover vector.** Counter-example from the wave: the proposed `encrypt.cr` fix escaped the *file* but left the *editor* injectable, so the shell vector survived. The repo already settled the correct pattern in `exec.cr`: `Process.run(cmd, [args])` (no shell) — prefer eliminating the shell over escaping. +- **Narrow fix — no over-broad rescue or auth bypass.** Do not wrap a wide block in `rescue` (it hides unrelated failures) and do not loosen an auth/verification check to make the symptom disappear. +- **Adversarial spec.** A spec that sends the malicious input (the XSS payload, the malformed cookie, the injected `$EDITOR`) and asserts it is neutralized — not just that the happy path still works. + +### Performance +Perf changes are the highest-risk "looks fine" category. A perf PR must be **provably behavior-identical**. +- **Differential edge-case table required.** Enumerate inputs where the old and new implementation could diverge and show they agree. Template: + + | Input | Old behavior | New behavior | Same? | + |---|---|---|---| + | normal path `/users/1` | route key `users/1` | route key `users/1` | yes | + | dotfile `/.well-known/x` | ... | ... | **must verify** | + | trailing slash `/users/` | ... | ... | **must verify** | + | extension-only / empty segment | ... | ... | **must verify** | + + Worked example (the wave): a PR replaced an anchored regex with `File.extname` in the router. **Not** semantically identical — dotfiles and trailing-slash inputs diverge, building a corrupted route key. CI stayed green because the specs were happy-path only. +- **"Green specs != correct."** If the existing suite only covers the happy path, green tells you nothing about the edge cases. Add the differential specs that exercise the divergent inputs. +- A benchmark (before/after numbers, method, hardware) justifying the change is worth making at all. + +### Refactor +- **Behavior-preserving by definition.** The existing suite must pass unchanged; if you had to edit a spec's *assertions*, it is not a pure refactor — re-file it. +- No public API or default changes ride along (those are Feature/default-change). +- If behavior could subtly shift, add the differential specs from the Performance bar. + +### Feature / default-change +- Tests for the new behavior **and** its edge cases. +- Docs/changelog updated; the PR description matches what the code does (§5). +- **Default/behavior changes get extra scrutiny — see §4.** New default middleware, a changed pipeline, a new on-by-default header all change every existing app on upgrade. + +## 4. Enhancement vs new feature (our values) + +- **Enhancement** = improves something Amber already does (faster, clearer error, better default ergonomics) without changing the framework's surface contract. **Bar:** the matching §3 type bar (usually Bugfix/Refactor/Performance) plus evidence it does not change behavior for existing apps. Welcome as a PR. +- **New feature** = adds surface area (new pipe, new CLI command, new config knob, new template default). **Bar:** the Feature/default-change bar, *plus* design buy-in. A new feature should usually **start as an issue or discussion**, not a drive-by PR — so we agree it belongs in core before you build it. + +**Why default/behavior changes get extra scrutiny (and a semver note):** +- They affect every app on upgrade, silently. Worked example (the wave): a SecureHeaders pipe PR added a pipe but never plugged it into the app template (`config/routes.cr.ecr` in the generated app) -> **inert dead code**; and it shipped **HSTS on-by-default**, which can hard-break HTTP for real sites. App defaults belong in the **CLI app template**, not bolted onto an existing app's runtime, and "on by default" needs an explicit safety argument. +- **Semver:** a new default/behavior change is **not patch-safe**. Changing a default or pipeline is a **minor** at minimum (new behavior) and a **major** if it can break an existing app's requests. Call out the bump in the PR. + +## 5. AI-assisted contributions + +AI-assisted PRs are **welcome and encouraged** — this whole rubric assumes them. The expectations: + +- [ ] **Description matches the code.** The PR text describes what the diff *actually does*, not what the assistant intended. Mismatch is the single most common wave failure — reviewers should grep the diff against the claims. +- [ ] **One concern per PR, scoped diff.** No bundling a security fix with a refactor with a rename. Small diffs review faster and revert cleaner. +- [ ] **Tests including the edge cases assistants tend to skip.** Dotfiles, empty/trailing segments, malformed input, non-development env — the exact spots where "green happy-path specs" hide divergence (§3 Performance). +- [ ] **No duplication.** Check the change against recently merged PRs before opening; assistants frequently re-propose something already landed or already settled (e.g. the `Process.run` no-shell pattern in `exec.cr`). +- [ ] **Learning notes welcome if accurate.** `.jules/*`-style notes are fine to include when they are correct and contain **nothing machine-specific** (no absolute local paths, hostnames, usernames, tokens, or env dumps). + +## 6. Mergeability mechanics + +Before review, confirm the PR can actually merge: + +```sh +gh pr view --json mergeable,mergeStateStatus,reviewDecision +``` + +- `mergeable: CONFLICTING` -> rebase/resolve; not reviewable yet. +- `mergeStateStatus: BLOCKED` -> **usually a required review/approval, not a merge conflict.** Do not chase a phantom conflict; check `reviewDecision`. +- `reviewDecision: REVIEW_REQUIRED` / `CHANGES_REQUESTED` -> exactly what it says. +- **Local checks must pass before a human spends review time.** A PR with failing `bin/ameba` or red specs gets bounced back, not reviewed (§7). + +## 7. Running the checks locally to match CI + +CI is **CircleCI** (`.circleci/config.yml`). Every PR runs three jobs — match them locally before you push: + +1. **ameba-test** (lint; image `crystallang/crystal:latest`): + ```sh + shards install # produces bin/ameba (dev dependency, ~> 1.5.0) + bin/ameba + ``` +2. **amber-specs** (full suite; image `crystallang/crystal:1.9.2`; **no database services**): + ```sh + crystal spec + ``` + CI pins **Crystal 1.9.2** for specs (shard.yml allows `>= 1.0.0, < 2.0`); if it passes for you on a newer compiler but fails in CI, suspect the version pin. +3. **granite-test** (`crystal spec spec/build_spec_granite.cr`; **needs a postgres service**): + ```sh + crystal spec ./spec/build_spec_granite.cr + ``` + Requires a reachable Postgres with DB `granite_test`, user `postgres`, password `postgres`. Without it, this spec cannot pass — start a local Postgres first. + +(The `osx_tests` job runs only for a manually triggered pre-release, not per PR.) + +**Shortcuts (use these, don't reinvent them):** +- `bin/amber_spec` runs all four checks in order: `./bin/ameba`, `crystal tool format --check`, `crystal spec`, `crystal spec ./spec/build_spec_granite.cr`. Run it before every push. +- The git hooks in `.githooks/` (installed via `bin/setup-dev`) run the fast checks automatically on commit/push so you catch failures before CI does. + +## 8. See also + +- **`.github/contributing/DESIGN_DECISIONS.md`** — deliberate trade-offs in Amber that reviewers should **discount** (do not flag a settled decision as a bug). Read it before raising a finding that smells like "why is it done this way." diff --git a/.github/contributing/skills/pre-review/SKILL.md b/.github/contributing/skills/pre-review/SKILL.md new file mode 100644 index 000000000..3b0fac9d3 --- /dev/null +++ b/.github/contributing/skills/pre-review/SKILL.md @@ -0,0 +1,138 @@ +--- +name: pre-review +description: Self-review checklist to run BEFORE opening or updating a PR against Amber. Use it whenever you (or an AI assistant) are about to push a branch and open or refresh a pull request — it grades your own change against the Amber Review Rubric so CI is green and a maintainer's first pass finds nothing avoidable. Use when about to open a PR, update an existing PR, or check whether a change is ready to submit. +--- + +# Pre-review: grade your own PR before a maintainer does + +You are about to open or update a PR against Amber. Run this procedure first. The +cheapest review note is the one you never trigger. The rubric below is the source +of truth — this skill is just the contributor lens on it. + +- Severity framework, per-type bars, enhancement-vs-feature values: **[REVIEW_RUBRIC.md](../../REVIEW_RUBRIC.md)** (canonical — do not duplicate it here). +- Intentional trade-offs you should NOT "fix": **[DESIGN_DECISIONS.md](../../DESIGN_DECISIONS.md)**. Scan it before touching anything that looks like a bug — you may be undoing a settled decision. + +Work the steps in order. Don't skip a checkbox because it "looks obvious" — that +is exactly where the 2026-06-15 wave failed. + +## 1. Classify your PR type + +Pick exactly one and write it in the PR description. The type sets your bar (rubric §3). + +- [ ] **Bugfix** — fixes incorrect behavior. +- [ ] **Security** — closes a real, reachable vulnerability. +- [ ] **Performance** — same behavior, faster. +- [ ] **Refactor** — same behavior, cleaner code. +- [ ] **Feature / default-change** — new surface area or a changed default/pipeline/header. + +If it's an **enhancement vs a new feature**, decide which (rubric §4). A *new feature* +(new pipe, CLI command, config knob, template default) should usually **start as an +issue/discussion** for design buy-in before you build it — not a drive-by PR. + +## 2. Make the description match the code + +This is the single most common wave failure: the PR text describes intent, not the diff. + +- [ ] Read your own `git diff` and write the description from what the code **actually does**. +- [ ] Grep your claims against the diff — every "fixes X", "adds Y", "no behavior change" + must be visible in the changed lines. +- [ ] Remove aspirational language ("should", "intended to") for things the code doesn't do. + +## 3. One concern, minimal diff + +- [ ] Exactly one concern. No bundling a security fix with a refactor with a rename. +- [ ] No unrelated drive-by edits, reformatting of untouched code, or dependency bumps. +- [ ] Smaller is better — small diffs review faster and revert cleaner. + +## 4. Meet your per-type bar (rubric §3) + +State which bar you're submitting against, then clear it with **evidence, not vibes**. + +**Bugfix** — a spec that's **red before the fix, green after**; fix the root cause, not the symptom; note what else calls this path. + +**Refactor** — the existing suite passes **unchanged**. If you had to edit a spec's +*assertions*, it is not a pure refactor — reclassify it. No public-API/default changes ride along. + +**Performance** — perf is the highest-risk "looks fine" category. You must prove it's +**behavior-identical**. Fill in a differential edge-case table in the PR description and +verify each row with a real spec — green happy-path specs tell you nothing about edges: + +| Input | Old behavior | New behavior | Same? | +|---|---|---|---| +| normal path `/users/1` | route key `users/1` | route key `users/1` | yes | +| dotfile `/.well-known/x` | … | … | **must verify** | +| trailing slash `/users/` | … | … | **must verify** | +| empty / extension-only segment | … | … | **must verify** | + +(The wave's perf PR swapped an anchored regex for `File.extname`; dotfiles and trailing +slashes diverged and built a corrupted route key, but CI stayed green on happy-path specs.) +Include a before/after benchmark (method + hardware) justifying the change. + +**Security** — clear **all** of these and write an **honest severity self-assessment** +using the three levers (rubric §2): **(a) reachability** — name the real +attacker-reachable sink AND state whether a caller `rescue` or top-level `Pipe::Error` +already mitigates it; **(b) threat model** — local dev CLI (operator is the attacker → Low) +vs network-facing; **(c) environment gating** — production path vs `development?`-only. +Don't over-state: the wave's "DoS" was already rescued at every reachable caller, and the +`encrypt.cr` command injection was a **Low** local-CLI issue. + +- [ ] Real + reachable + not-already-mitigated (apply levers a–c above). +- [ ] **Complete fix** — no leftover vector. Prefer eliminating the shell + (`Process.run(cmd, [args])`, the settled `exec.cr` pattern) over escaping one input. +- [ ] **Narrow fix** — no over-broad `rescue` hiding unrelated failures, no loosened auth check. +- [ ] **Adversarial spec** — send the actual malicious input (XSS payload, malformed cookie, + injected `$EDITOR`) and assert it's neutralized, not just that the happy path still works. + +**Feature / default-change** — tests for the new behavior **and** its edges; docs/changelog +updated. Default/behavior changes get extra scrutiny (rubric §4): they hit every app on +upgrade. App defaults belong in the **CLI app template** (e.g. `config/routes.cr.ecr`), not +bolted onto core; a new pipe must be **plugged into a pipeline** or it's inert dead code; and +"on by default" needs an explicit safety argument (the wave shipped HSTS-on-by-default — dangerous). +- [ ] Note the **semver** impact: a default/pipeline change is **minor** at minimum, **major** if it can break an existing app's requests. + +## 5. Add tests, including the edge cases assistants skip + +- [ ] New/changed behavior is covered by a spec. +- [ ] Edge cases the wave hid in: dotfiles, empty/trailing segments, malformed input, + and the **non-development** branch (production-only code paths). +- [ ] For perf/refactor, the **differential** specs that exercise the divergent inputs from your §4 table. + +## 6. Don't duplicate a recently-merged PR + +- [ ] Check open/recently-merged PRs first — assistants frequently re-propose something + already landed or already settled: + ```sh + gh pr list --state merged --limit 30 + gh pr list --state open --search "" + ``` +- [ ] Confirm you're not re-introducing a pattern the repo already settled (e.g. the + `Process.run` no-shell idiom) or contradicting a [DESIGN_DECISIONS.md](../../DESIGN_DECISIONS.md) entry. +- [ ] If you include `.jules/`-style learning notes, make sure they're accurate and contain + **nothing machine-specific** (no absolute local paths, hostnames, usernames, tokens, env dumps). + +## 7. Run the local checks so CI is green the first time + +CI is CircleCI and runs lint + the full spec suite + the postgres-backed granite build spec +on every PR (rubric §7). Match it locally **before** you push. + +- [ ] One-time setup (idempotent — installs git hooks + makes the skills loadable): + ```sh + shards install # produces bin/ameba + bin/setup-dev + ``` +- [ ] Run the full CI-matching check — lint, format, specs, and the granite build spec: + ```sh + bin/amber_spec + ``` + (The granite spec needs a local Postgres: DB `granite_test`, user `postgres`, password `postgres`.) +- [ ] Let the hooks installed by `bin/setup-dev` do their job — **pre-commit** runs the fast + `crystal tool format --check`, **pre-push** runs format + `bin/ameba` + `crystal spec`. + If a hook fails, fix the cause; don't reach for `SKIP_HOOKS=1`. +- [ ] CI pins **Crystal 1.9.2** for specs (shard allows `>= 1.0.0, < 2.0`). If it passes on a + newer compiler for you but you suspect the pin, verify against 1.9.2 — don't rely on + newer-stdlib-only APIs. + +## Final gate + +- [ ] PR type stated, description matches the diff, one concern, per-type bar met with evidence, + tests + edge cases, no duplication, `bin/amber_spec` green. Now fill in the PR template honestly and open it. diff --git a/.github/contributing/skills/review/SKILL.md b/.github/contributing/skills/review/SKILL.md new file mode 100644 index 000000000..1a4ac1541 --- /dev/null +++ b/.github/contributing/skills/review/SKILL.md @@ -0,0 +1,117 @@ +--- +name: review +description: Use when reviewing or triaging an Amber PR. Pulls the PR, checks mergeability mechanics, compiles and runs specs locally, classifies the PR type, applies the severity framework via the three discount levers (verifying claims rather than trusting the description), enforces the per-type correctness bar, discounts documented design trade-offs, decides enhancement vs feature, applies AI-PR hygiene, and produces a verdict (MERGE / MERGE_WITH_CHANGES / REQUEST_CHANGES / CLOSE) backed by file:line evidence. +--- + +# Reviewing an Amber PR + +You are the maintainer. The rubric is the law: [REVIEW_RUBRIC.md](../../REVIEW_RUBRIC.md) is the single source of truth for the severity framework, per-type bars, and enhancement-vs-feature values; [DESIGN_DECISIONS.md](../../DESIGN_DECISIONS.md) lists intentional trade-offs you must discount. This skill is only the *procedure* — it does not restate the rubric. + +Golden rule: **the PR description is a claim, not evidence.** Verify everything against the diff and against running code. Cite `file:line` for every point you make. + +## 1. Pull the PR and check mergeability mechanics + +```sh +gh pr checkout +gh pr view --json title,author,body,files,additions,deletions +gh pr view --json mergeable,mergeStateStatus,reviewDecision +gh pr diff +``` + +Read the mechanics before you read the code (see rubric §6): + +- `mergeable: CONFLICTING` -> not reviewable yet; ask for a rebase. +- `mergeStateStatus: BLOCKED` -> usually a required review/approval, **not** a merge conflict. Check `reviewDecision`; do not chase a phantom conflict. +- `reviewDecision: REVIEW_REQUIRED` / `CHANGES_REQUESTED` -> exactly that. +- Skim the diff once end-to-end now. Note its true scope (does it match the title?) before forming opinions. + +## 2. Compile and run specs locally + +A PR with red specs or `bin/ameba` errors gets bounced, not reviewed (rubric §6, §7). Match CI before spending review attention: + +```sh +shards install # produces bin/ameba +bin/amber_spec # ./bin/ameba, crystal tool format --check, crystal spec, crystal spec ./spec/build_spec_granite.cr +``` + +- The granite spec needs a local Postgres (DB `granite_test`, user `postgres`, password `postgres`); start one or note it as not-run. +- CI pins **Crystal 1.9.2** for specs. If it's green for you on a newer compiler, suspect a version-pin break and re-check against the declared range (`>= 1.0.0, < 2.0`). +- **Green specs do not mean correct.** If the suite is happy-path only, green tells you nothing about the edge cases the change can break (§3 Performance). Hold that thought for step 5. + +## 3. Classify the PR type + +Pick exactly one type and review against *that* bar (rubric §3): **Bugfix · Security · Performance · Refactor · Feature/default-change.** State your choice in the review. If the author claimed a type, confirm the diff actually fits it — a "refactor" that edits spec *assertions* is not a refactor; a "perf" change that alters output is a behavior change. + +## 4. Apply the severity framework — verify, then discount + +For every issue you raise, assign a tier (Critical / High / Medium / Low / Informational per rubric §2), then **discount it** through the three levers. A raw finding is a hypothesis; the surviving severity is what you report. **Verify each lever yourself in the diff/code — do not inherit the author's framing.** + +- **(a) Reachability + existing mitigations.** Trace the finding to a sink an attacker can actually drive, then check for `rescue` on the immediate caller and a top-level `Pipe::Error`/framework handler before assigning severity. Worked example: the `MessageVerifier`/`MessageEncryptor` `IndexError` "DoS" was fully contained by `SignedStore#verify`, `EncryptedStore#verify_and_decrypt`, and `Pipe::Error` -> Informational/Low, not High. If a mitigation exists, name it and discount. +- **(b) Threat model.** Network-facing vs local dev CLI. A `bin/amber`/CLI command already runs as the operator, so the `encrypt.cr` `system("#{ed} #{file}")` injection is **Low** (operator is the attacker). The same shape on a network endpoint jumps several tiers. +- **(c) Environment gating.** Where does the dangerous branch run? The `error.cr` unescaped `@ex.message` is in the **non-development** branch, so it's a production reflected-XSS (High); a finding that only fires in `development?` is usually Low/Informational because production never executes it. + +Do not accept a severity from the description. Re-derive it from the three levers, citing the lines. + +## 5. Apply the per-type correctness bar + +Grade the diff against the bar for the type you picked in step 3 (rubric §3). The bar is **evidence**, not vibes: + +- **Bugfix:** a spec that was red before the fix and is green now; root-cause not symptom; blast-radius note. +- **Security:** clears all of §3 Security — real+reachable+not-already-mitigated; **complete** fix (the `encrypt.cr` fix that escaped the file but left `$EDITOR` injectable failed this — prefer eliminating the shell via `Process.run(cmd, [args])`); **narrow** fix (no over-broad `rescue`, no loosened auth check); an **adversarial spec** that sends the malicious input and asserts neutralization. +- **Performance:** **provably behavior-identical.** Require the differential edge-case table (dotfiles, trailing slash, empty/extension-only segments) — this is exactly where the `File.extname` router PR diverged while CI stayed green. No table, no merge. +- **Refactor:** existing suite passes **unchanged**; if assertions were edited, it's not a pure refactor — reclassify. +- **Feature/default-change:** tests for the new behavior and its edge cases; docs/changelog updated; description matches code; extra scrutiny per step 7. + +## 6. Discount documented design trade-offs + +Before you flag anything that smells like "why is it done this way," scan [DESIGN_DECISIONS.md](../../DESIGN_DECISIONS.md). If an entry matches, it is **by design** — do not flag it, and reject a PR that "fixes" it without a separate discussion. Common ones the wave tripped on: + +- App defaults live in the **CLI app template**, not core — so "core has no default for X" is often correct, and a pipe not wired into `config/routes.cr.ecr` is dead code. +- `crystal-db`'s pool is already concurrency-safe — don't accept a re-added `Mutex` around checkout. +- `error.cr` is the **non-development** page (dev uses the `exception_page` shard) — review it for the production threat model, not its markup. +- `Process.run(cmd, [args])` (no shell) is the settled shell-out idiom; `headers["X"] ||= value` is the intentional non-clobbering default. +- Extension/content-type lists derive from `Content::TYPE` / `TYPE_EXT_REGEX` — a hand-coded duplicate that can drift *is* flaggable. + +A finding that contradicts an entry needs strong specific evidence (a concrete reachable failure or differential test), not "looks wrong." + +## 7. Decide enhancement vs feature + +Apply the rubric §4 values: + +- **Enhancement** (improves something Amber already does without changing its surface contract) -> review against its matching §3 type bar plus evidence it doesn't change behavior for existing apps. Welcome as a PR. +- **New feature** (new pipe / CLI command / config knob / template default) -> Feature/default-change bar **plus design buy-in**; it should usually have started as an issue/discussion. A drive-by feature PR with no prior agreement is a reason to ask for that discussion first. +- **Default/behavior changes get extra scrutiny.** They change every app silently on upgrade — recall the SecureHeaders PR that shipped HSTS on-by-default (can hard-break HTTP) *and* was inert dead code. Confirm the **semver** call-out in the PR: a default/behavior change is **not** patch-safe (minor at minimum, major if it can break existing requests). + +## 8. Apply AI-PR hygiene + +AI-assisted PRs are welcome (rubric §5) — hold them to the same checklist: + +- [ ] **Description matches the code** — grep the diff against the claims; mismatch is the single most common wave failure. +- [ ] **One concern, scoped diff** — no security-fix-plus-refactor-plus-rename bundles. +- [ ] **Edge-case tests present** — dotfiles, empty/trailing segments, malformed input, non-development env. +- [ ] **No duplication** — check against recently merged PRs; assistants re-propose already-settled patterns (e.g. the `Process.run` no-shell fix). +- [ ] **Learning notes accurate and clean** — `.jules/*`-style notes are fine if correct and contain nothing machine-specific (no absolute local paths, hostnames, usernames, tokens, env dumps). + +## 9. Produce the verdict + +End with one verdict and the evidence behind it. Every claim cites `file:line`. + +- **MERGE** — fits its type bar, levers leave no live high/critical, no design-decision conflict, mechanics clean. Say what you verified. +- **MERGE_WITH_CHANGES** — sound and mergeable after small, named fixes (e.g. add the one missing adversarial spec, wire the pipe into `config/routes.cr.ecr`, drop the machine-specific note). List each required change with its `file:line`. +- **REQUEST_CHANGES** — fails a bar or a lever in a way that needs author rework (incomplete security fix, missing differential table, over-broad `rescue`, description/code mismatch). State the bar it missed and what evidence would clear it. +- **CLOSE** — wrong by design (contradicts a DESIGN_DECISIONS entry without evidence), a non-issue after discounting (the "DoS" that was fully mitigated), a duplicate of settled work, or a feature with no design buy-in that belongs in a discussion. Point to the rubric/decision that settles it. + +Verdict template: + +``` +Type: +Mergeability: mergeable=<...> mergeStateStatus=<...> reviewDecision=<...> +Local checks: bin/amber_spec +Findings: + - [] (file:line) — levers applied: +Design-decision discounts: +Enhancement vs feature: +AI hygiene: +Verdict: +Required to clear: +``` diff --git a/.gitignore b/.gitignore index b3c65c40b..315524126 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ node_modules /myapp cli.dwarf .agents/ +/.claude/ diff --git a/bin/setup-dev b/bin/setup-dev new file mode 100755 index 000000000..293f4e7bd --- /dev/null +++ b/bin/setup-dev @@ -0,0 +1,62 @@ +#!/bin/sh +# Amber one-shot dev setup. Idempotent — safe to re-run. +# Activates the local git hooks and makes the contributor/maintainer skills +# loadable by Claude Code from .claude/skills/ (which is gitignored). +# +# Usage (from anywhere in the repo): bin/setup-dev + +set -e + +# Run from the repo top level so all paths below are repo-relative. +ROOT=$(git rev-parse --show-toplevel) +cd "$ROOT" + +echo "==> Setting git hooks path to .githooks" +git config core.hooksPath .githooks + +echo "==> Making hooks executable" +for hook in .githooks/*; do + [ -f "$hook" ] && chmod +x "$hook" +done + +echo "==> Linking skills into .claude/skills/ (relative symlinks)" +mkdir -p .claude/skills +# Each skill lives under .github/contributing/skills/; symlink it into +# .claude/skills/ using a relative target so the link survives moves/clones. +for skill in pre-review review; do + src=".github/contributing/skills/$skill" + dst=".claude/skills/$skill" + if [ ! -d "$src" ]; then + echo " note: $src does not exist yet — skipping (author it, then re-run bin/setup-dev)." + continue + fi + # Refresh the link idempotently: drop any existing link/dir, then recreate. + rm -rf "$dst" + ln -s "../../$src" "$dst" + echo " linked $dst -> ../../$src" +done + +echo +echo "============================================================" +echo " Dev setup complete." +echo "============================================================" +echo +echo "Git hooks are now ACTIVE (core.hooksPath = .githooks):" +echo " - pre-commit : fast 'crystal tool format --check'" +echo " - pre-push : format check + bin/ameba + 'crystal spec'" +echo " Bypass any hook with: SKIP_HOOKS=1 git ..." +echo +echo "Skills are now loadable by Claude Code from .claude/skills/" +echo "(.claude/ is gitignored, so this is local-only — not committed):" +echo " - pre-review : contributor self-review before opening a PR" +echo " - review : maintainer review pass" +echo +echo "Run the checks that match per-PR CI:" +echo " crystal tool format --check # formatting (CI: ameba-test/specs)" +echo " bin/ameba # lint (CI: ameba-test)" +echo " crystal spec # full suite, no DB (CI: amber-specs)" +echo +echo "Full CI-matching check (incl. the postgres-backed granite build spec):" +echo " bin/amber_spec" +echo +echo "Tip: if bin/ameba is missing, run 'shards install' first."