alibaba / alibaba/open-code-review

bug(llmloop): preserve task_done(DONE) completion from the grace round

Open
#1,018 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
24.4k
Forks
1.8k
Avg merge
2d 4m
Merged PRs (30d)
102

Description

### OpenCodeReview Version

v1.9.8 (latest release). Affected range: v1.9.3 through v1.9.8. The grace round was introduced in da972b2; v1.9.2 and earlier predate it.

### Operating System

macOS (Apple Silicon)

### Installation Method

Built from source

### LLM Provider

Other OpenAI-compatible endpoint

The deterministic reproduction uses a scripted client, so the defect is not provider-specific.

### Bug Description

When a file exhausts its tool-call budget, `RunPerFile` makes a final grace-round LLM request limited to `code_comment` and `task_done`.

If the model calls `task_done(DONE)` in that round, the tool call is parsed and executed, but its completion state is discarded. `RunPerFile` returns `completed=false`, so scan/review callers record the file as incomplete rather than persisting a reusable `review_item_done` checkpoint. A later `ocr resume` then re-runs a file that the model explicitly marked complete.

### Steps to Reproduce

1. Configure a test template with `MaxToolRequestTimes=1`.
2. Return a valid context-tool call in the first LLM response so the budget is exhausted.
3. Return `task_done` with `{"state":"DONE"}` in the grace-round response.
4. Observe that `RunPerFile` returns `(completed=false, StopMaxRounds, nil)`.
5. Run the same flow through scan mode, finalize the session, then load it with `LoadResumeState`.
6. Observe that no completed checkpoint is available and a resumed scan invokes the LLM again for the unchanged file.

This can be reproduced deterministically with a scripted LLM client; no API key is required.

### Expected Behavior

A `task_done(DONE)` executed in the grace round should propagate completion through `RunPerFile`. Scan/review should persist `review_item_done`, and a later resume should reuse the unchanged file without another LLM request.

### Logs / Error Output

```shell
main_task did not complete before stopping
```

### Additional Context

Root cause in v1.9.8:

- `internal/llmloop/loop.go:377-381` invokes `runGraceRound` after `StopMaxRounds` and then unconditionally returns `completed=false`.
- `internal/llmloop/loop.go:432-434` executes grace-round tool calls but discards each returned `TaskCheckpoint`.
- `task_done(DONE)` returns `TaskCheckpoint{Completed: true}`, but that state never reaches scan/review checkpoint persistence.

The grace round still records its LLM response and executes side effects such as `code_comment`; only the terminal completion state is lost.

Contributor guide

Open the contributing guide

Research direction

Start in internal/llmloop/loop.go at the StopMaxRounds path around lines 377-381 and the grace-round tool handling around lines 432-434. Trace the TaskCheckpoint returned by task_done(DONE) through RunPerFile, scan/review checkpoint persistence, and LoadResumeState; done means the completion reaches review_item_done and resume avoids another LLM request for the unchanged file.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
ai
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.