What a Senior Code Review Actually Catches: A Worked Example for Engineering Leaders

Engineering leaders often tell me their team does code review. When I ask what the reviews actually cover, the answer is usually some version of: "Does it work? Is the formatting okay? Does it follow the conventions?"

That is not a senior code review. That is a sanity check with a rubber stamp at the end.

A genuine senior code review - the kind that prevents production incidents, security breaches, and painful rewrites six months down the line - looks completely different. It catches things that junior and mid-level reviewers simply do not know to look for, because you only know to look for something after you have been burned by it.

This post walks through a real worked example. The code is a composite drawn from actual code quality consulting engagements, anonymized and simplified for clarity. The findings are real. If you lead an engineering team, this should give you a concrete picture of what you are getting - and what you are missing - depending on who is reviewing your code.

The Scenario: A User Search Endpoint

The feature is straightforward: a B2B SaaS application needs an admin endpoint that lets support staff search for users by name or email. The junior developer has written it, another mid-level developer has approved it, and it is ready to merge.

Here is the controller method, slightly simplified:

public function searchUsers(Request $request): JsonResponse
{
    $query = $request->get('q');
    $role = $request->get('role', 'all');

    $sql = "SELECT id, name, email, role, created_at
            FROM users
            WHERE (name LIKE '%" . $query . "%'
            OR email LIKE '%" . $query . "%')";

    if ($role !== 'all') {
        $sql .= " AND role = '" . $role . "'";
    }

    $results = $this->db->executeQuery($sql)->fetchAllAssociative();

    $enriched = [];
    foreach ($results as $user) {
        $enriched[] = array_merge($user, [
            'subscription' => $this->subscriptionService->getForUser($user['id']),
            'lastActivity' => $this->activityService->getLastActivity($user['id']),
        ]);
    }

    return new JsonResponse($enriched);
}

The mid-level reviewer left one comment: "Nice, looks good. Maybe add a comment explaining the LIKE syntax for future devs."

A senior reviewer would not approve this. Here is what they actually see.

Finding 1: SQL Injection, Directly Exploitable

The string concatenation in the SQL query is a textbook SQL injection vulnerability. Any user with access to this endpoint - and remember, this is an admin panel, so the attacker only needs to compromise one support account - can inject arbitrary SQL.

A query string of ' OR '1'='1 in the q parameter dumps the entire users table. A more sophisticated injection can drop tables, exfiltrate data to an external server, or escalate privileges. With the role parameter also unparameterized, the attack surface doubles.

The fix is parameterized queries. This is not a style preference. It is a hard security requirement, and no reviewer with production experience should let this merge.

$sql = "SELECT id, name, email, role, created_at
        FROM users
        WHERE (name LIKE :search OR email LIKE :search)";

$params = ['search' => '%' . $query . '%'];

if ($role !== 'all') {
    $sql .= " AND role = :role";
    $params['role'] = $role;
}

$results = $this->db->executeQuery($sql, $params)->fetchAllAssociative();

A junior reviewer might not have seen SQL injection exploited in the wild. A senior reviewer has, or has cleaned up after it. The cost of one incident - breach notification, regulatory fines under GDPR, customer churn, engineering time - dwarfs the cost of fixing it now.

Finding 2: An N+1 Query That Will Not Matter Until It Does

The loop that enriches each user result fires two additional database queries per row - one for subscription data, one for last activity. If the search returns 10 users, that is 21 database queries for a single request. If it returns 100 users, it is 201.

Right now, with a small dataset and light traffic, this is invisible. The endpoint responds in 80ms and nobody notices. Six months from now, when the customer base has grown and a support rep runs a broad search that returns 400 results, the endpoint will hang, the database connection pool will saturate, and other parts of the application will start timing out.

The senior reviewer flags this not because it is broken today but because it will break, and fixing it after the fact means a database migration, cache invalidation strategy, and probably a week of debugging a production incident at 2am.

The fix is to collect all user IDs from the initial query, load subscriptions and activity records in two bulk queries, and join them in memory:

$userIds = array_column($results, 'id');

$subscriptions = $this->subscriptionService->getForUsers($userIds);
$activities = $this->activityService->getLastActivities($userIds);

$enriched = array_map(function ($user) use ($subscriptions, $activities) {
    return array_merge($user, [
        'subscription' => $subscriptions[$user['id']] ?? null,
        'lastActivity' => $activities[$user['id']] ?? null,
    ]);
}, $results);

Three queries regardless of result size. This is a pattern every senior developer knows instinctively, because they have debugged enough N+1 problems to see the shape of one before it hurts.

Finding 3: No Result Cap, No Pagination

The query has no LIMIT clause. If someone searches for an empty string - or a single common letter like "a" - the query will attempt to return every user in the database in a single response.

At 10,000 users this will be slow. At 100,000 users it will timeout or crash. At any scale, serializing tens of thousands of enriched user objects into a JSON response and sending it over the wire is unnecessary load on the database, the application server, and the client.

The fix is mandatory pagination with a sensible default and a hard cap:

$page = max(1, (int) $request->get('page', 1));
$perPage = min(50, max(1, (int) $request->get('per_page', 20)));
$offset = ($page - 1) * $perPage;

$sql .= " LIMIT :limit OFFSET :offset";
$params['limit'] = $perPage;
$params['offset'] = $offset;

A mid-level reviewer might add pagination if the ticket explicitly required it. A senior reviewer adds it because they understand that unbounded queries are a denial-of-service vector and a performance time bomb, even when the ticket does not mention it.

Finding 4: Missing Authentication and Authorization Checks

The controller method contains no explicit authorization check. It relies entirely on whatever middleware or firewall is configured at the router level.

This is a coupling problem that is also a security problem. If a future refactor moves this endpoint, changes the routing configuration, or adds a new authentication bypass for testing, the endpoint silently becomes accessible without proper authorization. Defense in depth means checking authorization at the controller level as well, not trusting that the router will always protect you.

A senior reviewer asks: what happens if this route is accidentally registered without the ROLE_ADMIN firewall? The answer should not be "users get full access to the search endpoint." It should be a permission check in the method itself:

if (!$this->security->isGranted('ROLE_ADMIN')) {
    throw new AccessDeniedException();
}

One line. Zero performance cost. Eliminates an entire class of privilege escalation bugs.

Finding 5: Tight Coupling to Infrastructure Services

The controller reaches directly into $this->subscriptionService and $this->activityService. Both of these, presumably, make database calls or external API calls. Neither is wrapped in any error handling.

If the subscription service throws an exception - because of a database timeout, an API rate limit, or a transient network error - the entire search request fails with a 500 error. The user gets no results. Support can not do their job.

A senior reviewer thinks about partial failure. The user search itself worked. The enrichment failed. These are separable concerns. A robust implementation catches enrichment failures per-user and returns partial data rather than nothing:

try {
    $subscription = $this->subscriptionService->getForUser($user['id']);
} catch (\Exception $e) {
    $this->logger->warning('Failed to load subscription for user', [
        'user_id' => $user['id'],
        'error' => $e->getMessage(),
    ]);
    $subscription = null;
}

This is not over-engineering. It is the difference between a degraded experience and a broken one when a downstream service has a bad morning.

What the Mid-Level Review Missed

To be clear, the mid-level reviewer is not incompetent. They caught the things they were looking for: logical correctness, naming conventions, code style. The endpoint does what the ticket described.

What they did not catch: a directly exploitable security vulnerability, a query pattern that will cause production performance issues, an unbounded query that is a denial-of-service risk, a missing authorization defense, and brittle coupling to downstream services.

None of these show up in unit tests unless you specifically write tests for them. None of them cause visible failures in development or staging environments. They are the kind of problems that arrive in production, under load, at inconvenient times.

What This Means for Engineering Leaders

If your team's code review process is: does it work, does it look okay, approved - you are accumulating risk with every merge. The bugs that cause outages and security incidents are almost never logical errors. They are the structural problems that come from not knowing what to look for.

Bringing in a senior reviewer - whether that is hiring one, using a fractional arrangement, or engaging code quality consulting for periodic audits - does not replace your team. It augments their eye. It catches the problems that only become visible to someone who has shipped, debugged, and occasionally been on call for the consequences.

The five findings in this worked example took about 15 minutes to identify in a real review. The cost to fix them before merge: a few hours of focused work. The cost to fix them after a production incident: anywhere from a weekend to a month, plus reputational damage and a regulatory conversation you did not want to have.

If you are curious what a review like this would surface in your codebase, reach out at hello@wolf-tech.io or visit wolf-tech.io. The first conversation is always free.