Code Review and Working with Existing Codebases Questions

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

My immediate steps are to block the merge, notify whoever owns those credentials so they can be rotated right away, and treat every exposed key or password as compromised regardless of whether the PR has merged yet. Long term, I'd push for two things together: automated secret scanning that blocks a push before it ever lands, and a real secrets-manager so hardcoding a credential as a source constant stops being something a developer would even reach for.

Structured elaboration

Immediate steps.

• Block the PR with a clear comment: which lines, why they can't merge as-is.
• Notify the owner of each credential (the team responsible for that database, that API) so rotation starts immediately, in parallel with fixing the code, not after.
• Treat the credentials as compromised the moment they were pushed, even to a non-default branch, since they were already visible in the hosting platform's systems and any CI run's logs.

Remediation if already committed to shared history. Rotate every exposed credential first; that's the step that actually stops them being usable. Separately, decide whether history needs rewriting: if the branch is shared or already merged, a tool like git filter-repo can remove the secret from history, but that requires a force-push and everyone with a clone re-syncing, which is real coordination overhead, not a quick fix.

Long-term prevention policy.

• Automated secret scanning wired into CI on every PR, and ideally as a server-side push-protection check that rejects the push outright before the secret ever lands in the repository, which is strictly better than catching it in review after the fact.
• A pre-commit hook (the pre-commit framework, running something like detect-secrets or gitleaks) catches most cases even before a push happens.
• A managed secrets store (a cloud provider's secrets manager, or a tool like Vault) so credentials are injected at runtime rather than living in source at all; combined with short-lived, role-based credentials instead of long-lived static keys where the platform supports it.
• A written policy in the contribution guidelines stating plainly that credentials never go in source, plus periodic scans of existing history to catch anything that predates the automated checks.

Worked example

A reviewer notices DB_PASSWORD = "hunter2" hardcoded in settings.py, already merged two releases ago. Immediate step: rotate the database password right away, since it's been live and exposed in history for two full release cycles, not just caught in an open PR. Then: add settings.py's pattern to the secret-scanner's ruleset so this specific style of hardcoded constant is caught automatically going forward, replace it with a read from the team's secrets manager, and separately schedule the history rewrite (or accept the rotated-but-not-rewritten trade-off, documented as a deliberate decision) since this secret already reached the default branch and every existing clone.

Trade-offs and pitfalls

Push-protection style scanning that blocks a push outright is the strongest prevention, but it will occasionally false-positive on something that looks like a secret and isn't, which needs a fast override path or it trains developers to route around the check entirely. Migrating fully to a secrets manager and short-lived credentials is a real engineering investment, not a one-line config change, so a reasonable interim step is "no new hardcoded secrets, enforced by CI" while the migration for already-existing ones happens on its own timeline. The most common pitfall in the immediate response is fixing the code (removing the line from the diff) while forgetting that the review only ever showed you the PR, not everywhere else that credential might already be exposed.

Direct answer

I sequence the refactor so the public interface changes land first, additively, before internals move; I migrate call sites one at a time behind a feature flag with small commits that each leave the system green, and I pick a rollback point at every seam so the whole plan is expand-migrate-contract rather than one big merge.

Structured elaboration

Sequencing principle: expand, migrate, contract

1. Expand: add the new internal implementation alongside the old one, without removing anything
2. Migrate: switch call sites over incrementally, one at a time, behind a flag
3. Contract: once every call site is migrated and validated in production, delete the old code path in its own final, low-risk pull request (PR, a proposed code change submitted for review)

Interface first

If the refactor changes a library's public interface, not just its internals, land the new interface signature first as a strictly additive change (a new method or overload alongside the old one) before implementation work even starts. This lets consumer teams see or start adopting the new shape while the internals are still being built, instead of waiting for one giant landing.

Verify against real consumers, not just the library's own tests

Before merging any internal change, run each consumer's recorded expectations of the library (its actual inputs and outputs, sometimes called a consumer-driven contract) against the new implementation. This catches the case where the internals pass the library's own unit tests but silently break an assumption a downstream consumer relied on that the library never tested itself.

Branching and commit order

Prefer small commits on a short-lived branch or trunk-based development, with the feature flag doing the isolation, over a long-lived branch. A stale long-lived branch reintroduces the exact big-bang-merge risk this plan exists to avoid. Order commits so each one compiles, passes CI (continuous integration, the automated build/test pipeline), and leaves the system deployable: add the new path unused, add the flag defaulting off, migrate the lowest-risk call site, validate, migrate the rest in ascending risk order, then remove the old path.

Test strategy at each step

• Characterization tests (tests that pin down what the old code currently does, written before touching it) on the existing implementation first
• Consumer contract tests run on every PR in the sequence, not only the last one
• Shadow-run the new path against the old path's output on real traffic where feasible, before flipping the flag for real

Rollback points

Every PR that flips a flag default is a rollback point: flipping the flag back is the fast rollback, no code revert or redeploy needed. The final "delete the old path" PR is the one true point of no return, so it ships last, only after a full soak period with the flag on.

flowchart LR
    A["PR1: add new path, flag off"] --> B["PR2: add consumer contract tests"]
    B --> C["PR3: implement new internals"]
    C --> D["PR4: migrate lowest-risk consumer"]
    D --> E["PR5-7: migrate remaining consumers"]
    E --> F["PR8: delete old path"]

Worked example

Refactoring a core PaymentValidator library used by 4 internal services, changing its algorithm from a synchronous rule chain to a rule-table lookup for performance. PR1 adds validate_v2() alongside the existing validate(), unused, behind a payment_validator_v2 flag defaulting off, plus characterization tests pinning validate()'s current output on 50 representative inputs. PR2 adds contract tests recording each of the 4 consumers' actual usage patterns against validate(). PR3 implements validate_v2(), which must pass every contract test. PR4 migrates the lowest-risk consumer, an internal reporting service, behind its own flag, monitored for a week. PR5-7 migrate the remaining 3 consumers one at a time, with live payment processing last, each with its own soak period. PR8, once all 4 consumers are on v2 and soaked through a full billing cycle, deletes validate() and the flag. If PR5's shadow comparison shows a discrepancy in production, the rollback is flipping that one consumer's flag back, not reverting code.

Trade-offs and pitfalls

• Expand-migrate-contract takes longer in calendar time and leaves two code paths live temporarily, itself a maintenance cost and a place bugs can hide (fixing one path and forgetting the other)
• Skipping consumer contract tests and relying only on the library's own unit tests is the classic way this goes wrong: internals look correct in isolation but violate an assumption a consumer depended on
• A stale long-lived branch reintroduces the exact big-bang-merge risk the plan exists to avoid; if small trunk-based commits aren't feasible, that's a signal the split needs to be finer-grained, not a reason to accept one large final merge
• Deleting the old path before every consumer has soaked removes the rollback safety net that made the whole plan low-risk in the first place

Direct answer

I pick the test layer by how expensive and how real it needs to be to catch the risk: fast, free static checks for every PR, real-but-throwaway cloud resources for anything that has to prove the infrastructure actually works, and a full end-to-end pass sparingly, since it's the slowest and most expensive layer. For a module provisioning a VPC (virtual private cloud, an isolated network), subnets, and an Auto Scaling group (ASG, a group that automatically adds or removes instances to match demand), all three layers earn their place.

Structured elaboration

Unit-style / static tests. Tools: terraform validate for syntax, tflint for provider-specific correctness and style, and a policy-as-code tool (Checkov or Open Policy Agent) for security and convention rules. What they validate: the configuration is syntactically valid, uses provider arguments correctly, follows naming conventions, and doesn't violate a known policy (a publicly-open security group, a missing required tag). How to run safely: on every PR, no cloud resources touched, so it's fast and free to run as often as needed.

Integration tests. Tools: Terratest (a Go testing library for Terraform) or a similar framework that can actually run terraform apply, inspect the result, then terraform destroy. What they validate: that the module actually creates what it claims to, for example that subnet CIDR blocks (Classless Inter-Domain Routing blocks, the notation for an IP address range like 10.0.0.0/24) are correctly sized and non-overlapping, that subnets land in the intended availability zones, and that the ASG's launch configuration references a valid, existing image. How to run safely: against an isolated, short-lived sandbox account or project, using scoped, least-privilege credentials, with every resource tagged for the test run and torn down automatically, including on failure, so a crashed test doesn't leave orphaned billable resources behind.

End-to-end tests. What they validate: that the pieces actually work together as a live network, for example that an instance launched by the ASG in a private subnet can reach the internet through a NAT gateway, or that a load balancer in the public subnet can actually route to instances in the private one. How to run safely: in a dedicated, ephemeral environment, run less frequently (on merge to the main branch, or nightly) rather than on every PR, since it's the slowest and most expensive layer, with the same automatic teardown and budget guardrails as integration tests.

Worked example

The module provisions a VPC with CIDR block 10.0.0.0/16, split into three public subnets across three availability zones: 10.0.0.0/24, 10.0.1.0/24, and 10.0.2.0/24. Each of those is a distinct, non-overlapping /24 (256 addresses) carved consecutively out of the /16. A unit-style check confirms the CIDR math is valid and non-overlapping without touching any cloud account at all. An integration test actually applies the module in a sandbox account and asserts, via the cloud provider's API, that three subnets exist, each in a different availability zone, each with the expected CIDR. An end-to-end test then launches a real instance through the module's Auto Scaling group and confirms it can reach an external endpoint, proving the NAT gateway and routing are actually wired correctly, not just declared correctly.

Trade-offs and pitfalls

Running integration and end-to-end tests on every PR would catch problems faster but at real cost and risk: real cloud resources cost money even torn down immediately, and a bug in the teardown logic itself can leave orphaned resources running indefinitely if there's no separate cleanup safety net. The most common pitfall is skipping the unit-style layer because it "doesn't test anything real," when in practice it's what catches the majority of simple mistakes (a bad CIDR, a disallowed instance type) before they ever cost a cloud API call, leaving the expensive layers to catch the smaller number of problems that only show up when resources actually exist.

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.