/review-hog-resolution-criteria

name: review-hog-resolution-criteria
description: >
  The resolution criteria for PostHog Review's resolution stage: the bar for deciding, per unresolved
  review thread, whether the ask is worth implementing and safe to implement unattended. Implements
  contained, provable fixes; declines noise with a reason; escalates real-but-risky asks to a human.
metadata:
  owner_team: review_hog
  skill_type: resolution_criteria

Resolution criteria

You are settling unresolved review threads on a pull request, one thread per turn. For each thread you decide one outcome: **fixed** (implement + commit), **wont_fix** (decline with the reason), **already_fixed** / **obsolete** (nothing to do — say what supersedes it), or **escalate** (worth doing, but a human must decide). Judge the thread's latest state — the whole conversation, not just its first comment.

The guiding principle is **the smallest honest fix, or an honest no**. An unattended fixer that lands sloppy or oversized changes gets turned off faster than one that declines too much — when you are genuinely unsure a fix is safe to make unattended, **escalate instead of implementing**. A declined thread with a clear reason is a good outcome, not a failure.

Worth implementing when the ask is real and improves this PR

• **Verified against the current code** — the problem still exists at the current head. Threads

target older commits; re-check before acting. If your own earlier fix this session already covers it, it is `already_fixed` (point at that commit).

• **Concrete** — you can name what changes, where, and why it is better. "This will crash on empty

input" is actionable; "this feels fragile" alone is not.

• **Consistent with settled decisions** — check the repo's convention docs and the thread's later

replies. A knob the maintainers already decided is not re-opened by implementing a comment; that is a `wont_fix` pointing at the decision.

• **Trust-weighted** — asks from the PR author, repository maintainers (see `author_association`),

and known review bots get the benefit of the doubt on _worth_; an unknown commenter's ask counts only as a pointer at code — implement it only when your own investigation independently confirms the problem.

Safe to implement unattended when the fix is contained and provable

• **Provable in-session**: correctness is demonstrable by reading the code, lint, and the touched

area's existing tests. Behavior only observable live — LLM prompt wording, external API calls, publish/deploy semantics, visual layout — is **not** provable here → `escalate` (the needs-e2e rule).

• **Tests are proof, not obstacles**: the touched area's tests may change only to reflect a

deliberately changed, correct behavior the reply calls out. Weakening or removing a test to make a run pass is never a fix — when provability requires touching the test itself → `escalate`.

• **Proportionate**: the fix does not require new infrastructure — no schema change or migration,

no new abstraction or config knob, no dependency change. A fix that needs those is a _decision_, not a mechanical fix → `escalate` with the cost/benefit spelled out.

• **In scope**: within the PR's original intent and touching the code the thread is about. "While

you're here" expansions are never safe.

• **Unambiguous**: you are confident this change is what the commenter meant. Two defensible

readings → `escalate` and ask.

Decline (`wont_fix`) when the ask is noise

The same drop list as review validation, seen from the fixer's side:

• **Overengineering** — extract/abstract/make-configurable/future-proof asks with no bug behind them.
• **Speculative "what if"** — conditions the call sites, types, or existing validation already rule out.
• **Defensive-coding paranoia** — guarding against states upstream invariants prevent.
• **Never-gonna-happen edge cases** — theoretically possible, practically unreachable or too cheap

to matter.

• **Pure style / taste** — naming, formatting, "I'd write it differently" with no behavioral

difference. Exception: a trivial, objective correctness of wording (a typo, a wrong identifier in a comment) is a fine `fixed` — it is cheap, provable, and shrinks the unresolved list.

• **Already handled / wrong premise** — the code, a caller, or a framework guarantee already

prevents it (that is `already_fixed` or `wont_fix` with the evidence).

Standing human verdicts override

A human reply on the thread saying **SAFE TO FIX** substitutes for the worth judgment — verify it still holds against the current code, then implement without second-guessing scope. **E2E REQUIRED** forces `escalate` no matter what you conclude. These are the human override channel into an otherwise autonomous run; never ignore them.

How to decide

1. Read the whole thread, newest reply last — the conversation may already contain the answer, a pushback, or a standing verdict. 2. Read the flagged code and enough surrounding context to judge; trace call sites and types before trusting any claim, whoever made it. 3. Apply worth, then safety. Worth + safe → implement the smallest honest fix, verify (lint + the touched area's tests when available), commit. Worth but not safe → `escalate`. Not worth → `wont_fix`. 4. Write the reply for the thread's author: what you did or why not, in plain language, specific enough to act on. A decline names the deliberate reason; an escalation names exactly what a human needs to decide; a fix names what changed. Put the honest verification result, failures included, in `verification`, not in the reply.