Code review

Every PR gets at least one review before merge. No exceptions, including from the founder.

The reviewer’s job in one sentence

Decide whether merging this PR moves the codebase in the right direction — not whether it’s perfect, not whether you’d have written it the same way.

Checklist

Go through in this order:

1. Is the problem clear?

• The PR description explains the why, not just the what.
• If this fixes a bug, the bug is described (repro, ideally a failing test).
• If this is a feature, it matches an agreed-upon scope (Linear ticket, ADR, product spec).

If the problem isn’t clear, request clarification before reviewing the code. Don’t guess.

2. Is the solution the right shape?

• The approach fits the system’s existing patterns (see Architecture and Conventions).
• Business logic is in domain/ or pipeline/, not in api/.
• I/O is in adapters/, not in domain/.
• New external deps are justified — or avoided.

Rewrites that change the shape of an abstraction are ADR-level decisions, not PR-level. Escalate.

3. Is it correct?

• Happy path works.
• Error paths are handled at the right boundary (see Conventions → error handling).
• Edge cases tested: empty input, single item, huge input, Unicode, nulls, race conditions if relevant.
• Concurrency: async code doesn’t block the loop, no shared mutable state across requests.

4. Is it observable?

• New failure modes emit an audit event or a log line with enough context to debug.
• New code paths have a metric if they could cause latency regression.
• Feature flags wrap risky changes (see Feature flags).

5. Is it secure?

• No secrets in the diff, in tests, or in logs.
• No SQL string concatenation — parameterised queries only.
• No new endpoint that skips auth.
• Tenant isolation: every query filters by tenant_id at the top of the request.
• No unsafe deserialisation (pickle, yaml.load without SafeLoader).

6. Is it tested?

• The critical path has an integration test (see Testing).
• Unit tests cover the domain logic.
• Mocks are in the right places — we don’t mock the DB.
• Snapshot updates reviewed line by line.

7. Is it sized right?

• The PR does one thing. If it does three, ask for it to be split.
• Refactors are separate PRs from behaviour changes.
• Formatting-only commits are their own PRs.

Approval levels

Outcome	When
Approve	You’d ship this.
Approve with comments	You’d ship this, but there are small things to fix before / after merge. Be explicit about which.
Request changes	There’s something you want addressed before merge.
Comment only	Questions, suggestions, not blocking.

If in doubt between “approve with comments” and “request changes”, use request changes — it’s easier to clear a block than to reopen an approved PR.

Disagreeing

Disagreements happen. Norms:

• Disagree on Slack, not in the PR. Threaded back-and-forth in line comments gets painful. Move to #eng (or DM for spicier topics) once you’ve exchanged more than one comment.
• “I’d have written this differently” is not a reason to block. Block on bugs, missing tests, security issues, architectural misalignment. Otherwise, it’s the author’s call.
• Escalate to the eng lead if you can’t agree. Don’t let PRs sit.
• Once merged, it’s ours. Don’t re-open the debate post-merge.

What authors owe reviewers

• A clean diff (squashed work-in-progress commits)
• A description that answers “why now, why this way”
• Self-review done before requesting review
• Response within 1 business day (either “here’s the fix” or “here’s why I disagree”)

What reviewers owe authors

• First response within 1 business day
• Specific suggestions, not just “rethink this”
• If you block, tell them exactly what would unblock
• Don’t nitpick style if the formatter missed it — that’s a linter bug

Who reviews what

CODEOWNERS in each repo routes by path. General defaults:

Path	Owners
src/tappass/pipeline/**	@jens + anyone with pipeline knowledge
src/tappass/audit/**	@jens only — compliance-critical
src/tappass/api/**	Any backend engineer
frontend/**	Frontend lead + anyone in frontend
infra/**	Ops lead
config/policies/rego/**	@jens + policy reviewer

Add yourself to CODEOWNERS for paths you feel confident reviewing.

LGTM stamps

“LGTM” without a checklist check is not a review — it’s a rubber stamp. If you can’t articulate what you looked at, pair-review with someone instead of clicking approve.