Skip to content

fix(foreman/executor): route reviewer-role GO through modelDecidedResult#545

Merged
Defilan merged 1 commit into
defilantech:mainfrom
Defilan:fix/foreman-543-reviewer-verdict-mapping
May 26, 2026
Merged

fix(foreman/executor): route reviewer-role GO through modelDecidedResult#545
Defilan merged 1 commit into
defilantech:mainfrom
Defilan:fix/foreman-543-reviewer-verdict-mapping

Conversation

@Defilan

@Defilan Defilan commented May 25, 2026

Copy link
Copy Markdown
Member

What

Routes reviewer-role Agents' verdict=GO through modelDecidedResult instead of the HasChangesCommitPush flow that coder Agents take.

Why

Fixes #543

Reviewer Agents are read-only by design (Agent.spec.tools excludes write_file and str_replace), so they never produce workspace changes. When such an Agent submits verdict="GO" meaning APPROVE, the existing executor's GO branch hit HasChanges == false and fell through to noChangesResult, which downgraded the verdict to NO-GO with summary "model emitted GO but produced no diff" and dropped the model's submit_result.extra payload (reviewOutcome, findings, issueAsk, filesTouched, etc.) on the floor. The structured review survived only in the transcript ConfigMap.

Surfaced cleanly by the M5-lite reviewer demo on 2026-05-25: review-449, review-506, review-510 all emitted GO/APPROVE with 3-5 structured findings each, all got recorded as NO-GO no-diff at the AgenticTask level. Reviewer model behavior was correct; executor was discarding the output.

How

One-line change in pkg/foreman/agent/executor_native.go at the verdict-handling branch in runLLMPath: condition now reads "non-GO verdict OR reviewer-role Agent" and routes both to modelDecidedResult. The non-reviewer GO path (coder commit + push) is untouched.

if verdict != foremanv1alpha1.AgenticTaskVerdictGo ||
    agent.Spec.Role == foremanv1alpha1.AgentRoleReviewer {
    return e.modelDecidedResult(start, transcriptRef, loopRes, verdict), nil
}

Strict role check (not "Agent's tools lack write_file") so the contract is explicit in the CR and obvious to users modeling pipelines. Future Agent roles get their own path if they need one; this PR doesn't make assumptions about them.

Adds a regression test TestNativeExecutor_ReviewerGoIsApproveNotCommit that constructs a reviewer-role Agent with a read-only tool whitelist, has the fake loop submit GO with realistic structured extra (mirroring the v5 reviewer payload: reviewOutcome=APPROVE, two findings, issueAsk verbatim quote, filesTouched), and asserts:

  • res.Verdict == GO (not downgraded)
  • res.Extra["outcome"] == "MODEL-DECIDED" (not "NO-CHANGES")
  • res.Extra["modelExtra"] round-trips with reviewOutcome, findings, issueAsk intact
  • No branch is pushed to the bare remote (reviewers never push)
  • Transcript ConfigMap still gets written

Checklist

@codecov

codecov Bot commented May 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Reviewer-role Agents are read-only by design: their tool whitelist
excludes write_file and str_replace, so they never produce workspace
changes. When such an Agent submits verdict=GO (meaning APPROVE), the
executor's GO branch was hitting HasChanges -> false -> noChangesResult,
which (a) downgraded the verdict to NO-GO with summary
"model emitted GO but produced no diff" and (b) dropped the model's
submit_result.extra payload (reviewOutcome, findings, issueAsk,
filesTouched, etc.) on the floor. The structured review survived only
in the transcript ConfigMap.

Route reviewer-role GO through the same modelDecidedResult path as
non-GO verdicts. status.result preserves the verdict and surfaces
extra.modelExtra unmodified. No effect on coder/verifier/planner
roles: only Agent.spec.role == reviewer changes behavior.

Surfaced by the M5-lite reviewer demo on 2026-05-25: review-449,
review-506, review-510 all emitted GO/APPROVE with 3-5 structured
findings each, all got recorded as NO-GO no-diff at the task level.

Fixes defilantech#543

Signed-off-by: Christopher Maher <chris@mahercode.io>
@Defilan Defilan force-pushed the fix/foreman-543-reviewer-verdict-mapping branch from 40721bc to 04b3beb Compare May 26, 2026 00:38
@Defilan Defilan merged commit 16943a5 into defilantech:main May 26, 2026
22 checks passed
@github-actions github-actions Bot mentioned this pull request May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] executor downgrades reviewer Agent's APPROVE verdict to NO-GO no-diff

1 participant