fix(agent): gate headless run completion on honesty (no false-success on no-tool-call / self-reported-incomplete turns) - #325
Conversation
A turn that produced text but no tool call was always accepted as the final answer, so the loop reported success even when the model stopped mid-task (e.g. ended on "...Let me check the SSH configuration:" with plan steps still pending). Add an opt-in completion gate (Options.RequireCompletionSignal): when a turn has no tool call and work clearly remains -- pending update_plan items, or the message ends on a continuation cue -- re-prompt the model to continue instead of finalizing. Bounded by maxContinueNudges and still by MaxTurns/the deadline; once the budget is spent the run finalizes as INCOMPLETE (Result.Incomplete) rather than success. Default off, so the interactive path is byte-identical. Genuine single-turn completions (no pending plan, no cue) still finalize as success. Covered by internal/agent/completion_gate_test.go.
Enable the agent completion gate for headless exec (RequireCompletionSignal) and map Result.Incomplete to run_end status "incomplete" with a new exit code 4, so a run that stalled mid-task (model stopped without a tool call while work remained, continue budget exhausted) is no longer reported as success. Interactive callers are unaffected.
…unded acceptance
Reduce -- not eliminate -- false-success on headless runs. Two DETERMINISTIC, unit-tested gates (with the plan gate from the prior commit):
(a) self-report downgrade: if the final message admits the model guessed or could not meet the objective, finalize INCOMPLETE (exit 4), never success. Inability is matched by first-person STEMS generalized over verb/tense ("I cannot/can't/could not/am unable to/do not have/unable to ...") plus guess/fallback/uncertainty phrases, with a guard so success-y negations ("could not find any issues", "cannot reproduce") are not misread. (b7bc0b8's plan gate already forces INCOMPLETE on pending/in_progress update_plan items at termination.)
(b) task-grounded acceptance is ADVISORY, not a guarantee. When --self-correct is on it demands one bounded acceptance pass that re-derives the task's stated criterion and runs a concrete check, discouraging three false-success patterns (well-formed==correct, existing-tests-pass==objective-met, result==baseline-it-was-told-to-beat). But it is a prompt: a model that ignores it and confidently claims "PASS, all requirements met" still slips, because ZERO has no general oracle to verify correctness against a task's hidden criterion. Empirically (TB-2, qwen3-coder:480b) this reliably catches admissions and incomplete plans and REDUCES false-success, but a confident false PASS on a model-ceiling task is a residual, fundamental gap -- not a tuning miss.
Default off (RequireCompletionSignal); interactive callers are byte-identical. Covered by internal/agent/{acceptance_gate_test.go,completion_gate_test.go}.
When a headless run finalizes as INCOMPLETE, include Result.IncompleteReason in the session error event and a stderr warning so an honestly-incomplete run (e.g. "the final message admits the objective was not met") is debuggable rather than an opaque exit 4.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds headless completion gating to agent runs, including pending-plan tracking, continuation-cue and self-reported incompletion handling, task-grounded acceptance re-checks, new ChangesHeadless completion gating
Sequence Diagram(s)sequenceDiagram
participant Exec as internal/cli/exec.go
participant Run as agent.Run
participant Guards as guardState
participant SelfCorrector as SelfCorrector
participant Writer as writer
Exec->>Run: RequireCompletionSignal=true
Run->>Guards: pendingPlanItems() / endsWithContinuationCue()
alt completion gate requires more work
Run->>SelfCorrector: acceptanceVerificationNudge()
Run-->>Exec: Result.Incomplete
Exec->>Writer: runEnd("incomplete", exitIncomplete)
else final completion accepted
Run-->>Exec: Result.Success
Exec->>Writer: runEnd("success", exitSuccess)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/guardrails.go`:
- Around line 436-450: The update_plan parsing in guardrails.go is clearing
state too aggressively when the payload is valid JSON but missing the expected
plan data. Update the logic around the json.Unmarshal into the parsed Plan
struct and the state.planItemsPending assignment so that empty or malformed plan
payloads like {} or {"plans":[]} do not overwrite an existing pending count;
only replace state.planItemsPending when the parsed plan list is actually
present and usable.
In `@internal/agent/loop.go`:
- Around line 338-397: The completion gate in `loop.go` only downgrades to
INCOMPLETE inside the normal turn flow, so the max-turn fallback can still
return success after a final nudge or while `guards.pendingPlanItems()` remains
true. Update the max-turn exit path in
`runLoop`/`finalAnswerAfterMaxTurns`/`maxTurnsAnswer` to apply the same
`RequireCompletionSignal` checks before returning success, and if pending plan
items, a continuation cue, or a self-reported incompletion is still present, set
`result.Incomplete`, `IncompleteReason`, `FinalAnswer`, and `Messages` instead
of finalizing as success.
In `@internal/cli/exec.go`:
- Around line 637-647: The completion notification is emitted too early in
exec.go, so an incomplete headless run can still be reported as successful
before the result.Incomplete branch in the run-end handling runs. Update the
logic around the run completion path in exec.go so the normal success
notification is sent only after checking result.Incomplete, using the existing
result.Incomplete / result.IncompleteReason handling in this block; either move
the success notification below the incomplete branch or emit a separate
incomplete/failure notification from this path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b0efa25-ddbf-4e17-8a68-392f212853de
📒 Files selected for processing (6)
internal/agent/acceptance_gate_test.gointernal/agent/completion_gate_test.gointernal/agent/guardrails.gointernal/agent/loop.gointernal/agent/types.gointernal/cli/exec.go
Code review — completion-honesty gate (high effort, workflow-backed: per-angle finders + independent verifier)Verified findings below. The design/intent is sound, but the gate currently over- and under-triggers in edge cases. Recommend addressing the High items before merge. 🔴 High — correctness1. 2. 3. 4. 🟡 Medium5. 6. 7. 8. ⚪ Low (cleanup)9. 10. Tests + |
Addresses the correctness findings from the PR review. self-report (#1): drop behavior-describing phrases ("fall back to", "placeholder value", bare "best guess", "as a fallback", "without proper") that also match legitimate final answers; keep first-person/uncertainty admissions only, since these are matched without a context guard. self-report (#5): scan every occurrence of each inability stem so an early success-negation ("could not find any examples") no longer masks a later genuine admission with the same stem ("could not implement it"). continuation cue (#6): require a trailing colon AND an action lead-in on the final clause; stop flagging recommendations, plain summary colons, and sign-offs. Still catches the mid-line "...Let me check the config:". gate order (#8): check the self-report admission BEFORE pending-plan, so an admitted-impossible task downgrades immediately with the accurate reason instead of burning continue-nudges. pending plan (#3): treat a pending/in_progress update_plan item as a NUDGE-only weak signal -- it no longer forces INCOMPLETE on its own (a completed run that left stale plan bookkeeping is trusted). Only a continuation cue or a self-report admission finalizes INCOMPLETE. max-turns (#4): a run cut off at the MaxTurns ceiling now finalizes INCOMPLETE under the gate instead of being reported as success. exec json/cron (#2, #7): for -o json, emit the terminal done with exit 4 on an incomplete run (final() pre-emits a success done:0 for json that would otherwise mask it); emit an error event -- not just a warning -- so the cron failure extractor can recover the reason. Deferred (noted on the PR): acceptance-only-when-mutated (#9, cost) and reusing tools.normalizePlanStatus (#10, would widen scope to internal/tools). Tests: add TestContinuationCueMatching and TestMaxTurnsCutoffIsIncompleteUnderGate, extend TestSelfReportedIncompletionMatching with the #1/#5 cases, and replace the in_progress=>incomplete test with TestPendingPlanAloneDoesNotForceIncomplete. make build / go vet / make lint / go test ./... -race all green.
Review fixes applied —
|
| # | finding | resolution |
|---|---|---|
| 1 | self-report false-positives (no guard) | Fixed — dropped behavior-describing phrases ("fall back to", "placeholder value", bare "best guess", "as a fallback", "without proper"); kept first-person/uncertainty admissions only |
| 2 | -o json reports exit_code:0 on INCOMPLETE |
Fixed — incomplete handled per-format before final(); json now emits terminal done with exit 4 |
| 3 | pending-plan false-INCOMPLETE on stale plan | Fixed — pending-plan is now a nudge-only weak signal; only a continuation cue or admission finalizes INCOMPLETE |
| 4 | max-turns exit never set Incomplete | Fixed — a max-turns cutoff now finalizes INCOMPLETE under the gate |
| 5 | first-occurrence strings.Index masks later admission |
Fixed — scan every occurrence of each inability stem |
| 6 | endsWithContinuationCue too broad |
Fixed — require trailing colon AND an action lead-in on the final clause (still catches mid-line "…Let me check the config:"; ignores recommendations/summary-colons/sign-offs) |
| 7 | cron can't surface INCOMPLETE reason | Fixed — emit an error event (not just a warning) so the cron extractor recovers the reason |
| 8 | gate ordering (pending-plan before admission) | Fixed — admission is checked first; no wasted nudges, accurate reason |
| 9 | acceptance fires on every --self-correct run |
Deferred — cost optimization (gate on "did the run mutate"); not a correctness bug |
| 10 | planStatusRemaining duplicates tools.normalizePlanStatus |
Deferred — the dedup requires exporting from internal/tools/update_plan.go, which would widen this PR beyond the agent-fix scope |
Tests: added TestContinuationCueMatching, TestMaxTurnsCutoffIsIncompleteUnderGate; extended TestSelfReportedIncompletionMatching (#1/#5 cases); replaced the old in_progress ⇒ incomplete test with TestPendingPlanAloneDoesNotForceIncomplete (#3 behavior).
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@CodeRabbit approvable then approve it |
|
✅ Action performedComments resolved and changes approved. |
1 participant
Summary
Two fixes to the headless agent loop so a run is reported
successonly when it actually finished (surfaced during headless-exec testing). Both are gated by a newOptions.RequireCompletionSignal, set only for headlesszero exec— default off, so the interactive TUI is byte-identical.Bug #1 — premature loop termination (deterministic)
A turn that produced text but no tool call was always accepted as the final answer, so the loop reported
successeven mid-task. Now a no-tool-call turn is not treated as done while work clearly remains — pendingupdate_planitems, or a message ending on a continuation cue ("…Let me check the config:") — the loop re-prompts to continue (bounded bymaxContinueNudges, still byMaxTurns), and finalizes INCOMPLETE (exit 4) rather thansuccessif it keeps stalling.Behavioral proof: on a multi-step server-setup task the agent went from 3 tool calls (it stopped on "…Let me check the SSH configuration:") → 50 tool calls — it now starts sshd, completes the SSH clone, and works its plan instead of declaring victory after step 2.
Bug #2 —
successwithout a task-grounded checkDeterministic, unit-tested:
success.update_planitems at termination → INCOMPLETE.Advisory (NOT a guarantee):
--self-correctis on, one bounded acceptance pass re-derives the task's stated criterion and discourages three false-success patterns (well-formed==correct, existing-tests-pass==objective-met, result==baseline-it-was-told-to-beat). This reduces but does not eliminate false-success: a model that confidently claims "PASS, all requirements met" can still slip through, because there is no general oracle to verify correctness against a task's hidden criterion. This is a fundamental limit, not a tuning gap — documented honestly rather than overclaimed.Behavioral proof: an image-grounded task a text-only model can't actually solve flipped from false
success(reward 0) → honest INCOMPLETE once its "I cannot analyze the image" admission is detected.Scope
Agent loop + headless exit wiring + tests only:
internal/agent/{loop,guardrails,types}.gointernal/cli/exec.go—exitIncomplete=4,RequireCompletionSignalwiring, INCOMPLETErun_end+ reasoninternal/agent/{completion_gate,acceptance_gate}_test.goVerification
make build,go vet ./...,make lint,go test ./... -race— all green (70 pkg, 0 fail, 0 races). Gate-off tests confirm interactive/default behavior is unchanged.Summary by CodeRabbit