Review response policy

State: open/active

Summary

A responding session buckets every review comment into fix, decline, or verify-first, minimizes churn, and stops once a PR hits orchestrator.review.max_rounds.
Fix blocking comments, real bugs, and cheap/clear nits — a typo, a genuinely clearer name, a stale comment.
Decline, with a one-line reason, bikeshedding: a subjective preference with no clear winner, or a change whose churn outweighs its value.
Verify every comment against the code before acting, and make only surgical edits.
Past max_rounds, the Router holds all further feedback for that PR uniformly, regardless of severity — a session already running still triages what it already has, but nothing new is delivered past the cap.

Rule

A session responding to PR review feedback (pr-respond, whether dispatched fresh, resumed, or live-sent by the Router) triages each comment/thread into exactly one of four buckets before touching any code, and replies/resolves per the table in “Reply format and resolution rules” below.

Fix. Any comment marked blocking, plus any should-fix comment that points at a real bug, a factual error, a security issue, or text that would mislead an implementer who reads it later — plus a nit that is a clear, cheap, local improvement: a typo, a genuinely clearer name, a wrong or stale comment, a one-line correctness tidy at the site the comment names. These get fixed this round, full stop.
Decline, with a reply. Bikeshedding: a subjective style or wording preference with no clear winner, re-litigating a settled choice, scope expansion beyond what the comment’s site actually needs, a comment on this PR’s own previous-round fix wording, or a change whose churn outweighs its value — plus attribution quibbles, unverifiable claims about a third-party project, and comments about text a later commit already superseded. Reply “Declined — <one-clause reason>” and resolve the thread — a decline is a completed triage, not an open question. The test for a nit either way: does it make the code or doc meaningfully better at low cost and low churn? “Minimize churn” (rule 4) still applies even to a nit fixed under rule 1.
Verify before acting. Reviewers are sometimes wrong. Before fixing or declining, check the comment against the actual code at the commit it was left on. If the comment doesn’t hold up, reply “No change — <reason>” instead of making an edit that wasn’t needed, or declining a comment that was actually correct.
Minimize churn. Every fix is a surgical edit at the exact site the comment names — no reflowing, tidying, or renaming of neighboring text that the comment didn’t ask about. Every changed line is a line the next review round might comment on again; touching more than the comment requires manufactures new review surface for free.

Loop guard. Track review rounds per PR (the existing orchestrator.review.max_rounds cursor field, default 3 — see lib/config/load.ts’s orchestrator.review block; this policy does not define its own counter). The cap is enforced at the delivery layer, not by this policy: once a PR reaches max_rounds, deliver() (lib/services/dispatch/router.ts) holds ALL further feedback for that PR — live, resume, and fresh alike — with no severity distinction, because a FeedbackItem carries no blocking/real-bug field for the Router to check. A comment already delivered to a session before the cap tripped is still triaged normally by that session — bucket 1’s “real bug” and “blocking” cases get fixed as usual — but a comment that arrives after the cap has tripped is held, undelivered, until the owner raises max_rounds or otherwise intervenes; this policy’s triage buckets never run against a comment the Router hasn’t delivered.

Why

Without a fixed triage policy, a responding session either fixes everything asked (churning the diff on style preferences and re-litigating settled text) or fixes nothing consistently (leaving real bugs unaddressed while arguing with reviewers). Both failure modes cost review rounds: an over-eager session’s edits invite a new round of comments on the edits themselves; an under-responsive session leaves the same real issue open across rounds. The verify-before-acting step exists because a reviewer’s comment is a claim about the code, not a command — a session that edits on every comment without checking degrades trust in what “addressed” means. The loop guard exists for the same reason orchestrator.review.max_rounds exists at the dispatch layer (_pr_action.ts’s bumpReviewRounds/ skipped-max-rounds): an unbounded fix-and-re-review cycle on should-fix-only feedback is a stuck PR wearing the shape of progress.

How to apply

Where this is read. The pr-respond skill (solutions/ontological/plugin/plugins/sdlc/skills/pr-respond/pr-respond.md) reads this policy’s path from orchestrator.review.response_policy in sdlc.yaml and follows it when triaging comments. When the key is unset, the skill falls back to a short built-in policy inlined in its own prose — this standard is the editable, project-tunable version of that same triage shape, not a different rule.
Where the Router surfaces it. The Router’s feedback bundle (lib/services/dispatch/feedback.ts’s renderMessage, consumed by deliver()) references this policy — its path plus a one-line summary — so a resumed or live session that did not read the skill fresh still knows which policy governs its reply.

Reply format and resolution rules. Every triaged comment gets exactly one of these four replies, and the resolution column is what “done” means for that comment — a comment is not addressed until its thread is in the state this table says:

Reply	When	Thread resolution
`Fixed — <what changed>`	Bucket 1 (fix), and the edit landed.	Resolved.
`Already addressed — <where>`	The comment’s concern was already handled by an earlier commit in this PR or round.	Resolved.
`Declined — <one-clause reason>`	Bucket 2 (decline).	Resolved.
`No change — <reason>`	Bucket 3 found the comment didn’t hold up, or the loop guard capped a should-fix item.	Resolved, UNLESS the loop-guard case — leave open and tag the owner; a capped item is a decision for a human, not a closed triage.

Verify before acting, concretely. Read the file at the comment’s anchor as it stands now (not as described in the comment), and confirm the claim before choosing bucket 1 or 2. A comment left against a line a later commit in the same PR already changed is very often already correct or already moot — check before replying “Declined.”
Minimize churn, concretely. If a comment asks to fix a typo in one line, fix that line — do not also reformat the paragraph around it, even if the paragraph could use it. A separate cleanup is a separate PR.

Anti-examples

Example	Bucket / rule	What should have happened
A reviewer asks to rename a variable for clarity; the session also reflows the surrounding 40 lines while it’s in there.	Minimize churn	Rename only the variable; leave the surrounding lines untouched.
A reviewer’s comment describes a bug that a commit earlier in the same PR already fixed; the session fixes it “again” with a redundant edit.	Verify before acting	Reply “Already addressed — fixed in `<commit>`,” no new edit.
A should-fix comment (“consider extracting this into a helper”) arrives on round 4 of a PR capped at `max_rounds: 3`; the session extracts the helper anyway.	Loop guard	Reply “No change — at review-round cap; flagging for the owner,” leave the thread open, do not edit.
A comment claims a function is unreachable; the session declines it as a nit without checking, but the function really is dead code the diff introduced.	Verify before acting	Check the claim first — it was a real bug (dead code), so it belongs in bucket 1, not 2.

Notes

S-0017-code-review-rubric is a DIFFERENT document, applied a different way: it carries no config key at all. sdlc verify changes (the judge verb in sdlc.yaml) matches every standard whose applies_to.paths encloses a changed file and judges the diff against it directly, so S-0017 gates a diff purely by path overlap. orchestrator.review.response_policy needs its own config key instead of that same path-matching because pr-respond must resolve one specific policy document to hand to the responding session — a single lookup, not a judge run over every applicable standard. This standard is the opposite direction from S-0017: how a session RESPONDS to review feedback it already received, not how a diff gets judged in the first place.

← Back to Standards