diff --git a/.github/workflows/opencode-code-review.yml b/.github/workflows/opencode-code-review.yml index be5b8225a2..a4c642bcc5 100644 --- a/.github/workflows/opencode-code-review.yml +++ b/.github/workflows/opencode-code-review.yml @@ -199,13 +199,19 @@ jobs: set -euo pipefail git show "$BASE_SHA:scripts/opencode-code-review" > .opencode-review/opencode-code-review - python3 .opencode-review/opencode-code-review post-findings \ - --output .opencode-review/output.txt \ - --files .opencode-review/files.txt \ - --opencode-status .opencode-review/opencode.status \ - --stdout .opencode-review/opencode.stdout \ - --stderr .opencode-review/opencode.stderr \ + post_args=( + post-findings + --output .opencode-review/output.txt + --files .opencode-review/files.txt + --opencode-status .opencode-review/opencode.status + --stdout .opencode-review/opencode.stdout + --stderr .opencode-review/opencode.stderr --summary "$GITHUB_STEP_SUMMARY" + ) + if python3 .opencode-review/opencode-code-review post-findings --help | grep -q -- "--patch"; then + post_args+=(--patch .opencode-review/review.patch) + fi + python3 .opencode-review/opencode-code-review "${post_args[@]}" - name: Upload OpenCode review artifacts if: always() @@ -432,13 +438,19 @@ jobs: set -euo pipefail git show "$BASE_SHA:scripts/opencode-code-review" > .opencode-review/opencode-code-review - python3 .opencode-review/opencode-code-review post-findings \ - --output .opencode-review/output.txt \ - --files .opencode-review/files.txt \ - --opencode-status .opencode-review/opencode.status \ - --stdout .opencode-review/opencode.stdout \ - --stderr .opencode-review/opencode.stderr \ + post_args=( + post-findings + --output .opencode-review/output.txt + --files .opencode-review/files.txt + --opencode-status .opencode-review/opencode.status + --stdout .opencode-review/opencode.stdout + --stderr .opencode-review/opencode.stderr --summary "$GITHUB_STEP_SUMMARY" + ) + if python3 .opencode-review/opencode-code-review post-findings --help | grep -q -- "--patch"; then + post_args+=(--patch .opencode-review/review.patch) + fi + python3 .opencode-review/opencode-code-review "${post_args[@]}" - name: Upload OpenCode review artifacts if: always() diff --git a/Makefile b/Makefile index 19727f80f3..9d3da9f1bd 100644 --- a/Makefile +++ b/Makefile @@ -387,6 +387,7 @@ test-scripts: @printf "$(CYAN)Running script tests...$(RESET)\n" @bash scripts/pr-state.test.sh @bash scripts/opencode-code-review.test.sh + @python3 scripts/opencode-code-review.test.py .PHONY: test-e2e test-e2e: build-backend build-web diff --git a/scripts/opencode-code-review b/scripts/opencode-code-review index 852def3fef..60fd59a4a0 100755 --- a/scripts/opencode-code-review +++ b/scripts/opencode-code-review @@ -200,7 +200,7 @@ def fallback_body(rejected: list[dict[str, Any]], overflow: list[dict[str, Any]] body += section included += 1 - append_section("GitHub rejected inline placement", rejected) + append_section("Inline placement unavailable", rejected) append_section("Additional findings beyond inline comment limit", overflow) if omitted: @@ -212,6 +212,40 @@ def fallback_body(rejected: list[dict[str, Any]], overflow: list[dict[str, Any]] return body, included, omitted +def parse_commentable_right_lines(patch: str) -> dict[str, set[int]]: + commentable: dict[str, set[int]] = {} + path: str | None = None + next_right_line: int | None = None + + for line in patch.splitlines(): + if line.startswith("diff --git "): + path = None + next_right_line = None + continue + if line.startswith("+++ b/"): + path = line[len("+++ b/") :] + commentable.setdefault(path, set()) + continue + if line.startswith("@@ "): + match = re.search(r"\+(\d+)(?:,(\d+))?", line) + next_right_line = int(match.group(1)) if match else None + continue + if path is None or next_right_line is None: + continue + if line.startswith("+") and not line.startswith("+++ "): + commentable.setdefault(path, set()).add(next_right_line) + next_right_line += 1 + continue + if line.startswith("-"): + continue + if line.startswith("\\"): + continue + commentable.setdefault(path, set()).add(next_right_line) + next_right_line += 1 + + return commentable + + def normalize_finding(value: Any, changed_paths: set[str]) -> dict[str, Any] | None: if not isinstance(value, dict): return None @@ -341,6 +375,14 @@ def post_findings(args: argparse.Namespace) -> int: return 0 if posted_no_findings else 1 changed_paths = set(args.files.read_text(encoding="utf-8").splitlines()) + commentable_lines = None + patch_filter_warning = "" + if args.patch is not None: + try: + commentable_lines = parse_commentable_right_lines(args.patch.read_text(encoding="utf-8")) + except OSError as exc: + patch_filter_warning = f"Could not read OpenCode review patch for inline filtering: {exc}" + print(patch_filter_warning, file=sys.stderr) posted = 0 rejected = [] valid_findings = [ @@ -348,8 +390,20 @@ def post_findings(args: argparse.Namespace) -> int: for finding in (normalize_finding(item, changed_paths) for item in findings) if finding is not None ] - inline_findings = valid_findings[:MAX_INLINE_FINDINGS] - overflow_findings = valid_findings[MAX_INLINE_FINDINGS:] + prefiltered = [] + inline_candidates = [] + for finding in valid_findings: + if commentable_lines is not None and finding["line"] not in commentable_lines.get(finding["path"], set()): + print( + f"Skipped OpenCode inline finding outside commentable diff lines: {finding['path']}:{finding['line']}", + file=sys.stderr, + ) + prefiltered.append(finding) + continue + inline_candidates.append(finding) + + inline_findings = inline_candidates[:MAX_INLINE_FINDINGS] + overflow_findings = inline_candidates[MAX_INLINE_FINDINGS:] for finding in inline_findings: ok, error_message = post_inline_finding(finding) @@ -368,25 +422,24 @@ def post_findings(args: argparse.Namespace) -> int: posted_fallback = True fallback_included = 0 fallback_omitted = 0 - if rejected or overflow_findings: - fallback_comment, fallback_included, fallback_omitted = fallback_body(rejected, overflow_findings) + unplaced_findings = prefiltered + rejected + if unplaced_findings or overflow_findings: + fallback_comment, fallback_included, fallback_omitted = fallback_body(unplaced_findings, overflow_findings) posted_fallback = upsert_issue_comment(FALLBACK_MARKER, fallback_comment) - append_summary( - args.summary, - "OpenCode review", - [ - f"Parsed findings: `{len(findings)}`", - f"Inline comments posted: `{posted}`", - f"Inline comments rejected: `{len(rejected)}`", - f"Findings beyond inline limit: `{len(overflow_findings)}`", - f"Fallback findings included in comment: `{fallback_included}`", - f"Fallback findings omitted from comment: `{fallback_omitted}`", - "", - *snippet("OpenCode stdout", stdout), - *snippet("OpenCode stderr", stderr), - ], - ) + summary_lines = [ + f"Parsed findings: `{len(findings)}`", + f"Inline comments posted: `{posted}`", + f"Inline comments skipped before posting: `{len(prefiltered)}`", + f"Inline comments rejected: `{len(rejected)}`", + f"Findings beyond inline limit: `{len(overflow_findings)}`", + f"Fallback findings included in comment: `{fallback_included}`", + f"Fallback findings omitted from comment: `{fallback_omitted}`", + ] + if patch_filter_warning: + summary_lines.append(f"Patch filtering warning: {patch_filter_warning}") + summary_lines.extend(["", *snippet("OpenCode stdout", stdout), *snippet("OpenCode stderr", stderr)]) + append_summary(args.summary, "OpenCode review", summary_lines) print(f"Posted {posted} OpenCode inline comments; preserved {fallback_included} fallback findings.") return 0 if posted_fallback else 1 @@ -402,6 +455,7 @@ def build_parser() -> argparse.ArgumentParser: post.add_argument("--stdout", type=Path, required=True) post.add_argument("--stderr", type=Path, required=True) post.add_argument("--summary", type=Path, required=True) + post.add_argument("--patch", type=Path) post.set_defaults(func=post_findings) return parser diff --git a/scripts/opencode-code-review.test.py b/scripts/opencode-code-review.test.py index 637c2ea8d1..7276dfd80f 100755 --- a/scripts/opencode-code-review.test.py +++ b/scripts/opencode-code-review.test.py @@ -26,11 +26,13 @@ class OpenCodeReviewScriptTest(unittest.TestCase): self.stdout_path = self.workdir / "stdout.txt" self.stderr_path = self.workdir / "stderr.txt" self.summary_path = self.workdir / "summary.md" + self.patch_path = self.workdir / "review.patch" self.files_path.write_text("src/app.ts\n", encoding="utf-8") self.status_path.write_text("0\n", encoding="utf-8") self.stdout_path.write_text("stdout line\n", encoding="utf-8") self.stderr_path.write_text("stderr line\n", encoding="utf-8") self.summary_path.write_text("", encoding="utf-8") + self.patch_path.write_text("", encoding="utf-8") self.fake_gh = self.write_fake_gh() def write_fake_gh(self) -> Path: @@ -79,6 +81,7 @@ class OpenCodeReviewScriptTest(unittest.TestCase): inline_fail: bool = False, issue_comment_fail: bool = False, gh_bin: Path | None = None, + include_patch: bool = False, ) -> subprocess.CompletedProcess[str]: self.output_path.write_text(output, encoding="utf-8") env = { @@ -94,24 +97,27 @@ class OpenCodeReviewScriptTest(unittest.TestCase): env["INLINE_FAIL"] = "1" if issue_comment_fail: env["ISSUE_COMMENT_FAIL"] = "1" + command = [ + sys.executable, + str(SCRIPT), + "post-findings", + "--output", + str(self.output_path), + "--files", + str(self.files_path), + "--opencode-status", + str(self.status_path), + "--stdout", + str(self.stdout_path), + "--stderr", + str(self.stderr_path), + "--summary", + str(self.summary_path), + ] + if include_patch: + command.extend(["--patch", str(self.patch_path)]) return subprocess.run( - [ - sys.executable, - str(SCRIPT), - "post-findings", - "--output", - str(self.output_path), - "--files", - str(self.files_path), - "--opencode-status", - str(self.status_path), - "--stdout", - str(self.stdout_path), - "--stderr", - str(self.stderr_path), - "--summary", - str(self.summary_path), - ], + command, text=True, capture_output=True, env=env, @@ -190,6 +196,113 @@ class OpenCodeReviewScriptTest(unittest.TestCase): self.assertTrue(any("" in body for body in bodies)) self.assertTrue(any("src/app.ts:99" in body for body in bodies)) + def test_context_lines_are_commentable_when_patch_is_available(self) -> None: + self.patch_path.write_text( + textwrap.dedent( + """\ + diff --git a/src/app.ts b/src/app.ts + index 1111111..2222222 100644 + --- a/src/app.ts + +++ b/src/app.ts + @@ -8,5 +8,6 @@ + const before = 1; + const existing = 2; + +const added = 3; + const after = 4; + """ + ), + encoding="utf-8", + ) + findings = [ + {"path": "src/app.ts", "line": 9, "title": "Context line", "body": "This is unchanged context."}, + {"path": "src/app.ts", "line": 10, "title": "Added line", "body": "This is added."}, + ] + + result = self.run_script( + output=f"{json.dumps(findings)}\n", + include_patch=True, + ) + + self.assertEqual(result.returncode, 0, result.stderr) + pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg for arg in call)] + self.assertEqual(len(pull_comment_calls), 2, pull_comment_calls) + self.assertTrue(any("line=9" in arg for call in pull_comment_calls for arg in call)) + self.assertTrue(any("line=10" in arg for call in pull_comment_calls for arg in call)) + self.assertFalse(any("" in body for body in self.bodies())) + self.assertIn("Inline comments skipped before posting: `0`", self.summary_path.read_text()) + + def test_lines_outside_patch_hunks_are_prefiltered_to_fallback(self) -> None: + self.patch_path.write_text( + textwrap.dedent( + """\ + diff --git a/src/app.ts b/src/app.ts + index 1111111..2222222 100644 + --- a/src/app.ts + +++ b/src/app.ts + @@ -8,5 +8,6 @@ + const before = 1; + const existing = 2; + +const added = 3; + const after = 4; + """ + ), + encoding="utf-8", + ) + findings = [ + {"path": "src/app.ts", "line": 99, "title": "Outside hunk", "body": "This is not in the diff."}, + {"path": "src/app.ts", "line": 10, "title": "Added line", "body": "This is added."}, + ] + + result = self.run_script( + output=f"{json.dumps(findings)}\n", + include_patch=True, + ) + + self.assertEqual(result.returncode, 0, result.stderr) + pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg for arg in call)] + self.assertEqual(len(pull_comment_calls), 1, pull_comment_calls) + self.assertTrue(any("line=10" in arg for arg in pull_comment_calls[0])) + self.assertFalse(any("line=99" in arg for call in self.read_calls() for arg in call)) + bodies = self.bodies() + self.assertTrue(any("" in body for body in bodies)) + self.assertTrue(any("src/app.ts:99" in body for body in bodies)) + self.assertFalse(any("src/app.ts:10" in body for body in bodies)) + self.assertIn("Inline comments skipped before posting: `1`", self.summary_path.read_text()) + + def test_deleted_lines_starting_with_dashes_do_not_shift_right_lines(self) -> None: + self.patch_path.write_text( + textwrap.dedent( + """\ + diff --git a/src/app.ts b/src/app.ts + index 1111111..2222222 100644 + --- a/src/app.ts + +++ b/src/app.ts + @@ -8,4 +8,3 @@ + const before = 1; + --- removed separator + const after = 2; + """ + ), + encoding="utf-8", + ) + findings = [ + {"path": "src/app.ts", "line": 9, "title": "After context", "body": "Still commentable."}, + {"path": "src/app.ts", "line": 10, "title": "Shifted line", "body": "Should be outside hunk."}, + ] + + result = self.run_script( + output=f"{json.dumps(findings)}\n", + include_patch=True, + ) + + self.assertEqual(result.returncode, 0, result.stderr) + pull_comment_calls = [call for call in self.read_calls() if any("/pulls/" in arg for arg in call)] + self.assertEqual(len(pull_comment_calls), 1, pull_comment_calls) + self.assertTrue(any("line=9" in arg for arg in pull_comment_calls[0])) + self.assertFalse(any("line=10" in arg for call in self.read_calls() for arg in call)) + fallback = next(body for body in self.bodies() if "" in body) + self.assertIn("src/app.ts:10", fallback) + def test_integral_float_line_values_are_normalized(self) -> None: result = self.run_script( output=textwrap.dedent( @@ -234,7 +347,7 @@ class OpenCodeReviewScriptTest(unittest.TestCase): self.assertTrue(any("" in body for body in bodies)) self.assertTrue(any("src/app.ts:1" in body for body in bodies)) self.assertTrue(any("src/app.ts:30" in body for body in bodies)) - self.assertTrue(any("## GitHub rejected inline placement" in body for body in bodies)) + self.assertTrue(any("## Inline placement unavailable" in body for body in bodies)) self.assertTrue(any("## Additional findings beyond inline comment limit" in body for body in bodies)) summary = self.summary_path.read_text() self.assertIn("Inline comments rejected: `20`", summary) diff --git a/scripts/opencode-code-review.test.sh b/scripts/opencode-code-review.test.sh index d60db34e55..99858e04cc 100644 --- a/scripts/opencode-code-review.test.sh +++ b/scripts/opencode-code-review.test.sh @@ -82,6 +82,18 @@ if [[ "$explicit_model_env_count" != "2" ]]; then fi pass "OpenCode posting steps explicitly pass OPENCODE_MODEL" +patch_capability_count="$(count_occurrences 'post-findings --help | grep -q -- "--patch"')" +if [[ "$patch_capability_count" != "2" ]]; then + fail "OpenCode posting steps check trusted parser support before passing review.patch" +fi +pass "OpenCode posting steps check trusted parser support before passing review.patch" + +explicit_patch_arg_count="$(count_occurrences 'post_args+=(--patch .opencode-review/review.patch)')" +if [[ "$explicit_patch_arg_count" != "2" ]]; then + fail "OpenCode posting steps explicitly pass review.patch for inline filtering" +fi +pass "OpenCode posting steps explicitly pass review.patch for inline filtering" + trusted_script_count="$(count_occurrences 'git show "$BASE_SHA:scripts/opencode-code-review" > .opencode-review/opencode-code-review')" if [[ "$trusted_script_count" != "2" ]]; then fail "OpenCode review executes the parser script from the trusted base commit in both workflow paths"