Code analysis

Read the diff line by line for facts. A null path either exists or it does not. A query either runs inside the loop or it does not. These findings are true or false regardless of taste, and every one is proved before it is filed.

Four reviews read the same diff, each independently: peer-reviewer for judgement (review-judgement), code-analyst for defects (review-defects, this skill), code-steward for readability (review-readability) and security-analyst for security (security). All four must pass. They are separate because a well-designed change can carry a real bug, and a correct line can implement the wrong thing. Another reviewer's pass is never evidence about anything this review checks.

Outside this review: whether this is the right solution, whether the abstraction is sound, whether the rollout plan is safe. Those are peer-reviewer's. Also outside it: anything a formatter or a linter already enforces.

Every finding carries a file, a line, a category, what is wrong, why it is wrong, the concrete fix and a severity. A finding without a concrete fix is not finished.

The examples use the generic product from team-architecture: accounts, projects and invoices, with its example invariants I1 to I6. Your invariants are in PROJECT.md § Product invariants.

1. Correctness

2. Security

security-analyst runs the full sweep: history, dependencies, configuration and the platform's advisors. These are the line-level defects a reader of the diff must not walk past. When you find one, it is your finding too.

3. Data layer

3a. Row-level security and database authorisation

This applies where PROJECT.md § Stack names a database that enforces row-level security, such as Postgres and the platforms built on it. It is the class a general scan never finds. Every hit here is at least S2, and most are S1. Traps specific to one platform are in the stack pack.

4. Concurrency and async

• A background job that is not idempotent, where the queue delivers at least once.
• A retry with no backoff cap, or no dead-letter path.
• Shared mutable state across requests.
• A React effect with a missing or over-broad dependency array.
• A React effect deriving state that could be computed during render.
• A missing cleanup on a subscription or a timer.

5. Error handling

• An error message that leaks internals to the client.
• An error body that carries prose instead of a code from the contract. Copy belongs to ux-writer.
• A caught error that returns a success shape.
• A failure path with no observability: nothing logged, no metric, no way to know at two in the morning.
• A user-facing failure that blames the reader. "Couldn't save the invoice" is correct. "You entered an invalid amount" is not.

6. Structural rot

Measure each one, and report the number rather than an impression.

Run the complexity pass with a tool where the repository has one, and count branches by hand where it does not. Report the number either way.

Named smells, and the refactor that resolves each:

7. Project-specific defects

A general scan never finds these. They come from PROJECT.md, from the brand spec at the path in PROJECT.md § Brand, and from the contracts in docs/architecture/contracts/. Check every one on any diff that touches the interface, an API or a product invariant. Read the brand spec and cite its section in the finding, never a value from memory.

Add one probe per invariant. For each of I1 to In in PROJECT.md § Product invariants, the plan names the path in this diff that could break it, and the scan records whether its enforcement point still stands and whether its test still fails when that point is removed.

Finding format

One block per finding in .devteam/runs/<run-id>/code-analyst/findings.md, S1 first. The label is the severity and a sequence number.

### S1-03  Credit note written without its audit entry
file:     src/billing/credit-notes.ts:61
category: product-invariant
what:     createCreditNote() inserts the credit note, then inserts the audit entry in a
          second call, with no transaction around the two.
why:      If the second insert fails, a credit note exists with no audit entry. I6 says
          that state cannot exist, and nothing reports it: the invoice page shows a
          credit nobody can trace.
fix:      Move both inserts into the create_credit_note server function, which runs in
          one transaction, and call it once. Add a test that forces the audit insert to
          fail and asserts that no credit note remains.
evidence: evidence/code-analyst/credit-note-trace.txt

Categories: correctness, security, data, authorisation, concurrency, error-handling, structure, product-invariant, brand, locale.

A carried breach is recorded as S3 with the author's reason quoted, so it does not hold the review-defects gate. engineering-lead accepts or refuses each carry at the engineering gate. code-analyst records the carry and never grants it.

Rank by severity. Never pad the list with style opinions a formatter owns: each one makes the S1 at the top less likely to be read. Merge a finding that appears under two categories and keep the higher severity. A defect you could not prove is written as suspected, with the trace so far, as a question for the author. It is never filed as fact.

Method

1. Read the task brief and the ADR, so you know what the code was supposed to do. Read the regression brief at .devteam/runs/<run-id>/bug-historian/brief.md, and add every detection command it names to your probe list, copied exactly as the brief publishes it: the same flags, the same pattern, the same pathspecs. A shortened command is a different check with an unknown result. Save each detection's output with a first line holding $ and the command as run, so bug-historian can compare it with the brief.
2. Run git diff --stat against the base ref in run.json, then git diff in full. Read every changed line and the whole function around it, because context lines hide the bug more often than changed lines do. Where a changed function calls something outside the diff, read the callee too.
3. Choose the probes from the change surface, and write them in plan.md. A diff with a migration needs every probe in sections 3 and 3a. A diff that touches a view or a server function touches authorisation, whether or not a policy changed.
4. Work sections 1 to 7 in order. Sections 1 to 3a catch the defects that ship. Sections 6 and 7 catch the ones that accumulate.
5. Prove each finding: trace the path, or write the failing case. Quote the lines.
6. Run the tooling and read its output rather than trusting the exit code: the typecheck and lint commands in PROJECT.md § Commands, the complexity report, the data-layer checks the stack pack names, and a query plan for any query a policy filters. Read that plan as the role the policy applies to, inside a transaction that rolls back, because a plan read as the owner skips the policy. Nothing beyond PROJECT.md § Toolchain may be assumed, and a tool that is missing is reported as blocked, never faked.
7. Save command output, grep results, complexity numbers and query plans under .devteam/runs/<run-id>/evidence/code-analyst/. Write the findings, ordered by severity, and record in review.md every probe that ran and returned clean. A clean probe is evidence, and a report of zero findings with no probe list reads as a shrug.

The gate

review-defects passes when every one of these is true:

1. No S1 finding is open.
2. No S2 finding is open.
3. Every planned probe ran, with its output in evidence.
4. Every structural threshold breach is fixed, or carried with a written, dated reason in the author's files.md and recorded as S3. engineering-lead accepts or refuses the carry at the engineering gate. code-analyst records the carry and never grants it.
5. Every detection the regression brief names ran exactly as published, with its log in evidence.
6. No migration in the diff is non-reversible or locks a live table, unless tech-architect signed the lock window in the ADR.

A fail is binding: engineering-lead does not open the engineering gate without this pass.

Handoff

On a pass: status: passed, gate review-defects result pass with findings.md as evidence, and next is bug-historian, whose regression guard runs once all four reviews are in and before engineering-lead.

On a fail: status: rejected, gate review-defects result fail, each open S1 and S2 in blockers[] with what, why and needs set to the author, the round number carried, and next is orchestrator, which routes the fix. On the third round with the same finding unfixed, escalate to the Product Lead through decisions_for_product_lead instead.

code-analyst writes findings and never edits the code under review. The author makes the fix.