From ab69188c9c5960e6fbc7d9d7694451262a96f444 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 15:59:52 +0900 Subject: [PATCH 01/12] 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/12] 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/12] 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/12] 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 57c80f87ae4abd3895381ecbc3acbe8628f19410 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 16:29:54 +0900 Subject: [PATCH 05/12] fix(review): require HTTP 422 tokens, not a 422 substring CWE-1288: a commit SHA or issue number containing 422 must not start the one-at-a-time inline retry. Classify only HTTP 422 lines, Unprocessable Entity, or JSON error phrases. --- ARCHITECTURE.md | 80 +++++++++++++++++++ CHANGELOG.md | 2 +- CLAUDE.md | 3 + .../review-inline-comment-422-fallback.md | 8 ++ .../ci/opencode_inline_comment_fallback.py | 14 ++-- ...st_materialize_base_python_requirements.py | 16 ++++ .../test_opencode_inline_comment_fallback.py | 9 ++- 7 files changed, 125 insertions(+), 7 deletions(-) create mode 100644 ARCHITECTURE.md diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 000000000..f583d7db2 --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,80 @@ +# Architecture — ContextualWisdomLab `.github` + +This repository is the organization control plane. It is not naruon and it +does not own product data. Sibling products remain standalone modules; this +repo publishes org profile assets, reusable required workflows, and the +review/merge schedulers those products consume. + +## System context + +```mermaid +flowchart LR + Buyer["Commercial buyer / reviewer"] + Agents["Agents on AGENTS.md"] + Project["GitHub Project #1"] + Hub["This repo: org .github"] + Products["Owned products
naruon · orchestrator · engines"] + Runner["Required workflows in each repo context"] + + Buyer --> Hub + Agents --> Project + Agents --> Hub + Project --> Hub + Hub --> Runner + Runner --> Products + Products -->|"standalone or as module"| Buyer +``` + +## Inline-comment 422 classification gate + +```mermaid +flowchart TD + Err["gh api review write error"] + Kind{"HTTP 422 line, Unprocessable Entity, or JSON errors[].message?"} + Retry["Split batch and retry one comment"] + Fail["Do not treat SHA or issue 422 as unprocessable"] + + Err --> Kind + Kind -->|"yes"| Retry + Kind -->|"no"| Fail +``` + +CWE-1288: a bare `422` substring is not an HTTP status. + +## Control-plane data flow + +```mermaid +sequenceDiagram + participant PR as Pull request + participant RW as Required workflows + participant OC as OpenCode reviewer + participant SV as sandboxed_verify / web E2E + participant MS as Merge scheduler + + PR->>RW: pull_request_target on trusted base + RW->>OC: bounded evidence + NVIDIA NIM / OpenCode + OC->>SV: PoC command in isolated copy + SV-->>OC: redacted stdout/stderr + command metadata + OC-->>PR: APPROVE or request changes + MS->>PR: merge only on current-head approval + green checks +``` + +## Trust boundaries + +- Required review workflows execute **base-branch** scripts. +- Reviewer agents stay `edit: deny`. +- Logs redact credential shapes. They do not mask operational PII. +- LLM and scheduled agents bind `NVIDIA_NIM_API_KEY`. They never use + `COPILOT_GITHUB_TOKEN`. +- Rust remains the psychometric arithmetic owner. + +## Quality gates + +`scripts/ci/` ships with 100% statement/branch coverage and 100% +docstrings. + +## Related durable documents + +- [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) +- [`docs/doctoring/review-inline-comment-422-fallback.md`](docs/doctoring/review-inline-comment-422-fallback.md) +- [`docs/agent-github-project-protocol.md`](docs/agent-github-project-protocol.md) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3f91113b..ce2ef0fa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +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. +- 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. Classification now requires an `HTTP 422` line, `Unprocessable Entity`, or a JSON error phrase — a `422` substring inside a SHA or issue number no longer starts that retry (CWE-1288). - 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/CLAUDE.md b/CLAUDE.md index 1c7bdb2f6..8e48c382d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,6 +63,9 @@ Details: `README.md` and `PR_GOVERNANCE_AUDIT.md`. dependency sets (see below). - `fuzz/` + `.clusterfuzzlite/` — Atheris fuzz targets for the review-output normalizer and the ClusterFuzzLite discovery marker. +- `ARCHITECTURE.md` — control-plane mermaid (system context, 422 + classification gate, review sequence, trust boundaries). Reconstruct + from the repo, not private agent memory. - `docs/` — master context, Project protocol, `org-required-workflow-rollout.md`, `scorecard-governance.md`, SBOM inventory. - `.jules/` — recorded performance (`bolt.md`) and security (`sentinel.md`) learnings from past work diff --git a/docs/doctoring/review-inline-comment-422-fallback.md b/docs/doctoring/review-inline-comment-422-fallback.md index 35a3e1d21..c2c4c09ab 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -37,6 +37,11 @@ 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. +CWE-1288: classify a 422 only from an `HTTP 422` line, the +`Unprocessable Entity` reason phrase, or a JSON `errors[].message` +already rendered as `GitHub HTTP 422`. A bare `422` substring (commit +SHA, issue number) must not start the one-at-a-time retry. + ## Verification contract - `tests/test_opencode_inline_comment_fallback.py` pins safe-pair extraction, @@ -55,6 +60,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 143cf39ad..616beb827 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -106,17 +106,21 @@ def github_publication_error_phrase(text: str) -> str: 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 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, byte + offset) 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]]: diff --git a/tests/test_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 8a383f0c2..a9e974d12 100644 --- a/tests/test_materialize_base_python_requirements.py +++ b/tests/test_materialize_base_python_requirements.py @@ -30,6 +30,19 @@ 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") + materializer._install_trusted_uv.cache_clear() + + 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 +657,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 +704,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 +736,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 f32b1fc2c..592bda311 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -91,7 +91,10 @@ 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("https://api.github.example/HTTP 422") == "GitHub HTTP 422: 422" @@ -302,6 +305,10 @@ def test_github_error_is_unprocessable_detects_real_422_bodies(): assert github_error_is_unprocessable("gh: HTTP 422: Unprocessable Entity") assert not github_error_is_unprocessable("Resource not accessible by integration") assert not github_error_is_unprocessable("") + assert not github_error_is_unprocessable( + "gh: HTTP 403 Forbidden sha=154a33d092422abc issue #422" + ) + assert not github_error_is_unprocessable("status code 422 only") def test_iter_single_comment_payloads_keeps_only_safe_comments(): From e49bc316d6ccf490a219d6c2c887b923427972bd Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 13 Aug 2026 17:11:15 +0900 Subject: [PATCH 06/12] fix(review): escape one-at-a-time 422 receipt phrases Keep surviving hunks attachable after a batch 422, but escape backticks and HTML metacharacters in GitHub error phrases before they enter the overview body. --- CHANGELOG.md | 2 +- .../review-inline-comment-422-fallback.md | 4 +- .../ci/opencode_inline_comment_fallback.py | 38 ++++++++++++++----- .../test_opencode_inline_comment_fallback.py | 16 ++++++++ 4 files changed, 48 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce2ef0fa0..7f7f58f31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +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. Classification now requires an `HTTP 422` line, `Unprocessable Entity`, or a JSON error phrase — a `422` substring inside a SHA or issue number no longer starts that retry (CWE-1288). +- 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. Classification now requires an `HTTP 422` line, `Unprocessable Entity`, or a JSON error phrase — a `422` substring inside a SHA or issue number no longer starts that retry (CWE-1288). Receipt phrases now escape backticks and HTML metacharacters before they are written into the overview body. - 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 c2c4c09ab..03d6ea8e2 100644 --- a/docs/doctoring/review-inline-comment-422-fallback.md +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -31,7 +31,9 @@ write durable receipts into the OpenCode overview comment `` `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 phrase is 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. diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py index 616beb827..92b671e26 100644 --- a/scripts/ci/opencode_inline_comment_fallback.py +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -70,6 +70,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 "" @@ -99,14 +112,18 @@ 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] - 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: @@ -200,7 +217,8 @@ def render_inline_comment_receipts( if not locations: return [] if error_phrase: - return [f"- `{path}:{line}` — {error_phrase}" for path, line in locations] + safe_phrase = escape_receipt_text(error_phrase) + return [f"- `{path}:{line}` — {safe_phrase}" for path, line in locations] return [f"- `{path}:{line}`" for path, line in locations] @@ -243,7 +261,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)}") lines.append("") return "\n".join(lines) diff --git a/tests/test_opencode_inline_comment_fallback.py b/tests/test_opencode_inline_comment_fallback.py index 592bda311..0380f79de 100644 --- a/tests/test_opencode_inline_comment_fallback.py +++ b/tests/test_opencode_inline_comment_fallback.py @@ -100,6 +100,22 @@ def test_github_publication_error_phrase_falls_back_to_http_line(): == "GitHub HTTP 422: 422" ) assert render_inline_comment_receipts([], "GitHub HTTP 422") == [] + assert ( + github_publication_error_phrase( + '{"errors":[{"message":"path `