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 e498526e6e2 [fix](ci) Preserve failed review submission diagnostics
(#69154)
e498526e6e2 is described below
commit e498526e6e292ee137292a5c0f188380dbfb13d4
Author: shuke <[email protected]>
AuthorDate: Fri Oct 16 18:41:22 2026 +0800
[fix](ci) Preserve failed review submission diagnostics (#69154)
### What problem does this PR solve?
When a GitHub review POST fails, the helper currently discards captured
stdout/stderr. Its persisted intent correctly prevents reposting, but
subsequent verification reports only that no final review was found. The
workflow archives only rejected candidates, leaving the attempted
submission and original API failure unavailable.
This change records credential-redacted API output, endpoint/method,
exit status and timeout details for failed commands and invalid JSON
responses. It retains earlier diagnostics across readback attempts,
includes the original API error in unverified-delivery failures, and
uploads the submission intent, rejected candidate and API diagnostics as
an explicit three-file failure artifact.
Exact GitHub readback, submission ownership, stale-head checks and the
no-repost guard are unchanged. The tests inject HTTP 422 and timeout
responses; they do not assert that a historical production failure was
HTTP 422.
---
.github/scripts/submit_review.py | 85 +++++++++++++++--
.github/scripts/test_submit_review.py | 154 ++++++++++++++++++++++++++++++-
.github/workflows/code-review-runner.yml | 11 ++-
3 files changed, 235 insertions(+), 15 deletions(-)
diff --git a/.github/scripts/submit_review.py b/.github/scripts/submit_review.py
index 358c05a95d4..036d5387c71 100644
--- a/.github/scripts/submit_review.py
+++ b/.github/scripts/submit_review.py
@@ -21,15 +21,19 @@
import argparse
import hashlib
import json
+import os
import re
import subprocess
+import sys
import time
from collections import Counter
+from datetime import datetime, timezone
from pathlib import Path
SUBMISSION_FILE = "review-submission.json"
REJECTION_FILE = "review-submission-rejected.json"
+API_ERRORS_FILE = "review-api-errors.jsonl"
RUN_FILE = "review-run.json"
RESULT_FILE = "review-result.json"
PRIORITY = re.compile(r"^\[P([0-3])\]\s+\S")
@@ -91,20 +95,68 @@ def validate_submission(value):
return value
+def diagnostic_text(value):
+ if isinstance(value, bytes):
+ value = value.decode("utf-8", errors="replace")
+ value = value or ""
+ # These are the credentials consumed by gh. Artifacts do not inherit the
+ # GitHub Actions log masker, so remove them before persisting API output.
+ for name in ("GH_TOKEN", "GITHUB_TOKEN", "GH_ENTERPRISE_TOKEN",
"GITHUB_ENTERPRISE_TOKEN"):
+ token = os.environ.get(name)
+ if token:
+ value = value.replace(token, "[REDACTED]")
+ value = re.sub(r"\b(?:gh[pousr]_[A-Za-z0-9]+|github_pat_[A-Za-z0-9_]+)\b",
"[REDACTED]", value)
+ return value[:65536] + ("\n[truncated after 65536 characters]" if
len(value) > 65536 else "")
+
+
+class GitHubAPIError(ValueError):
+ def __init__(self, path, method, error, stdout=None, stderr=None):
+ self.details = {
+ "method": method, "path": path, "error_type": type(error).__name__,
+ "returncode": getattr(error, "returncode", None),
+ "timeout_seconds": getattr(error, "timeout", None),
+ "stdout": diagnostic_text(stdout), "stderr":
diagnostic_text(stderr),
+ }
+ message = (
+ f"GitHub API {method} {path} failed ({type(error).__name__}, "
+ f"exit={self.details['returncode']},
timeout={self.details['timeout_seconds']}); "
+ f"stderr={json.dumps(self.details['stderr'][:1000])}; "
+ f"stdout={json.dumps(self.details['stdout'][:1000])}"
+ )
+ self.details["message"] = message
+ super().__init__(message)
+
+
def api(path, remaining, *, payload=None, paginated=False):
command = ["gh", "api", path]
if paginated:
command += ["--paginate", "--slurp"]
if payload is not None:
command += ["--method", "POST", "--input", "-"]
- result = subprocess.run(
- command, input=json.dumps(payload) if payload is not None else None,
- capture_output=True, text=True, check=True, timeout=min(30,
remaining()),
- )
- value = json.loads(result.stdout)
+ method = "POST" if payload is not None else "GET"
+ try:
+ result = subprocess.run(
+ command, input=json.dumps(payload) if payload is not None else
None,
+ capture_output=True, text=True, check=True, timeout=min(30,
remaining()),
+ )
+ except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as error:
+ raise GitHubAPIError(path, method, error, error.stdout, error.stderr)
from None
+ try:
+ value = json.loads(result.stdout)
+ except json.JSONDecodeError as error:
+ raise GitHubAPIError(path, method, error, result.stdout,
result.stderr) from None
return [item for page in value for item in page] if paginated else value
+def with_api_failure(context, reason):
+ diagnostics = context / API_ERRORS_FILE
+ if diagnostics.exists():
+ with diagnostics.open() as handle:
+ first = json.loads(next(handle))
+ reason += f"; first recorded API failure: {first['message']} (see
{API_ERRORS_FILE})"
+ return reason
+
+
def check_target(run, remaining):
root = f"repos/{run['repository']}/pulls/{run['pr_number']}"
pr = api(root, remaining)
@@ -160,6 +212,21 @@ def review_payload(submission, run, root, remaining):
def verify_completion(context, run, remaining, *, submit=False,
submission=None):
+ try:
+ return _verify_completion(context, run, remaining, submit=submit,
submission=submission)
+ except GitHubAPIError as error:
+ diagnostic = dict(error.details,
recorded_at=datetime.now(timezone.utc).isoformat(),
+ repository=run["repository"],
pr_number=run["pr_number"],
+ head_sha=run["head_sha"], base_sha=run["base_sha"])
+ # Append rather than replace: later readback failures must not erase
the
+ # original POST response. Neither this log nor the intent proves
delivery.
+ with (context / API_ERRORS_FILE).open("a") as handle:
+ handle.write(json.dumps(diagnostic) + "\n")
+ print(f"Review API diagnostic: {json.dumps(diagnostic)}",
file=sys.stderr, flush=True)
+ raise
+
+
+def _verify_completion(context, run, remaining, *, submit=False,
submission=None):
"""Never infer completion from an unowned review or a local result file."""
path = context / SUBMISSION_FILE
if submission is None:
@@ -186,7 +253,9 @@ def verify_completion(context, run, remaining, *,
submit=False, submission=None)
write_json(context / REJECTION_FILE, {"reason": reason,
"submission": submission})
raise ValueError(reason)
if path.exists():
- raise ValueError("Final submission was attempted but cannot be
verified; refusing to repost")
+ raise ValueError(with_api_failure(
+ context, "Final submission was attempted but cannot be
verified; refusing to repost"
+ ))
# Persist the complete intent before POST: if the response is lost, the
# runner can read back this exact review without repeating the write.
# Exclusive creation also prevents two accidental simultaneous helper
@@ -197,7 +266,9 @@ def verify_completion(context, run, remaining, *,
submit=False, submission=None)
matching = [r for r in api(f"{root}/reviews", remaining,
paginated=True)
if marker in (r.get("body") or "")]
if len(matching) != 1:
- raise ValueError("Could not verify exactly one final review from this
invocation")
+ raise ValueError(with_api_failure(
+ context, "Could not verify exactly one final review from this
invocation"
+ ))
review = matching[0]
expected_state = "CHANGES_REQUESTED" if payload["event"] ==
"REQUEST_CHANGES" else "COMMENTED"
if (review.get("user", {}).get("login") != "github-actions[bot]"
diff --git a/.github/scripts/test_submit_review.py
b/.github/scripts/test_submit_review.py
index 546bfecede6..42735555b29 100644
--- a/.github/scripts/test_submit_review.py
+++ b/.github/scripts/test_submit_review.py
@@ -52,6 +52,7 @@ class FinalReviewTest(unittest.TestCase):
self.comments = []
self.existing = []
self.posts = []
+ self.real_api = submitter.api
self.api = self.enterContext(mock.patch.object(submitter, "api",
side_effect=self.fake_api))
self.remaining = lambda: 60
@@ -90,6 +91,117 @@ class FinalReviewTest(unittest.TestCase):
def verify(self):
return submitter.verify_completion(self.context, self.run,
self.remaining)
+ def gh_response(self, command, **kwargs):
+ payload = json.loads(kwargs["input"]) if kwargs.get("input") is not
None else None
+ paginated = "--paginate" in command
+ value = self.fake_api(command[2], self.remaining, payload=payload,
paginated=paginated)
+ return SimpleNamespace(stdout=json.dumps([value] if paginated else
value), stderr="")
+
+ def test_failed_post_retains_response_candidate_and_no_repost_guard(self):
+ self.api.side_effect = self.real_api
+ rejection = '{"message":"Validation
Failed","errors":[{"field":"position"}]}'
+ post_attempts = []
+
+ def reject_post(command, **kwargs):
+ if kwargs.get("input") is not None:
+ post_attempts.append(json.loads(kwargs["input"]))
+ raise subprocess.CalledProcessError(1, command,
output=rejection,
+ stderr="gh: Validation
Failed (HTTP 422)")
+ return self.gh_response(command, **kwargs)
+
+ with mock.patch.object(submitter.subprocess, "run",
side_effect=reject_post), \
+ redirect_stderr(io.StringIO()) as logs:
+ with self.assertRaisesRegex(ValueError, "HTTP 422"):
+ self.submit()
+ for call, reason in ((self.submit, "refusing to repost"),
(self.verify, "exactly one")):
+ with self.assertRaisesRegex(ValueError, reason) as failure:
+ call()
+ self.assertIn("HTTP 422", str(failure.exception))
+ self.assertIn("Validation Failed", str(failure.exception))
+ self.assertEqual(1, len(post_attempts))
+ self.assertEqual(self.submission(), json.loads((self.context /
submitter.SUBMISSION_FILE).read_text()))
+ diagnostic = json.loads((self.context /
submitter.API_ERRORS_FILE).read_text())
+ self.assertEqual(("POST", self.root + "/reviews", 1),
+ (diagnostic["method"], diagnostic["path"],
diagnostic["returncode"]))
+ self.assertEqual(rejection, diagnostic["stdout"])
+ self.assertEqual(self.run["head_sha"], diagnostic["head_sha"])
+ self.assertIn("HTTP 422", logs.getvalue())
+ self.assertFalse((self.context / submitter.RESULT_FILE).exists())
+
+ def
test_later_api_failure_does_not_overwrite_original_post_diagnostic(self):
+ self.api.side_effect = self.real_api
+
+ def reject_post(command, **kwargs):
+ if kwargs.get("input") is not None:
+ raise subprocess.CalledProcessError(1, command, stderr="HTTP
422: position invalid")
+ return self.gh_response(command, **kwargs)
+
+ with mock.patch.object(submitter.subprocess, "run",
side_effect=reject_post) as gh, \
+ redirect_stderr(io.StringIO()):
+ with self.assertRaisesRegex(ValueError, "HTTP 422"):
+ self.submit()
+ gh.side_effect = subprocess.CalledProcessError(1, "gh",
stderr="HTTP 502: read unavailable")
+ with self.assertRaisesRegex(ValueError, "HTTP 502"):
+ self.verify()
+ gh.side_effect = self.gh_response
+ with self.assertRaisesRegex(ValueError, "first recorded API
failure.*HTTP 422"):
+ self.verify()
+ records = [json.loads(line) for line in (self.context /
submitter.API_ERRORS_FILE).read_text().splitlines()]
+ self.assertEqual(["POST", "GET"], [record["method"] for record in
records])
+
+ def test_timed_out_post_still_recovers_by_exact_readback(self):
+ self.api.side_effect = self.real_api
+
+ def lose_post_response(command, **kwargs):
+ result = self.gh_response(command, **kwargs)
+ if kwargs.get("input") is not None:
+ raise subprocess.TimeoutExpired(command, 30, output=b'partial
response', stderr=b'timed out')
+ return result
+
+ with mock.patch.object(submitter.subprocess, "run",
side_effect=lose_post_response), \
+ redirect_stderr(io.StringIO()):
+ with self.assertRaisesRegex(ValueError, "TimeoutExpired"):
+ self.submit()
+ self.assertEqual("failure", self.submit()["state"])
+ self.assertEqual("failure", self.verify()["state"])
+ self.assertEqual(1, len(self.posts))
+ diagnostic = json.loads((self.context /
submitter.API_ERRORS_FILE).read_text())
+ self.assertEqual((30, "partial response", "timed out"),
+ (diagnostic["timeout_seconds"], diagnostic["stdout"],
diagnostic["stderr"]))
+
+ def test_runner_failure_keeps_original_post_error(self):
+ (self.context / "codex_goal_prompt.txt").write_text("Review the PR")
+ args = SimpleNamespace(context_dir=self.context, cwd=self.context, **{
+ k: self.run[k] for k in ("repository", "pr_number", "head_sha",
"base_sha")
+ }, model="gpt-6-sol", effort="xhigh", budget_seconds=60)
+ self.api.side_effect = self.real_api
+
+ def reject_post(command, **kwargs):
+ if kwargs.get("input") is not None:
+ raise subprocess.CalledProcessError(1, command, stderr="gh:
Validation Failed (HTTP 422)")
+ return self.gh_response(command, **kwargs)
+
+ def attempt(command, events, stderr, timeout, reaper=None,
watchdog=None):
+ run = json.loads((self.context / submitter.RUN_FILE).read_text())
+ with self.assertRaisesRegex(ValueError, "HTTP 422"):
+ submitter.verify_completion(self.context, run, self.remaining,
submit=True,
+ submission=self.submission())
+ events.write_text(json.dumps({"type": "thread.started",
"thread_id":
+ "0199a213-81c0-7800-8aa1-bbab2a035a53"}) + "\n" +
+ json.dumps({"type": "turn.completed"}) + "\n")
+ stderr.write_text("")
+ return 0
+
+ with mock.patch.object(runner, "run_attempt", side_effect=attempt), \
+ mock.patch.object(runner.HeadWatchdog, "check"), \
+ mock.patch.object(submitter.subprocess, "run",
side_effect=reject_post), \
+ redirect_stderr(io.StringIO()):
+ self.assertEqual(1, runner.run_review(args))
+ error = runner.read_events(self.context /
"codex-events.jsonl")[-1]["error"]["message"]
+ self.assertIn("HTTP 422", error)
+ self.assertIn(submitter.API_ERRORS_FILE, error)
+ self.assertFalse((self.context / submitter.RESULT_FILE).exists())
+
def test_submits_once_and_verifies_same_complete_review(self):
result = self.submit()
self.assertEqual("failure", result["state"])
@@ -252,7 +364,7 @@ class FinalReviewTest(unittest.TestCase):
k: self.run[k] for k in ("repository", "pr_number", "head_sha",
"base_sha")
}, model="gpt-6-sol", effort="xhigh", budget_seconds=60)
- def attempt(command, events, stderr, timeout, reaper=None):
+ def attempt(command, events, stderr, timeout, reaper=None,
watchdog=None):
run = json.loads((self.context / submitter.RUN_FILE).read_text())
self.reviews = [{"user": {"login": "github-actions[bot]"},
"commit_id": run["head_sha"], "submitted_at":
run["started_at"]}]
@@ -267,6 +379,7 @@ class FinalReviewTest(unittest.TestCase):
return 0
with mock.patch.object(runner, "run_attempt", side_effect=attempt), \
+ mock.patch.object(runner.HeadWatchdog, "check"), \
mock.patch.object(runner, "check_resume_target") as guard, \
redirect_stderr(io.StringIO()):
self.assertEqual(1, runner.run_review(args))
@@ -442,7 +555,7 @@ class FinalReviewTest(unittest.TestCase):
class ReviewWorkflowTest(unittest.TestCase):
- def test_queue_is_per_pr_and_rejected_artifact_is_explicit(self):
+ def test_queue_is_per_pr_and_failure_artifact_is_explicit(self):
workflow = (Path(__file__).resolve().parents[1] /
"workflows/code-review-runner.yml").read_text()
review_job = workflow.split(" code-review:\n", 1)[1].split("
aggregate-status:", 1)[0]
concurrency = re.search(r"^ concurrency:\n((?: .*\n)+)",
review_job, re.MULTILINE)
@@ -452,11 +565,14 @@ class ReviewWorkflowTest(unittest.TestCase):
self.assertIn("queue: max", concurrency[1])
self.assertNotIn("sha", concurrency[1])
self.assertNotRegex(workflow, r"(?m)^concurrency:")
- artifact = review_job.split(" - name: Preserve rejected final
review\n", 1)[1]
+ artifact = review_job.split(" - name: Preserve failed final
review diagnostics\n", 1)[1]
artifact = artifact.split(" - name:", 1)[0]
self.assertIn("always()", artifact)
self.assertIn("steps.review.outcome == 'failure'", artifact)
- self.assertIn("path: ${{ env.REVIEW_CONTEXT_DIR }}/" +
submitter.REJECTION_FILE, artifact)
+ paths = artifact.split(" path: |\n", 1)[1].split("
#", 1)[0]
+ self.assertEqual({"${{ env.REVIEW_CONTEXT_DIR }}/" + name for name in (
+ submitter.SUBMISSION_FILE, submitter.REJECTION_FILE,
submitter.API_ERRORS_FILE
+ )}, {line.strip() for line in paths.splitlines()})
self.assertIn("include-hidden-files: true", artifact)
self.assertIn("if-no-files-found: ignore", artifact)
@@ -471,6 +587,36 @@ class GitHubAPITest(unittest.TestCase):
self.assertIn("--paginate", command.call_args.args[0])
self.assertIn("--slurp", command.call_args.args[0])
+ def test_api_error_redacts_credentials_before_logging_or_archiving(self):
+ secret = "private-token-from-environment"
+ error = subprocess.CalledProcessError(1, "gh",
output='{"message":"invalid ghp_exampleToken"}',
+ stderr=f"HTTP 403: token={secret}")
+ with mock.patch.dict(submitter.os.environ, {"GH_TOKEN": secret}), \
+ mock.patch.object(submitter.subprocess, "run",
side_effect=error), \
+ self.assertRaises(submitter.GitHubAPIError) as failure:
+ submitter.api("reviews", lambda: 60, payload={"body": "not a
credential"})
+ text = json.dumps(failure.exception.details) + str(failure.exception)
+ self.assertNotIn(secret, text)
+ self.assertNotIn("ghp_exampleToken", text)
+ self.assertIn("[REDACTED]", text)
+ self.assertIn("HTTP 403", text)
+ self.assertNotIn("not a credential", text)
+
+ def test_invalid_json_retains_successful_command_output(self):
+ with mock.patch.object(submitter.subprocess, "run",
return_value=SimpleNamespace(
+ stdout="incomplete response", stderr="diagnostic"
+ )), self.assertRaises(submitter.GitHubAPIError) as failure:
+ submitter.api("reviews", lambda: 7)
+ self.assertEqual("JSONDecodeError",
failure.exception.details["error_type"])
+ self.assertEqual("incomplete response",
failure.exception.details["stdout"])
+
+ def test_diagnostics_bound_large_output_after_redaction(self):
+ output = "ghp_secret " + "x" * 70000
+ result = submitter.diagnostic_text(output.encode())
+ self.assertNotIn("ghp_secret", result)
+ self.assertLess(len(result), 65600)
+ self.assertTrue(result.endswith("[truncated after 65536 characters]"))
+
if __name__ == "__main__":
unittest.main()
diff --git a/.github/workflows/code-review-runner.yml
b/.github/workflows/code-review-runner.yml
index 231a7a01fce..3f61b66a669 100644
--- a/.github/workflows/code-review-runner.yml
+++ b/.github/workflows/code-review-runner.yml
@@ -979,15 +979,18 @@ jobs:
"review_state=\(.state)\np0=\(.p0)\np1=\(.p1)\nconverged=\(.converged)\nrounds=\(.rounds)"'
\
"$REVIEW_CONTEXT_DIR/review-result.json" >> "$GITHUB_OUTPUT"
- - name: Preserve rejected final review
+ - name: Preserve failed final review diagnostics
if: ${{ always() && steps.review_context.outcome == 'success' &&
steps.review.outcome == 'failure' }}
continue-on-error: true
timeout-minutes: 2
uses: actions/upload-artifact@v4
with:
- name: rejected-code-review-${{ github.run_id }}-${{
github.run_attempt }}
- path: ${{ env.REVIEW_CONTEXT_DIR }}/review-submission-rejected.json
- # The context directory is hidden; include only the explicit JSON
above.
+ name: failed-code-review-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: |
+ ${{ env.REVIEW_CONTEXT_DIR }}/review-submission.json
+ ${{ env.REVIEW_CONTEXT_DIR }}/review-submission-rejected.json
+ ${{ env.REVIEW_CONTEXT_DIR }}/review-api-errors.jsonl
+ # The context directory is hidden; include only these explicit files.
include-hidden-files: true
if-no-files-found: ignore
retention-days: 14
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
