Ops and security in review

SDE2 reviewers expand the checklist past style. You do not need to be a security engineer to ask "who is allowed to call this?" and "what happens at 10× traffic?" Those questions catch real incidents.

Use this as a pass over non-trivial PRs:

• Purpose and rollout story are clear from the description alone
• Design fits the module boundaries; no drive-by architecture
• Correctness under failure: retries, empty inputs, authz, concurrency
• Tests cover the risky behavior; no obvious flake sources (time, order)
• Names and structure match repo conventions
• No secrets, PII leaks, or unsafe migrations
• Comments are labeled; blocking vs nit is obvious
• You could debug this in production from the diff + tests

Walking through the ops slice

• Authz: Is the check on every path, including error and admin paths?
• Injection / SSRF / path traversal: Especially around new URL or file handling.
• Secrets: No keys in logs, fixtures, or client bundles.
• PII: Are new fields retained, logged, or exported lawfully?
• Migrations: Expand/contract? Lock time on large tables? Rollback story?
• Flags and kill switches: Can we turn this off without a hotfix deploy?
• Load: New query patterns need indexes; new fan-out needs budgets.

When AI-generated diffs show up, apply more scrutiny, not less: plausible wrong code, missed authz, invented APIs. You still own every merged line.

Read senior engineers' review comments on other PRs — patterns transfer faster than style guides. Mentoring often is this checklist spoken kindly; see mentorship and production ownership.

If you could not debug this in production from the diff plus tests, the description or the tests are not done. Ask for the dashboard, the flag name, or the failure fixture before you LGTM.

Common mistakes

• Style-only review on billing, auth or data.
• Migration with no rollback. Expand/contract and lock time are the review.
• LGTM without a kill switch on a default-on behavior change.

Check yourself

1. A PR adds a new URL-fetching feature. Which two questions come first?

Who is allowed to call it, and can the URL be turned into SSRF or path traversal?

2. Why do flags belong in the review, not only in the launch doc?

So you can turn the change off without a hotfix if production disagrees.

Key takeaways

• Authz, secrets, PII, migrations, flags and load are SDE2 review, not extras.
• You could debug this from the diff and tests, or it is not ready.
• AI diffs get a stricter pass, not a weaker one.

Further learning

Software Engineering at Google — Code Review chapterSoftware Engineering at Googleabseil.io

Back to the map: code review.