pre-review

# 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 "<keyword from your change>" ``` - [ ] 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.