feat(review): default the reviewer to review-only, with AUTO_FIX to opt back in

KH
2mo ago
+106-11

Follow-up to #135. With verification delegated to the repo's CI, an applied edit is no longer proven before it lands — the agent learns it broke something only afterwards, and it still cannot revert its own push (piece 4 is not built). Until that loop has earned trust, the reviewer proposes instead of applying.

The asymmetry is the whole argument: a wrong suggestion costs a reader a moment, a wrong push costs the author a broken PR. cloud#1926 is what the second one looks like in practice — three broken pushes in a day.

What changes

Review-only is the default. The harness is told not to edit, create or delete any file, and that anything left in the working tree is a push, because cloud commits the tree after the run. Everything it would have fixed becomes a suggestion naming the file, the line and the exact replacement. Failing CI is explicitly included: diagnose it and state the fix, don't apply it.

AUTO_FIX turns applied edits back on. One switch rather than a gradient, so the deployed behaviour is legible from a single input instead of inferred from several interacting rules. Unset means review-only.

This is a change to what the reviewer does, so it's worth agreeing on deliberately rather than merging on green.

Test changes, called out

reviewHarnessPrompt limits auto-edits to mechanical changes was retargeted at the AUTO_FIX prompt rather than deleted. Those limits are exactly what stops applied edits being unbounded once an operator opts in, so the coverage still earns its place — it just applies to the mode where editing happens.

Verification

  • 3 new tests: the AUTO_FIX truth table, the two policy texts, and that the default prompt is review-only while still carrying the rest of the contract.
  • Mutation-tested, each reverted after measuring: flipping the prompt default to auto-fix, and making autoFixEnabled always return true — both fail the suite.
  • npm test: 374 pass (the 2 failures are the pre-existing harness-kit environmental ones). npx tsc --noEmit: clean.

Known gap, disclosed

Hardcoding autoFix: true at the call site inside reviewAndFix does not fail any test — that function needs GitHub and memory mocks to exercise, so only the pure pieces around it are covered. It is the most damaging single edit in this change (it would silently make every deployment apply edits again) and it is currently guarded by review alone.

I'd rather say that plainly than imply the mutation testing covers more than it does. The durable fix is getting reviewAndFix under test; that's a bigger change than this PR and I didn't want to bundle it.

Sequencing

This is the intermediate that gets the OOM fix to customers with zero push risk. When piece 4 lands — bounded fix-forward, then back out — AUTO_FIX becomes a reasonable default again.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BmMcdDakbnDkHBxG4pit7z

Summary by cubic

Defaults the reviewer to review-only, so it proposes every fix as a suggestion instead of editing files. Verification now happens in the repo's CI, so an applied edit is no longer proven before it lands — a wrong push breaks the PR, while a wrong suggestion costs only a moment to read.

Behavior

  • The reviewer never edits, creates, or deletes files; anything left in the working tree gets committed and pushed.
  • Failing CI is diagnosed with an exact suggested replacement, not applied.
  • Set AUTO_FIX to true, 1, or yes to re-enable applied edits, limited to mechanical non-semantic changes.

Known gap

  • Hardcoding autoFix: true inside reviewAndFix would not fail any test, since that path needs GitHub and memory mocks that aren't covered yet.