fix(investigate): tag alert even when remediation fails; skip empty fork - #130
Conversation
…ork (#129) Root cause of the re-investigation loop: the 'Tag alert as investigated' step was the last step in the remediate job and ran only on success, so any failed remediation step (e.g. cloning an empty private-fork URL on the affected path) aborted the job before tagging — leaving the alert ungated and re-investigated every sweep. #119 only patched the yarn step; this generalizes it. - Tag step now runs with !cancelled() so a failed remediation still tags the alert (no remediation failure can cause a re-investigation loop). - post_alert_action: when the advisory has no private fork, route to advisory_only instead of private_fork, so we never attempt an empty clone. - Tests for both affected-path routings.
There was a problem hiding this comment.
Pull request overview
Fixes #129 by preventing failed remediation from repeatedly triggering costly investigations.
Changes:
- Tags alerts as investigated even after remediation failures.
- Avoids cloning when no private fork exists.
- Adds reporting and tests for advisory-only remediation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/investigate-security-alert.yml |
Runs alert tagging after failures. |
scripts/post_alert_action.py |
Routes missing forks to advisory_only. |
scripts/alert_report.py |
Adds advisory-only reporting labels. |
tests/scripts/test_post_alert_action.py |
Tests routing with and without a fork. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
groovecoder
left a comment
groovecoder
left a comment
There was a problem hiding this comment.
question (blocking): The logic is sounds, but I don't understand the scenario that broke on fxa?
Why was there an empty private fork?
This PR seems like it WILL solve the duplicate workflow runs in the scenario where there's an empty private fork ... but why did that happen?
What I'm worried about is if the workflow maybe created the private fork to create the fix, but then tried to clone the private fork before it was ready on Github? If that's the case, then the real fix is to fix the workflow so it will NOT try to clone the private fork before it's ready, right? Or we might end up putting a number of alerts into the "advisory_only" when they really do need a fix?
Fair! I don’t have definitive proof without the app-token runtime, but two empty-fork sources are visible in create_advisory_and_fork:
If it’s the race, the real fix is to wait for the fork before cloning. So then we could: • Keep the tag-on-failure change here — it’s correct regardless of root cause and is the actual loop fix (no failed remediation should ever re-trigger investigation). Let me know your pref! |
There was a problem hiding this comment.
change (blocking): This advisory_only says "Advisory created" but there are cases where it might not create an advisory. Something like "No private fork - manual fix needed." is true in all cases.
An advisory isn't always created on this path, so 'No private fork - manual fix needed' is accurate in all cases.
Fixes #129.
The loop
The Tag alert as investigated step was the last step in the
remediatejob and ran only on success. So when a remediation step failed, the job aborted before tagging — the alert was never gated, and the sweep re-investigated it every cycle (re-billing Claude each time). We hit this live on mozilla/fxa: affected alerts failed cloning an empty private-fork URL and looped.#119 only made the yarn step exit 0. The gate was still tied to remediation succeeding, so other paths (private-fork clone, etc.) still looped. This fixes the general case.
Changes
!cancelled()— a failed remediation still tags the alert, so no remediation failure can cause a re-investigation loop.post_alert_action: when the advisory has no private fork, route toadvisory_onlyinstead ofprivate_fork, so we never attempt an empty clone. (The failed run is still visible; it just no longer loops.)private_fork; no fork →advisory_only).Note
This makes the loop impossible for any remediation failure. Why the private fork comes back empty in the first place (advisory fork-creation/polling) is a separate correctness issue worth a follow-up — but it can no longer cause a loop.