Code Review and Working with Existing Codebases Questions

Direct answer

Don't quarantine and forget. Flaky tests, tests that pass locally but fail intermittently in CI (continuous integration), erode trust in the whole suite fast. Confirm it's actually flaky rather than a real intermittent bug, isolate the likely cause, fix or explicitly quarantine it with a tracked owner, and keep the team visibly informed the whole time so people don't start treating red CI as normal.

Structured elaboration

Confirm it's flaky, not a real bug: re-run the specific test many times in isolation, and also alongside its usual neighbors. If it only fails in combination with other tests, that's a strong clue it isn't really about the test itself.

Common causes, checked in order:

1. Timing or async: does it wait on a real signal that an async operation finished, or on a fixed sleep?
2. Shared or leaked state: does it depend on data left behind by another test, or share a global or database row with a test running in parallel?
3. Test order dependency: does it pass in isolation but fail only after a specific other test runs first?
4. External dependency: does it hit a real network call, the system clock, or unseeded randomness?

Remediate: fix the root cause where possible, mock the external dependency, isolate test data, replace a sleep with waiting on the actual condition. If it can't be fixed immediately, quarantine it explicitly, skip it with a linked ticket and a named owner, rather than leaving it red and training people to ignore CI failures.

Communicate status: post in the team channel when a test is quarantined and why, keep a visible list of currently-quarantined tests so it doesn't become an invisible, growing pile, and set an expectation for when it'll be revisited, since "quarantined forever" behaves the same as deleted, but worse, because it still looks like coverage exists.

Worked example

A test for an async file-upload flow fails about 1 in 20 runs in CI but never locally. Re-running it 30 times in isolation, it passes every time. Re-running the full suite, it fails only when run after a specific test that doesn't clean up its own temp file. Root cause: the earlier test's leftover file makes the upload test see unexpected existing state. Fix: add proper teardown to the earlier test, and switch the flaky test's own wait off a fixed sleep and onto the actual upload-complete signal, reducing remaining timing risk too. Communication: a short note in the team channel when it's identified as flaky, not yet a merge blocker, and a follow-up once it's confirmed fixed.

Trade-offs and pitfalls

Automatically retrying a flaky test until it passes hides the problem instead of fixing it, and the underlying issue can silently get worse over time. Blocking all merges on every flaky failure without triage first punishes the whole team for one bad test and trains people to distrust CI. The right balance is quarantine with a named owner and a deadline, not indefinite silence in either direction.

Direct answer

A pull request (PR, a proposed code change submitted for review) is too large when a reviewer can't hold it in their head in one sitting. I use three heuristics (rough size, whether it mixes unrelated concerns, and how long a careful read actually takes), and I split along seams the codebase already offers: interface first, behind a flag, refactor separate from behavior change, tests first, and by call-site group.

Structured elaboration

Three heuristics for "too large"

1. Size as a rough proxy: diffs over roughly 300-400 changed lines (excluding generated or vendored files) tend to see review quality drop, not a hard rule, a warning sign
2. Single-concern test: can you describe the PR's purpose in one sentence without the word "and"? "Adds retry logic AND refactors the client AND fixes a typo" is really three PRs
3. Review-time test: would a careful reviewer need more than roughly 30-60 minutes to actually read and reason about every changed line? If mechanically reading it takes that long, understanding the trade-offs and edge cases takes much longer

Five techniques to split a large refactor

1. Interface first: land the new function or class signature as an additive change before the implementation that uses it, so the shape of the change is reviewable on its own
2. Feature flag it: land the new code path disabled by default, so the PR that introduces it carries no behavior change and is low-risk to review and merge
3. Refactor, then behavior: separate pure mechanical changes (rename, extract a method, move a file) with zero behavior change from PRs that actually change behavior, so a reviewer never has to untangle "is this different because of the rename or because of a real change"
4. Tests first: land characterization tests (tests that pin down what the old code currently does, written before touching it) or new tests in their own PR before the implementation change, so the implementation is reviewed against an already-agreed spec
5. By layer or call site: split a change touching many call sites into one PR per logical group (by service or module), migrating incrementally rather than all at once

Worked example

A PR replaces a hand-rolled caching layer with a library-backed one across 12 files and 900 lines. The size heuristic flags it (900 well over 400), and the single-concern test flags it too, it both swaps the cache implementation and renames several variables along the way. Split: PR1 (60 lines) is a pure rename with zero behavior change. PR2 (120 lines) adds the new cache client behind a flag, unused. PR3 (80 lines) adds tests characterizing current cache behavior. PR4-6 (roughly 200 lines each) migrate call sites in three groups by module, flag still off, verified against the characterization tests. PR7 (40 lines) flips the flag on in a low-traffic environment first. PR8 (20 lines) removes the old cache code once the new one has run cleanly for a week. Each PR is independently reviewable in under 30 minutes.

Trade-offs and pitfalls

• Splitting purely by line count, ignoring logical seams, can produce PRs that are individually small but incoherent, harder to review than one clear large PR
• Too many tiny PRs adds coordination overhead (more review requests, more continuous integration (CI, the automated build/test pipeline) runs, more chances to merge out of order), there's a floor below which splitting further isn't worth it
• A "pure refactor" PR only earns that label if it really has zero behavior change; verify that with tests, don't just assert it
• The 30-60 minute review-time heuristic is inherently subjective and varies with reviewer familiarity; use it as a gut check, not a hard gate

Direct answer

I treat this as a decision-making process problem, not a technical one: gather input broadly but make the actual call with a small accountable group, pilot the standard on a real team before mandating it org-wide, communicate the reasoning and not just the rule, and measure adoption with an honest, mechanically-enforced signal rather than assuming a written standard changes behavior on its own.

Structured elaboration

Gather input

Survey affected teams for their current conventions and actual friction, not just preferences, so the decision is grounded in real problems (for example, "our formatter conflicts with theirs when we share a monorepo") rather than taste. Reduce the debate to 2-3 genuinely competing proposals rather than open-ended bikeshedding; most style disagreements collapse to a small number of real axes, like import ordering or naming convention.

Weigh technical trade-offs

Favor whichever option has the strongest tooling support (an existing auto-formatter, mature editor integration, linter support) over one that's marginally nicer but manual to enforce, since manual enforcement is where standards quietly die. Weigh switching cost too: a team with a large existing codebase in the new language has more sunk cost in its current convention than a team just starting out.

Pilot before mandating

Pick one or two willing teams to adopt the standard for a real sprint or two, not a toy example, and explicitly ask what broke, what felt like friction, and what they'd change before finalizing anything.

Communicate the decision

Publish the reasoning, not just the rule: why this option over the alternatives, and what trade-offs were accepted. Give a clear timeline, a grace period, and name who to raise disagreement with.

Measure adoption and minimize disruption

Track adoption through the automated formatter or linter's own pass/fail rate in CI (continuous integration, the automated build/test pipeline) across repos, an honest signal since it's mechanically enforced rather than self-reported. Roll out with tools that fix code automatically instead of requiring manual compliance, the single biggest lever for minimizing disruption. Grandfather existing code with format-on-touch (only reformat files as they're naturally edited) rather than one disruptive mass reformat that breaks blame history and floods review queues.

Worked example

Three teams adopting a new backend language disagree on import-ordering and error-handling conventions. I run a two-week input-gathering round: two teams prefer style A, matching their existing microservices; one team already has 40,000 lines in style B, a shared library they don't want to rewrite. Rather than forcing a binary choice, I pick style A as the org standard, since it has better tooling support (an existing auto-formatter plugin), but scope the rollout as format-on-touch: new and touched files get reformatted automatically by CI, while untouched legacy files in the third team's library keep style B until they're naturally edited, with a linter configuration that doesn't flag the legacy files. I publish a short doc explaining the tooling reasoning and the grandfather policy, pilot on the first team for two weeks, adjust the auto-formatter's import-grouping rule after they report false-positive linter noise, then roll out to the other two teams. I track adoption as the percentage of touched files in each repo that pass the new formatter in CI, which climbs past 90% within a month without anyone manually reformatting anything.

Trade-offs and pitfalls

• Forcing a big-bang reformat of an entire existing codebase creates a disruptive diff that breaks git blame and swamps review queues; format-on-touch avoids this at the cost of a longer period of inconsistency
• Deciding by committee vote often produces a compromise nobody's tooling actually supports well; weigh tooling maturity heavily, not just preference counts
• Skipping the pilot and mandating org-wide immediately is the most common way this backfires, since the standard hasn't been tested against a real team's actual workflow
• Publishing a rule without publishing the reasoning breeds quiet non-compliance; people follow standards they understand the "why" of far more reliably

Direct answer

Constructive review feedback that preserves psychological safety (a shared sense that it's safe to be wrong or imperfect without punishment) comes down to a handful of concrete, repeatable practices: address the code rather than the person, lead with intent, label severity honestly, give real praise, offer a concrete alternative, and leave the door open on next steps.

Structured elaboration

At least six concrete practices, and why each helps the author actually receive and act on the feedback:

1. Phrase it about the code, not the person ("this function doesn't handle X" rather than "you forgot X"). It keeps the comment about the artifact, which is easier to hear without feeling personally judged.
2. Lead with a question or the underlying intent ("what happens if the list is empty here?" instead of "you missed the empty case"). It invites the author to reason it through rather than just comply, and softens the tone.
3. Label severity explicitly (blocking versus a "nit:" versus optional). It removes the guesswork of whether every comment is a must-fix, which reduces the feeling of being buried under criticism.
4. Include genuine, specific praise, not filler. It reinforces what to keep doing and signals the review isn't only a list of what's wrong.
5. Give a concrete example or alternative, not just "this is unclear." A vague criticism with no path forward reads as judgment; a concrete suggestion reads as help.
6. Offer next steps when there's no obvious fix ("happy to pair on this if useful"). It shows the reviewer is invested in the outcome, not just gatekeeping.
7. Time the delivery, avoiding a flood of stylistic comments while the core design is still in question, since dozens of comments landing at once reads as harsher than any single one intended.

Worked example

A function is missing a null check. A comment that violates most of these practices: "this is wrong, add a null check." A comment applying several practices at once: "nice catch handling the retry case above! One thing: what happens if user is null here, e.g. a deleted account mid-request? Might be worth an early return. Happy to pair if useful." Same underlying concern, delivered in a way the author can act on without feeling attacked.

Trade-offs and pitfalls

Over-softening a genuinely blocking issue ("just a thought, feel free to ignore") creates ambiguity about severity, and the issue can ship anyway. Psychological safety is not the same as avoiding disagreement; being clear that something is blocking is itself respectful, because it's honest rather than vague.

Direct answer

I would not try to find a production memory leak purely by reading code. I'd combine dynamic evidence, actually profiling the running agent under load, with a targeted code-review checklist for the patterns most likely to cause a slow, steady leak, and then lock in whatever I find with a CI check so the same class of bug can't silently come back.

Structured elaboration

Profilers and dynamic evidence. Before trusting any code-review guess, I'd reproduce the leak under a controlled, repeatable workload and watch memory over time: resident set size (RSS, the actual physical memory the process holds) climbing steadily and never coming back down after garbage collection runs is the signature of a real leak, as opposed to a memory spike that a full garbage collection cycle reclaims. For a compiled agent, a built-in heap profiler (Go's pprof, for example) can take a heap snapshot before and after a workload and show exactly which allocations are still retained; for an interpreted agent, an allocation tracer (Python's tracemalloc, for example) does the equivalent by snapshotting live allocations and diffing two points in time.

Code patterns to look for. The question names circular references specifically, and it's worth being precise about when that's actually the cause: in a language with a tracing garbage collector (which reclaims anything unreachable from a root, cycles included), a circular reference on its own is usually NOT what leaks memory, since the collector can free a cycle nobody outside it points to. What actually leaks in that kind of runtime is something still reachable that shouldn't be:

• Unbounded global caches or maps: a cache or dictionary that only ever grows, with no eviction policy or size cap, is one of the most common leak sources in a long-running service, since every entry is reachable from a live root for the life of the process.
• Leaked lightweight concurrent tasks (goroutines, in Go, or the equivalent worker/coroutine in another runtime): a task that's spawned per request or per event but never exits, most often because it's blocked forever waiting on a channel or queue that nothing will ever write to, keeps itself and everything it captured in its closure alive indefinitely. Every hung task is a small, permanent leak.
• Forgotten deregistration: an event listener, callback, or subscription that's added but never removed when the thing that registered it goes away, so a long-lived object (say, a connection pool) keeps growing a list of listeners that should have been cleaned up.
• Circular references DO matter directly in a reference-counted runtime (no tracing collector, or one that only handles simple cases), where two objects each holding a strong reference to the other can prevent the count from ever reaching zero; if the agent embeds any component like that, it's worth checking specifically.

CI checks and metrics that catch regressions early.

• A memory-regression test: run the agent against a fixed, deterministic synthetic workload (a set number of requests or events, not "for N minutes," so the result is reproducible), take a heap snapshot before and after, and fail the build if retained memory grows past a set threshold.
• A task-leak assertion: after the synthetic workload finishes and drains, assert that the number of live background tasks (goroutines, threads, whatever the runtime calls them) has returned to its known baseline count, not just "isn't growing forever."
• Production metrics and alerting: track RSS trend, heap object count, and live background-task count over time, and alert on sustained upward trend rather than a single spike, since a spike that recovers after garbage collection is expected behavior, not a leak.

Worked example

A concrete pattern this would catch: the monitoring agent spawns one background task per incoming metric batch to forward it to an upstream collector over HTTP, and that HTTP call has no timeout or deadline. If the upstream collector stops responding, every task blocked on that call never returns, and each one keeps its metric-batch buffer alive in its closure. Under normal conditions this is invisible, since tasks come and go quickly; the moment the upstream collector gets slow or unresponsive, the agent starts accumulating one leaked task (and its buffer) per batch, forever, which shows up as a slow, steady RSS climb that never plateaus.

The review-time catch: does every code path that spawns a background task per request or event pass it a context or deadline, so a hung downstream call can't block that task forever? The CI catch: a synthetic test that sends, say, 500 metric batches to a fake upstream collector that never responds, then asserts the live background-task count returns to its pre-test baseline (not zero, since some fixed background workers are expected) once the test workload finishes, rather than staying elevated by roughly 500.

Trade-offs and pitfalls

Continuous production profiling has real overhead, so it's usually run on-demand or sampled, not left on all the time. A CI memory-regression gate with an absolute byte threshold is prone to flaking across different CI runner hardware; a relative threshold (percent growth over baseline, measured on the same run) is more stable. The most common wrong turn is fixing the symptom instead of the cause: bounding a growing cache with a size limit stops the crash but, if entries are evicted on a schedule that doesn't match how they're actually used, can just delay the same leak rather than fix it, so the review should ask why the cache grows unbounded in the first place, not just cap it and move on.