fix(ci): don't red-fail Olympix Static Analysis on analyzer timeout / missing SARIF#2096
fix(ci): don't red-fail Olympix Static Analysis on analyzer timeout / missing SARIF#20960xDEnYO wants to merge 3 commits into
Conversation
… missing SARIF The Olympix analyzer has a fixed internal ~3-minute analysis timeout with no action input to raise it. On large changed-file sets it hits that timeout, prints "Code analysis finished unsuccessfully because of timeout", exits 0 (green), and writes NO olympix.sarif. The downstream upload-sarif step then hard-failed with a cryptic "Path does not exist: olympix.sarif", turning an uncontrollable infra timeout into a red gate on every large-diff PR (e.g. #2055). Fix: only upload the SARIF when it was actually produced, and surface an incomplete scan as a loud warning + job-summary note instead of a file-not-found error. The analyzer step is continue-on-error so an incomplete scan can't hard -fail the gate; real findings, when produced, still upload and gate normally. This check is not a required status check, so keeping the job green on an incomplete scan does not weaken merge protection; the warning keeps it honest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe Olympix workflow documents analyzer timeouts, verifies whether ChangesOlympix SARIF handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/olympixStaticAnalysis.yml:
- Around line 54-57: Remove the continue-on-error: true setting from the Olympix
analyzer step in the workflow. Preserve the existing timeout behavior and the
subsequent “Verify Olympix produced a SARIF report” step so successful timeout
exits remain handled while genuine analyzer failures stop the job.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8af4e120-96b7-472b-97bb-1b9f643d9da2
📒 Files selected for processing (1)
.github/workflows/olympixStaticAnalysis.yml
Timeouts already exit 0; continue-on-error only masked real analyzer failures. Missing SARIF is still handled by the verify/warn step. Co-authored-by: Cursor <cursoragent@cursor.com>
Which Linear task belongs to this PR?
https://linear.app/lifi-linear/issue/EXSC-665/ci-olympix-static-analysis-red-fails-on-analyzer-timeout-missing-sarif
Why did I implement it this way?
Problem
The
Static Analysis Security Checkjob (.github/workflows/olympixStaticAnalysis.yml) fails deterministically on large-diff PRs (e.g. #2055, ~14 changed.solfiles):Run Olympix Integrated Securitystep reaches the Olympix analyzer's fixed internal ~3-minute analysis timeout, printsCode analysis finished unsuccessfully because of timeout, and — critically — exits 0 (green) while writing noolympix.sarif.Upload Result to GitHub Code Scanning, then hard-fails with##[error]Path does not exist: olympix.sarif.Net: an uncontrollable infra timeout surfaces as a cryptic file-not-found red on every large-diff PR. It passed on an earlier, smaller-diff commit of the same PR and reproduced across two re-runs.
The
olympix/integrated-securityaction exposes no timeout input (only-w/-p/-f/-o/--no-<vuln>per its README/action.yaml), so the timeout cannot simply be raised.Fix
sarif_existscheck.Verify Olympix produced a SARIF reportstep turns a missing report into a loud::warningannotation + a job-summary note (naming the likely timeout and the analyzer step outcome) instead of an opaque file-not-found error.continue-on-errorso an incomplete scan can't hard-fail the gate on an infra limit we don't control. When a SARIF is produced, real findings still upload and gate exactly as before.Why not the alternatives
-pfiles (c): rejected — the run logs showScan Type: fullregardless of-p, so batching's benefit is uncertain; it also adds SARIF-merge complexity and up to N×3-min sequential Docker runs.main, so keeping the job green on an incomplete scan does not weaken merge protection; the warning + job summary keep it honest, and other gates (Aikido,check-security-alerts, CodeRabbit, human audit) provide defense in depth.Tradeoff called out
With
continue-on-error, a genuine analyzer misconfiguration (e.g. a bad API token) that produces no SARIF now surfaces as the same warning rather than a red — acceptable given the check is non-required and the warning names the analyzer step outcome. A follow-up option, if the team wants the scan to actually complete on large diffs, is upstream Olympix perf / a configurable timeout.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)