diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 83f6830d5..59017c88f 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -5623,8 +5623,56 @@ 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:-}" + local control_json="${6:-}" local gh_error_file local rewritten_payload_file local review_response_file @@ -5640,6 +5688,19 @@ 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 + 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" @@ -5766,12 +5827,20 @@ jobs: build_inline_comment_failure_body() { local body_file="$1" local output_file="$2" - - { - cat "$body_file" - printf '\n## Inline comment publishing failed\n\n' - printf 'GitHub did not accept the inline review comments for the cited finding lines, so OpenCode did not copy suggested diffs into this PR-level body. Re-run the review after the findings are anchored to changed diff lines, or inspect the workflow log/control JSON and apply the changes manually.\n' - } >"$output_file" + local control_json="$3" + local error_file="${4:-}" + local -a fallback_args + + 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() { @@ -5785,8 +5854,8 @@ jobs: fallback_body_file="$(mktemp)" format_request_changes_body "$control_json" "$body_file" build_request_changes_review_payload "$control_json" "$body_file" "$payload_file" - build_inline_comment_failure_body "$body_file" "$fallback_body_file" - create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" + build_inline_comment_failure_body "$body_file" "$fallback_body_file" "$control_json" + create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" "$body_file" "$control_json" rm -f "$body_file" "$payload_file" "$fallback_body_file" } diff --git a/AGENTS.md b/AGENTS.md index 688b33035..dda304ab1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,3 +2,10 @@ > **Agents: read the master context FIRST.** Before any work, read [`docs/CWL-MASTER-CONTEXT.md`](docs/CWL-MASTER-CONTEXT.md) (mission · naruon-as-platform + inter-component UML · cross-cutting disciplines · conventions · roadmap · current state), the live **GitHub Project #1** (work/roadmap source of truth), the full spec **ContextualWisdomLab/naruon#974**, and operate the Project per [`docs/agent-github-project-protocol.md`](docs/agent-github-project-protocol.md). The repo/Project — not any private agent memory — is the source of truth. +Materialize accepts only exact SHA-256 pins or a bounded relative `-r` include (no `.`/`..`); a lone `--require-hashes` directive is not trust evidence. See [`docs/doctoring/review-inline-comment-422-fallback.md`](docs/doctoring/review-inline-comment-422-fallback.md). + +One-at-a-time 422 retries must keep multi-line start_line/start_side. +One-at-a-time 422 retries are capped at 20 comments; leftovers become deferred path:line rows. +One-at-a-time 422 retries strip leftover ```diff/```patch fences so unapplyable leftover diffs cannot 422 the retry. +Leftover overview receipts sanitize path and phrase so a leftover cannot close the HTML comment or reopen a suggestion fence. +Leftover overview paths that contain `-->`, ` 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 bf30091dd..48281577a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,15 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Omitted leftover 422-fallback paths that contain `-->`, `` or reopen an applyable GitHub suggestion block (CWE-116). +- Sanitized leftover overview receipt path and phrase so a leftover cannot close `` or reopen a GitHub suggestion fence (CWE-116). +- Materialized base Python locks only when every package line is an exact SHA-256 pin or a bounded relative `-r`/`--requirement` include. A lone `--require-hashes` directive, a dotted include such as `./lock.txt`, or `-r other-hashes.txt` no longer enters the trusted build context. +- Stripped leftover unapplyable `` ```diff `` / `` ```patch `` fences from one-at-a-time OpenCode inline retries so leftover manual diffs cannot 422 the retry, while applyable `` ```suggestion `` fences stay on the surviving hunk. +- Capped one-at-a-time OpenCode inline retries at 20 comments and recorded leftover `path:line` rows past that cap so a batch 422 cannot open unbounded `gh api` writes. +- Kept `start_line`/`start_side` on one-at-a-time 422 retries so a multi-line GitHub suggestion still posts as one range instead of a single comment on the last line. +- 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. - 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/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 new file mode 100644 index 000000000..7f5d60bdb --- /dev/null +++ b/docs/doctoring/review-inline-comment-422-fallback.md @@ -0,0 +1,91 @@ +# GitHub 422 inline-comment fallback cites trusted path:line + +검토 기준일: **2026-08-13** + +## Incident + +When GitHub rejects an OpenCode `REQUEST_CHANGES` review because one or more +inline comments cannot attach, the publisher already falls back to a PR-level +body and does not copy suggested diffs into that body. The fallback sentence +said only “the cited finding lines.” Authors then had to open the workflow log +or control JSON to learn *which* `path:line` GitHub refused (GitHub, n.d.-a, +n.d.-b). That is weaker than the line-anchored review artifact modern code +review expects (Bacchelli & Bird, 2013). + +## Decision + +Leftover overview paths that contain `-->`, ``). 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. 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. +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, + 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, leftover `` ```diff `` fence stripping, 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`. + +## 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) + +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. +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/materialize_base_python_requirements.py b/scripts/ci/materialize_base_python_requirements.py index 98cdad459..7a9c204b8 100755 --- a/scripts/ci/materialize_base_python_requirements.py +++ b/scripts/ci/materialize_base_python_requirements.py @@ -87,6 +87,58 @@ def _is_candidate_lock_name(name: str) -> bool: ) + +def _is_candidate_lock_path(path: pathlib.PurePosixPath) -> bool: + """Return whether one safe tracked path can name a pip requirements lock. + + In addition to conventional ``requirements*.txt`` names, repositories often + keep concrete environment closures as direct children such as + ``requirements/ci.txt`` or ``service/requirements/package.txt``. Only direct + ``.txt`` children of a directory named ``requirements`` gain this path-based + eligibility; content must still pass the independent complete hash-pin + validation before it reaches the trusted image build context. + """ + return _is_candidate_lock_name(path.name) or ( + path.suffix == ".txt" and path.parent.name == "requirements" + ) + + +def _is_bounded_requirement_include(line: str) -> bool: + """Return whether one requirements include names a bounded relative file. + + Includes are accepted only as a two-token ``-r``/``--requirement`` form + whose target is itself a candidate lock path written as a normalized + relative POSIX path. Absolute paths, ``.`` or ``..`` components, double + slashes, URLs, option-like targets, shell/Windows path separators, + fragments, queries, extra inline options or hashes, and includes of + non-lock files are rejected before a base-owned file can enter the + trusted build context. + The downstream installer still proves that the candidate is an independently + complete hash closure; this predicate grants syntax eligibility only. + """ + fields = line.split() + if len(fields) != 2 or fields[0] not in {"-r", "--requirement"}: + return False + target = fields[1] + if ( + target.startswith(("-", "~")) + or "\\" in target + or ":" in target + or "?" in target + or "#" in target + ): + return False + include_path = pathlib.PurePosixPath(target) + return ( + bool(include_path.parts) + and target == include_path.as_posix() + and not include_path.is_absolute() + and "." not in include_path.parts + and ".." not in include_path.parts + and _is_candidate_lock_path(include_path) + ) + + def _requirement_lines(content: bytes) -> list[str]: """Return logical requirement lines, joining backslash line-continuations. @@ -107,26 +159,27 @@ def _requirement_lines(content: bytes) -> list[str]: def _is_hash_pinned(content: bytes) -> bool: - """Return whether content carries hash pins and is safe to preflight. - - Discovery is content-based rather than name-based so hash-pinned locks in any - location (a service subdirectory, ``requirements-dev.txt``, - ``requirements-test.txt``) can be considered for offline coverage, while an - unpinned or PR-mutable requirements file is still excluded from the networked - build context. Hash syntax cannot prove that a file includes every transitive - dependency, so the trusted image installer separately preflights every - candidate as an independent ``--require-hashes`` closure. An empty file - carries no installable dependency and is not materialized. + """Return whether content carries only trusted pins or bounded includes. + + Discovery is content-based rather than name-based so exact hash-pinned locks + in service subdirectories and role-specific requirements files can be + considered for offline coverage. Candidate syntax is deliberately stricter + than a substring search: each package line must be an exact ``==`` pin with + one or more complete SHA-256 hashes, or a bounded relative requirements + include. A global ``--require-hashes`` directive is not trust evidence by + itself. The downstream installer separately preflights every candidate as an + independent ``pip --require-hashes`` closure, so syntax eligibility never + substitutes for dependency-closure proof. """ lines = _requirement_lines(content) - if not lines: + requirement_lines = [line for line in lines if line != "--require-hashes"] + if not requirement_lines: return False - return any(line == "--require-hashes" for line in lines) or all( - "--hash=" in line or line.startswith(("-r ", "--requirement ")) - for line in lines + return all( + _is_fully_hash_pinned_requirement(line) + or _is_bounded_requirement_include(line) + for line in requirement_lines ) - - def _is_fully_hash_pinned_requirement(line: str) -> bool: """Return whether one uv-export line is an exact package pin with SHA-256 hashes.""" fields = re.split(r"\s+(?=--hash=)", line) diff --git a/scripts/ci/opencode_inline_comment_fallback.py b/scripts/ci/opencode_inline_comment_fallback.py new file mode 100644 index 000000000..6cc7c5aca --- /dev/null +++ b/scripts/ci/opencode_inline_comment_fallback.py @@ -0,0 +1,433 @@ +#!/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 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.*)$") +LEFTOVER_DIFF_FENCE_RE = re.compile( + r"```(?:diff|patch)\b[\s\S]*?```", + re.IGNORECASE, +) + + +def safe_finding_path(raw_path: object) -> str | None: + """Return a repository-relative finding path, or None when it is unsafe. + + Rejects traversal, absolute and drive paths, backslashes, and characters + that would break Markdown receipt fences (backtick, ``<``, ``>``, ``&``) + or close the overview HTML comment / reopen a suggestion fence + (``-->``, ``", "``. + A leftover path or reason with ``-->`` or an HTML metacharacter would + close that comment or inject markup (CWE-116). Fence markers are also + removed so a leftover cannot reopen a GitHub suggestion block. + """ + excerpt = (text or "").replace("\r\n", "\n").replace("\t", " ") + excerpt = ( + excerpt.replace("```", "") + .replace("", "") + .replace("<", "") + .replace(">", "") + .replace("&", "") + ) + return excerpt.strip("\n") + + +def render_inline_comment_receipts( + locations: list[tuple[str, int]], error_phrase: str +) -> list[str]: + """Return durable overview receipt lines for refused inline comments.""" + if not locations: + return [] + if error_phrase: + safe_phrase = escape_receipt_text(error_phrase) + return [ + f"- `{sanitize_leftover_excerpt(path)}:{line}` — {safe_phrase}" + for path, line in locations + ] + return [f"- `{sanitize_leftover_excerpt(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 = [ + "", + heading, + "", + ] + if locations: + 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("") + 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." + ) + if error_phrase: + lines.append("") + lines.append(f"- GitHub error: {escape_receipt_text(error_phrase)}") + lines.append("") + return "\n".join(lines) + + +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), + error_phrase=error_phrase, + ) + + +def load_control(path: Path) -> dict[str, Any]: + """Load one trusted review-control JSON object.""" + try: + value = json.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + raise ValueError(f"control JSON could not be read: {exc}") from exc + if not isinstance(value, dict): + raise ValueError("control JSON must be an object") + return value + + +def main(argv: list[str] | None = None) -> int: + """Write 422 fallback text or split a batch review into single comments.""" + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--control", type=Path) + parser.add_argument("--body", type=Path) + parser.add_argument("--output", type=Path) + parser.add_argument("--error-file", type=Path) + parser.add_argument("--split-payload", type=Path) + parser.add_argument("--output-dir", type=Path) + parser.add_argument("--retry-limit", type=int) + parser.add_argument("--deferred-locations", 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, + limit=args.retry_limit, + deferred_path=args.deferred_locations, + ) + return 0 + if args.control is None or args.body is None or args.output is None: + raise ValueError("--control, --body, and --output are required") + control = load_control(args.control) + body = args.body.read_text(encoding="utf-8") + error_text = ( + args.error_file.read_text(encoding="utf-8") if args.error_file else "" + ) + 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), + 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..801e16584 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -1474,7 +1474,12 @@ 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" + 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_materialize_base_python_requirements.py b/tests/test_materialize_base_python_requirements.py index 8a383f0c2..b56955a21 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" @@ -150,9 +163,24 @@ def test_lock_name_candidates_are_pip_requirements_files() -> None: def test_hash_pin_detection_includes_pinned_and_excludes_unpinned_or_empty() -> None: """Only fully hash-pinned, non-empty lock content is materialized.""" assert not materializer._is_hash_pinned(b"# comment only\n\n") - assert materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") + assert not materializer._is_hash_pinned(b"--require-hashes\ndemo==1\n") assert materializer._is_hash_pinned(b"demo==1 --hash=sha256:" + b"a" * 64 + b"\n") - assert materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert materializer._is_hash_pinned(b"-r requirements-other.txt\n") + assert not materializer._is_hash_pinned(b"-r other-hashes.txt\n") + assert not materializer._is_hash_pinned(b"-r ./requirements-other.txt\n") + assert not materializer._is_hash_pinned(b"-r ../escape.txt\n") + assert materializer._is_bounded_requirement_include( + "--requirement requirements-other.txt" + ) + assert not materializer._is_bounded_requirement_include("-r .") + assert not materializer._is_bounded_requirement_include("-r -evil.txt") + assert not materializer._is_bounded_requirement_include("-r ~evil.txt") + assert not materializer._is_bounded_requirement_include("-r C:foo.txt") + assert not materializer._is_bounded_requirement_include("-r foo?bar.txt") + assert not materializer._is_bounded_requirement_include("-r foo#bar.txt") + assert not materializer._is_bounded_requirement_include(r"-r foo\\bar.txt") + assert not materializer._is_bounded_requirement_include("-r") + assert not materializer._is_bounded_requirement_include("-r /abs/requirements.txt") assert not materializer._is_hash_pinned(b"untrusted==1\n") # uv export / pip-compile multi-line continuation format (spec, then --hash= lines). assert materializer._is_hash_pinned( @@ -644,6 +672,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 +719,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 +751,7 @@ def test_install_trusted_uv_rejects_wrong_version_or_exit_status( completed: subprocess.CompletedProcess[bytes], ) -> None: """Unexpected version output or a nonzero status cannot satisfy the pin.""" + _simulate_linux_x86_64_runner(monkeypatch) tool_dir = tmp_path / f"uv-{completed.returncode}-{len(completed.stdout)}" monkeypatch.setattr( materializer.tempfile, diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index daeaa37a2..0d7a6881a 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1609,6 +1609,19 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): 'post_pull_review_with_retry "inline review" "$review_write_token"' in publish_step ) + assert "opencode_inline_comment_fallback.py" in workflow + assert ( + 'build_inline_comment_failure_body "$body_file" "$fallback_body_file" "$control_json"' + in workflow + ) + assert ( + 'create_pull_review_with_payload "REQUEST_CHANGES" "$(cat "$body_file")" "$payload_file" "$fallback_body_file" "$body_file" "$control_json"' + in workflow + ) + assert 'fallback_args+=(--error-file "$error_file")' in workflow + assert "retry_inline_comments_one_at_a_time" in workflow + assert "--is-unprocessable" in workflow + assert "inline review one-at-a-time" in workflow assert "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..ba082a002 --- /dev/null +++ b/tests/test_opencode_inline_comment_fallback.py @@ -0,0 +1,549 @@ +import json +import runpy +import sys + +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, + render_inline_comment_failure_body, + render_inline_comment_receipts, + sanitize_leftover_excerpt, + render_single_comment_review, + single_comment_range_fields, + single_comment_retry_limit, + strip_leftover_diff_fences, + trusted_finding_locations, + write_single_comment_payloads, +) + + +def control(*findings: dict[str, object]) -> dict[str, object]: + """Return a REQUEST_CHANGES control object for fallback tests.""" + return { + "result": "REQUEST_CHANGES", + "findings": list(findings), + } + + +def test_trusted_finding_locations_keeps_first_safe_path_line_pairs(): + locations = trusted_finding_locations( + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": 12, "line": 1}, + {"path": "../escape.py", "line": 1}, + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "/abs.py", "line": 3}, + {"path": "scripts/ci/other.py", "line": 0}, + {"path": "scripts/ci/other.py", "line": True}, + {"path": "scripts/ci/other.py", "line": 12}, + {"path": "scripts/ci/close-->comment.py", "line": 8}, + {"path": "scripts/ci/fence```suggestion.py", "line": 9}, + "not-an-object", + ) + ) + + assert locations == [ + ("scripts/ci/example.py", 7), + ("scripts/ci/other.py", 12), + ] + assert trusted_finding_locations({"findings": None}) == [] + assert trusted_finding_locations({}) == [] + + +def test_fallback_body_cites_each_trusted_path_line(): + body = render_inline_comment_failure_body( + "## Findings\n\nexisting body\n", + control( + {"path": "scripts/ci/example.py", "line": 7}, + {"path": "README.md", "line": 3}, + ), + ) + + assert body.startswith("## Findings\n\nexisting body") + assert "GitHub did not accept the inline review comments" in body + assert "- `scripts/ci/example.py:7`" in body + assert "- `README.md:3`" in body + assert "did not copy suggested diffs into this PR-level body" in body + + +def test_github_publication_error_phrase_prefers_json_error_messages(): + phrase = github_publication_error_phrase( + "gh: HTTP 422: Unprocessable Entity " + "(https://api.github.com/repos/org/repo/pulls/1/reviews)\n" + '{"message":"Validation Failed","errors":[' + '{"resource":"PullRequestReview","field":"comments","code":"custom",' + '"message":"pull_request_review_thread.path is invalid"},' + '{"message":"Review comments is invalid"}' + "]}\n" + ) + + assert phrase.startswith("GitHub HTTP 422:") + assert "pull_request_review_thread.path is invalid" in phrase + assert "Review comments is invalid" in phrase + assert "https://api.github.com" not in phrase + + +def test_github_publication_error_phrase_falls_back_to_http_line(): + assert ( + github_publication_error_phrase( + "post failed\ngh: Validation Failed (HTTP 422)\n" + ) + == "GitHub HTTP 422: Validation Failed (HTTP 422)" + ) + assert ( + github_publication_error_phrase("GitHub HTTP 422: already normalized\n") + == "GitHub HTTP 422: already normalized" + ) + assert ( + github_publication_error_phrase("status code 422 only") + == "GitHub review write failed" + ) + assert ( + github_publication_error_phrase("https://api.github.example/HTTP 422") + == "GitHub HTTP 422: 422" + ) + assert render_inline_comment_receipts([], "GitHub HTTP 422") == [] + assert sanitize_leftover_excerpt("a & b ") == "a b c" + assert "-->" not in sanitize_leftover_excerpt("close --> comment") + assert "```" not in sanitize_leftover_excerpt("```suggestion\nsecret") + assert ( + github_publication_error_phrase( + '{"errors":[{"message":"path `