diff --git a/.genai-review.yml.example b/.genai-review.yml.example index 27b62f2..aecb883 100644 --- a/.genai-review.yml.example +++ b/.genai-review.yml.example @@ -4,13 +4,18 @@ # Action inputs in the workflow take precedence over anything here, so a # workflow can always override the repository's defaults. # +# The file is read from the pull request's base branch, not from the pull +# request itself, so a change to this file takes effect once it is merged. +# That is deliberate: otherwise a pull request could configure its own review. +# # There is deliberately no `api_key` key. Credentials belong in a repository -# secret; a key in a file here is a key that gets committed by accident. +# secret; a key in a file here is a key that gets committed by accident. For +# the same reason there is no `base_url`: it decides which server receives the +# key, so it can only be set as an action input. # --- which model reviews the code --------------------------------------- provider: gemini # gemini | openai | anthropic | openai-compatible # model: gemini-3.8-flash # defaults to a cost-conscious model per provider -# base_url: http://localhost:11434/v1 # for openai-compatible only # --- how the review is posted ------------------------------------------- mode: review # review (inline comments) | files | patch (v2 behaviour) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2092494..b22a361 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,8 +16,51 @@ reported separately rather than averaged — a resolved thread means the conversation ended, not that the finding was right. +### Security + +- **`.genai-review.yml` is now read from the pull request's base branch.** It + was read from the working directory, which meant two things. With an + `actions/checkout` step, the file came from the pull request itself, so + whoever opened it could set `ignore_paths: ["**"]` or `max_comments: 0` and + review their own change out of existence, or point `base_url` at their own + server and receive the API key. Without a checkout — which is what the README + recommends — the file was silently never read at all. It is now fetched over + the API from the base commit, so it works without a checkout and a pull + request cannot configure its own review. +- **`base_url` can no longer be set in `.genai-review.yml`.** It decides which + server receives the API key, so it is an action input only. + ### Fixed +- **Files that were not reviewed dropped out of the summary on the next push.** + The last reviewed SHA advanced even when a run skipped files as too large, + truncated the diff, or got unusable output from the model. The next push + diffed from there, so those files were neither reviewed nor listed under + "Not reviewed" — the one promise the summary exists to keep. The SHA now + advances only when a run covered everything, and the summary says the next + push will review the earlier range again. +- **Findings counted as posted when the review failed to post.** If GitHub + rejected the review, its findings were still recorded as posted and never + tried again. They are now listed in the summary instead and retried on the + next push. +- **Incremental review broke after merging the base branch or rebasing.** The + comparison with the last reviewed commit then contains changes that are not + part of the pull request, and GitHub rejects the whole review with a 422 over + a single comment on one of them. The comparison is now checked first: a + rewritten history or a merge commit in the range falls back to reviewing the + whole pull request, and the summary says why. Findings are also placed + against the pull request's own diff, so a file outside it is never reviewed + or commented on. +- **One-click suggestions could land on the wrong lines.** A finding up to + three lines off the diff is moved to the nearest line, or its range is + shortened to fit a hunk, but its `suggestion` was still offered as a one-click + change — so "Commit suggestion" replaced code the suggestion was not written + for. A moved finding now shows its suggestion as a plain code block. +- **The action was listed as "ChatGPT GitHub Actions".** `action.yml` now names + it GenAI Code Review and describes what v3 does. +- **The migration guide said `@v2` was untouched.** It was moved once, to the + fix-only v2.1; the guide and the v3 release notes now say so. + - **The comment budget was unmeasurable.** Precision and recall were computed over every finding the model produced, and the budget changes only what is posted — so a budgeted and an unbudgeted run were identical by construction, @@ -61,7 +104,7 @@ - **`mode` now defaults to `review`** instead of `files`. A workflow that never set `mode` gets inline comments on the diff instead of one long summary comment. Workflows that set `mode` explicitly are unaffected, and the `v2` - tag is untouched — this applies only when the pin moves to `v3`. Pin + tag keeps the `files` default — this applies only when the pin moves to `v3`. Pin `mode: files` to keep the previous output. ### Added diff --git a/README.md b/README.md index 059f1ad..cf5230c 100644 --- a/README.md +++ b/README.md @@ -169,9 +169,15 @@ Every setting can be given as an action input or in a `.genai-review.yml` file at the repository root. **Action inputs take precedence** over the file, so a workflow can always override the repository's defaults. +The file is read over the API from the pull request's **base branch**, not from +the pull request, and needs no checkout. A change to it takes effect once it is +merged. Otherwise whoever opens a pull request could set `ignore_paths: ["**"]` +and review their own change out of existence. + See [`.genai-review.yml.example`](.genai-review.yml.example) for a commented file. There is deliberately no `api_key` key in it: credentials belong in a -secret. +secret. `base_url` is also input-only, because it decides which server receives +the key. ### Reference @@ -194,7 +200,7 @@ secret. | `custom_prompt` | — | Extra instructions for the reviewer | | `temperature` | `0.5` | Sampling temperature | | `max_tokens` | `8192` | Response length cap. Below ~4000 a large review truncates and is lost | -| `config_path` | `.genai-review.yml` | Repository config file | +| `config_path` | `.genai-review.yml` | Repository config file, read from the base branch | | `runlog_path` | `genai-review-runlog.json` | Where the per-run JSON log is written | `ignore_paths` defaults to skipping lockfiles, vendored and generated code, @@ -233,7 +239,11 @@ diff; its only output is findings validated against a schema. **API keys.** Pass them from a secret. The key is never written to the run log, never included in an error message posted to a pull request, and never read from -`.genai-review.yml`. +`.genai-review.yml`. Neither is `base_url`, the server the key is sent to. + +**The config file cannot be changed by the pull request it configures.** +`.genai-review.yml` is read from the base branch, so a pull request that edits it +is reviewed under the settings already merged. **What leaves your repository.** The diff of the files being reviewed is sent to whichever provider you configure. If that is not acceptable for a given diff --git a/action.yml b/action.yml index 3ae6b22..f5eaf5c 100644 --- a/action.yml +++ b/action.yml @@ -1,5 +1,5 @@ -name: "ChatGPT GitHub Actions" -description: "This GitHub Action automates code review for pull requests using OpenAI's GPT models. By integrating this action into your CI/CD pipeline, it provides detailed, AI-generated reviews of your code directly within the PR comments. This tool helps maintain code quality, ensures adherence to best practices, and highlights potential improvements and bugs." +name: "GenAI Code Review" +description: "Reviews pull request diffs with an LLM (Gemini, OpenAI, Anthropic or any OpenAI-compatible server) and posts only the few inline comments most worth a human's attention, saying what it held back and what it did not review." inputs: provider: description: "LLM provider: gemini (default), openai, anthropic, or openai-compatible. A workflow that sets the deprecated openai_api_key and no provider stays on openai." @@ -81,7 +81,7 @@ inputs: required: false default: "" config_path: - description: "Repository config file. Action inputs take precedence over it. Default .genai-review.yml." + description: "Repository config file, read from the pull request's base branch (never from the pull request itself). Action inputs take precedence over it. Default .genai-review.yml." required: false default: "" runlog_path: diff --git a/docs/migrating-v2-to-v3.md b/docs/migrating-v2-to-v3.md index 0054d9c..33a0ed1 100644 --- a/docs/migrating-v2-to-v3.md +++ b/docs/migrating-v2-to-v3.md @@ -38,9 +38,11 @@ To keep the v2 behaviour, pin it: mode: files # <- keeps the single-comment output ``` -Note that `@v2` itself is untouched. A workflow pinned to `cirolini/genai-code-review@v2` -keeps the `files` default and keeps behaving exactly as it does today; only -moving the pin to `@v3` changes anything. +`@v2` keeps the v2 behaviour. It was moved once, on 2026-09-19, to +[v2.1](../CHANGELOG.md#v21--2026-09-19): a maintenance release that made v2 +start again after it had stopped working everywhere, with no new inputs and the +same `files` default. A workflow pinned to `cirolini/genai-code-review@v2` keeps +the single-comment output; only moving the pin to `@v3` changes it. ### Also check your model diff --git a/docs/release-notes-v3.md b/docs/release-notes-v3.md index 2147422..5c4669c 100644 --- a/docs/release-notes-v3.md +++ b/docs/release-notes-v3.md @@ -11,7 +11,8 @@ https://github.com/cirolini/genai-code-review/releases/tag/v3.0.0 tags are independent and not aligned. - **`mode` defaults to `review`.** A major version is the right moment to make v3 do what v3 is for. Workflows that set `mode` explicitly are unaffected; - `@v2` is untouched and keeps the `files` default. + `@v2` keeps the `files` default. (It points at v2.1, a fix-only release + from the same day; see the changelog.) - **`provider` defaults to `gemini`.** It is the only configuration with measured numbers behind it — 100% precision, 93% recall, 0% noise, $0.0009 per pull request. A workflow passing the deprecated `openai_api_key` without @@ -22,8 +23,9 @@ https://github.com/cirolini/genai-code-review/releases/tag/v3.0.0 ### Marketplace The existing listing is named "ChatGPT GitHub Actions", which no longer -describes the action. Updating it is a separate, explicit step and has not been -done. +describes the action. `action.yml` was renamed to "GenAI Code Review" after +v3.0.0; the listing itself changes only when a release is published to the +Marketplace, which is a separate, explicit step and has not been done. ### Two adapters remain unverified against a live API @@ -51,7 +53,8 @@ never set `mode` gets inline comments instead of one summary comment — pin though a workflow passing the deprecated `openai_api_key` without a `provider` stays on OpenAI. -Every v2 input still works, and `@v2` itself is untouched. See +Every v2 input still works, and `@v2` keeps the v2 behaviour (it points at the +fix-only v2.1). See [the migration guide](migrating-v2-to-v3.md). #### Multiple providers diff --git a/src/clients/github_client.py b/src/clients/github_client.py index 3bdd86b..ad64e12 100644 --- a/src/clients/github_client.py +++ b/src/clients/github_client.py @@ -9,7 +9,7 @@ import os import requests -from github import Github +from github import Github, UnknownObjectException logger = logging.getLogger(__name__) @@ -118,6 +118,35 @@ def get_compare_patch(self, base_sha, head_sha): logger.info("Retrieved diff %s..%s", base_sha[:7], head_sha[:7]) return response.text + def get_compare(self, base_sha, head_sha): + """ + The comparison between two commits, as JSON: its `status` and commits. + + Read before trusting an incremental diff. A force-push leaves the two + commits `diverged`, and a merge from the base branch puts commits in the + range that are not part of the pull request. + """ + url = f"{self.api_root}/repos/{self.repo_name}/compare/{base_sha}...{head_sha}" + response = requests.get(url, headers=self._api_headers(), timeout=60) + response.raise_for_status() + return response.json() + + def get_base_file(self, pr_id, path): + """ + A file's text on the pull request's base commit, or None if absent. + + Configuration is read from here rather than from the pull request's own + branch, because whoever opens a pull request controls its branch. + """ + base_sha = self.get_pr(pr_id).base.sha + try: + content = self.repo.get_contents(path, ref=base_sha) + except UnknownObjectException: + return None + if isinstance(content, list): # a directory, not a file + return None + return content.decoded_content.decode("utf-8") + def create_review(self, pr_id, comments, body): """ Post one review containing every inline comment. diff --git a/src/findings.py b/src/findings.py index 9821549..c712d8a 100644 --- a/src/findings.py +++ b/src/findings.py @@ -37,6 +37,10 @@ class Finding: title: str rationale: str suggestion: str | None = None + # Set when placement moved or shortened the model's line range to fit the + # diff. The suggestion was written for the original lines, so it must not + # be offered as a one-click change on the new ones. + relocated: bool = False def to_dict(self) -> dict: return asdict(self) diff --git a/src/main.py b/src/main.py index 57ed53e..d656cf7 100644 --- a/src/main.py +++ b/src/main.py @@ -12,7 +12,7 @@ from panel import parse_panel from paths import parse_ignore_paths from providers import ProviderError, build_provider -from repo_config import load as load_repo_config +from repo_config import load_from_base as load_repo_config from repo_config import resolve as resolve_setting from review_run import ReviewSettings, execute from runlog import write as write_runlog @@ -32,14 +32,13 @@ def main(): review that never happened. """ try: - env_vars = get_env_vars() + github_client = GithubClient(get_env_variable("GITHUB_TOKEN", required=True)) + pr_id = _as_int("GITHUB_PR_ID", get_env_variable("GITHUB_PR_ID", required=True)) + env_vars = get_env_vars(load_repo_config(github_client, pr_id)) except ValueError as e: logger.error("Environment variable error: %s", e) raise - github_client = GithubClient(env_vars["GITHUB_TOKEN"]) - pr_id = env_vars["GITHUB_PR_ID"] - try: provider = build_provider( env_vars["PROVIDER"], @@ -108,10 +107,13 @@ def report_provider_failure(github_client, pr_id, error): logger.exception("Could not post the failure comment; original error follows") -def get_env_vars(): +def get_env_vars(config=None): """ Read configuration from the environment and `.genai-review.yml`. + `config` is the repository config file, already read from the pull + request's base commit by the caller. None means there is none. + Precedence is action input, then repository config file, then default. The workflow is the more specific statement of intent, and a file in the repository should not silently override what a workflow asked for. @@ -123,7 +125,7 @@ def get_env_vars(): Raises: ValueError: if a required variable is missing or cannot be converted. """ - config = load_repo_config() + config = config or {} def setting(name, key, default=None): return resolve_setting(get_env_variable(name, required=False), config, key, default) @@ -135,7 +137,8 @@ def setting(name, key, default=None): "LANGUAGE": setting("LANGUAGE", "language", "en"), "CUSTOM_PROMPT": setting("CUSTOM_PROMPT", "custom_prompt"), "PROVIDER": _resolve_provider(setting("PROVIDER", "provider", None)), - "BASE_URL": setting("BASE_URL", "base_url") or None, + # Never from the config file: it decides where the API key is sent. + "BASE_URL": get_env_variable("BASE_URL", required=False) or None, "MIN_SEVERITY": str(setting("MIN_SEVERITY", "min_severity", "nit")).strip().lower(), "INCREMENTAL": _as_bool(setting("INCREMENTAL", "incremental", True), True), "PANEL": _as_panel(setting("PANEL", "panel")), diff --git a/src/repo_config.py b/src/repo_config.py index 0f70614..e55787d 100644 --- a/src/repo_config.py +++ b/src/repo_config.py @@ -9,9 +9,15 @@ specific statement of intent and because a repository should not be able to silently override what a workflow explicitly asked for. -The file is parsed with PyYAML's safe loader. It is repository content, so it -is untrusted in the same way the diff is: only known keys are read, values are -coerced to the expected types, and anything unrecognised is ignored with a +The file is read from the pull request's **base** commit over the API, never +from the pull request's own branch or from disk. Whoever opens a pull request +controls its branch, and a config file read from there would let them set +`ignore_paths: ["**"]` or `max_comments: 0` and review their own change out of +existence. Reading over the API also means the file works without an +`actions/checkout` step, which this action does not need. + +The file is parsed with PyYAML's safe loader, only known keys are read, values +are coerced to the expected types, and anything unrecognised is ignored with a warning rather than passed through. """ @@ -23,8 +29,7 @@ DEFAULT_CONFIG_PATH = ".genai-review.yml" # Only these keys are read. Anything else in the file is ignored. -STRING_KEYS = ("provider", "model", "base_url", "mode", "language", "custom_prompt", - "min_severity") +STRING_KEYS = ("provider", "model", "mode", "language", "custom_prompt", "min_severity") INT_KEYS = ("max_tokens", "max_comments") FLOAT_KEYS = ("temperature", "min_confidence") BOOL_KEYS = ("incremental",) @@ -37,39 +42,74 @@ # accident. FORBIDDEN_KEYS = {"api_key", "openai_api_key", "github_token", "token"} +# Also absent: base_url. It decides which server receives the API key, so it +# belongs in the workflow next to the key, not in a file that a change to the +# repository can edit. +WORKFLOW_ONLY_KEYS = {"base_url"} + def load(path: str | None = None) -> dict: """ - Read `.genai-review.yml` if it exists. + Read a config file from disk. Used by the tests and the evals. - Returns an empty dict when the file is missing, empty, unparseable, or not - a mapping. A broken config file should not fail somebody's pull request — - it should be reported and ignored. + The action itself calls `load_from_base`; see the module docstring for why. """ - path = path or os.getenv("CONFIG_PATH") or DEFAULT_CONFIG_PATH + path = path or DEFAULT_CONFIG_PATH if not os.path.exists(path): return {} + try: + with open(path, encoding="utf-8") as handle: + text = handle.read() + except OSError: + logger.exception("Could not read %s; ignoring it", path) + return {} + return parse(text, path) + + +def load_from_base(github_client, pr_id, path: str | None = None) -> dict: + """ + Read the config file as it is on the pull request's base commit. + + Returns an empty dict when the file is absent or cannot be fetched. A + missing or unreadable config file should not fail somebody's pull request. + """ + path = path or os.getenv("CONFIG_PATH") or DEFAULT_CONFIG_PATH + try: + text = github_client.get_base_file(pr_id, path) + except Exception: + logger.exception("Could not read %s from the base branch; ignoring it", path) + return {} + if text is None: + return {} + return parse(text, f"{path} (base branch)") + +def parse(text: str, source: str = DEFAULT_CONFIG_PATH) -> dict: + """ + Parse config text into known, correctly typed settings. + + Returns an empty dict when the text is empty, unparseable, or not a + mapping, logging why. + """ try: import yaml except ImportError: - logger.warning("PyYAML is unavailable; ignoring %s", path) + logger.warning("PyYAML is unavailable; ignoring %s", source) return {} try: - with open(path, encoding="utf-8") as handle: - raw = yaml.safe_load(handle) + raw = yaml.safe_load(text) except Exception: - logger.exception("Could not parse %s; ignoring it", path) + logger.exception("Could not parse %s; ignoring it", source) return {} if raw is None: return {} if not isinstance(raw, dict): - logger.warning("%s must contain a mapping at the top level; ignoring it", path) + logger.warning("%s must contain a mapping at the top level; ignoring it", source) return {} - return _coerce(raw, path) + return _coerce(raw, source) def _coerce(raw: dict, path: str) -> dict: @@ -87,6 +127,15 @@ def _coerce(raw: dict, path: str) -> dict: ) continue + if name in WORKFLOW_ONLY_KEYS: + logger.warning( + "%s sets `%s`, which is ignored. It decides where the API key is " + "sent, so it can only be set as an action input.", + path, + name, + ) + continue + if name not in KNOWN_KEYS: logger.warning("%s sets unknown key `%s`; ignoring it", path, name) continue diff --git a/src/review_run.py b/src/review_run.py index 4e2a37a..82ff04b 100644 --- a/src/review_run.py +++ b/src/review_run.py @@ -61,6 +61,13 @@ class RunReport: repaired: bool = False panel = None results: list = field(default_factory=list) + # Why an incremental review fell back to the whole pull request, if it did. + full_review_reason: str | None = None + # The inline comments were rejected by GitHub; the summary lists them instead. + post_failed: bool = False + # The SHA the next run will diff from. It only advances past this run's + # head when this run actually covered everything and delivered it. + next_base_sha: str | None = None @property def fully_reviewed(self) -> bool: @@ -79,13 +86,19 @@ def execute(github_client, build_member, pr_id, settings: ReviewSettings) -> Run previous = sticky.previous_run(github_client, pr_id) report.head_sha = github_client.get_pr_head_sha(pr_id) - patch, report.base_sha, report.incremental = _select_diff( + patch, report.base_sha, report.incremental, report.full_review_reason = _select_diff( github_client, pr_id, previous, settings, report.head_sha ) parsed = parse_diff(patch) + # Comments must land on lines of the pull request's own diff: GitHub + # rejects the whole review over a single line outside it. An incremental + # diff is checked against the full one, so that anything it carries that is + # not part of the pull request can neither be reviewed nor commented on. + placement = parse_diff(github_client.get_pr_patch(pr_id)) if report.incremental else parsed reviewable, report.ignored_paths = partition( - _reviewable_paths(parsed), settings.ignore_paths + [p for p in _reviewable_paths(parsed) if placement.get(p) is not None], + settings.ignore_paths, ) if not reviewable: @@ -107,7 +120,7 @@ def execute(github_client, build_member, pr_id, settings: ReviewSettings) -> Run outcome = run_review( provider, chunk.text, - parsed, + placement, language=settings.language, custom_prompt=settings.custom_prompt, schema=FINDINGS_SCHEMA, @@ -134,7 +147,7 @@ def execute(github_client, build_member, pr_id, settings: ReviewSettings) -> Run already_posted_keys=set(previous.get("posted", [])), ) - _publish(github_client, pr_id, report, parsed, previous) + _publish(github_client, pr_id, report, placement, previous) return report @@ -154,52 +167,98 @@ def _select_diff(github_client, pr_id, previous, settings, head_sha): """ Decide what to review: everything, or only what is new. + Returns (patch, base_sha, incremental, reason_for_full_review). + Falls back to the full diff whenever the incremental path is not clearly - safe — no previous SHA, the comparison fails, or it comes back empty. A - review that covers too much is a waste; one that covers too little while - claiming otherwise is a lie. + safe — no previous SHA, history rewritten, a merge in the range, the + comparison fails, or it comes back empty. A review that covers too much is + a waste; one that covers too little while claiming otherwise is a lie. """ last_sha = previous.get("last_reviewed_sha") if not (settings.incremental and last_sha and head_sha and last_sha != head_sha): - return github_client.get_pr_patch(pr_id), None, False + return github_client.get_pr_patch(pr_id), None, False, None try: - patch = github_client.get_compare_patch(last_sha, head_sha) + reason = _unsafe_range(github_client.get_compare(last_sha, head_sha)) + patch = None if reason else github_client.get_compare_patch(last_sha, head_sha) except Exception: logger.exception( "Could not diff %s..%s; reviewing the whole pull request", last_sha, head_sha ) - return github_client.get_pr_patch(pr_id), None, False + reason = "the comparison with the last review failed" + patch = None - if not patch or not patch.strip(): + if patch is not None and not patch.strip(): logger.info("No changes since %s; reviewing the whole pull request", last_sha) - return github_client.get_pr_patch(pr_id), None, False + reason, patch = None, None + + if patch is None: + if reason: + logger.info("Reviewing the whole pull request: %s", reason) + return github_client.get_pr_patch(pr_id), None, False, reason logger.info("Incremental review of %s..%s", last_sha[:7], head_sha[:7]) - return patch, last_sha, True + return patch, last_sha, True, None + + +def _unsafe_range(comparison) -> str | None: + """ + Why the commits since the last review cannot be diffed on their own, or None. + + A rebase or force-push leaves the last reviewed commit off the branch, and + the comparison then runs from an old merge base. A merge from the base + branch brings in commits that are not part of the pull request. Either way + the diff contains lines GitHub will not accept a comment on. + """ + status = comparison.get("status") + if status != "ahead": + return f"the branch history was rewritten since the last review ({status})" + + commits = comparison.get("commits") or [] + if comparison.get("total_commits", len(commits)) > len(commits): + return "too many commits since the last review to check them" + if any(len(commit.get("parents") or []) > 1 for commit in commits): + return "a merge commit was added since the last review" + return None def _publish(github_client, pr_id, report, parsed, previous, nothing_to_review=False): - """Post the inline comments and update the single sticky summary.""" + """ + Post the inline comments and update the single sticky summary. + + State only moves forward for what actually happened. Findings count as + posted only if the review request succeeded, and the last reviewed SHA only + advances if this run covered everything — otherwise the next run would diff + from here, and whatever this run skipped would drop out of the summary + without ever having been reviewed. + """ comments = [_inline_comment(f) for f in report.budget.posted] - body = build_summary(report, parsed, nothing_to_review=nothing_to_review) if comments: try: github_client.create_review(pr_id, comments, "") except Exception: - logger.exception("Could not post inline comments; the summary will still be posted") + logger.exception("Could not post inline comments; listing them in the summary") + report.post_failed = True posted_keys = set(previous.get("posted", [])) - posted_keys.update(fingerprint(f) for f in report.budget.posted) + if not report.post_failed: + posted_keys.update(fingerprint(f) for f in report.budget.posted) + + if nothing_to_review or (report.fully_reviewed and not report.post_failed): + report.next_base_sha = report.head_sha + else: + report.next_base_sha = previous.get("last_reviewed_sha") + + body = build_summary(report, parsed, nothing_to_review=nothing_to_review) sticky.upsert( github_client, pr_id, body, sticky.build_state( - last_reviewed_sha=report.head_sha, + last_reviewed_sha=report.next_base_sha, posted_fingerprints=posted_keys, suppressed_count=len(report.budget.suppressed), ), @@ -212,7 +271,20 @@ def _inline_comment(finding): "", finding.rationale, ] - if finding.suggestion: + if finding.suggestion and finding.relocated: + # The model's line numbers did not match the diff and the comment was + # moved. Its suggestion was written for the original lines, so a + # one-click "Commit suggestion" would overwrite the wrong code. + parts += [ + "", + "Suggested change (not offered as a one-click suggestion: the line " + "numbers were adjusted to fit the diff, so check where it applies):", + "", + "```", + finding.suggestion.rstrip(), + "```", + ] + elif finding.suggestion: parts += ["", "```suggestion", finding.suggestion.rstrip(), "```"] parts += ["", f"confidence {finding.confidence:.0%}"] @@ -250,6 +322,8 @@ def build_summary(report, parsed, nothing_to_review=False) -> str: "", "The model did not return output matching the required schema, even after " "a repair attempt. **This diff was not reviewed.**", + "", + *_coverage(report), ] return "\n".join([*lines, "", _footer(report)]) @@ -265,6 +339,20 @@ def build_summary(report, parsed, nothing_to_review=False) -> str: lines += ["", "### Findings", "", _counts_line(report)] + if report.post_failed: + lines += [ + "", + "**GitHub rejected the inline comments**, so the findings that would " + "have been posted are listed here instead. They will be tried again " + "on the next push.", + "", + ] + for finding in sorted(posted, key=lambda f: f.rank): + lines.append( + f"- `{finding.file}:{finding.line_start}` — **{finding.severity}** " + f"{finding.title}" + ) + suppressed = report.budget.suppressed if suppressed: lines += [ @@ -328,7 +416,8 @@ def _counts_line(report) -> str: counts = budget.counts_by_severity(report.all_findings) by_severity = ", ".join(f"{count} {name}" for name, count in counts.items()) or "none" - parts = [f"{len(budget.posted)} posted"] + verb = "not posted (see below)" if report.post_failed else "posted" + parts = [f"{len(budget.posted)} {verb}"] if budget.over_budget: parts.append(f"{len(budget.over_budget)} over budget") if budget.below_threshold: @@ -363,8 +452,22 @@ def _coverage(report) -> list[str]: f"- Only changes since `{(report.base_sha or '')[:7]}` were reviewed. " "Earlier commits were reviewed in previous runs." ) + if report.full_review_reason: + lines.append( + f"- The whole pull request was reviewed again: {report.full_review_reason}." + ) if report.parse_failed: lines.append("- Part of the diff produced unusable model output and was not reviewed.") + if not report.fully_reviewed or report.post_failed: + since = ( + f"everything since `{report.next_base_sha[:7]}`" + if report.next_base_sha + else "the whole pull request" + ) + lines.append( + f"- This run was incomplete, so the next push reviews {since} again " + "rather than only its own commits." + ) return lines diff --git a/src/reviewer.py b/src/reviewer.py index 15f7e6b..d7d4db5 100644 --- a/src/reviewer.py +++ b/src/reviewer.py @@ -143,6 +143,7 @@ def place_findings( span = finding.line_end - finding.line_start finding.line_start = nearest finding.line_end = nearest + span + finding.relocated = True placed.append(_clamp_range(finding, diff_file)) return placed, rejected @@ -161,6 +162,8 @@ def _clamp_range(finding: Finding, diff_file) -> Finding: end = finding.line_end while end > finding.line_start and not diff_file.can_comment_on(end): end -= 1 + if end != finding.line_end: + finding.relocated = True finding.line_end = end return finding diff --git a/tests/test_findings.py b/tests/test_findings.py index 3356873..5dfb966 100644 --- a/tests/test_findings.py +++ b/tests/test_findings.py @@ -166,7 +166,11 @@ def test_required_fields_match_the_dataclass(self): required = set(FINDINGS_SCHEMA["properties"]["findings"]["items"]["required"]) fields = set(Finding.__dataclass_fields__) self.assertTrue(required <= fields) - self.assertEqual(fields - required, {"suggestion"}) + # `relocated` is set by placement, never by the model, so it is not + # in the schema at all. + self.assertEqual(fields - required, {"suggestion", "relocated"}) + item = FINDINGS_SCHEMA["properties"]["findings"]["items"] + self.assertNotIn("relocated", item["properties"]) class RankingTests(unittest.TestCase): diff --git a/tests/test_github_client.py b/tests/test_github_client.py index e34281a..edf818c 100644 --- a/tests/test_github_client.py +++ b/tests/test_github_client.py @@ -76,6 +76,25 @@ def test_get_file_content(self): self.mock_repo.get_contents.assert_called_with(self.filename, ref=self.commit_sha) self.assertEqual(content, "file content") + def test_get_base_file_reads_the_base_commit_not_the_head(self): + mock_pr = MagicMock() + mock_pr.base.sha = "base-sha" + self.mock_repo.get_pull.return_value = mock_pr + mock_content = MagicMock() + mock_content.decoded_content = b"max_comments: 2\n" + self.mock_repo.get_contents.return_value = mock_content + + text = self.github_client.get_base_file(self.pr_id, ".genai-review.yml") + self.mock_repo.get_contents.assert_called_with(".genai-review.yml", ref="base-sha") + self.assertEqual(text, "max_comments: 2\n") + + def test_get_base_file_returns_none_when_the_file_is_absent(self): + from github import UnknownObjectException + + self.mock_repo.get_pull.return_value = MagicMock() + self.mock_repo.get_contents.side_effect = UnknownObjectException(404, "Not Found", {}) + self.assertIsNone(self.github_client.get_base_file(self.pr_id, ".genai-review.yml")) + @patch('clients.github_client.requests.get') def test_get_pr_patch(self, mock_get): mock_response = MagicMock() diff --git a/tests/test_main.py b/tests/test_main.py index ddd5f46..9c4a9bb 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,3 +1,4 @@ +import os import unittest from types import SimpleNamespace from typing import ClassVar @@ -155,6 +156,16 @@ def test_no_key_at_all_still_defaults_to_gemini(self): class MainDispatchTests(unittest.TestCase): + def setUp(self): + # main() builds the GitHub client and reads the config file from the + # base branch before resolving the rest of its settings. + for patcher in ( + patch.dict(os.environ, {"GITHUB_TOKEN": "t", "GITHUB_PR_ID": "1"}), + patch("main.load_repo_config", return_value={}), + ): + patcher.start() + self.addCleanup(patcher.stop) + @patch("main.build_provider") @patch("main.GithubClient") @patch("main.get_env_vars") diff --git a/tests/test_repo_config.py b/tests/test_repo_config.py index f731e47..6de878c 100644 --- a/tests/test_repo_config.py +++ b/tests/test_repo_config.py @@ -11,7 +11,7 @@ import unittest from pathlib import Path -from repo_config import load, resolve +from repo_config import load, load_from_base, resolve def write_config(text): @@ -72,6 +72,40 @@ def test_api_keys_are_never_read_from_the_config_file(self): self.assertNotIn("secret-value", str(config)) +class TrustTests(unittest.TestCase): + """ + Whoever opens a pull request controls its branch. The config file is read + from the base commit, so a pull request cannot configure its own review. + """ + + class FakeGithub: + def __init__(self, text=None, error=None): + self.text, self.error, self.calls = text, error, [] + + def get_base_file(self, pr_id, path): + self.calls.append((pr_id, path)) + if self.error: + raise self.error + return self.text + + def test_the_file_is_read_from_the_base_commit(self): + github = self.FakeGithub("max_comments: 2\n") + self.assertEqual(load_from_base(github, 7), {"max_comments": 2}) + self.assertEqual(github.calls, [(7, ".genai-review.yml")]) + + def test_a_file_missing_on_the_base_branch_is_not_an_error(self): + self.assertEqual(load_from_base(self.FakeGithub(None), 7), {}) + + def test_an_api_failure_is_not_an_error(self): + self.assertEqual(load_from_base(self.FakeGithub(error=RuntimeError("503")), 7), {}) + + def test_base_url_is_never_read_from_the_file(self): + """It decides which server receives the API key.""" + config = load(write_config("base_url: https://attacker.example/v1\nprovider: openai\n")) + self.assertNotIn("base_url", config) + self.assertEqual(config["provider"], "openai") + + class PrecedenceTests(unittest.TestCase): def test_the_action_input_beats_the_config_file(self): """A workflow is the more specific statement of intent.""" diff --git a/tests/test_review_pressure.py b/tests/test_review_pressure.py index 1640bd6..da922ce 100644 --- a/tests/test_review_pressure.py +++ b/tests/test_review_pressure.py @@ -395,6 +395,12 @@ def __init__(self, patch): self.comments = [] self.reviews = [] self.head_sha = "sha-one" + # What the compare endpoint reports; tests override it to simulate a + # rebase or a merge from the base branch. + self.comparison = {"status": "ahead", "total_commits": 1, + "commits": [{"parents": [{"sha": "p"}]}]} + self.compare_patch = None + self.fail_reviews = False def get_pr_head_sha(self, pr_id): return self.head_sha @@ -402,8 +408,11 @@ def get_pr_head_sha(self, pr_id): def get_pr_patch(self, pr_id): return self.patch + def get_compare(self, base, head): + return self.comparison + def get_compare_patch(self, base, head): - return self.patch + return self.patch if self.compare_patch is None else self.compare_patch def get_pr_comments(self, pr_id): return list(self.comments) @@ -415,6 +424,8 @@ def post_comment(self, pr_id, body): return comment def create_review(self, pr_id, comments, body): + if self.fail_reviews: + raise RuntimeError("422 Unprocessable Entity") self.reviews.append(comments) return {"id": len(self.reviews)} @@ -539,5 +550,146 @@ def test_nothing_to_review_still_produces_a_summary(self): self.assertIn("No reviewable files", github.comments[0].body) +class NothingIsForgottenTests(unittest.TestCase): + """ + State moves forward only for what actually happened. + + The summary's promise is that it says what was not reviewed. If the last + reviewed SHA advanced past a run that skipped files, the next push would + diff from there and those files would drop out of the summary unreviewed. + """ + + def setUp(self): + self.github = FakeGithub(NOISY_PATCH) + self.provider = provider_returning(noisy_findings(4)) + self.settings = ReviewSettings(max_comments=5) + + def run_once(self): + return execute(self.github, lambda _: self.provider, 1, self.settings) + + def state(self): + return sticky.parse_state(self.github.comments[0].body) + + def test_an_incomplete_run_does_not_advance_the_reviewed_sha(self): + self.provider.review.return_value.text = "not json at all" + report = self.run_once() + self.assertTrue(report.parse_failed) + self.assertIsNone(self.state()["last_reviewed_sha"]) + self.assertIn("next push reviews the whole pull request again", + self.github.comments[0].body) + + def test_an_incomplete_second_run_keeps_the_previous_base(self): + self.run_once() + self.github.head_sha = "sha-two" + self.provider.review.return_value.text = "not json at all" + self.run_once() + self.assertEqual(self.state()["last_reviewed_sha"], "sha-one") + + def test_a_complete_run_advances_the_reviewed_sha(self): + self.run_once() + self.assertEqual(self.state()["last_reviewed_sha"], "sha-one") + + def test_findings_are_not_marked_posted_when_the_review_is_rejected(self): + self.github.fail_reviews = True + report = self.run_once() + self.assertTrue(report.post_failed) + self.assertEqual(self.state()["posted"], []) + self.assertIsNone(self.state()["last_reviewed_sha"]) + + def test_a_rejected_review_lists_its_findings_in_the_summary(self): + self.github.fail_reviews = True + report = self.run_once() + body = self.github.comments[0].body + self.assertIn("GitHub rejected the inline comments", body) + for finding in report.budget.posted: + self.assertIn(finding.title, body) + + def test_findings_rejected_once_are_posted_on_the_next_push(self): + self.github.fail_reviews = True + first = self.run_once() + self.github.fail_reviews = False + self.github.head_sha = "sha-two" + second = self.run_once() + self.assertEqual({f.title for f in second.budget.posted}, + {f.title for f in first.budget.posted}) + + +class IncrementalSafetyTests(unittest.TestCase): + """After a rebase or a merge from main, the compare diff is not the PR's.""" + + def setUp(self): + self.github = FakeGithub(NOISY_PATCH) + self.provider = provider_returning(noisy_findings(2)) + self.settings = ReviewSettings(max_comments=5) + execute(self.github, lambda _: self.provider, 1, self.settings) + self.github.head_sha = "sha-two" + + def run_again(self): + return execute(self.github, lambda _: self.provider, 1, self.settings) + + def test_a_rewritten_history_falls_back_to_the_whole_pull_request(self): + self.github.comparison = {"status": "diverged", "commits": []} + report = self.run_again() + self.assertFalse(report.incremental) + self.assertIn("rewritten", report.full_review_reason) + self.assertIn("reviewed again", self.github.comments[0].body) + + def test_a_merge_from_the_base_branch_falls_back_to_the_whole_pull_request(self): + self.github.comparison = { + "status": "ahead", + "total_commits": 1, + "commits": [{"parents": [{"sha": "a"}, {"sha": "b"}]}], + } + report = self.run_again() + self.assertFalse(report.incremental) + self.assertIn("merge", report.full_review_reason) + + def test_files_outside_the_pull_request_are_never_reviewed(self): + """Defence in depth, if something from main slips into the range.""" + self.github.compare_patch = NOISY_PATCH + ( + "diff --git a/from_main.py b/from_main.py\n" + "--- a/from_main.py\n+++ b/from_main.py\n@@ -1,1 +1,2 @@\n x\n+y\n" + ) + self.provider = provider_returning([{ + "file": "from_main.py", "line_start": 2, "line_end": 2, + "severity": "blocker", "category": "bug", "confidence": 0.9, + "title": "Not this pull request's code", "rationale": "r", + }]) + report = self.run_again() + self.assertTrue(report.incremental) + self.assertNotIn("from_main.py", self.provider.review.call_args.args[0]) + self.assertEqual(report.budget.posted, []) + + +class RelocatedSuggestionTests(unittest.TestCase): + """A suggestion written for lines 10-11 must not be committed onto 12-13.""" + + PATCH = ( + "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n" + "@@ -1,1 +1,3 @@\n import os\n+x = 1\n+y = 2\n" + ) + + def finding(self, line): + return { + "file": "a.py", "line_start": line, "line_end": line, + "severity": "major", "category": "bug", "confidence": 0.9, + "title": "Wrong value", "rationale": "r", "suggestion": "x = 2", + } + + def posted_body(self, line): + github = FakeGithub(self.PATCH) + provider = provider_returning([self.finding(line)]) + execute(github, lambda _: provider, 1, ReviewSettings(ignore_paths=())) + return github.inline_comments[0]["body"] + + def test_an_exact_finding_keeps_its_one_click_suggestion(self): + self.assertIn("```suggestion", self.posted_body(2)) + + def test_a_snapped_finding_loses_its_one_click_suggestion(self): + body = self.posted_body(5) + self.assertNotIn("```suggestion", body) + self.assertIn("x = 2", body) + + if __name__ == "__main__": unittest.main()