From ab69188c9c5960e6fbc7d9d7694451262a96f444 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 15:59:52 +0900 Subject: [PATCH 01/13] fix(review): cite trusted path:line in GitHub 422 inline fallback When GitHub refuses inline review comments, the PR-level fallback now lists each sanitized current-head finding location instead of a generic sentence. Suggested diffs stay out of the body. --- .../workflows/opencode-review-dispatch.yml | 12 +- CHANGELOG.md | 1 + .../review-inline-comment-422-fallback.md | 54 ++++++ .../ci/opencode_inline_comment_fallback.py | 133 ++++++++++++++ scripts/ci/test_strix_quick_gate.sh | 2 + tests/test_opencode_agent_contract.py | 5 + .../test_opencode_inline_comment_fallback.py | 171 ++++++++++++++++++ 7 files changed, 372 insertions(+), 6 deletions(-) create mode 100644 docs/doctoring/review-inline-comment-422-fallback.md create mode 100644 scripts/ci/opencode_inline_comment_fallback.py create mode 100644 tests/test_opencode_inline_comment_fallback.py diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 83f6830d5..7a72d4a94 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5766,12 +5766,12 @@ jobs: build_inline_comment_failure_body() { local body_file="$1" local output_file="$2" + local control_json="$3" - { - 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" + python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --control "$control_json" \ + --body "$body_file" \ + --output "$output_file" } publish_request_changes_from_control() { @@ -5785,7 +5785,7 @@ 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" + 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" rm -f "$body_file" "$payload_file" "$fallback_body_file" } diff --git a/CHANGELOG.md b/CHANGELOG.md index bf30091dd..dac72d56d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- 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..6375bf801 --- /dev/null +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -0,0 +1,54 @@ +# 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 + +`scripts/ci/opencode_inline_comment_fallback.py` reads the trusted control +JSON, keeps first-seen safe relative `path` plus positive integer `line` +pairs, and appends them to the fallback body as `` `path:line` `` list +items. Unsafe paths (`..`, absolute, drive, backslash) and non-positive +lines are omitted. An empty location set is stated explicitly. + +The publisher calls this helper from `build_inline_comment_failure_body` +with the same control object used to build the inline `comments` array. +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, the empty-set sentence, CLI success, and fail-closed + unreadable control input. +- `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 diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py new file mode 100644 index 000000000..c9ab8f328 --- /dev/null +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -0,0 +1,133 @@ +#!/usr/bin/env python3 +"""Render a GitHub 422 inline-comment fallback that cites trusted path:line.""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path, PurePosixPath, PureWindowsPath +from typing import Any + + +def safe_finding_path(raw_path: object) -> str | None: + """Return a repository-relative finding path, or None when it is unsafe.""" + if not isinstance(raw_path, str): + return None + path = raw_path.strip() + posix_path = PurePosixPath(path) + windows_path = PureWindowsPath(path) + if ( + not path + or "\\" in path + or path.startswith(("/", "//")) + or posix_path.is_absolute() + or windows_path.is_absolute() + or bool(windows_path.drive) + or ".." in posix_path.parts + or path != posix_path.as_posix() + ): + return None + return path + + +def safe_finding_line(raw_line: object) -> int | None: + """Return a positive integer finding line, or None when it is not one.""" + if isinstance(raw_line, bool) or not isinstance(raw_line, int) or raw_line <= 0: + return None + return raw_line + + +def trusted_finding_locations(control: dict[str, Any]) -> list[tuple[str, int]]: + """Return unique sanitized finding path:line pairs in first-seen order.""" + findings = control.get("findings") + if not isinstance(findings, list): + return [] + locations: list[tuple[str, int]] = [] + seen: set[tuple[str, int]] = set() + for finding in findings: + if not isinstance(finding, dict): + continue + path = safe_finding_path(finding.get("path")) + line = safe_finding_line(finding.get("line")) + if path is None or line is None: + continue + location = (path, line) + if location in seen: + continue + seen.add(location) + locations.append(location) + return locations + + +def render_inline_comment_failure_suffix(locations: list[tuple[str, int]]) -> str: + """Return the PR-body suffix used when GitHub rejects inline comments.""" + lines = [ + "", + "## Inline comment publishing failed", + "", + ] + if locations: + lines.append( + "GitHub did not accept the inline review comments for these " + "trusted current-head finding locations:" + ) + lines.append("") + lines.extend(f"- `{path}:{line}`" for path, line in 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." + ) + else: + 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." + ) + lines.append("") + return "\n".join(lines) + + +def render_inline_comment_failure_body(body: str, control: dict[str, Any]) -> str: + """Append the 422 fallback suffix to an existing REQUEST_CHANGES body.""" + return body.rstrip("\n") + render_inline_comment_failure_suffix( + trusted_finding_locations(control) + ) + + +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 a REQUEST_CHANGES body plus the exact path:line 422 suffix.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--control", required=True, type=Path) + parser.add_argument("--body", required=True, type=Path) + parser.add_argument("--output", required=True, type=Path) + args = parser.parse_args(argv) + try: + control = load_control(args.control) + body = args.body.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), 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..0507fafde 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1475,6 +1475,8 @@ assert_opencode_review_posts_suggested_diffs_inline() { 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 "$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" "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_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index daeaa37a2..c3d9978ff 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1609,6 +1609,11 @@ 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 "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..6ee60354d --- /dev/null +++ b/tests/test_opencode_inline_comment_fallback.py @@ -0,0 +1,171 @@ +import json +import runpy +import sys + +import pytest + +from scripts.ci.opencode_inline_comment_fallback import ( + main, + render_inline_comment_failure_body, + trusted_finding_locations, +) + + +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}, + "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_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 + + +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 + + 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 + ) + + 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 From e099c28cf2d371df376c2f38dee05c309decf90b Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:07:34 +0900 Subject: [PATCH 02/13] fix(review): persist 422 inline failures as overview receipts Rebuild the fallback from gh api stderr after a refused attach so the OpenCode overview keeps each trusted path:line next to the GitHub 422 phrase instead of a location-only list. --- .../workflows/opencode-review-dispatch.yml | 22 +++- CHANGELOG.md | 1 + .../review-inline-comment-422-fallback.md | 13 +- .../ci/opencode_inline_comment_fallback.py | 100 ++++++++++++++- scripts/ci/test_strix_quick_gate.sh | 1 + tests/test_opencode_agent_contract.py | 5 + .../test_opencode_inline_comment_fallback.py | 120 ++++++++++++++++++ 7 files changed, 249 insertions(+), 13 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 7a72d4a94..9f7355a66 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5625,6 +5625,8 @@ jobs: 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 @@ -5640,6 +5642,10 @@ jobs: 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" + 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" || true + fi rm -f "$gh_error_file" "$review_response_file" if [ "${REVIEW_PUBLICATION_STALE_HEAD:-}" = "1" ]; then printf '::error::OpenCode inline review publication stopped because PR head advanced beyond %s.\n' "$HEAD_SHA" @@ -5767,11 +5773,19 @@ jobs: local body_file="$1" local output_file="$2" local control_json="$3" + local error_file="${4:-}" + local -a fallback_args - python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ - --control "$control_json" \ - --body "$body_file" \ + fallback_args=( + python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" + --control "$control_json" + --body "$body_file" --output "$output_file" + ) + if [ -n "$error_file" ]; then + fallback_args+=(--error-file "$error_file") + fi + "${fallback_args[@]}" } publish_request_changes_from_control() { @@ -5786,7 +5800,7 @@ jobs: 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" "$control_json" - create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" + 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/CHANGELOG.md b/CHANGELOG.md index dac72d56d..a175254c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- 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. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index 6375bf801..f683d6540 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -20,6 +20,14 @@ pairs, and appends them to the fallback body as `` `path:line` `` list items. Unsafe paths (`..`, absolute, drive, backslash) and non-positive lines are omitted. An empty location set is stated explicitly. +After a refused attach, the publisher rebuilds the fallback from the +`gh api` error file and writes durable receipts into the OpenCode +overview comment (``). Each receipt is +`` `path:line` — GitHub HTTP 422: ``. The phrase prefers JSON +`errors[].message` (for example `pull_request_review_thread.path is +invalid`) and otherwise the first `HTTP 422` line. URLs are stripped and +the phrase is bounded to 240 characters. + The publisher calls this helper from `build_inline_comment_failure_body` with the same control object used to build the inline `comments` array. Suggested diffs stay out of the PR-level body. @@ -27,8 +35,9 @@ 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, the empty-set sentence, CLI success, and fail-closed - unreadable control input. + the exact location list, GitHub JSON `errors[].message` phrases, HTTP 422 + line fallback, empty-set sentence, CLI success with `--error-file`, and + fail-closed unreadable control or error input. - `tests/test_opencode_agent_contract.py` and `scripts/ci/test_strix_quick_gate.sh` pin the workflow call with `$control_json`. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index c9ab8f328..6eaa9411a 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -5,10 +5,14 @@ import argparse import json +import re import sys from pathlib import Path, PurePosixPath, PureWindowsPath from typing import Any +ERROR_PHRASE_MAX_CHARS = 240 +HTTP_422_LINE_RE = re.compile(r"(?im)^(?:gh:\s*)?(.*HTTP 422.*)$") + def safe_finding_path(raw_path: object) -> str | None: """Return a repository-relative finding path, or None when it is unsafe.""" @@ -60,11 +64,78 @@ def trusted_finding_locations(control: dict[str, Any]) -> list[tuple[str, int]]: return locations -def render_inline_comment_failure_suffix(locations: list[tuple[str, int]]) -> str: +def _collapse_error_text(text: str) -> str: + """Return one-line error text without URLs or extra whitespace.""" + without_urls = re.sub(r"https?://\S+", "", text) + return " ".join(without_urls.split()) + + +def github_publication_error_phrase(text: str) -> str: + """Return a bounded GitHub 422 phrase from ``gh api`` stderr or JSON.""" + raw = text or "" + messages: list[str] = [] + seen: set[str] = set() + decoder = json.JSONDecoder() + index = 0 + while index < len(raw): + start = raw.find("{", index) + if start < 0: + break + try: + value, consumed = decoder.raw_decode(raw[start:]) + except json.JSONDecodeError: + index = start + 1 + continue + index = start + consumed + errors = value.get("errors") + if not isinstance(errors, list): + continue + for item in errors: + if not isinstance(item, dict) or not isinstance(item.get("message"), str): + continue + message = _collapse_error_text(item["message"]) + if not message or message in seen: + continue + seen.add(message) + messages.append(message) + if messages: + return f"GitHub HTTP 422: {'; '.join(messages)}"[:ERROR_PHRASE_MAX_CHARS] + match = HTTP_422_LINE_RE.search(raw) + if match: + line = _collapse_error_text(match.group(1)) + if line.casefold().startswith("github http 422"): + return line[:ERROR_PHRASE_MAX_CHARS] + return f"GitHub HTTP 422: {line}".rstrip(": ")[:ERROR_PHRASE_MAX_CHARS] + if "422" in raw: + return "GitHub HTTP 422" + return "GitHub review write failed" + + +def render_inline_comment_receipts( + locations: list[tuple[str, int]], error_phrase: str +) -> list[str]: + """Return durable overview receipt lines for refused inline comments.""" + if not locations: + return [] + if error_phrase: + return [f"- `{path}:{line}` — {error_phrase}" for path, line in locations] + return [f"- `{path}:{line}`" for path, line in locations] + + +def render_inline_comment_failure_suffix( + locations: list[tuple[str, int]], + *, + error_phrase: str = "", +) -> str: """Return the PR-body suffix used when GitHub rejects inline comments.""" + heading = ( + "## Inline comment publication receipts" + if error_phrase + else "## Inline comment publishing failed" + ) lines = [ "", - "## Inline comment publishing failed", + heading, "", ] if locations: @@ -73,7 +144,7 @@ def render_inline_comment_failure_suffix(locations: list[tuple[str, int]]) -> st "trusted current-head finding locations:" ) lines.append("") - lines.extend(f"- `{path}:{line}`" for path, line in locations) + lines.extend(render_inline_comment_receipts(locations, error_phrase)) lines.append("") lines.append( "OpenCode did not copy suggested diffs into this PR-level body. " @@ -88,14 +159,24 @@ def render_inline_comment_failure_suffix(locations: list[tuple[str, int]]) -> st "workflow log and apply any remaining blockers from the review " "body manually." ) + if error_phrase: + lines.append("") + lines.append(f"- GitHub error: {error_phrase}") lines.append("") return "\n".join(lines) -def render_inline_comment_failure_body(body: str, control: dict[str, Any]) -> str: +def render_inline_comment_failure_body( + body: str, + control: dict[str, Any], + *, + error_text: str = "", +) -> str: """Append the 422 fallback suffix to an existing REQUEST_CHANGES body.""" + error_phrase = github_publication_error_phrase(error_text) if error_text else "" return body.rstrip("\n") + render_inline_comment_failure_suffix( - trusted_finding_locations(control) + trusted_finding_locations(control), + error_phrase=error_phrase, ) @@ -111,20 +192,25 @@ def load_control(path: Path) -> dict[str, Any]: def main(argv: list[str] | None = None) -> int: - """Write a REQUEST_CHANGES body plus the exact path:line 422 suffix.""" + """Write a REQUEST_CHANGES body plus path:line receipts and optional 422 phrase.""" parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--control", required=True, type=Path) parser.add_argument("--body", required=True, type=Path) parser.add_argument("--output", required=True, type=Path) + parser.add_argument("--error-file", type=Path) args = parser.parse_args(argv) try: 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 "" + ) except (OSError, UnicodeDecodeError, ValueError) as exc: print(exc, file=sys.stderr) return 2 args.output.write_text( - render_inline_comment_failure_body(body, control), encoding="utf-8" + render_inline_comment_failure_body(body, control, error_text=error_text), + encoding="utf-8", ) return 0 diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 0507fafde..9e8039879 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1477,6 +1477,7 @@ assert_opencode_review_posts_suggested_diffs_inline() { 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 "$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" "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_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index c3d9978ff..23f82bf05 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1614,6 +1614,11 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): '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 "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 index 6ee60354d..c08ee840b 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -5,8 +5,10 @@ import pytest from scripts.ci.opencode_inline_comment_fallback import ( + github_publication_error_phrase, main, render_inline_comment_failure_body, + render_inline_comment_receipts, trusted_finding_locations, ) @@ -58,12 +60,88 @@ def test_fallback_body_cites_each_trusted_path_line(): 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("https://api.github.example/HTTP 422") + == "GitHub HTTP 422: 422" + ) + assert render_inline_comment_receipts([], "GitHub HTTP 422") == [] + 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_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): @@ -94,6 +172,33 @@ def test_cli_writes_fallback_and_rejects_unreadable_control(tmp_path, monkeypatc 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 + ) + assert ( main( [ @@ -150,6 +255,21 @@ def test_cli_writes_fallback_and_rejects_unreadable_control(tmp_path, monkeypatc ) == 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, From d37885d88c8854faaf2edf706d30e20b015eec28 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:13:42 +0900 Subject: [PATCH 03/13] fix(review): retry inline comments one at a time after batch 422 A single invalid path:line 422s the whole comments array. After that failure, split the payload and retry each comment so surviving hunks still attach; remaining failures keep the overview receipts. --- .../workflows/opencode-review-dispatch.yml | 55 ++++++++ CHANGELOG.md | 1 + .../review-inline-comment-422-fallback.md | 16 ++- .../ci/opencode_inline_comment_fallback.py | 102 +++++++++++++- scripts/ci/test_strix_quick_gate.sh | 2 + tests/test_opencode_agent_contract.py | 3 + .../test_opencode_inline_comment_fallback.py | 128 ++++++++++++++++++ 7 files changed, 298 insertions(+), 9 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 9f7355a66..59017c88f 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5623,6 +5623,52 @@ jobs: exit 0 } + retry_inline_comments_one_at_a_time() { + local batch_payload_file="$1" review_body="$2" + local split_dir comment_file wrapped_file error_file response_file + local attached=0 + local found=0 + + split_dir="$(mktemp -d)" + if ! python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --split-payload "$batch_payload_file" \ + --output-dir "$split_dir"; 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 + 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 + } + create_pull_review_with_payload() { local event="$1" body="$2" review_payload_file="$3" fallback_body_file="$4" local source_body_file="${5:-}" @@ -5642,6 +5688,15 @@ jobs: 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" + if [ "${REVIEW_PUBLICATION_STALE_HEAD:-}" != "1" ] \ + && python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ + --is-unprocessable --error-file "$gh_error_file"; then + if retry_inline_comments_one_at_a_time "$review_payload_file" "$body"; then + rm -f "$gh_error_file" "$review_response_file" + update_review_overview "$event" "$body" + return 0 + fi + 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" || true diff --git a/CHANGELOG.md b/CHANGELOG.md index a175254c6..c3f91113b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- 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. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index f683d6540..35a3e1d21 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -20,9 +20,14 @@ pairs, and appends them to the fallback body as `` `path:line` `` list items. Unsafe paths (`..`, absolute, drive, backslash) and non-positive lines are omitted. An empty location set is stated explicitly. -After a refused attach, the publisher rebuilds the fallback from the -`gh api` error file and writes durable receipts into the OpenCode -overview comment (``). Each receipt is +After a refused attach, the publisher first checks that the failure is +HTTP 422, splits the batch `comments` array into single-comment review +payloads, and retries each with the same write helper. The first success +uses `REQUEST_CHANGES` plus the review body; later successes use +`COMMENT`. Survivors therefore still appear on Files changed. Remaining +failures still rebuild the fallback from the `gh api` error file and +write durable receipts into the OpenCode overview comment +(``). Each receipt is `` `path:line` — GitHub HTTP 422: ``. The phrase prefers JSON `errors[].message` (for example `pull_request_review_thread.path is invalid`) and otherwise the first `HTTP 422` line. URLs are stripped and @@ -36,8 +41,9 @@ Suggested diffs stay out of the PR-level body. - `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`, and - fail-closed unreadable control or error input. + line fallback, empty-set sentence, CLI success with `--error-file`, + fail-closed unreadable control or error input, batch-to-single comment + splitting, and `--is-unprocessable` classification. - `tests/test_opencode_agent_contract.py` and `scripts/ci/test_strix_quick_gate.sh` pin the workflow call with `$control_json`. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index 6eaa9411a..143cf39ad 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -111,6 +111,84 @@ def github_publication_error_phrase(text: str) -> str: return "GitHub review write failed" +def github_error_is_unprocessable(text: str) -> bool: + """Return whether GitHub rejected the review write as HTTP 422.""" + raw = text or "" + if "422" in raw or "Unprocessable Entity" in raw: + return True + return "422" in github_publication_error_phrase(raw) + + +def iter_single_comment_payloads(payload: dict[str, Any]) -> list[dict[str, Any]]: + """Return safe single-comment slices from a batch review payload.""" + comments = payload.get("comments") + commit_id = payload.get("commit_id") + if not isinstance(comments, list) or not isinstance(commit_id, str): + return [] + commit_id = commit_id.strip() + if not commit_id: + return [] + singles: list[dict[str, Any]] = [] + for comment in comments: + if not isinstance(comment, dict): + continue + path = safe_finding_path(comment.get("path")) + line = safe_finding_line(comment.get("line")) + body = comment.get("body") + if path is None or line is None or not isinstance(body, str) or not body.strip(): + continue + side = comment.get("side") + singles.append( + { + "path": path, + "line": line, + "side": side if side in {"LEFT", "RIGHT"} else "RIGHT", + "body": body, + "commit_id": commit_id, + } + ) + return singles + + +def render_single_comment_review( + item: dict[str, Any], + *, + event: str, + review_body: str, +) -> dict[str, Any]: + """Return one GitHub review payload that carries a single inline comment.""" + return { + "event": event, + "body": review_body, + "commit_id": item["commit_id"], + "comments": [ + { + "path": item["path"], + "line": item["line"], + "side": item["side"], + "body": item["body"], + } + ], + } + + +def write_single_comment_payloads(payload: dict[str, Any], output_dir: Path) -> int: + """Write COMMENT-event single-comment payloads and return the file count.""" + output_dir.mkdir(parents=True, exist_ok=True) + count = 0 + for index, item in enumerate(iter_single_comment_payloads(payload)): + path = output_dir / f"comment-{index:03d}.json" + path.write_text( + json.dumps( + render_single_comment_review(item, event="COMMENT", review_body=""), + ensure_ascii=True, + ), + encoding="utf-8", + ) + count += 1 + return count + + def render_inline_comment_receipts( locations: list[tuple[str, int]], error_phrase: str ) -> list[str]: @@ -192,14 +270,30 @@ def load_control(path: Path) -> dict[str, Any]: def main(argv: list[str] | None = None) -> int: - """Write a REQUEST_CHANGES body plus path:line receipts and optional 422 phrase.""" + """Write 422 fallback text or split a batch review into single comments.""" parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--control", required=True, type=Path) - parser.add_argument("--body", required=True, type=Path) - parser.add_argument("--output", required=True, type=Path) + 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") args = parser.parse_args(argv) try: + 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) + 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 = ( diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 9e8039879..74c26fe88 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1478,6 +1478,8 @@ assert_opencode_review_posts_suggested_diffs_inline() { 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" "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_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 23f82bf05..0d7a6881a 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1619,6 +1619,9 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): 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 "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 index c08ee840b..f32b1fc2c 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -5,10 +5,13 @@ import pytest from scripts.ci.opencode_inline_comment_fallback import ( + github_error_is_unprocessable, github_publication_error_phrase, + iter_single_comment_payloads, main, render_inline_comment_failure_body, render_inline_comment_receipts, + render_single_comment_review, trusted_finding_locations, ) @@ -289,3 +292,128 @@ def test_cli_writes_fallback_and_rejects_unreadable_control(tmp_path, monkeypatc "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("") + + +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 From 154a33d092e0ce23f5299981bef5fe12cc8cab41 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:18:33 +0900 Subject: [PATCH 04/13] test(review): pin 422 fallback sentence in the Python helper The publisher moved that phrase out of the workflow YAML, so the exact-head path-policy harness failed looking in the old file. --- scripts/ci/test_strix_quick_gate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 74c26fe88..801e16584 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1474,7 +1474,7 @@ 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 "$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" From dc261ff834a81f1fbd45e16bde1c86fe6c870c40 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:27:51 +0900 Subject: [PATCH 05/13] fix(review): receipt only refused path:line after mixed 422 retry When some one-at-a-time inline comments attach and others 422, the overview must list only the refused locations so attached hunks are not reported as failed. --- .../workflows/opencode-review-dispatch.yml | 34 +++++++- CHANGELOG.md | 1 + .../review-inline-comment-422-fallback.md | 7 +- .../ci/opencode_inline_comment_fallback.py | 68 +++++++++++++-- scripts/ci/test_strix_quick_gate.sh | 2 + tests/test_opencode_agent_contract.py | 2 + .../test_opencode_inline_comment_fallback.py | 83 +++++++++++++++++++ 7 files changed, 184 insertions(+), 13 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 59017c88f..83b4273eb 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5624,11 +5624,12 @@ jobs: } retry_inline_comments_one_at_a_time() { - local batch_payload_file="$1" review_body="$2" + local batch_payload_file="$1" review_body="$2" refused_locations_file="$3" local split_dir comment_file wrapped_file error_file response_file local attached=0 local found=0 + : >"$refused_locations_file" split_dir="$(mktemp -d)" if ! python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ --split-payload "$batch_payload_file" \ @@ -5655,6 +5656,12 @@ jobs: "$error_file" \ "$response_file"; then attached=1 + else + jq -r '.comments[0] | "\(.path):\(.line)"' "$comment_file" \ + >>"$refused_locations_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 @@ -5691,11 +5698,26 @@ jobs: if [ "${REVIEW_PUBLICATION_STALE_HEAD:-}" != "1" ] \ && python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ --is-unprocessable --error-file "$gh_error_file"; then - if retry_inline_comments_one_at_a_time "$review_payload_file" "$body"; then - rm -f "$gh_error_file" "$review_response_file" - update_review_overview "$event" "$body" + refused_locations_file="$(mktemp)" + if retry_inline_comments_one_at_a_time \ + "$review_payload_file" "$body" "$refused_locations_file"; then + if [ -s "$refused_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" || 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" return 0 fi + rm -f "$refused_locations_file" "${refused_locations_file}.errors" fi if [ -n "$source_body_file" ] && [ -n "$control_json" ]; then build_inline_comment_failure_body \ @@ -5829,6 +5851,7 @@ jobs: local output_file="$2" local control_json="$3" local error_file="${4:-}" + local refused_locations_file="${5:-}" local -a fallback_args fallback_args=( @@ -5840,6 +5863,9 @@ jobs: 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 "${fallback_args[@]}" } diff --git a/CHANGELOG.md b/CHANGELOG.md index c3f91113b..0509adc41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- 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. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index 35a3e1d21..237b906a2 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -27,7 +27,9 @@ uses `REQUEST_CHANGES` plus the review body; later successes use `COMMENT`. Survivors therefore still appear on Files changed. Remaining failures still rebuild the fallback from the `gh api` error file and write durable receipts into the OpenCode overview comment -(``). Each receipt is +(``). On mixed success the receipt list +contains only refused `path:line` rows, not the comments that already +attached. Each receipt is `` `path:line` — GitHub HTTP 422: ``. The phrase prefers JSON `errors[].message` (for example `pull_request_review_thread.path is invalid`) and otherwise the first `HTTP 422` line. URLs are stripped and @@ -43,7 +45,8 @@ Suggested diffs stay out of the PR-level body. 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, and `--is-unprocessable` classification. + splitting, `--is-unprocessable` classification, and mixed-success + receipts that omit attached path:line rows. - `tests/test_opencode_agent_contract.py` and `scripts/ci/test_strix_quick_gate.sh` pin the workflow call with `$control_json`. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index 143cf39ad..96a4fac2c 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -64,6 +64,31 @@ def trusted_finding_locations(control: dict[str, Any]) -> list[tuple[str, int]]: return locations +def parse_refused_locations(text: str) -> list[tuple[str, int]]: + """Parse ``path:line`` rows from one-at-a-time retry failures.""" + locations: list[tuple[str, int]] = [] + seen: set[tuple[str, int]] = set() + for raw_line in text.splitlines(): + line = raw_line.strip() + if not line or line.startswith("#") or ":" not in line: + continue + path_text, _, line_text = line.rpartition(":") + path = safe_finding_path(path_text) + try: + parsed_line = int(line_text) + except ValueError: + parsed_line = 0 + line_number = safe_finding_line(parsed_line) + if path is None or line_number is None: + continue + location = (path, line_number) + if location in seen: + continue + seen.add(location) + locations.append(location) + return locations + + def _collapse_error_text(text: str) -> str: """Return one-line error text without URLs or extra whitespace.""" without_urls = re.sub(r"https?://\S+", "", text) @@ -204,11 +229,12 @@ def render_inline_comment_failure_suffix( locations: list[tuple[str, int]], *, error_phrase: str = "", + mixed_success: bool = False, ) -> str: """Return the PR-body suffix used when GitHub rejects inline comments.""" heading = ( "## Inline comment publication receipts" - if error_phrase + if error_phrase or mixed_success else "## Inline comment publishing failed" ) lines = [ @@ -217,10 +243,16 @@ def render_inline_comment_failure_suffix( "", ] if locations: - lines.append( - "GitHub did not accept the inline review comments for these " - "trusted current-head finding locations:" - ) + if mixed_success: + 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)) lines.append("") @@ -249,12 +281,23 @@ def render_inline_comment_failure_body( control: dict[str, Any], *, error_text: str = "", + refused_locations: list[tuple[str, 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 "" + if refused_locations is None: + locations = trusted_finding_locations(control) + mixed_success = False + else: + allowed = set(trusted_finding_locations(control)) + locations = [item for item in refused_locations if item in allowed] + mixed_success = True + if not locations: + return body.rstrip("\n") + "\n" return body.rstrip("\n") + render_inline_comment_failure_suffix( - trusted_finding_locations(control), + locations, error_phrase=error_phrase, + mixed_success=mixed_success, ) @@ -279,6 +322,7 @@ def main(argv: list[str] | None = None) -> int: 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) args = parser.parse_args(argv) try: if args.is_unprocessable: @@ -299,11 +343,21 @@ def main(argv: list[str] | None = None) -> int: error_text = ( args.error_file.read_text(encoding="utf-8") if args.error_file else "" ) + refused_locations = ( + parse_refused_locations(args.refused_locations.read_text(encoding="utf-8")) + if args.refused_locations is not None + else None + ) 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), + render_inline_comment_failure_body( + body, + control, + error_text=error_text, + refused_locations=refused_locations, + ), encoding="utf-8", ) return 0 diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 801e16584..7d555c84c 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1475,11 +1475,13 @@ assert_opencode_review_posts_suggested_diffs_inline() { 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 "$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" "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_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 0d7a6881a..dcc06546d 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1622,6 +1622,8 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): 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 "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 index f32b1fc2c..9c240fef5 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -9,6 +9,7 @@ github_publication_error_phrase, iter_single_comment_payloads, main, + parse_refused_locations, render_inline_comment_failure_body, render_inline_comment_receipts, render_single_comment_review, @@ -133,6 +134,40 @@ def test_fallback_body_attaches_error_phrase_to_each_receipt(): ) +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("") == [] + 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_fallback_body_explains_missing_trusted_locations(): body = render_inline_comment_failure_body("overview\n", control()) @@ -201,6 +236,54 @@ def test_cli_writes_fallback_and_rejects_unreadable_control(tmp_path, monkeypatc "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( From 94ee03d507b014ba73840f9fca9052d60c75d0ee Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:34:11 +0900 Subject: [PATCH 06/13] fix(review): keep each refused comment's own GitHub 422 phrase Mixed one-at-a-time retries can fail for different reasons. Record path:line plus that comment's gh api error so the overview does not reuse one shared sentence for every refused hunk. --- .../workflows/opencode-review-dispatch.yml | 7 +- CHANGELOG.md | 1 + .../review-inline-comment-422-fallback.md | 14 +- .../ci/opencode_inline_comment_fallback.py | 128 +++++++++++-- scripts/ci/test_strix_quick_gate.sh | 1 + tests/test_opencode_agent_contract.py | 1 + .../test_opencode_inline_comment_fallback.py | 177 ++++++++++++++++++ 7 files changed, 303 insertions(+), 26 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 83b4273eb..e09f88fdc 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5657,8 +5657,11 @@ jobs: "$response_file"; then attached=1 else - jq -r '.comments[0] | "\(.path):\(.line)"' "$comment_file" \ - >>"$refused_locations_file" || true + 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 0509adc41..7050a3e23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- 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`. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index 237b906a2..5a6a747e0 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -29,11 +29,12 @@ failures still rebuild the fallback from the `gh api` error file and write durable receipts into the OpenCode overview comment (``). On mixed success the receipt list contains only refused `path:line` rows, not the comments that already -attached. Each receipt is -`` `path:line` — GitHub HTTP 422: ``. The phrase prefers JSON -`errors[].message` (for example `pull_request_review_thread.path is -invalid`) and otherwise the first `HTTP 422` line. URLs are stripped and -the phrase is bounded to 240 characters. +attached. Each refused row keeps the 422 phrase from that comment's own +`gh api` stderr (JSON `errors[].message` such as +`pull_request_review_thread.path is invalid`, or the first `HTTP 422` +line). A later comment's different GitHub error does not overwrite an +earlier one. URLs are stripped and each phrase is bounded to 240 +characters. The publisher calls this helper from `build_inline_comment_failure_body` with the same control object used to build the inline `comments` array. @@ -46,7 +47,8 @@ Suggested diffs stay out of the PR-level body. 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, and mixed-success - receipts that omit attached path:line rows. + receipts that omit attached path:line rows, and per-comment 422 + phrases recorded beside each refused location. - `tests/test_opencode_agent_contract.py` and `scripts/ci/test_strix_quick_gate.sh` pin the workflow call with `$control_json`. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index 96a4fac2c..15b857ae1 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -64,15 +64,18 @@ def trusted_finding_locations(control: dict[str, Any]) -> list[tuple[str, int]]: return locations -def parse_refused_locations(text: str) -> list[tuple[str, int]]: - """Parse ``path:line`` rows from one-at-a-time retry failures.""" - locations: list[tuple[str, int]] = [] +def parse_refused_receipts(text: str) -> list[tuple[str, int, str]]: + """Parse ``path:line`` or ``path:linephrase`` retry-failure rows.""" + receipts: list[tuple[str, int, str]] = [] seen: set[tuple[str, int]] = set() for raw_line in text.splitlines(): line = raw_line.strip() - if not line or line.startswith("#") or ":" not in line: + if not line or line.startswith("#"): + continue + loc_text, _sep, phrase = line.partition("\t") + if ":" not in loc_text: continue - path_text, _, line_text = line.rpartition(":") + path_text, _, line_text = loc_text.rpartition(":") path = safe_finding_path(path_text) try: parsed_line = int(line_text) @@ -85,8 +88,26 @@ def parse_refused_locations(text: str) -> list[tuple[str, int]]: if location in seen: continue seen.add(location) - locations.append(location) - return locations + receipts.append((path, line_number, phrase.strip())) + return receipts + + +def parse_refused_locations(text: str) -> list[tuple[str, int]]: + """Parse ``path:line`` rows from one-at-a-time retry failures.""" + return [(path, line) for path, line, _phrase in parse_refused_receipts(text)] + + +def record_refused_receipt( + dest: Path, path: str, line: int, error_text: str +) -> None: + """Append one refused ``path:line`` and its GitHub 422 phrase.""" + safe_path = safe_finding_path(path) + safe_line = safe_finding_line(line) + if safe_path is None or safe_line is None: + return + phrase = github_publication_error_phrase(error_text) + with dest.open("a", encoding="utf-8") as handle: + handle.write(f"{safe_path}:{safe_line}\t{phrase}\n") def _collapse_error_text(text: str) -> str: @@ -215,14 +236,25 @@ def write_single_comment_payloads(payload: dict[str, Any], output_dir: Path) -> def render_inline_comment_receipts( - locations: list[tuple[str, int]], error_phrase: str + 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 [] - if error_phrase: - return [f"- `{path}:{line}` — {error_phrase}" for path, line in locations] - return [f"- `{path}:{line}`" for path, line in locations] + 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 + if phrase: + lines.append(f"- `{path}:{line}` — {phrase}") + else: + lines.append(f"- `{path}:{line}`") + return lines def render_inline_comment_failure_suffix( @@ -230,6 +262,7 @@ def render_inline_comment_failure_suffix( *, error_phrase: str = "", mixed_success: bool = False, + phrases: dict[tuple[str, int], str] | None = None, ) -> str: """Return the PR-body suffix used when GitHub rejects inline comments.""" heading = ( @@ -254,7 +287,11 @@ def render_inline_comment_failure_suffix( "trusted current-head finding locations:" ) lines.append("") - lines.extend(render_inline_comment_receipts(locations, error_phrase)) + 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. " @@ -282,10 +319,27 @@ def render_inline_comment_failure_body( *, error_text: str = "", refused_locations: list[tuple[str, int]] | None = None, + refused_receipts: list[tuple[str, int, str]] | 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 "" - if refused_locations is None: + phrases: dict[tuple[str, int], str] | None = None + if refused_receipts is not None: + allowed = set(trusted_finding_locations(control)) + 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: + return body.rstrip("\n") + "\n" + elif refused_locations is None: locations = trusted_finding_locations(control) mixed_success = False else: @@ -298,6 +352,7 @@ def render_inline_comment_failure_body( locations, error_phrase=error_phrase, mixed_success=mixed_success, + phrases=phrases, ) @@ -323,8 +378,37 @@ def main(argv: list[str] | None = None) -> int: 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("--record-refusal", action="store_true") + parser.add_argument("--comment-file", type=Path) args = parser.parse_args(argv) try: + 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") @@ -343,11 +427,18 @@ def main(argv: list[str] | None = None) -> int: error_text = ( args.error_file.read_text(encoding="utf-8") if args.error_file else "" ) - refused_locations = ( - parse_refused_locations(args.refused_locations.read_text(encoding="utf-8")) - if args.refused_locations is not None - else None - ) + refused_locations = None + refused_receipts = 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 + ] except (OSError, UnicodeDecodeError, ValueError) as exc: print(exc, file=sys.stderr) return 2 @@ -357,6 +448,7 @@ def main(argv: list[str] | None = None) -> int: control, error_text=error_text, refused_locations=refused_locations, + refused_receipts=refused_receipts, ), encoding="utf-8", ) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 7d555c84c..788096d40 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1482,6 +1482,7 @@ assert_opencode_review_posts_suggested_diffs_inline() { 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" "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_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index dcc06546d..05c87c0d1 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1623,6 +1623,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): 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 "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 diff --git a/tests/test_opencode_inline_comment_fallback.py b/tests/test_opencode_inline_comment_fallback.py index 9c240fef5..b8bc2ec51 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -10,6 +10,8 @@ iter_single_comment_payloads, main, parse_refused_locations, + parse_refused_receipts, + record_refused_receipt, render_inline_comment_failure_body, render_inline_comment_receipts, render_single_comment_review, @@ -159,6 +161,13 @@ def test_mixed_success_receipts_list_only_refused_path_lines(): ) 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}), @@ -168,6 +177,174 @@ def test_mixed_success_receipts_list_only_refused_path_lines(): 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()) From 3f06fb8cbeae3e9941b891669a9ac22ff41ab71b Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:47:37 +0900 Subject: [PATCH 07/13] fix(review): cap inline retry at 20 and list attached path:line Unbounded one-at-a-time retry after a batch 422 can thrash GitHub, and mixed receipts listed only refused locations. Cap retries at 20, persist attached path:line beside refused ones, and record leftovers the cap left untried so the overview shows every outcome. --- .../workflows/opencode-review-dispatch.yml | 52 ++- CHANGELOG.md | 1 + .../review-inline-comment-422-fallback.md | 35 +- .../ci/opencode_inline_comment_fallback.py | 184 ++++++++++- scripts/ci/test_strix_quick_gate.sh | 4 + tests/test_opencode_agent_contract.py | 5 + .../test_opencode_inline_comment_fallback.py | 299 ++++++++++++++++++ 7 files changed, 541 insertions(+), 39 deletions(-) diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index e09f88fdc..587c73cd7 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5625,15 +5625,29 @@ jobs: 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)" - if ! python3 "$GITHUB_WORKSPACE/scripts/ci/opencode_inline_comment_fallback.py" \ - --split-payload "$batch_payload_file" \ - --output-dir "$split_dir"; then + 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 @@ -5656,6 +5670,12 @@ jobs: "$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 \ @@ -5702,25 +5722,32 @@ jobs: && 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"; then - if [ -s "$refused_locations_file" ] && [ -n "$source_body_file" ] && [ -n "$control_json" ]; then + "$review_payload_file" "$body" "$refused_locations_file" \ + "$attached_locations_file" "$deferred_locations_file"; then + if { [ -s "$refused_locations_file" ] || [ -s "$deferred_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" || true + "$mixed_error_file" "$refused_locations_file" \ + "$attached_locations_file" "$deferred_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" + "$refused_locations_file" "${refused_locations_file}.errors" \ + "$attached_locations_file" "$deferred_locations_file" return 0 fi - rm -f "$refused_locations_file" "${refused_locations_file}.errors" + 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 \ @@ -5855,6 +5882,8 @@ jobs: local control_json="$3" local error_file="${4:-}" local refused_locations_file="${5:-}" + local attached_locations_file="${6:-}" + local deferred_locations_file="${7:-}" local -a fallback_args fallback_args=( @@ -5862,6 +5891,7 @@ jobs: --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") @@ -5869,6 +5899,12 @@ jobs: 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 "${fallback_args[@]}" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 7050a3e23..555842d88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- 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. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index 5a6a747e0..3f3d0bae7 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -21,20 +21,22 @@ items. Unsafe paths (`..`, absolute, drive, backslash) and non-positive lines are omitted. An empty location set is stated explicitly. After a refused attach, the publisher first checks that the failure is -HTTP 422, splits the batch `comments` array into single-comment review -payloads, and retries each with the same write helper. The first success -uses `REQUEST_CHANGES` plus the review body; later successes use -`COMMENT`. Survivors therefore still appear on Files changed. Remaining -failures still rebuild the fallback from the `gh api` error file and -write durable receipts into the OpenCode overview comment -(``). On mixed success the receipt list -contains only refused `path:line` rows, not the comments that already -attached. Each refused row keeps the 422 phrase from that comment's own -`gh api` stderr (JSON `errors[].message` such as +HTTP 422, splits the batch `comments` array into at most 20 +single-comment review payloads (`OPENCODE_INLINE_COMMENT_RETRY_LIMIT`, +default 20), and retries each with the same write helper. Comments past +that cap are recorded as not retried instead of opening unbounded `gh +api` writes. The first success uses `REQUEST_CHANGES` plus the review +body; later successes use `COMMENT`. Survivors therefore still appear on +Files changed. Remaining failures still rebuild the fallback from the +`gh api` error file and write durable receipts into the OpenCode +overview comment (``). 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). A later comment's different GitHub error does not overwrite an -earlier one. URLs are stripped and each phrase is bounded to 240 -characters. +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. The publisher calls this helper from `build_inline_comment_failure_body` with the same control object used to build the inline `comments` array. @@ -46,9 +48,10 @@ Suggested diffs stay out of the PR-level body. 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, and mixed-success - receipts that omit attached path:line rows, and per-comment 422 - phrases recorded beside each refused location. + 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. - `tests/test_opencode_agent_contract.py` and `scripts/ci/test_strix_quick_gate.sh` pin the workflow call with `$control_json`. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index 15b857ae1..15ef5fd42 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -5,11 +5,13 @@ 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.*)$") @@ -97,6 +99,23 @@ def parse_refused_locations(text: str) -> list[tuple[str, int]]: return [(path, line) for path, line, _phrase in parse_refused_receipts(text)] +def single_comment_retry_limit(raw: object | None = None) -> int: + """Return a positive one-at-a-time retry cap, defaulting to 20.""" + if raw is None: + raw = os.environ.get("OPENCODE_INLINE_COMMENT_RETRY_LIMIT") + if isinstance(raw, bool): + return DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + if isinstance(raw, int) and raw > 0: + return raw + if isinstance(raw, str): + text = raw.strip() + if text.isdigit(): + value = int(text) + if value > 0: + return value + return DEFAULT_SINGLE_COMMENT_RETRY_LIMIT + + def record_refused_receipt( dest: Path, path: str, line: int, error_text: str ) -> None: @@ -110,6 +129,16 @@ def record_refused_receipt( handle.write(f"{safe_path}:{safe_line}\t{phrase}\n") +def record_attached_receipt(dest: Path, path: str, line: int) -> None: + """Append one attached ``path:line`` row.""" + safe_path = safe_finding_path(path) + safe_line = safe_finding_line(line) + if safe_path is None or safe_line is None: + return + with dest.open("a", encoding="utf-8") as handle: + handle.write(f"{safe_path}:{safe_line}\n") + + def _collapse_error_text(text: str) -> str: """Return one-line error text without URLs or extra whitespace.""" without_urls = re.sub(r"https?://\S+", "", text) @@ -218,11 +247,18 @@ def render_single_comment_review( } -def write_single_comment_payloads(payload: dict[str, Any], output_dir: Path) -> int: - """Write COMMENT-event single-comment payloads and return the file count.""" +def write_single_comment_payloads( + payload: dict[str, Any], + output_dir: Path, + limit: int | None = None, + deferred_path: Path | None = None, +) -> int: + """Write at most ``limit`` COMMENT payloads and leftover ``path:line`` rows.""" + items = iter_single_comment_payloads(payload) + cap = single_comment_retry_limit(limit) output_dir.mkdir(parents=True, exist_ok=True) count = 0 - for index, item in enumerate(iter_single_comment_payloads(payload)): + for index, item in enumerate(items[:cap]): path = output_dir / f"comment-{index:03d}.json" path.write_text( json.dumps( @@ -232,6 +268,11 @@ def write_single_comment_payloads(payload: dict[str, Any], output_dir: Path) -> encoding="utf-8", ) count += 1 + if deferred_path is not None: + deferred_path.write_text( + "".join(f"{item['path']}:{item['line']}\n" for item in items[cap:]), + encoding="utf-8", + ) return count @@ -263,11 +304,16 @@ def render_inline_comment_failure_suffix( 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, + 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 [] heading = ( "## Inline comment publication receipts" - if error_phrase or mixed_success + if error_phrase or mixed_success or attached or deferred else "## Inline comment publishing failed" ) lines = [ @@ -275,12 +321,22 @@ def render_inline_comment_failure_suffix( 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: - lines.append( - "GitHub accepted some inline comments. These trusted " - "current-head finding locations were still refused:" - ) + 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 " @@ -299,7 +355,7 @@ def render_inline_comment_failure_suffix( "current-head changed hunks, or inspect the workflow log/control " "JSON and apply the changes manually." ) - else: + elif not attached and not deferred: lines.append( "GitHub did not accept the inline review comments, and the " "control JSON had no trusted path:line findings. Inspect the " @@ -309,10 +365,44 @@ def render_inline_comment_failure_suffix( 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." + ) 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], @@ -320,12 +410,25 @@ def render_inline_comment_failure_body( 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, + 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 [] + ) phrases: dict[tuple[str, int], str] | None = None if refused_receipts is not None: - allowed = set(trusted_finding_locations(control)) locations = [ (path, line) for path, line, _phrase in refused_receipts @@ -337,22 +440,29 @@ def render_inline_comment_failure_body( if phrase and (path, line) in allowed } mixed_success = True - if not locations: + if not locations and not attached and not deferred: return body.rstrip("\n") + "\n" - elif refused_locations is None: + elif refused_locations is None and attached_locations is None and deferred_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: + return body.rstrip("\n") + "\n" else: - allowed = set(trusted_finding_locations(control)) locations = [item for item in refused_locations if item in allowed] mixed_success = True - if not locations: + if not locations and not attached and not deferred: 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, + retry_limit=retry_limit, ) @@ -378,10 +488,36 @@ def main(argv: list[str] | None = None) -> int: 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("--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) args = parser.parse_args(argv) try: + 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 @@ -418,7 +554,12 @@ def main(argv: list[str] | None = None) -> int: 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) + 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") @@ -429,6 +570,8 @@ def main(argv: list[str] | None = None) -> int: ) refused_locations = None refused_receipts = None + attached_locations = None + deferred_locations = None if args.refused_locations is not None: parsed_receipts = parse_refused_receipts( args.refused_locations.read_text(encoding="utf-8") @@ -439,6 +582,14 @@ def main(argv: list[str] | None = None) -> int: 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") + ) except (OSError, UnicodeDecodeError, ValueError) as exc: print(exc, file=sys.stderr) return 2 @@ -449,6 +600,9 @@ def main(argv: list[str] | None = None) -> int: error_text=error_text, refused_locations=refused_locations, refused_receipts=refused_receipts, + attached_locations=attached_locations, + deferred_locations=deferred_locations, + retry_limit=args.retry_limit, ), encoding="utf-8", ) diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 788096d40..bc4222f7f 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1483,6 +1483,10 @@ assert_opencode_review_posts_suggested_diffs_inline() { 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" "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_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 05c87c0d1..f6cc8329e 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1624,6 +1624,11 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): 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 "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 diff --git a/tests/test_opencode_inline_comment_fallback.py b/tests/test_opencode_inline_comment_fallback.py index b8bc2ec51..9da4d38af 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -5,17 +5,21 @@ import pytest from scripts.ci.opencode_inline_comment_fallback import ( + DEFAULT_SINGLE_COMMENT_RETRY_LIMIT, github_error_is_unprocessable, github_publication_error_phrase, iter_single_comment_payloads, main, parse_refused_locations, parse_refused_receipts, + record_attached_receipt, record_refused_receipt, render_inline_comment_failure_body, render_inline_comment_receipts, render_single_comment_review, + single_comment_retry_limit, trusted_finding_locations, + write_single_comment_payloads, ) @@ -677,3 +681,298 @@ def test_cli_splits_batch_payload_into_single_comment_files(tmp_path): 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 From cbc836275cc81067db6c1936d282a417bd235fcf Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 17:37:36 +0900 Subject: [PATCH 08/13] fix(review): require a real HTTP 422 before bounded retry A 422 substring in a SHA or issue number no longer starts the one-at-a-time retry (CWE-1288). Receipt phrases escape backticks and HTML metacharacters before they enter the overview body. --- CHANGELOG.md | 2 +- .../review-inline-comment-422-fallback.md | 9 +++- .../ci/opencode_inline_comment_fallback.py | 51 +++++++++++++------ ...st_materialize_base_python_requirements.py | 15 ++++++ .../test_opencode_inline_comment_fallback.py | 30 ++++++++++- 5 files changed, 88 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 555842d88..e7bef4cfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed -- 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. +- 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. A `422` substring inside a SHA or issue number no longer starts that retry (CWE-1288), and receipt phrases escape backticks and HTML metacharacters. - 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. diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index 3f3d0bae7..909718e23 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -21,7 +21,8 @@ items. Unsafe paths (`..`, absolute, drive, backslash) and non-positive lines are omitted. An empty location set is stated explicitly. After a refused attach, the publisher first checks that the failure is -HTTP 422, splits the batch `comments` array into at most 20 +HTTP 422 (not a bare `422` substring in a SHA or issue number; CWE-1288), +splits the batch `comments` array into at most 20 single-comment review payloads (`OPENCODE_INLINE_COMMENT_RETRY_LIMIT`, default 20), and retries each with the same write helper. Comments past that cap are recorded as not retried instead of opening unbounded `gh @@ -36,7 +37,8 @@ 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. +bounded to 240 characters. Backticks and HTML metacharacters are +escaped before the phrase is written into the overview body. The publisher calls this helper from `build_inline_comment_failure_body` with the same control object used to build the inline `comments` array. @@ -63,6 +65,9 @@ If GitHub later accepts off-diff comments, keep citing the attempted ## References (APA 7th) +MITRE. (2026). *CWE-1288: Improper validation of syntactic correctness of +input*. https://cwe.mitre.org/data/definitions/1288.html + 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. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index 15ef5fd42..a0e8d1fb7 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -145,6 +145,19 @@ def _collapse_error_text(text: str) -> str: return " ".join(without_urls.split()) +def escape_receipt_text(text: str) -> str: + """Escape HTML and Markdown metacharacters in a receipt phrase.""" + escaped = text + for character, replacement in ( + ("`", "\\u0060"), + ("<", "\\u003c"), + (">", "\\u003e"), + ("&", "\\u0026"), + ): + escaped = escaped.replace(character, replacement) + return escaped + + def github_publication_error_phrase(text: str) -> str: """Return a bounded GitHub 422 phrase from ``gh api`` stderr or JSON.""" raw = text or "" @@ -174,24 +187,32 @@ def github_publication_error_phrase(text: str) -> str: seen.add(message) messages.append(message) if messages: - return f"GitHub HTTP 422: {'; '.join(messages)}"[:ERROR_PHRASE_MAX_CHARS] - match = HTTP_422_LINE_RE.search(raw) - if match: - line = _collapse_error_text(match.group(1)) - if line.casefold().startswith("github http 422"): - return line[:ERROR_PHRASE_MAX_CHARS] - return f"GitHub HTTP 422: {line}".rstrip(": ")[:ERROR_PHRASE_MAX_CHARS] - if "422" in raw: - return "GitHub HTTP 422" - return "GitHub review write failed" + phrase = f"GitHub HTTP 422: {'; '.join(messages)}" + else: + match = HTTP_422_LINE_RE.search(raw) + if match: + line = _collapse_error_text(match.group(1)) + if line.casefold().startswith("github http 422"): + phrase = line + else: + phrase = f"GitHub HTTP 422: {line}".rstrip(": ") + else: + phrase = "GitHub review write failed" + return escape_receipt_text(phrase[:ERROR_PHRASE_MAX_CHARS]) def github_error_is_unprocessable(text: str) -> bool: - """Return whether GitHub rejected the review write as HTTP 422.""" + """Return whether GitHub rejected the review write as HTTP 422. + + CWE-1288: a bare ``422`` substring (commit SHA, issue number) is not + an HTTP status. Retry one-at-a-time only for a real ``HTTP 422`` + line, ``Unprocessable Entity``, or a JSON error phrase already + classified as GitHub HTTP 422. + """ raw = text or "" - if "422" in raw or "Unprocessable Entity" in raw: + if HTTP_422_LINE_RE.search(raw) or "Unprocessable Entity" in raw: return True - return "422" in github_publication_error_phrase(raw) + return github_publication_error_phrase(raw).startswith("GitHub HTTP 422") def iter_single_comment_payloads(payload: dict[str, Any]) -> list[dict[str, Any]]: @@ -292,7 +313,7 @@ def render_inline_comment_receipts( if not phrase: phrase = error_phrase if phrase: - lines.append(f"- `{path}:{line}` — {phrase}") + lines.append(f"- `{path}:{line}` — {escape_receipt_text(phrase)}") else: lines.append(f"- `{path}:{line}`") return lines @@ -364,7 +385,7 @@ def render_inline_comment_failure_suffix( ) if error_phrase: lines.append("") - lines.append(f"- GitHub error: {error_phrase}") + lines.append(f"- GitHub error: {escape_receipt_text(error_phrase)}") if deferred: if locations or attached: lines.append("") diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 8a383f0c2..8e230d018 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" @@ -644,6 +656,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 +703,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 +735,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_inline_comment_fallback.py b/tests/test_opencode_inline_comment_fallback.py index 9da4d38af..c7e9f8da6 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -98,12 +98,37 @@ def test_github_publication_error_phrase_falls_back_to_http_line(): 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("status code 422 only") + == "GitHub review write failed" + ) + assert ( + github_publication_error_phrase( + "gh: HTTP 403 Forbidden sha=154a33d092422abc issue #422" + ) + == "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 ( + github_publication_error_phrase( + '{"errors":[{"message":"path `