Skip to content

Week 6: Agentic code review

Learning objectives

By the end of this week, you can:

  • review a small change against its stated behavior before reading a model review (LO5);
  • reproduce, refute, or leave unresolved each model finding with a bounded check (LO5); and
  • explain what a silent review does not establish and who owns an acceptance decision (LO9).

Minimal example

The continuing dashboard case

An archived build is analogous to an inactive account: ownership must not bypass the edit restriction. This course-created guard is a small demonstration, separate from any student project or graded instance. Review the change yourself before asking a model to review it:

 def can_edit(active: bool, owner: bool, staff: bool) -> bool:
-    return active and (owner or staff)
+    return owner or staff and active

The contract is small and explicit: boolean inputs may edit exactly when the account is active and is an owner or staff member. An active outsider and every inactive account must be denied. The code above is the intentionally regressed candidate in fixture/access.py; it is included so you can inspect and verify a concrete defect.

First write one review note without a tool. Include a location, an input, the expected result, the observed or traced result, and the consequence. The input active=False, owner=True, staff=False separates the two readings of the expression: the candidate allows it because and binds more tightly than or, while the contract requires denial. A passing active-staff example cannot establish the inactive owner case.

Save that first note before reading further. Run the unchanged public checks from the course root using the supplied code, after inspecting it:

python lectures/week-06/fixture/check_access.py -v

The bundled candidate fails test_inactive_owner and the inactive assertion in test_overlapping_roles, with exit 1. Record your actual output, not this prediction. The public checks cover the eight boolean combinations. After an agent proposes return active and (owner or staff), inspect the diff, apply only that expression in a separate copy, and rerun the unchanged checks there. All seven tests should pass. A deny-all patch would fail the active owner and staff cases. Inspect the tests too: changing their expectations to fit the candidate would not verify the contract.

The model reviews; you verify each point

Now request a review of the same diff with the contract and the exact files in context. Treat each comment as a hypothesis. A useful comment names a location and a check; a confident tone is not evidence. The model might catch the precedence bug, miss it after seeing only a happy path, or invent a requirement to parse strings even though the contract accepts booleans only.

After saving your own note, classify this course-authored comparison as you would model output. It is synthetic text, not a real model run:

Claim Disposition and check
“An inactive owner can edit.” Confirmed: the input above reproduces the contract violation.
“Parse the string false before deciding access.” Refuted: boolean inputs are the explicit contract; the claim invents scope.
“A caller in another file relies on inactive owners being allowed.” Cannot tell: the provided function and checks contain no caller. Record the files inspected and request the caller and its agreed behavior.

If the model only reports the string claim, it misses the actual inactive-owner defect. Compare your first finding with every model comment; keep any finding the model omitted.

The same precedence pattern appears in this older notation:

allowed = is_owner or is_staff and not archived
allowed = (is_owner or is_staff) and not archived

The current fixture uses active, owner and staff; the older names are only a notation bridge. In either form, an inactive or archived owner is the separating case.

Hands-on exercise

Follow the studio guide. Bring one change from your first open-source contribution or from your project. Before using a model, inspect the diff and write a small review record. Then run one model review with the same contract, diff and relevant tests in context. For every finding, choose exactly one disposition:

Disposition Evidence to record
Confirmed A reproducible input and observed result, or a source trace that proves the claim.
Refuted A check or code reading that contradicts the claim, with the relevant input or line.
Cannot tell What you inspected, what is missing, and the smallest next check that would decide it.

Finish with one sentence about the review's coverage: a reviewer reporting no finding only says that no finding was reported for the supplied context. It does not prove that the change is correct or that an unmentioned requirement is satisfied.

Verification

Use a short, repeatable review loop:

  1. Pin the base and candidate revisions and write the behavior the change is meant to preserve or add.
  2. Review the diff unaided. Pick boundary inputs, not only the example shown in the issue.
  3. Ask the model for findings, keeping its output separate from executable code and from your own first notes.
  4. For each finding, run the smallest safe check you can explain. Inspect any command before running it and follow the safety practices in the Tools and model access practices.
  5. Record the disposition, evidence, unresolved questions and the final decision to accept, narrow, recheck or stop.

Your record should let another reader reproduce the accepted claim. A model can suggest a test, but a human runs and interprets that test. Keep a no-finding result beside the scope the model saw, the checks you ran, and the cases it did not cover.

In a repository PR, the person with merge authority owns acceptance. The author owns a truthful handoff and must answer the four explanation questions (what changed, why, what could be wrong, and what was verified); a model or another reviewer supplies evidence and questions, never an automatic approval. If the evidence is incomplete, the decision is recheck or stop rather than an approval inferred from a green command.

Common mistakes

  • Reading the model comments first and then mistaking agreement for an independent review.
  • Treating one passing example as coverage of all role, error and boundary cases.
  • Marking a plausible comment confirmed without an input, trace or failing-then-passing check.
  • Calling a requirement refuted when the provided files cannot answer it; use cannot tell and name the missing evidence.
  • Treating a quiet reviewer as a clean bill of health. Search the diff and the contract yourself.
  • Running a model-suggested command without inspecting its scope, credentials, network use or outputs.
  • Letting a reviewer decide acceptance by default. State the human acceptance owner and the evidence that person used.