diff --git a/README.ja-JP.md b/README.ja-JP.md index eb36f1ef..7dc332c6 100644 --- a/README.ja-JP.md +++ b/README.ja-JP.md @@ -497,6 +497,7 @@ JSON出力ではこの2つのフィールドは`content`や`start_line`などと - [`github_actions/`](./examples/github_actions/) — GitHub Actions統合の例 - [`gitlab_ci/`](./examples/gitlab_ci/) — GitLab CI統合の例 - [`gitflic_ci/`](./examples/gitflic_ci/) — GitFlic CI統合の例 +- [`gerrit_ci/`](./examples/gerrit_ci/) — Gerrit (Jenkins / Gerrit Trigger) 統合の例 #### GitHub Action diff --git a/README.ko-KR.md b/README.ko-KR.md index 6320777d..7b4e4954 100644 --- a/README.ko-KR.md +++ b/README.ko-KR.md @@ -497,6 +497,7 @@ JSON 출력에서 두 field는 `content`, `start_line` 등과 같은 수준의 s - [`github_actions/`](./examples/github_actions/): GitHub Actions 통합 예시 - [`gitlab_ci/`](./examples/gitlab_ci/): GitLab CI 통합 예시 - [`gitflic_ci/`](./examples/gitflic_ci/): GitFlic CI 통합 예시 +- [`gerrit_ci/`](./examples/gerrit_ci/): Gerrit (Jenkins / Gerrit Trigger) 통합 예시 #### GitHub Action diff --git a/README.md b/README.md index 80bc3597..51377d09 100644 --- a/README.md +++ b/README.md @@ -499,6 +499,7 @@ See the [`examples/`](./examples/) directory for integration examples: - [`github_actions/`](./examples/github_actions/) — GitHub Actions integration example - [`gitlab_ci/`](./examples/gitlab_ci/) — GitLab CI integration example - [`gitflic_ci/`](./examples/gitflic_ci/) — GitFlic CI integration example +- [`gerrit_ci/`](./examples/gerrit_ci/) — Gerrit (Jenkins / Gerrit Trigger) integration example #### GitHub Action diff --git a/README.ru-RU.md b/README.ru-RU.md index e3671a0c..729780fd 100644 --- a/README.ru-RU.md +++ b/README.ru-RU.md @@ -499,6 +499,7 @@ ocr review \ - [`github_actions/`](./examples/github_actions/) — пример интеграции с GitHub Actions - [`gitlab_ci/`](./examples/gitlab_ci/) — пример интеграции с GitLab CI - [`gitflic_ci/`](./examples/gitflic_ci/) — пример интеграции с GitFlic CI +- [`gerrit_ci/`](./examples/gerrit_ci/) — пример интеграции с Gerrit (Jenkins / Gerrit Trigger) #### GitHub Action diff --git a/README.zh-CN.md b/README.zh-CN.md index b3eabde3..bd59336e 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -497,6 +497,7 @@ ocr review \ - [`github_actions/`](./examples/github_actions/) — GitHub Actions 集成示例 - [`gitlab_ci/`](./examples/gitlab_ci/) — GitLab CI 集成示例 - [`gitflic_ci/`](./examples/gitflic_ci/) — GitFlic CI 集成示例 +- [`gerrit_ci/`](./examples/gerrit_ci/) — Gerrit (Jenkins / Gerrit Trigger) 集成示例 #### GitHub Action diff --git a/examples/README.md b/examples/README.md index 2ffc3ffe..7133174f 100644 --- a/examples/README.md +++ b/examples/README.md @@ -7,5 +7,6 @@ This directory contains examples for integrating OpenCodeReview (OCR) into vario - **[github_actions/](./github_actions/)** - GitHub Actions integration example - **[gitlab_ci/](./gitlab_ci/)** - GitLab CI integration example - **[gitflic_ci/](./gitflic_ci/)** - GitFlic CI integration example +- **[gerrit_ci/](./gerrit_ci/)** - Gerrit (Jenkins / Gerrit Trigger) integration example Each subdirectory contains its own README with detailed setup instructions. diff --git a/examples/gerrit_ci/Jenkinsfile b/examples/gerrit_ci/Jenkinsfile new file mode 100644 index 00000000..c8dcef4e --- /dev/null +++ b/examples/gerrit_ci/Jenkinsfile @@ -0,0 +1,107 @@ +// OpenCodeReview - Gerrit patchset auto-review (Jenkins + Gerrit Trigger) +// +// Reviews every new patchset with OpenCodeReview and posts the findings back +// onto the change via post_review.py (see README.md in this directory). +// +// Trigger: configure the Gerrit Trigger plugin ON THE JOB (Configure -> +// Gerrit Trigger -> trigger on "Patchset Created"). A declarative +// `triggers { gerrit(...) }` block exists, but its map syntax varies across +// plugin versions, so real jobs configure the trigger in the UI. The plugin +// injects GERRIT_BRANCH, GERRIT_REFSPEC, GERRIT_CHANGE_NUMBER, +// GERRIT_CHANGE_URL and GERRIT_PATCHSET_REVISION, used below. +// +// Jenkins credentials: ocr-llm-auth-token (Secret text, LLM API token) and +// gerrit-http (Username/password: bot account + its Gerrit HTTP password -- +// Settings > HTTP Credentials, NOT the account password). +// The agent needs git, node/npm 20+ and python3 on PATH. + +pipeline { + agent any + + 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' + // GERRIT_URL: set here or in Jenkins global env; if unset, + // post_review.py derives it from the injected GERRIT_CHANGE_URL. + } + + stages { + stage('Checkout') { + steps { + // Fresh workspaces lack the target ref, so fetch the branch + // with an explicit refspec that materializes the tracking ref + // (ocr diffs against origin/$GERRIT_BRANCH; a bare fetch only + // updates FETCH_HEAD when the clone refspec is narrow), then + // fetch the patchset ref and check out its revision. + sh ''' + git fetch origin "+refs/heads/$GERRIT_BRANCH:refs/remotes/origin/$GERRIT_BRANCH" + git fetch origin "$GERRIT_REFSPEC" + git checkout "$GERRIT_PATCHSET_REVISION" + ''' + } + } + + stage('Install OCR') { + steps { + // Pin the version you have validated; bump deliberately. + sh 'npm install -g @alibaba-group/open-code-review@1.7.12' + // extra_body has no env-var equivalent. Providers that accept + // a top-level "thinking" field (e.g. DashScope/GLM; OpenAI + // rejects unknown fields) must fall back to the config file, + // which then needs the full triple (the config-file strategy + // resolves url/token/model together, ignoring the env vars): + // ocr config set llm.url/auth_token/model/use_anthropic ... + // ocr config set llm.extra_body '{"thinking": {"type": "disabled"}}' + // That writes the token to ~/.opencodereview/config.json, so on + // a shared agent clean it up with: + // post { always { sh 'rm -f ~/.opencodereview/config.json' } } + } + } + + stage('Review') { + steps { + // `|| 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 + ''' + } + } + + stage('Post to Gerrit') { + steps { + 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. + sh ''' + if [ ! -s result.json ]; then + echo "OCR review produced no output, skipping post." + exit 0 + fi + python3 examples/gerrit_ci/post_review.py \ + --input result.json \ + --revision "$GERRIT_PATCHSET_REVISION" + ''' + } + } + } + } +} diff --git a/examples/gerrit_ci/README.md b/examples/gerrit_ci/README.md new file mode 100644 index 00000000..62690ba0 --- /dev/null +++ b/examples/gerrit_ci/README.md @@ -0,0 +1,115 @@ +# OpenCodeReview - Gerrit CI Demo + +This demo shows how to integrate OpenCodeReview into a [Gerrit](https://www.gerritcodereview.com) code-review flow to automatically review changes and post the findings as inline comments on the patchset, plus a summary message. + +Like the GitHub Actions, GitLab CI, and GitFlic examples, the posting glue lives in the CI layer rather than in the `ocr` binary. Here it is a small, dependency-free Python script — [`post_review.py`](post_review.py) — that reads `ocr review --format json` and posts it as **one batched ReviewInput** to Gerrit's set-review endpoint, so a review lands atomically: inline comments grouped per file plus a summary message, in a single request. + +## How It Works + +``` +Gerrit Trigger (patchset-created) → Jenkins job → ocr review --format json → post_review.py → POST /a/changes/{change}/revisions/{revision}/review +``` + +1. The Gerrit Trigger plugin fires a Jenkins job on `patchset-created` +2. The job fetches the change ref and runs `ocr review --format json --audience agent` +3. `python3 post_review.py` reads the JSON and POSTs a single `ReviewInput` containing: + - **Inline comments** on the changed lines, grouped per file + - **A summary message** with the totals (plus any comments that could not be placed inline) + +The script handles the Gerrit-specific wrinkles for you: preemptive HTTP basic auth on `/a/` endpoints, the `)]}'` anti-XSSI prefix on responses, a 200 that is actually an HTML login page (treated as a configuration error), and an HTTP 400 on the batch (retried once with all inline comments folded into the summary message so findings still reach the change). + +## Setup + +### 1. Create a bot account and HTTP password + +Create a dedicated Gerrit account for the bot (its name appears on the review comments). Log in as that account and generate a token under **Settings → HTTP Credentials → Generate Password**. + +> **Important:** this HTTP password is **not** the account/login password — using the login password is the most common cause of HTTP 401 from `/a/` endpoints. `post_review.py` says so explicitly when Gerrit rejects the credentials. + +### 2. Grant minimal permissions + +The bot only needs to read changes and comment on them. In the project's (or `All-Projects`') access settings, grant the bot's group **Read** on `refs/*` — commenting on open changes requires nothing more. Do not grant `Label: Code-Review` voting rights unless you enable label voting (see Notes). + +### 3. Configure the pipeline + +Copy `post_review.py` into your repository (or fetch it in the job) and wire it into your Jenkins job — see the [`Jenkinsfile`](Jenkinsfile) in this directory. Store `GERRIT_HTTP_USER` / `GERRIT_HTTP_PASSWORD` as Jenkins credentials, plus the LLM API token for the review step itself. `ocr` resolves `OCR_LLM_URL` / `OCR_LLM_TOKEN` / `OCR_LLM_MODEL` directly from the environment — no `ocr config set` needed, so the token is never written to disk on a shared agent. + +## Configuration Reference + +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 | + +`GERRIT_CHANGE_URL` derivation: modern change URLs (`{base}/c/{project}/+/{number}`) split at `/c/`, which keeps any context path; legacy URLs (`{base}/{number}`) drop the trailing number. + +## Usage + +### Jenkins (Gerrit Trigger) + +See the [`Jenkinsfile`](Jenkinsfile) in this directory. The Gerrit Trigger plugin injects `GERRIT_CHANGE_NUMBER`, `GERRIT_PATCHSET_REVISION`, and `GERRIT_CHANGE_URL` into the build environment, so the script needs no positional wiring: + +```bash +ocr review --format json --audience agent | python3 post_review.py +``` + +### Other triggers + +The script is trigger-agnostic: anything that can set the environment variables above (or pass the flags) can drive it. + +- **Zuul** — the same env contract can be set in a Zuul job from `zuul.change` / `zuul.patchset` variables; the script does not care who exported them. +- **`patchset-created` hook** — post directly from a server-side hook: + + ```bash + ocr review --format json | python3 post_review.py \ + --gerrit-url https://gerrit.example.com --change "$CHANGE" --revision "$REVISION" + ``` + +## Dry Run + +Test the posting step locally without touching the change (no credentials required): + +```bash +ocr review --from origin/main --to HEAD --format json > /tmp/r.json +python3 post_review.py /tmp/r.json --dry-run # or: --input /tmp/r.json +``` + +`--dry-run` prints the exact ReviewInput that would be POSTed: + +```json +{ + "message": "OpenCodeReview found 2 issue(s) in 1 file(s) reviewed; 2 posted as inline comment(s).", + "tag": "autogenerated:opencodereview", + "notify": "OWNER", + "omit_duplicate_comments": true, + "comments": { + "internal/scan/scan.go": [ + { "message": "**Severity:** high · ...", "unresolved": true, "line": 42 }, + ... +``` + +## Notes & Limitations + +- **Re-reviews and duplicates** — the ReviewInput sets `omit_duplicate_comments`, but Gerrit only drops **byte-identical** comments at the same location. A fresh LLM run usually rewords its findings, so re-triggered reviews **will** repeat comments. All bot comments carry the tag `autogenerated:opencodereview`, so UIs and scripts can filter or collapse them. +- **Retries** — the POST is retried (up to `MAX_ATTEMPTS`, with exponential backoff) only on failures that provably did **not** apply the review: HTTP 5xx and pre-response connection errors (refused/reset/DNS). Read timeouts are **not** retried — they are ambiguous (the server may have applied the review) and `omit_duplicate_comments` does not dedupe the summary `message`, so a retry could post a duplicate change message. 4xx responses (400/401/404/409) are not retried; they are classified and handled directly. +- **Native fix suggestions and label voting** — intentional future options. `fix_suggestions` (Gerrit's native apply-a-fix) is experiment-gated in Gerrit 3.10+ and needs character-level ranges this example does not carry, so suggested code is emitted under a `**Suggestion:**` heading in a plain fenced code block, which renders portably on any Gerrit version. Label voting (Code-Review ±1) is likewise left out of v1 — see **Label voting** below. +- **Label voting** — deliberately not in v1: the script comments, it does not vote. Teams that want gating can add one line to the ReviewInput in `build_review_input`: `"labels": {"Code-Review": -1}` (and grant the bot the label permission). +- **Revision race** — always pass the trigger-injected revision SHA (`GERRIT_PATCHSET_REVISION`). The `current` default is for manual runs: if a new patchset lands mid-pipeline, `current` would retarget the comments onto code the review never saw. +- **Exit codes** — `0` on success (HTTP 409 change-closed is tolerated and also exits 0); `1` when the input JSON is unreadable or unparsable; `2` on configuration or HTTP errors, so the CI step fails visibly. The 409 handling is defensive: modern Gerrit (verified on 3.14) accepts label-free reviews even on abandoned changes, so it matters mainly for older/stricter servers. + +## Tests + +`post_review.py` ships with [`post_review_test.py`](post_review_test.py) — standard-library `unittest`, no network or git required: + +```bash +cd examples/gerrit_ci +python3 post_review_test.py +``` diff --git a/examples/gerrit_ci/post_review.py b/examples/gerrit_ci/post_review.py new file mode 100644 index 00000000..73baf430 --- /dev/null +++ b/examples/gerrit_ci/post_review.py @@ -0,0 +1,436 @@ +#!/usr/bin/env python3 +"""Post an OpenCodeReview result onto a Gerrit change. + +This is the CI-layer "glue" for Gerrit, mirroring examples/gitflic_ci: it keeps +platform-specific publishing out of the `ocr` binary and lives entirely in the +pipeline. It reads the JSON emitted by `ocr review --format json` and posts it +as ONE batched ReviewInput via + + POST {GERRIT_URL}/a/changes/{change}/revisions/{revision}/review + +so a review lands atomically: inline comments grouped per file plus a summary +message, in a single request (Gerrit's set-review endpoint). + +Gerrit specifics handled here: + + - preemptive HTTP basic auth: Gerrit's /a/ endpoints reply 401 without a + challenge round-trip in some setups, so the Authorization header is always + sent up front (urllib's HTTPBasicAuthHandler does not do this); + - the ")]}'" XSSI prefix on every JSON response is stripped before parsing; + - a 200 response that is not Gerrit JSON (e.g. an HTML login page after a + redirect) is treated as a configuration error, not success; + - HTTP 400 on the batch is retried once with all inline comments folded into + the summary message, so findings still reach the change. + +Comments are placed on a single line (`line` = end_line); Gerrit's range form +is deliberately not used in v1. Credentials are the Gerrit *HTTP password* +(Settings > HTTP Credentials), not the account password, and are never echoed +into logs or error output. + +Standard library only (json, urllib) so it runs on any stock python3 image. +""" + +import argparse +import base64 +import json +import os +import re +import socket +import sys +import time +import urllib.error +import urllib.request + +XSSI_PREFIX = ")]}'" +MAX_MESSAGE_LEN = 16000 +TRUNCATION_MARKER = "\n\n[truncated]" +DEFAULT_REVISION = "current" +# Total attempts (so 2 retries) for the safe-to-retry transport failures in +# make_poster; see post() for exactly which failures qualify. +MAX_ATTEMPTS = 3 +INLINE_CLAUSE_RE = re.compile(r"; \d+ posted as inline comment\(s\)\.") + +# Injectable so tests can run without real delays; production uses time.sleep. +_sleep = time.sleep + + +def log(msg): + print(msg, file=sys.stderr) + + +# --------------------------------------------------------------------------- # +# Pure layer: OCR result JSON -> Gerrit ReviewInput +# --------------------------------------------------------------------------- # + + +def format_comment(c): + """Render one inline comment body (same shape as gitflic's format_comment). + + The Severity/Category header line exists because Gerrit's CommentInput has + no structural severity/category slot, so the metadata must live in the + message text itself. + """ + header = [] + if c.get("severity"): + header.append("**Severity:** " + c["severity"]) + if c.get("category"): + header.append("**Category:** " + c["category"]) + body = c.get("content", "") + if header: + body = " · ".join(header) + "\n\n" + body + suggestion = c.get("suggestion_code", "") + existing = c.get("existing_code", "") + if suggestion and existing: + body += "\n\n**Suggestion:**\n```\n" + suggestion + "\n```" + return body + + +def truncate_message(msg): + if len(msg) <= MAX_MESSAGE_LEN: + return msg + return msg[: MAX_MESSAGE_LEN - len(TRUNCATION_MARKER)] + TRUNCATION_MARKER + + +def build_review_input(result): + """Map `ocr review --format json` output to one Gerrit ReviewInput dict.""" + comments = result.get("comments") or [] + warnings = result.get("warnings") or [] + + grouped = {} # path -> [CommentInput, ...] + folded = [] # comment bodies without a usable path; go into the summary + for c in comments: + text = truncate_message(format_comment(c)) + path = c.get("path") or "" + if not path: + folded.append(text) + continue + entry = { + "message": text, + "unresolved": c.get("severity") in ("critical", "high"), + } + # ponytail: single-line placement only (line = end_line); the range + # form is E2E-gated and deliberately left out of v1. + line = c.get("end_line") or 0 + if line > 0: + entry["line"] = line + grouped.setdefault(path, []).append(entry) + + if comments: + inline = sum(len(v) for v in grouped.values()) + summary = "OpenCodeReview found %d issue(s)" % len(comments) + files_reviewed = (result.get("summary") or {}).get("files_reviewed") or 0 + if files_reviewed: + summary += " in %d file(s) reviewed" % files_reviewed + summary += "; %d posted as inline comment(s)." % inline + else: + summary = "OpenCodeReview: " + ( + result.get("message") or "No comments generated. Looks good to me." + ) + if warnings: + summary += "\n\n%d warning(s) occurred during review." % len(warnings) + for text in folded: + summary += "\n\n---\n\n" + text + + review = { + "message": truncate_message(summary), + "tag": "autogenerated:opencodereview", + "notify": "OWNER", + "omit_duplicate_comments": True, + # Old Gerrit servers defaulted to deleting the caller's draft comments + # on set-review; KEEP is a valid enum everywhere and protects humans + # running this with personal credentials. + "drafts": "KEEP", + } + if grouped: + # Plain `comments` (CommentInput), not `robot_comments`/RobotCommentInput: + # robot comments are deprecated since Gerrit 3.6, disabled-by-default in + # 3.12, and slated for removal; the `tag: autogenerated:...` above already + # marks these as bot-generated. + review["comments"] = grouped + return review + + +def fold_comments(review_input): + """Rebuild a ReviewInput with all inline comments folded into the summary. + + Used when Gerrit rejects the batch (HTTP 400, e.g. a line outside the + file): the findings still reach the change as one message. + """ + folded = {k: v for k, v in review_input.items() if k != "comments"} + # The reused summary still claims "N posted as inline comment(s)." from + # build_review_input; drop that clause so the message doesn't say the + # comments were posted inline and then explain they couldn't be placed. + summary = INLINE_CLAUSE_RE.sub(".", review_input.get("message", "")) + parts = [ + summary, + "\nInline comments could not be placed; findings follow:", + ] + for path, entries in (review_input.get("comments") or {}).items(): + for e in entries: + loc = "`%s:%d`" % (path, e["line"]) if "line" in e else "`%s`" % path + parts.append("\n---\n\n%s\n\n%s" % (loc, e["message"])) + full = "\n".join(parts) + if len(full) > MAX_MESSAGE_LEN: + log("warning: folded summary truncated (%d of %d chars dropped)" + % (len(full) - MAX_MESSAGE_LEN + len(TRUNCATION_MARKER), len(full))) + folded["message"] = truncate_message(full) + return folded + + +def strip_xssi(text): + """Strip Gerrit's ")]}'" anti-XSSI prefix (possibly stacked) from a response.""" + text = text.lstrip("\n") + while text.startswith(XSSI_PREFIX): + text = text[len(XSSI_PREFIX):].lstrip("\n") + return text + + +def build_endpoint(base, change, revision): + """Join base URL (context path kept, trailing slash tolerated) and route.""" + return "%s/a/changes/%s/revisions/%s/review" % ( + str(base).rstrip("/"), + change, + revision or DEFAULT_REVISION, + ) + + +def derive_base_url(change_url): + """Derive the Gerrit base URL from a change URL. + + Modern URLs ({base}/c/{project}/+/{number}) split at "/c/", which keeps + any context path; legacy URLs ({base}/{number}) drop the trailing number. + """ + url = change_url.rstrip("/") + if "/c/" in url: + return url.split("/c/")[0] + return re.sub(r"/\d+$", "", url) + + +# --------------------------------------------------------------------------- # +# Transport (urllib only) +# --------------------------------------------------------------------------- # + + +def make_poster(url, user, password, timeout): + """Return post(review_input) that POSTs to the Gerrit set-review endpoint. + + Auth is preemptive basic auth; the parsed (XSSI-stripped) response JSON is + returned. A response that does not parse as JSON raises ValueError -- a + 2xx with an HTML body means the request never reached the REST API. + """ + credentials = base64.b64encode( + ("%s:%s" % (user, password)).encode("utf-8") + ).decode("ascii") + + def post(review_input): + body = json.dumps(review_input, ensure_ascii=False).encode("utf-8") + # Bounded retry, scoped to the provably-safe failures only. A read + # timeout is deliberately NOT retried: it is ambiguous (the server may + # have applied the review), and omit_duplicate_comments dedupes only + # byte-identical *inline* comments, not the summary `message`, so + # retrying an ambiguous request risks posting a duplicate change + # message. depot_tools' gerrit_util.py retries hard (TRY_LIMIT=6); we + # retry only server errors and pre-response connection failures. + for attempt in range(MAX_ATTEMPTS): + req = urllib.request.Request(url, data=body, method="POST") + # unredirected: credentials must not follow redirects to other hosts + # (urllib forwards ordinary headers cross-host on redirect). + req.add_unredirected_header("Authorization", "Basic " + credentials) + req.add_header("Content-Type", "application/json; charset=UTF-8") + try: + with urllib.request.urlopen(req, timeout=timeout) as resp: + raw = resp.read().decode("utf-8", "replace") + return json.loads(strip_xssi(raw)) + except urllib.error.HTTPError as e: + # Retry only 5xx (the review was not applied). 4xx + # (400/401/404/409) are classified by main() and must propagate + # unchanged. + if e.code < 500 or attempt == MAX_ATTEMPTS - 1: + raise + except urllib.error.URLError as e: + # Retry pre-response connection errors (refused/reset/DNS), but + # NOT a connection-phase timeout wrapped as URLError -- see the + # ambiguity note above. (A read timeout raises a bare + # socket.timeout/TimeoutError, which is not caught here and so + # already propagates without retry.) + if isinstance(e.reason, (socket.timeout, TimeoutError)): + raise + if attempt == MAX_ATTEMPTS - 1: + raise + _sleep(0.5 * (2 ** attempt)) + + return post + + +def make_dry_run_poster(): + """Return post(review_input) that prints instead of calling Gerrit.""" + + def post(review_input): + print(json.dumps(review_input, ensure_ascii=False, indent=2)) + return {} + + return post + + +# --------------------------------------------------------------------------- # +# CLI +# --------------------------------------------------------------------------- # + + +def load_review_result(path): + """Read the JSON produced by `ocr review --format json` (path '-' = stdin).""" + if path == "-": + data = sys.stdin.read() + else: + with open(path, encoding="utf-8") as f: + data = f.read() + result = json.loads(data) + if not isinstance(result, dict): + raise ValueError("expected a JSON object") + return result + + +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 read_error_body(e): + try: + return e.read(512).decode("utf-8", "replace").strip() + except Exception: # noqa: BLE001 - body is best-effort diagnostics only + return "" + + +def positive_float(value): + f = float(value) + if f <= 0: + raise argparse.ArgumentTypeError("timeout must be > 0") + return f + + +def parse_args(argv): + p = argparse.ArgumentParser( + description="Post `ocr review --format json` output onto a Gerrit change." + ) + p.add_argument("input", nargs="?", default=None, + help="review result JSON path (same as --input; wins if both given)") + p.add_argument("--gerrit-url", default="", + help="Gerrit base URL (default: $GERRIT_URL, or derived from $GERRIT_CHANGE_URL)") + p.add_argument("--change", default="", + help="change number (default: $GERRIT_CHANGE_NUMBER)") + p.add_argument("--revision", default="", + help="revision/patchset (default: $GERRIT_PATCHSET_REVISION or 'current')") + p.add_argument("--user", default="", + help="HTTP credentials username (default: $GERRIT_HTTP_USER)") + p.add_argument("--password", default="", + help="Gerrit HTTP password, NOT the account password (default: $GERRIT_HTTP_PASSWORD)") + p.add_argument("--input", dest="input_flag", default="-", + help="review result JSON ('-' = stdin, default)") + p.add_argument("--dry-run", action="store_true", + help="print the ReviewInput instead of posting it") + p.add_argument("--timeout", type=positive_float, default=30.0, + help="HTTP timeout in seconds (default: 30)") + return p.parse_args(argv) + + +def main(argv=None): + args = parse_args(sys.argv[1:] if argv is None else argv) + env = os.environ.get + + base_url = args.gerrit_url or env("GERRIT_URL", "") + if not base_url and env("GERRIT_CHANGE_URL", ""): + base_url = derive_base_url(env("GERRIT_CHANGE_URL", "")) + change = args.change or env("GERRIT_CHANGE_NUMBER", "") + revision = args.revision or env("GERRIT_PATCHSET_REVISION", "") or DEFAULT_REVISION + user = args.user or env("GERRIT_HTTP_USER", "") + password = args.password or env("GERRIT_HTTP_PASSWORD", "") + # Positional path wins over --input (gitflic parity: its script is positional). + input_path = args.input if args.input is not None else args.input_flag + + if not args.dry_run: + missing = [name for name, value in ( + ("GERRIT_URL", base_url), + ("GERRIT_CHANGE_NUMBER", change), + ("GERRIT_HTTP_USER", user), + ("GERRIT_HTTP_PASSWORD", password), + ) if not value] + if missing: + log("error: missing required %s (set via flag or environment)" + % ", ".join(missing)) + return 2 + + try: + result = load_review_result(input_path) + except (OSError, ValueError) as e: + log("error: cannot read review result %s: %s" % (input_path, e)) + return 1 + + review_input = build_review_input(result) + + if args.dry_run: + make_dry_run_poster()(review_input) + return 0 + + endpoint = build_endpoint(base_url, change, revision) + post = make_poster(endpoint, user, password, args.timeout) + try: + post(review_input) + except urllib.error.HTTPError as e: + body = scrub(read_error_body(e), password, user) + if e.code in (401, 403): + log("error: Gerrit rejected the credentials (HTTP %d). Use the " + "HTTP password from Settings > HTTP Credentials, not the " + "account password: %s" % (e.code, body)) + return 2 + if e.code == 404: + log("error: change %s not found at %s (HTTP 404). Check the change " + "number and GERRIT_URL (a proxy stripping the /a/ prefix also " + "causes this): %s" % (change, endpoint, body)) + return 2 + if e.code == 409: + log("warning: change %s is closed/abandoned (HTTP 409); nothing " + "posted: %s" % (change, body)) + return 0 + if e.code == 400 and "comments" in review_input: + log("warning: Gerrit rejected the batched comments (HTTP 400); " + "retrying with findings folded into the summary: %s" % body) + try: + post(fold_comments(review_input)) + except (urllib.error.HTTPError, urllib.error.URLError, ValueError) as e2: + log("error: fallback post failed: %s" % scrub(str(e2), password, user)) + return 2 + print("Posted review to change %s (inline comments folded into " + "summary)." % change) + return 0 + else: + log("error: Gerrit API HTTP %d: %s" % (e.code, body)) + return 2 + except urllib.error.URLError as e: + log("error: cannot reach Gerrit at %s (check GERRIT_URL): %s" + % (endpoint, scrub(str(e.reason), password, user))) + return 2 + except ValueError as e: + log("error: response from %s was not Gerrit JSON; check the GERRIT_URL " + "scheme (http vs https) and any redirects: %s" + % (endpoint, scrub(str(e), password, user))) + return 2 + + total = len(result.get("comments") or []) + print("Posted review with %d comment(s) to change %s." % (total, change)) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/examples/gerrit_ci/post_review_test.py b/examples/gerrit_ci/post_review_test.py new file mode 100644 index 00000000..56ad327e --- /dev/null +++ b/examples/gerrit_ci/post_review_test.py @@ -0,0 +1,623 @@ +#!/usr/bin/env python3 +"""Tests for post_review.py. + +Standard-library unittest only (no pytest, no network, no live Gerrit): run with + + python3 post_review_test.py # from examples/gerrit_ci/ + python3 -m unittest examples/gerrit_ci/post_review_test.py -v # from the repo root + python3 -m unittest discover -s examples/gerrit_ci -p '*_test.py' + +The transport tests drive main() through a Recorder fake poster, so the whole +flag/env/error-handling flow is exercised without any HTTP. +""" + +import base64 +import contextlib +import io +import json +import os +import socket +import sys +import tempfile +import unittest +import urllib.error +from unittest import mock + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +import post_review as pr # noqa: E402 + + +def comment(**overrides): + """One OCR comment as `ocr review --format json` emits it.""" + c = { + "path": "main.go", + "content": "possible nil dereference", + "start_line": 6, + "end_line": 6, + } + c.update(overrides) + return c + + +def build(comments, **extra): + result = dict(extra) + result["comments"] = comments + return pr.build_review_input(result) + + +def entry_of(ri, path="main.go"): + return ri["comments"][path][0] + + +class BuildReviewInputTest(unittest.TestCase): + # ---- line mapping (never a "range" key in v1) ---- + + def assert_line(self, c, want_line): + entry = entry_of(build([c])) + self.assertNotIn("range", entry) + if want_line is None: + self.assertNotIn("line", entry) + else: + self.assertEqual(entry["line"], want_line) + + def test_single_line_comment(self): + self.assert_line(comment(start_line=6, end_line=6), 6) + + def test_multi_line_range(self): + self.assert_line(comment(start_line=3, end_line=6), 6) + + def test_file_level_comment(self): + self.assert_line(comment(start_line=0, end_line=0), None) + + def test_missing_line_keys(self): + self.assert_line({"path": "main.go", "content": "no lines"}, None) + + def test_start_gt_end_degenerate(self): + self.assert_line(comment(start_line=8, end_line=3), 3) + + def test_negative_start_line(self): + self.assert_line(comment(start_line=-1, end_line=6), 6) + + def test_missing_path(self): + ri = build([comment(path="", content="orphan finding")]) + self.assertNotIn("comments", ri) + self.assertIn("orphan finding", ri["message"]) + + # ---- severity -> unresolved ---- + + def assert_unresolved(self, severity, want): + c = comment() + if severity is not None: + c["severity"] = severity + self.assertIs(entry_of(build([c]))["unresolved"], want) + + def test_severity_critical_unresolved(self): + self.assert_unresolved("critical", True) + + def test_severity_high_unresolved(self): + self.assert_unresolved("high", True) + + def test_severity_medium_resolved(self): + self.assert_unresolved("medium", False) + + def test_severity_low_resolved(self): + self.assert_unresolved("low", False) + + def test_severity_missing_resolved(self): + self.assert_unresolved(None, False) + + # ---- message formatting ---- + + def test_severity_category_line(self): + msg = entry_of(build([comment(severity="high", category="security")]))["message"] + lines = msg.split("\n") + self.assertEqual(lines[0], "**Severity:** high · **Category:** security") + self.assertEqual(lines[1], "") + self.assertIn("possible nil dereference", msg) + + def test_severity_line_omitted_when_unset(self): + msg = entry_of(build([comment()]))["message"] + self.assertTrue(msg.startswith("possible nil dereference")) + self.assertNotIn("**Severity:**", msg) + self.assertNotIn("**Category:**", msg) + + def test_suggestion_block(self): + msg = entry_of(build([comment( + existing_code="x := y.Field", + suggestion_code="if y != nil { x = y.Field }", + )]))["message"] + self.assertIn("**Suggestion:**\n```\nif y != nil { x = y.Field }\n```", msg) + + def test_suggestion_without_existing(self): + msg = entry_of(build([comment(suggestion_code="if y != nil {}")]))["message"] + self.assertNotIn("**Suggestion:**", msg) + + def test_unicode_comment_roundtrip(self): + content = "空指针解引用：y 可能为 nil" + ri = build([comment(path="pkg/服务.go", content=content)]) + self.assertIn("pkg/服务.go", ri["comments"]) + self.assertIn(content, entry_of(ri, "pkg/服务.go")["message"]) + self.assertEqual(json.loads(json.dumps(ri, ensure_ascii=False)), ri) + + def test_path_with_spaces(self): + ri = build([comment(path="docs/my file.go")]) + self.assertIn("docs/my file.go", ri["comments"]) + self.assertNotIn("docs/my%20file.go", ri["comments"]) + + def test_very_long_message_truncated(self): + msg = entry_of(build([comment(content="x" * 20000)]))["message"] + self.assertLessEqual(len(msg), 16000) + self.assertIn("[truncated]", msg) + + # ---- batching & summary ---- + + def test_multiple_files_grouped(self): + ri = build([ + comment(path="a.go", end_line=1), + comment(path="a.go", end_line=2), + comment(path="b.go", end_line=3), + ]) + self.assertEqual(len(ri["comments"]["a.go"]), 2) + self.assertEqual(len(ri["comments"]["b.go"]), 1) + + def test_batch_metadata(self): + ri = build([comment(path="a.go"), comment(path="b.go"), comment(path="c.go")]) + self.assertEqual(ri["tag"], "autogenerated:opencodereview") + self.assertEqual(ri["notify"], "OWNER") + self.assertIs(ri["omit_duplicate_comments"], True) + self.assertEqual(ri["drafts"], "KEEP") + self.assertIn("3", ri["message"]) + + def test_empty_comments_summary_only(self): + ri = build([]) + self.assertNotIn("comments", ri) + self.assertIn("Looks good", ri["message"]) + + def test_result_message_only(self): + ri = pr.build_review_input({"message": "No comments generated. Looks good to me."}) + self.assertNotIn("comments", ri) + self.assertIn("Looks good to me", ri["message"]) + + def test_warnings_in_summary(self): + ri = pr.build_review_input({ + "comments": [comment()], + "warnings": [{"file": "a.go", "message": "skipped", "type": "subtask_error"}], + }) + self.assertIn("1 warning(s)", ri["message"]) + self.assertIn("comments", ri) + + def test_warnings_only_no_comments(self): + ri = pr.build_review_input({ + "comments": [], + "warnings": [{"file": "a.go", "message": "skipped", "type": "subtask_error"}], + }) + self.assertNotIn("comments", ri) + self.assertIn("1 warning(s)", ri["message"]) + + +class StripXssiTest(unittest.TestCase): + def test_strip_xssi(self): + cases = [ + ("prefix_with_newline", ")]}'\n{\"a\": 1}"), + ("bare_prefix", ")]}'{\"a\": 1}"), + ("no_prefix", "{\"a\": 1}"), + ("stacked_prefix", ")]}'\n)]}'\n{\"a\": 1}"), + ] + for name, raw in cases: + with self.subTest(name): + self.assertEqual(json.loads(pr.strip_xssi(raw)), {"a": 1}) + + +class BuildEndpointTest(unittest.TestCase): + def test_trailing_slash_base(self): + self.assertEqual( + pr.build_endpoint("https://g.example/", 42, "abc"), + "https://g.example/a/changes/42/revisions/abc/review", + ) + + def test_context_path_base(self): + self.assertEqual( + pr.build_endpoint("https://host/gerrit", "42", "abc"), + "https://host/gerrit/a/changes/42/revisions/abc/review", + ) + + def test_revision_default_current(self): + for revision in ("", None): + with self.subTest(revision=revision): + self.assertEqual( + pr.build_endpoint("https://g.example", "42", revision), + "https://g.example/a/changes/42/revisions/current/review", + ) + + def test_url_from_change_url_modern(self): + self.assertEqual(pr.derive_base_url("https://host/c/proj/+/42"), "https://host") + + def test_url_from_change_url_path_prefix(self): + self.assertEqual( + pr.derive_base_url("https://host/gerrit/c/proj/+/42"), "https://host/gerrit" + ) + + def test_url_from_change_url_legacy(self): + self.assertEqual(pr.derive_base_url("https://g.example/42"), "https://g.example") + + +# --------------------------------------------------------------------------- # +# main() driven through a Recorder fake poster +# --------------------------------------------------------------------------- # + +PASSWORD = "s3cret-pass" + +BASE_ENV = { + "GERRIT_URL": "https://gerrit.example", + "GERRIT_CHANGE_NUMBER": "42", + "GERRIT_PATCHSET_REVISION": "abc", + "GERRIT_HTTP_USER": "review-bot", + "GERRIT_HTTP_PASSWORD": PASSWORD, +} + +SAMPLE_RESULT = { + "comments": [ + comment(path="a.go", content="nil deref", start_line=3, end_line=6, + severity="high", category="bug"), + comment(path="a.go", content="shadowed err", start_line=10, end_line=10, + severity="low"), + comment(path="b.go", content="missing test", start_line=1, end_line=2, + category="test"), + ], +} + + +def http_error(code, body=b"", reason="error"): + return urllib.error.HTTPError( + "https://gerrit.example", code, reason, {}, io.BytesIO(body) + ) + + +class Recorder: + """Stands in for make_poster(); replays canned outcomes per call.""" + + def __init__(self, outcomes=None): + self.calls = [] + self.outcomes = list(outcomes or []) + self.url = None + self.user = None + self.password = None + + def factory(self, url, user, password, timeout): + self.url = url + self.user = user + self.password = password + return self + + def __call__(self, review_input): + self.calls.append(review_input) + if self.outcomes: + outcome = self.outcomes.pop(0) + if isinstance(outcome, Exception): + raise outcome + return outcome + return {} + + +class PostTest(unittest.TestCase): + def run_main(self, argv=(), env=BASE_ENV, result=SAMPLE_RESULT, outcomes=None, + input_args=None): + rec = Recorder(outcomes) + f = tempfile.NamedTemporaryFile("w", suffix=".json", delete=False) + json.dump(result, f) + f.close() + self.addCleanup(os.unlink, f.name) + self.input_path = f.name + if input_args is None: + input_args = ["--input", f.name] + else: # "{path}" in input_args stands for the temp result file + input_args = [a.format(path=f.name) for a in input_args] + stdout, stderr = io.StringIO(), io.StringIO() + with mock.patch.dict(os.environ, env, clear=True), \ + mock.patch.object(pr, "make_poster", rec.factory), \ + contextlib.redirect_stdout(stdout), \ + contextlib.redirect_stderr(stderr): + rc = pr.main(list(argv) + input_args) + return rc, rec, stdout.getvalue(), stderr.getvalue() + + def test_single_post_batching(self): + rc, rec, _out, _err = self.run_main() + self.assertEqual(rc, 0) + self.assertEqual(len(rec.calls), 1) + ri = rec.calls[0] + self.assertEqual(sorted(ri["comments"]), ["a.go", "b.go"]) + self.assertEqual(len(ri["comments"]["a.go"]), 2) + dumped = json.dumps(ri, ensure_ascii=False) + for text in ("nil deref", "shadowed err", "missing test"): + self.assertIn(text, dumped) + + def test_auth_failure_401(self): + rc, _rec, out, err = self.run_main(outcomes=[http_error(401, b"Unauthorized")]) + self.assertEqual(rc, 2) + self.assertIn("HTTP password", err) + self.assertNotIn(PASSWORD, out) + self.assertNotIn(PASSWORD, err) + + def test_password_never_in_error_body(self): + body = ("bad credentials: " + PASSWORD).encode("utf-8") + rc, _rec, out, err = self.run_main(outcomes=[http_error(401, body)]) + self.assertEqual(rc, 2) + self.assertIn("***", err) + self.assertNotIn(PASSWORD, out) + self.assertNotIn(PASSWORD, err) + + def test_b64_credentials_never_in_error_body(self): + # Echoed Authorization header: base64(user:password) decodes trivially. + creds = base64.b64encode( + ("review-bot:" + PASSWORD).encode("utf-8") + ).decode("ascii") + body = ("echoed header: Basic " + creds).encode("utf-8") + rc, _rec, out, err = self.run_main(outcomes=[http_error(401, body)]) + self.assertEqual(rc, 2) + self.assertIn("***", err) + self.assertNotIn(creds, out) + self.assertNotIn(creds, err) + + def test_not_found_404(self): + rc, _rec, _out, err = self.run_main(outcomes=[http_error(404, b"Not found")]) + self.assertEqual(rc, 2) + self.assertIn("42", err) + self.assertIn("/a/", err) + + def test_change_closed_409(self): + rc, _rec, _out, err = self.run_main(outcomes=[http_error(409, b"change is closed")]) + self.assertEqual(rc, 0) + self.assertIn("warning", err.lower()) + + def test_bad_request_400_falls_back(self): + rc, rec, _out, _err = self.run_main( + outcomes=[http_error(400, b"invalid line"), {}] + ) + self.assertEqual(rc, 0) + self.assertEqual(len(rec.calls), 2) + fallback = rec.calls[1] + self.assertNotIn("comments", fallback) + self.assertIn("nil deref", fallback["message"]) + # The folded message must not claim comments were posted inline while + # explaining they couldn't be placed (see fold_comments). + self.assertNotIn("posted as inline comment(s)", fallback["message"]) + + def test_non_gerrit_response_body(self): + rc, _rec, _out, err = self.run_main( + outcomes=[ValueError("body was 'Sign in'")] + ) + self.assertEqual(rc, 2) + self.assertIn("GERRIT_URL", err) + self.assertIn("scheme", err) + + def test_timeout_urlerror(self): + rc, _rec, _out, err = self.run_main(outcomes=[urllib.error.URLError("timed out")]) + self.assertEqual(rc, 2) + self.assertIn("GERRIT_URL", err) + self.assertNotIn("Traceback", err) + + def test_dry_run_no_credentials(self): + rc, rec, out, _err = self.run_main(argv=["--dry-run"], env={}) + self.assertEqual(rc, 0) + self.assertEqual(rec.calls, []) + self.assertIsNone(rec.url) + ri = json.loads(out) + self.assertEqual(ri["tag"], "autogenerated:opencodereview") + + def test_env_resolution(self): + rc, rec, _out, _err = self.run_main() + self.assertEqual(rc, 0) + self.assertEqual(rec.url, "https://gerrit.example/a/changes/42/revisions/abc/review") + + def test_missing_required_env(self): + env = dict(BASE_ENV) + del env["GERRIT_CHANGE_NUMBER"] + rc, rec, _out, err = self.run_main(env=env) + self.assertEqual(rc, 2) + self.assertIn("GERRIT_CHANGE_NUMBER", err) + self.assertEqual(rec.calls, []) + + def test_stdin_input(self): + rec = Recorder() + stdout, stderr = io.StringIO(), io.StringIO() + with mock.patch.dict(os.environ, BASE_ENV, clear=True), \ + mock.patch.object(pr, "make_poster", rec.factory), \ + mock.patch.object(sys, "stdin", io.StringIO(json.dumps(SAMPLE_RESULT))), \ + contextlib.redirect_stdout(stdout), \ + contextlib.redirect_stderr(stderr): + rc = pr.main(["--input", "-"]) + self.assertEqual(rc, 0) + self.assertEqual(len(rec.calls), 1) + self.assertIn("Posted review", stdout.getvalue()) + + def test_flags_override_env(self): + rc, rec, _out, _err = self.run_main(argv=[ + "--gerrit-url", "https://flag.example", + "--change", "7", + "--revision", "def", + "--user", "flag-user", + "--password", "flag-pass", + ]) + self.assertEqual(rc, 0) + self.assertEqual(rec.url, "https://flag.example/a/changes/7/revisions/def/review") + self.assertEqual(rec.user, "flag-user") + self.assertEqual(rec.password, "flag-pass") + + def test_fold_retry_also_fails(self): + rc, rec, _out, err = self.run_main( + outcomes=[http_error(400, b"invalid line"), http_error(500, b"boom")] + ) + self.assertEqual(rc, 2) + self.assertEqual(len(rec.calls), 2) + self.assertIn("fallback post failed", err) + + def test_fold_truncation_and_message(self): + result = {"comments": [comment(path="a.go", content="x" * 20000)]} + rc, rec, out, err = self.run_main( + result=result, outcomes=[http_error(400, b"invalid line"), {}] + ) + self.assertEqual(rc, 0) + self.assertEqual(len(rec.calls), 2) + self.assertLessEqual(len(rec.calls[1]["message"]), pr.MAX_MESSAGE_LEN) + self.assertIn("folded summary truncated", err) + self.assertIn("folded into summary", out) + self.assertNotIn("Posted review with", out) + + def test_change_url_derivation(self): + env = {k: v for k, v in BASE_ENV.items() if k != "GERRIT_URL"} + env["GERRIT_CHANGE_URL"] = "https://host/gerrit/c/proj/+/42" + rc, rec, _out, _err = self.run_main(env=env) + self.assertEqual(rc, 0) + self.assertEqual(rec.url, "https://host/gerrit/a/changes/42/revisions/abc/review") + + def test_non_dict_json_input(self): + rc, rec, _out, err = self.run_main(result=[]) + self.assertEqual(rc, 1) + self.assertEqual(rec.calls, []) + self.assertIn("cannot read review result", err) + self.assertIn("expected a JSON object", err) + self.assertNotIn("Traceback", err) + + def test_positional_input(self): + rc, rec, _out, _err = self.run_main(input_args=["{path}"]) + self.assertEqual(rc, 0) + self.assertEqual(len(rec.calls), 1) + + def test_positional_input_wins_over_flag(self): + rc, rec, _out, _err = self.run_main( + input_args=["{path}", "--input", "/nonexistent.json"] + ) + self.assertEqual(rc, 0) + self.assertEqual(len(rec.calls), 1) + + +class ParseArgsTest(unittest.TestCase): + def test_timeout_must_be_positive(self): + for value in ("0", "-5"): + with self.subTest(value=value): + stderr = io.StringIO() + with contextlib.redirect_stderr(stderr), \ + self.assertRaises(SystemExit) as cm: + pr.parse_args(["--timeout", value]) + self.assertEqual(cm.exception.code, 2) + self.assertIn("timeout must be > 0", stderr.getvalue()) + + def test_timeout_float_still_parses(self): + self.assertEqual(pr.parse_args(["--timeout", "2.5"]).timeout, 2.5) + + +class FakeResponse: + def __init__(self, body): + self._body = body + + def read(self): + return self._body + + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + +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}}) + + # ---- bounded retry on transient failures ---- + + def post_seq(self, outcomes): + """Drive post() over a sequence of per-call fake_urlopen outcomes. + + Each outcome is either the raw response body (bytes, returned) or an + Exception (raised). Sleeps are stubbed to a no-op so retries are fast. + Returns (calls, result_or_None, raised_or_None). + """ + calls = {"n": 0} + outcomes = list(outcomes) + + def fake_urlopen(req, timeout=None): + calls["n"] += 1 + outcome = outcomes.pop(0) + if isinstance(outcome, Exception): + raise outcome + return FakeResponse(outcome) + + with mock.patch.object(pr.urllib.request, "urlopen", fake_urlopen), \ + mock.patch.object(pr, "_sleep", lambda _s: None): + post = pr.make_poster(self.ENDPOINT, "review-bot", "s3cret-pass", 30) + try: + result = post({"message": "hi"}) + except Exception as e: # noqa: BLE001 - counted and asserted below + return calls["n"], None, e + return calls["n"], result, None + + def test_retry_503_then_200(self): + n, result, raised = self.post_seq([http_error(503, b"boom"), b")]}'\n{}"]) + self.assertIsNone(raised) + self.assertEqual(result, {}) + self.assertEqual(n, 2) + + def test_retry_connection_urlerror_then_200(self): + conn_err = urllib.error.URLError(ConnectionRefusedError("refused")) + n, result, raised = self.post_seq([conn_err, b")]}'\n{}"]) + self.assertIsNone(raised) + self.assertEqual(result, {}) + self.assertEqual(n, 2) + + def test_retry_503_exhausts_and_propagates(self): + n, _result, raised = self.post_seq([http_error(503, b"boom")] * pr.MAX_ATTEMPTS) + self.assertIsInstance(raised, urllib.error.HTTPError) + self.assertEqual(raised.code, 503) + self.assertEqual(n, pr.MAX_ATTEMPTS) + + 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_4xx_not_retried(self): + for code in (400, 409): + with self.subTest(code=code): + n, _result, raised = self.post_seq([http_error(code, b"nope")]) + self.assertIsInstance(raised, urllib.error.HTTPError) + self.assertEqual(raised.code, code) + self.assertEqual(n, 1) + + +if __name__ == "__main__": + unittest.main()