Code-review scenarios

Status: Complete. Last reviewed 2026-08-28.

These exercises test production-risk reasoning, not how many comments a candidate can produce. State assumptions, separate supported findings from questions, and rank correctness, security, data, compatibility, and operability ahead of style.

Exercises

1. Checkout across database and payment boundaries
2. Tenant-scoped CRUD with broken object authorization
3. Import job with unbounded memory and unsafe retries
4. API resource with a hidden N+1
5. Cache wrapper with incomplete keys and an unsafe lock
6. Migration incompatible with old workers
7. Over-abstracted CRUD module

Tenant-scoped CRUD with broken object authorization

Context and invariant: An authenticated tenant administrator may rename projects belonging to the current tenant. A project identifier must never provide read or write access across tenants, and tenant_id is server-owned.

Route::put('/tenants/{tenant}/projects/{project}', function (
    Request $request, Tenant $tenant, Project $project
) {
    // previous implementation omitted
    $project->update($request->validate([
        'name' => ['required', 'string'],
        'tenant_id' => ['sometimes', 'integer'],
    ]));

    return new ProjectResource($project);
});

Operating assumptions: Authentication middleware runs. Route-model binding is unscoped unless the route or relationship says otherwise. Project::$fillable includes name and tenant_id. No policy call or database tenant relationship is shown. Whether database constraints prevent tenant reassignment is unknown.

Candidate task: Produce a severity-ranked review. For each blocking finding, cite the violated invariant and distinguish code evidence from missing evidence. Ask for the smallest evidence packet needed to resolve unknowns, propose the smallest safe repair, and name tests plus deployment/operational checks. End with useful non-blocking feedback without presenting preferences as defects.

Discussion guide

Likely blocker: The independently bound project is not proven to belong to the route tenant, and no actor/resource policy is visible. An attacker may substitute another project’s ID. Accepting tenant_id also permits authority-bearing reassignment if mass assignment allows it. Authentication, validation, and a numeric existing ID do not establish authorization.

Missing evidence: Route definitions may enable scoped bindings; middleware or an application action may authorize elsewhere; policy hooks and database composite constraints are not shown. Request the effective route/middleware list, binding configuration, policy/action path, fillable fields, and relevant constraints before claiming those controls are absent system-wide.

Smallest safe repair: Resolve the project through $tenant->projects() or scoped binding, explicitly authorize the update, map only name, and derive tenant identity from trusted context. Keep a database constraint for tenant/resource integrity where the schema permits it. Choose consistent 403/404 denial semantics.

Verification and operations: Add feature tests for allowed update, unrelated user, cross-tenant ID substitution, attempted tenant_id change, missing resource, and response shape. Exercise the production database constraints. Audit rejected and privileged cross-tenant operations without leaking sensitive data. A naming suggestion is non-blocking unless it obscures the security boundary.

Owning guidance: validation, authorization, policies, and tenant boundaries. Canonical questions: LARAVEL-BOUNDARIES-04, LARAVEL-BOUNDARIES-06, and BACKEND-SECURITY-02.

Import job with unbounded memory and unsafe retries

Context and invariant: A queued job imports customer rows from object storage. Files may contain millions of rows, workers have a fixed memory limit, and redelivery after interruption must converge without duplicating customer records or notifications.

public function handle(): void
{
    $rows = collect(explode("\n", Storage::get($this->path)));

    DB::transaction(function () use ($rows) {
        foreach ($rows as $row) {
            Customer::create($this->parse($row));
            Mail::to($row['email'])->send(new WelcomeMail());
        }
    });
}

Operating assumptions: The queue is at-least-once and can retry the whole job after timeout or worker loss. The file is trusted to exist but its size and row quality are unbounded. The snippet has no durable run/checkpoint or stable source-row identity. Provider mail idempotency is unknown.

Candidate task: Produce a severity-ranked review. Identify missing evidence before inferring exact memory, timeout, transaction, and provider behavior. Recommend the smallest safe repair that bounds resources and makes retry semantics explicit, with tests and operational controls. Separate blockers from non-blocking readability or decomposition suggestions.

Discussion guide

Likely blockers: Storage::get, explode, and collect create several full-file representations, so a valid large file can exhaust worker memory. One transaction grows with the entire import and holds database resources while synchronous external mail runs. A crash after inserts or mail can replay the file; the transaction cannot roll back sent mail, and plain creates have no shown deduplication identity.

Missing evidence: Request file-size distribution, memory and timeout limits, queue visibility/retry settings, required all-or-nothing versus row-level semantics, duplicate policy, database keys, mail-provider behavior, and current import telemetry. Do not prescribe one job per row without capacity evidence.

Smallest safe repair: Stream the object, process bounded chunks, assign a durable import and source-row identity, enforce uniqueness/upsert or guarded transitions, commit each resumable unit, and checkpoint only committed progress. Record notification intent transactionally and deliver it through an idempotent post-commit path. Reject or report permanent row errors according to an explicit partial-failure contract.

Verification and operations: Test a file larger than worker memory, malformed rows, duplicate source rows, retry after each commit/effect boundary, hard termination, resumed checkpoints, and old payload compatibility. Observe peak memory, checkpoint age, rows processed/rejected, retries, queue age, duplicate suppression, and terminal import state. Extracting more classes is non-blocking unless it clarifies an actual lifecycle owner.

Owning guidance: Artisan commands and task scheduling, queues and retries, and concurrency and bounded work. Canonical questions: LARAVEL-ARTISAN-02, LARAVEL-QUEUES-03, and ARCH-SYSTEMS-04.

API resource with a hidden N+1

Context and invariant: A paginated order endpoint must keep query and memory cost bounded as page size grows. Its response includes customer name and a line count, but not line objects.

// Controller
return OrderResource::collection(Order::query()->paginate(50));
return OrderResource::collection(Order::query()->with('lines')->paginate(50));

// OrderResource::toArray()
return [
    'id' => $this->id,
    'customer' => $this->customer->name,
    'line_count' => $this->lines()->count(),
];

Operating assumptions: Lazy loading is enabled in production. customer is not loaded elsewhere. lines() issues a database query even if the lines property was eagerly loaded. Page size is capped at 50, but line cardinality can be high. Existing query indexes and resource wrappers are unknown.

Candidate task: Rank findings by production severity and confidence. Name the evidence needed to prove query growth and plan cost, propose the smallest safe query/resource repair, and specify tests plus production verification. Include non-blocking feedback only after correctness and performance risks.

Discussion guide

Likely high finding: For each order, $this->customer can lazy-load once and $this->lines()->count() executes a count query. The new eager load hydrates all lines but does not satisfy the relation-method count, adding memory without removing that N+1. For 50 orders this is approximately the parent query, pagination count, customer queries, line-count queries, and eager line query; exact counts depend on framework/query details.

Missing evidence: Capture SQL/query spans with several orders, row counts and line skew, actual plans, selected columns, serialization accessors, and production latency/memory. Confirm no global/default/automatic loading or resource wrapper changes the path.

Smallest safe repair: Make the owning query declare with('customer:id,name') and withCount('lines'), then have the resource read only loaded values such as customer.name and lines_count using relationship-aware resource helpers where optionality matters. Remove full line loading because the contract needs only a count.

Verification and operations: Add a feature test for response shape and authorization plus a query-count/growth test with several parents; verify representative plans and memory against the production engine. Enable lazy-loading prevention in development/tests where compatible and monitor query count/rows/latency by endpoint. Resource naming is non-blocking unless it misstates the wire contract.

Owning guidance: Eloquent relationships and loading. Canonical questions: LARAVEL-RELATIONSHIPS-04, LARAVEL-RELATIONSHIPS-05, and LARAVEL-RELATIONSHIPS-06.

Cache wrapper with incomplete keys and an unsafe lock

Context and invariant: A project summary varies by tenant, locale, and viewer permission version. The database is authoritative. Cache failure may reduce performance but must not change authorization or expose another representation.

public function summary(User $viewer, Project $project, string $locale): array
{
    $key = "project-summary:{$project->id}";

    return Cache::remember($key, 3600, function () use ($project) {
        return Cache::lock("refresh:{$project->id}", 5)->block(10, function () use ($project) {
            return $this->buildAuthorizedSummary($project);
        });
    });
}

Operating assumptions: Project IDs are unique only within a tenant. Output fields depend on the viewer’s current permissions and locale. Summary generation can exceed five seconds. All hosts share the lock-capable store, but cache outage behavior, invalidation, and permission-version ownership are not shown.

Candidate task: Rank supported findings, request evidence for unknown cache/store behavior, and propose the smallest safe repair. Cover correctness after lease expiry, tests, degraded operation, metrics, and non-blocking design feedback.

Discussion guide

Likely blocker: The value and lock keys omit tenant, locale, authorization scope, and representation version, so cache hits can disclose the wrong tenant or viewer representation. The closure also fails to capture $viewer and $locale, indicating either a non-runnable defect or hidden ambient context. A five-second lease can expire while generation continues; a second owner may overlap, and a ten-second blocking wait can amplify latency.

Missing evidence: Request store topology and atomic-lock support, key uniqueness, exact authorization-neutral versus personalized fields, generation distribution, freshness contract, invalidation events, stale behavior, and source capacity during cache failure. A lock cannot be assessed from syntax alone.

Smallest safe repair: Prefer caching an authorization-neutral tenant-scoped projection, then apply current authorization and localization after retrieval. Otherwise include every selecting dimension and a schema/permission version in both value and refresh identity. Use bounded stale-while-revalidate or a measured short refresh lease with safe overlap; never use the lease as the final authorization or correctness boundary.

Verification and operations: Test two tenants sharing a project ID, two permission sets, locales, permission changes, expiry races, a generator outliving the lease, cache outage, and mass expiry. Observe hit/miss by workload, stale age, refresh duration/errors, lock contention, source load, and fallback saturation. Wrapper naming or preferred cache API is non-blocking unless it changes these guarantees.

Owning guidance: Laravel cache, sessions, and Redis and cache and Redis reasoning. Canonical questions: LARAVEL-CACHE-01, LARAVEL-CACHE-02, and DATA-CACHE-04.

Migration incompatible with old workers

Context and invariant: Web instances and queue workers roll independently. Jobs can wait for hours. Every active release must read the schema and queued representations it encounters, and rollback must not reinterpret already-written data.

// release migration
Schema::table('orders', fn (Blueprint $table) => $table->string('status'));
Schema::table('orders', function (Blueprint $table) {
    $table->renameColumn('status', 'fulfilment_status');
    $table->string('carrier')->default('internal');
});

// new worker
$order->update(['status' => 'shipped']);
$order->update(['fulfilment_status' => 'dispatched']);

Operating assumptions: Old web and worker processes continue reading/writing status during the rollout. Queued jobs contain the old status vocabulary. The database engine/version, table size, exact rename locking behavior, model compatibility code, and rollback window are unknown.

Candidate task: Rank compatibility, data, and availability findings. Ask for engine- and fleet-specific evidence, identify the smallest safe release sequence rather than only a final schema, and include migration tests, rollout/rollback operations, and non-blocking feedback.

Discussion guide

Likely blocker: Renaming the column immediately breaks old readers/writers and can make rollback unsafe. Changing shipped to dispatched is also a protocol/data-semantic change that old queued payloads and exhaustive consumers may reject. The default may silently assign meaning to historical rows. Exact lock/rewrite severity is unknown until the engine and table are examined.

Missing evidence: Request active release and worker inventory, queue age/payload samples, all producers/consumers, table size/write rate, engine/version DDL behavior, replica topology, backfill semantics, and a tested rollback point. migrate --force is not safety evidence.

Smallest safe repair: Use expand/migrate/contract: add a nullable new column, deploy readers tolerant of old/new vocabulary and writers with one named authority, backfill in bounded resumable batches, compare semantics, switch reads, drain/restart old processes and old payloads, then enforce constraints and remove the old column in a later release. Preserve old vocabulary or translate at explicit boundaries throughout coexistence.

Verification and operations: Test an old producer/new consumer and new producer/old consumer where coexistence requires them, queued payload replay, rollback artifact compatibility, DDL on production-like data, concurrent backfill writes, and restart/drain behavior. Gate rollout on lock/latency/replica lag, error and unknown-status rates, backfill mismatch, queue age, and release IDs. Migration class naming is non-blocking.

Owning guidance: Laravel deployment and long-running workers and databases, indexes, and concurrency. Canonical questions: LARAVEL-DEPLOY-03, DATA-RELATIONAL-06, and OPS-DEPLOY-03.

Over-abstracted CRUD module

Context and invariant: One Laravel module performs ordinary project CRUD. There is one Eloquent implementation, no alternate data source, and policy authorization plus tenant scope must remain visible. Most feature changes currently touch every layer shown.

final class UpdateProjectAction
{
    public function __construct(
        private ProjectServiceInterface $service,
        private ProjectRepositoryInterface $projects,
        private ProjectMapperInterface $mapper,
    ) {}

    public function execute(UpdateProjectDto $dto): ProjectDto
    {
        return $this->mapper->toDto(
            $this->service->update($this->projects->find($dto->id), $dto)
        );
    }
}

Operating assumptions: The service delegates to Project::update, the repository returns Eloquent models, and the mapper copies model fields. No layer currently owns locking, transaction, provider translation, a stable external contract, or a second implementation. Hidden authorization, eager-loading, events, and exception mapping are unknown until characterized.

Candidate task: Rank actual production risks separately from design cost. Identify evidence needed before deleting layers, propose the smallest safe simplification, and define tests/operational comparison. Include non-blocking feedback and a trigger that would justify retaining or reintroducing an abstraction.

Discussion guide

Likely finding: The snippet shows navigation and wiring cost, duplicated persistence dependencies, and no clear semantic boundary. That is usually maintainability feedback, not a production blocker by itself. It becomes blocking only if the split hides authorization, transaction ownership, exception behavior, or contradictory writes. “Too many classes” is not a sufficient severity argument.

Missing evidence: Trace a representative change through history; inspect bindings, all implementations/callers, policies, SQL/query counts, events, transactions, exception mapping, DTO wire use, tests, and production traces. A pass-through layer may contain shipped behavior not visible here.

Smallest safe repair: Put tenant-scoped lookup, authorization, named field mapping, and the update in one cohesive application operation or controller/action consistent with the codebase. Inline one pass-through layer at a time, retain a DTO only if it owns an external/stable boundary, and use a named query or repository only when it owns consequential query/locking semantics.

Verification and operations: Characterize response, denial, persistence, events, queries, exceptions, and transaction behavior before simplification; run feature and production-database tests after each step. Compare files touched, review/navigation time, query count, latency, and error classification. Naming and preferred suffixes are non-blocking. A second implementation, independent owner, volatile provider, or repeated locking/query guarantee is a concrete reintroduction trigger.

Owning guidance: software design. Canonical questions: ARCH-DESIGN-02, ARCH-DESIGN-05, and ARCH-DESIGN-06.