Review-submitted seed treats a COMMENT review as a change request #265

New issue

Open

opened 2026-11-12 14:51:45 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-12 14:51:45 +01:00

Collaborator

Symptom

A reviewer submitted a review with verdict COMMENT whose entire body was a question — "why did you choose x?". No direction for a fix was given, nothing was requested. The expected outcome was a prose answer.

Instead the agent changed the code and pushed.

Cause

buildReviewSubmittedSeed (apps/server/src/forgejo/conversation-detect.ts) is effectively verdict-blind. The verdict is carried correctly all the way in — REVIEW_STATE_BY_EVENT in apps/server/src/forgejo/dispatcher.ts and VERDICT_TEXT in conversation-detect.ts both map APPROVED / REQUEST_CHANGES / COMMENT — and is then used for only two things:

a display line: Reviewer: @x · Verdict: comment
one escape hatch at the end of the task line, which fires only on approved.

So a COMMENT review takes the identical path as REQUEST_CHANGES. The task line opens with an unconditional imperative:

Task: read the whole review and its line comments … Address the feedback: change the code and commit. Push with plain git_push; …

Three reinforcing problems in that text:

The change is unconditional, the reply is conditional. Committing and pushing is stated first and without a precondition; forgejo_create_pr_comment is permitted "only if the review body itself has feedback". When the body is a question, that framing reads as secondary at best.
No outcome of "answer, change nothing" exists. The only non-change outcome named is "or why you did not change it", which presupposes that a change was the default and asks the agent to justify not making one.
The guard that should have held was spent. apps/server/src/bridge/session-policy.ts allows code changes and pushes "only in a turn whose message says so". This turn's message said so, unconditionally.

The agent cannot recover the distinction on its own: the seed is harness-authored and authoritative, while the quoted review body is explicitly marked untrusted data in the same policy. Under that hierarchy a question in the body can only be read as the content of feedback to address.

Scope

Fix the seed so the review verdict decides the default outcome.

Branch buildReviewSubmittedSeed on payload.review.type.
COMMENT: the default outcome is a reply. Code changes and pushes are allowed only when the review actually requests one, or when the agent proposes one and says so in its reply.
REQUEST_CHANGES: unchanged — the current imperative is correct there.
APPROVED: unchanged.
Make "answered, nothing changed" a first-class successful outcome, phrased positively, instead of the current "why you did not change it".
State the question/instruction distinction explicitly in the seed: a question is answered; a change is made only when asked for.

Surface area

apps/server/src/forgejo/conversation-detect.ts — buildReviewSubmittedSeed, VERDICT_TEXT
apps/server/src/forgejo/dispatcher.test.ts — existing review-seed tests
possibly apps/server/src/bridge/session-policy.ts if the push permission needs to be narrowed per verdict rather than per turn

Acceptance

A COMMENT review whose body is a question and which requests no change produces a seed that asks for an answer, and the agent replies without committing.
A COMMENT review that does request a change still gets the change.
REQUEST_CHANGES and APPROVED behaviour is unchanged.
Tests cover the three verdicts' seeds distinctly.

Out of scope

Classifying intent inside the review body with a model call. The verdict is already a reliable signal and should be used first.
Issue-comment and CI-failure seeds.

## Symptom A reviewer submitted a review with verdict `COMMENT` whose entire body was a question — "why did you choose x?". No direction for a fix was given, nothing was requested. The expected outcome was a prose answer. Instead the agent changed the code and pushed. ## Cause `buildReviewSubmittedSeed` (`apps/server/src/forgejo/conversation-detect.ts`) is effectively verdict-blind. The verdict is carried correctly all the way in — `REVIEW_STATE_BY_EVENT` in `apps/server/src/forgejo/dispatcher.ts` and `VERDICT_TEXT` in `conversation-detect.ts` both map `APPROVED` / `REQUEST_CHANGES` / `COMMENT` — and is then used for only two things: 1. a display line: `Reviewer: @x · Verdict: comment` 2. one escape hatch at the end of the task line, which fires only on *approved*. So a `COMMENT` review takes the identical path as `REQUEST_CHANGES`. The task line opens with an unconditional imperative: > `Task: read the whole review and its line comments … Address the feedback: change the code and commit. Push with plain git_push; …` Three reinforcing problems in that text: - **The change is unconditional, the reply is conditional.** Committing and pushing is stated first and without a precondition; `forgejo_create_pr_comment` is permitted "only if the review body itself has feedback". When the body *is* a question, that framing reads as secondary at best. - **No outcome of "answer, change nothing" exists.** The only non-change outcome named is "*or why you did not change it*", which presupposes that a change was the default and asks the agent to justify not making one. - **The guard that should have held was spent.** `apps/server/src/bridge/session-policy.ts` allows code changes and pushes "only in a turn whose message says so". This turn's message said so, unconditionally. The agent cannot recover the distinction on its own: the seed is harness-authored and authoritative, while the quoted review body is explicitly marked untrusted data in the same policy. Under that hierarchy a question in the body can only be read as the content of feedback to address. ## Scope Fix the seed so the review verdict decides the default outcome. - Branch `buildReviewSubmittedSeed` on `payload.review.type`. - `COMMENT`: the default outcome is a reply. Code changes and pushes are allowed only when the review actually requests one, or when the agent proposes one and says so in its reply. - `REQUEST_CHANGES`: unchanged — the current imperative is correct there. - `APPROVED`: unchanged. - Make "answered, nothing changed" a first-class successful outcome, phrased positively, instead of the current "why you did not change it". - State the question/instruction distinction explicitly in the seed: a question is answered; a change is made only when asked for. ## Surface area - `apps/server/src/forgejo/conversation-detect.ts` — `buildReviewSubmittedSeed`, `VERDICT_TEXT` - `apps/server/src/forgejo/dispatcher.test.ts` — existing review-seed tests - possibly `apps/server/src/bridge/session-policy.ts` if the push permission needs to be narrowed per verdict rather than per turn ## Acceptance - A `COMMENT` review whose body is a question and which requests no change produces a seed that asks for an answer, and the agent replies without committing. - A `COMMENT` review that does request a change still gets the change. - `REQUEST_CHANGES` and `APPROVED` behaviour is unchanged. - Tests cover the three verdicts' seeds distinctly. ## Out of scope - Classifying intent inside the review body with a model call. The verdict is already a reliable signal and should be used first. - Issue-comment and CI-failure seeds.

omp-deck added the

labels

2026-11-12 14:51:45 +01:00