ocr runs with || true, so a review that fails reports a passing check.
.github/workflows/code-review.yml:126:
ocr "${ARGS[@]}" > /tmp/ocr-result.json 2>/tmp/ocr-stderr.log || trueThe exit code is discarded unconditionally, and no later step reads it. The review / code-review check therefore reports success whether the review produced findings, produced nothing, or crashed.
Observed
On vanducng/skills#381 the reviewer failed twice with an internal error:
agent: model produced no tool calls and no parseable findings after 3 rounds
Both runs reported a green review / code-review check. The failure was only visible by reading the bot's comment body. A green check on a failed review is worse than a red one: it tells a maintainer the change was reviewed when nothing was reviewed, and any branch protection built on that check is satisfied by a review that did not happen.
Why || true is presumably there
A non-zero exit would fail the job, and the following step still wants to post whatever partial output exists (including the stderr notice at line 149). That intent is right; discarding the status is the part that is not.
Suggested change
Capture the status, let the posting step run, then fail the job at the end:
set +e
ocr "${ARGS[@]}" > /tmp/ocr-result.json 2>/tmp/ocr-stderr.log
echo "ocr_exit=$?" >> "$GITHUB_OUTPUT"
set -eand a final step:
- name: Fail if the review did not complete
if: steps.review.outputs.ocr_exit != '0'
run: |
echo "::error::OpenCodeReview exited ${{ steps.review.outputs.ocr_exit }} - review did not complete"
exit ${{ steps.review.outputs.ocr_exit }}That keeps the comment-posting behaviour and makes the check honest. If a hard failure is too aggressive for org-wide rollout, a middle option is to keep the job green but post the failure as a check annotation via ::error::, so it is visible in the Checks tab rather than only inside a comment body.
Worth distinguishing a genuine crash from "ran fine, found nothing" - only the former should fail.
ocrruns with|| true, so a review that fails reports a passing check..github/workflows/code-review.yml:126:The exit code is discarded unconditionally, and no later step reads it. The
review / code-reviewcheck therefore reportssuccesswhether the review produced findings, produced nothing, or crashed.Observed
On
vanducng/skills#381the reviewer failed twice with an internal error:Both runs reported a green
review / code-reviewcheck. The failure was only visible by reading the bot's comment body. A green check on a failed review is worse than a red one: it tells a maintainer the change was reviewed when nothing was reviewed, and any branch protection built on that check is satisfied by a review that did not happen.Why
|| trueis presumably thereA non-zero exit would fail the job, and the following step still wants to post whatever partial output exists (including the stderr notice at line 149). That intent is right; discarding the status is the part that is not.
Suggested change
Capture the status, let the posting step run, then fail the job at the end:
and a final step:
That keeps the comment-posting behaviour and makes the check honest. If a hard failure is too aggressive for org-wide rollout, a middle option is to keep the job green but post the failure as a check annotation via
::error::, so it is visible in the Checks tab rather than only inside a comment body.Worth distinguishing a genuine crash from "ran fine, found nothing" - only the former should fail.