diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 83f6830d5..c7941a2eb 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5623,24 +5623,196 @@ jobs: exit 0 } + retry_inline_comments_one_at_a_time() { + local batch_payload_file="$1" review_body="$2" refused_locations_file="$3" + local attached_locations_file="${4:-}" + local deferred_locations_file="${5:-}" + local split_dir comment_file wrapped_file error_file response_file + local attached=0 + local found=0 + local -a split_args + + : >"$refused_locations_file" + if [ -n "$attached_locations_file" ]; then + : >"$attached_locations_file" + fi + split_dir="$(mktemp -d)" + split_args=( + python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" + --split-payload "$batch_payload_file" + --output-dir "$split_dir" + --retry-limit "${OPENCODE_INLINE_COMMENT_RETRY_LIMIT:-20}" + ) + if [ -n "$deferred_locations_file" ]; then + : >"$deferred_locations_file" + split_args+=(--deferred-locations "$deferred_locations_file") + fi + if ! "${split_args[@]}"; then + rm -rf "$split_dir" + return 1 + fi + for comment_file in "$split_dir"/comment-*.json; do + [ -f "$comment_file" ] || continue + found=1 + wrapped_file="$(mktemp)" + error_file="$(mktemp)" + response_file="$(mktemp)" + if [ "$attached" -eq 0 ]; then + jq --arg body "$review_body" --arg event "REQUEST_CHANGES" \ + '.event = $event | .body = $body' "$comment_file" >"$wrapped_file" + else + cp "$comment_file" "$wrapped_file" + fi + if post_pull_review_with_retry \ + "inline review one-at-a-time" \ + "$review_write_token" \ + "$wrapped_file" \ + "$error_file" \ + "$response_file"; then + attached=1 + if [ -n "$attached_locations_file" ]; then + python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --record-attach \ + --attached-locations "$attached_locations_file" \ + --comment-file "$comment_file" || true + fi + else + python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --record-refusal \ + --refused-locations "$refused_locations_file" \ + --comment-file "$comment_file" \ + --error-file "$error_file" || true + if [ -s "$error_file" ]; then + cat "$error_file" >>"${refused_locations_file}.errors" + fi + fi + rm -f "$wrapped_file" "$error_file" "$response_file" + if [ "${REVIEW_PUBLICATION_STALE_HEAD:-}" = "1" ]; then + rm -rf "$split_dir" + return 1 + fi + done + rm -rf "$split_dir" + if [ "$found" -eq 0 ] || [ "$attached" -eq 0 ]; then + return 1 + fi + return 0 + } + + prefilter_inline_comments_to_hunks() { + local payload_file="$1" + local skipped_file="$2" + local hunks_diff_file + local filtered_file + local merge_base + hunks_diff_file="$(mktemp)" + filtered_file="$(mktemp)" + : >"$skipped_file" + if [ -z "${OPENCODE_SOURCE_WORKDIR:-}" ] || [ -z "${PR_BASE_SHA:-}" ] || [ -z "${PR_HEAD_SHA:-}" ]; then + rm -f "$hunks_diff_file" "$filtered_file" + return 0 + fi + merge_base="$(git -C "$OPENCODE_SOURCE_WORKDIR" merge-base "$PR_BASE_SHA" "$PR_HEAD_SHA" 2>/dev/null || true)" + if [ -z "$merge_base" ]; then + merge_base="$PR_BASE_SHA" + fi + if ! git -C "$OPENCODE_SOURCE_WORKDIR" diff --unified=3 --find-renames --no-color --no-ext-diff \ + "$merge_base" "$PR_HEAD_SHA" >"$hunks_diff_file"; then + rm -f "$hunks_diff_file" "$filtered_file" + return 0 + fi + if python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --filter-hunks \ + --payload "$payload_file" \ + --hunks-diff "$hunks_diff_file" \ + --output "$filtered_file" \ + --skipped-locations "$skipped_file"; then + mv "$filtered_file" "$payload_file" + else + rm -f "$filtered_file" + fi + rm -f "$hunks_diff_file" + } + create_pull_review_with_payload() { local event="$1" body="$2" review_payload_file="$3" fallback_body_file="$4" + local source_body_file="${5:-}" + local control_json="${6:-}" local gh_error_file local rewritten_payload_file local review_response_file + local skipped_locations_file + local comment_count gh_error_file="$(mktemp)" rewritten_payload_file="$(mktemp)" review_response_file="$(mktemp)" + skipped_locations_file="$(mktemp)" body="$(ensure_review_body_has_change_graph "$body")" if jq --arg body "$body" '.body = $body' "$review_payload_file" >"$rewritten_payload_file"; then mv "$rewritten_payload_file" "$review_payload_file" else rm -f "$rewritten_payload_file" fi + prefilter_inline_comments_to_hunks "$review_payload_file" "$skipped_locations_file" + comment_count="$(jq '.comments | length' "$review_payload_file" 2>/dev/null || printf '0')" + if [ "$comment_count" = "0" ] && [ -s "$skipped_locations_file" ] \ + && [ -n "$source_body_file" ] && [ -n "$control_json" ]; then + build_inline_comment_failure_body \ + "$source_body_file" "$fallback_body_file" "$control_json" \ + "" "" "" "" "$skipped_locations_file" || true + if [ -s "$fallback_body_file" ]; then + body="$(cat "$fallback_body_file")" + if jq --arg body "$body" '.body = $body | del(.comments)' \ + "$review_payload_file" >"$rewritten_payload_file"; then + mv "$rewritten_payload_file" "$review_payload_file" + else + rm -f "$rewritten_payload_file" + fi + fi + fi emit_review_body_to_action_log "$event" "$body" "$review_payload_file" if ! post_pull_review_with_retry "inline review" "$review_write_token" "$review_payload_file" "$gh_error_file" "$review_response_file"; then warn_gh_publication_failure "pull review inline comments" "$gh_error_file" - rm -f "$gh_error_file" "$review_response_file" + if [ "${REVIEW_PUBLICATION_STALE_HEAD:-}" != "1" ] \ + && python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --is-unprocessable --error-file "$gh_error_file"; then + refused_locations_file="$(mktemp)" + attached_locations_file="$(mktemp)" + deferred_locations_file="$(mktemp)" + if retry_inline_comments_one_at_a_time \ + "$review_payload_file" "$body" "$refused_locations_file" \ + "$attached_locations_file" "$deferred_locations_file"; then + if { [ -s "$refused_locations_file" ] || [ -s "$deferred_locations_file" ] \ + || [ -s "$skipped_locations_file" ]; } \ + && [ -n "$source_body_file" ] && [ -n "$control_json" ]; then + mixed_error_file="$gh_error_file" + if [ -s "${refused_locations_file}.errors" ]; then + mixed_error_file="${refused_locations_file}.errors" + fi + build_inline_comment_failure_body \ + "$source_body_file" "$fallback_body_file" "$control_json" \ + "$mixed_error_file" "$refused_locations_file" \ + "$attached_locations_file" "$deferred_locations_file" \ + "$skipped_locations_file" || true + update_review_overview "$event" "$(cat "$fallback_body_file")" + else + update_review_overview "$event" "$body" + fi + rm -f "$gh_error_file" "$review_response_file" \ + "$refused_locations_file" "${refused_locations_file}.errors" \ + "$attached_locations_file" "$deferred_locations_file" \ + "$skipped_locations_file" + return 0 + fi + rm -f "$refused_locations_file" "${refused_locations_file}.errors" \ + "$attached_locations_file" "$deferred_locations_file" + fi + if [ -n "$source_body_file" ] && [ -n "$control_json" ]; then + build_inline_comment_failure_body \ + "$source_body_file" "$fallback_body_file" "$control_json" \ + "$gh_error_file" "" "" "" "$skipped_locations_file" || true + fi + rm -f "$gh_error_file" "$review_response_file" "$skipped_locations_file" if [ "${REVIEW_PUBLICATION_STALE_HEAD:-}" = "1" ]; then printf '::error::OpenCode inline review publication stopped because PR head advanced beyond %s.\n' "$HEAD_SHA" return 1 @@ -5652,7 +5824,15 @@ jobs: fi return 1 fi - rm -f "$gh_error_file" "$review_response_file" + if [ -s "$skipped_locations_file" ] && [ -n "$source_body_file" ] && [ -n "$control_json" ]; then + build_inline_comment_failure_body \ + "$source_body_file" "$fallback_body_file" "$control_json" \ + "" "" "" "" "$skipped_locations_file" || true + if [ -s "$fallback_body_file" ]; then + body="$(cat "$fallback_body_file")" + fi + fi + rm -f "$gh_error_file" "$review_response_file" "$skipped_locations_file" update_review_overview "$event" "$body" } @@ -5766,12 +5946,37 @@ jobs: build_inline_comment_failure_body() { local body_file="$1" local output_file="$2" - - { - cat "$body_file" - printf '\n## Inline comment publishing failed\n\n' - printf 'GitHub did not accept the inline review comments for the cited finding lines, so OpenCode did not copy suggested diffs into this PR-level body. Re-run the review after the findings are anchored to changed diff lines, or inspect the workflow log/control JSON and apply the changes manually.\n' - } >"$output_file" + local control_json="$3" + local error_file="${4:-}" + local refused_locations_file="${5:-}" + local attached_locations_file="${6:-}" + local deferred_locations_file="${7:-}" + local skipped_locations_file="${8:-}" + local -a fallback_args + + fallback_args=( + python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" + --control "$control_json" + --body "$body_file" + --output "$output_file" + --retry-limit "${OPENCODE_INLINE_COMMENT_RETRY_LIMIT:-20}" + ) + if [ -n "$error_file" ]; then + fallback_args+=(--error-file "$error_file") + fi + if [ -n "$refused_locations_file" ]; then + fallback_args+=(--refused-locations "$refused_locations_file") + fi + if [ -n "$attached_locations_file" ]; then + fallback_args+=(--attached-locations "$attached_locations_file") + fi + if [ -n "$deferred_locations_file" ]; then + fallback_args+=(--deferred-locations "$deferred_locations_file") + fi + if [ -n "$skipped_locations_file" ]; then + fallback_args+=(--skipped-locations "$skipped_locations_file") + fi + "${fallback_args[@]}" } publish_request_changes_from_control() { @@ -5785,8 +5990,8 @@ jobs: fallback_body_file="$(mktemp)" format_request_changes_body "$control_json" "$body_file" build_request_changes_review_payload "$control_json" "$body_file" "$payload_file" - build_inline_comment_failure_body "$body_file" "$fallback_body_file" - create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" + build_inline_comment_failure_body "$body_file" "$fallback_body_file" "$control_json" + create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" "$body_file" "$control_json" rm -f "$body_file" "$payload_file" "$fallback_body_file" } diff --git a/AGENTS.md b/AGENTS.md index 688b33035..bb58d4ffb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,3 +2,9 @@ > **Agents: read the master context FIRST.** Before any work, read [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) (mission · naruon-as-platform + inter-component UML · cross-cutting disciplines · conventions · roadmap · current state), the live **GitHub Project #1** (work/roadmap source of truth), the full spec **ContextualWisdomLab/naruon#974**, and operate the Project per [`docs/agent-github-project-protocol.md`](docs/agent-github-project-protocol.md). The repo/Project — not any private agent memory — is the source of truth. +Materialize accepts only exact SHA-256 pins or a bounded relative `-r` include (no `.`/`..`); a lone `--require-hashes` directive is not trust evidence. See [`docs/doctoring/review-inline-comment-422-fallback.md`](docs/doctoring/review-inline-comment-422-fallback.md). + +A bare `422` or issue `#422` is not a sealed GitHub HTTP 422. +Surviving hunk comments convert suggested diffs into closed GitHub suggestion fences. +Leftover overview receipts sanitize path and phrase so a leftover cannot close the HTML comment or reopen a suggestion fence. +Leftover overview paths that contain `-->`, ``, `` or reopen an applyable GitHub suggestion block (CWE-116). +- Sanitized leftover overview receipt path and phrase so a leftover cannot close `` or reopen a GitHub suggestion fence (CWE-116). +- Materialized base Python locks only when every package line is an exact SHA-256 pin or a bounded relative `-r`/`--requirement` include. A lone `--require-hashes` directive, a dotted include such as `./lock.txt`, or `-r other-hashes.txt` no longer enters the trusted build context. +- Converted surviving OpenCode inline suggested diffs into GitHub `suggestion` blocks so authors can apply the replacement on the current-head hunk in one click. A closed ```suggestion fence, not a bare substring, is treated as already applyable (CWE-1288). +- Treated only sealed GitHub HTTP 422 tokens (`HTTP 422`, `status code 422`, `Error code: 422`, `Unprocessable Entity`) as unprocessable review writes, so issue `#422` or a path containing `422` cannot trigger the inline-comment 422 fallback. +- Dropped OpenCode inline comments that sit outside every current-head changed hunk before the GitHub POST so those comments become overview receipts instead of a 422 that wipes the batch. A present collected diff with no commentable hunks (binary-only) now skips every comment instead of fail-opening the original payload. +- Capped one-at-a-time OpenCode inline retries at 20 comments and listed attached `path:line` beside refused receipts so the overview shows both outcomes, plus any locations left untried by the cap. +- Kept each refused OpenCode inline comment's own GitHub 422 phrase next to its `path:line` so mixed retries do not collapse every failure into one shared error sentence. +- After a mixed one-at-a-time inline retry, listed only the refused `path:line` rows in the overview receipts so attached hunks are not reported as failed. +- After a batch GitHub 422, retried OpenCode inline comments one at a time so comments on surviving hunks still attach instead of dropping the entire review thread. +- Stored each refused OpenCode inline comment as a durable overview receipt that pairs the trusted `path:line` with the GitHub 422 error phrase from `gh api` stderr or JSON `errors[].message`. +- Named each trusted `path:line` in the OpenCode GitHub 422 inline-comment fallback so a refused attach still tells the author the exact current-head location instead of a generic “cited finding lines” sentence. - Bounded the Strix quality self-test's deterministic timeout fixtures to 3-second process and 5-second fake-sleep budgets so exact-head policy evidence completes inside the existing job limit without changing production Strix scanner timeouts, providers, credentials, or review semantics. - Allowed commas and ASCII parentheses in the bounded Strix changed-file path policy so legal tracked Packrat fixtures can receive exact-head security analysis, while rejecting raw `..` components before normalization and keeping controls, backslashes, whitespace ambiguity, and shell punctuation fail-closed. - Bound each review-agent invocation key to the wrapper's complete canonical payload, including the base branch and requesting actor; altered fields with a valid-format key now fail before durable-leader election or forwarding, and wrapper write permission is job-scoped. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md new file mode 100644 index 000000000..524a547ef --- /dev/null +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -0,0 +1,104 @@ +# GitHub 422 inline-comment fallback cites trusted path:line + +검토 기준일: **2026-08-13** + +## Incident + +When GitHub rejects an OpenCode `REQUEST_CHANGES` review because one or more +inline comments cannot attach, the publisher already falls back to a PR-level +body and does not copy suggested diffs into that body. The fallback sentence +said only “the cited finding lines.” Authors then had to open the workflow log +or control JSON to learn *which* `path:line` GitHub refused (GitHub, n.d.-a, +n.d.-b). That is weaker than the line-anchored review artifact modern code +review expects (Bacchelli & Bird, 2013). + +## Decision + +Leftover overview paths that contain `-->`, ``). On mixed success +the overview lists attached `path:line` rows beside refused `path:line` +rows (each refused row keeps that comment's own 422 phrase) and any +locations left untried by the retry cap. JSON `errors[].message` such as +`pull_request_review_thread.path is invalid`, or the first `HTTP 422` +line, is the phrase source. A later comment's different GitHub error +does not overwrite an earlier one. URLs are stripped and each phrase is +bounded to 240 characters. + +Before the first GitHub POST, the publisher runs +`git diff --unified=3` from the merge base to the current head and keeps +only comments whose `path:line` sits inside a parsed hunk range, including +hunk context lines. GitHub accepts review comments only on those diff +hunks (GitHub, n.d.-b); off-hunk comments are recorded as skipped +`path:line` receipts instead of being sent. An empty collected diff +leaves the payload unchanged so a failed `git diff` cannot drop every +comment. A present collected diff with no commentable hunks (binary-only +or unparseable headers) skips every comment instead of posting them. +This matches the modern-review expectation that discussion belongs on the +changed hunk rather than elsewhere in the file (Bacchelli & Bird, 2013; +Sadowski et al., 2018). + +The publisher calls this helper from `build_inline_comment_failure_body` +with the same control object used to build the inline `comments` array. +Surviving comments convert a `` ```diff `` suggested replacement into a +closed GitHub `` ```suggestion `` fence so authors can apply it on the +current-head hunk in one click (GitHub, n.d.-c). A prose mention of the +token is not a fence (CWE-1288). Suggested diffs stay out of the PR-level body. + +## Verification contract + +- `tests/test_opencode_inline_comment_fallback.py` pins safe-pair extraction, + the exact location list, GitHub JSON `errors[].message` phrases, HTTP 422 + line fallback, empty-set sentence, CLI success with `--error-file`, + fail-closed unreadable control or error input, batch-to-single comment + splitting, `--is-unprocessable` classification, mixed-success + receipts that list attached path:line beside refused path:line, + per-comment 422 phrases, the 20-comment one-at-a-time retry cap, and + leftover path:line rows that were not retried, unified-diff hunk + parsing, and the pre-POST filter that drops off-hunk comments. +- `tests/test_opencode_agent_contract.py` and + `scripts/ci/test_strix_quick_gate.sh` pin the workflow call with + `$control_json`. + +## Rollback + +If GitHub later accepts off-diff comments, keep citing the attempted +`path:line` in the fallback. Do not restore a location-free sentence. + +## References (APA 7th) + +Bacchelli, A., & Bird, C. (2013). Expectations, outcomes, and challenges of +modern code review. In *Proceedings of the 35th International Conference on +Software Engineering* (pp. 712–721). IEEE. +https://doi.org/10.1109/ICSE.2013.6606617 + +GitHub. (n.d.-a). *Create a review for a pull request*. GitHub Docs. Retrieved +August 13, 2026, from +https://docs.github.com/en/rest/pulls/reviews#create-a-review-for-a-pull-request + +GitHub. (n.d.-b). *Create a review comment for a pull request*. GitHub Docs. +Retrieved August 13, 2026, from +https://docs.github.com/en/rest/pulls/comments#create-a-review-comment-for-a-pull-request + +Sadowski, C., Söderberg, E., Church, L., Sipko, M., & Bacchelli, A. (2018). +Modern code review: A case study at Google. In *Proceedings of the 40th +International Conference on Software Engineering: Software Engineering in +Practice* (pp. 181–190). ACM. https://doi.org/10.1145/3183519.3183525 diff --git a/scripts/ci/materialize_base_python_requirements.py b/scripts/ci/materialize_base_python_requirements.py index 98cdad459..7a9c204b8 100755 --- a/scripts/ci/materialize_base_python_requirements.py +++ b/scripts/ci/materialize_base_python_requirements.py @@ -87,6 +87,58 @@ def _is_candidate_lock_name(name: str) -> bool: ) + +def _is_candidate_lock_path(path: pathlib.PurePosixPath) -> bool: + """Return whether one safe tracked path can name a pip requirements lock. + + In addition to conventional ``requirements*.txt`` names, repositories often + keep concrete environment closures as direct children such as + ``requirements/ci.txt`` or ``service/requirements/package.txt``. Only direct + ``.txt`` children of a directory named ``requirements`` gain this path-based + eligibility; content must still pass the independent complete hash-pin + validation before it reaches the trusted image build context. + """ + return _is_candidate_lock_name(path.name) or ( + path.suffix == ".txt" and path.parent.name == "requirements" + ) + + +def _is_bounded_requirement_include(line: str) -> bool: + """Return whether one requirements include names a bounded relative file. + + Includes are accepted only as a two-token ``-r``/``--requirement`` form + whose target is itself a candidate lock path written as a normalized + relative POSIX path. Absolute paths, ``.`` or ``..`` components, double + slashes, URLs, option-like targets, shell/Windows path separators, + fragments, queries, extra inline options or hashes, and includes of + non-lock files are rejected before a base-owned file can enter the + trusted build context. + The downstream installer still proves that the candidate is an independently + complete hash closure; this predicate grants syntax eligibility only. + """ + fields = line.split() + if len(fields) != 2 or fields[0] not in {"-r", "--requirement"}: + return False + target = fields[1] + if ( + target.startswith(("-", "~")) + or "\\" in target + or ":" in target + or "?" in target + or "#" in target + ): + return False + include_path = pathlib.PurePosixPath(target) + return ( + bool(include_path.parts) + and target == include_path.as_posix() + and not include_path.is_absolute() + and "." not in include_path.parts + and ".." not in include_path.parts + and _is_candidate_lock_path(include_path) + ) + + def _requirement_lines(content: bytes) -> list[str]: """Return logical requirement lines, joining backslash line-continuations. @@ -107,26 +159,27 @@ def _requirement_lines(content: bytes) -> list[str]: def _is_hash_pinned(content: bytes) -> bool: - """Return whether content carries hash pins and is safe to preflight. - - Discovery is content-based rather than name-based so hash-pinned locks in any - location (a service subdirectory, ``requirements-dev.txt``, - ``requirements-test.txt``) can be considered for offline coverage, while an - unpinned or PR-mutable requirements file is still excluded from the networked - build context. Hash syntax cannot prove that a file includes every transitive - dependency, so the trusted image installer separately preflights every - candidate as an independent ``--require-hashes`` closure. An empty file - carries no installable dependency and is not materialized. + """Return whether content carries only trusted pins or bounded includes. + + Discovery is content-based rather than name-based so exact hash-pinned locks + in service subdirectories and role-specific requirements files can be + considered for offline coverage. Candidate syntax is deliberately stricter + than a substring search: each package line must be an exact ``==`` pin with + one or more complete SHA-256 hashes, or a bounded relative requirements + include. A global ``--require-hashes`` directive is not trust evidence by + itself. The downstream installer separately preflights every candidate as an + independent ``pip --require-hashes`` closure, so syntax eligibility never + substitutes for dependency-closure proof. """ lines = _requirement_lines(content) - if not lines: + requirement_lines = [line for line in lines if line != "--require-hashes"] + if not requirement_lines: return False - return any(line == "--require-hashes" for line in lines) or all( - "--hash=" in line or line.startswith(("-r ", "--requirement ")) - for line in lines + return all( + _is_fully_hash_pinned_requirement(line) + or _is_bounded_requirement_include(line) + for line in requirement_lines ) - - def _is_fully_hash_pinned_requirement(line: str) -> bool: """Return whether one uv-export line is an exact package pin with SHA-256 hashes.""" fields = re.split(r"\s+(?=--hash=)", line) diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py new file mode 100644 index 000000000..ada699d1f --- /dev/null +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -0,0 +1,923 @@ +#!/usr/bin/env python3 +"""Filter inline comments to current-head hunks and cite refused path:line.""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import sys +from pathlib import Path, PurePosixPath, PureWindowsPath +from typing import Any + +DEFAULT_SINGLE_COMMENT_RETRY_LIMIT = 20 +ERROR_PHRASE_MAX_CHARS = 240 +HTTP_422_LINE_RE = re.compile(r"(?im)^(?:gh:\s*)?(.*HTTP 422.*)$") +SEALED_422_RE = re.compile( + r"(?i)(?:HTTP\s+422|status(?:\s+code)?\s+422|Error code:\s*422|Unprocessable Entity)" +) +HUNK_HEADER_RE = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") +PLUS_PATH_RE = re.compile(r"^\+\+\+ b/(.+?)(?:\t.*)?$") +MINUS_PATH_RE = re.compile(r"^--- a/(.+?)(?:\t.*)?$") +DIFF_FENCE_RE = re.compile(r"```diff\r?\n(.*?)```", re.DOTALL) +SUGGESTION_FENCE_RE = re.compile(r"```suggestion\r?\n(.*?)```", re.DOTALL) +DIFF_CONTENT_PREFIXES = ( + "diff --git", + "@@ ", + "+++ ", + "--- ", + "Binary files ", + "GIT binary patch", +) + + +def safe_finding_path(raw_path: object) -> str | None: + """Return a repository-relative finding path, or None when it is unsafe. + + Rejects traversal, absolute and drive paths, backslashes, and characters + that would break Markdown receipt fences (backtick, ``<``, ``>``, ``&``) + or close the overview HTML comment / reopen a suggestion fence + (``-->``, ``", "``. + A leftover path or reason with ``-->`` or an HTML metacharacter would + close that comment or inject markup (CWE-116). Fence markers are also + removed so a leftover cannot reopen a GitHub suggestion block. + """ + excerpt = (text or "").replace("\r\n", "\n").replace("\t", " ") + excerpt = ( + excerpt.replace("```", "") + .replace("", "") + .replace("<", "") + .replace(">", "") + .replace("&", "") + ) + return excerpt.strip("\n") + + +def render_inline_comment_receipts( + locations: list[tuple[str, int]], + error_phrase: str = "", + phrases: dict[tuple[str, int], str] | None = None, +) -> list[str]: + """Return durable overview receipt lines for refused inline comments.""" + if not locations: + return [] + lines: list[str] = [] + for path, line in locations: + phrase = "" + if phrases is not None: + phrase = phrases.get((path, line), "") + if not phrase: + phrase = error_phrase + path = sanitize_leftover_excerpt(path) + phrase = sanitize_leftover_excerpt(phrase) + if phrase: + lines.append(f"- `{path}:{line}` — {phrase}") + else: + lines.append(f"- `{path}:{line}`") + return lines + + +def render_inline_comment_failure_suffix( + locations: list[tuple[str, int]], + *, + error_phrase: str = "", + mixed_success: bool = False, + phrases: dict[tuple[str, int], str] | None = None, + attached_locations: list[tuple[str, int]] | None = None, + deferred_locations: list[tuple[str, int]] | None = None, + skipped_locations: list[tuple[str, int]] | None = None, + retry_limit: int | None = None, +) -> str: + """Return the PR-body suffix used when GitHub rejects inline comments.""" + attached = attached_locations or [] + deferred = deferred_locations or [] + skipped = skipped_locations or [] + heading = ( + "## Inline comment publication receipts" + if error_phrase or mixed_success or attached or deferred or skipped + else "## Inline comment publishing failed" + ) + lines = [ + "", + heading, + "", + ] + if attached: + lines.append("GitHub accepted these trusted current-head finding locations:") + lines.append("") + lines.extend(render_inline_comment_receipts(attached)) + lines.append("") + if locations: + if mixed_success: + if attached: + lines.append( + "These trusted current-head finding locations were still refused:" + ) + else: + lines.append( + "GitHub accepted some inline comments. These trusted " + "current-head finding locations were still refused:" + ) + else: + lines.append( + "GitHub did not accept the inline review comments for these " + "trusted current-head finding locations:" + ) + lines.append("") + lines.extend( + render_inline_comment_receipts( + locations, error_phrase, phrases=phrases + ) + ) + lines.append("") + lines.append( + "OpenCode did not copy suggested diffs into this PR-level body. " + "Re-run the review after those exact path:line anchors sit on " + "current-head changed hunks, or inspect the workflow log/control " + "JSON and apply the changes manually." + ) + elif not attached and not deferred and not skipped: + lines.append( + "GitHub did not accept the inline review comments, and the " + "control JSON had no trusted path:line findings. Inspect the " + "workflow log and apply any remaining blockers from the review " + "body manually." + ) + if error_phrase: + lines.append("") + lines.append(f"- GitHub error: {error_phrase}") + if deferred: + if locations or attached: + lines.append("") + lines.append( + "These trusted current-head finding locations were not retried " + f"(retry limit {single_comment_retry_limit(retry_limit)}):" + ) + lines.append("") + lines.extend(render_inline_comment_receipts(deferred)) + if not locations: + lines.append("") + lines.append( + "OpenCode did not copy suggested diffs into this PR-level body. " + "Re-run the review after those exact path:line anchors sit on " + "current-head changed hunks, or inspect the workflow log/control " + "JSON and apply the changes manually." + ) + if skipped: + if locations or attached or deferred: + lines.append("") + lines.append( + "These trusted current-head finding locations were not posted " + "because they sit outside every current-head changed hunk:" + ) + lines.append("") + lines.extend(render_inline_comment_receipts(skipped)) + if not locations and not deferred: + lines.append("") + lines.append( + "OpenCode did not copy suggested diffs into this PR-level body. " + "Re-run the review after those exact path:line anchors sit on " + "current-head changed hunks, or inspect the workflow log/control " + "JSON and apply the changes manually." + ) + lines.append("") + return "\n".join(lines) + + +def _trusted_location_subset( + items: list[tuple[str, int]] | None, + allowed: set[tuple[str, int]], +) -> list[tuple[str, int]]: + """Return first-seen locations that remain in the trusted control set.""" + if not items: + return [] + kept: list[tuple[str, int]] = [] + seen: set[tuple[str, int]] = set() + for item in items: + if item not in allowed or item in seen: + continue + seen.add(item) + kept.append(item) + return kept + + +def render_inline_comment_failure_body( + body: str, + control: dict[str, Any], + *, + error_text: str = "", + refused_locations: list[tuple[str, int]] | None = None, + refused_receipts: list[tuple[str, int, str]] | None = None, + attached_locations: list[tuple[str, int]] | None = None, + deferred_locations: list[tuple[str, int]] | None = None, + skipped_locations: list[tuple[str, int]] | None = None, + retry_limit: int | None = None, +) -> str: + """Append the 422 fallback suffix to an existing REQUEST_CHANGES body.""" + error_phrase = github_publication_error_phrase(error_text) if error_text else "" + allowed = set(trusted_finding_locations(control)) + attached = ( + _trusted_location_subset(attached_locations, allowed) + if attached_locations is not None + else [] + ) + deferred = ( + _trusted_location_subset(deferred_locations, allowed) + if deferred_locations is not None + else [] + ) + skipped = ( + _trusted_location_subset(skipped_locations, allowed) + if skipped_locations is not None + else [] + ) + phrases: dict[tuple[str, int], str] | None = None + if refused_receipts is not None: + locations = [ + (path, line) + for path, line, _phrase in refused_receipts + if (path, line) in allowed + ] + phrases = { + (path, line): phrase + for path, line, phrase in refused_receipts + if phrase and (path, line) in allowed + } + mixed_success = True + if not locations and not attached and not deferred and not skipped: + return body.rstrip("\n") + "\n" + elif ( + refused_locations is None + and attached_locations is None + and deferred_locations is None + and skipped_locations is None + ): + locations = trusted_finding_locations(control) + mixed_success = False + elif refused_locations is None: + locations = [] + mixed_success = True + if not attached and not deferred and not skipped: + return body.rstrip("\n") + "\n" + else: + locations = [item for item in refused_locations if item in allowed] + mixed_success = True + if not locations and not attached and not deferred and not skipped: + return body.rstrip("\n") + "\n" + return body.rstrip("\n") + render_inline_comment_failure_suffix( + locations, + error_phrase=error_phrase, + mixed_success=mixed_success, + phrases=phrases, + attached_locations=attached or None, + deferred_locations=deferred or None, + skipped_locations=skipped or None, + retry_limit=retry_limit, + ) + + +def load_control(path: Path) -> dict[str, Any]: + """Load one trusted review-control JSON object.""" + try: + value = json.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + raise ValueError(f"control JSON could not be read: {exc}") from exc + if not isinstance(value, dict): + raise ValueError("control JSON must be an object") + return value + + +def main(argv: list[str] | None = None) -> int: + """Write 422 fallback text or split a batch review into single comments.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--control", type=Path) + parser.add_argument("--body", type=Path) + parser.add_argument("--output", type=Path) + parser.add_argument("--error-file", type=Path) + parser.add_argument("--split-payload", type=Path) + parser.add_argument("--output-dir", type=Path) + parser.add_argument("--is-unprocessable", action="store_true") + parser.add_argument("--refused-locations", type=Path) + parser.add_argument("--attached-locations", type=Path) + parser.add_argument("--deferred-locations", type=Path) + parser.add_argument("--skipped-locations", type=Path) + parser.add_argument("--retry-limit", type=int) + parser.add_argument("--record-refusal", action="store_true") + parser.add_argument("--record-attach", action="store_true") + parser.add_argument("--comment-file", type=Path) + parser.add_argument("--filter-hunks", action="store_true") + parser.add_argument("--payload", type=Path) + parser.add_argument("--hunks-diff", type=Path) + args = parser.parse_args(argv) + try: + if args.filter_hunks: + if args.payload is None or args.hunks_diff is None or args.output is None: + raise ValueError( + "--payload, --hunks-diff, and --output are required " + "with --filter-hunks" + ) + payload = load_control(args.payload) + diff_text = args.hunks_diff.read_text(encoding="utf-8") + hunks = parse_unified_diff_hunk_lines(diff_text) + write_hunk_filtered_payload( + payload, + hunks, + args.output, + skipped_path=args.skipped_locations, + diff_text=diff_text, + ) + return 0 + if args.record_attach: + if args.attached_locations is None or args.comment_file is None: + raise ValueError( + "--attached-locations and --comment-file are required " + "with --record-attach" + ) + payload = load_control(args.comment_file) + comments = payload.get("comments") + if ( + not isinstance(comments, list) + or not comments + or not isinstance(comments[0], dict) + ): + raise ValueError("comment file must contain comments[0]") + first = comments[0] + line = first.get("line") + record_attached_receipt( + args.attached_locations, + str(first.get("path") or ""), + line if isinstance(line, int) and not isinstance(line, bool) else 0, + ) + return 0 + if args.record_refusal: + if ( + args.refused_locations is None + or args.comment_file is None + or args.error_file is None + ): + raise ValueError( + "--refused-locations, --comment-file, and --error-file " + "are required with --record-refusal" + ) + payload = load_control(args.comment_file) + comments = payload.get("comments") + if ( + not isinstance(comments, list) + or not comments + or not isinstance(comments[0], dict) + ): + raise ValueError("comment file must contain comments[0]") + first = comments[0] + line = first.get("line") + record_refused_receipt( + args.refused_locations, + str(first.get("path") or ""), + line if isinstance(line, int) and not isinstance(line, bool) else 0, + args.error_file.read_text(encoding="utf-8"), + ) + return 0 + if args.is_unprocessable: + if args.error_file is None: + raise ValueError("--error-file is required with --is-unprocessable") + error_text = args.error_file.read_text(encoding="utf-8") + return 0 if github_error_is_unprocessable(error_text) else 1 + if args.split_payload is not None: + if args.output_dir is None: + raise ValueError("--output-dir is required with --split-payload") + payload = load_control(args.split_payload) + write_single_comment_payloads( + payload, + args.output_dir, + limit=args.retry_limit, + deferred_path=args.deferred_locations, + ) + return 0 + if args.control is None or args.body is None or args.output is None: + raise ValueError("--control, --body, and --output are required") + control = load_control(args.control) + body = args.body.read_text(encoding="utf-8") + error_text = ( + args.error_file.read_text(encoding="utf-8") if args.error_file else "" + ) + refused_locations = None + refused_receipts = None + attached_locations = None + deferred_locations = None + skipped_locations = None + if args.refused_locations is not None: + parsed_receipts = parse_refused_receipts( + args.refused_locations.read_text(encoding="utf-8") + ) + if any(phrase for _path, _line, phrase in parsed_receipts): + refused_receipts = parsed_receipts + else: + refused_locations = [ + (path, line) for path, line, _phrase in parsed_receipts + ] + if args.attached_locations is not None: + attached_locations = parse_refused_locations( + args.attached_locations.read_text(encoding="utf-8") + ) + if args.deferred_locations is not None: + deferred_locations = parse_refused_locations( + args.deferred_locations.read_text(encoding="utf-8") + ) + if args.skipped_locations is not None: + skipped_locations = parse_refused_locations( + args.skipped_locations.read_text(encoding="utf-8") + ) + except (OSError, UnicodeDecodeError, ValueError) as exc: + print(exc, file=sys.stderr) + return 2 + args.output.write_text( + render_inline_comment_failure_body( + body, + control, + error_text=error_text, + refused_locations=refused_locations, + refused_receipts=refused_receipts, + attached_locations=attached_locations, + deferred_locations=deferred_locations, + skipped_locations=skipped_locations, + retry_limit=args.retry_limit, + ), + encoding="utf-8", + ) + return 0 + + +if __name__ == "__main__": # pragma: no cover - exercised through runpy CLI test + raise SystemExit(main()) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 7343c06ac..a52de34a5 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1474,7 +1474,22 @@ assert_opencode_review_posts_suggested_diffs_inline() { assert_file_contains "$workflow_file" "create_pull_review_with_payload" "opencode review can post custom review payloads" assert_file_contains "$workflow_file" "comments: [" "opencode review payload includes inline review comments" assert_file_contains "$workflow_file" '#### Suggested diff\n```diff\n' "opencode review puts suggested diffs inside inline review comments" - assert_file_contains "$workflow_file" "GitHub did not accept the inline review comments" "opencode review explains anchor failures instead of copying diffs to the PR body" + assert_file_contains "$REPO_ROOT/scripts/ci/opencode_inline_comment_fallback.py" "GitHub did not accept the inline review comments" "opencode review explains anchor failures instead of copying diffs to the PR body" + assert_file_contains "$REPO_ROOT/scripts/ci/opencode_inline_comment_fallback.py" "accepted some inline comments" "opencode mixed-success receipts distinguish attached and refused comments" + assert_file_contains "$workflow_file" "opencode_inline_comment_fallback.py" "opencode 422 fallback cites trusted path:line via the dedicated helper" + assert_file_contains "$workflow_file" 'build_inline_comment_failure_body "$body_file" "$fallback_body_file" "$control_json"' "opencode 422 fallback receives the trusted control JSON" + assert_file_contains "$workflow_file" 'fallback_args+=(--error-file "$error_file")' "opencode 422 overview receipt includes the GitHub error file" + assert_file_contains "$workflow_file" "retry_inline_comments_one_at_a_time" "opencode retries inline comments one at a time after batch 422" + assert_file_contains "$workflow_file" "inline review one-at-a-time" "opencode one-at-a-time retries use the bounded review-write helper" + assert_file_contains "$workflow_file" '--refused-locations "$refused_locations_file"' "opencode mixed-success receipts pass only refused path:line rows" + assert_file_contains "$workflow_file" "--record-refusal" "opencode records per-comment 422 phrases on refused path:line rows" + assert_file_contains "$workflow_file" "--record-attach" "opencode records attached path:line rows beside refused receipts" + assert_file_contains "$workflow_file" '--attached-locations "$attached_locations_file"' "opencode mixed-success receipts persist attached path:line rows" + assert_file_contains "$workflow_file" "--retry-limit" "opencode bounds one-at-a-time inline comment retries" + assert_file_contains "$workflow_file" '${OPENCODE_INLINE_COMMENT_RETRY_LIMIT:-20}' "opencode default one-at-a-time retry cap is 20" + assert_file_contains "$workflow_file" "--filter-hunks" "opencode drops off-hunk inline comments before GitHub POST" + assert_file_contains "$workflow_file" "prefilter_inline_comments_to_hunks" "opencode prefilters inline comments against current-head hunks" + assert_file_contains "$workflow_file" '--skipped-locations "$skipped_locations_file"' "opencode records off-hunk path:line rows that were not posted" assert_file_contains "$workflow_file" "publish_request_changes_from_control" "opencode review REQUEST_CHANGES path publishes findings from the control JSON" if awk '/format_request_changes_body\(\)/,/build_request_changes_review_payload\(\)/ { print }' "$workflow_file" | diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 8a383f0c2..6058326d1 100644 --- a/tests/test_materialize_base_python_requirements.py +++ b/tests/test_materialize_base_python_requirements.py @@ -30,6 +30,18 @@ def _created_tool_directory(path: Path) -> str: return str(path) +def _simulate_linux_x86_64_runner(monkeypatch: pytest.MonkeyPatch) -> None: + """Let installer verification tests run on a non-Linux developer host. + + Production still fail-closes unless ``sys.platform`` is Linux and + ``platform.machine()`` is ``x86_64``. These unit tests pin both values so + they measure version verification, caching, and cleanup instead of the + host architecture gate already covered by the portability contract. + """ + monkeypatch.setattr(materializer.sys, "platform", "linux") + monkeypatch.setattr(materializer.platform, "machine", lambda: "x86_64") + + def test_materializes_only_regular_hash_locks_from_exact_base(tmp_path: Path) -> None: """A PR-modified lock cannot enter the networked coverage image build context.""" repo = tmp_path / "repo" @@ -150,9 +162,24 @@ def test_lock_name_candidates_are_pip_requirements_files() -> None: def test_hash_pin_detection_includes_pinned_and_excludes_unpinned_or_empty() -> None: """Only fully hash-pinned, non-empty lock content is materialized.""" assert not materializer._is_hash_pinned(b"# comment only\n\n") - assert materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") + assert not materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") assert materializer._is_hash_pinned(b"demo==1 --hash=sha256:" + b"a" * 64 + b"\n") - assert materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert materializer._is_hash_pinned(b"-r requirements-other.txt\n") + assert not materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert not materializer._is_hash_pinned(b"-r ./requirements-other.txt\n") + assert not materializer._is_hash_pinned(b"-r ../escape.txt\n") + assert materializer._is_bounded_requirement_include( + "--requirement requirements-other.txt" + ) + assert not materializer._is_bounded_requirement_include("-r .") + assert not materializer._is_bounded_requirement_include("-r -evil.txt") + assert not materializer._is_bounded_requirement_include("-r ~evil.txt") + assert not materializer._is_bounded_requirement_include("-r C:foo.txt") + assert not materializer._is_bounded_requirement_include("-r foo?bar.txt") + assert not materializer._is_bounded_requirement_include("-r foo#bar.txt") + assert not materializer._is_bounded_requirement_include(r"-r foo\\bar.txt") + assert not materializer._is_bounded_requirement_include("-r") + assert not materializer._is_bounded_requirement_include("-r /abs/requirements.txt") assert not materializer._is_hash_pinned(b"untrusted==1\n") # uv export / pip-compile multi-line continuation format (spec, then --hash= lines). assert materializer._is_hash_pinned( @@ -644,6 +671,7 @@ def test_install_trusted_uv_verifies_version_and_caches_path( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: """The installer writes one executable, verifies its version, and caches it.""" + _simulate_linux_x86_64_runner(monkeypatch) tool_dir = tmp_path / "uv" monkeypatch.setattr( materializer.tempfile, @@ -690,6 +718,7 @@ def test_install_trusted_uv_rejects_version_process_failures( failure: OSError | subprocess.TimeoutExpired, ) -> None: """A missing or hung downloaded executable is removed and rejected.""" + _simulate_linux_x86_64_runner(monkeypatch) tool_dir = tmp_path / "uv" monkeypatch.setattr( materializer.tempfile, @@ -721,6 +750,7 @@ def test_install_trusted_uv_rejects_wrong_version_or_exit_status( completed: subprocess.CompletedProcess[bytes], ) -> None: """Unexpected version output or a nonzero status cannot satisfy the pin.""" + _simulate_linux_x86_64_runner(monkeypatch) tool_dir = tmp_path / f"uv-{completed.returncode}-{len(completed.stdout)}" monkeypatch.setattr( materializer.tempfile, diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index daeaa37a2..150280297 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1609,6 +1609,32 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): 'post_pull_review_with_retry "inline review" "$review_write_token"' in publish_step ) + assert "opencode_inline_comment_fallback.py" in workflow + assert ( + 'build_inline_comment_failure_body "$body_file" "$fallback_body_file" "$control_json"' + in workflow + ) + assert ( + 'create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" "$body_file" "$control_json"' + in workflow + ) + assert 'fallback_args+=(--error-file "$error_file")' in workflow + assert "retry_inline_comments_one_at_a_time" in workflow + assert "--is-unprocessable" in workflow + assert "inline review one-at-a-time" in workflow + assert '--refused-locations "$refused_locations_file"' in workflow + assert "--record-refusal" in workflow + assert "--record-attach" in workflow + assert '--attached-locations "$attached_locations_file"' in workflow + assert "--deferred-locations" in workflow + assert "--retry-limit" in workflow + assert "${OPENCODE_INLINE_COMMENT_RETRY_LIMIT:-20}" in workflow + assert "--filter-hunks" in workflow + assert "--hunks-diff" in workflow + assert "prefilter_inline_comments_to_hunks" in workflow + assert '--skipped-locations "$skipped_locations_file"' in workflow + assert "--unified=3" in workflow + assert "accepted some inline comments" not in workflow assert "OPENCODE_EXHAUSTED_REKICK_" not in publish_step assert 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "10800"' not in publish_step assert "steps.opencode_review_model_pool.outcome == 'success'" not in workflow diff --git a/tests/test_opencode_inline_comment_fallback.py b/tests/test_opencode_inline_comment_fallback.py new file mode 100644 index 000000000..b509cedc3 --- /dev/null +++ b/tests/test_opencode_inline_comment_fallback.py @@ -0,0 +1,1401 @@ +import json +import runpy +import sys + +import pytest + +from scripts.ci.opencode_inline_comment_fallback import ( + DEFAULT_SINGLE_COMMENT_RETRY_LIMIT, + comment_on_changed_hunk, + filter_payload_comments_to_hunks, + github_error_is_unprocessable, + github_publication_error_phrase, + iter_single_comment_payloads, + main, + parse_refused_locations, + parse_refused_receipts, + parse_unified_diff_hunk_lines, + unified_diff_has_content, + record_attached_receipt, + record_refused_receipt, + render_inline_comment_failure_body, + render_inline_comment_receipts, + sanitize_leftover_excerpt, + render_single_comment_review, + single_comment_retry_limit, + trusted_finding_locations, + write_hunk_filtered_payload, + write_single_comment_payloads, + extract_suggestion_replacement, + render_github_suggestion_block, + apply_github_suggestion_blocks, + body_has_suggestion_fence, +) + + +def control(*findings: dict[str, object]) -> dict[str, object]: + """Return a REQUEST_CHANGES control object for fallback tests.""" + return { + "result": "REQUEST_CHANGES", + "findings": list(findings), + } + + +def test_trusted_finding_locations_keeps_first_safe_path_line_pairs(): + locations = trusted_finding_locations( + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": 12, "line": 1}, + {"path": "../escape.py", "line": 1}, + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "/abs.py", "line": 3}, + {"path": "scripts/ci/other.py", "line": 0}, + {"path": "scripts/ci/other.py", "line": True}, + {"path": "scripts/ci/other.py", "line": 12}, + {"path": "scripts/ci/close-->comment.py", "line": 8}, + {"path": "scripts/ci/fence```suggestion.py", "line": 9}, + "not-an-object", + ) + ) + + assert locations == [ + ("scripts/ci/example.py", 7), + ("scripts/ci/other.py", 12), + ] + assert trusted_finding_locations({"findings": None}) == [] + assert trusted_finding_locations({}) == [] + + +def test_fallback_body_cites_each_trusted_path_line(): + body = render_inline_comment_failure_body( + "## Findings\n\nexisting body\n", + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "README.md", "line": 3}, + ), + ) + + assert body.startswith("## Findings\n\nexisting body") + assert "GitHub did not accept the inline review comments" in body + assert "- `scripts/ci/example.py:7`" in body + assert "- `README.md:3`" in body + assert "did not copy suggested diffs into this PR-level body" in body + + +def test_github_publication_error_phrase_prefers_json_error_messages(): + phrase = github_publication_error_phrase( + "gh: HTTP 422: Unprocessable Entity " + "(https://api.github.com/repos/org/repo/pulls/1/reviews)\n" + '{"message":"Validation Failed","errors":[' + '{"resource":"PullRequestReview","field":"comments","code":"custom",' + '"message":"pull_request_review_thread.path is invalid"},' + '{"message":"Review comments is invalid"}' + "]}\n" + ) + + assert phrase.startswith("GitHub HTTP 422:") + assert "pull_request_review_thread.path is invalid" in phrase + assert "Review comments is invalid" in phrase + assert "https://api.github.com" not in phrase + + +def test_github_publication_error_phrase_falls_back_to_http_line(): + assert ( + github_publication_error_phrase( + "post failed\ngh: Validation Failed (HTTP 422)\n" + ) + == "GitHub HTTP 422: Validation Failed (HTTP 422)" + ) + assert ( + github_publication_error_phrase("GitHub HTTP 422: already normalized\n") + == "GitHub HTTP 422: already normalized" + ) + assert github_publication_error_phrase("status code 422 only") == "GitHub HTTP 422" + assert github_publication_error_phrase("see issue #422") == ( + "GitHub review write failed" + ) + assert github_publication_error_phrase("scripts/ci/file422.py") == ( + "GitHub review write failed" + ) + assert ( + github_publication_error_phrase("https://api.github.example/HTTP 422") + == "GitHub HTTP 422: 422" + ) + assert render_inline_comment_receipts([], "GitHub HTTP 422") == [] + assert sanitize_leftover_excerpt("a & b ") == "a b c" + assert "-->" not in sanitize_leftover_excerpt("close --> comment") + assert "```" not in sanitize_leftover_excerpt("```suggestion\nsecret") + assert render_inline_comment_receipts([("a.py -->", 1)], "close --> comment") == [ + "- `a.py :1` — close comment" + ] + assert github_publication_error_phrase("") == "GitHub review write failed" + assert ( + github_publication_error_phrase("secondary rate limit") + == "GitHub review write failed" + ) + assert github_publication_error_phrase("{") == "GitHub review write failed" + assert ( + github_publication_error_phrase('{"errors":"not-a-list","message":"x"}') + == "GitHub review write failed" + ) + assert ( + github_publication_error_phrase('{"errors":[{"code":"custom"}]}') + == "GitHub review write failed" + ) + assert ( + github_publication_error_phrase('{"errors":[1,{"message":""}]}') + == "GitHub review write failed" + ) + + +def test_fallback_body_attaches_error_phrase_to_each_receipt(): + body = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/example.py", "line": 7}), + error_text=( + '{"errors":[{"message":"Line could not be resolved"}]}' + ), + ) + + assert "## Inline comment publication receipts" in body + assert ( + "- `scripts/ci/example.py:7` — GitHub HTTP 422: Line could not be resolved" + in body + ) + + +def test_mixed_success_receipts_list_only_refused_path_lines(): + refused = parse_refused_locations( + "scripts/ci/other.py:12\n# note\n../escape.py:1\n" + "scripts/ci/example.py:0\nbadline\nscripts/ci/other.py:12\n" + "scripts/ci/skip.py:x\n" + ) + assert refused == [("scripts/ci/other.py", 12)] + + body = render_inline_comment_failure_body( + "## Findings\nattached example.py:7\n", + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "scripts/ci/other.py", "line": 12}, + ), + error_text='{"errors":[{"message":"Line could not be resolved"}]}', + refused_locations=refused, + ) + + assert "accepted some inline comments" in body + assert ( + "- `scripts/ci/other.py:12` — GitHub HTTP 422: Line could not be resolved" + in body + ) + assert "scripts/ci/example.py:7`" not in body + assert parse_refused_locations("") == [] + assert parse_refused_receipts( + "scripts/ci/a.py:3\tGitHub HTTP 422: path is invalid\n" + "scripts/ci/b.py:9\tGitHub HTTP 422: Line could not be resolved\n" + ) == [ + ("scripts/ci/a.py", 3, "GitHub HTTP 422: path is invalid"), + ("scripts/ci/b.py", 9, "GitHub HTTP 422: Line could not be resolved"), + ] + all_attached = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/example.py", "line": 7}), + refused_locations=[], + ) + assert "were still refused" not in all_attached + assert "did not accept the inline review comments" not in all_attached + + +def test_mixed_success_receipts_keep_per_comment_422_phrases(tmp_path): + receipts = [ + ( + "scripts/ci/example.py", + 7, + "GitHub HTTP 422: pull_request_review_thread.path is invalid", + ), + ( + "scripts/ci/other.py", + 12, + "GitHub HTTP 422: Line could not be resolved", + ), + ] + body = render_inline_comment_failure_body( + "## Findings\n", + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "scripts/ci/other.py", "line": 12}, + {"path": "scripts/ci/ok.py", "line": 4}, + ), + refused_receipts=receipts, + ) + assert "accepted some inline comments" in body + assert ( + "- `scripts/ci/example.py:7` — GitHub HTTP 422: " + "pull_request_review_thread.path is invalid" + in body + ) + assert ( + "- `scripts/ci/other.py:12` — GitHub HTTP 422: Line could not be resolved" + in body + ) + assert "scripts/ci/ok.py:4" not in body + unmatched = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/example.py", "line": 7}), + refused_receipts=[("scripts/ci/missing.py", 1, "GitHub HTTP 422")], + ) + assert "were still refused" not in unmatched + + dest = tmp_path / "refused.txt" + record_refused_receipt( + dest, + "scripts/ci/example.py", + 7, + '{"errors":[{"message":"pull_request_review_thread.path is invalid"}]}', + ) + record_refused_receipt( + dest, + "scripts/ci/other.py", + 12, + '{"errors":[{"message":"Line could not be resolved"}]}', + ) + assert parse_refused_receipts(dest.read_text(encoding="utf-8")) == receipts + + comment = tmp_path / "comment.json" + comment.write_text( + json.dumps( + { + "comments": [ + {"path": "scripts/ci/example.py", "line": 7, "body": "x"} + ] + } + ), + encoding="utf-8", + ) + error = tmp_path / "err.txt" + error.write_text( + '{"errors":[{"message":"pull_request_review_thread.path is invalid"}]}\n', + encoding="utf-8", + ) + dest2 = tmp_path / "cli-refused.txt" + assert ( + main( + [ + "--record-refusal", + "--refused-locations", + str(dest2), + "--comment-file", + str(comment), + "--error-file", + str(error), + ] + ) + == 0 + ) + assert "example.py:7\tGitHub HTTP 422: pull_request_review_thread.path is invalid" in dest2.read_text( + encoding="utf-8" + ) + assert main(["--record-refusal"]) == 2 + loc_only = tmp_path / "loc-only.txt" + loc_only.write_text("scripts/ci/example.py:7\n", encoding="utf-8") + control_only = tmp_path / "control-only.json" + body_only = tmp_path / "body-only.md" + out_only = tmp_path / "out-loc.md" + control_only.write_text( + json.dumps(control({"path": "scripts/ci/example.py", "line": 7})), + encoding="utf-8", + ) + body_only.write_text("## Findings\n", encoding="utf-8") + assert ( + main( + [ + "--control", + str(control_only), + "--body", + str(body_only), + "--output", + str(out_only), + "--refused-locations", + str(loc_only), + ] + ) + == 0 + ) + assert "`scripts/ci/example.py:7`" in out_only.read_text(encoding="utf-8") + two_control = tmp_path / "two-control.json" + two_out = tmp_path / "two-out.md" + two_control.write_text( + json.dumps( + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "scripts/ci/other.py", "line": 12}, + ) + ), + encoding="utf-8", + ) + assert ( + main( + [ + "--control", + str(two_control), + "--body", + str(body_only), + "--output", + str(two_out), + "--refused-locations", + str(dest), + ] + ) + == 0 + ) + two_text = two_out.read_text(encoding="utf-8") + assert "pull_request_review_thread.path is invalid" in two_text + assert "Line could not be resolved" in two_text + dest3 = tmp_path / "skip.txt" + record_refused_receipt(dest3, "../escape.py", 1, "HTTP 422") + assert dest3.read_text(encoding="utf-8") == "" if dest3.exists() else True + if dest3.exists(): + assert dest3.read_text(encoding="utf-8") == "" + bad_comment = tmp_path / "bad-comment.json" + bad_comment.write_text("{}", encoding="utf-8") + assert ( + main( + [ + "--record-refusal", + "--refused-locations", + str(dest2), + "--comment-file", + str(bad_comment), + "--error-file", + str(error), + ] + ) + == 2 + ) + + +def test_fallback_body_explains_missing_trusted_locations(): + body = render_inline_comment_failure_body("overview\n", control()) + + assert "GitHub did not accept the inline review comments" in body + assert "no trusted path:line findings" in body + assert "- `" not in body + with_error = render_inline_comment_failure_body( + "overview\n", + control(), + error_text='{"errors":[{"message":"Review comments is invalid"}]}', + ) + assert "GitHub error: GitHub HTTP 422: Review comments is invalid" in with_error + + +def test_cli_writes_fallback_and_rejects_unreadable_control(tmp_path, monkeypatch): + control_path = tmp_path / "control.json" + body_path = tmp_path / "body.md" + output_path = tmp_path / "fallback.md" + control_path.write_text( + json.dumps( + control({"path": "scripts/ci/example.py", "line": 7}), + ), + encoding="utf-8", + ) + body_path.write_text("## Findings\n", encoding="utf-8") + + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + ] + ) + == 0 + ) + written = output_path.read_text(encoding="utf-8") + assert "- `scripts/ci/example.py:7`" in written + + error_path = tmp_path / "gh-error.txt" + error_path.write_text( + '{"errors":[{"message":"pull_request_review_thread.path is invalid"}]}\n', + encoding="utf-8", + ) + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + "--error-file", + str(error_path), + ] + ) + == 0 + ) + written = output_path.read_text(encoding="utf-8") + assert ( + "- `scripts/ci/example.py:7` — GitHub HTTP 422: " + "pull_request_review_thread.path is invalid" + in written + ) + two_findings = tmp_path / "two.json" + two_findings.write_text( + json.dumps( + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "scripts/ci/other.py", "line": 12}, + ) + ), + encoding="utf-8", + ) + refused_path = tmp_path / "refused.txt" + refused_path.write_text("scripts/ci/other.py:12\n", encoding="utf-8") + assert ( + main( + [ + "--control", + str(two_findings), + "--body", + str(body_path), + "--output", + str(output_path), + "--error-file", + str(error_path), + "--refused-locations", + str(refused_path), + ] + ) + == 0 + ) + mixed = output_path.read_text(encoding="utf-8") + assert "accepted some inline comments" in mixed + assert "`scripts/ci/other.py:12`" in mixed + assert "`scripts/ci/example.py:7`" not in mixed + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + "--refused-locations", + str(tmp_path / "missing-refused.txt"), + ] + ) + == 2 + ) + + assert ( + main( + [ + "--control", + str(tmp_path / "missing.json"), + "--body", + str(body_path), + "--output", + str(output_path), + ] + ) + == 2 + ) + bad_json = tmp_path / "list.json" + bad_json.write_text("[]", encoding="utf-8") + assert ( + main( + [ + "--control", + str(bad_json), + "--body", + str(body_path), + "--output", + str(output_path), + ] + ) + == 2 + ) + broken = tmp_path / "broken.json" + broken.write_text("{", encoding="utf-8") + assert ( + main( + [ + "--control", + str(broken), + "--body", + str(body_path), + "--output", + str(output_path), + ] + ) + == 2 + ) + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(tmp_path / "missing-body.md"), + "--output", + str(output_path), + ] + ) + == 2 + ) + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + "--error-file", + str(tmp_path / "missing-error.txt"), + ] + ) + == 2 + ) + + monkeypatch.setattr( + sys, + "argv", + [ + "opencode_inline_comment_fallback.py", + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + ], + ) + with pytest.raises(SystemExit) as excinfo: + runpy.run_path( + "scripts/ci/opencode_inline_comment_fallback.py", run_name="__main__" + ) + assert excinfo.value.code == 0 + + +def test_github_error_is_unprocessable_detects_real_422_bodies(): + assert github_error_is_unprocessable( + '{"message":"Validation Failed","errors":[' + '{"message":"pull_request_review_thread.path is invalid"}]}' + ) + assert github_error_is_unprocessable("gh: HTTP 422: Unprocessable Entity") + assert not github_error_is_unprocessable("Resource not accessible by integration") + assert not github_error_is_unprocessable("") + assert not github_error_is_unprocessable("see issue #422") + assert not github_error_is_unprocessable("scripts/ci/file422.py") + + +def test_iter_single_comment_payloads_keeps_only_safe_comments(): + payload = { + "event": "REQUEST_CHANGES", + "body": "review body", + "commit_id": "a" * 40, + "comments": [ + { + "path": "scripts/ci/example.py", + "line": 7, + "side": "RIGHT", + "body": "first", + }, + {"path": "../escape.py", "line": 1, "body": "bad"}, + {"path": "scripts/ci/other.py", "line": 12, "body": "second"}, + {"path": "scripts/ci/plain.py", "line": 4, "body": "no-side"}, + "not-an-object", + {"path": "scripts/ci/empty.py", "line": 3, "body": " "}, + ], + } + + singles = iter_single_comment_payloads(payload) + assert [(item["path"], item["line"], item["side"]) for item in singles] == [ + ("scripts/ci/example.py", 7, "RIGHT"), + ("scripts/ci/other.py", 12, "RIGHT"), + ("scripts/ci/plain.py", 4, "RIGHT"), + ] + first = render_single_comment_review( + singles[0], event="REQUEST_CHANGES", review_body="review body" + ) + assert first["event"] == "REQUEST_CHANGES" + assert first["body"] == "review body" + assert first["comments"] == [ + { + "path": "scripts/ci/example.py", + "line": 7, + "side": "RIGHT", + "body": "first", + } + ] + later = render_single_comment_review( + singles[1], event="COMMENT", review_body="" + ) + assert later["event"] == "COMMENT" + assert later["body"] == "" + assert later["comments"][0]["path"] == "scripts/ci/other.py" + assert iter_single_comment_payloads({"comments": []}) == [] + assert iter_single_comment_payloads({"comments": "bad"}) == [] + assert iter_single_comment_payloads({"commit_id": "", "comments": [{}]}) == [] + + +def test_cli_splits_batch_payload_into_single_comment_files(tmp_path): + payload = tmp_path / "batch.json" + payload.write_text( + json.dumps( + { + "event": "REQUEST_CHANGES", + "body": "review body", + "commit_id": "b" * 40, + "comments": [ + { + "path": "scripts/ci/example.py", + "line": 7, + "side": "RIGHT", + "body": "first", + }, + { + "path": "scripts/ci/other.py", + "line": 12, + "side": "LEFT", + "body": "second", + }, + ], + } + ), + encoding="utf-8", + ) + output_dir = tmp_path / "singles" + + assert ( + main( + [ + "--split-payload", + str(payload), + "--output-dir", + str(output_dir), + ] + ) + == 0 + ) + files = sorted(output_dir.glob("comment-*.json")) + assert [path.name for path in files] == ["comment-000.json", "comment-001.json"] + first = json.loads(files[0].read_text(encoding="utf-8")) + assert first["event"] == "COMMENT" + assert first["comments"][0]["line"] == 7 + assert ( + main( + [ + "--split-payload", + str(tmp_path / "missing-batch.json"), + "--output-dir", + str(output_dir), + ] + ) + == 2 + ) + assert main(["--split-payload", str(payload)]) == 2 + error_path = tmp_path / "422.txt" + error_path.write_text("gh: HTTP 422: Unprocessable Entity\n", encoding="utf-8") + assert main(["--is-unprocessable", "--error-file", str(error_path)]) == 0 + error_path.write_text("Resource not accessible by integration\n", encoding="utf-8") + assert main(["--is-unprocessable", "--error-file", str(error_path)]) == 1 + assert main(["--is-unprocessable"]) == 2 + assert main([]) == 2 + + +def _batch_payload(*comments: dict[str, object]) -> dict[str, object]: + """Return a batch review payload for retry-limit tests.""" + return { + "event": "REQUEST_CHANGES", + "body": "review body", + "commit_id": "c" * 40, + "comments": list(comments), + } + + +def test_single_comment_retry_limit_defaults_and_rejects_invalid(monkeypatch): + monkeypatch.delenv("OPENCODE_INLINE_COMMENT_RETRY_LIMIT", raising=False) + assert single_comment_retry_limit() == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + assert single_comment_retry_limit(5) == 5 + assert single_comment_retry_limit("3") == 3 + assert single_comment_retry_limit(" 8 ") == 8 + assert single_comment_retry_limit(0) == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + assert single_comment_retry_limit(-1) == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + assert single_comment_retry_limit(True) == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + assert single_comment_retry_limit("abc") == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + assert single_comment_retry_limit("0") == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + monkeypatch.setenv("OPENCODE_INLINE_COMMENT_RETRY_LIMIT", "4") + assert single_comment_retry_limit() == 4 + monkeypatch.setenv("OPENCODE_INLINE_COMMENT_RETRY_LIMIT", "nope") + assert single_comment_retry_limit() == DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + + +def test_write_single_comment_payloads_caps_retry_and_records_deferred(tmp_path): + payload = _batch_payload( + {"path": "scripts/ci/a.py", "line": 1, "body": "one"}, + {"path": "scripts/ci/b.py", "line": 2, "body": "two"}, + {"path": "scripts/ci/c.py", "line": 3, "body": "three"}, + ) + output_dir = tmp_path / "singles" + deferred = tmp_path / "deferred.txt" + assert write_single_comment_payloads(payload, output_dir, limit=1, deferred_path=deferred) == 1 + files = sorted(output_dir.glob("comment-*.json")) + assert [path.name for path in files] == ["comment-000.json"] + assert parse_refused_locations(deferred.read_text(encoding="utf-8")) == [ + ("scripts/ci/b.py", 2), + ("scripts/ci/c.py", 3), + ] + empty_deferred = tmp_path / "none.txt" + assert write_single_comment_payloads(payload, tmp_path / "all", limit=20, deferred_path=empty_deferred) == 3 + assert empty_deferred.read_text(encoding="utf-8") == "" + + +def test_mixed_success_receipts_list_attached_beside_refused(): + body = render_inline_comment_failure_body( + "## Findings\n", + control( + {"path": "scripts/ci/ok.py", "line": 4}, + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "scripts/ci/later.py", "line": 20}, + {"path": "scripts/ci/skip.py", "line": 9}, + ), + refused_receipts=[ + ( + "scripts/ci/example.py", + 7, + "GitHub HTTP 422: pull_request_review_thread.path is invalid", + ) + ], + attached_locations=[("scripts/ci/ok.py", 4), ("scripts/ci/missing.py", 1)], + deferred_locations=[("scripts/ci/later.py", 20), ("scripts/ci/later.py", 20)], + retry_limit=1, + ) + assert "GitHub accepted these trusted current-head finding locations:" in body + assert "- `scripts/ci/ok.py:4`" in body + assert "These trusted current-head finding locations were still refused:" in body + assert ( + "- `scripts/ci/example.py:7` — GitHub HTTP 422: " + "pull_request_review_thread.path is invalid" + in body + ) + assert "were not retried (retry limit 1):" in body + assert "- `scripts/ci/later.py:20`" in body + assert "scripts/ci/skip.py:9" not in body + assert "scripts/ci/missing.py:1" not in body + deferred_only = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/later.py", "line": 20}), + refused_locations=[], + deferred_locations=[("scripts/ci/later.py", 20)], + retry_limit=1, + ) + assert "were not retried (retry limit 1):" in deferred_only + assert "- `scripts/ci/later.py:20`" in deferred_only + assert "did not copy suggested diffs" in deferred_only + attached_only = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/ok.py", "line": 4}), + refused_receipts=[], + attached_locations=[("scripts/ci/ok.py", 4)], + ) + assert "- `scripts/ci/ok.py:4`" in attached_only + assert "were still refused" not in attached_only + attached_without_refused_kw = render_inline_comment_failure_body( + "## Findings\n", + control( + {"path": "scripts/ci/ok.py", "line": 4}, + {"path": "scripts/ci/later.py", "line": 20}, + ), + attached_locations=[("scripts/ci/ok.py", 4)], + deferred_locations=[("scripts/ci/later.py", 20)], + retry_limit=1, + ) + assert "- `scripts/ci/ok.py:4`" in attached_without_refused_kw + assert "were not retried (retry limit 1):" in attached_without_refused_kw + assert "were still refused" not in attached_without_refused_kw + empty_outcome_files = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/ok.py", "line": 4}), + attached_locations=[], + deferred_locations=[], + ) + assert "were still refused" not in empty_outcome_files + assert "did not accept the inline review comments" not in empty_outcome_files + + +def test_cli_records_attached_and_bounded_split(tmp_path): + payload = tmp_path / "batch.json" + payload.write_text( + json.dumps( + _batch_payload( + {"path": "scripts/ci/a.py", "line": 1, "side": "RIGHT", "body": "one"}, + {"path": "scripts/ci/b.py", "line": 2, "side": "RIGHT", "body": "two"}, + ) + ), + encoding="utf-8", + ) + output_dir = tmp_path / "singles" + deferred = tmp_path / "deferred.txt" + assert ( + main( + [ + "--split-payload", + str(payload), + "--output-dir", + str(output_dir), + "--retry-limit", + "1", + "--deferred-locations", + str(deferred), + ] + ) + == 0 + ) + assert [path.name for path in sorted(output_dir.glob("comment-*.json"))] == [ + "comment-000.json" + ] + assert "scripts/ci/b.py:2" in deferred.read_text(encoding="utf-8") + + comment = tmp_path / "comment.json" + comment.write_text( + json.dumps({"comments": [{"path": "scripts/ci/a.py", "line": 1, "body": "x"}]}), + encoding="utf-8", + ) + attached = tmp_path / "attached.txt" + assert ( + main( + [ + "--record-attach", + "--attached-locations", + str(attached), + "--comment-file", + str(comment), + ] + ) + == 0 + ) + assert attached.read_text(encoding="utf-8") == "scripts/ci/a.py:1\n" + record_attached_receipt(tmp_path / "skip.txt", "../escape.py", 1) + record_attached_receipt(tmp_path / "skip.txt", "scripts/ci/a.py", 0) + assert not (tmp_path / "skip.txt").exists() + assert main(["--record-attach"]) == 2 + string_line = tmp_path / "string-line.json" + string_line.write_text( + json.dumps({"comments": [{"path": "scripts/ci/a.py", "line": "1"}]}), + encoding="utf-8", + ) + dest_empty = tmp_path / "empty-attach.txt" + assert ( + main( + [ + "--record-attach", + "--attached-locations", + str(dest_empty), + "--comment-file", + str(string_line), + ] + ) + == 0 + ) + assert not dest_empty.exists() or dest_empty.read_text(encoding="utf-8") == "" + assert ( + main( + [ + "--record-attach", + "--attached-locations", + str(attached), + "--comment-file", + str(tmp_path / "missing-comment.json"), + ] + ) + == 2 + ) + bad_comment = tmp_path / "bad-comment.json" + bad_comment.write_text("{}", encoding="utf-8") + assert ( + main( + [ + "--record-attach", + "--attached-locations", + str(attached), + "--comment-file", + str(bad_comment), + ] + ) + == 2 + ) + + control_path = tmp_path / "control.json" + body_path = tmp_path / "body.md" + output_path = tmp_path / "out.md" + refused = tmp_path / "refused.txt" + control_path.write_text( + json.dumps( + control( + {"path": "scripts/ci/a.py", "line": 1}, + {"path": "scripts/ci/b.py", "line": 2}, + {"path": "scripts/ci/c.py", "line": 3}, + ) + ), + encoding="utf-8", + ) + body_path.write_text("## Findings\n", encoding="utf-8") + refused.write_text( + "scripts/ci/b.py:2\tGitHub HTTP 422: Line could not be resolved\n", + encoding="utf-8", + ) + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + "--refused-locations", + str(refused), + "--attached-locations", + str(attached), + "--deferred-locations", + str(deferred), + "--retry-limit", + "1", + ] + ) + == 0 + ) + written = output_path.read_text(encoding="utf-8") + assert "- `scripts/ci/a.py:1`" in written + assert "- `scripts/ci/b.py:2` — GitHub HTTP 422: Line could not be resolved" in written + assert "- `scripts/ci/c.py:3`" not in written + assert "scripts/ci/b.py:2`" in written or "were still refused" in written + assert "were not retried (retry limit 1):" in written + assert "- `scripts/ci/b.py:2`" in written or "scripts/ci/b.py:2" in written + assert main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + "--attached-locations", + str(tmp_path / "missing-attached.txt"), + ] + ) == 2 + assert main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(output_path), + "--deferred-locations", + str(tmp_path / "missing-deferred.txt"), + ] + ) == 2 + + +EXAMPLE_UNIFIED_DIFF = """\ +diff --git a/scripts/ci/example.py b/scripts/ci/example.py +index 1111111..2222222 100644 +--- a/scripts/ci/example.py ++++ b/scripts/ci/example.py +@@ -5,7 +5,8 @@ def run(): + keep + keep + keep +- old ++ new + keep + keep + keep +diff --git a/scripts/ci/removed.py b/scripts/ci/removed.py +index 3333333..0000000 100644 +--- a/scripts/ci/removed.py ++++ /dev/null +@@ -10,3 +0,0 @@ leftover +-gone +-gone +-gone +diff --git a/scripts/ci/added.py b/scripts/ci/added.py +new file mode 100644 +index 0000000..4444444 +--- /dev/null ++++ b/scripts/ci/added.py +@@ -0,0 +1 @@ ++created +diff --git a/old/name.py b/scripts/ci/renamed.py +similarity index 90% +rename from old/name.py +rename to scripts/ci/renamed.py +index 5555555..6666666 100644 +--- a/old/name.py ++++ b/scripts/ci/renamed.py +@@ -2 +2 @@ +-old ++new +""" + + +def test_parse_unified_diff_hunk_lines_covers_github_commentable_ranges(): + hunks = parse_unified_diff_hunk_lines(EXAMPLE_UNIFIED_DIFF) + + assert hunks["scripts/ci/example.py"]["RIGHT"] == set(range(5, 13)) + assert hunks["scripts/ci/example.py"]["LEFT"] == set(range(5, 12)) + assert hunks["scripts/ci/removed.py"]["LEFT"] == {10, 11, 12} + assert hunks["scripts/ci/removed.py"]["RIGHT"] == set() + assert hunks["scripts/ci/added.py"]["RIGHT"] == {1} + assert hunks["scripts/ci/added.py"]["LEFT"] == set() + assert hunks["scripts/ci/renamed.py"]["RIGHT"] == {2} + assert hunks["old/name.py"]["LEFT"] == {2} + assert parse_unified_diff_hunk_lines("") == {} + assert parse_unified_diff_hunk_lines("+++ not-a-path\n--- also-bad\n") == {} + assert unified_diff_has_content("") is False + assert unified_diff_has_content("notes only\n") is False + assert unified_diff_has_content("Binary files a/icon.png and b/icon.png differ\n") + assert unified_diff_has_content("+++ not-a-path\n") + assert comment_on_changed_hunk("scripts/ci/example.py", 5, hunks) + assert comment_on_changed_hunk("scripts/ci/example.py", 12, hunks) + assert not comment_on_changed_hunk("scripts/ci/example.py", 20, hunks) + assert comment_on_changed_hunk( + "scripts/ci/removed.py", 11, hunks, side="LEFT" + ) + assert not comment_on_changed_hunk( + "scripts/ci/removed.py", 11, hunks, side="RIGHT" + ) + assert not comment_on_changed_hunk("../escape.py", 1, hunks) + assert not comment_on_changed_hunk("scripts/ci/example.py", 0, hunks) + assert comment_on_changed_hunk( + "scripts/ci/example.py", 7, hunks, side="NOPE" + ) + + +def test_filter_payload_comments_to_hunks_drops_off_hunk_before_post(): + hunks = parse_unified_diff_hunk_lines(EXAMPLE_UNIFIED_DIFF) + payload = _batch_payload( + {"path": "scripts/ci/example.py", "line": 7, "side": "RIGHT", "body": "on hunk"}, + {"path": "scripts/ci/example.py", "line": 20, "side": "RIGHT", "body": "past hunk"}, + {"path": "scripts/ci/example.py", "line": 20, "side": "RIGHT", "body": "duplicate skip"}, + {"path": "scripts/ci/removed.py", "line": 11, "side": "LEFT", "body": "deleted"}, + {"path": "scripts/ci/missing.py", "line": 3, "body": "unchanged path"}, + {"path": "../escape.py", "line": 1, "body": "unsafe"}, + "not-an-object", + ) + + filtered, skipped = filter_payload_comments_to_hunks(payload, hunks) + assert [item["line"] for item in filtered["comments"]] == [7, 11] + assert skipped == [ + ("scripts/ci/example.py", 20), + ("scripts/ci/missing.py", 3), + ] + unchanged, no_skip = filter_payload_comments_to_hunks(payload, {}) + assert unchanged["comments"] == payload["comments"] + assert no_skip == [] + no_comments, empty_skip = filter_payload_comments_to_hunks( + {"event": "COMMENT", "comments": "bad"}, hunks + ) + assert no_comments["comments"] == "bad" + assert empty_skip == [] + + +def test_skipped_receipts_list_off_hunk_locations(): + body = render_inline_comment_failure_body( + "## Findings\n", + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "scripts/ci/example.py", "line": 20}, + {"path": "scripts/ci/ok.py", "line": 4}, + ), + attached_locations=[("scripts/ci/ok.py", 4)], + skipped_locations=[ + ("scripts/ci/example.py", 20), + ("scripts/ci/foreign.py", 1), + ], + ) + assert "GitHub accepted these trusted current-head finding locations:" in body + assert "- `scripts/ci/ok.py:4`" in body + assert ( + "were not posted because they sit outside every current-head changed hunk:" + in body + ) + assert "- `scripts/ci/example.py:20`" in body + assert "scripts/ci/foreign.py" not in body + skipped_only = render_inline_comment_failure_body( + "## Findings\n", + control({"path": "scripts/ci/example.py", "line": 20}), + skipped_locations=[("scripts/ci/example.py", 20)], + ) + assert ( + "were not posted because they sit outside every current-head changed hunk:" + in skipped_only + ) + assert "did not copy suggested diffs" in skipped_only + assert "did not accept the inline review comments" not in skipped_only + skipped_with_deferred = render_inline_comment_failure_body( + "## Findings\n", + control( + {"path": "scripts/ci/later.py", "line": 20}, + {"path": "scripts/ci/example.py", "line": 20}, + ), + deferred_locations=[("scripts/ci/later.py", 20)], + skipped_locations=[("scripts/ci/example.py", 20)], + retry_limit=1, + ) + assert "were not retried (retry limit 1):" in skipped_with_deferred + assert ( + "were not posted because they sit outside every current-head changed hunk:" + in skipped_with_deferred + ) + + +def test_cli_filters_payload_to_current_head_hunks(tmp_path): + payload = tmp_path / "batch.json" + payload.write_text( + json.dumps( + _batch_payload( + { + "path": "scripts/ci/example.py", + "line": 7, + "side": "RIGHT", + "body": "keep", + }, + { + "path": "scripts/ci/example.py", + "line": 20, + "side": "RIGHT", + "body": "drop", + }, + ) + ), + encoding="utf-8", + ) + hunks_diff = tmp_path / "hunks.diff" + hunks_diff.write_text(EXAMPLE_UNIFIED_DIFF, encoding="utf-8") + output = tmp_path / "filtered.json" + skipped = tmp_path / "skipped.txt" + + assert ( + main( + [ + "--filter-hunks", + "--payload", + str(payload), + "--hunks-diff", + str(hunks_diff), + "--output", + str(output), + "--skipped-locations", + str(skipped), + ] + ) + == 0 + ) + filtered = json.loads(output.read_text(encoding="utf-8")) + assert [item["line"] for item in filtered["comments"]] == [7] + assert skipped.read_text(encoding="utf-8") == "scripts/ci/example.py:20\n" + assert write_hunk_filtered_payload( + json.loads(payload.read_text(encoding="utf-8")), + parse_unified_diff_hunk_lines(EXAMPLE_UNIFIED_DIFF), + tmp_path / "again.json", + ) == 1 + binary_diff = tmp_path / "binary.diff" + binary_diff.write_text( + "diff --git a/icon.png b/icon.png\n" + "Binary files a/icon.png and b/icon.png differ\n", + encoding="utf-8", + ) + binary_out = tmp_path / "binary.json" + binary_skipped = tmp_path / "binary-skipped.txt" + assert ( + main( + [ + "--filter-hunks", + "--payload", + str(payload), + "--hunks-diff", + str(binary_diff), + "--output", + str(binary_out), + "--skipped-locations", + str(binary_skipped), + ] + ) + == 0 + ) + assert json.loads(binary_out.read_text(encoding="utf-8"))["comments"] == [] + assert "scripts/ci/example.py:7" in binary_skipped.read_text(encoding="utf-8") + empty_diff = tmp_path / "empty.diff" + empty_diff.write_text("", encoding="utf-8") + empty_out = tmp_path / "empty.json" + assert ( + main( + [ + "--filter-hunks", + "--payload", + str(payload), + "--hunks-diff", + str(empty_diff), + "--output", + str(empty_out), + ] + ) + == 0 + ) + assert len(json.loads(empty_out.read_text(encoding="utf-8"))["comments"]) == 2 + assert main(["--filter-hunks"]) == 2 + assert ( + main( + [ + "--filter-hunks", + "--payload", + str(tmp_path / "missing.json"), + "--hunks-diff", + str(hunks_diff), + "--output", + str(output), + ] + ) + == 2 + ) + + control_path = tmp_path / "control.json" + body_path = tmp_path / "body.md" + receipt = tmp_path / "receipt.md" + control_path.write_text( + json.dumps(control({"path": "scripts/ci/example.py", "line": 20})), + encoding="utf-8", + ) + body_path.write_text("## Findings\n", encoding="utf-8") + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(receipt), + "--skipped-locations", + str(skipped), + ] + ) + == 0 + ) + assert "scripts/ci/example.py:20" in receipt.read_text(encoding="utf-8") + assert ( + main( + [ + "--control", + str(control_path), + "--body", + str(body_path), + "--output", + str(receipt), + "--skipped-locations", + str(tmp_path / "missing-skipped.txt"), + ] + ) + == 2 + ) + + +SUGGESTED_DIFF_BODY = """\ +### HIGH replace old line + +- Location: `scripts/ci/example.py:7` +- Problem: The old line is wrong. +- Root cause: The review found the current-head hunk. +- Fix: Replace the old line. +- Regression test: Keep the hunk prefilter. + +#### Suggested diff +```diff +@@ -7 +7 @@ +- old ++ new +``` +""" + + +def test_extract_suggestion_replacement_from_unified_and_plain_diffs(): + assert ( + extract_suggestion_replacement("@@ -7 +7 @@\n- old\n+ new\n") + == " new" + ) + assert extract_suggestion_replacement( + "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -1,2 +1,3 @@\n keep\n-old\n+new1\n+new2\n" + ) == "new1\nnew2" + assert extract_suggestion_replacement("plain replacement") == "plain replacement" + assert extract_suggestion_replacement("Cannot provide diff - inaccessible") is None + assert extract_suggestion_replacement("n/a") is None + assert extract_suggestion_replacement("") is None + assert extract_suggestion_replacement("- only removed\n") is None + assert extract_suggestion_replacement("+has ``` fence") is None + assert extract_suggestion_replacement("plain ``` no") is None + assert render_github_suggestion_block(" new") == "```suggestion\n new\n```" + + +def test_apply_github_suggestion_blocks_on_surviving_right_comments(): + payload = _batch_payload( + { + "path": "scripts/ci/example.py", + "line": 7, + "side": "RIGHT", + "body": SUGGESTED_DIFF_BODY, + }, + { + "path": "scripts/ci/removed.py", + "line": 11, + "side": "LEFT", + "body": SUGGESTED_DIFF_BODY, + }, + { + "path": "scripts/ci/plain.py", + "line": 4, + "side": "RIGHT", + "body": "no suggested diff here", + }, + { + "path": "scripts/ci/done.py", + "line": 2, + "side": "RIGHT", + "body": "already\n\n```suggestion\nkept\n```\n", + }, + { + "path": "scripts/ci/mention.py", + "line": 3, + "side": "RIGHT", + "body": "Authors should use a ```suggestion fence.\n\n```diff\n+fixed\n```\n", + }, + "not-an-object", + ) + updated = apply_github_suggestion_blocks(payload) + bodies = [item["body"] for item in updated["comments"] if isinstance(item, dict)] + assert "```suggestion\n new\n```" in bodies[0] + assert "```suggestion" not in bodies[1] + assert bodies[2] == "no suggested diff here" + assert bodies[3].count("```suggestion") == 1 + assert "```suggestion\nfixed\n```" in bodies[4] + mention_only = "Authors should use a ```suggestion fence.\n" + assert not body_has_suggestion_fence(mention_only) + assert apply_github_suggestion_blocks({"comments": "bad"}) == {"comments": "bad"} + second_fence = apply_github_suggestion_blocks( + _batch_payload( + { + "path": "scripts/ci/example.py", + "line": 7, + "side": "RIGHT", + "body": "```diff\nn/a\n```\n\n```diff\n+fixed\n```\n", + } + ) + ) + assert "```suggestion\nfixed\n```" in second_fence["comments"][0]["body"]