Code Review and Working with Existing Codebases Questions

Direct answer

I'd write at least four tests, covering the success path, an HTTP error response, a malformed JSON body missing the expected key, and a network-level failure like a timeout, all with requests.get mocked so no test makes a real network call. Each test targets a distinct way this function can fail in production, not just variations on the happy path.

Structured elaboration

Approach. Mock requests.get so the function's own logic, not the network, is what's under test. For each test, build a fake response object with just enough behavior to drive the code path being tested (raise_for_status either does nothing or raises, json() returns a controlled payload), then assert on get_latest_image's return value or on the exception it raises.

Why these specific tests.

• Success proves the normal path works and the correct value is extracted from a realistic JSON payload.
• HTTP error (a 4xx or 5xx status) proves the function surfaces the failure via raise_for_status() rather than silently returning something wrong.
• Missing key in the response body proves that if the API's response shape doesn't match what the code expects, the caller gets a clear exception rather than a confusing downstream error somewhere else.
• Network-level failure (a timeout, a connection error) proves the function doesn't swallow or mask an infrastructure problem, which matters specifically because this function is used in a deployment script where a caller needs to know the difference between "the deploy image genuinely doesn't exist" and "we couldn't reach the registry at all."

Worked example

# deploy_utils.py
import requests

def get_latest_image(repo):
    r = requests.get(f'https://registry.example/api/{repo}/latest')
    r.raise_for_status()
    return r.json()['tag']

# test_deploy_utils.py
from unittest.mock import Mock, patch
import pytest
import requests
from deploy_utils import get_latest_image


def make_response(json_data=None, raise_error=None):
    resp = Mock()
    resp.raise_for_status = Mock(side_effect=raise_error) if raise_error else Mock()
    resp.json = Mock(return_value=json_data or {})
    return resp


def test_get_latest_image_returns_tag_on_success():
    resp = make_response(json_data={'tag': 'v1.2.3'})
    with patch('deploy_utils.requests.get', return_value=resp) as mock_get:
        result = get_latest_image('myapp')
    assert result == 'v1.2.3'
    mock_get.assert_called_once_with('https://registry.example/api/myapp/latest')


def test_get_latest_image_raises_on_http_error():
    resp = make_response(raise_error=requests.exceptions.HTTPError('404 Client Error'))
    with patch('deploy_utils.requests.get', return_value=resp):
        with pytest.raises(requests.exceptions.HTTPError):
            get_latest_image('missing-repo')


def test_get_latest_image_raises_keyerror_on_malformed_body():
    resp = make_response(json_data={'digest': 'sha256:abc'})
    with patch('deploy_utils.requests.get', return_value=resp):
        with pytest.raises(KeyError):
            get_latest_image('myapp')


def test_get_latest_image_propagates_network_timeout():
    with patch('deploy_utils.requests.get', side_effect=requests.exceptions.Timeout):
        with pytest.raises(requests.exceptions.Timeout):
            get_latest_image('myapp')

Actually run with pytest, output:

test_deploy_utils.py::test_get_latest_image_returns_tag_on_success PASSED
test_deploy_utils.py::test_get_latest_image_raises_on_http_error PASSED
test_deploy_utils.py::test_get_latest_image_raises_keyerror_on_malformed_body PASSED
test_deploy_utils.py::test_get_latest_image_propagates_network_timeout PASSED

4 passed

Complexity

This is straightforward, constant-time mocked I/O per test, no algorithmic complexity to speak of; the interesting design decision is which failure modes are worth a dedicated test, not runtime cost.

Edge cases

• A 500 server error takes the exact same code path as a 404, since both raise via raise_for_status(); one test covering "any HTTP error" is representative, a second status-specific test adds little.
• A response that's valid JSON but not a dict at all (a bare list, for example) would raise TypeError rather than KeyError when ['tag'] is applied; worth a fifth test if this API's contract is genuinely uncertain.
• Real production code often uses a requests.Session with a configured retry adapter rather than a bare requests.get; mocking at the requests.get level, as done here, doesn't exercise that retry behavior at all, which would need a different test approach.

Trade-offs and pitfalls

Mocking at the requests.get level is fast and has zero network flakiness, but it also means these tests can't catch a real integration problem, like the registry's actual response shape changing; a smaller number of separate integration tests against a real or realistic staging registry are worth having alongside these, not instead of them. A common pitfall is mocking so aggressively that the test asserts almost nothing about get_latest_image's own logic, for example forgetting to assert on the exact URL called, which would let a bug in the f-string (a wrong path, a typo) slip through unnoticed.

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

This workflow has real problems in all three categories asked about. Security: it echoes a secret to the build log and pipes a remote install script straight into bash with no verification, and it deploys to production on every single push with no approval gate. Caching: nothing is cached, so every run rebuilds from scratch. Reliability: there is no build or test step before deploy, no pinned action or tool versions, and no rollback path if deploy.sh fails partway through.

Structured elaboration

Security

• echo $SECRET_KEY prints the secret's value straight into the build log, which anyone with log-read access can see, and the workflow never even sets SECRET_KEY from secrets.SECRET_KEY in the first place, so as written this line is also just dead, misleading code.
• curl -sL https://example.com/install.sh | bash runs unaudited code from a remote server directly as root-equivalent on the runner: a supply-chain compromise of that URL becomes a compromise of the deploy job. Download the script, verify a checksum or signature against a value you pin yourself, then run it.
• actions/checkout@v2 is pinned to a mutable major-version tag; a compromised or hijacked tag could silently change what code checks out. Pin to a current major version (v7 as of this writing) or, for maximum supply-chain safety, a full commit SHA.
• The job has no permissions block, so it inherits broad default token permissions it likely doesn't need. Long-lived cloud credentials in secrets can usually be replaced entirely with OpenID Connect (OIDC, an identity-token exchange protocol): the job requests a short-lived, workflow-scoped token from the cloud provider instead of storing a static key.

Caching

• Nothing here is cached, so dependency installs and any build step re-run from a cold cache on every push. actions/cache, keyed off a lockfile hash, is the standard fix for anything with reusable dependency state.

Reliability

• on: [push] with no branch filter means every push to every branch attempts a production deploy. Restrict the trigger to the deploy branch, and consider requiring workflow_dispatch (a manual trigger button) for production specifically.
• There is no build, test, or lint step before deploy.sh runs, so a broken commit deploys straight to production.
• No environment: protection rule, so there is no required approval, no audit trail, and secrets are available to every job rather than scoped to a protected environment.
• No timeout, no health check after deploy, and no visible rollback strategy if deploy.sh fails halfway through.

Worked example

A corrected version of the workflow, addressing every issue above:

name: deploy
on:
  push:
    branches: [main]

permissions:
  id-token: write   # enables OIDC token exchange with the cloud provider
  contents: read

jobs:
  deploy:
    runs-on: ubuntu-latest
    environment: production   # requires a configured approver, scopes secrets to this job
    timeout-minutes: 15
    steps:
      - uses: actions/checkout@v7

      - name: Cache dependencies
        uses: actions/cache@v6
        with:
          path: ~/.cache/pip
          key: ${{ runner.os }}-deps-${{ hashFiles('requirements.txt') }}

      - name: Run tests
        run: ./run_tests.sh

      - name: Install deploy tool (pinned + verified, not piped blind)
        run: |
          curl -sSLO https://example.com/install.sh
          echo "<pin-the-real-published-checksum-here>  install.sh" | sha256sum -c -
          bash install.sh

      - name: Deploy
        run: ./deploy.sh
        env:
          SECRET_KEY: ${{ secrets.SECRET_KEY }}   # bound only where used, never echoed

Trade-offs and pitfalls

Requiring an environment approval and a test step before deploy adds real friction and latency to every deploy, which is a deliberate trade against the speed of the original one-shot script; for a low-risk internal tool that trade might not be worth it, but for anything customer-facing it almost always is. Pinning actions to a full commit SHA is the strongest supply-chain guarantee but adds maintenance overhead, since you have to manually bump the SHA to pick up fixes; pinning to a major version tag like @v4 is the common middle ground. A subtle pitfall: even secrets referenced correctly via ${{ secrets.X }} get automatic log masking from GitHub, but that masking is best-effort, exact-string matching that a transformed or partially-printed value can defeat, so "never print a secret at all" stays the real rule rather than relying on masking as a safety net.

Direct answer

I'd require a small, layered set of tools in CI rather than one do-everything linter: an auto-fixable formatter for style, a vet-style correctness checker for undefined-behavior-class bugs, the language's built-in data-race detector for concurrency bugs, and benchmarks for performance regressions. Then I'd prioritize fixes by what class of defect they represent, not by which tool happened to report it: races and correctness bugs block the merge, performance and style don't.

Structured elaboration

Formatting. Go ships a canonical formatter (gofmt, plus goimports for import ordering) that produces one deterministic output for any given source file. Running this in CI and failing on any diff removes formatting from human review entirely, since there's no room for a style opinion once the formatter is authoritative.

Undefined behavior and correctness. go vet catches a specific, well-known set of suspicious constructs (a Printf-style call whose format string doesn't match its arguments, a struct copied by value that embeds a mutex, which silently breaks that mutex's locking guarantee for the copy). staticcheck goes further: unreachable code, an unused error return, or a comparison that can never be true.

Race conditions. Go's toolchain has a built-in data race detector (a data race is two goroutines, Go's lightweight concurrent tasks, accessing the same memory at the same time with at least one of them writing, and no synchronization between them), invoked with go test -race. This instruments the binary and reports real races observed while the tests run; it can't find a race that no test path exercises, so it's only as good as test coverage of the concurrent code paths.

Performance. Go's benchmark tooling (go test -bench, combined with the same heap/CPU profiler used for leak hunting, pprof) surfaces regressions when compared against a stored baseline, rather than trying to guess at performance from source alone.

Prioritizing fixes in review. I'd rank findings by consequence, not by tool:

1. Blocking: data races, and any go vet/staticcheck finding that represents an actual correctness bug (the mutex-copy example above is a real bug, not a style nit).
2. Strongly recommended before merge: ignored errors on operations that can fail meaningfully, and confirmed performance regressions with a benchmark to back them up.
3. Follow-up acceptable: non-blocking staticcheck suggestions, dead code that adds noise but no risk.
4. Auto-fixed, not discussed: formatting and import ordering, since a human should never spend review time on something a formatter already fixed for them.

Worked example

A PR modifies a shared Stats struct that's passed around by value and embeds a sync.Mutex. go vet flags this: copying a struct that embeds a lock triggers its copylocks check, warning that the assignment copies a lock value. That's not a style nit, it's a real bug: copying a struct that contains a mutex creates a second, independent lock, so code holding the copy's lock provides no actual protection against code holding the original's lock, and the two sides can now race on the data the mutex was supposed to protect. Separately, go test -race flags a genuine concurrent map write elsewhere in the same PR, and gofmt reports three files with import-order diffs.

Prioritization: the mutex-copy bug and the concurrent map write are both blocking, since both are real correctness/concurrency bugs, not opinions; the import-order diffs get auto-fixed by running goimports -w and are never discussed in review at all.

Trade-offs and pitfalls

Running -race on every test invocation roughly doubles memory use and meaningfully slows the run, so most teams run it in a dedicated CI lane rather than on every local go test. Turning on a strict linter for the first time in a legacy repo produces a wall of pre-existing findings that block every future PR unless you baseline them (only fail on NEW findings in changed lines) rather than demanding the whole codebase get fixed at once. The most common pitfall in review is treating every tool finding as equally urgent: burying a genuine data race in the same list as ten cosmetic suggestions makes it likely the race gets the same "eh, later" treatment as the cosmetics.

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.