The code quality engine: review that catches the real bug and commits that explain why

Your team already has the code-review and git-workflow playbooks — the worked recipes for a structured review by dimension and for staging and committing each concern separately. This module is the layer above the recipe. It’s where you master the two moves that keep a codebase readable and reviewable — the adversarial review that catches the real bug and drops the false positives, and the commit discipline that means a future engineer can actually use git blame — and prove, against a real rubric, that you can do both.

The easy failure in code review is the rubber stamp: skim it, run the tests, approve. The harder failure — the one that makes you less useful than just saying nothing — is posting a confidently-worded “bug” that isn’t one, because you didn’t verify the finding against the actual code before sending it to the author. Claude makes a fast adversarial reviewer. It also makes a fast wrong one. The judgment this module teaches is the difference between those two modes.

The adversarial review — why framing is the whole game

The playbook teaches you to ask for a review by dimension: correctness, edge cases, security, operability. Mastery is the judgment behind why that framing matters and what to do when it’s not enough.

• A reviewer that wants to approve finds nothing. The default stance of a polite review — “does this look right?” — produces “looks good to me” on non-trivial diffs. The adversarial framing flips this: “assume there is at least one real bug and find the worst one.” That single instruction is what separates a review that finds the 3am bug from one that approves it. It’s not aggression; it’s method.
• Dimensions prevent the vague once-over. Asking for a review “organized by correctness, edge cases, security, and operability” forces coverage that “review this diff” doesn’t. The 3am operability bug — the one that works fine in development and pages someone when load is ten times higher — only surfaces if someone explicitly asked whether the code behaves under load, partial failure, or a race condition.
• If it comes back clean on a non-trivial diff, push harder. “Found nothing serious” on a 200-line diff that crosses an auth boundary is almost never the right answer. If the first pass comes back clean, instruct it to be adversarial: “Assume there’s a bug. Now find it.” That instruction alone surfaces findings the polite mode glossed over.
• The framing does not replace your judgment. Claude is a fast adversarial first pass. It is not the approver. A human owns the sign-off because a human is the author of record. The faster the first pass, the more important it is that the final call stays yours.

The verification step — the guardrail that makes the rest safe

The playbook mentions verifying findings; this module makes it mandatory. This is the step that separates a useful review from an embarrassing one.

• Claude will assert a non-bug with full confidence. It sometimes misreads control flow — flagging a guard that already exists, missing a check two functions up the call stack, asserting a type mismatch that the compiler already prevents. A confidently-worded finding is not a verified one. The verification step is not optional polish; it’s what makes the review worth sending.
• “Show me the lines” is the test. For every material finding, ask Claude to show you the exact lines that prove it’s real — the code that creates the problem, not a description of it. If it can’t show you the specific lines, the finding gets dropped, not posted as a comment. “I believe this could be a bug” is not a review comment; “Here are src/webhooks/stripe.ts:84-92, where the event type check runs after the payload is already processed” is.
• Kill false positives before they reach the author. Posting an unverified finding wastes the author’s time and burns your credibility as a reviewer. The false-positive rate on an unverified pass is high enough that you should assume every finding is wrong until the lines prove it’s right. That’s not skepticism of Claude — it’s professional standard for any review tool.
• One test per confirmed bug. For every finding that survives verification, ask for the single test that fails on the current code and would pass once it’s fixed. That test turns “I think this is broken” into “here’s the red test that proves it” — which is a far stronger review comment than prose, and a concrete next step for the author instead of just a complaint.

The commit discipline — the judgment the recipe can’t teach

The playbook shows you the steps to stage and commit each concern separately and draft a message that explains why. Mastery is the judgment that distinguishes a commit history worth inheriting from one that’s archaeology.

• “Why” is not obvious from “what.” “Update session TTL” describes what changed. “Fix session expiry: TTL was read in ms from config but compared in seconds in session.ts:142, causing sessions to expire 1000× faster. No test covered the comparison — adding one.” That’s why — the constraint the code was working around, the reasoning behind the choice, the thing a reader couldn’t reconstruct from the diff alone. The body of a commit message is not a description of the change; it’s the missing context the diff doesn’t carry.
• One concern per commit is the non-negotiable. A commit that fixes a bug, updates a dependency, and renames three functions is a commit nobody can bisect. When a regression appears and the bisect lands on that commit, the engineer has to read 300 lines to find which of the three things was the relevant change. One concern per commit is two minutes of discipline that saves an afternoon every time someone traces a bug back to it.
• The message is the document you write in motion. The CLAUDE.md you wrote in Module 1 tells Claude your commit convention. After that, Claude drafts messages that follow your format and you approve them. The discipline is requiring the body when the change needs it — not every commit needs a three-paragraph why, but every commit that a future engineer might have a question about does.
• The working-tree explanation is the non-negotiable first move. Before Claude stages or commits anything, it explains the state: what’s staged, what’s unstaged, which files span two concerns. You read that explanation and check it against your own view of the tree before a single command runs. Understanding before action is the whole rule of the git-workflow playbook and it’s load-bearing in the review workflow too.

The MENA dimension — review and history for a bilingual engineering team

Code review and commit history are English for most engineering teams, and this module doesn’t change that. The bilingual standard here is narrower and more specific.

• PR descriptions for a bilingual reviewer. A PR whose changes affect client-facing Arabic strings — invoice templates, onboarding emails, error messages — should include, in the PR description, a note on what the Arabic-language change does and who reviewed it. That’s not a translation; it’s the context a bilingual engineering manager or technical lead needs to review the change intelligently.
• Review findings for Arabic string files are scoped to register. If a finding involves Arabic-language content — a string that was generated rather than authored, a term that’s inconsistent with the CLAUDE.md glossary, a Gulf Arabic phrase that reads as translated — the finding names the specific string, the problem, and the correct form. “This translation reads as machine-generated; the Gulf Arabic equivalent would be [X]” is a review comment with the same structure as a code-quality finding.
• The commit body is still English. This is not a translation context. The commit that changes an Arabic string explains in English what the Arabic change does: “fix(invoice-ar): correct Arabic amount formatting for VAT line. The فاتورة ضريبة القيمة المضافة label was using MSA register instead of Gulf Arabic — replaced with the standard used in the rest of the invoice template.”

Your assignment

Complete the code quality loop for one real PR or committed change — use your own (recommended) or the Mizan Stripe webhook integration worked through this module. Open the branch or diff in Claude Desktop and work in the chat — no terminal needed to read and review; running the suite is the Power Track step.

Module 2 deliverable — the code quality engine

1. Structured review (one page)
   - organized by dimension: correctness, edge cases, security, operability
   - each finding with file:line, the problem, severity
   - each material finding verified: the lines that prove it's real, or
     explicitly dropped as a false positive
   - one failing test drafted for each confirmed bug

2. Clean commit history (demonstrable in git log)
   - each concern its own commit
   - commit messages with a subject line (imperative, ≤50 chars) plus a body
     that explains why the change was needed — not what it does
   - working tree explained before anything was staged

How it’s graded — the rubric

Code quality rubric

1. Adversarial framing   The review assumed a bug and looked for the worst
   was used              case — not just "does this look right?" At least one
                         finding came from the adversarial pass, not the
                         polite first scan.

2. Findings are          Every material finding is grounded in specific lines
   verified              that prove it's real. False positives are explicitly
                         dropped, not posted as uncertain comments.

3. One test per          For each confirmed bug: a test that fails on the
   confirmed bug         current code and would pass after the fix. The test
                         pins the exact behaviour that's broken.

4. Commits are           One concern per commit. Each message has a body that
   one-concern +         answers "why was this needed" — not "what did I do."
   why body              The history is navigable by someone who wasn't there.

5. Working tree was      Claude explained the state before staging anything.
   explained first       Destructive commands (reset, force-push) were
                         explained and approved before running.

The bar, shown — a worked model answer (Mizan)

Mizan — Stripe webhook integration PR review (excerpt)

Diff: src/webhooks/stripe.ts (190 lines added), tests/unit/webhooks/stripe.test.ts (47 lines)

Correctness finding (verified):
  stripe.ts:84-92 — The webhook processes the payload before verifying the
  Stripe signature. If verification fails (line 94), the payload has already
  been acted on. In the current flow this allows a replay attack: any caller
  who knows the event shape can trigger a payment_intent.succeeded handler
  without a valid Stripe signature.

  Lines that prove it: processingEvent(payload) is called at :84;
  stripe.webhooks.constructEvent() is called at :94. Reorder is the fix.

  Failing test:
    it('rejects payload processed before signature verification', async () => {
      const invalidSig = 'invalid'
      // send well-formed payload with bad sig — handler should reject before
      // any processing, not after
      const res = await handler(mockPayload, invalidSig)
      expect(res.status).toBe(400)
      expect(mockProcessingFn).not.toHaveBeenCalled()  // currently fails
    })

False positive dropped:
  Initial finding: "event type not validated against an allowlist."
  Verification: stripe.ts:31-35 defines HANDLED_EVENTS = ['payment_intent.succeeded',
  'invoice.payment_failed']; the handler returns 200 early for any other type.
  Finding dropped — the allowlist exists.

Commit history — Mizan Stripe webhook (excerpt)

fix(webhooks): verify Stripe signature before processing payload

Processing the event before calling constructEvent() allowed replay
attacks on the webhook handler. Reordered to verify first; added a
test for the rejection path. Closes #201.

test(webhooks): add missing coverage for payment_failed handler

The invoice.payment_failed path had no test. Added three cases:
successful retry scheduling, idempotency on duplicate events, and
the error path when the customer is not found.

chore(webhooks): extract HANDLED_EVENTS to a shared constant

The allowlist was duplicated between the handler and the tests.
Extracted to src/webhooks/constants.ts so both import from one place.

What you’ve proven — and what’s next

Clear the rubric and you’ve demonstrated the two moves that make an engineering team reviewable: finding the real bug, not a plausible-sounding one, and leaving a history the next engineer can navigate.

Module 3 — shipping features is next: the spec-first, plan-before-diff loop that ships a feature behind a green suite. The commit discipline from this module and the project context from Module 1 both carry forward — the feature’s commits use the same convention, and the plan mode prompt uses the CLAUDE.md to get project-specific advice.