Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions .githooks/pre-push
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@
# ONLY WHEN mix.exs CHANGED IN THIS PUSH (release preflight):
# mix test --exclude macos_only --exclude requires_zig
#
# ALWAYS, ADVISORY (never blocks):
# how far behind origin/master this branch is
# whether code changed without any test changing
#
# The advisory tier exists because both of those got missed repeatedly, and
# neither is safely blockable: a worktree legitimately lags, and plenty of
# real changes need no test. They are printed, loudly, and you decide. A
# check that blocks on a judgement call is a check people learn to bypass,
# which costs more than it catches.
#
# Intentionally NOT in the always-tier: the full test suite. It's a
# 30-60s wait per push and that's exactly what teaches people to reach
# for --no-verify. CI runs the suite on every push regardless; this
Expand Down Expand Up @@ -46,12 +56,59 @@ release_preflight() {
fi
}

# ── Advisory checks ───────────────────────────────────────────────────────
#
# Facts, not opinions: how stale the branch is, and whether code moved without
# tests moving. Both print and return 0.

staleness_notice() {
git fetch --quiet origin master 2>/dev/null || return 0

local behind
behind=$(git rev-list --count "HEAD..origin/master" 2>/dev/null) || return 0
[ -z "$behind" ] && return 0
[ "$behind" -eq 0 ] && return 0

echo "[pre-push] note: this branch is $behind commit(s) behind origin/master."

# 100 is not a magic threshold, it is "far enough that you are probably
# reading, building, or reviewing against something that no longer exists".
# A checkout ~370 commits behind once produced a review finding that a
# function "did not exist" when it did, and cost about forty minutes of
# debugging code that was never running.
if [ "$behind" -gt 100 ]; then
echo "[pre-push] That is far enough to matter. Anything you built,"
echo "[pre-push] measured, or asked a reviewer to read may not"
echo "[pre-push] reflect master. Consider merging before you push."
fi
}

coverage_notice() {
local range="$1"
local changed code_changed test_changed

changed=$(git diff --name-only "$range" 2>/dev/null) || return 0
[ -z "$changed" ] && return 0

code_changed=$(printf '%s\n' "$changed" | grep -E '^(lib|src|priv/templates|ios|android)/' || true)
test_changed=$(printf '%s\n' "$changed" | grep -E '^test/' || true)

if [ -n "$code_changed" ] && [ -z "$test_changed" ]; then
echo "[pre-push] note: code changed, no test changed in this push:"
printf '%s\n' "$code_changed" | sed 's/^/[pre-push] /' | head -10
echo "[pre-push] If that is right — a refactor, a doc string, a"
echo "[pre-push] rename — carry on. If it is not, this is the"
echo "[pre-push] cheapest moment to notice."
fi
}

# git invokes pre-push with no args but pipes
# <local_ref> <local_sha> <remote_ref> <remote_sha>
# on stdin, one line per ref being pushed. We diff the local sha
# against the remote sha to see what mix.exs looks like in this push.
zero=0000000000000000000000000000000000000000
mix_exs_changed=false
push_range=""

while read -r local_ref local_sha remote_ref remote_sha; do
# Branch deletion (local_sha all zeros) → nothing to diff.
Expand All @@ -68,6 +125,8 @@ while read -r local_ref local_sha remote_ref remote_sha; do
if git diff --name-only "$range" 2>/dev/null | grep -qx 'mix.exs'; then
mix_exs_changed=true
fi

push_range="$range"
done

cheap_checks
Expand All @@ -76,4 +135,9 @@ if [ "$mix_exs_changed" = "true" ]; then
release_preflight
fi

# Advisory last, so the notices are the final thing on screen rather than
# scrolled away by compiler output.
staleness_notice
[ -n "${push_range:-}" ] && coverage_notice "$push_range"

echo "[pre-push] ✓ all checks passed"
86 changes: 86 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,21 @@ If something in mob_dev isn't tested today, that's a bug-discovery
opportunity in waiting — list it as a follow-up rather than letting
the next user find it.

## Issue tracking — status lives in Linear

Status lives in **Linear** (team `MOB`), which is the single board across `mob`,
`mob_dev` and `mob_new` — see `mob/CLAUDE.md` for the full split of
responsibilities. The short version, because work in this repo routinely starts
from an issue filed against another one:

- **Linear (`MOB`)** — live status and worklist. One issue per thread.
- **`decisions/`** — durable rationale. Link it from the issue; don't copy it in.
- **PRs / git** — the code. Reference the issue id.

Keep the issue current as you go, not at the end. An issue that says what was
tried and ruled out is worth more than one that says "done" — most of what this
project has learned lives in the ruled-out half.

## Pre-commit checklist

Before committing changes, run **all** in this order:
Expand Down Expand Up @@ -162,6 +177,18 @@ analysis. See [`README.md`](README.md#security-scan-mix-mobsecurity_scan)
for the full layer list and the one-time `brew install` of external
scanners.

### Tests are part of the change, not a follow-up

New behaviour ships with a test unless the change is small enough that a test
would only restate it — a rename, a doc string, a formatting pass. "I'll add
coverage later" is how the untested paths in this repo got there.

The bar is not coverage percentage, it is: **would this test fail if the fix
were reverted?** Check by reverting it. A test that passes either way is worse
than none, because it is claimed as evidence. More than one fix here shipped
with a test that could not fail — including a headline fix whose entire clause
could be deleted with the full suite still green.

### Decision log — check both directions

Before committing, ask two questions, not one.
Expand Down Expand Up @@ -236,6 +263,65 @@ otherwise have shipped:
The one substantial change that skipped review that session was the largest one
in the batch. Do not let size be the reason to skip.

### Before the merge — a second review, on the PR

The pre-commit review reads a diff. This one reads a diff **that claims to be
finished**, against a master that has moved since you started. Those are
different questions, and the second one has caught more.

Both frame-timing PRs in one session passed pre-commit review. The pre-merge
review then found that one of them shipped its headline fix untested — it
deleted the conversion and all 1545 tests still passed — and blocked the other
outright over per-widget state that navigation had silently stopped resetting.
Neither was visible in the diff alone; both needed someone asking "is this
actually done, and does it still fit?"

Give the reviewer the PR, what it claims, and what you are least sure of, and
ask for a verdict — MERGE or DO NOT MERGE, with reasons. Then act on it. A
review you overrule is fine if you say why; a review you skip because the work
felt done is the case this exists for.

**Check the mechanical preconditions yourself; do not delegate them:**

- **CI is green AND the run is newer than the last commit.** A green check from
before your latest push proves nothing. One PR here carried a month-old green
run from 40 commits of master ago.
- **The branch is not behind master.** The `pre-push` hook says how far.
- **Cross-repo claims are true now, not eventually.** Documentation that names
a sibling's version — "requires mob_new 0.4.32" — is false until that version
exists. Land the sibling first, or make the claim true in the same session.
- **Stacked PRs merge base-first**, and the child gets retargeted and re-checked
after the base lands.

### Trust the instrument last

Every rung of the fidelity ladder assumes the thing measuring is honest. When it
is not, the failure does not look like an error — it looks like a result.

Two from one session, both of which were believed for a while:

* A navigation benchmark reported a 6.5x improvement. The tree was installed by
a `LaunchedEffect`, which runs *after* composition, so the frame being timed
still showed the old screen. The real figure was about half that, and the
published numbers had to be retracted.
* An on-device check printed `PASS` against a build that had failed to compile,
because the deploy before it had failed and the previous build was still
installed. The screen it claimed proved the fix had never scrolled.

So:

- **A number better than the theory allows is a bug in the measurement.**
Navigation cannot be cheaper than re-rendering the same tree. When the result
is too good, go and find out why before reporting it.
- **Make a probe fail loudly when its own precondition does not hold.** A check
that silently passes when the setup did not happen is worse than no check.
- **Corroborate against something you did not build.** Platform counters,
`Davey!` frame reports, `Skipped N frames`, a screenshot. Agreement within
30% of an independent source is evidence; your own instrument agreeing with
itself is not.
- **When you publish a number that turns out wrong, retract it in place** and
say what was wrong. Someone will otherwise act on it.

## Release flow

Canonical process lives in
Expand Down
Loading