fix(llmloop): propagate grace-round task_done completion

[AllenMuu]

Description

When the tool-request budget is exhausted, RunPerFile runs one grace round, giving the model a final chance to submit findings.

Previously, the grace round discarded the result of task_done: a model that spent its normal rounds on context tools and then correctly called task_done(DONE) in the grace round was still recorded as incomplete. Callers therefore kept the misleading main_task did not complete before stopping result instead of recording a reusable checkpoint.

The grace round now mirrors the main loop's terminal semantics:

• task_done(DONE) completes the run. RunPerFile returns completed=true with StopNone; remaining calls in the same round still execute, so accompanying code_comment calls land.
• task_done(FAILED) returns a task-failure error. The round stops immediately and does not execute subsequent calls, intentionally matching the main loop's DONE/FAILED asymmetry.
• Cancellation returns ctx.Err() at every grace-round window. Callers classify it as FailureCancelled, consistently with the main loop and run-level cancellation handling.
• Transient LLM errors remain swallowed, so a network failure in this best-effort round does not mask the original budget stop.
• Task-failure errors wrap ErrTaskFailed, allowing callers to identify them with errors.Is while preserving the existing error text.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (describe below)

Unit tests in internal/llmloop:

• TestRunPerFile_GraceRoundTaskDoneCompletes — the core regression: task_done(DONE) in the grace round completes the run.
• TestRunPerFile_GraceRoundCommentAfterTaskDoneStillLands / TestRunPerFile_GraceRoundTaskDoneFailedSurfaces — same-round ordering semantics for DONE and FAILED, including the ErrTaskFailed sentinel.
• TestRunPerFile_GraceRoundLLMErrorSwallowed — a transient LLM outage keeps budget exhaustion as the stop cause.
• TestRunPerFile_GraceRoundCancelled{DuringLLMCall,AfterResponseDiscarded,BetweenToolCalls,InFinalCallDiscardsCompletion} and TestRunPerFile_GraceRoundSkippedWhenContextCancelled — each grace-round cancellation window surfaces context.Canceled.
• grace_round_protocol_test.go — propagation verified over the OpenAI chat-completions, Anthropic messages, and OpenAI responses wire formats, with normal-round control groups.

Manual verification used real LLM runs against a scratch Git repository with a staged Go diff containing a swallowed-error bug:

• Before/after comparison. Built binaries from the pre-fix v1.9.8 baseline and this branch. For a deterministic grace-round trigger, each binary used a local test-only template change setting MAX_TOOL_REQUEST_TIMES=1; that change is not part of this PR. Both binaries ran ocr review --audience agent --no-filter against the same repository.
  • Before: Review failed: 0 finding(s); 1 of 1 selected item(s) failed — the grace round's task_done(DONE) was dropped, and the misleading error told the user to "check your LLM configuration and API key".
  • After: Review complete: 1 finding(s) across 1 selected item(s) — the code_comment submitted in the grace round landed, correctly flagging the swallowed error with a suggested fix; completion propagated instead of failing the item.
• Cancellation. Re-ran the review and sent SIGINT mid-run. The summary reported Review failed (cancelled) and the per-item retry report recorded server.go / main_task #1: cancelled; item-level and run-level attribution agree rather than classifying the item as budget exhaustion. The precise grace-round cancellation windows are covered by unit tests because LLM latency jitter makes them hard to trigger deterministically in a live run.

Downstream coverage: the new scan integration test verifies checkpoint persistence and resume reuse. Existing internal/agent and internal/scan suites also pass.

Additional local verification:

• go test -race on internal/llmloop passes.
• make check passes locally.
• make coverage passes, meeting the repository's 90% threshold.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

Closes #1018

[github-actions]

🔍 OpenCodeReview found 2 issue(s) in this PR.

• ✅ Successfully posted inline: 2 comment(s)