feat(examples): add Gerrit CI integration example (Jenkins + Gerrit Trigger)#401
feat(examples): add Gerrit CI integration example (Jenkins + Gerrit Trigger)#401chethanuk 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).
- 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.
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ocr config set llm.url "$OCR_LLM_URL" | ||
| ocr config set llm.auth_token "$OCR_LLM_AUTH_TOKEN" |
There was a problem hiding this comment.
Security: LLM auth token persisted on disk. ocr config set llm.auth_token writes the secret to ~/.opencodereview/config.json (confirmed in config_cmd.go). On shared Jenkins agents this file survives the build and can be read by subsequent jobs from other projects/users.
Consider adding a cleanup step in a post { always { ... } } block to remove the config file after the pipeline completes, or use a per-build OCR_CONFIG_PATH pointing into the workspace so it gets cleaned up automatically:
environment {
OCR_CONFIG_PATH = "${WORKSPACE}/.ocr-config.json"
}This way the config (including the token) lives inside the workspace and is removed by Jenkins' workspace cleanup.
There was a problem hiding this comment.
Good catch — fixed in db49e19, though not with OCR_CONFIG_PATH: ocr config set and ocr review deliberately ignore that variable (only read-only commands like ocr llm test honor it — see config_cmd.go, the comment there notes a leaked OCR_CONFIG_PATH must not redirect writes), so pointing it at the workspace would not have kept the token off disk.
The root fix is to skip the config file entirely: ocr resolves the OCR_LLM_URL / OCR_LLM_TOKEN / OCR_LLM_MODEL env triple directly (resolver.go tryOCREnv, tried after the config file), so the Jenkinsfile now sets those in the environment block with the token as a masked credentials() binding — nothing is ever written to ~/.opencodereview/config.json. The one case that still needs the config file, extra_body (no env equivalent), is documented with the post { always { rm -f ... } } cleanup you suggested.
|
|
||
| stage('Install OCR') { | ||
| steps { | ||
| sh 'npm install -g @alibaba-group/open-code-review' |
There was a problem hiding this comment.
Reproducibility / supply-chain risk: Installing globally without a pinned version makes builds non-reproducible and vulnerable to supply-chain attacks if a compromised or breaking version is published. For an example that users will copy into production pipelines, pin to a known-good version:
sh 'npm install -g @alibaba-group/open-code-review@<version>'Also consider using a local install (npm install in the workspace) instead of -g to avoid conflicts between concurrent builds on shared agents.
There was a problem hiding this comment.
Fixed in db49e19 — pinned to @alibaba-group/open-code-review@1.7.12 (current latest) with a comment to bump deliberately. Kept it a global install to match the other CI examples (gitflic_ci, gitlab_ci); a workspace-local install would diverge from the established pattern for little gain on a single-tool image.
| credentials = base64.b64encode( | ||
| ("%s:%s" % (user, password)).encode("utf-8") | ||
| ).decode("ascii") |
There was a problem hiding this comment.
The scrub() function only removes the raw password from error output, but the base64-encoded credentials string (which encodes user:password) is not scrubbed. If an HTTPError response body, proxy log, or exception message includes the Authorization header value (e.g., some servers/proxies echo request headers in error responses), the base64-encoded credentials would be logged in plain text and could be trivially decoded.
Consider also scrubbing the base64-encoded credentials string. You could pass it as an additional value to scrub, or restructure make_poster so the caller can scrub both forms.
There was a problem hiding this comment.
Fixed in db49e19. scrub() now also replaces the exact base64(user:password) string that make_poster puts in the Authorization header, so a proxy or server that echoes the request header in an error body can no longer leak decodable credentials. Added test_b64_credentials_never_in_error_body (401 body echoing Basic <b64>) alongside the existing raw-password test.
- 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.
…, 4/4 smoke checks)
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).
1 participant
Fixes #316
Implements the approach outlined in my comment on #316: a self-contained
examples/gerrit_ci/following theexamples/gitflic_ciCI-glue pattern — a stdlib-onlypost_review.pythat readsocr review --format jsonand publishes everything (summary, inline comments with ranges, suggestions, severity) onto the triggering Gerrit change in one REST call, plus a Jenkinsfile wired for the Gerrit Trigger plugin.Design decisions (each verified against a live Gerrit)
POST /a/changes/{change}/revisions/{rev}/review— one ReviewInput carries the summary message and all inline comments, so a review is atomic (no comment-spam, no partial posts).omit_duplicate_commentsmakes re-runs idempotent;tag: autogenerated:opencodereviewmarks bot comments;notify: OWNERavoids spamming all reviewers;drafts: KEEPpinned (depot_tools convention — old servers defaulted to deleting the caller's drafts).lineinstead ofrangefor multi-line comments — trialed both against Gerrit 3.14: a{start_line, end_line}range withstart_character: 0, end_character: 0excludes the final line in the UI, so the script anchors toend_lineand keeps the OCR line span in the message text.)]}'strip — Gerrit doesn't send a 401 challenge on/a/paths; the auth header is attached up-front (viaadd_unredirected_header, so credentials can never follow a redirect to another host) and the XSSI prefix is stripped before JSON parsing. Both match what chromiumdepot_tools/gerrit_util.pydoes.Verification
python3 -m unittest examples/gerrit_ci/post_review_test.py -v— 55/55 pass (matrices for ReviewInput building, endpoint/base-URL derivation incl.GERRIT_CHANGE_URL, XSSI stripping, HTTP outcomes 400/401/404/409, fold-retry failure, truncation, stdin/positional input, flag-over-env precedence, timeout validation)git push refs/for/master, posted a real OCR JSON result withpost_review.py— summary + inline comments (single-line, multi-line, suggestion fence, severity header) all render on the change; re-run posts zero duplicates (omit_duplicate_comments); wrong password → clean exit 2, password absent from output; out-of-diff line and abandoned-change paths exercisedmake build,make test, coverage 81.0% (≥80% gate)examples/README.mdand all five root READMEs (same placement as the GitFlic example, commit 7497d5a)E2E evidence (Gerrit 3.14.2, Docker)
Change page after
post_review.py— summary + inline comments with severity headers and suggestion fence:Inline anchoring details (bare-line rendering + range trial)
Bare
lineanchoring on lines 3 and 10:Range trial (lines 5–8) that motivated the bare-
linedecision — the character-0 range excludes the final line in the UI:Full unit test output (55 tests)
Happy to wire
python3 -m unittest discover examplesinto CI as a follow-up if you'd like the example tests gated.