[gnodet]

Summary

• A dependency POM's own <properties> must not activate its property-based profiles
• The externalActivationContext() sandbox was merging model properties into the system-property lookup, causing POMs like jruby-base to self-activate profiles unintentionally (Maven 3.10.0 activates dependency POM profiles using the POM’s own properties #13390)
• Remove the merge — only system properties and negated-property checks drive profile activation in external builds, restoring 3.9.x semantics
• This does not affect 3.10.x fails on org.jboss.resteasy:resteasy-client:jar:7.0.0.Final #13084 (RESTEasy), which uses a negated-property profile (!resteasy.dependencies.eap) that works correctly without the merge

Background

The #13084 fix introduced a sandboxed ProfileActivationContext for external model builds. The sandbox correctly suppresses user -D flags and file activation, but it also merged POM <properties> into the system-property lookup as a new behavior that never existed in 3.9.x. This caused #13390: POMs that declare a property with the same name as a profile activation key (like jruby-base's tzdata.version) would self-activate that profile.

Analysis of the actual RESTEasy POM that motivated #13084 shows it uses a negated-property profile (!resteasy.dependencies.eap), not a positive POM-property activation. Negated-property profiles work correctly without the merge — the property is absent from the sandbox, so !flag evaluates to true.

Changes

Both stacks (compat maven-model-builder and new impl/maven-impl):

• DefaultModelBuilder / DefaultProfileActivationContext: remove POM-property merge from external-model sandbox
• PropertyProfileActivator: update comments to reflect new semantics
• ProfileActivationContext interfaces: update Javadoc on model/project properties

Tests: added jruby-style (#13390 regression) and resteasy-style (#13084 non-regression) test cases

Test plan

• Compat ExternalModelProfileActivationTest — 12 tests pass
• Impl DefaultModelBuilderTest — has pre-existing ApiRunner compile failure on master (unrelated)
• Integration test with jruby-base-9.4.2.0 dependency (manual verification)

[gnodet-bot]

The fix correctly addresses the core PropertyProfileActivator path, but there is a gap in the ConditionProfileActivator path that the sandbox does not cover.

Finding 1 (MEDIUM): ConditionProfileActivator.doGetProperty() calls context.getModelProperty(name) directly (line 189) — bypassing the sandbox. The sandbox now correctly delegates getSystemProperty() cleanly, but getModelProperty() (line 403–404) still delegates to the real context. So a dependency POM with a <condition> profile referencing a POM-declared property will still self-activate after this fix.

Finding 2 (LOW): The Javadoc at line 367–369 says “Does not expose model properties through system property lookups” — true for getSystemProperty(), but ConditionProfileActivator reaches model properties via getModelProperty() directly, not through system-property lookups.

Finding 3 (LOW): No test exercises a <condition> profile activated by a POM-declared property, so the ConditionProfileActivator gap is not caught. The new pom-declared-property-condition profile uses <property> activation (tested via PropertyProfileActivator) but there is no equivalent for the condition path.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

[gnodet]

Thanks for the review!

Finding 1 addressed: ConditionProfileActivator.doGetProperty() calls context.getModelProperty() directly — the original sandbox only stopped model properties from leaking into getSystemProperty(), but getModelProperty() itself was still delegating to the real context. Fixed by overriding getModelProperty() to return null in withoutUserPropertiesAndFilesystem(), alongside getUserProperty().

Finding 2 addressed: Updated the Javadoc bullet to accurately describe that model properties are suppressed via getModelProperty() returning null, not just through the system-property lookup.

Finding 3 addressed: Added a pom-declared-property-via-condition profile to the test fixture (resolved-model-with-profiles.xml) with <condition>${pom.declared.prop} == 'hello'</condition>, and added assertions:

• testDependencyModelActivatesOnlyEnvironmentIndependentProfiles: assertNull(profile.pom.condition) — sandbox suppresses it
• testProjectBuildEvaluatesAllActivators: assertEquals("activated", profile.pom.condition) — non-sandboxed context exposes POM properties

Same fix applied to #13452 (4.0.x, which also has ConditionProfileActivator). PR #13451 (3.10.x) has no impl stack so no change needed there — its PropertyProfileActivator already only reads from getUserProperties() + getSystemProperties().

[gnodet-bot]

Following up on my earlier review — all 3 findings have been addressed.

• Finding 1 (getModelProperty() gap in ConditionProfileActivator): ✅ Fixed — the sandbox now overrides getModelProperty() to return null in DefaultProfileActivationContext.withoutUserPropertiesAndFilesystem(), closing the gap for condition-based profiles. Not needed in compat (no ConditionProfileActivator there).
• Finding 2 (Javadoc scope): ✅ Fixed — Javadoc now accurately documents that model properties are suppressed via getModelProperty() returning null, not just through the system-property lookup.
• Finding 3 (condition profile test coverage): ✅ Fixed — resolved-model-with-profiles.xml adds a <condition>${pom.declared.prop} == 'hello'</condition> profile, with assertions verifying it's suppressed in external builds (assertNull) but fires in project builds (assertEquals). Test confirmed failing before the fix.

Pre-fix validation: both compat (testExternalModelPomPropertyDoesNotSelfActivateProfile) and impl (testDependencyModelActivatesOnlyEnvironmentIndependentProfiles) tests fail without the fix. LGTM.

This review was generated by an AI agent, Hermès on behalf of @gnodet.