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 3c060af5da7 [fix](ci) Deliver non-converged reviews at the round limit
(#68996)
3c060af5da7 is described below
commit 3c060af5da766b4483edbfc974369e42a3372a72
Author: shuke <[email protected]>
AuthorDate: Tue Oct 13 16:15:46 2026 +0800
[fix](ci) Deliver non-converged reviews at the round limit (#68996)
Problem Summary:
The review runner simultaneously required unanimous
`NO_NEW_VALUABLE_FINDINGS` before submission and required submission
after at most three rounds. When round 3 still produced candidates, the
agent could mark its goal blocked and withhold verified findings. This
happened in the reviews of
[#67549](https://github.com/apache/doris/actions/runs/38222336278) and
[#68499](https://github.com/apache/doris/actions/runs/38271916203).
Separate verified delivery from convergence:
- Submit after convergence or the third round. At the cap, publish
verified findings and disclose remaining questions or coverage gaps;
unverified candidates must not become inline findings.
- Require `converged`, `rounds`, and `incomplete_reason` in the
submission. Non-convergence requires round 3 and a nonempty reason. The
trusted helper renders this declaration into the review body and binds
it to the existing digest and exact API readback.
- A verified delivery completes execution, but non-convergence produces
a failing review status even with zero P0/P1. Both the source and
aggregate status explain non-convergence. Reviews without P0/P1 remain
GitHub comments; no artificial blocking finding is created.
- Preserve head/base validation, submission deduplication, capacity
recovery, stale-head cancellation, and existing authorized local/skip
overrides.
The prompt and helpers are loaded from the same workflow revision.
Existing invocations retain their old schema; new invocations reject
missing convergence fields rather than defaulting to a pass. This change
does not update the agent binary or model.
---
.github/scripts/resolve_code_review_status.py | 15 ++-
.github/scripts/run_review_with_resume.py | 1 +
.github/scripts/submit_review.py | 33 +++++-
.github/scripts/test_resolve_code_review_status.py | 30 ++++++
.../scripts/test_review_convergence_workflow.py | 117 +++++++++++++++++++++
.github/scripts/test_run_review_with_resume.py | 2 +
.github/scripts/test_submit_review.py | 99 +++++++++++++++--
.github/workflows/code-review-runner.yml | 37 ++++---
8 files changed, 303 insertions(+), 31 deletions(-)
diff --git a/.github/scripts/resolve_code_review_status.py
b/.github/scripts/resolve_code_review_status.py
index a2d54302577..5bc50f32417 100644
--- a/.github/scripts/resolve_code_review_status.py
+++ b/.github/scripts/resolve_code_review_status.py
@@ -73,19 +73,30 @@ def resolve_status(
approved_contexts: set[tuple[int, str]] = set()
blocked_contexts: set[tuple[int, str]] = set()
+ incomplete_contexts: set[tuple[int, str]] = set()
for context, item in latest_by_context.items():
match = SOURCE_CONTEXT_RE.fullmatch(context)
if match is not None and item["state"] == "success":
approved_contexts.add((int(match.group(1)),
match.group(2).casefold()))
elif (match is not None and item["state"] == "failure"
and context.startswith("code-review/source/automated/")):
- blocked_contexts.add((int(match.group(1)),
match.group(2).casefold()))
+ key = (int(match.group(1)), match.group(2).casefold())
+ blocked_contexts.add(key)
+ # State remains the sole gating signal. Use the runner's
description
+ # only to explain an already-blocking result, never to grant a
pass.
+ description = item.get("description")
+ if isinstance(description, str) and description.startswith("Review
did not converge after "):
+ incomplete_contexts.add(key)
# Preserve the authorized local PASS / skip override. Otherwise a completed
- # automated review with P0/P1 blocks, rather than appearing to still be
running.
+ # automated review with P0/P1 or incomplete coverage blocks instead of
pending.
blocked = sorted((open_contexts - approved_contexts) & blocked_contexts)
if blocked:
number, base_sha = blocked[0]
+ if (number, base_sha) in incomplete_contexts:
+ return Resolution(
+ "failure", f"Review did not converge for PR #{number} at
{base_sha[:12]}.",
+ )
return Resolution(
"failure", f"Review completed with P0/P1 for PR #{number} at
{base_sha[:12]}.",
)
diff --git a/.github/scripts/run_review_with_resume.py
b/.github/scripts/run_review_with_resume.py
index b0dfd51ee36..7f8bbf1589d 100644
--- a/.github/scripts/run_review_with_resume.py
+++ b/.github/scripts/run_review_with_resume.py
@@ -568,6 +568,7 @@ def run_review(args, reaper=None):
print(
f"Verified final review {result['review_id']}: "
f"P0={result['p0']}, P1={result['p1']}, "
+ f"converged={result['converged']},
rounds={result['rounds']}, "
f"recovered_after_capacity={result['recovered_after_capacity']}",
file=sys.stderr, flush=True,
)
diff --git a/.github/scripts/submit_review.py b/.github/scripts/submit_review.py
index edd8fe9cf18..6db8997b3b9 100644
--- a/.github/scripts/submit_review.py
+++ b/.github/scripts/submit_review.py
@@ -49,9 +49,25 @@ def priority(body):
def validate_submission(value):
if not isinstance(value, dict) or set(value) != {
- "body", "comments", "existing_blocking_comment_ids"
+ "body", "comments", "existing_blocking_comment_ids",
+ "converged", "rounds", "incomplete_reason"
}:
- raise ValueError("Final review requires body, comments and
existing_blocking_comment_ids")
+ raise ValueError(
+ "Final review requires body, comments,
existing_blocking_comment_ids, "
+ "converged, rounds and incomplete_reason"
+ )
+ if type(value["converged"]) is not bool:
+ raise ValueError("converged must be a boolean")
+ if type(value["rounds"]) is not int or not 1 <= value["rounds"] <= 3:
+ raise ValueError("rounds must be an integer from 1 to 3")
+ reason = value["incomplete_reason"]
+ if not isinstance(reason, str):
+ raise ValueError("incomplete_reason must be text")
+ if value["converged"]:
+ if reason.strip():
+ raise ValueError("A converged review cannot have an
incomplete_reason")
+ elif value["rounds"] != 3 or not reason.strip():
+ raise ValueError("A non-converged review requires three rounds and an
incomplete_reason")
if not isinstance(value["body"], str) or not value["body"].strip():
raise ValueError("Final review summary is empty")
if not isinstance(value["comments"], list):
@@ -119,6 +135,15 @@ def review_payload(submission, run, root, remaining):
digest = hashlib.sha256(json.dumps(submission,
sort_keys=True).encode()).hexdigest()
marker = f"<!-- code-review-completion:{run['token']}:{digest} -->"
body = submission["body"]
+ # Render the declared convergence state ourselves and bind it to the same
+ # digest/readback as the findings. A delivered review can still be
incomplete.
+ if submission["converged"]:
+ body += f"\n\nReview converged after {submission['rounds']} round(s)."
+ else:
+ body += (
+ "\n\n**Review did not converge after 3 rounds. This automated
review cannot pass the review check.**"
+ f"\n\n{submission['incomplete_reason']}"
+ )
if ids:
body += "\n\nExisting P0/P1 findings confirmed for this head: " + ",
".join(
f"https://github.com/{run['repository']}/pull/{run['pr_number']}#discussion_r{i}"
@@ -187,7 +212,9 @@ def verify_completion(context, run, remaining, *,
submit=False, submission=None)
# Recheck after the API reads so a moving PR cannot pass an obsolete
review.
check_target(run, remaining)
return {
- "state": "failure" if counts[0] + counts[1] else "success",
+ "state": "failure" if counts[0] + counts[1] or not
submission["converged"] else "success",
+ "converged": submission["converged"], "rounds": submission["rounds"],
+ "incomplete_reason": submission["incomplete_reason"],
"p0": counts[0], "p1": counts[1], "review_id": review["id"],
"review_url": review["html_url"],
}
diff --git a/.github/scripts/test_resolve_code_review_status.py
b/.github/scripts/test_resolve_code_review_status.py
index c9576ab5085..1f1f4f30ce2 100644
--- a/.github/scripts/test_resolve_code_review_status.py
+++ b/.github/scripts/test_resolve_code_review_status.py
@@ -115,6 +115,36 @@ class ResolveCodeReviewStatusTest(unittest.TestCase):
self.assertEqual("failure", result.state)
self.assertIn("P0/P1", result.description)
+ def test_nonconverged_result_blocks_with_an_explicit_description(self) ->
None:
+ incomplete = dict(status(1, source="automated", state="failure"),
+ description="Review did not converge after 3 rounds
for PR #123: P0=0, P1=0.")
+ result = resolve_status([pull(123)], [incomplete], head_sha=HEAD_SHA)
+ self.assertEqual("failure", result.state)
+ self.assertIn("did not converge", result.description)
+ self.assertNotIn("P0/P1", result.description)
+
+ def test_description_does_not_decide_whether_review_passes(self) -> None:
+ for state in ("success", "pending", "failure"):
+ with self.subTest(state=state):
+ source = dict(status(1, source="automated", state=state),
+ description="Review did not converge after 3
rounds")
+ self.assertEqual(state, resolve_status([pull(123)], [source],
head_sha=HEAD_SHA).state)
+
+ def test_incomplete_result_can_be_replaced_or_explicitly_overridden(self)
-> None:
+ incomplete = dict(status(1, source="automated", state="failure"),
+ description="Review did not converge after 3 rounds")
+ for source in ("automated", "local", "skip"):
+ with self.subTest(source=source):
+ result = resolve_status([pull(123)], [incomplete, status(2,
source=source)], head_sha=HEAD_SHA)
+ self.assertEqual("success", result.state)
+
+ def test_incomplete_result_does_not_follow_other_heads_or_bases(self) ->
None:
+ incomplete = dict(status(1, source="automated", state="failure"),
+ description="Review did not converge after 3 rounds")
+ for pulls in ([pull(124)], [pull(123, base=OTHER_BASE_SHA)],
[pull(123, head=OTHER_HEAD_SHA)]):
+ with self.subTest(pulls=pulls):
+ self.assertEqual("pending", resolve_status(pulls,
[incomplete], head_sha=HEAD_SHA).state)
+
def test_local_or_skip_success_can_override_blocking_review(self) -> None:
for source in ("local", "skip"):
with self.subTest(source=source):
diff --git a/.github/scripts/test_review_convergence_workflow.py
b/.github/scripts/test_review_convergence_workflow.py
new file mode 100644
index 00000000000..97953d01b77
--- /dev/null
+++ b/.github/scripts/test_review_convergence_workflow.py
@@ -0,0 +1,117 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import json
+import os
+import re
+import shutil
+import subprocess
+import tempfile
+import textwrap
+import unittest
+from pathlib import Path
+
+from resolve_code_review_status import resolve_status
+from submit_review import validate_submission
+
+
+WORKFLOW = Path(__file__).resolve().parents[1] /
"workflows/code-review-runner.yml"
+
+
+def step_script(name):
+ step = WORKFLOW.read_text().split(f" - name: {name}\n",
1)[1].split("\n - name:", 1)[0]
+ script = textwrap.dedent(step.split(" run: |\n", 1)[1])
+ return re.sub(r"\$\{\{.*?\}\}", "test", script)
+
+
+class ReviewPromptContractTest(unittest.TestCase):
+ def test_prompt_schema_supports_both_delivery_outcomes(self):
+ prompt = WORKFLOW.read_text().split("<<'PROMPT'\n", 1)[1].split("
PROMPT\n", 1)[0]
+ example = next(line.strip() for line in prompt.splitlines() if
line.strip().startswith('{"body":'))
+ submission = json.loads(example.replace("<diff_position>", "1"))
+ validate_submission(submission)
+ validate_submission(dict(submission, converged=False, rounds=3,
+ incomplete_reason="Round 3 still found
candidates."))
+ # Regression guard for the clauses that blocked #67549 and #68499.
+ for obsolete in (
+ "can only be submitted after all subagents return",
+ "MUST remain incomplete until every suspicious point",
+ "Only finish when there are no unresolved suspicious points",
+ "declare the COMPLETE final review",
+ "Submit only after all review completion criteria are satisfied",
+ ):
+ self.assertNotIn(obsolete, prompt)
+ self.assertIn("do not start a fourth round", prompt)
+ self.assertIn("MUST NOT prevent delivery of verified findings", prompt)
+
+
[email protected](shutil.which("jq"), "workflow shell test requires jq")
+class ReviewConvergenceWorkflowTest(unittest.TestCase):
+ def test_verified_result_flows_through_source_and_aggregate_status(self):
+ output_script = step_script("Run automated code
review").split("status=$?\n", 1)[1]
+ source_script = step_script("Sync Code Review check for current head")
+ # Replace the only external mutation with an argument capture. Run the
+ # actual Bash/jq blocks, then feed their source into the real resolver.
+ capture = """gh() {
+ python3 -c 'import json, sys; print(json.dumps(sys.argv[1:]))' "$@"
+}
+"""
+ for converged, p0, p1, expected in (
+ (True, 0, 0, "success"),
+ (True, 1, 0, "failure"),
+ (True, 0, 1, "failure"),
+ (False, 0, 0, "failure"),
+ (False, 0, 1, "failure"),
+ ):
+ with self.subTest(converged=converged, p0=p0, p1=p1),
tempfile.TemporaryDirectory() as tmp:
+ root = Path(tmp)
+ (root / "review-result.json").write_text(json.dumps({
+ "state": expected, "p0": p0, "p1": p1, "converged":
converged,
+ "rounds": 3, "incomplete_reason": "" if converged else
"Coverage incomplete",
+ }))
+ env = {**os.environ, "status": "0", "REVIEW_CONTEXT_DIR": tmp,
+ "GITHUB_OUTPUT": str(root / "output"),
"GITHUB_STEP_SUMMARY": str(root / "summary")}
+ subprocess.run(["bash", "-e", "-c", output_script], env=env,
+ capture_output=True, text=True, check=True)
+ outputs = dict(line.split("=", 1) for line in (root /
"output").read_text().splitlines())
+ self.assertEqual(str(converged).lower(), outputs["converged"])
+ result = subprocess.run(["bash", "-e", "-c", capture +
source_script], env={
+ **env, "BASE_SHA": "b" * 40, "HEAD_SHA": "a" * 40,
+ "PR_NUMBER": "123", "REPO": "apache/doris", "JOB_STATUS":
"success",
+ "REVIEW_CONTEXT_OUTCOME": "success", "REVIEW_OUTCOME":
"success",
+ "REVIEW_STATE": outputs["review_state"], "REVIEW_P0":
outputs["p0"],
+ "REVIEW_P1": outputs["p1"], "REVIEW_CONVERGED":
outputs["converged"],
+ "REVIEW_ROUNDS": outputs["rounds"],
+ }, capture_output=True, text=True, check=True)
+ args = json.loads(result.stdout)
+ posted = dict(args[i + 1].split("=", 1) for i in
range(len(args)) if args[i] == "-f")
+ self.assertEqual(expected, posted["state"])
+ self.assertIn("repos/apache/doris/statuses/" + "a" * 40, args)
+ resolution = resolve_status([{
+ "state": "open", "number": 123, "head": {"sha": "a" * 40},
"base": {"sha": "b" * 40},
+ }], [{"id": 1, **posted}], head_sha="a" * 40)
+ self.assertEqual(expected, resolution.state)
+ if not converged:
+ self.assertIn("did not converge", posted["description"])
+ self.assertIn("did not converge", resolution.description)
+ else:
+ self.assertNotIn("did not converge",
resolution.description)
+
+
+if __name__ == "__main__":
+ unittest.main()
diff --git a/.github/scripts/test_run_review_with_resume.py
b/.github/scripts/test_run_review_with_resume.py
index e58d35d36c7..6651d65b145 100644
--- a/.github/scripts/test_run_review_with_resume.py
+++ b/.github/scripts/test_run_review_with_resume.py
@@ -118,6 +118,7 @@ class ResumeReviewTest(unittest.TestCase):
self.verification = self.enterContext(mock.patch.object(
runner, "verify_completion", return_value={
"state": "success", "p0": 0, "p1": 0, "review_id": 123,
+ "converged": True, "rounds": 1, "incomplete_reason": "",
}
))
@@ -1362,6 +1363,7 @@ else:
mock.patch.object(runner, "check_resume_target"),
mock.patch.object(runner, "verify_completion", return_value={
"state": "success", "p0": 0, "p1": 0, "review_id": 123,
+ "converged": True, "rounds": 1, "incomplete_reason": "",
}),
):
self.assertEqual(0, runner.run_review(args))
diff --git a/.github/scripts/test_submit_review.py
b/.github/scripts/test_submit_review.py
index 702f2ceb558..50dbfed228d 100644
--- a/.github/scripts/test_submit_review.py
+++ b/.github/scripts/test_submit_review.py
@@ -18,6 +18,7 @@
import copy
import io
+import itertools
import json
import subprocess
import tempfile
@@ -53,10 +54,11 @@ class FinalReviewTest(unittest.TestCase):
self.api = self.enterContext(mock.patch.object(submitter, "api",
side_effect=self.fake_api))
self.remaining = lambda: 60
- def submission(self, priority=1):
+ def submission(self, priority=1, *, converged=True, rounds=1):
return {"body": "All checkpoints reviewed.", "comments": [] if
priority is None else [
{"path": "file.py", "position": 12, "body": f"[P{priority}] Fix
the verified issue"}
- ], "existing_blocking_comment_ids": []}
+ ], "existing_blocking_comment_ids": [], "converged": converged,
"rounds": rounds,
+ "incomplete_reason": "" if converged else "Round 3 found another
candidate; coverage is not settled."}
def fake_api(self, path, remaining, *, payload=None, paginated=False):
if path == self.root:
@@ -105,6 +107,77 @@ class FinalReviewTest(unittest.TestCase):
self.assertEqual("success", result["state"])
self.assertEqual("COMMENT", self.posts[-1]["event"])
+ def test_round_limit_delivers_verified_findings_without_passing(self):
+ submission = self.submission(2, converged=False, rounds=3)
+ result = self.submit(submission)
+ self.assertEqual(("failure", 0, 0), (result["state"], result["p0"],
result["p1"]))
+ self.assertFalse(result["converged"])
+ self.assertEqual(3, result["rounds"])
+ self.assertEqual(submission["incomplete_reason"],
result["incomplete_reason"])
+ self.assertEqual(submission["comments"], self.posts[0]["comments"])
+ self.assertEqual("COMMENT", self.posts[0]["event"])
+ self.assertIn("Review did not converge after 3 rounds",
self.posts[0]["body"])
+ self.assertIn(submission["incomplete_reason"], self.posts[0]["body"])
+ self.assertEqual(result, self.verify())
+ self.assertEqual(result, self.submit(submission))
+ self.assertEqual(1, len(self.posts))
+
+ def test_convergence_metadata_is_bound_to_frozen_submission(self):
+ original = self.submission(None, converged=False, rounds=3)
+ self.submit(original)
+ changes = (
+ {"converged": True, "incomplete_reason": ""},
+ {"incomplete_reason": "A different gap"},
+ )
+ for changeset in changes:
+ with self.subTest(changeset=changeset),
self.assertRaisesRegex(ValueError, "already declared"):
+ self.submit(dict(original, **changeset))
+ self.assertEqual(1, len(self.posts))
+
+ def test_convergence_statement_is_verified_by_api_readback(self):
+ self.submit(self.submission(None, converged=False, rounds=3))
+ self.reviews[0]["body"] = self.reviews[0]["body"].replace("did not
converge", "converged")
+ with self.assertRaisesRegex(ValueError, "summary did not match"):
+ self.verify()
+
+ def test_nonconverged_submission_still_checks_live_head(self):
+ self.pr["head"]["sha"] = "c" * 40
+ with self.assertRaisesRegex(ValueError, "stale"):
+ self.submit(self.submission(None, converged=False, rounds=3))
+ self.assertEqual([], self.posts)
+
+ def test_nonconverged_post_with_lost_response_is_not_duplicated(self):
+ submission = self.submission(None, converged=False, rounds=3)
+ def lost_response(path, *args, **kwargs):
+ result = self.fake_api(path, *args, **kwargs)
+ if kwargs.get("payload") is not None:
+ raise subprocess.TimeoutExpired("gh", 30)
+ return result
+ self.api.side_effect = lost_response
+ with self.assertRaises(subprocess.TimeoutExpired):
+ self.submit(submission)
+ self.api.side_effect = self.fake_api
+ self.assertEqual("failure", self.submit(submission)["state"])
+ self.assertEqual(1, len(self.posts))
+
+ def test_convergence_schema_rejects_ambiguous_or_premature_results(self):
+ invalid = [
+ {"converged": "false"}, {"converged": 1}, {"converged": None},
+ {"rounds": True}, {"rounds": 0}, {"rounds": 4}, {"rounds": "3"},
{"rounds": 3.0},
+ {"incomplete_reason": None}, {"incomplete_reason": "still
unresolved"},
+ {"converged": False, "rounds": 3, "incomplete_reason": " "},
+ {"converged": False, "rounds": 2, "incomplete_reason": "not
settled"},
+ ]
+ for changes in invalid:
+ with self.subTest(changes=changes), self.assertRaises(ValueError):
+ self.submit(dict(self.submission(None), **changes))
+ for field in ("converged", "rounds", "incomplete_reason"):
+ value = self.submission(None)
+ del value[field]
+ with self.subTest(missing=field), self.assertRaises(ValueError):
+ self.submit(value)
+ self.assertEqual([], self.posts)
+
def
test_existing_confirmed_blocker_is_counted_without_duplicate_inline(self):
self.existing = [{"id": 101, "body": "[P0] Previously reported
blocker"}]
submission = self.submission(None)
@@ -235,11 +308,11 @@ class FinalReviewTest(unittest.TestCase):
self.submit()
self.assertEqual(1, len(self.posts))
- def
test_capacity_after_submission_completes_execution_and_gates_findings(self):
+ def
test_delivery_completes_execution_and_gates_findings_and_convergence(self):
# Reproduce #66227: final write/readback, then capacity (also on the
last
# allowed attempt). Run the real verifier and SHA-wide status resolver.
- for level in (None, 0, 1, 2):
- with self.subTest(priority=level), tempfile.TemporaryDirectory()
as tmp:
+ for level, converged, capacity in itertools.product((None, 0, 1, 2),
(True, False), (True, False)):
+ with self.subTest(priority=level, converged=converged,
capacity=capacity), tempfile.TemporaryDirectory() as tmp:
context = Path(tmp)
(context / "codex_goal_prompt.txt").write_text("Review the PR")
args = SimpleNamespace(context_dir=context, cwd=context, **{
@@ -250,12 +323,13 @@ class FinalReviewTest(unittest.TestCase):
def attempt(command, events, stderr, timeout, reaper=None,
watchdog=None):
run = json.loads((context /
submitter.RUN_FILE).read_text())
submitter.verify_completion(context, run, self.remaining,
submit=True,
-
submission=self.submission(level))
+
submission=self.submission(level, converged=converged, rounds=3))
events.write_text(json.dumps({"type": "thread.started",
"thread_id":
"0199a213-81c0-7800-8aa1-bbab2a035a53"}) + "\n" +
json.dumps({
- "type": "turn.failed", "error": {"message":
runner.CAPACITY_MESSAGE}}) + "\n")
+ "type": "turn.failed" if capacity else
"turn.completed",
+ "error": {"message": runner.CAPACITY_MESSAGE} if
capacity else {}}) + "\n")
stderr.write_text("")
- return 1
+ return 1 if capacity else 0
with mock.patch.object(runner, "run_attempt",
side_effect=attempt), \
mock.patch.object(runner, "RETRY_DELAYS", ()), \
@@ -266,8 +340,13 @@ class FinalReviewTest(unittest.TestCase):
guard.assert_not_called()
sleep.assert_not_called()
result = json.loads((context /
submitter.RESULT_FILE).read_text())
- self.assertTrue(result["recovered_after_capacity"])
- state = "failure" if level in (0, 1) else "success"
+ self.assertEqual(capacity, result["recovered_after_capacity"])
+ self.assertEqual(converged, result["converged"])
+ self.assertEqual(3, result["rounds"])
+ completion = runner.read_events(context /
"codex-events.jsonl")[-1]
+ self.assertEqual("review.completed", completion["type"])
+ self.assertEqual(converged, completion["converged"])
+ state = "failure" if level in (0, 1) or not converged else
"success"
self.assertEqual(state, result["state"])
resolution = resolve_status([self.pr], [{"id": 1, "state":
result["state"],
"context":
f"code-review/source/automated/pr-123/base-{self.run['base_sha']}"}],
diff --git a/.github/workflows/code-review-runner.yml
b/.github/workflows/code-review-runner.yml
index dc8826ac6cc..fc227900abc 100644
--- a/.github/workflows/code-review-runner.yml
+++ b/.github/workflows/code-review-runner.yml
@@ -690,8 +690,8 @@ jobs:
This environment is only used for review operations, so do not
attempt any builds or code modifications. However, temporary scripts you use
for review purposes are exceptions.
You MUST NOT attempt to access any files outside the current
directory. and you DO NOT need to. But this does not prevent you from normally
using any skill or web fetch tools.
You can comment on the pull request.
- This review task is executed in Codex goal mode. Subsequent prompts
and skill indicate all the review and commenting actions you need to perform.
The current round can only be concluded after completing the main agent risk
scan, the normal full-review subagent review for each round, and the
risk-focused subagent scan (if applicable). The final GitHub review can only be
submitted after all subagents return NO_NEW_VALUABLE_FINDINGS, and all
candidate points from the subagents hav [...]
- In addition, the number of iterations shall not exceed 3 rounds. If
new subagents identify valuable candidate points after 3 rounds, the main agent
must indicate in the final summary that the review is incomplete.
+ This review task is executed in Codex goal mode. The goal is to
deliver one verified GitHub review within at most 3 review rounds, with an
explicit convergence result. Delivery and convergence are separate: a review
that reaches the round limit MUST still deliver its verified findings and
describe its remaining limits. Follow this bounded delivery contract for
stopping and submission.
+ A review is converged only when all assigned subagents in the latest
round return NO_NEW_VALUABLE_FINDINGS, the required coverage is complete, and
every candidate and risk focus has been verified and accepted, deduplicated, or
dismissed with evidence. Otherwise continue until round 3. At round 3, perform
the final sweep and submit with converged=false if any of these conditions
still fail; do not start a fourth round, wait for an override, or mark the goal
blocked solely becaus [...]
Context:
- Repository: PLACEHOLDER_REPO
@@ -715,11 +715,11 @@ jobs:
The active review goal's progress tracking MUST include, and must
stay current throughout the review:
1. Read the review prompt, code-review skill, required AGENTS.md
files, existing review threads, user focus, changed-file list, and the shared
subagent review ledger.
2. Perform a main-agent initial risk scan before spawning review
subagents. You must thoroughly read all the changes in this PR and understand
all the involved mechanisms, pointing out: if there is a problem with this PR,
where is the problem most likely to be? Or are there any points you suspect to
be risky? write the result under `## Main Initial Risk Scan` in the shared
ledger.
- 3. Based on the requirements of the code-review skill regarding the
coverage points of the review spawn 1-3 subagents (depends on the complexity of
the PR), each focusing on certain aspects. The sum of their focus points must
fully cover the entire content of PR, as well as the points required in skill
and any additional review points you consider necessary. They must complete the
review of their own aspects and identify all possible issues before they can
finish their work. Yo [...]
+ 3. Based on the requirements of the code-review skill regarding the
coverage points of the review spawn 1-3 subagents (depends on the complexity of
the PR), each focusing on certain aspects. The sum of their focus points must
fully cover the entire content of PR, as well as the points required in skill
and any additional review points you consider necessary. Each subagent must
investigate its assigned scope within the bounded review process, report every
substantiated finding, [...]
4. If the main initial risk scan identifies one or more suspicious
mechanisms, spawn additional risk-focused subagent(s), separate from the normal
complete-review subagents, to specially investigate those specific mechanisms
and their upstream/downstream interactions.
- 5. Read the shared ledger after every subagent result, then
independently verify, deduplicate, accept, or dismiss every candidate in the
main merged section. The status of each candidate must be clarified after this
stage.
- 6. If all subagents return `NO_NEW_VALUABLE_FINDINGS`, or if this
loop has already executed 3 rounds, conduct the necessary review to ensure that
all current candidate points have been verified, deduplicated, accepted, or
rejected, and then submit the final GitHub comment. Otherwise, continue with
the next round of the same review process from the beginning.
- 7. Submit the final GitHub review and verify that all accepted
comments landed before marking the goal complete. You must address all
checkpoints required by the skill, user concerns (if any), and indicate the
status of the review completion.
+ 5. Read the shared ledger after every subagent result, independently
verify and deduplicate candidates, and classify each as accepted, already
covered, dismissed with evidence, or unresolved with a concrete remaining
question. Keep unresolved candidates out of the inline findings until verified.
+ 6. After each round, evaluate the convergence conditions above. If
they hold, or if 3 rounds have executed, perform the final sweep and deliver
the review using the trusted submission helper. At the round limit, publish all
verified findings even if the review did not converge, and explicitly describe
unresolved candidates or coverage limits. Otherwise continue the next round
without resetting the shared ledger or round count.
+ 7. Submit the GitHub review and verify that all accepted findings
landed before marking the bounded delivery goal complete. Address the required
skill checkpoints and user concerns, and report convergence separately. A
verified non-converged delivery completes this execution while leaving the
review check blocked.
Subagent Constraints:
- Every subagent appends only to its assigned ledger section, while
reading the whole ledger to avoid duplicate candidates.
@@ -729,13 +729,13 @@ jobs:
- Before returning, a further thought must be made—what problem, if
any, might I have missed just now, and where? Then a thorough recheck should be
conducted to confirm whether this potential issue is real and whether it needs
to be reported.
Main-agent Constraints:
- - The active review goal MUST remain incomplete until every
suspicious point found during review has a clear conclusion: submitted as an
inline issue, dismissed as already covered by existing review context, or
dismissed with concrete code evidence explaining why it is not a bug.
+ - Before delivery, every suspicious point must have an explicit
disposition in the ledger: a verified finding, already covered by an existing
review, dismissed with concrete evidence, or unresolved. At round 3, disclose
unresolved points as remaining questions in the summary and incomplete_reason;
they prevent convergence but MUST NOT prevent delivery of verified findings.
- The main initial risk scan MUST be written under `## Main Initial
Risk Scan` in the shared ledger before any subagent is spawned. For every risk
focus item, include: ID, changed files/lines involved, related mechanisms to
inspect, why it is suspicious, required upstream/downstream files or paths, and
the exact question the risk-focused subagent must answer.
- The shared subagent review ledger is
PLACEHOLDER_CONTEXT_DIR/subagent_review_findings.md. Before spawning any
subagent, the main agent MUST read this ledger. Every subagent prompt MUST
include this ledger path and the exact ledger section assigned to that subagent.
- The main agent must read the shared ledger after each subagent
result, merge duplicate candidates into the main merged section, update
candidate statuses, and keep a proposed final comment set in the main-owned
ledger sections. It must also update the status of every main risk focus item.
- - Before submitting the final review, do one explicit final sweep
over the changed-file list and your unresolved candidate list. Only finish when
there are no unresolved suspicious points and all possible substantiated bugs
have been pointed out in the GitHub review. At this stage, you can conduct
final research on any part you still have doubts about or which may not have
been fully investigated, ensuring that all potential issues have been
thoroughly investigated and reported.
+ - Before submitting, do one final sweep over the changed-file list,
candidate dispositions, and risk focus items. Confirm the evidence and anchors
for every accepted finding and record any remaining coverage gaps. This sweep
must respect the three-round limit. Remaining uncertainty at the cap sets
converged=false and must be disclosed; do not claim it has been resolved or
withhold the verified review.
- Any agent MUST NOT stop after finding the first blocking issue. Keep
reviewing changed files, related control flow, tests, and parallel/special-case
paths until all plausible correctness, lifecycle, configuration, compatibility,
performance, and coverage bugs have been investigated and every bug you can
substantiate has been reported.
+ Any agent MUST NOT stop after finding the first blocking issue.
Within the three-round limit, keep reviewing changed files, related control
flow, tests, and parallel/special-case paths. Publish every verified bug and
explicitly disclose remaining uncertainty at the limit.
Before inspecting the PR diff or related code, you MUST read the
contents of every AGENTS.md file listed below. These paths are computed from
the PR changed file ancestors in this checkout. Searching for or listing paths
is not sufficient; read each listed file directly.
Required AGENTS.md files for this PR:
@@ -747,15 +747,15 @@ jobs:
In addition, you can perform any desired review operations to
observe suspicious code and details in order to identify issues as much as
possible.
## Final response format
- - After completing the review, you MUST provide a final summary
opinion based on the rules defined in AGENTS.md and the code-review skill. The
summary must include conclusions for each applicable critical checkpoint.
- - After the required convergence rounds and final sweep, declare the
COMPLETE final review in a JSON file with exactly these fields:
- {"body":"<comprehensive
summary>","comments":[{"path":"<file>","position":<diff_position>,"body":"[P1]
<finding title and explanation>"}],"existing_blocking_comment_ids":[]}
+ - On delivery, provide a final summary opinion addressing every
applicable critical checkpoint, user concerns, and any remaining coverage
limits. State whether the review converged.
+ - After convergence or the third round and final sweep, declare the
full set of verified findings for this invocation in a JSON file with exactly
these fields:
+ {"body":"<comprehensive
summary>","comments":[{"path":"<file>","position":<diff_position>,"body":"[P1]
<finding title and
explanation>"}],"existing_blocking_comment_ids":[],"converged":true,"rounds":1,"incomplete_reason":""}
- Every new finding must be inline and start with [P0], [P1], [P2]
or [P3]. Suggested changes are welcome for precise, self-contained fixes.
- Include ALL accepted new findings in comments. Use an empty array
when there are none. In existing_blocking_comment_ids, list the numeric IDs of
every already-reported P0/P1 inline finding you independently confirmed still
applies to this head. Do not duplicate those findings. Explain their
disposition in the summary; use an empty array only if none still apply.
- - Submit only after all review completion criteria are satisfied,
using the trusted helper: python3 "$RUNNER_TEMP/submit_review.py" --context-dir
PLACEHOLDER_CONTEXT_DIR --input-file <json_file>
- - The helper pins the head, adds this invocation's completion
marker, submits the review and verifies the entire summary and inline comment
set. P0/P1 results request changes; other completed reviews are comments. This
is the ONLY final GitHub submission path. Do not submit reviews or inline
comments directly with gh.
+ - Set rounds to the actual total (1 through 3), never reset it on
resume. Set converged=true only when all convergence conditions hold and use an
empty incomplete_reason. At the cap without convergence, set converged=false,
rounds=3, and a nonempty incomplete_reason explaining which conditions failed,
remaining questions, and uncovered scope. Deliver in either case using the
trusted helper: python3 "$RUNNER_TEMP/submit_review.py" --context-dir
PLACEHOLDER_CONTEXT_DIR --input-fi [...]
+ - The helper pins the head, adds this invocation's delivery marker
and convergence statement, and verifies the summary and inline set. P0/P1
results request changes; other deliveries are comments. A non-converged
delivery blocks the review check even with zero P0/P1. This is the ONLY GitHub
submission path; do not submit reviews or inline comments directly with gh.
- If the helper reports an error, inspect it and verify the outcome
before proceeding. Do not change or delete its review-run.json or
review-submission.json files. A retry with the identical JSON reads back a
submitted review instead of posting it again.
- - Mark the goal complete only after the helper reports the verified
review ID. A completed review with P0/P1 is still a completed execution; the
workflow applies the separate blocking verdict.
+ - Mark the bounded delivery goal complete only after the helper
reports the verified review ID, including a non-converged delivery after round
3. The workflow applies the separate blocking verdict. A changed PR head, an
unverified submission, or a helper error still prevents successful delivery.
PROMPT
sed -i "s|PLACEHOLDER_REPO|${REPO}|g"
"$REVIEW_CONTEXT_DIR/review_prompt.txt"
@@ -970,7 +970,7 @@ jobs:
# Execution succeeded even when verified findings block the PR check.
# This result is written only after the runner's own API readback.
jq -er 'select(.state == "success" or .state == "failure") |
- "review_state=\(.state)\np0=\(.p0)\np1=\(.p1)"' \
+
"review_state=\(.state)\np0=\(.p0)\np1=\(.p1)\nconverged=\(.converged)\nrounds=\(.rounds)"'
\
"$REVIEW_CONTEXT_DIR/review-result.json" >> "$GITHUB_OUTPUT"
- name: Record invalid Codex auth
@@ -1111,6 +1111,8 @@ jobs:
REVIEW_STATE: ${{ steps.review.outputs.review_state }}
REVIEW_P0: ${{ steps.review.outputs.p0 }}
REVIEW_P1: ${{ steps.review.outputs.p1 }}
+ REVIEW_CONVERGED: ${{ steps.review.outputs.converged }}
+ REVIEW_ROUNDS: ${{ steps.review.outputs.rounds }}
run: |
state="pending"
summary="Automated review did not pass for PR #${PR_NUMBER} at
${BASE_SHA:0:12}."
@@ -1122,6 +1124,9 @@ jobs:
success|failure)
state="$REVIEW_STATE"
summary="Review completed for PR #${PR_NUMBER}:
P0=${REVIEW_P0}, P1=${REVIEW_P1}."
+ if [ "$REVIEW_CONVERGED" = "false" ]; then
+ summary="Review did not converge after ${REVIEW_ROUNDS}
rounds for PR #${PR_NUMBER}: P0=${REVIEW_P0}, P1=${REVIEW_P1}."
+ fi
;;
*) echo "::error::Missing verified review verdict"; exit 1 ;;
esac
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
