This is an automated email from the ASF dual-hosted git repository.
hello-stephen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new d4f052b9efa [fix](ci) Check terminal events before accepting review
recovery (#68372)
d4f052b9efa is described below
commit d4f052b9efa8154d201998ef18ebe9f00f59ada7
Author: shuke <[email protected]>
AuthorDate: Tue Sep 22 16:01:46 2026 +0800
[fix](ci) Check terminal events before accepting review recovery (#68372)
### What problem does this PR solve?
After a capacity error, the review helper can accept `codex exec --goal
resume` exiting with status 0 even when the attempt reports
`turn.failed` or never produces a terminal event. The helper then stops
retrying and the workflow replaces the underlying error with “no new
pull request review was submitted.”
Require both a zero process exit and a completed latest turn before
accepting an attempt. Explicit capacity errors remain eligible for the
existing bounded retry sequence even when the process exits with 0.
Authentication, usage-limit and other errors fail without retry; a
missing terminal event fails explicitly instead of borrowing a previous
attempt’s capacity error or completion. Existing PR-state checks,
duplicate-submission protection and the shared deadline are retained.
Each attempt now logs its exit status, last turn event and event count.
The installation step logs the actual goal binary’s SHA-256 to make
runtime failures reproducible.
Related PR: #67624
### Validation
- Linux Python 3.12: all 147 pipeline tests passed, including real
process cleanup and workflow/auth-quarantine integration tests. Tests
ran without network access or host mounts.
- Added 12 regression tests for zero-exit capacity retries and
exhaustion, zero-exit authentication failures, missing terminal events,
event ordering and workflow error propagation.
- The new helper tests fail against the previous implementation,
including false success and premature retry termination; they pass after
the fix.
- macOS helper suite: 55 tests, passed with 3 Linux-only skips.
- Ruff checks and helper/test formatting, actionlint, workflow YAML
parsing, all 22 shell blocks with `bash -n`, and `git diff --check`
passed.
**Rollout limit:** this validates helper behavior with controlled Codex
output and real Linux subprocesses. The actual OSS goal binary was not
available for validation (anonymous read is denied); no claim is made
that the underlying binary’s goal/resume issue is fixed. Before
production rollout, validate that exact binary’s resumed turn execution
and terminal-event behavior. The change fails incomplete resumes
explicitly; it does not blindly retry empty successful exits.
---
.github/scripts/run_review_with_resume.py | 53 +++++++--
.github/scripts/test_review_auth_quarantine.py | 14 +++
.github/scripts/test_run_review_with_resume.py | 143 +++++++++++++++++++++++++
.github/workflows/code-review-runner.yml | 3 +
4 files changed, 203 insertions(+), 10 deletions(-)
diff --git a/.github/scripts/run_review_with_resume.py
b/.github/scripts/run_review_with_resume.py
index 0803ad4c147..c27a65c9fdf 100755
--- a/.github/scripts/run_review_with_resume.py
+++ b/.github/scripts/run_review_with_resume.py
@@ -105,14 +105,41 @@ def read_events(path):
return events
-def failure(events, status, stderr_path):
- for event_type in ("turn.failed", "error"):
- for event in reversed(events):
- if event.get("type") == event_type:
- error = event.get("error") or event
- return error.get("message") or f"Codex exited with status
{status}"
+def attempt_result(events, status, stderr_path):
+ """Use the latest turn in this attempt, never a previous attempt's
failure."""
+ terminal = next(
+ (
+ event
+ for event in reversed(events)
+ if event.get("type")
+ in ("turn.started", "turn.completed", "turn.failed", "error")
+ ),
+ {},
+ )
+ event_type = terminal.get("type", "missing")
+ if event_type == "turn.completed":
+ message = (
+ None
+ if status == 0
+ else f"Codex exited with status {status} after turn.completed"
+ )
+ return event_type, message
+ if event_type in ("turn.failed", "error"):
+ error = terminal.get("error") or terminal
+ message = (
+ error.get("message")
+ or f"Codex reported {event_type} (exit status {status})"
+ )
+ return event_type, message
+ # A zero exit after only thread.started (or an unfinished new turn) does
+ # not prove recovery. Do not reuse an older capacity event to retry it.
+ if status == 0:
+ return (
+ event_type,
+ "Codex exited with status 0 without a terminal turn event; review
is incomplete",
+ )
lines = stderr_path.read_text(errors="replace").splitlines()
- return next(
+ return event_type, next(
(line for line in reversed(lines) if line.strip()),
f"Codex exited with status {status}",
)
@@ -388,8 +415,14 @@ def run_review(args, reaper=None):
return fail(
f"Codex was interrupted or timed out (status {status});
not resuming"
)
- message = failure(events, status, stderr_path)
- if status != 0 and (
+ terminal, message = attempt_result(events, status, stderr_path)
+ print(
+ f"Finished Codex review attempt {attempt + 1}/4 "
+ f"(exit_status={status}, terminal_event={terminal},
events={len(events)})",
+ file=sys.stderr,
+ flush=True,
+ )
+ if message is not None and (
message != CAPACITY_MESSAGE or attempt == len(RETRY_DELAYS)
):
return fail(message)
@@ -399,7 +432,7 @@ def run_review(args, reaper=None):
"Codex resumed a different session; refusing further
attempts"
)
thread_id = current_id
- if status == 0:
+ if message is None:
return 0
require_rollout(Path(os.environ["CODEX_HOME"]), thread_id,
args.cwd)
# Check parser support without authenticating or starting a model
request.
diff --git a/.github/scripts/test_review_auth_quarantine.py
b/.github/scripts/test_review_auth_quarantine.py
index 2f414d97391..cc9b509cba7 100755
--- a/.github/scripts/test_review_auth_quarantine.py
+++ b/.github/scripts/test_review_auth_quarantine.py
@@ -373,9 +373,23 @@ class ReviewAuthQuarantineTest(unittest.TestCase):
{"type": "turn.failed", "error": {"message": "Request timed out"}},
], expected_invalid=False)
+ def test_zero_exit_auth_failure_still_reaches_quarantine(self):
+ _, outputs = self.fail_review(FAKE_CODEX_STATUS="0")
+ self.assertIn(REUSED_MESSAGE, outputs)
+ self.assertNotIn("no new pull request review", outputs)
+
+ def
test_zero_exit_without_terminal_event_does_not_pass_with_a_review(self):
+ _, outputs = self.fail_review(
+ events=[], expected_invalid=False, FAKE_CODEX_STATUS="0"
+ )
+ # The fake GitHub API reports a review, but an incomplete attempt must
+ # still fail rather than borrowing that review as proof of completion.
+ self.assertIn("without a terminal turn event", outputs)
+
def test_success_is_not_quarantined_even_with_earlier_stderr_error(self):
_, outputs = self.run_step(
"Run automated code review", FAKE_CODEX_STATUS="0",
+ FAKE_CODEX_EVENTS=json.dumps({"type": "turn.completed", "usage":
{}}),
FAKE_CODEX_STDERR='{"code":"refresh_token_reused"}',
)
self.assertNotIn("auth_invalid_reason", outputs)
diff --git a/.github/scripts/test_run_review_with_resume.py
b/.github/scripts/test_run_review_with_resume.py
index 24f91723f4b..54a69fb7916 100755
--- a/.github/scripts/test_run_review_with_resume.py
+++ b/.github/scripts/test_run_review_with_resume.py
@@ -188,6 +188,149 @@ class ResumeReviewTest(unittest.TestCase):
self.assertEqual("done", (self.context /
"codex-final-message.txt").read_text())
self.target_check.assert_called_once()
+ def test_zero_exit_capacity_on_resume_still_retries(self):
+ self.assertEqual(
+ 0,
+ self.execute(
+ [
+ {"events": [thread_event(), failed()]},
+ {"events": [thread_event(), failed()], "status": 0},
+ {"events": [thread_event(), completed()], "status": 0},
+ ]
+ ),
+ )
+ self.assertEqual(3, len(self.commands))
+ self.assertEqual([30, 60], self.sleeps)
+ self.assertEqual(2, self.target_check.call_count)
+ self.assertEqual("completed",
exporter.latest_turn_result(self.events())[0])
+
+ def test_zero_exit_capacity_stops_at_the_retry_limit(self):
+ self.assertEqual(
+ 1, self.execute([{"events": [thread_event(), failed()], "status":
0}] * 4)
+ )
+ self.assertEqual([30, 60, 120], self.sleeps)
+ self.assertEqual(4, len(self.commands))
+ self.assertEqual(runner.CAPACITY_MESSAGE, self.last_error())
+
+ def test_zero_exit_error_event_can_identify_capacity(self):
+ self.assertEqual(
+ 0,
+ self.execute(
+ [
+ {
+ "events": [
+ thread_event(),
+ {"type": "error", "message":
runner.CAPACITY_MESSAGE},
+ ],
+ "status": 0,
+ },
+ {"events": [thread_event(), completed()], "status": 0},
+ ]
+ ),
+ )
+ self.assertEqual([30], self.sleeps)
+
+ def test_zero_exit_auth_usage_and_generic_failures_do_not_retry(self):
+ for message in (
+ "refresh_token_reused",
+ "You've hit your usage limit.",
+ "HTTP 500",
+ ):
+ with self.subTest(message=message), tempfile.TemporaryDirectory()
as tmp:
+ self.args.context_dir = Path(tmp)
+ (self.args.context_dir /
"codex_goal_prompt.txt").write_text("review")
+ self.assertEqual(
+ 1,
+ self.execute(
+ [{"events": [thread_event(), failed(message)],
"status": 0}]
+ ),
+ )
+ events = runner.read_events(
+ self.args.context_dir / "codex-events.jsonl"
+ )
+ self.assertEqual(message, events[-1]["error"]["message"])
+ self.assertEqual([], self.sleeps)
+ self.target_check.assert_not_called()
+
+ def test_zero_exit_without_terminal_event_fails_closed(self):
+ self.assertEqual(1, self.execute([{"events": [thread_event()],
"status": 0}]))
+ self.assertIn("without a terminal turn event", self.last_error())
+ self.assertEqual([], self.sleeps)
+
+ def test_empty_resume_does_not_reuse_the_previous_capacity_error(self):
+ self.assertEqual(
+ 1,
+ self.execute(
+ [
+ {"events": [thread_event(), failed()]},
+ {"events": [thread_event()], "status": 0},
+ ]
+ ),
+ )
+ self.assertEqual(2, len(self.commands))
+ self.assertEqual([30], self.sleeps)
+ self.assertIn("without a terminal turn event", self.last_error())
+ self.assertNotEqual(runner.CAPACITY_MESSAGE, self.last_error())
+
+ def
test_later_completion_supersedes_earlier_error_in_the_same_attempt(self):
+ self.assertEqual(
+ 0,
+ self.execute(
+ [{"events": [thread_event(), failed(), completed()], "status":
0}]
+ ),
+ )
+ self.assertEqual([], self.sleeps)
+
+ def test_later_failure_supersedes_earlier_completion(self):
+ self.assertEqual(
+ 1,
+ self.execute(
+ [
+ {
+ "events": [thread_event(), completed(), failed("auth
failed")],
+ "status": 0,
+ }
+ ]
+ ),
+ )
+ self.assertEqual("auth failed", self.last_error())
+ self.assertEqual([], self.sleeps)
+
+ def test_unfinished_new_turn_does_not_reuse_an_earlier_completion(self):
+ self.assertEqual(
+ 1,
+ self.execute(
+ [
+ {
+ "events": [
+ thread_event(),
+ completed(),
+ {"type": "turn.started"},
+ ],
+ "status": 0,
+ }
+ ]
+ ),
+ )
+ self.assertIn("without a terminal turn event", self.last_error())
+ self.assertEqual([], self.sleeps)
+
+ def
test_completion_with_nonzero_exit_does_not_retry_old_stderr_capacity(self):
+ self.assertEqual(
+ 1,
+ self.execute(
+ [
+ {
+ "events": [thread_event(), completed()],
+ "status": 1,
+ "stderr": runner.CAPACITY_MESSAGE,
+ }
+ ]
+ ),
+ )
+ self.assertIn("status 1 after turn.completed", self.last_error())
+ self.assertEqual([], self.sleeps)
+
def test_retry_count_is_bounded(self):
self.assertEqual(1, self.execute([{"events": [thread_event(),
failed()]}] * 4))
self.assertEqual([30, 60, 120], self.sleeps)
diff --git a/.github/workflows/code-review-runner.yml
b/.github/workflows/code-review-runner.yml
index 75b168d4d96..dcb97cf206d 100644
--- a/.github/workflows/code-review-runner.yml
+++ b/.github/workflows/code-review-runner.yml
@@ -200,6 +200,9 @@ jobs:
sudo install -m 0755 "$tmp_dir/codex-goal" "$codex_target"
"$codex_cmd" exec --help | grep -q -- '--goal'
"$codex_cmd" --version
+ # The deployed goal binary currently reports only 0.0.0. Record its
+ # identity so a resume failure can be reproduced with the same build.
+ sha256sum "$codex_target"
env:
OSS_AK: ${{ secrets.OSS_AK }}
OSS_SK: ${{ secrets.OSS_SK }}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
