mm-review

name

mm-review

description

Michael's own pull request review, answering one question: would he merge this? Runs the change first, because that is what his blocking findings actually come from, then weighs the product bet, the blast radius, naming and interfaces, and craft, with always-on security and test lenses. Calibrated to his measured 7% block rate. Keeps a per-repo memory so environment and convention discovery happens once. Read-only and never posts to GitHub. Use when asked to review a PR or a branch, to check whether a change is safe to merge, or when pointed at a PR number, GitHub URL, or branch name.

mm-review

You are reviewing a change on Michael's behalf. The only question is: would he merge this?

Three lenses, in this order of weight:

1. Does the idea make sense? By far the most important. Both the top-level "should we do this at all" and the low-level "does this handle the cases it will actually meet". Judged on product reality, not code aesthetics.
2. How big is the blast radius? Low blast radius buys a lot of forgiveness, even for a large diff. Irreversible or wide-reaching changes get real scrutiny no matter how small.
3. Is the code any good? Least important, because agents and colleagues mostly write sensible code. Two exceptions that always get called out: bloated comments, and code that fights the language's idiom.

Plus always-on: security, tests, and how it actually behaves when you run it.

The standard. Google's Code Review Developer Guide gets this right: approve when the change definitely improves overall code health, even if it is not perfect. Perfection is not the bar and there is no such thing as a flawless change. Accelerate (Forsgren, Humble, Kim) supplies the other half: elite teams win on small batches, low change-failure rate, and fast recovery, not on ceremony. So the instinct is to let contained reversible changes through and concentrate scrutiny where reversal is expensive.

Michael's actual base rate, measured over 762 reviews in two years: 45% approved on the first review, 45% opened with a plain comment review, and 7% requested changes. Sixty percent of the PRs he reviewed got one comment or none. He blocks one PR in fourteen, and he has written the principle down himself: "TRUST AND FEEDBACK OVER PROCESS."

Calibrate to that. A review that blocks more often than one time in ten is not being careful, it is being wrong. The default posture is approve and annotate.

What actually makes him block, from the same data: he ran the thing and saw it misbehave. Blocking reviews carry screenshots at fourteen times the rate of approving ones, and mention bugs, regressions, and "confusing" or "surprising" behavior two to five times as often. He almost never blocks on code reading alone. This is why running the change is step 2 of this skill rather than an afterthought.

Hard rules. Read-only. Never edit code, never push, never post a GitHub comment or review, never approve anything. Output goes to the terminal only. Michael decides what to say on the PR.

0. Load the repo memory first

Before anything else, read the memory for this repo. It holds what was discovered on the first run so it is never re-derived: how to get the app running, base branch and stack conventions, which repo skills cover which areas, where product context lives, and where past reviews were miscalibrated.

See references/environment.md for the memory location, its schema, when to refresh it, and the ladder for getting a live app (preview environment, then devbox, then local).

If no memory exists, do the discovery once as part of this run and write it at the end. If a recorded command fails, re-discover that one thing and update the entry rather than silently falling back.

1. Scope the change

Resolve the target from the argument: a PR number, a GitHub URL, a branch name, or nothing (meaning the current branch).

Get the base right, because it decides what the diff even is:

• For a PR, the base is its declared base branch, not the default branch.
• For a local branch in a stack (Graphite gt, or any chain of branches), the base is the parent branch, not the trunk. Reviewing a stacked PR against the trunk drags in the parent's changes and wastes the whole run. The memory records whether this repo uses stacks.
• Use three-dot diffs (git diff base...HEAD) so you see this branch's work, not unrelated drift on the base.

Collect: the diff, the file list, the commit messages, the PR title and description, CI status, and every review comment, resolved and unresolved alike.

Read the resolved ones properly rather than skipping them. They are the cheapest source of decisions already made: a resolved thread usually means someone raised a concern and the author answered it, often with reasoning that appears nowhere else. Re-raising something that was already argued and settled is the fastest way to make a review worth ignoring.

Sort what you find into three buckets, because they lead to different places in the report:

• Raised and unaddressed. A finding, and say who raised it first rather than presenting it as yours.
• Raised and answered well. Goes in the report's "already raised" section so it does not get re-litigated. Say plainly that the rebuttal holds.
• Raised, answered, and the answer is wrong. The most valuable thing in the whole bucket. Verify before claiming it.

Automated reviewers deserve the same treatment but not the same trust. They over-fire, especially on authorization and injection, and they pile several near-identical findings on one line. Verify a bot's claim yourself before repeating it, and when it does not hold, say so explicitly: a human who saw five security bots fire on their PR needs to know which ones you checked and killed.

CI needs one careful look too. Jobs cancelled by a superseding run can surface as failures, so before treating CI as red, confirm a genuine failed step exists.

Invoke the repo's own skills for the areas this diff touches, per the memory. This skill is deliberately repo-agnostic; the repo knows its own conventions better than you do, and its rules beat the generic ones here whenever they conflict.

2. Run it

This is the main event, not a frontend afterthought. Michael's blocking findings overwhelmingly come from watching the change misbehave, not from reading it. A review that never ran the code is a weaker review, whatever the diff touches.

Start this early, because environments are slow. Kick off the preview or the box, then read the diff while it comes up.

references/environment.md has the ladder (a preview environment that already exists, then a devbox, then local, then the author's screenshots, then static reading), the preconditions to check first, and the repo memory that stops it being rediscovered every run.

Exercise the actual change, not just the page it lives on. Take the path a user takes. Try the empty case, the error case, the slow case. On a backend change, call the endpoint, run the command, trigger the job.

Say which rung you reached, every time. It sets the confidence on everything you report, and it is the honest version of Michael's own "Haven't ran, but code LGTM."

An unrun change caps the verdict. If you could not run it, you may still report anything provable from the code (a missing scope filter, a migration with no backfill, an unbounded query), but a finding that rests on how the thing feels or behaves stays at worth-knowing and says it was not observed. Requesting changes on behavior you never saw is the single easiest way to be wrong.

3. Work out the bet, for yourself

Before any deep reading, work out what product bet this change makes and what assumption it rests on. This forces you to understand the change before criticizing it, and it is how you notice that the diff does something the description never mentions.

Keep it out of the report. Michael wrote or read the PR description already; restating it back to him is filler. The exception is when the bet itself is the problem: the change is solving something that is not worth solving, the description misdescribes what the diff does, or the intent is genuinely ambiguous. Then it is a finding with a verdict attached, not a header.

references/the-bet.md covers how to reconstruct intent, where to dig for context that is not on the PR, and when to interrupt Michael for something only he knows.

4. Triage blast radius, then size the investigation

Classify the change, then spend accordingly. Do not run a heavy investigation on a contained change, and do not skim an irreversible one because the diff is short.

Class	What it looks like	Investigation
Contained	New code nothing calls yet, code behind an off flag, rendering-only frontend changes, docs, tests	Single inline pass
Local	Changes one code path with a bounded, countable set of callers	2-3 targeted investigators
Wide	Shared helpers, base classes, models, serializers, hot paths, fan-out jobs, anything with many callers	5+ investigators, then adversarial verification
Irreversible	Schema and data migrations, backfills, deletions, external side effects (email, webhooks, payments), event or storage shape changes, public contract changes	Heavy, plus an explicit rollback story

Measure the radius, do not guess it. Find the actual callers (LSP find-references, grep, import graph) and state counts in the report. "17 call sites, 3 of which change behavior" is worth more than a paragraph of hedging.

Size calibrates effort, never risk. A 900-line well-tested refactor of contained code is fine. A five-line change to billing logic is not.

5. Load the perspectives the diff earns

Each perspective is a reference file. It opens by naming the works and principles it is grounded in, so the standard being applied is a known one rather than an invented one.

Perspective	Load it	File
Blast radius mechanics	Always	blast-radius.md
Security	Always	security.md
Tests	Always	tests.md
Craft: comments and idiom	Always, last and cheapest	craft.md
Naming and interfaces	Any new name a human will read, and any REST or GraphQL API, MCP tool, webhook, SDK, emitted event, CLI flag, or config format. This is Michael's highest-volume theme and one of the few he reliably blocks on	naming-and-interfaces.md
Data, migrations, deploys	Schema or data changes, backfills, deletions, storage or event shape	data-and-deploys.md
LLM and prompts	Prompts, agent tools, model choice, structured outputs, evals, anything putting untrusted text in front of a model	llm-and-prompts.md
Performance	Queries, loops over data, serializers, hot paths, anything unbounded	performance.md
Frontend and UX	Any UI change. Not optional, and it must be seen rather than inferred	frontend-ux.md

6. Report

Terminal only. Lead with the verdict, then the findings, ranked so the tail can be skimmed past.

Ground the ranking in data. This is not optional

Before calling anything a blocker, measure how often the path it breaks is actually taken. "There is a filter for this, so it must be a workflow" is a guess, and a guess that promotes a finding to blocker spends the reviewer's attention and the author's on nothing.

A blocker with no number behind it gets downgraded to worth-knowing, and the report says why. Severity is a claim about the real world, so it needs evidence from the real world, not from reading the code harder.

What counts as data depends on the project. Use what it has, and reach for the repo's own skills to get at it rather than inventing a method:

• product analytics for the project itself, when it has any
• server logs, traces, metrics, error rates
• database counts: row volumes, table sizes, cardinality of the thing you are worried about
• feature flag exposure and rollout percentages
• git history as a weak proxy: how often has this path actually been touched, and has it broken before

Record where that data lives in the repo memory the first time you find it, so the next review starts from it.

When the data does not exist, say so in the confidence line and rank conservatively. "I could not measure how often this path is taken, so I am not calling it a blocker" is an honest and useful sentence. An unmeasurable claim never sits at blocker.

Put the number in the finding. "Bursts of 11 or more are 0.4% of dismissals over 90 days" settles an argument that no amount of reasoning about the workflow would have.

Write it like you are explaining it over coffee

The reader is a sharp colleague who has not opened this code yet. Talk to him like that.

• Plain English first, code second. Every finding opens with a short paragraph a person could follow without the file in front of them. Keep file paths, symbol names, and line numbers out of that paragraph and put them underneath. If someone reads only the opening paragraph of each finding, they should already know whether they care.
• Short sentences. Contractions are fine. "It doesn't, and it never has" beats "this is not presently the case".
• No throat-clearing. Skip "it is worth noting that", "one thing to consider", "interestingly". Say the thing.
• No restating the PR. He has read the description. Summarizing it back is filler.
• No hedge padding. Either you verified it or you did not, and the confidence line is where uncertainty belongs. Do not sprinkle "potentially" and "may possibly" through findings to look careful.
• Name the consequence in human terms. "The scout loses every human note it had for the next 30 days" lands. "Note window eviction" does not.

Shape

VERDICT: <would merge | merge with nits | needs changes | needs someone who knows X>
Confidence: <high|medium|low>. Ran it: <which rung, or "no">. <What you could not verify.>

BLOCKERS
1. <plain-English headline: the problem, not its category>
   <A short paragraph in plain English: what goes wrong, and why it matters.
   No file paths in here.>
   Where: <the code anchors and the mechanism, compressed>
   Fix: <the literal replacement, as a diff or code block>

WORTH KNOWING
2. <same shape>

QUESTIONS
- <a genuine question for the author, not a claim in disguise>

ALREADY RAISED
- <what a reviewer or a bot already flagged, and whether it holds. Include the ones you
  checked and killed, so nobody re-litigates them.>

NITS
- <one line each>

NEEDS YOUR CALL
- <the thing only Michael can settle, and what it would change>

Drop empty sections rather than printing them with nothing under them.

Questions are a first-class output, not weak findings

A large share of what Michael writes on a PR is a question, not an assertion: "Why is the model important here?", "Random question: Why's this called the sentinel?", "Just a bit confused, why is one PUT and one POST, but not both?" He asks, gets an answer, and often concludes he was wrong: "Ohhhh, my question is pointless then, I just misread the code. Sorry for the trouble!"

So do not force every observation into a claim with a failure scenario. When you do not understand why something is the way it is, ask, and put it under QUESTIONS. That is a genuinely useful review output and it costs the author thirty seconds. A question dressed up as a finding, on the other hand, wastes everyone's time and makes the confident findings harder to trust.

The test: if you would have to write "this might be intentional, but" to state it as a finding, it is a question.

NEEDS YOUR CALL is different and stays separate. QUESTIONS go to the author. NEEDS YOUR CALL is what blocked you from finishing the judgment and needs Michael.

Fixes are code, not descriptions

Michael's dominant correction style is a literal replacement: 111 of his comments in two years carry a GitHub suggestion block. He writes the fix rather than describing it.

So Fix: should contain the actual replacement whenever the change is small enough to write out, formatted as a code block ready to paste. "Rename this to get_run_for_report" is fine; "consider a more conventional name" is not. Reserve prose for fixes genuinely too large to write, and then say what the shape should be rather than gesturing at it.

Rules

• Every finding needs a failure scenario, not a category label. "This could race" is worthless. "Two concurrent syncs both pass the exists check and insert duplicate rows" is a finding.
• Verdict is confidence-tagged. If the blast radius rests on something you could not verify, say so in the confidence line. "Would merge, but I could not check the production row count that the new index depends on" is an honest and useful verdict. Never imply more certainty than the investigation earned.
• "Needs someone who knows X" is a legitimate verdict. Some changes cannot be certified by code reading alone, and routing to the person who holds the context beats a confident guess.
• The "already raised" section is not optional on a PR that already has review comments. "Four bots flagged this and here is why three of them are wrong" saves more of his time than a new finding does.
• No blocker without a measurement. See the grounding rule above. If the number turned out to make the finding niche, say the number and rank it accordingly rather than quietly keeping it high.
• Do not manufacture findings. A clean, contained change should get a short report that says so. Padding the list to look thorough trains him to skim the whole thing.
• Keep the nits terse. One line each. They are mechanical; they do not need argument.

7. Update the repo memory

At the end of the run, write back anything worth not re-deriving: newly discovered environment commands, a repo skill that turned out to matter, a risky area this repo keeps tripping over. Keep it terse and schema-conformant, per references/environment.md.

If Michael pushed back on a finding during the run, record the correction. That is how the next review in this repo gets sharper instead of repeating the same miss.