Skip to content

feat(skills): add fxa-dep-triage and wire it into fxa-triage - #21231

Merged
toufali merged 1 commit into
mainfrom
FXA-14513-dep-triage-skill
Sep 16, 2026
Merged

toufali merged 1 commit into
mainfrom
FXA-14513-dep-triage-skill

Conversation

@toufali

@toufali toufali commented Sep 15, 2026 •

Copy link
Copy Markdown
Member

Because

  • Security alerts BLEnder leaves open have nowhere to get triaged, and the dependency bump PRs it opens need shepherding.
  • The FxA daily summary lists dependency alerts and PRs, but often nobody acts on them.

This pull request

  • Adds a new skill, fxa-dep-triage, forked from @vbudhram 's dependabot skill. Can be run stand-alone or selected as part of the fxa-triage skill.

  • for each open vulnerability alert it shows the reasoning BLEnder recorded for why the alert is still open and recommends what to do (dismiss, bump, or finish the in-flight PR).

  • Adds a second choice at the fxa-triage opening, "dependency alert & bump PR review", that runs fxa-dep-triage in the same session, so its findings/context carry into the rest of triage session.

Issue that this pull request solves

FXA-14513

Checklist

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review

  • Run fxa-triage and choose the option "dependency alert & bump PR review".
  • Alternatively, run the dep triage skill as stand-alone: fxa-dep-triage

@toufali
toufali marked this pull request as ready for review September 15, 2026 22:52
@toufali
toufali requested a review from a team as a code owner September 15, 2026 22:52
Copilot AI balanced review requested due to automatic review settings September 15, 2026 22:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

CI classification, gate approval, pagination, and rebase safety issues can produce incomplete or unsafe triage results.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds automated dependency-security triage to FxA and integrates it with daily triage.

Changes:

  • Assesses Dependabot alerts and recommends dispositions.
  • Classifies and remediates dependency PR failures.
  • Adds dependency review as an fxa-triage workflow.
File summaries
File Description
.claude/skills/fxa-triage/SKILL.md Adds the dependency-review option.
.claude/skills/fxa-dep-triage/SKILL.md Defines the dependency-triage workflow and safeguards.
.claude/skills/fxa-dep-triage/scripts/deps.sh Implements PR reporting, CI actions, and rebasing.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 9
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/SKILL.md Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/SKILL.md Outdated

@dschom dschom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll test this out tomorrow and we can iterate if necessary.

@toufali
toufali force-pushed the FXA-14513-dep-triage-skill branch from 6f058e0 to 5cbbb81 Compare September 16, 2026 04:59
@toufali

toufali commented Sep 16, 2026 •

Copy link
Copy Markdown
Member Author

Addressed all copilot comments above. Where a finding was a symptom of too-autonomous behavior for a human-in-the-loop triage skill, I removed the feature rather than porting risky mutation:

Fixed (PR-side, feature kept):

  • alert collection now uses gh api --paginate (no 100-alert cap)
  • gh pr list --limit 100 (no 30-PR cap)
  • gate approval uses .approval_request_id, not the job .id (per _scripts/approve-functional-tests.sh)
  • GREEN classification: non-success terminal results (CANCELLED/TIMED_OUT/ERROR) now count as failing
  • rerun budget is spent only when CircleCI actually starts a workflow

Removed (advisory now — no working-tree mutation):

  • auto-rebase (rebase() + _merge_resolutions) — conflicts are reported for a human; this also removes the set -e-on-setup and package.json-allow-list concerns
  • Tier-2 auto-fix/push (verify()) — real breaks are reported with a suggested fix; guardrails now consistently forbid committing/pushing, and Edit/Write are dropped from allowed-tools

deps.sh is ~100 lines smaller as a result. The skill never merges, edits code, pushes, or touches your working tree.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

CI classification can incorrectly report merge readiness, misclassify failures, and silently omit PRs.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

.claude/skills/fxa-dep-triage/scripts/deps.sh:60

  • GREEN is the fallback whenever no checks are failing or pending, including when statusCheckRollup is empty or contains no functional-test result. This does not enforce the skill's guarantee that functional tests ran, and a newly opened PR can be reported merge-ready before CircleCI registers its checks. Require an explicit successful functional/Tests Complete (PR) result; otherwise classify it as running.
       elif ($pending|length>0) then "RUNNING"
       else "GREEN" end) as $state
  • Files reviewed: 3/3 changed files
  • Comments generated: 6
  • Review effort level: Balanced

Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/SKILL.md Outdated
@toufali
toufali force-pushed the FXA-14513-dep-triage-skill branch from 5cbbb81 to 0c60048 Compare September 16, 2026 05:14
@toufali

toufali commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Round 2 — all 6 addressed:

Fixed:

  • SKILL.md intro no longer says "attempt fixes" — Part 2 is fully advisory now (my leftover from the first pass).
  • Cosmetic-check filter now applies to the pending bucket too, so a queued Storybook run no longer holds a PR in RUNNING.
  • risk_of matches the parsed package and classifies dev-only/@types/*/actions before sensitive runtime names, so @types/jsonwebtoken is LOW.
  • report captures status output and compares classified-vs-open counts, printing an explicit INCOMPLETE warning if a gh error dropped a PR (no more silent omission via process substitution).

Removed:

  • Dropped auto-rerun entirely (rerun(), the $TMPDIR counter, MAX_RERUN) — it had generated three findings for a minor convenience. faillog still prints an advisory flaky/real HINT, but nothing auto-acts on it, so there's no rerun budget to defeat or job-name heuristic to misfire.

The skill's only mutation is now a single CircleCI action: approving the functional-test gate. Everything else is read-only + reported.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Repository overrides and suppressed API failures can target incorrect pipelines or produce misleadingly complete reports.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Comment thread .claude/skills/fxa-dep-triage/scripts/deps.sh Outdated
Because:
- Dependabot alerts BLEnder leaves open had no triage disposition step, and
  open bump PRs need triaging (FXA-14513).
- The dependency review should run standalone and from daily triage.

This commit:
- Add fxa-dep-triage (forked from fxa-dependabot): Part 1 assesses open alerts
  via BLEnder verdict tags; Part 2 classifies bot dependency PRs and takes only
  safe CI-side actions (approve gate, rerun flakes). Advisory: never merges,
  edits code, pushes, or touches the working tree.
- Wire a run choice into fxa-triage's opening; run 2 invokes fxa-dep-triage.
@toufali
toufali force-pushed the FXA-14513-dep-triage-skill branch from 0c60048 to b936cf5 Compare September 16, 2026 15:00
@toufali

toufali commented Sep 16, 2026

Copy link
Copy Markdown
Member Author

Round 3 — all 4 addressed. These were one pattern (unchecked API calls in the shell toolkit), so I did a full fail-fast pass across every gh/CircleCI call, not just the flagged lines:

  • Dropped the FXA_REPO override — the script now hardcodes mozilla/fxa, so the gh repo and the CircleCI slug can't diverge (no risk of reading a fork's branch and approving the prod pipeline).
  • list returns nonzero if any author query fails; report treats that (or a classified-vs-listed count mismatch) as an explicit INCOMPLETE warning instead of trusting a short list.
  • gate and faillog now use curl -fsS (fail on HTTP error) with a shared _cget helper, plus null/empty checks and a token check, and return nonzero when a request fails — so an API error can't be reported as "nothing to approve" or "no failed jobs".

The skill's only mutation remains a single CircleCI action (approve the functional gate); everything else is read-only and now fails loudly rather than silently.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@toufali
toufali merged commit cbc5e28 into main Sep 16, 2026
18 of 20 checks passed
@toufali
toufali deleted the FXA-14513-dep-triage-skill branch September 16, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants