Remove fully rolled out feature flags - #16046
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the fully rolled-out Gradle lockfile updater feature flag.
Changes:
- Makes lockfile updates and convention-plugin source fetching unconditional.
- Removes obsolete experiment setup from Gradle specs.
Show a summary per file
| File | Description |
|---|---|
gradle/lib/dependabot/gradle/file_fetcher.rb |
Always fetches convention-plugin sources. |
gradle/lib/dependabot/gradle/file_updater.rb |
Always updates lockfiles. |
gradle/spec/dependabot/gradle/file_fetcher_spec.rb |
Removes experiment setup. |
gradle/spec/dependabot/gradle/file_updater_spec.rb |
Removes experiment lifecycle setup. |
gradle/spec/smoke/gradle_lockfile_spec.rb |
Removes feature-flag mocking. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
gradle/lib/dependabot/gradle/file_fetcher.rb:94
- The unconditional source scan also affects the existing
when a script plugin is presentexample. That fixture containsincluded/build.gradle, so this call requests all fourincluded/src/main/*directories, but that example only stubs the included build's wrapper requests; WebMock will reject these new requests. Addstub_no_plugin_source_files("included")to that example's setup.
files += convention_plugin_source_files(files, root_dir)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- The now-unconditional reporting path is not exercised by
create_group_update_pull_request_spec.rb; its onlyperformexample stubs/observesperformitself and does not assert error reporting. Please add#performexamples for a security-only group covering both a partially updated group and the no-update branch, so removal of this gate cannot silently stop or duplicate failed-dependency errors.
report_failed_dependency_updates_for_security_updates
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- The operation spec does not exercise
perform's security-reporting path, so removing this gate is not covered at the orchestration boundary. Please add examples for both outcomes here (a group with updates and a group with no updates) that use a security-only job and verify failed dependency errors are reported without registering an experiment; this also covers the unconditional call in theelsebranch.
report_failed_dependency_updates_for_security_updates
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
updater/lib/dependabot/updater/group_update_creation.rb:87
- This comparison is case-sensitive, so grouped security jobs can now report a dependency as missing when an advisory uses different casing from the parsed Gradle, Maven, or NuGet dependency.
DependencySnapshot#job_dependenciesexplicitly normalizes both sides for these ecosystems (updater/lib/dependabot/dependency_snapshot.rb:92-103). Normalize names for this check too while preserving the original job name in the error.
dependency_names = dependency_snapshot.all_dependencies.map(&:name)
missing_dependencies = job_dependencies - dependency_names
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
Make the rolled-out Gradle lockfile updater and grouped security error reporting behavior unconditional. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40013d5b-b127-42be-8d94-8691e1f7840c
65c8ee8 to
71b5a5a
Compare
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb — This now reports every non-updated group.dependencies entry as a failed security update, even… View resolved comment |
Suppressed comments (1)
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- This now reports intentionally deferred dependencies as failures. Regular semver groups deliberately leave members rejected by
semver_rules_allow_grouping?unhandled so they can receive individual PRs, but this helper interprets every non-updated, unhandled group member as a failed update and emits a genericDependabotError. The changedsu-group-semverfixture demonstrates the regression:dependency-cstill gets its individual PR, yet the run now fails withunknown_error. Please distinguish semver-excluded/deferred members from actual failed attempts before reporting them.
report_failed_dependency_updates_for_security_updates
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Suppressed comments (2)
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- This call treats every group dependency that is neither updated nor marked handled as a failed security update, but ordinary semver groups intentionally leave dependencies unhandled when the group’s update-type rules reject them (
group_update_creation.rb:301-303,351-360) so they can receive an individual PR later. The changedsu-group-semverfixture demonstrates the false positive:dependency-cis successfully updated inexpected-individual.json, yet this call emits anunknown_error. Track actual attempted failures explicitly, or defer this fallback until individual updates have completed, rather than inferring failure from the handled set.
report_failed_dependency_updates_for_security_updates
updater/lib/dependabot/updater/group_update_creation.rb:86
- This comparison is case-sensitive, so a valid Gradle, Maven, or NuGet dependency whose advisory casing differs from the parsed manifest name is reported as missing. The snapshot’s established matching path explicitly lowercases both sides for these ecosystems (
dependency_snapshot.rb:92-103). Preserve the original job name for the error while comparing normalized names.
dependency_names = dependency_snapshot.all_dependencies.map(&:name)
missing_dependencies = job_dependencies - dependency_names
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ater' into ruben/remove-gradle-lockfile-updater
3772656
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Balanced
Findings: None
Suppressed comments (1)
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- This now reports a false
unknown_errorfor dependencies that were intentionally excluded from the group by itsupdate-types.compile_updates_forreturns[]on a SemVer-rule mismatch and deliberately leaves that dependency unhandled so the later ungrouped pass can create an individual PR; this reporter therefore classifies it as a failed group update before that successful fallback runs. The changedsu-group-semver.txtdemonstrates the regression by expectingunknown_errorwhile both the group and individual PRs are created. Please exclude SemVer-rejected/fallback dependencies from this report (or defer reporting until after the ungrouped pass) rather than turning a successful security update job into an error.
report_failed_dependency_updates_for_security_updates
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Review tier: Balanced
Findings: None
Suppressed comments (2)
updater/lib/dependabot/updater/group_update_creation.rb:86
- This new missing-dependency check is case-sensitive, although Gradle, Maven, and NuGet dependency names may differ in case between security advisories and parsed manifests.
DependencySnapshot#job_dependenciesalready normalizes both sides for this reason (updater/lib/dependabot/dependency_snapshot.rb:92-103). As written, a present dependency can produce a falsedependency_not_founderror; compare normalized names while retaining the originals for the message.
dependency_names = dependency_snapshot.all_dependencies.map(&:name)
missing_dependencies = job_dependencies - dependency_names
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- This unconditional call reports intentionally deferred dependencies as failures. In a group with
update-types,compile_updates_forreturns[]when the semver rule rejects a dependency and leaves it unhandled sorun_ungrouped_dependency_updatescan create an individual PR later.report_failed_dependency_updates_for_security_updatesnevertheless treats every non-group-updated, unhandled group member as failed and emits a genericDependabotError; the updatedsu-group-semver.txtfixture confirms the result is two successful PRs plus anunknown_errorand a failing command. Please distinguish actual failed attempts from dependencies deferred to individual updates before reporting.
report_failed_dependency_updates_for_security_updates
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
silent/tests/testdata/su-group-semver.txt — This expectation codifies a false failure. In this fixture, dependency-c is intentionally… |
Suppressed comments (1)
updater/lib/dependabot/updater/operations/create_group_update_pull_request.rb:67
- For multi-directory jobs, this runs after
dependency_changehas iterated every directory, leavingdependency_snapshot.current_directoryset to the last one. The reporter then consults only that directory'shandled_dependencies, so a dependency whose real error was already handled in an earlier directory is reported again here as a genericunknown_error. Aggregate handled dependency outcomes across directories (while preserving directory identity where names repeat) before performing this unconditional report.
report_failed_dependency_updates_for_security_updates
| @@ -1,4 +1,5 @@ | |||
| dependabot update -f input.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent | |||
| ! dependabot update -f input.yml --local . --updater-image ghcr.io/dependabot/dependabot-updater-silent | |||
| stderr unknown_error | |||

The
gradle_lockfile_updaterandenhanced_grouped_security_error_reportingfeature flags are fully rolled out. Gradle lockfile and plugin-source behavior is now unconditional, and grouped security error reporting is now unconditional. Obsolete disabled-path specs and experiment setup were removed. Docker-based tests could not be run locally because Docker is unavailable.