Repository navigation
Withhold Maven pins whose coordinates a profile redefines - #2026
Draft
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Draft
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
Conversation
Mikola Lysenko (mikolalysenko)
force-pushed
the
fix/maven-discovery-property-overrides
branch
from
November 4, 2026 21:39
d7b4b40 to
ec89bd9
Compare
Collaborator
Author
|
Burn-down agent: merged the rewritten base (#2020 was force-pushed to
Local: Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
force-pushed
the
fix/maven-discovery-coordinate-overrides
branch
from
November 5, 2026 19:16
9ce83ae to
2b06722
Compare
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Nov 6, 2026
Review findings on the profile / nested property resolution (#2020): 1. Model references moved by properties (false attestation). The model (${project.version} from <version>${revision}</version>) was bound already resolved from the root properties, and profiles were assumed unable to change it. Model references are now bound raw and resolved wherever they are read, with that read's user properties and active profiles: a profile <revision> moves ${project.version} (unattested), and a -Drevision=<pin> only .mvn/maven.config supplies makes the spelled and built versions differ, so it is diagnosed, never a ref. 2. Attributed / spaced property tags (false attestation). read_properties took the whole tag as the name, so <v combine.self="override"> and <v > were skipped. The name now ends at whitespace or '/', closing tags may carry whitespace, and <v/> defines an empty value. 3. Nested references in a profile value resolved against the root. Each combination of profiles is now resolved as one property view (the profiles merged over the pom's, a later one winning), so a profile's ${w} reads its own <w> first. 4. Profile <properties> parse errors failed the whole scope, including the hosted rewriter's. PomScope::with_user_properties no longer reads profile properties; parse_pom_with calls read_profiles(), and an unreadable one withholds every ${...} pin (MovedBy::Unreadable) while the rest of the pom is read. 5. The shared cross-product budget collapsed ordinary multi-profile poms. interpolate_each and its 16-result / 1024-lookup product are gone: profile_combinations walks the names reachable from the version, keeps only the profiles that define one (at most 8, else fail closed), and every combination of those is resolved on its own budget. 6. The detail named the lexicographically first alternative. Alternatives are listed smallest combination first and carry the profile ids (MovedBy), and the detail names them and the version they give. 7. Two precedence implementations. formats::maven::properties now holds lookup_property (Maven's order: model, user properties, each pom's properties with its active profiles merged in, bare ${version}), PomLayer and profile_combinations; PomScope and the vendored Reactor::lookup_with / version_override both use them, so vendoring now weighs the same combinations VEX does (it previously tried one profile at a time) and fails closed past the same bound. 8. Two interpolators. The round-based coordinate interpolate is gone: coordinates and model references go through the same expansion as versions, in a keep-unresolved mode (interpolate_lenient). Coordinate alternatives stay for #2026; PomScope::resolve_in_build takes the interpolation so it can reuse it. 9. Cleanups: resolutions() drops its from-profile flag (the classifier check derives it), PomScope::fixed and the profile-set guard are gone, and readers that never weigh profiles do not scan them. Tests (each failed before this commit): - vex::discover::maven::tests::model_references_move_with_the_properties_they_are_spelled_through - vex::discover::maven::tests::profile_properties_with_attributes_are_read - vex::discover::maven::tests::profile_values_resolve_through_their_own_profile - vex::discover::maven::tests::unreadable_profile_properties_withhold_only_property_pins - vex::discover::maven::tests::many_profiles_that_keep_the_pin_are_attested - vex::discover::maven::tests::profile_property_detail_names_the_profile_and_version - vendor::jvm::maven_reactor::tests::profiles_that_move_the_version_only_together_are_weighed plus formats::maven::properties unit tests and extended formats::maven::tests::profile_properties_give_version_alternatives. The golden corpus is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes the last Maven property false-attestation gap in VEX discovery after #2020: a <groupId> / <artifactId> written as ${prop} that a profile's <properties> redefine. Coordinates were read only with no profile active, so discovery attested two shapes the build may not resolve: (a) The declaration carrying the pin names another package whenever a profile redefining its coordinate property is active; the pin then no longer applies to the patched GA, which resolves from its other declarations or transitively, unpinned. (b) Another declaration (live, or inside a profile whose own properties retarget it) becomes the patched GA under a profile, at its own version off the pin. Both readers now resolve coordinates through #2020's shared helpers (formats::maven::properties): the groupId and artifactId are joined with COORDINATE_SEPARATOR (U+0001, which XML 1.0 forbids) so one profile_combinations walk and one interpolation per combination read both under the same active profiles, through interpolate_keeping (the keep-unresolved interpolate_lenient as an Interpolate), and split_coordinates splits the result. formats/maven: PomScope::coordinate_alternatives runs the joined text through resolve_in_build; PomDep carries group_text / artifact_text and coordinate_alternatives (CoordinateAlternative: group, artifact, MovedBy), every pair some combination gives other than the no-profile reading (ScopedDep.group / artifact, unchanged), plus a profile declaration's own-profile reading. More than 8 relevant profiles or an unreadable profile fail closed (the text as written, which may_name reads as any text). vex/discover/maven: bypasses() runs coordinate_override after profile_override, reported as MavenProfileUnpatched (detail names the profiles), for (a) the live main-jar declaration carrying the pin (build_version == pin) with any alternative, and (b) any declaration with an alternative that may be the GA's main jar (literal or may_shadow; pom / sources / javadoc types skipped) at a version other than the pin among its resolutions(), or literally an executable classifier copy off the pin or with no version of its own. As in profile_override, a managed declaration counts only where management governs the pin, and a versionless main jar takes the live pin. CLI note and CLI_CONTRACT.md updated. vendor/jvm/maven_reactor: the v5 planner had the same gap. interpolate_with and profile_override take the Interpolate to use (interpolate_strict for versions, as before), and coordinates_elsewhere reads the joined coordinates in each inheritor and under each profile combination on the chain through them. (a) The existing coordinates_overridden check now weighs profiles, not only inheritors. (b) New warn_moved_coordinates: a declaration that names another artifact in its pom but may be the GA in an inheritor or under a profile combination is warned (coordinates_overridden) and, when versioned, unpins its root and is uncertain, so attestable_checked no longer attests it. This also covers the inheritor form of (b). Tests (each fails before this change): - vex::discover::maven::tests::profile_properties_moving_the_pinned_coordinates_are_not_attested - vex::discover::maven::tests::profile_properties_turning_a_declaration_into_the_pinned_ga_are_not_attested - vendor::jvm::maven_reactor::tests::a_profile_moving_the_declared_coordinates_is_left_alone - vendor::jvm::maven_reactor::tests::a_profile_turning_a_declaration_into_the_patched_artifact_is_weighed - formats::maven::tests::profile_properties_give_coordinate_alternatives Guard: vex::discover::maven::tests::unrelated_coordinate_properties_do_not_withhold. User-property variants (a chain through -D, -D beating the profile) are in each. The golden corpus is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko)
force-pushed
the
fix/maven-discovery-coordinate-overrides
branch
from
November 6, 2026 05:00
2b06722 to
e458517
Compare
…ecks /code-review high findings on #2026. Discovery (vex/discover/maven.rs): coordinate_override is gone. Each declaration's coordinate_alternatives now carry the version it resolves under the same profile combination, and bypasses() weighs every such reading as a declaration of its own (retargeted(): read only under those profiles, marked retargeted_by) through the existing unpatched_classifier_copy and profile_override checks; only the pinned declaration moving off the GA keeps its own small check (pin_retargeted). So: - (1) a classifier copy retargeted onto the GA is weighed with the Wiring: under the pre-v5 vendored repository (main jar only) it withholds even at the vendored version; - (6) a profile that moves coordinates and version onto the pin together no longer withholds; - (8) the reading's <type> maps as Maven's does: test-jar is the tests classifier copy, any other non-jar type (war, pom, ...) is not the GA's jar. formats/maven: resolve_in_build is a wrapper over resolve_all_in_build, which reads several texts of one declaration under the same profile combinations but each through its own interpolation and work budget; profile_combinations takes all of them. (5) The U+0001 joined text is gone: an unterminated ${ in one coordinate stays in that one, and a reading always names a non-empty profile set. Vendored (maven_reactor.rs): profile_override / read_elsewhere read several texts under one combination, each on its own budget. (7) warn_unresolved_coordinates and warn_moved_coordinates are one pass, warn_property_coordinates, classifying each declaration as unresolved or moved with a shared warn-and-unpin tail. (2)(4) Text that can never read the patched GA (may_interpolate_to) is ruled out before any inheritor or profile is read, so a >8-profile fail-closed walk no longer unpins roots for ${g}:guava. (3) An executable classifier copy an inheritor or profile retargets onto the GA warns classifier_declared, as a literal one does. Hosted (9, back-port): PomIndex reads profile <properties> and each declaration's coordinate_alternatives, and rewrite_maven_pom refuses a dep whose pinned declaration a profile moves off the GA, or that another declaration becomes at another version (or as an executable classifier copy), with nothing written: redirect_maven_profile_coordinates_unpatched (CLI_CONTRACT.md). The vex_maven_profile_unpatched note no longer suggests re-patching such a pin with vendored mode. Tests (each fails with the fix disabled): - vex::discover::maven::tests::vendored_ref_under_a_retargeted_classifier_copy_is_not_attested - vendor::jvm::maven_reactor::tests::many_profiles_on_an_unrelated_coordinate_are_not_weighed - vendor::jvm::maven_reactor::tests::a_profile_retargeting_a_classifier_copy_warns - patch::redirect::tests::maven_pom_profile_moved_coordinates_refuse_the_dep - new cases in profile_properties_turning_a_declaration_into_the_pinned_ga_are_not_attested (test-jar withheld; war and coordinates+version onto the pin attested) and formats::maven::tests::profile_properties_give_coordinate_alternatives (an unterminated ${ in one coordinate). Redirect and VEX goldens unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Back-port rule: hosted must not write a pin VEX can never attest.
Hosted scan already refuses a live ${property} version, so the shape
left was a versioned <profiles> declaration of the GA beside a live
one: hosted warned redirect_maven_profile_dependency_unpatched but
still pinned the live declaration, and discovery's profile_override
withheld that pin forever. That covers a profile literal at the base or
another release, a profile ${property} (undefined, or one its own or
another profile's <properties> move off the pin, alone or combined),
a profile declaration through an undefined coordinate property, and a
profile <dependencyManagement> entry where management governs the pin
(the <dependencyManagement> pin hosted adds when no live direct
declaration carries a version).
PomIndex now records each declaration's managed flag and every version
the build may resolve for it (PomScope::resolve_in_build with
interpolate_strict, the same data discovery reads), and
PomIndex::profile_bypass mirrors discovery's profile check. When it
finds one, rewrite_maven_pom refuses the dep with nothing written,
under the existing redirect_maven_profile_dependency_unpatched code
(detail: the declaration, the version it resolves, the suffixed pin).
A versionless profile declaration, one resolving the pin, or profile
management under a direct pin still lets the pin land; the old
informational warning now fires only when nothing live is pinned or on
the legacy same-GAV path. CLI_CONTRACT.md and the
vex_maven_profile_unpatched note say so.
Tests (each fails with the check disabled):
- patch::redirect::tests::maven_profile_versions_are_weighed_before_pinning (new)
- patch::redirect::tests::maven_versioned_profile_declarations_are_never_pinned_over
and maven_pom_undefined_coordinate_property_in_a_profile_is_reported,
which asserted the old pin-beside-a-profile behavior.
Redirect golden (profile-literal keeps its warning code) and the VEX
golden corpus are unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Back-port rule: hosted must not write a pin VEX can never attest.
Hosted scan skipped a dep only when an executable classifier copy
(not sources / javadoc) named the patched release; one at another
release, through a ${property}, or with no version in the pom let the
main jar's pin land, and discovery's unpatched_classifier_copy then
withheld it forever (vex_maven_classifier_unpatched).
PomIndex::unserved_classifier_copy mirrors that check with the shared
is_executable_classifier and the per-declaration build_versions (every
version the build may resolve, profile <properties> included): a
non-managed executable copy of the GA, live or inside <profiles>, whose
own versions, or else its classifier-keyed <dependencyManagement>
entries' (live; a profile's too; none live = no version in this pom),
include anything but the suffixed pin. rewrite_maven_pom then refuses
the dep with nothing written under the existing
redirect_maven_classifier_unsupported code (detail: the copy, the
version it resolves, the suffixed pin). sources / javadoc copies still
only warn, and the legacy same-GAV path keeps its release check. A
test-jar typed declaration has no classifier and stays refused as an
unsupported packaging (live) or a profile bypass. CLI_CONTRACT.md says
so in both the hosted and the vex_maven_classifier_unpatched sections.
Test (fails with the check disabled):
- patch::redirect::tests::maven_executable_classifier_off_the_pin_blocks_the_redirect
Redirect golden and the VEX golden corpus are unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rrides' into fix/maven-discovery-coordinate-overrides # Conflicts: # crates/socket-patch-core/src/vex/discover/maven.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #2020. This closes the last Maven property false-attestation gap in VEX discovery: a
<groupId>/<artifactId>written as${prop}that a profile's<properties>redefine. Hosted scan gets the same refusal, and so does the v5 vendored planner.The gap
Coordinates were resolved only with no profile active. Discovery therefore attested two shapes the build may not resolve:
Fix
formats/maven: coordinates now go through Resolve Maven pin properties through profiles and nesting #2020's shared helpers.
resolve_in_buildis now a wrapper overresolve_all_in_build. That function reads a declaration's groupId, artifactId and version under the same profile combinations, with each text on its own interpolation (interpolate_keepingfor coordinates,interpolate_strictfor the version) and its own work budget.profile_combinationstakes all three texts.PomDep::coordinate_alternativesholds each reading whose coordinates differ from the no-profile reading, with its version and the profiles that cause it. A profile declaration's reading under its own profile counts too.<properties>that can't be parsed, fail closed: the text as written becomes the reading.vex/discover/maven:
bypasses()treats each such reading as a declaration of its own (retargeted()), active only under those profiles and at the version it resolves there. The existing checks then weigh it:unpatched_classifier_copy(with theWiring) andprofile_override.<type>is read the way Maven maps it, consistent withmay_shadow:test-jaris thetestsclassifier copy, and any other non-jartype isn't a jar on the classpath.pin_retargeted.vendor/jvm/maven_reactor: the v5 planner had the same gap.
read_elsewhereandprofile_overrideread several texts under one combination, each on its own budget.version_overrideandcoordinates_elsewhereboth use them.coordinates_overriddencheck now covers profiles as well as inheritors.warn_property_coordinatesis one pass over the property-coordinate declarations. It first rules out text that can never name the GA (may_interpolate_to), then sorts each remaining declaration into unresolved (property_unresolved) or moved under a profile or inheritor (coordinates_overridden). Both share one warn-and-unpin tail. An executable classifier copy getsclassifier_declared, as a literal one does.Hosted (back-port):
PomIndexnow reads profile<properties>and each declaration'scoordinate_alternatives.rewrite_maven_pomrefuses the dep, writing nothing, in two cases:The warning is
redirect_maven_profile_coordinates_unpatched, documented in CLI_CONTRACT.md.scantherefore never writes a pin thatvexwould withhold for this reason. Thevex_maven_profile_unpatchednote no longer suggests re-patching such a pin with vendored mode.Hosted, profile versions (back-port): hosted already refused a live
${property}version, but it still pinned a live declaration that had a versioned<profiles>declaration of the GA beside it, and discovery then withheld that pin forever.PomIndexnow records each declaration's managed flag and every version the build may resolve for it, usingresolve_in_buildwithinterpolate_strict, the same data discovery reads.PomIndex::profile_bypassmirrors discovery'sprofile_overridecheck.redirect_maven_profile_dependency_unpatchedcode.${property}that is undefined, or that its own or any combination of profiles'<properties>move off the pin; a profile declaration through an undefined coordinate property; and a profile<dependencyManagement>entry where management governs the pin.Hosted, classifier copies (back-port): hosted used to skip a dep only when an executable classifier copy named the patched release.
PomIndex::unserved_classifier_copynow mirrors discovery'sunpatched_classifier_copy, using the sharedis_executable_classifierand each declaration'sbuild_versions.redirect_maven_classifier_unsupportedcode, when an executable copy may resolve anything but the suffixed pin.<profiles>, and versions that are another release, a${property}a profile moves, or none in the pom once its classifier-keyed<dependencyManagement>(a profile's too) is consulted.sources/javadoccopies still only warn. Atest-jartyped declaration stays refused as an unsupported packaging, or as a profile bypass.Tests
Each of these fails before this change, or with the fix disabled:
vex::discover::maven::tests::profile_properties_moving_the_pinned_coordinates_are_not_attestedvex::discover::maven::tests::profile_properties_turning_a_declaration_into_the_pinned_ga_are_not_attested. New cases:test-jaris withheld;war, and coordinates plus version moved onto the pin together, are attested.vex::discover::maven::tests::vendored_ref_under_a_retargeted_classifier_copy_is_not_attestedvendor::jvm::maven_reactor::tests::a_profile_moving_the_declared_coordinates_is_left_alonevendor::jvm::maven_reactor::tests::a_profile_turning_a_declaration_into_the_patched_artifact_is_weighedvendor::jvm::maven_reactor::tests::many_profiles_on_an_unrelated_coordinate_are_not_weighedvendor::jvm::maven_reactor::tests::a_profile_retargeting_a_classifier_copy_warnspatch::redirect::tests::maven_pom_profile_moved_coordinates_refuse_the_deppatch::redirect::tests::maven_executable_classifier_off_the_pin_blocks_the_redirectpatch::redirect::tests::maven_profile_versions_are_weighed_before_pinning. Also updated:maven_versioned_profile_declarations_are_never_pinned_overandmaven_pom_undefined_coordinate_property_in_a_profile_is_reported, which asserted the old behavior of pinning beside a profile declaration.formats::maven::tests::profile_properties_give_coordinate_alternatives, including an unterminated${in one coordinate.The guard test is
unrelated_coordinate_properties_do_not_withhold. The user-property variants are inside each test. The redirect goldens (tests/redirect_golden.rs) and the VEX golden corpus are unchanged.Review findings (/code-review high)
unpatched_classifier_copywith theWiring. Under the pre-v5 vendored repository it is withheld even at the vendored version.warn_property_coordinatesfilters withmay_interpolate_toon the raw text before any inheritor or profile walk, so the >8-profile fail-closed path only applies to text that could name the GA. The reactor'sprofile_overridestill fails closed whatevermovedsays; the filter is the caller's job, as its doc now states.classifier_declared, without unpinning the root. That is the same treatment as a literal copy inrewrite_declarations.sources/javadoccopies are still not warned about.coordinates_elsewhere.${stays in its own coordinate, and every reading names a non-empty profile set: combinations are non-empty, and an own-profile reading is added only when that profile exists.coordinate_overrideis removed. Readings are expanded into per-combination declarations and go through the existing checks, which closes (1) and the "coordinates and version both moved onto the pin" gap. Only the "pin moves off the GA" case keeps its own small check, because no existing check looks for a pin disappearing.warn_property_coordinates.retargeted()) and hosted (PomIndex::retargeted). A literal-coordinate declaration's type handling is Stop attesting Maven pins a profile or user property overrides #2005's and is unchanged.<properties>) bypasses.Not done
attestable_checkeddoes not withhold for it. This predates this PR (rewrite_declarations'classifier_declared).Generated by Claude Code