Code reviewer should run typecheck/lint/format checks (not just behavioural inspection)

[HenryLach]

Summary

The reviewer agent (spawned via review_step) evaluates code changes through behavioural inspection only. It does NOT run the project's declared typecheck / lint / format checks as part of its review. As a result, code that has TypeScript strict-mode errors, lint failures, or format violations can receive a code-review APPROVE — these issues are then deferred to the task's final "Testing & Verification" step (typically Step N-1), at which point a single failure can block the entire batch from completing.

The cost of catching these issues earlier (per step) is approximately one extra npm run typecheck invocation per code review. The cost of not catching them early is the entire investment in the affected step plus all subsequent steps that depended on it.

Reproduction

1. Configure a task with Review Level: 2 (Plan + Code).
2. Have the worker introduce a typecheck-failing pattern in step N (e.g., a strict-mode as cast on a possibly-undefined value: (init as RequestInit) where init is undefined per the inferred tuple type).
3. Worker runs targeted vitest — passes (TS strict-mode errors don't fail vitest by default).
4. Worker calls review_step(step=N, type='code'). Reviewer reads the diff, evaluates behavioural correctness, returns APPROVE.
5. Step N closes as complete; batch proceeds.
6. At Step N-1 (Testing & Verification) the worker runs npm run typecheck — fails with strict-mode errors. Now the worker must revisit step N's code, re-fix, re-test — possibly with reviewer involvement.

If the batch dies before reaching Step N-1 (e.g., issue #537 death-spiral), these errors slip through entirely and surface only when an operator does manual recovery.

Concrete evidence

Production batch 20260506T105850, MIG-002, Step 1:

• R003-code-step1.md returned APPROVE for the Step 1 implementation
• After the batch died and supervisor took over, running npm run typecheck against the Step 1 commit (80ab797) produced 5 TypeScript strict-mode errors:

tests/unit/turnstile.test.ts(121,13): error TS2352: Conversion of type 'undefined' to type 'RequestInit' may be a mistake because neither type sufficiently overlaps with the other...
tests/unit/turnstile.test.ts(122,34): error TS2352: ...
tests/unit/turnstile.test.ts(126,19): error TS2352: ...
tests/unit/turnstile.test.ts(142,14): error TS2493: Tuple type '[]' of length '0' has no element at index '1'.
tests/unit/turnstile.test.ts(143,41): error TS2352: ...

All 5 errors were in test code touching the new turnstile.ts module — exactly the surface that R003 had reviewed.

The errors followed a single recognisable pattern (vi.fn's inferred-empty-tuple type can't be widened via direct as RequestInit). Any reviewer that ran npm run typecheck would have caught it instantly.

These errors blocked the supervisor's recovery commit (Step 4 quality gate would have failed) until they were manually fixed at commit 28ffef9.

Why this matters

Project northstars (AGENTS.md etc.) typically declare strict TypeScript (no any, no @ts-ignore) as a hard rule. The reviewer agent sees this rule in its system prompt but currently has no easy way to verify it without running the actual compiler. Behavioural inspection of the diff catches some type issues (e.g., obvious any literals) but misses entire classes:

• Strict-mode-only errors (TS2352 conversion mismatches)
• Inferred-type narrowing failures
• Function-overload mismatches
• Missing/extra return statements
• All lint rules (e.g., @typescript-eslint/no-floating-promises)
• All format violations

Each of these is cheap to detect with a single npm run invocation. Each is moderately expensive to recover from when caught at the final quality gate or, worse, post-merge.

Fix proposals

A. Reviewer auto-runs project quality checks

The reviewer's bash tool (already allowlisted via TP-184's ENGINE_BRIDGE_TOOLS plus the standard read,bash,grep,find,ls) can invoke:

npm run typecheck
npm run lint
npm run format:check

The reviewer should run these against the task's working tree before returning a verdict. Failures from any of them are surfaced as Issues Found with severity important, regardless of the behavioural review's findings. The reviewer's existing prompt structure (Verdict / Summary / Issues Found / Pattern Violations / Test Gaps / Suggestions) accommodates this naturally.

B. Configurable per-project quality-check commands

Read the commands from .pi/taskplane-config.json → taskRunner.testing.commands (or wherever the project declares its health-checks). If unset, fall back to a sensible default (npm run typecheck, npm run lint, npm run build for Node projects).

C. Reviewer prompt augmentation

Add to templates/agents/task-reviewer.md:

## Quality-check verification

Before returning your verdict, run the project's declared quality checks against the working tree (typically `npm run typecheck`, `npm run lint`, `npm run format:check`). Surface any failures as Issues Found with severity `important`. A behavioural-correctness APPROVE is invalidated by failing quality checks — return REVISE in that case, with the failure output included in the review file.

Recommendation

Ship A + C. B is a follow-up if multi-project support becomes a need.

Why this is a P2 (not P1)

The downstream impact is real but recoverable: the issues would have been caught at Step 4 (Testing & Verification) within the same batch in normal operation. They only escape entirely if the batch dies before Step 4, which is itself a different (worse) class of failure (issues #537, #2, #3). Fixing the reviewer to catch quality issues per step is hardening, not unblocking.

Acceptance criteria

• Reviewer agent automatically runs the project's declared quality checks during code review.
• A code review whose target produces typecheck/lint/format failures returns REVISE with the failures listed in the review file.
• Test: introduce a strict-mode-only TS error; assert the code review returns REVISE not APPROVE.

Related

• Issue Worker spins indefinitely when code review returns REVISE on a step already marked complete in STATUS #537 caused the batch to die before Step 4, which is what allowed the Step 1 typecheck failures to fully escape detection until manual recovery.

Affected version: taskplane@0.28.4. Concrete failing diff + R003 review file available on request.