From ecb204e9f7a6f227664ca74036be650cbe19e1dc Mon Sep 17 00:00:00 2001 From: Ryan Melton Date: Sun, 11 Oct 2026 16:27:48 -0600 Subject: [PATCH 1/2] Give the AI reviewers the failing step of each failed CI job The reviewers have never received a failed job's log: every download in the gate failed in Actions with its error sent to /dev/null, so they saw only "(log unavailable)". Even a successful download kept the last 150 lines of the job, which are harden-runner's post-job cleanup rather than the failure. Keep the lines leading up to the last error before the post steps, cap long lines, and report a failed download as a warning with its error so the cause shows in the gate log. Co-Authored-By: Claude Opus 5.5 (1M context) --- ai-review/ai_review_gate.sh | 27 +++++++++++++++++++++++---- tests/test_ai_review.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/ai-review/ai_review_gate.sh b/ai-review/ai_review_gate.sh index becc1f8..a9df131 100644 --- a/ai-review/ai_review_gate.sh +++ b/ai-review/ai_review_gate.sh @@ -157,6 +157,22 @@ case "$scan_state" in esac # Collect failed job logs, with a workflow-level fallback for failures before jobs start. +log_file="$(mktemp)" +log_error="$(mktemp)" +trap 'rm -f "$log_file" "$log_error"' EXIT +# A job log ends with its post steps (harden-runner's cleanup alone runs to well over 100 lines), +# so keep the last LOG_LINES before the last error ahead of them: the end of the step that failed. +# Strip the timestamp prefix and ANSI colors, and cut very long lines, to save tokens. +failed_step_log() { + sed -E 's/^\xef\xbb\xbf//; s/^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9:.]+Z //; s/\x1b\[[0-9;]*m//g' \ + | awk -v n="$LOG_LINES" ' + /^Post job cleanup\.$/ { exit } + { line[NR] = $0; if (/^##\[error\]/) last = NR } + END { + stop = last ? last : NR + for (i = (stop > n ? stop - n + 1 : 1); i <= stop; i++) print substr(line[i], 1, 1000) + }' +} failures=0 while IFS=$'\t' read -r run_id run_name run_conclusion run_url; do [[ -n "$run_id" ]] || continue @@ -173,10 +189,13 @@ while IFS=$'\t' read -r run_id run_name run_conclusion run_url; do echo "$job_url" echo echo '```' - # Strip the timestamp prefix and ANSI colors to save tokens - gh api "repos/$repo/actions/jobs/$job_id/logs" 2> /dev/null \ - | sed -E 's/^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9:.]+Z //; s/\x1b\[[0-9;]*m//g' \ - | tail -n "$LOG_LINES" || echo "(log unavailable)" + if gh api "repos/$repo/actions/jobs/$job_id/logs" > "$log_file" 2> "$log_error"; then + failed_step_log < "$log_file" + else + error="$(tr -s '\n' ' ' < "$log_error" | sed 's/ *$//' | cut -c1-300)" + echo "::warning::Could not download the log of $run_name / $job_name: ${error:-unknown error}" >&2 + echo "(log unavailable: ${error:-unknown error})" + fi echo '```' echo } >> "$CI_FILE" diff --git a/tests/test_ai_review.py b/tests/test_ai_review.py index c60edd0..ef2fa71 100644 --- a/tests/test_ai_review.py +++ b/tests/test_ai_review.py @@ -99,6 +99,7 @@ def workflow_script(name, workflow=WORKFLOW): value = sequence.pop(0) if len(sequence) > 1 else sequence[0] fixture_path.write_text(json.dumps(fixtures)) if isinstance(value, dict) and value.get('test_api_error'): + print('test API error', file=sys.stderr) sys.exit(1) if isinstance(value, dict) and 'test_zip' in value: buffer = io.BytesIO() @@ -1011,6 +1012,35 @@ def test_failed_job_logs_are_not_double_counted(self): self.assertIn("An actual lint failure", report) self.assertNotIn("workflow failure", report) + def test_failed_job_log_keeps_the_failed_step_not_the_post_steps(self): + self.fixtures["repos/owner/repo/actions/runs/123/jobs"] = { + "jobs": [{"id": 10, "name": "unit", "conclusion": "failure", "html_url": "https://example.invalid/job/10"}] + } + cleanup = "\n".join(f"2026-01-01T00:00:01.0000000Z cleanup noise {i}" for i in range(300)) + self.fixtures["repos/owner/repo/actions/jobs/10/logs"] = ( + "\ufeff2026-01-01T00:00:00.0000000Z \x1b[31mFailures:\x1b[0m\n" + "2026-01-01T00:00:00.0000000Z 1) expected 1, got 2\n" + "2026-01-01T00:00:00.0000000Z ##[error]Process completed with exit code 1.\n" + "2026-01-01T00:00:00.0000000Z Post job cleanup.\n" + cleanup + ) + result = self.run_shell(f'bash "{GATE}"') + self.assertEqual(result.returncode, 0, result.stderr) + report = (self.directory / "out/ci_failures.md").read_text() + self.assertIn("\nFailures:\n 1) expected 1, got 2\n##[error]Process completed", report) + self.assertNotIn("cleanup noise", report) + self.assertNotIn("2026-01-01T", report) + + def test_failed_job_log_download_error_is_reported(self): + self.fixtures["repos/owner/repo/actions/runs/123/jobs"] = { + "jobs": [{"id": 10, "name": "unit", "conclusion": "failure", "html_url": "https://example.invalid/job/10"}] + } + self.fixtures["repos/owner/repo/actions/jobs/10/logs"] = {"test_api_error": True} + result = self.run_shell(f'bash "{GATE}"') + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.outputs()["ci_failures"], "1") + self.assertIn("::warning::Could not download the log of", result.stderr) + self.assertIn("(log unavailable: test API error)", (self.directory / "out/ci_failures.md").read_text()) + def test_pending_builtin_run_does_not_block_review(self): runs = self.fixtures["repos/owner/repo/actions/runs?head_sha=test-head&per_page=100"]["workflow_runs"] runs.append({"id": 124, "name": "CodeQL", "event": "dynamic", "status": "in_progress", "conclusion": None}) From 16e06a4c0723bed6bfaf9f8adf60d381a32d15bb Mon Sep 17 00:00:00 2001 From: Ryan Melton Date: Sun, 11 Oct 2026 16:32:04 -0600 Subject: [PATCH 2/2] Leave Trivy scan failures out of the AI review A Trivy failure means a base image or dependency has a known vulnerability, which a fix to the PR cannot address, so the reviewers spent turns on it and it counted toward the CI fix rounds. Skip a failed job whose only failed steps are Trivy scans; a job that also failed elsewhere, such as an image build, is still reported. Co-Authored-By: Claude Opus 5.5 (1M context) --- ai-review/ai_review_gate.sh | 19 +++++++++++++++---- tests/test_ai_review.py | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/ai-review/ai_review_gate.sh b/ai-review/ai_review_gate.sh index a9df131..ec72f5b 100644 --- a/ai-review/ai_review_gate.sh +++ b/ai-review/ai_review_gate.sh @@ -11,7 +11,7 @@ # if purchased from OpenC3, Inc. # Decides whether the AI review loop should run for a PR and collects failed -# CI job logs and SonarQube findings for the reviewers. Every CI workflow completion triggers the AI +# CI job logs (but not Trivy scan failures) and SonarQube findings for the reviewers. Every CI workflow completion triggers the AI # Review workflow, so this lets only the run that sees all CI finished proceed. # The Malicious Code Scan does not trigger the review, so that run waits for a # scan still running on the PR head. A manual (workflow_dispatch) run fails @@ -177,11 +177,22 @@ failures=0 while IFS=$'\t' read -r run_id run_name run_conclusion run_url; do [[ -n "$run_id" ]] || continue failures_before=$failures + ignored=0 + # A job whose only failed steps are Trivy scans failed on a vulnerability in a base image or + # dependency, which a fix to the PR cannot address, so the reviewers never see it + # shellcheck disable=SC2016 jobs="$(gh api "repos/$repo/actions/runs/$run_id/jobs" --paginate \ - --jq '.jobs[] | select(.conclusion == "failure" or .conclusion == "timed_out") | [.id, .name, .html_url] | @tsv')" \ + --jq '.jobs[] | select(.conclusion == "failure" or .conclusion == "timed_out") + | [.steps[]? | select(.conclusion == "failure" or .conclusion == "timed_out") | .name] as $failed + | [.id, .name, .html_url, ($failed != [] and all($failed[]; test("trivy"; "i")))] | @tsv')" \ || jobs="" - while IFS=$'\t' read -r job_id job_name job_url; do + while IFS=$'\t' read -r job_id job_name job_url trivy_only; do [[ -n "$job_id" ]] || continue + if [[ "$trivy_only" == "true" ]]; then + echo "::notice::Ignoring $run_name / $job_name: only its Trivy scan failed" + ignored=$((ignored + 1)) + continue + fi failures=$((failures + 1)) { echo "### $run_name / $job_name" @@ -200,7 +211,7 @@ while IFS=$'\t' read -r run_id run_name run_conclusion run_url; do echo } >> "$CI_FILE" done <<< "$jobs" - if (( failures == failures_before )); then + if (( failures == failures_before && ignored == 0 )); then failures=$((failures + 1)) { echo "### $run_name / workflow failure" diff --git a/tests/test_ai_review.py b/tests/test_ai_review.py index ef2fa71..539df6a 100644 --- a/tests/test_ai_review.py +++ b/tests/test_ai_review.py @@ -1041,6 +1041,42 @@ def test_failed_job_log_download_error_is_reported(self): self.assertIn("::warning::Could not download the log of", result.stderr) self.assertIn("(log unavailable: test API error)", (self.directory / "out/ci_failures.md").read_text()) + def test_trivy_scan_failures_are_ignored(self): + def job(job_id, *failed_steps): + steps = [{"name": "Build", "conclusion": "success"}] + steps += [{"name": name, "conclusion": "failure"} for name in failed_steps] + return { + "id": job_id, + "name": f"job {job_id}", + "conclusion": "failure", + "html_url": f"https://example.invalid/job/{job_id}", + "steps": steps, + } + + self.fixtures["repos/owner/repo/actions/runs/123/jobs"] = { + "jobs": [job(10, "Run Trivy on image ruby", "Run trivy on image node")] + } + result = self.run_shell(f'bash "{GATE}"') + self.assertEqual(result.returncode, 0, result.stderr) + # The head is an AI review fix, so a scan failure alone does not send it back for another round + self.assertEqual(self.outputs()["skip"], "true") + self.assertIn("CI passed", self.outputs()["reason"]) + self.assertEqual((self.directory / "out/ci_failures.md").read_text(), "") + self.assertIn("Ignoring Python Lint / job 10", result.stdout) + + # A failure besides the scan still reaches the reviewers + self.fixtures["repos/owner/repo/actions/runs/123/jobs"] = { + "jobs": [job(10, "Run Trivy on image ruby"), job(11, "Run Trivy on image ruby", "Build images")] + } + self.fixtures["repos/owner/repo/actions/jobs/11/logs"] = "The build broke" + self.outputs_path.unlink() + result = self.run_shell(f'bash "{GATE}"') + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(self.outputs()["ci_failures"], "1") + report = (self.directory / "out/ci_failures.md").read_text() + self.assertIn("The build broke", report) + self.assertNotIn("job 10", report) + def test_pending_builtin_run_does_not_block_review(self): runs = self.fixtures["repos/owner/repo/actions/runs?head_sha=test-head&per_page=100"]["workflow_runs"] runs.append({"id": 124, "name": "CodeQL", "event": "dynamic", "status": "in_progress", "conclusion": None}) @@ -1235,7 +1271,7 @@ def test_ai_fix_commit_goes_again_for_sonarqube_findings(self): self.outputs_path.unlink() result = self.run_shell(f'bash "{GATE}"') self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(self.outputs()["skip"], "false") + self.assertEqual(self.outputs()["skip"], "false", self.outputs().get("reason")) self.assertEqual(self.outputs()["ci_failures"], "0") def test_loop_hands_sonarqube_findings_to_the_reviewers(self):