feat(examples): add Gerrit CI integration for publishing review results#12
Open
chethanuk wants to merge 4 commits into
Open
feat(examples): add Gerrit CI integration for publishing review results#12chethanuk wants to merge 4 commits into
chethanuk wants to merge 4 commits into
Conversation
Adds examples/gerrit_ci following the gitflic_ci CI-glue pattern (alibaba#316): a stdlib-only post_review.py that reads 'ocr review --format json' and publishes summary, inline comments, suggestion blocks, and file-level findings in ONE batched POST /a/changes/{change}/revisions/{rev}/review, tagged autogenerated:opencodereview with notify=OWNER and omit_duplicate_comments. Decisions (validated against a live Gerrit 3.14 in Docker): - Plain comments, bare line = end_line, no CommentRange: the range form {start,0,end,0} renders lines start..end-1 in the UI (end_character 0 excludes the final line), so correct ranges would need file contents in CI. Verified via UI screenshots during E2E. - Preemptive HTTP Basic auth (urllib's handler does not preempt), XSSI )]}' stripping, HTML-200 detected as config error, 400 batch fold-retry, 409 change-closed tolerated (defensive on modern Gerrit: label-free reviews post fine on closed changes), password scrubbed from all error output. - Jenkins Gerrit Trigger as the reference integration; the script is trigger-agnostic via flags/env (Zuul/hook recipes in the README). Jenkinsfile always passes the injected patchset SHA (revision race). 45 table-driven stdlib-unittest tests, red-to-green TDD; E2E against gerritcodereview/gerrit covering live post, unicode round-trip, dedup re-run, and failure paths (401 exit 2 with scrubbed password).
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds a Gerrit CI integration using Jenkins and Gerrit Trigger, including a Python script that converts OpenCodeReview results into Gerrit reviews, a declarative pipeline, unit tests, documentation, and README links. ChangesGerrit CI integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GerritTrigger
participant Jenkins
participant OCR
participant post_review.py
participant GerritREST
GerritTrigger->>Jenkins: Start patchset job
Jenkins->>OCR: Run ocr review
OCR-->>Jenkins: Write result.json
Jenkins->>post_review.py: Submit review result
post_review.py->>GerritREST: POST batched ReviewInput
GerritREST-->>post_review.py: Return review response
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Send Authorization via add_unredirected_header so Basic credentials cannot follow a redirect to another host (urllib forwards ordinary headers cross-host). - Reject non-object JSON input cleanly instead of an AttributeError traceback; validate --timeout > 0 at argparse time. - Warn on stderr when the 400-fallback folded summary is truncated, and report the fold accurately instead of claiming N inline comments. - Pin drafts=KEEP in ReviewInput (depot_tools convention; old servers defaulted to deleting the caller's drafts). - Jenkinsfile: fetch with an explicit dest refspec so origin/$GERRIT_BRANCH materializes under narrow-refspec clones; comment out extra_body thinking (OpenAI rejects unknown fields). - Gitflic parity: optional positional input arg; single-sourced 'current' revision default; scrub() skips sub-4-char passwords. - README: document exit 1 and the defensive 409 branch; add gerrit_ci row to all five root READMEs (parity with the GitFlic example). - Tests: 44 -> 55, covering stdin input, flag-over-env precedence, fold-retry failure, fold truncation, GERRIT_CHANGE_URL wiring, non-dict JSON, timeout validation, positional input.
Owner
Author
|
Review pass complete (3 finder angles + adversarial verification per finding, each claim repro'd before fixing). Fixed in a528bed: Correctness (all verified with live repros)
Cleanup / conventions
Tests: 44 → 55, all passing — new rows for stdin input, flag-over-env precedence, fold-retry-also-fails (rc 2), fold truncation + accurate fold message, Waived (no change), with reasons
|
4 tasks
- Jenkinsfile: resolve the LLM endpoint from the OCR_LLM_URL/TOKEN/MODEL env triple instead of `ocr config set`, so the auth token stays env-only and is never written to ~/.opencodereview/config.json on a shared agent (OCR_CONFIG_PATH is deliberately ignored by write paths, so it can't redirect the leak). Pin the npm install to a validated version. Document the config-file fallback (and its cleanup) for extra_body, which has no env equivalent. - post_review.py: scrub the base64(user:password) Authorization value from error output too, not just the raw password — a proxy echoing the request header would otherwise leak decodable credentials. +1 test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/gerrit_ci/Jenkinsfile`:
- Around line 70-79: Update the OCR review shell step around `ocr review` so a
total reviewer failure propagates a non-zero Jenkins status instead of being
suppressed by `|| true`; preserve handling for valid partial-review results and
ensure the downstream empty-output guard does not convert total OCR failure into
success.
- Around line 86-93: Update the `post_review.py` execution in the Jenkins
pipeline so it never runs a script from the reviewed workspace while
`withCredentials` binds Gerrit secrets. Use a trusted, pre-provisioned or
shared-library copy outside the workspace, and ensure any documented `curl`
fallback fetches a commit-pinned file and verifies its integrity before
execution.
- Around line 21-30: Remove OCR_LLM_TOKEN from the pipeline-wide environment
block and bind the credential only within stage('Review') using withCredentials
around the ocr review invocation. Preserve OCR_LLM_URL, OCR_LLM_MODEL, and
OCR_USE_ANTHROPIC as pipeline configuration while ensuring npm install and later
stages cannot access the token.
In `@examples/gerrit_ci/post_review_test.py`:
- Around line 520-553: Add a cross-host redirect regression test alongside
MakePosterTest.post that exercises make_poster through urllib’s redirect
handling, then assert the redirected request does not contain an Authorization
header. Preserve the existing preemptive-auth assertion for the initial request
and use the request-header behavior established by add_unredirected_header
rather than changing unrelated tests.
In `@examples/gerrit_ci/post_review.py`:
- Around line 250-263: The scrub function must redact passwords of every length,
including 1–3 character values. Remove the short-password early return in scrub
while preserving base64 credential redaction, and add a regression test
confirming short passwords are replaced in diagnostics rather than logged
verbatim.
In `@examples/gerrit_ci/README.md`:
- Around line 39-50: Update the introductory README statement to qualify that
only supported values can be provided through flags or environment variables,
while preserving the existing flag-over-environment precedence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 181e52d7-1a51-412a-8500-33fd5533c8b4
📒 Files selected for processing (10)
README.ja-JP.mdREADME.ko-KR.mdREADME.mdREADME.ru-RU.mdREADME.zh-CN.mdexamples/README.mdexamples/gerrit_ci/Jenkinsfileexamples/gerrit_ci/README.mdexamples/gerrit_ci/post_review.pyexamples/gerrit_ci/post_review_test.py
Comment on lines
+21
to
+30
| environment { | ||
| // ocr resolves OCR_LLM_URL / OCR_LLM_TOKEN / OCR_LLM_MODEL directly | ||
| // from the environment (no `ocr config set`), so the token stays | ||
| // env-only and is never written to disk on a shared agent. | ||
| OCR_LLM_URL = 'https://api.openai.com/v1/chat/completions' | ||
| OCR_LLM_MODEL = 'gpt-4o' | ||
| OCR_LLM_TOKEN = credentials('ocr-llm-auth-token') | ||
| // The env resolver defaults to the Anthropic protocol; this is an | ||
| // OpenAI chat-completions endpoint. | ||
| OCR_USE_ANTHROPIC = 'false' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files examples/gerrit_ci/Jenkinsfile
echo "----"
ast-grep outline examples/gerrit_ci/Jenkinsfile --view expanded || true
echo "----"
cat -n examples/gerrit_ci/Jenkinsfile | sed -n '1,220p'Repository: chethanuk/open-code-review
Length of output: 6229
Scope OCR_LLM_TOKEN to stage('Review').
The pipeline-wide environment exposes the secret to npm install lifecycle scripts and every later stage; bind it only around ocr review with withCredentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/gerrit_ci/Jenkinsfile` around lines 21 - 30, Remove OCR_LLM_TOKEN
from the pipeline-wide environment block and bind the credential only within
stage('Review') using withCredentials around the ocr review invocation. Preserve
OCR_LLM_URL, OCR_LLM_MODEL, and OCR_USE_ANTHROPIC as pipeline configuration
while ensuring npm install and later stages cannot access the token.
Comment on lines
+70
to
+79
| // `|| true` is REQUIRED: `ocr review` exits non-zero only | ||
| // when EVERY sub-agent fails; partial failures still exit 0. | ||
| // The guard in the next stage handles the empty-output case. | ||
| sh ''' | ||
| ocr review \ | ||
| --from "origin/$GERRIT_BRANCH" \ | ||
| --to "$GERRIT_PATCHSET_REVISION" \ | ||
| --format json \ | ||
| --audience agent \ | ||
| > result.json || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not report total OCR failure as pipeline success.
|| true suppresses the all-agents-failed status, and the empty-output guard then exits successfully. The patchset receives no review while Jenkins appears healthy.
if [ ! -s result.json ]; then
- echo "OCR review produced no output, skipping post."
- exit 0
+ echo "OCR review failed to produce output." >&2
+ exit 1
fiAlso applies to: 95-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/gerrit_ci/Jenkinsfile` around lines 70 - 79, Update the OCR review
shell step around `ocr review` so a total reviewer failure propagates a non-zero
Jenkins status instead of being suppressed by `|| true`; preserve handling for
valid partial-review results and ensure the downstream empty-output guard does
not convert total OCR failure into success.
Comment on lines
+86
to
+93
| withCredentials([usernamePassword(credentialsId: 'gerrit-http', usernameVariable: 'GERRIT_HTTP_USER', passwordVariable: 'GERRIT_HTTP_PASSWORD')]) { | ||
| // post_review.py sits in the reviewed repo only in this | ||
| // demo; real jobs vendor it or fetch it: | ||
| // curl -fsSLO https://raw.githubusercontent.com/alibaba/open-code-review/main/examples/gerrit_ci/post_review.py | ||
| // --revision pins the reviewed SHA explicitly: the | ||
| // `current` default would retarget comments if a new | ||
| // patchset landed mid-pipeline. GERRIT_CHANGE_NUMBER and | ||
| // GERRIT_URL/GERRIT_CHANGE_URL come from the environment. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not execute the patchset’s publisher with Gerrit credentials.
Line 99 runs post_review.py from the revision checked out at Line 46. A malicious patchset can replace that script and read or exfiltrate the bound bot credentials. Execute a pinned, trusted copy outside the reviewed workspace, such as a Jenkins shared-library or pre-provisioned tool.
The documented curl alternative should also be commit-pinned and integrity-verified.
Also applies to: 99-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/gerrit_ci/Jenkinsfile` around lines 86 - 93, Update the
`post_review.py` execution in the Jenkins pipeline so it never runs a script
from the reviewed workspace while `withCredentials` binds Gerrit secrets. Use a
trusted, pre-provisioned or shared-library copy outside the workspace, and
ensure any documented `curl` fallback fetches a commit-pinned file and verifies
its integrity before execution.
Comment on lines
+520
to
+553
| class MakePosterTest(unittest.TestCase): | ||
| ENDPOINT = "https://gerrit.example/a/changes/42/revisions/current/review" | ||
|
|
||
| def post(self, review_input, body=b")]}'\n{}"): | ||
| captured = {} | ||
|
|
||
| def fake_urlopen(req, timeout=None): | ||
| captured["req"] = req | ||
| return FakeResponse(body) | ||
|
|
||
| with mock.patch.object(pr.urllib.request, "urlopen", fake_urlopen): | ||
| post = pr.make_poster(self.ENDPOINT, "review-bot", "s3cret-pass", 30) | ||
| parsed = post(review_input) | ||
| return captured["req"], parsed | ||
|
|
||
| def test_preemptive_basic_auth_and_utf8_body(self): | ||
| import base64 | ||
|
|
||
| req, _parsed = self.post({"message": "空指针解引用：y 可能为 nil"}) | ||
| auth = req.get_header("Authorization") | ||
| self.assertIsNotNone(auth, "Authorization header must be set preemptively") | ||
| self.assertTrue(auth.startswith("Basic ")) | ||
| self.assertEqual( | ||
| base64.b64decode(auth[len("Basic "):]).decode("utf-8"), | ||
| "review-bot:s3cret-pass", | ||
| ) | ||
| self.assertIn("空指针解引用".encode("utf-8"), req.data) | ||
| self.assertIn("application/json", req.get_header("Content-type")) | ||
|
|
||
| def test_xssi_response_parses(self): | ||
| _req, parsed = self.post( | ||
| {"message": "hi"}, body=b")]}'\n{\"labels\": {\"Code-Review\": 0}}" | ||
| ) | ||
| self.assertEqual(parsed, {"labels": {"Code-Review": 0}}) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test file and implementation references.
git ls-files | rg 'examples/gerrit_ci/(post_review_test\.py|post_review\.py)$|post_review'
# Show the relevant portions of the test file around the cited lines.
python3 - <<'PY'
from pathlib import Path
p = Path('examples/gerrit_ci/post_review_test.py')
text = p.read_text().splitlines()
for start, end in [(480, 580)]:
print(f'--- {p}:{start}-{end} ---')
for i in range(start-1, min(end, len(text))):
print(f'{i+1:4d}: {text[i]}')
PY
# Search for redirect/auth handling in the implementation.
rg -n "add_unredirected_header|add_header|Authorization|redirect|HTTPRedirectHandler|urlopen|Request\(" examples/gerrit_ci -SRepository: chethanuk/open-code-review
Length of output: 5570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation if present.
python3 - <<'PY'
from pathlib import Path
for candidate in [Path('examples/gerrit_ci/post_review.py'),
Path('examples/gerrit_ci/post_review_test.py')]:
if candidate.exists():
print(f'=== {candidate} ===')
text = candidate.read_text().splitlines()
for start, end in [(1, min(260, len(text))), (260, min(520, len(text)))]:
if start <= len(text):
print(f'--- {start}-{end} ---')
for i in range(start-1, end):
print(f'{i+1:4d}: {text[i]}')
PYRepository: chethanuk/open-code-review
Length of output: 40128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the implementation uses urllib.request.Request directly, inspect the auth setup.
python3 - <<'PY'
from pathlib import Path
p = Path('examples/gerrit_ci/post_review.py')
if p.exists():
text = p.read_text().splitlines()
keys = ['Request(', 'Authorization', 'add_unredirected_header', 'add_header', 'HTTPPasswordMgr', 'HTTPBasicAuthHandler', 'build_opener']
for k in keys:
print(f'## {k}')
for i, line in enumerate(text, 1):
if k in line:
print(f'{i:4d}: {line}')
PYRepository: chethanuk/open-code-review
Length of output: 854
🏁 Script executed:
python3 - <<'PY'
import urllib.request
def probe(label, add_unredirected):
req = urllib.request.Request("https://gerrit.example/a/changes/42/revisions/current/review", data=b"{}", method="POST")
if add_unredirected:
req.add_unredirected_header("Authorization", "Basic Zm9vOmJhcg==")
else:
req.add_header("Authorization", "Basic Zm9vOmJhcg==")
req.add_header("Content-Type", "application/json; charset=UTF-8")
redir = urllib.request.HTTPRedirectHandler().redirect_request(
req, None, 302, "Found", {}, "https://attacker.example/login"
)
print(label)
print(" original header:", req.get_header("Authorization"))
print(" redirected header:", redir.get_header("Authorization") if redir else None)
print(" original headers:", req.header_items())
print(" redirected headers:", redir.header_items() if redir else None)
print()
probe("add_header", False)
probe("add_unredirected_header", True)
PYRepository: chethanuk/open-code-review
Length of output: 654
Add a redirect regression test for Authorization in examples/gerrit_ci/post_review_test.py:535-547.
The existing check only covers the initial request. Add a cross-host redirect case to ensure the redirected request has no Authorization header; add_header() would leak credentials, while add_unredirected_header() does not.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 538-538: String contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/gerrit_ci/post_review_test.py` around lines 520 - 553, Add a
cross-host redirect regression test alongside MakePosterTest.post that exercises
make_poster through urllib’s redirect handling, then assert the redirected
request does not contain an Authorization header. Preserve the existing
preemptive-auth assertion for the initial request and use the request-header
behavior established by add_unredirected_header rather than changing unrelated
tests.
Comment on lines
+250
to
+263
| def scrub(text, password, user=""): | ||
| """Never let the HTTP credentials reach CI logs, even via echoed error bodies.""" | ||
| if password: | ||
| # Servers/proxies that echo the Authorization header would leak the | ||
| # base64(user:password) form, which decodes trivially; scrub it too. | ||
| b64 = base64.b64encode( | ||
| ("%s:%s" % (user, password)).encode("utf-8") | ||
| ).decode("ascii") | ||
| text = text.replace(b64, "***") | ||
| # A very short "password" (misconfiguration) would mangle unrelated | ||
| # diagnostics, e.g. "Operation" -> "O***eration"; skip it. | ||
| if not password or len(password) < 4: | ||
| return text | ||
| return text.replace(password, "***") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Never exempt short passwords from redaction.
A 1–3 character password echoed by Gerrit or a proxy is currently written verbatim to CI logs, contradicting this function’s security guarantee. Prefer mangled diagnostics over leaking credentials, and add a short-password regression test.
Proposed fix
def scrub(text, password, user=""):
"""Never let the HTTP credentials reach CI logs, even via echoed error bodies."""
- if password:
- # Servers/proxies that echo the Authorization header would leak the
- # base64(user:password) form, which decodes trivially; scrub it too.
- b64 = base64.b64encode(
- ("%s:%s" % (user, password)).encode("utf-8")
- ).decode("ascii")
- text = text.replace(b64, "***")
- # A very short "password" (misconfiguration) would mangle unrelated
- # diagnostics, e.g. "Operation" -> "O***eration"; skip it.
- if not password or len(password) < 4:
+ if not password:
return text
+
+ b64 = base64.b64encode(
+ ("%s:%s" % (user, password)).encode("utf-8")
+ ).decode("ascii")
+ text = text.replace(b64, "***")
return text.replace(password, "***")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def scrub(text, password, user=""): | |
| """Never let the HTTP credentials reach CI logs, even via echoed error bodies.""" | |
| if password: | |
| # Servers/proxies that echo the Authorization header would leak the | |
| # base64(user:password) form, which decodes trivially; scrub it too. | |
| b64 = base64.b64encode( | |
| ("%s:%s" % (user, password)).encode("utf-8") | |
| ).decode("ascii") | |
| text = text.replace(b64, "***") | |
| # A very short "password" (misconfiguration) would mangle unrelated | |
| # diagnostics, e.g. "Operation" -> "O***eration"; skip it. | |
| if not password or len(password) < 4: | |
| return text | |
| return text.replace(password, "***") | |
| def scrub(text, password, user=""): | |
| """Never let the HTTP credentials reach CI logs, even via echoed error bodies.""" | |
| if not password: | |
| return text | |
| b64 = base64.b64encode( | |
| ("%s:%s" % (user, password)).encode("utf-8") | |
| ).decode("ascii") | |
| text = text.replace(b64, "***") | |
| return text.replace(password, "***") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/gerrit_ci/post_review.py` around lines 250 - 263, The scrub function
must redact passwords of every length, including 1–3 character values. Remove
the short-password early return in scrub while preserving base64 credential
redaction, and add a regression test confirming short passwords are replaced in
diagnostics rather than logged verbatim.
Comment on lines
+39
to
+50
| Every value can be passed via flag or environment variable; **flags override the environment**. Run `python3 post_review.py -h` for the same list. | ||
|
|
||
| | Flag | Env fallback | Default | Description | | ||
| |------|--------------|---------|-------------| | ||
| | `--gerrit-url` | `GERRIT_URL`, else derived from `GERRIT_CHANGE_URL` | — | Gerrit base URL (context path kept, trailing slash tolerated) | | ||
| | `--change` | `GERRIT_CHANGE_NUMBER` | — | Change number | | ||
| | `--revision` | `GERRIT_PATCHSET_REVISION` | `current` | Revision/patchset to comment on | | ||
| | `--user` | `GERRIT_HTTP_USER` | — | HTTP credentials username | | ||
| | `--password` | `GERRIT_HTTP_PASSWORD` | — | Gerrit HTTP password, **not** the account password | | ||
| | `--input` | — | `-` (stdin) | Review result JSON file (`-` = stdin); also accepted as a positional argument, which wins if both are given | | ||
| | `--dry-run` | — | off | Print the ReviewInput instead of posting it | | ||
| | `--timeout` | — | `30` | HTTP timeout in seconds | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the environment-variable claim.
“Every value” is inaccurate: --input, --dry-run, and the positional input have no environment fallback in the table. Change this to “Supported values can be passed…” or document the missing fallbacks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/gerrit_ci/README.md` around lines 39 - 50, Update the introductory
README statement to qualify that only supported values can be provided through
flags or environment variables, while preserving the existing
flag-over-environment precedence.
Post-review hardening from an OSS-precedent study (depot_tools, kudu, Gerrit REST docs): - Bounded retry (3 attempts, exp backoff) in make_poster, scoped to the provably-safe failures only: HTTP 5xx and pre-response connection errors (refused/reset/DNS). Read-timeouts are deliberately NOT retried — a timeout is ambiguous (the server may have applied the review) and omit_duplicate_comments dedupes only inline comments, not the summary message, so a blind retry could post a duplicate change message. 4xx (400/401/404/409) propagate unchanged so main() classifies them as before. - Document why plain comments are used, not robot_comments: the latter is deprecated since Gerrit 3.6, disabled-by-default in 3.12, and slated for removal; the tag already marks bot origin. - Fold fallback: strip the '; N posted as inline comment(s).' clause from the reused summary so the folded message doesn't claim inline comments were posted and then explain they couldn't be placed. - README: document the retry scoping and note fix_suggestions / label voting as intentional future options. Tests: 56 -> 61 (5 retry cases: 5xx-then-ok, conn-err-then-ok, 5xx-exhaust, read-timeout-not-retried, 4xx/409-not-retried).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/gerrit_ci/post_review_test.py (1)
606-611: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover timeouts wrapped in
URLError.This only tests the bare exception. Add the distinct
URLError(socket.timeout(...))path so a regression cannot retry an ambiguous request and duplicate the summary.Proposed test update
- def test_read_timeout_not_retried(self): - # A urlopen read-timeout raises a bare socket.timeout/TimeoutError, - # which is ambiguous and must NOT be retried. - n, _result, raised = self.post_seq([socket.timeout("timed out"), b")]}'\n{}"]) - self.assertIsInstance(raised, socket.timeout) - self.assertEqual(n, 1) + def test_timeout_not_retried(self): + errors = [ + socket.timeout("timed out"), + urllib.error.URLError(socket.timeout("timed out")), + ] + for error in errors: + with self.subTest(error=type(error).__name__): + n, _result, raised = self.post_seq([error, b")]}'\n{}"]) + self.assertIs(raised, error) + self.assertEqual(n, 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/gerrit_ci/post_review_test.py` around lines 606 - 611, Extend test_read_timeout_not_retried to cover a socket.timeout wrapped inside URLError, verifying the request is not retried and the original timeout remains the raised exception; retain the existing bare-timeout coverage and assert the call count stays at one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@examples/gerrit_ci/post_review_test.py`:
- Around line 606-611: Extend test_read_timeout_not_retried to cover a
socket.timeout wrapped inside URLError, verifying the request is not retried and
the original timeout remains the raised exception; retain the existing
bare-timeout coverage and assert the call count stays at one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21ff5a4f-262c-44c6-a1f9-fb2dcfb9b550
📒 Files selected for processing (3)
examples/gerrit_ci/README.mdexamples/gerrit_ci/post_review.pyexamples/gerrit_ci/post_review_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/gerrit_ci/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fork-side review PR for upstream alibaba#316. One batched ReviewInput publisher (post_review.py, stdlib-only, 349 LOC) + 45 table-driven tests + README + Jenkinsfile (Gerrit Trigger).
Verification: 45/45 unit tests green; make check/test/coverage green (81.0% >= 80%); Docker-Gerrit 3.14 E2E all-checked (live post, unicode byte-exact round-trip, dedup re-run 6->6, wrong-password exit 2 with scrubbed output, range-trial verdict: keep bare line — {S,0,E,0} excludes the final line in UI rendering).
Summary by CodeRabbit
New Features
Documentation
Tests