Code Review and Working with Existing Codebases Questions

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

Treat code review as one of several teaching tools, not the only one: use written comments for things that are efficient to explain in text, switch to pairing for anything that needs real back-and-forth, and track whether the same class of comment keeps recurring as the honest signal of whether the mentee is learning, not just complying.

Structured elaboration

Structuring comments
Lead with the "why," not just "change this to that." Separate must-fix from optional or learning-opportunity comments explicitly, so the mentee isn't guessing at severity. Ask a question ("what happens if this list is empty?") instead of dictating the fix when the goal is for them to reason through it themselves; state it directly when time pressure or risk is high enough that the learning moment can wait.

Pair-programming versus commenting
Pair when the concept is genuinely new to them, a new pattern or a new part of the codebase, or when a comment thread has gone back and forth more than twice without converging. Use written comments for anything they've seen before and just need a nudge on, since it's asynchronous and doesn't interrupt their flow.

Targeted learning resources
Link to the specific doc, prior PR (pull request), or style-guide section that addresses the exact gap, not a generic "read the docs on X." Even better: point to a real example already in the codebase that does it well, since it's concrete and has already passed review.

Measuring progress
Track whether the same category of comment (e.g. "add error handling," "extract this function") shows up less often across their later PRs. A good sign is the mentee starting to anticipate the class of feedback you'd give and addressing it before you comment. A bad sign is the same class of feedback repeating PR after PR with no change in how the code arrives.

Worked example

Mentoring a junior engineer over a couple of months: early PRs draw frequent comments on missing error handling and untested edge cases. When they hit an unfamiliar part of the codebase (an async job queue), that becomes a pairing session rather than a comment thread, since it's genuinely new. For a recurring "extract this into a function" pattern, the mentor points them at a specific earlier PR in the codebase that does it well, rather than a generic style guide link. The measurable, honest signal by the third or fourth PR: the error-handling comments mostly stop appearing, and their tests start covering edge cases unprompted, not a fabricated precise percentage.

Trade-offs and pitfalls

Mentoring through review can tip into micromanaging, rewriting their solution in comments instead of letting them arrive at it themselves. It can also become one-sided, where the mentor never learns anything from the mentee's perspective on the code. A common wrong turn is being so gentle that a genuinely blocking issue reads as optional and ships anyway, which helps no one.

Direct answer

Don't let the disagreement stay abstract: get concrete about what's actually at stake, how many callers exist and how costly a rename would be later, state your reasoning once clearly, and if it's still unresolved, use a fast, low-cost tiebreaker (a quick prototype, a third opinion, or an explicit decision-maker) rather than letting the thread drag on.

Structured elaboration

1. Separate preference from cost. Ask explicitly whether this is reversible. An API used by many callers is expensive to rename later, a breaking change or a long deprecation, so the bar for insisting on a preferred name should scale with how many callers exist and how costly a change would be, not with how strongly either person feels.
2. State your reasoning once, concretely, and ask for theirs. Name the specific downside, not just "I don't like this name": "getUser reads as a lookup that returns null, but this always creates one if missing, that's surprising for the many call sites that assume a pure read."
3. If still unresolved, do a quick, cheap check instead of more debate. Grep the existing callers to see how the API is actually used, or spend ten minutes prototyping both names in real call sites to see which reads better. This replaces opinion with evidence.
4. If still stuck, get a third opinion fast. A tech lead, or whoever owns that part of the API, makes the call; anyone involved can request this, and it's cheap to ask compared to letting the PR (pull request) stall.
5. Once decided, commit and close the thread visibly. Whoever didn't get their preference says so out loud ("okay, going with X") so the disagreement doesn't linger as an unresolved grudge, and the reasoning gets written down briefly (in the PR or a short doc) so the next naming debate doesn't restart from zero.

Worked example

Two engineers disagree on getUser versus findOrCreateUser for a method with roughly forty existing call sites. The reviewer greps the callers and finds most assume a pure lookup with no side effects. Bringing that evidence back, "most callers don't expect this to write anything, the name should signal that," the author agrees the evidence changes the calculus, and they land on findOrCreateUser. A short note is added to the PR explaining the reasoning so a future reader understands why the name is what it is.

Trade-offs and pitfalls

Escalating to a tech lead for every minor naming choice trains the team to stop deciding things themselves. Conversely, letting every naming disagreement burn hours of back-and-forth stalls delivery on something that's often genuinely reversible later behind an alias. The failure mode to avoid is a reviewer pulling rank, "just do it my way, I'm the reviewer," instead of showing the reasoning; that resolves the immediate PR but erodes trust for the next one.

Direct answer

I'll walk through a real example from reviewing infrastructure-as-code: catching a security group change that would have opened a database to unrestricted inbound access, and how that turned into both an immediate fix and a lasting change to how the team reviews that class of change.

Structured elaboration

Situation. I was reviewing a routine-looking Terraform PR meant to let a new internal reporting service reach a database. Task. As the reviewer, my job was to catch anything that changed the actual security posture of that database, not just check that the Terraform plan applied cleanly. Action. I noticed the PR's security group rule used 0.0.0.0/0 for the inbound CIDR range (CIDR, Classless Inter-Domain Routing, is the notation for writing a whole block of IP addresses as one value; 0.0.0.0/0 specifically means "every possible IP address"), instead of the reporting service's specific subnet, almost certainly copy-pasted from an example rather than deliberately chosen. Result covers what happened next, both the immediate fix and the longer-term process change, below.

Worked example

I marked the PR as blocking with a specific comment explaining the exposure: this rule would allow any host on the internet to attempt a connection to the database's port, not just the internal reporting service the PR was supposedly scoping access to. I proposed the concrete fix, scoping the rule to the reporting service's actual subnet CIDR instead, and pushed a one-line diff to make it easy for the author to just take. Given the severity, I also flagged it in the team's on-call channel rather than waiting for an asynchronous PR reply, since an already-merged version of a similar mistake elsewhere in the account was worth checking for immediately, not after the PR conversation finished. The PR was updated and merged with the corrected, scoped rule within the hour. Separately, I proposed and helped add an automated policy check (using Open Policy Agent, a policy-as-code tool that can evaluate Terraform plans against written rules) to the CI pipeline that specifically rejects any security group rule opening a sensitive port to 0.0.0.0/0 without an explicit, reviewed exception, so this exact mistake can't reach production again without a deliberate override.

Trade-offs and pitfalls

The judgment call in a story like this is deciding how loudly to escalate: raising it in a live channel instead of just a PR comment was the right call given the actual exposure, but that same urgency would be the wrong tone for a much lower-severity finding, and using it there would just train the team to tune out urgent-sounding messages. A pitfall to watch for when telling this kind of story is stopping at "I found the bug and it got fixed," without the automation follow-up: the more convincing version of this story is one where the process change means the same category of mistake gets caught automatically next time, not just this one instance.

Direct answer

For a migration touching a 5GB, heavy-write table, I check six things specifically: whether the change runs inside a transaction and what that actually implies for locking, how long any lock is held and what kind, whether backfills are batched instead of one giant statement, how the change rolls out (all at once or staged), what's monitored during and after, and whether there's a real rollback path, not just "revert the migration file."

Structured elaboration

• Transactional behavior. Some DDL (data definition language, schema-changing statements like ALTER TABLE) is transactional and can be rolled back cleanly if something fails mid-statement; some isn't, and a failure partway through leaves the schema in a partially-changed state. This differs by database engine and even by the specific kind of ALTER, so I check what's actually true for the database and statement in question rather than assuming.
• Lock duration. The real question is what kind of lock the statement takes and for how long. On Postgres, ADD COLUMN with a constant default has been a fast, metadata-only change since Postgres 11, but adding a NOT NULL constraint that requires validating every existing row, or an index build without CONCURRENTLY, still takes a lock for the duration of that work. On MySQL 8.0+, many ALGORITHM=INSTANT operations (a modifier that changes only table metadata instead of rewriting existing rows, including some column adds) are near-instant, but not all ALTER variants qualify, and older MySQL versions may rebuild the whole table. I check which case this migration actually falls into for the target database and version, not just assume it's cheap.
• Backfills. A backfill on a 5GB table should never be one giant UPDATE, which holds locks and generates a huge amount of write-ahead log or binlog (the record every database write generates so changes can be replayed or recovered) in one shot. It should run in small batches (a bounded WHERE clause, a sleep or throttle between batches), be resumable from where it left off, and be idempotent (safe to re-run a batch that partially completed).
• Rollout strategy. For anything beyond a trivial, fast schema change, I look for a staged rollout: add new structure first without changing behavior, deploy application code that can handle both old and new shapes, backfill, then only later switch behavior over, rather than one migration that changes schema and behavior simultaneously.
• Monitoring. During the migration I want to see replication lag, lock wait time, and query latency on this table specifically, not just overall database health, since a table-specific problem can hide inside a healthy-looking aggregate.
• Rollback. A real rollback plan says what to actually do if something goes wrong mid-migration, not just "we have backups." For a pure schema addition, that's usually just dropping the new column; for anything involving a backfill or application-visible behavior change, it needs to say how to safely stop and either resume or revert without leaving the table in a half-migrated state.

Worked example

The migration adds a NOT NULL column with a default to the 5GB table. On a database and version where that's not a metadata-only operation, the safe sequence is: add the column as nullable with no default first (cheap), backfill it in batches ordered by primary key, verify every row is populated, then add the NOT NULL constraint (which on Postgres can validate against already-known-good data faster once nothing is actually null). A batch looks like:

UPDATE big_table
SET new_col = <derived value>
WHERE id BETWEEN :batch_start AND :batch_end
  AND new_col IS NULL;

run in a loop over consecutive primary-key ranges with a short pause between batches, so the migration never holds a lock or generates a burst of writes for longer than one small batch takes.

Trade-offs and pitfalls

Splitting a migration into more, smaller, safer steps is real added engineering and coordination work compared to "just run the ALTER," and for a genuinely small or low-traffic table that overhead isn't justified; the 5GB, heavy-write detail in the question is exactly what tips the balance toward doing it the careful way. A common pitfall is testing the migration only against a small local database, where an operation that's instant on a toy table can be a multi-minute, lock-holding operation on the real, much larger one, so the review should ask whether this was tested against production-scale data, not just correctness-tested against a handful of rows.