Skip to content

Gate npm_and_yarn audit-fix fallbacks to security updates only#15051

Open
robaiken wants to merge 3 commits into
mainfrom
gate-npm-audit-fix-to-security-updates
Open

Gate npm_and_yarn audit-fix fallbacks to security updates only#15051
robaiken wants to merge 3 commits into
mainfrom
gate-npm-audit-fix-to-security-updates

Conversation

@robaiken
Copy link
Copy Markdown
Contributor

What are you trying to accomplish?

Restrict the npm/yarn/pnpm audit fix lockfile fallbacks so they only run for security updates, where adding registry-level overrides to package.json is justified by a known vulnerability. Regular version bumps no longer trigger audit fix.

audit fix for npm/yarn/pnpm has user-facing side-effects (it adds overrides to package.json, or rewrites lockfile entries based on advisory data) that are only appropriate for security updates. Running it as a generic fallback for every transitive-dependency update was producing surprising changes for routine version bumps.

Production code changes:

  • SubdependencyVersionResolver (npm_and_yarn): removed all audit fix code. The resolver now reports the version straight from the updated lockfile. The audit_fix_used metadata path that flowed through here is gone.
  • FileUpdater (npm_and_yarn): added a security_update? predicate that reads from dependency metadata (dependencies.any? { |d| d.metadata[:security_update] }) and forwards it as a boolean constructor arg to YarnLockfileUpdater, NpmLockfileUpdater, and PnpmLockfileUpdater.
  • YarnLockfileUpdater / NpmLockfileUpdater / PnpmLockfileUpdater: each now takes a security_update: keyword arg. The audit fix fallback is gated on both Experiments.enabled?(:enable_audit_fix_fallback) and security_update?.
  • UpdateChecker (npm_and_yarn): stamps metadata[:security_update] = true on every Dependency returned from #updated_dependencies, across all three unlock paths (:none, :own, :all), via overrides of updated_dependency_without_unlock, updated_dependency_with_own_req_unlock, and the existing build_updated_dependency hook — all routed through a with_security_update_metadata helper so the signal survives the UpdateChecker → FileUpdater boundary.

Anything you want to highlight for special attention from reviewers?

  • The signal flows as metadata (Dependency#metadata[:security_update]) from UpdateChecker to FileUpdater, and then as an explicit keyword arg (security_update:) from FileUpdater into each lockfile updater. The metadata-only approach was considered but rejected for the lockfile updaters because they sometimes receive a single dependencies array containing a mix of stamped and unstamped deps; an explicit arg keeps the gate unambiguous.
  • pnpm's deep_update fallback (pnpm update --depth Infinity) is intentionally not gated by security_update? — it does not modify package.json, so it remains a safe general fallback gated only on the :enable_audit_fix_fallback experiment. Only pnpm audit --fix (which writes overrides) is gated on security_update?.
  • The three unlock paths in Dependabot::UpdateCheckers::Base (:none / :own / :all) needed three different stamping touch-points. The two single-dependency paths build Dependency objects in the base class, so we override those methods to apply the stamp after super; the :all path is built by build_updated_dependency in the subclass, where the stamp is folded into the metadata hash before constructing the Dependency.

How will you know you've accomplished your goal?

Tests assert both directions of the gate:

Spec Coverage
update_checker_spec.rb (new security_update metadata stamping group) Stamps security_update: true for all unlock paths when advisories present; no stamp otherwise.
yarn_lockfile_updater_spec.rb run_yarn_audit_fix_command runs only when security_update: true AND experiment enabled; skipped when either is off.
npm_lockfile_updater_spec.rb Same matrix for run_npm_audit_fix_command.
pnpm_lockfile_updater_spec.rb Existing "regular package dependency" test split into security / non-security contexts. Deep-update fallback still runs without the security flag; audit --fix only with it.
subdependency_version_resolver_spec.rb Audit-related specs removed alongside the production code.

Local test results (run via bin/test npm_and_yarn):

  • update_checker_spec.rb (new group only): 6 examples, 0 failures.
  • subdependency_version_resolver_spec.rb: 11 examples, 0 failures.
  • file_updater_spec.rb: 165 examples, 0 failures, 2 pending (pre-existing).
  • {yarn,npm,pnpm}_lockfile_updater_spec.rb combined: 119 examples, 1 pending, 1 failure (pre-existing: pnpm/github_dependency_private times out making a real network call to GitHub — unrelated to this change).
  • Rubocop: 11 files inspected, no offenses.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

Copilot AI review requested due to automatic review settings May 18, 2026 17:49
@robaiken robaiken requested a review from a team as a code owner May 18, 2026 17:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR gates npm/yarn/pnpm audit-fix lockfile fallbacks so they only run for security updates, using metadata stamped by the npm_and_yarn update checker and forwarded through the file updater boundary.

Changes:

  • Removes audit-fix fallback logic from SubdependencyVersionResolver.
  • Adds security_update propagation from UpdateCheckerFileUpdater → lockfile updaters.
  • Updates specs to cover security vs non-security audit-fix behavior.
Show a summary per file
File Description
npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker/subdependency_version_resolver.rb Removes audit-fix fallback handling from subdependency resolution.
npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb Stamps updated dependencies with metadata[:security_update].
npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater.rb Detects security-update metadata and forwards it to lockfile updaters.
npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/yarn_lockfile_updater.rb Gates Yarn audit-fix fallback on security-update status.
npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb Gates npm audit-fix fallback on security-update status.
npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/pnpm_lockfile_updater.rb Gates pnpm audit-fix fallback while retaining deep-update fallback.
npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker/subdependency_version_resolver_spec.rb Removes audit-fix resolver expectations and updates pnpm fallback expectations.
npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker_spec.rb Adds security-update metadata stamping coverage.
npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/yarn_lockfile_updater_spec.rb Adds Yarn security/non-security audit-fix gating coverage.
npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater_spec.rb Adds npm security/non-security audit-fix gating coverage.
npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/pnpm_lockfile_updater_spec.rb Adds pnpm security/non-security audit-fix gating coverage.

Copilot's findings

  • Files reviewed: 11/11 changed files
  • Comments generated: 5

Comment thread npm_and_yarn/lib/dependabot/npm_and_yarn/update_checker.rb
Comment thread npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker_spec.rb Outdated
Comment thread npm_and_yarn/spec/dependabot/npm_and_yarn/update_checker_spec.rb Outdated
Comment thread npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater.rb
@robaiken robaiken force-pushed the gate-npm-audit-fix-to-security-updates branch from d1641cb to 05b0ac4 Compare May 18, 2026 18:32
@robaiken robaiken marked this pull request as draft May 20, 2026 16:38
@robaiken robaiken marked this pull request as ready for review May 20, 2026 16:40
robaiken added 3 commits May 22, 2026 16:24
This restricts the npm/yarn/pnpm 'audit fix' lockfile fallbacks so they
only run for security updates, where adding registry-level overrides to
package.json is justified by a known vulnerability. Regular version
bumps no longer trigger audit-fix.

Changes:

* Remove all audit-fix code from SubdependencyVersionResolver. The
  resolver now reports the version directly from the updated lockfile;
  audit-based version inference and the audit_fix_used metadata path
  are gone.
* In FileUpdater, expose security_update? based on dependency metadata
  (dependencies.any? { |d| d.metadata[:security_update] }) and forward
  it as a constructor arg to YarnLockfileUpdater, NpmLockfileUpdater,
  and PnpmLockfileUpdater.
* In each lockfile updater, gate the audit-fix fallback on both
  Experiments.enabled?(:enable_audit_fix_fallback) AND the new
  security_update? flag. pnpm's deep_update fallback (pnpm update
  --depth Infinity) remains gated on the experiment only since it
  doesn't modify package.json.
* In UpdateChecker, stamp metadata[:security_update] = true on every
  Dependency returned from #updated_dependencies. This works across all
  three unlock paths (:none, :own, :all) via overrides of
  updated_dependency_without_unlock and
  updated_dependency_with_own_req_unlock, plus the existing
  build_updated_dependency hook.

Tests:

* New 'security_update metadata stamping' group in
  update_checker_spec.rb verifies the flag is set across all unlock
  paths when security_advisories are present, and absent otherwise.
* New gating contexts in the three lockfile updater specs assert that
  audit-fix runs only when security_update: true AND the experiment is
  enabled, and is skipped otherwise.
* Existing pnpm 'regular package dependency' test split into security
  and non-security contexts to reflect the new gating.
* Audit-fix specs removed from subdependency_version_resolver_spec.rb.
- Make base UpdateChecker#updated_dependency_(without|with_own_req)_unlock overridable so npm_and_yarn can declare 'sig { override }' validly under Sorbet

- Restore speculative-version inference in SubdependencyVersionResolver, gated to security advisories + the enable_audit_fix_fallback experiment, so transitive security updates whose normal npm/yarn update is a no-op still flow through to the FileUpdater's audit-fix fallback

- Thread security_advisories through to SubdependencyVersionResolver and update its constructor matchers in update_checker_spec.rb

- Rewrite the two :none unlock metadata stamping tests to use the public updated_dependencies(requirements_to_unlock: :none) API instead of send(:updated_dependency_without_unlock)

- Add a security_update metadata bridge describe block in file_updater_spec.rb that verifies YarnLockfileUpdater, PnpmLockfileUpdater, and NpmLockfileUpdater receive security_update: true/false reflecting dependency metadata
…etadata stamp

These tests target security advisories (locked transitive dep, duplicated dep, no-op fix_update scenarios) where the npm_and_yarn UpdateChecker now stamps :security_update => true on each returned dependency. Bring expected metadata in line with the new stamping contract introduced earlier in this PR.
@robaiken robaiken force-pushed the gate-npm-audit-fix-to-security-updates branch from 947566b to 3bb1026 Compare May 22, 2026 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants