Skip to content

fix(cmd): return non-zero for partial operational failures - #1030

Open
Qiyuanqiii wants to merge 3 commits into
alibaba:mainfrom
Qiyuanqiii:codex/fix-partial-failure-exit-code
Open

Qiyuanqiii wants to merge 3 commits into
alibaba:mainfrom
Qiyuanqiii:codex/fix-partial-failure-exit-code

Conversation

@Qiyuanqiii

@Qiyuanqiii Qiyuanqiii commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Description

Summary

  • Return a non-zero process status when a partial review contains an operational review-item failure such as a timeout, provider error, bad configuration, panic, cancellation, or unknown failure.
  • Preserve the completed findings, manifest, warnings, and retry report before returning the process error.
  • Keep the historical successful status for partial reviews caused exclusively by configured budget limits.
  • Use the same success policy for the CLI exit status and SARIF executionSuccessful metadata.
  • Add regression coverage for timeout, provider, budget-only, mixed, fully failed, JSON, text, and SARIF outcomes.
  • Synchronize the exit-code documentation across the English, Chinese, Japanese, Korean, and Russian documentation.

Motivation and Scope

Issue #1027 reports that a review with one timed-out file produces a partial result but exits with status 0. That makes CI unable to distinguish complete coverage from an operationally incomplete review.

The previous aggregate exit logic returned an error only for a fully failed manifest. This PR narrows the successful partial case to declared budget limits and makes other operational review-item failures visible to CI without discarding usable output from completed review items.

Implementation

  • Add a shared reviewManifestRequiresNonZeroExit policy for terminal manifest evaluation.
  • Return review incomplete with selected/failed item counts for operational partial failures.
  • Keep fully failed manifests non-zero regardless of failure classification.
  • Apply the same policy to SARIF invocation success metadata while continuing to publish partial SARIF output.
  • Update JSON and text end-to-end tests so provider failures require a non-zero status while preserving their emitted reports.
  • Cover timeout and mixed failure classifications directly, including the budget-only compatibility boundary.
  • Update CLI reference and FAQ content in all maintained locales.

Compatibility and User Impact

  • Complete and skipped reviews continue to exit 0.
  • Partial reviews caused only by configured budget limits continue to exit 0.
  • Partial reviews containing operational failures now exit 1, allowing CI to detect incomplete coverage.
  • Findings and diagnostics from completed files remain available before the error is returned.
  • No CLI flags, configuration keys, manifest schema, or output formats change.

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?

Initial Validation Environment

  • OS: Windows/amd64, build 26200.9168
  • Go: go1.26.5 windows/amd64
  • Baseline: cfbb62e2296d7684dc648e27fe1b906e8c960f9c

Initial Automated Validation

  • Project make check target through mingw32-make: license headers, English-only source check, go mod tidy, formatting, and go vet
  • Project make test target through mingw32-make: complete Go test suite with the race detector
  • Project make coverage target through mingw32-make: 90.6% total statement coverage, above the required 90%
  • git diff --check
  • LF-only verification for every changed file

Documentation Completion and Main Sync

The Korean CLI reference and FAQ now describe operational partial failures, preserved review output, and budget-only partial success, matching the other four locales.

  • Merged upstream 070805f86cc6ae4b987bfe733dfc14ecd5bb803a in ecbf7dab0b6dfe89e7d0fef181592e698d063db1. The two retry-report E2E assertions now match upstream's four-file fixture.
  • Added the Korean documentation in 91b3bfa4452aaae710d6e4157f4cf4f40ed7ac69.
  • Validation after the sync: make check, make test for cmd/opencodereview and internal/agent with the race detector, and full make coverage passed. Total statement coverage is 91.1%.
  • Documentation translation sync, repository links, UTF-8/LF validation, and git diff --check passed.

Regression Coverage

  • A partial manifest containing a timeout now produces a process error with failed/selected item counts.
  • Provider-failure end-to-end tests require a non-zero status while still parsing the emitted JSON and text retry reports.
  • A budget-only partial remains successful; a mixed budget and timeout partial fails.
  • A fully failed manifest remains non-zero regardless of classification.
  • SARIF marks operational partial runs unsuccessful and budget-only partial runs successful.

The repository-requested self-review completed successfully:

[ocr] Summary: 3 file(s) reviewed, 0 comment(s)
Review complete: 0 finding(s) across 3 selected item(s).

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 the fix works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly
  • I have signed the CLA

Known Limitations

  • Local validation was performed on Windows/amd64; remote CI remains responsible for the repository's other supported environments.
  • The timeout exit boundary is covered deterministically at the manifest layer rather than by waiting for a live slow provider; provider-failure end-to-end tests exercise the same partial-output/non-zero-exit command path.

Related Issues

Closes #1027.

@github-actions

Copy link
Copy Markdown
Contributor

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

@Qiyuanqiii
Qiyuanqiii marked this pull request as ready for review August 21, 2026 10:44

@wu21-web wu21-web left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good job.

@Qiyuanqiii
Qiyuanqiii force-pushed the codex/fix-partial-failure-exit-code branch from c6ea09f to 4469700 Compare August 25, 2026 13:38
@crestonepeaks

Copy link
Copy Markdown
Contributor

@lizhengfeng101, would you be able to review the current head when you have a chance? We’re successfully using OpenCodeReview downstream, and we’re treating the fix for #1027 as the remaining upstream blocker before enabling a broader rollout. The current checks are green and GitHub reports the PR as mergeable. If additional changes are needed—or if you can share an approximate review timeline—we’d appreciate the guidance. We’re also happy to help with this PR in any way that would be useful. Thank you!

This branch has not been deployed

No deployments
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

3 participants