Skip to content

fix: honor per-file review terminal states - #582

Merged
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
4-1-1:codex/fix-review-terminal-state
Jul 31, 2026
Merged

fix: honor per-file review terminal states#582
lizhengfeng101 merged 2 commits into
alibaba:mainfrom
4-1-1:codex/fix-review-terminal-state

Conversation

@4-1-1

@4-1-1 4-1-1 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

Per-file review and scan tasks could be reported as successful even when the model explicitly returned task_done({"state":"FAILED"}) or stopped without reaching a valid terminal state. Invalid terminal states could also be accepted instead of being returned to the model for correction.

This change makes terminal-state handling explicit across the per-file agent loop and its callers:

  • DONE completes the task.
  • FAILED terminates the task as a failure.
  • Empty, unknown, non-string, or malformed states return a tool error so the model can retry.
  • A loop that exits without successful task_done is propagated as a review or scan failure.
  • Partial scan output and the session viewer use the same terminal-state semantics.
  • Legacy task_done({}) remains a successful completion for compatibility.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change
  • Refactoring
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • Added executable regression coverage for explicit DONE, FAILED, invalid state retry, and missing completion.
  • Added caller coverage for review and scan failure propagation.
  • Added viewer coverage for terminal states and tool-result matching.
  • Targeted regression tests pass.
  • go vet passes for all affected packages.

Checklist

  • My code follows the project coding style.
  • I have performed a self-review.
  • I have added tests that prove the fix is effective.
  • Relevant tests pass locally.
  • No user-facing documentation changes are required.
  • I have signed the CLA.

Related Issues

No linked issue.

@CLAassistant

CLAassistant commented Jul 29, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@4-1-1

4-1-1 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

CLA Assistant confirms that I signed the CLA as 4-1-1. I rechecked the original PR #578 and recreated it as this PR (#582), but the license/cla check remains pending. The status context currently shown on #582 still points to pullRequest=578 for commit 80f51414162dc5d3ff681aba20e69d606f5973c0, so it appears to be stale. Could a maintainer please retrigger or reset the CLA status for this commit/PR? Thank you.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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

  • ✅ Successfully posted inline: 2 comment(s)

Comment thread internal/llmloop/loop.go
Comment thread internal/viewer/store.go
@4-1-1
4-1-1 force-pushed the codex/fix-review-terminal-state branch 2 times, most recently from 31596c9 to 2507df0 Compare July 30, 2026 03:13
@4-1-1

4-1-1 commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Hi @lizhengfeng101, the CLA is now green and this PR is mergeable. When convenient, could you please take a look?

It fixes per-file tasks incorrectly treating FAILED or incomplete terminal states as successful, while preserving compatibility with legacy task_done({}). Targeted regression tests and go vet for the affected packages pass.

Thank you!

Treat FAILED and missing completion as review failures, and retry
invalid task_done states instead of accepting them as success.

Keep partial scan output and session history aligned with those states.
@4-1-1
4-1-1 force-pushed the codex/fix-review-terminal-state branch from 2507df0 to fd102b2 Compare July 31, 2026 02:24

@lizhengfeng101 lizhengfeng101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit c391892 into alibaba:main Jul 31, 2026
7 checks passed
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.

3 participants