Code Review and Working with Existing Codebases Questions

Direct answer

A rollback-safe rename plus backfill never touches the old column or existing readers directly. It adds the new column alongside the old one, writes to both while backfilling the new one in batches, verifies the backfilled data matches, only then switches reads over behind a flag, and keeps the old column around for a retention window so any step can be reversed just by flipping the flag back, not by undoing a destructive change.

Structured elaboration

Each phase below names what code review should specifically confirm before approving it, as a "Review check," plus what to monitor once it ships.

1. Schema change. Add the new column as nullable, with no constraints yet:

ALTER TABLE events ADD COLUMN new_name text NULL;

Review check: confirm this specific statement is additive only and backward-compatible, meaning every existing reader and writer keeps working unmodified the moment this ships, with zero application changes required yet.

2. Dual-write. Deploy an application change, behind a feature flag, that writes both the old and new column on every write to a row. Review check: is the write to both columns transactional or otherwise guaranteed consistent (not "write old, then separately and non-atomically write new"), and is the flag off by default so this ships dormant before anything depends on it?

3. Backfill. A batched, idempotent job fills in the new column for existing rows, only where it's still NULL, ordered by primary key, with a checkpoint so it can resume after an interruption instead of restarting from row one:

UPDATE events
SET new_name = old_name
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_name IS NULL;

Review check: is progress persisted somewhere durable (not just in the running process's memory), and does re-running an already-completed batch do nothing (true idempotence), not create incorrect data? Monitoring: track batches completed, rows backfilled, replication lag, and write error rate on the table for the duration of the backfill, and pause automatically if replication lag crosses an agreed threshold.

4. Verification. Before trusting the backfill, sample a random set of rows and confirm new_name matches what old_name implies for each. Review check: is the sample size and comparison method actually specified in the PR, not just asserted as "we verified it"?

5. Read cutover. Only after verification passes, flip the flag so reads prefer the new column, falling back to the old one if the new one is somehow still empty for a given row. Review check: is the fallback logic actually tested, not just written? Monitoring: application error rate and query latency on this table specifically, right after the flag flips, since a regression here is the trigger for the rollback shown in the diagram below.

6. Cleanup. Once reads have run on the new column successfully for a defined retention window, make it NOT NULL, add any index it needs, and only then drop the old column, in a separate, later PR. Review check: is dropping the old column genuinely a separate step from everything above, so it can never accidentally ship bundled with a change that hasn't been verified yet?

flowchart LR
  A[Step 1: add new_name column, nullable] --> B[Step 2: dual write old_name plus new_name]
  B --> C[Step 3: backfill new_name in batches where NULL]
  C --> D{Verification: sampled row values match}
  D -- mismatch found --> C
  D -- fully verified --> E[Step 4: flip reads to new_name behind a flag]
  E --> F{Error rate normal after cutover}
  F -- regression --> G[Rollback: flip flag back to old_name, dual write stays intact]
  F -- healthy --> H[Step 5: make new_name NOT NULL, add index concurrently]
  H --> I[Step 6: drop old_name after a retention window]

Worked example

Renaming user_email to primary_email on a table with 40 million rows, backfilled in batches of 5,000 rows: that's 40,000,000 / 5,000 = 8,000 batches total. With a short pause between batches to keep replication lag bounded, the job runs as a background process over however long it takes to work through all 8,000 batches, checkpointing its position after each one so a restart resumes from the last completed batch instead of row one. Verification samples 10,000 random rows after the backfill reports complete and confirms primary_email equals user_email for every one of them before the flag is ever flipped to prefer reads from the new column.

Trade-offs and pitfalls

Every step here is reversible specifically because the old column and old read path stay intact until the very last, separate cleanup step, which is exactly what makes this slower and more code than a single rename statement; that trade is worth it for a heavily-used column and wrong for a rarely-touched internal table, where a single migration with a maintenance window might be simpler and perfectly safe. The most dangerous version of this pattern to review is one that quietly combines two of these steps, most often shipping the read cutover and the old-column drop in the same change, which collapses the rollback safety the whole design exists to provide.

Direct answer

I detect leaked secrets with automated scanning, both in CI on every PR and as a pre-commit hook on developer machines, using a dedicated secret scanner rather than relying on human review to spot them. Once found, the sequence is always the same regardless of remediation approach: rotate the credential immediately, then separately decide whether the exposure also needs to be scrubbed from git history, since those are two different problems with two different urgencies.

Structured elaboration

Detection tools. gitleaks and truffleHog scan a repository or a diff for patterns that look like credentials (API key formats, private key headers, common token shapes); git-secrets is a lighter pre-commit-focused option. Many hosted platforms also offer built-in secret scanning on push, which can block the push entirely before the secret ever reaches a shared branch, which is strictly better than catching it after the fact in review.

Immediate reviewer actions. If it's caught in a PR that hasn't merged yet: block the PR, and if the branch hasn't been pushed to a shared/protected branch, the author can often just rewrite their local history (amend or interactive rebase) and force-push a clean version, meaning the secret may never actually land in the repository's permanent history at all. Either way, treat the credential as compromised the moment it left a developer's machine, since it may already be visible in CI logs, in anyone who pulled the branch, or cached by the hosting platform itself, regardless of whether it ever reaches the default branch.

Secret rotation. Rotate first, always, independent of whatever happens to git history: generate a new credential, update every system that consumes it (deployment pipeline, running services, other developers' local environments), verify the new one works, then revoke the old one. This is the step that actually closes the exposure; history cleanup on its own does not, since a secret already scraped or cloned by someone stays valid until it's rotated.

Cleaning git history. Only needed if the secret already reached a shared branch. git filter-repo is the currently recommended tool for rewriting history (it replaced the older, simpler BFG Repo Cleaner as the generally recommended option, though BFG is still common and simpler for basic cases). Both require a force-push and coordination: every existing clone and open PR based on the old history becomes stale and needs to be re-cloned or rebased, which is real organizational disruption, not just a technical step.

Worked example

An AWS access key literal shows up in config.py in an open PR. As reviewer, I block the merge immediately and comment describing exactly what to do: remove the key from the diff, and separately (in parallel, not instead of) notify whoever owns that AWS account to rotate the key right now, since the moment it was pushed to the PR's remote branch it was visible in GitHub's systems and CI logs regardless of whether the PR ever merges. Because the PR hasn't merged to the default branch, the author can amend the offending commit locally and force-push the corrected branch, meaning the key likely never needs a full history rewrite of the shared branch, only its own rotation.

Trade-offs and pitfalls

Rewriting shared git history is disruptive enough (a mandatory force-push, every collaborator needing to reset their local clone) that many teams choose to rotate-and-leave-history-alone rather than rewrite, accepting that the old, now-revoked value stays visible in history forever; that's a reasonable trade when the credential can be fully rotated, and a much weaker option when the "secret" is something that can't be rotated, like a hardcoded document containing real customer data. The most common pitfall is treating history rewriting as the fix and skipping or delaying rotation, when rotation is the step that actually stops the credential from being usable, and history cleanup by itself does nothing about a secret that was already copied somewhere before it was removed.

Direct answer

The function has one real correctness bug (l.split()[2] assumes every line has at least three whitespace-separated tokens, and crashes on any line that doesn't) plus several maintainability smells: a single-letter variable name that's easy to misread, a hardcoded print instead of a return value, and no way to unit test it without capturing stdout. The fix is to make it a pure function that returns a dictionary of counts, validate or skip malformed lines instead of assuming they're well-formed, and push the printing to a separate, thin caller.

Structured elaboration

Correctness problem. level = l.split()[2] indexes into the result of split() without checking its length first. Any line with fewer than three tokens, a blank line, a truncated line, a line in a slightly different format, raises IndexError and crashes the whole batch instead of just that one line.

Maintainability smells.

• l as a variable name for a log line is easy to misread as the digit 1; line costs nothing and is unambiguous.
• The function both computes and prints, so it can never be reused somewhere that needs the counts as data (an API response, a different report format) without also printing.
• print(counts) is untestable directly: a test would have to capture stdout and parse it back out, instead of asserting on a return value.
• No handling for malformed input is documented anywhere, so a caller has no way to know this function is fragile without reading the implementation.

Refactor. Split into a pure counting function and a thin printing function, and decide explicitly what to do with malformed lines (skip them, which is what the version below does) rather than let that be an accident of the indexing bug.

Worked example

The original function actually crashing on a realistic mixed batch of syslog-style lines (one blank line, one truncated line):

def parse(lines):
    counts = {}
    for l in lines:
        level = l.split()[2]
        counts.setdefault(level, 0)
        counts[level] += 1
    print(counts)

sample_lines = [
    "Jun 10 ERROR disk full on /dev/sda1",
    "Jun 10 INFO service restarted",
    "Jun 10 ERROR connection refused",
    "",
    "Jun 10 WARN retrying",
    "malformed",
]

try:
    parse(sample_lines)
except Exception as e:
    print(f"{type(e).__name__}: {e}")

Output:

IndexError: list index out of range

The refactored version, run on the exact same input:

from collections import Counter
from typing import Iterable, Dict

def parse_levels(lines: Iterable[str]) -> Dict[str, int]:
    """Count the syslog severity level (3rd whitespace-separated token) per line.
    Lines with fewer than 3 tokens are skipped rather than raising."""
    counts: Counter = Counter()
    for line in lines:
        parts = line.split()
        if len(parts) < 3:
            continue
        counts[parts[2]] += 1
    return dict(counts)

def print_summary(counts: Dict[str, int]) -> None:
    for level, n in sorted(counts.items()):
        print(f"{level}: {n}")

result = parse_levels(sample_lines)
print(result)
print_summary(result)

Output:

{'ERROR': 2, 'INFO': 1, 'WARN': 1}
ERROR: 2
INFO: 1
WARN: 1

Complexity

Time is O(n * m), where n is the number of lines and m is the average tokens per line from split(); space is O(k) for k distinct severity levels. The refactor doesn't change this, it changes which lines survive to be counted at all.

Edge cases

• A blank line or a line with fewer than 3 tokens is now skipped silently instead of crashing the whole batch. Whether "skip silently" or "skip and log a warning" or "raise in a strict mode" is correct depends on whether malformed lines are expected noise or a symptom of an upstream bug, worth asking the author.
• A line where the 3rd token isn't actually a severity level (a genuinely different log format mixed into the same file) will still get counted as if it were one; parse_levels trusts position, not content, same as the original.
• An empty lines iterable returns {} cleanly in both versions.

Trade-offs and pitfalls

Silently skipping malformed lines trades visibility for robustness: it keeps the batch from crashing, but a caller who cares about data quality needs a way to know how many lines were skipped, which this version doesn't surface. A stricter parser using a regex anchored to the real syslog format would catch more malformed input explicitly, at the cost of more code and a format assumption that's easier to get wrong than "at least 3 tokens." The most common wrong turn here is fixing only the crash (add a length check) without also fixing the testability problem (still printing instead of returning), which leaves the function just as hard to unit test as before.

Direct answer

A Terraform-module checklist at org scale has to cover four layers: the module's public interface (inputs and outputs), its internal implementation quality, its security posture, and its operational safety (versioning, blast radius). Each layer matters more, not less, as more teams consume the module, because a mistake in a shared module multiplies across every consumer's pull requests (PRs, proposed code changes submitted for review) instead of staying contained to one team.

Structured elaboration

Worked example

A platform team reviews a change to a shared vpc Terraform module used by 30 downstream service repositories. The change adds a new enable_flow_logs variable defaulting to true, and removes an old legacy_subnet_cidr variable outright. Applying the checklist: the new variable has a type, a description, and a sensible default, so the interface item passes. But removing legacy_subnet_cidr outright fails the backward-compatibility item, since any of the 30 consumers still passing that variable would fail their next terraform plan with an "unsupported argument" error, all at once, with no warning. The reviewer requires the author to deprecate it first (keep the variable, mark it deprecated in the description, stop using it internally) rather than remove it in the same release, and to bump the module's major version so consumers opt in on their own schedule instead of being broken automatically on their next terraform init.

Trade-offs and pitfalls

• A checklist this thorough adds real review time to every module PR; scope its strictness to modules with many consumers, a single-team internal module doesn't need the same bar
• Security scanners produce false positives on legitimate exceptions, like an intentionally public storage bucket for static assets; the checklist needs an explicit, documented suppression mechanism, not a blanket bypass
• Over-indexing on interface stability can make a module slow to evolve; balance it with a clear deprecation and major-version-bump policy rather than freezing the interface forever
• Skipping the "why it matters at scale" framing turns this into generic Terraform best practices instead of an org-scale review lens; the whole point is that consumer count changes the cost of every mistake

Direct answer

I check the code for the things that turn a normal provisioning run into an expensive or throttled one: whether it respects the provider's API rate limits, whether it batches calls instead of making one API call per resource, whether retries use backoff instead of hammering a failing endpoint, whether resource sizes default to something reasonable, and whether re-running the script is safe (idempotent) rather than creating duplicate, billable resources.

Structured elaboration

• Rate-limit awareness. Why it matters: every cloud provider throttles API calls per account or per project, and hitting that limit turns a fast run into a slow one full of failed requests and retries. How I verify it: check whether the client respects the provider's documented per-second or per-minute limit (many SDKs expose this directly), and look for a test that simulates a throttled (HTTP 429) response and confirms the code backs off instead of hammering the endpoint again immediately.
• Batching. Why it matters: creating, updating, or tagging resources one API call at a time multiplies both latency and the chance of hitting a rate limit, when many providers support a bulk operation that does the same work in one call. How I verify it: look for a loop making one API call per resource where a bulk equivalent exists in the provider's API, and check whether the batch size used is close to the provider's documented maximum per call.
• Retry and backoff policy. Why it matters: a naive retry-immediately loop against a struggling API amplifies the problem instead of recovering from it, and racks up cost from repeated attempts. How I verify it: confirm retries use exponential backoff with jitter (randomized delay, to avoid many callers retrying in lockstep) and a hard cap on attempts.
• Resource size defaults. Why it matters: an oversized default (a large instance type where a small one would do) silently inflates the monthly bill for every resource created with that default; an undersized one hurts performance instead. How I verify it: check that instance/resource sizes come from an explicit, reviewed configuration rather than a hardcoded value buried in the script, and look for a cost-linting check (policy-as-code) that flags anything above an agreed tier.
• Idempotence. Why it matters: if re-running the script after a partial failure creates duplicate resources instead of recognizing what already exists, every retry becomes extra, unwanted cost. How I verify it: check that resources are created with a stable, deterministic identifier (a name or tag derived from the input, not a random one) so a second run can detect "this already exists" instead of blindly creating it again, and look for a test that runs the script twice and asserts the second run is a no-op.

Worked example

A provisioning script includes this loop, creating VMs one at a time with a hardcoded large default size:

for name in vm_names:
    client.create_instance(name=name, machine_type="n1-standard-8")

Review comments: no rate-limit handling (a burst of vm_names will start hitting 429s partway through with no backoff), no batching (most providers support a bulk-create call for exactly this case), an oversized hardcoded default (n1-standard-8 for every VM regardless of what it's actually for), and no idempotence check (running this twice after a partial failure creates duplicate VMs for any name that already succeeded). An improved version batches the creates, makes the size a required, reviewed parameter instead of a hardcoded default, and checks for an existing instance with the same name before creating a new one.

Trade-offs and pitfalls

Simulating rate-limit and failure conditions in CI (mocking a 429 response, for example) adds test complexity that a straight-line happy-path test doesn't need, but it's the only way to actually verify backoff behavior rather than assume it works. A common pitfall: treating idempotence as "the script doesn't crash on a second run" when the real bar is "the script doesn't create duplicate billable resources on a second run," which is a stricter and more important guarantee.