Skip to content

fix(review): exit non-zero when a partial run has real item failures - #1254

Open
HoneyTyagii wants to merge 1 commit into
alibaba:mainfrom
HoneyTyagii:fix/review-partial-exit-nonzero-1027
Open

HoneyTyagii wants to merge 1 commit into
alibaba:mainfrom
HoneyTyagii:fix/review-partial-exit-nonzero-1027

Conversation

@HoneyTyagii

@HoneyTyagii HoneyTyagii commented Sep 14, 2026

Copy link
Copy Markdown

Summary

Fixes #1027.

ocr review exits 0 on a partial run (some items completed, some failed with timeout/provider errors), so CI cannot detect an incomplete review or a misconfigured harness from the exit status alone. The GitHub Action then skips posting (Post review comments gates on OCR_EXIT_CODE == '0') while the job stays green.

Change

  • reviewResultError (cmd/opencodereview/review_cmd.go): a partial manifest whose failed set contains any non-budget item now returns an error, so the run exits non-zero. Budget-only partials (the documented --max-tokens-budget controlled-truncation contract) keep exiting 0. Partial results are still published before the non-zero exit, so JSON consumers keep the full coverage diagnosis.
  • Updated the unit and e2e tests that pinned the old exit-0 contract, including a new mixed budget+provider partial case that counts only real failures.
  • Docs synced: --max-tokens-budget flag help, action.yml input description, and the exit-codes tables / FAQ entries across en/zh/ja/ko/ru pages.

Verification

  • go build ./..., go vet ./cmd/..., gofmt -s -l clean
  • go test ./cmd/... ./internal/session/... passes (LC_ALL=C)
  • New pinned cases: partial with a timeout failure exits non-zero; budget-only partial still exits 0

Fixes #1027

Copilot AI lite review requested due to automatic review settings September 14, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@CLAassistant

CLAassistant commented Sep 14, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

@rajpratham1 rajpratham1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new exit-code behavior is directionally correct, but the process status now depends on nonBudgetFailures() treating every non-FailureBudget classification as a real failure. Since this changes the CLI/CI contract, please add coverage for the other possible failure classifications and verify that only controlled budget truncation returns 0 for a partial run. After those cases are covered, this should be ready to merge.

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.

ocr timed out and failed for some files, but status code=0

4 participants