Code Review and Working with Existing Codebases Questions

Direct answer

Don't let "fix security later" comments merge invisibly. Treat each one as an immediate, lightweight risk assessment: read what it actually concerns, estimate roughly how bad it would be if exploited, and use that to decide whether the PR (pull request) can merge as-is, merge with a tracked mitigation, or must be blocked until it's fixed. Attach a label, an owner, and a deadline to whichever one applies, so it doesn't quietly become permanent, invisible technical debt.

Structured elaboration

Immediate triage. For each TODO, identify the affected area, authentication, cryptography, input validation, or secret handling, and reason briefly about how it could actually be exploited. Classify severity: critical (a remote attacker could read or corrupt data), high (privilege escalation), medium (a local or low-likelihood information leak), low (hardening or cleanup with no known concrete exploit path).

Merge decision by severity. Critical or high severity blocks the merge until it's fixed, or has an explicit, reviewed compensating control (for example, the new code path is fully behind a feature flag that's off in production). Medium severity can merge with a documented mitigation and a tight timeline. Low severity can merge with a tracked follow-up and a longer timeline.

A concrete policy. Labels: security-blocker (must fix before merge), security-urgent (fix within the current sprint), security-techdebt (scheduled within a few sprints, with a named owner). A hard rule that a PR cannot merge while it carries a security-blocker label. Every security-* label requires a linked ticket and an assignee, so the work doesn't just live as a comment in an already-merged PR where nobody will see it again.

Reviewer actions. Leave an inline comment naming the specific risk and the label being applied, and for anything critical, loop in a security-focused teammate directly rather than deciding alone.

Worked example

An illustrative review comment applying this to the scenario in the question:

"This TODO, 'fix security later', sits in the token-validation path. If an attacker can skip validation here, that's a critical authentication bypass, not a stylistic nit, regardless of how it's phrased. I'm labeling this security-blocker. Can we either fix it in this PR, or gate the new code path behind a feature flag that stays off in production until it's fixed, with a linked ticket and an owner?"

Trade-offs and pitfalls

A policy that's too strict, blocking on every single TODO regardless of severity, pushes people to stop writing TODOs at all, which hides risk instead of surfacing it. A policy that's too loose lets "fix later" quietly become "fix never." The real lever is making the label-ticket-deadline process cheap enough that engineers actually use it instead of working around it, plus a regular cadence for revisiting the security-techdebt backlog so it doesn't just grow unbounded and become the next audit's bad surprise.

Direct answer

Before a human looks at the code, I want checks that are cheap, deterministic, and fast: build/compile, the existing test suite, correctness-oriented linting, formatting, and security or secret scanning. Build, tests, correctness linting, formatting, and security scanning are effectively mandatory since each is cheap and prevents real harm; a coverage-delta threshold is the one I'd keep optional or advisory.

Structured elaboration

Why this ordering matters

Machines are fast, consistent, and never get tired or skip a check under time pressure, exactly the trait humans lose in a rushed review. This reframes the human reviewer's job: not "did the tests pass," but "does this change make sense, is the design right, are there edge cases the tests don't cover."

Worked example

A PR adds a new /export endpoint. Continuous integration (CI, the automated build/test pipeline) runs: the build passes; the existing 340-test suite passes; linting flags an unused import, requiring a one-line manual fix; the secret scanner catches a hardcoded key accidentally left in a test fixture and blocks the merge until the author replaces it with an environment-variable reference; the coverage-delta check shows the new endpoint added 0 new tests against 45 new lines and posts an advisory warning, not a block, prompting the author to add a test before requesting review. Only after the build, existing tests, and the secret-scan block clear does the PR reach a human reviewer, who focuses entirely on whether the endpoint's authorization check is correct, something none of the automated checks could evaluate.

Trade-offs and pitfalls

• Too many mandatory gates slows every PR for marginal benefit; keep the mandatory set to checks that catch real, expensive-to-miss problems, not personal style preferences
• A coverage-delta threshold enforced as a hard block gets gamed with low-value tests written just to hit a number; keep it advisory and let a human interpret it
• Automated checks can produce false confidence, "CI is green" means the mechanical bar was cleared, not that the design is right
• Security or secret scanners produce false positives; if they cry wolf too often, engineers start ignoring or bypassing them

Direct answer

Don't accept "the tests were flaky" as self-certifying. Treat a large coverage drop as a risk decision that needs evidence: ask for the flakiness data, look for a way to keep the safety net while removing the noise (fix, quarantine, or narrow the test before deleting it), and if deletion really is the right call, document the risk and the reasoning so a reviewer looking at this months later can see what trade-off was made and why.

Structured elaboration

Evidence to request. Continuous integration (CI, the automated pipeline that builds and runs tests on every change) history for the removed tests over recent weeks, showing pass/fail per run; failure logs for a few representative failures; any links to prior attempts to fix them; and a plain description of what behavior each removed test actually asserted, since "flaky" and "not worth having" are two different claims that both need their own evidence.

How to measure test value. A useful starting point is a flakiness rate: how often a test fails non-deterministically, out of how many runs, over a recent window. Teams commonly treat anything in the low single digits as worth investigating and anything much higher as a real problem, but that's a tunable convention to calibrate to your own pipeline, not a fixed rule. Weigh that flakiness rate against the test's actual defect-detection value: does it cover a code path that has broken in production before, or a genuinely critical invariant, versus an incidental implementation detail that happens to be covered along the way?

Alternatives to removing tests. Fix the root cause (replace a real sleep-based wait with an explicit event or a fake clock, mock a flaky network call, isolate shared test state between runs); quarantine the test into a separate, still-running "known flaky" suite that's tracked but doesn't block merges; add a bounded, justified retry for a genuinely transient cause; or narrow the test so only the brittle assertion is removed, keeping the rest of the coverage.

Documenting the decision. Put the evidence, the alternatives considered, and the reasoning for why deletion won (or why a different fix was chosen) into the PR (pull request) description or a linked ticket, with a named owner and, for anything quarantined rather than truly resolved, a re-evaluation date. A future reviewer looking at the coverage graph should be able to find out why it dropped and whether that was a considered decision or a silent regression.

Worked example

An illustrative review comment applying this process to the scenario in the question:

"This PR removes 4 tests and drops coverage by a double-digit number of percentage points. Before I approve: can you attach the CI history showing these 4 failing intermittently over the last several weeks, and confirm whether you tried at least one stabilization approach, for example replacing a sleep-based wait with an explicit event, before deciding to delete them? If they're genuinely not fixable in a reasonable time, I'd rather see them moved into a quarantined suite with a tracking ticket and an owner than deleted outright, so we don't lose the safety net for the code path they cover. Either way, please add a short note to the PR description with the evidence and the decision, so this is traceable later."

Trade-offs and pitfalls

Quarantining without real follow-through is functionally the same as deletion but looks safer on paper, so a quarantine ticket needs a real owner and a real re-evaluation date, not just a label that never gets revisited. Sometimes a test genuinely is asserting an implementation detail rather than behavior, and removing it is a real improvement, so don't reflexively block every coverage drop, the point is evidence, not a floor on the percentage. The biggest pitfall is judging purely by the coverage number: two tests covering the same trivial line count the same toward that percentage as one test covering a critical path that has actually broken production before, so the review has to look at what was removed, not just how much.

Direct answer

I track a mix of speed, thoroughness, and outcome metrics, because optimizing for any one alone, especially speed, invites gaming. Below are five metrics with how I'd collect each and its specific gaming risk, plus how a team would use target thresholds operationally instead of just observing raw numbers.

Structured elaboration

Five metrics

Target-behavior and threshold framing

Rather than just reporting raw numbers, a team typically sets a target band per metric, for example "median time-to-first-review under 8 business hours" or "defect-escape rate under a set percentage of merged PRs," and treats the metric as healthy while inside the band, investigating only when it drifts outside. Thresholds should come from the team's own historical baseline, not an industry number, since review load and risk profile vary a lot by codebase. Alert on trend, not a single data point: a metric crossing its threshold for one week is noise, three consecutive weeks is worth a retro.

Using these without them backfiring

• Report at the team level, never rank individual reviewers publicly, individual rankings are exactly what triggers the gaming behaviors above
• Pair a speed metric with a quality metric (time-to-first-review alongside defect-escape rate) so a team can't "win" by optimizing speed alone
• Revisit the metric set periodically; a metric that's been stable for months provides less signal than one that's actively moving

Worked example

A platform team sets these threshold bands: time-to-first-review under 8 business hours (current baseline 6h), rubber-stamp rate under 20% (current baseline 15%), defect-escape rate under 5% of merged PRs (current baseline 3%). In week 3 of a sprint, time-to-first-review spikes to 18 hours for two consecutive weeks while rubber-stamp rate climbs to 35% at the same time. Read together, this isn't "reviewers are slow," it's "reviewers are overloaded and starting to skim": three reviewers are pulled onto a separate incident response for two weeks, leaving review understaffed. The team's response is temporary reviewer reassignment, not a mandate to review faster, because the paired metrics pointed at capacity, not diligence.

Trade-offs and pitfalls

• Any single metric optimized in isolation gets gamed; the value is in reading two or three together, never acting on one alone
• Metrics tied to individual performance reviews reliably produce the gaming behaviors listed above; keep them team-level diagnostics
• Defect-escape rate has a lag (bugs surface weeks later), so it's a trailing indicator, useful for validating whether a change worked, not for fast feedback
• Thresholds set once and never revisited stop being meaningful as the team, codebase, or risk profile changes; review them roughly quarterly

Direct answer

Reviewing a memoization pull request (PR) means checking three separate things, not just "does it look right": is the cache key actually correct, is the cache's memory use bounded, and is it safe if the function can be called from more than one thread at once. Leave specific, line-referenced comments with a proposed fix, not a general "looks good."

Structured elaboration

Memoization means caching a function's return value keyed by its input, so a repeat call with the same input is served from the cache instead of recomputed.

Correctness

• Does the cache key capture every input that affects the output, including default/optional arguments and any instance state for a method? A key that's missing one input will return a stale, wrong answer for calls that only differ in that field.
• Is the function actually deterministic and side-effect-free for a given input? If it has a side effect (an increment, a log line, a downstream call), that side effect now only fires on a cache miss, which the PR should call out as an intentional, documented behavior change.
• Is there any invalidation path, or does a cached value live forever even after the underlying data it depends on changes?

Memory safety

• Is the cache bounded, for example an LRU (least-recently-used) cache with a maximum size, or a TTL (time-to-live) that expires old entries, or can it grow without limit and eventually cause an OOM (out-of-memory) failure on a long-running process?
• If the cache keys or values hold references to large or long-lived objects (like an instance whose method is being memoized), does the cache keep those objects alive longer than intended? That's a slow memory leak, not a crash, so it's easy to miss in a quick review.

Thread-safety

• If two threads call the function with the same new argument at almost the same moment, what happens? A plain dict/HashMap being read and written by two threads without synchronization can corrupt its internal structure, not just return a stale value.
• Is the "check cache, then compute, then store" sequence atomic, or can two threads both miss the cache and both do the (possibly expensive) work at once? That's usually acceptable for a pure read-through cache, but it should be a stated decision, not an accident.

Worked example

Concrete review comments for this PR, each pointing at a specific problem and a fix:

• "Nit: please add a test that calls this function twice with identical inputs and asserts the second call returns the cached result without re-running the underlying computation (for example by mocking the expensive call and asserting it was invoked once)."
• "This cache is an unbounded dict, so a long-running process will keep every distinct input in memory forever. Can we bound it, for example with functools.lru_cache(maxsize=...) in Python, or an explicit LRU wrapper in other languages?"
• "This cache isn't synchronized. If this function can be called from more than one thread, either guard the check-then-store sequence with a lock, or use a data structure built for concurrent access (a ConcurrentHashMap in Java, or a dict behind an explicit lock in Python)."
• "Key correctness: the cache key here only includes user_id, but the function's output also depends on as_of_date. Two calls for the same user on different dates will incorrectly return the same cached value."

Trade-offs and pitfalls

A lock around the whole cache is simple but serializes every call, even ones for different keys; a per-key lock or a genuinely concurrent map avoids that contention at the cost of more complex code, worth it once the function is actually called concurrently at meaningful volume, not before. An unbounded cache is the fastest option and the easiest to get wrong in production, since it fails silently (memory just grows) until it doesn't. If the cached value is a mutable object, a caller who mutates what they got back can corrupt the cached copy for every future caller unless the function returns a defensive copy, that's a subtle correctness bug worth its own comment when the return type is mutable.