Move new maven detector out from experiment by zhenghao104 · Pull Request #1683 · microsoft/component-detection

zhenghao104 · 2026-03-04T23:14:10Z

We are replacing MvnCliComponentDetector logic and its unit test with MavenWithFallBackDetector

A separate PR will remove the legacy Maven static parsing detector in another repository.

codecov · 2026-03-04T23:16:29Z

Codecov Report

❌ Patch coverage is 94.07666% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.7%. Comparing base (ebb5d90) to head (8b745c6).

Files with missing lines	Patch %	Lines
...tection.Detectors/maven/MvnCliComponentDetector.cs	89.2%	38 Missing and 12 partials ⚠️
...ntDetection.Detectors.Tests/MvnCliDetectorTests.cs	99.7%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1683     +/-   ##
=======================================
- Coverage   90.8%   90.7%   -0.2%     
=======================================
  Files        453     451      -2     
  Lines      40322   39919    -403     
  Branches    2447    2426     -21     
=======================================
- Hits       36631   36223    -408     
- Misses      3191    3197      +6     
+ Partials     500     499      -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR promotes the experimental MavenWithFallbackDetector logic into the production MvnCliComponentDetector, consolidating Maven dependency detection. The MavenWithFallbackDetector is now deleted, and its behavior (Maven CLI execution with static pom.xml parsing fallback) is merged directly into MvnCliComponentDetector. A separate PR will handle removing the legacy static parsing detector from another repository.

Changes:

MvnCliComponentDetector is significantly expanded with fallback logic, environment variable control (CD_MAVEN_DISABLE_CLI), telemetry tracking, static pom.xml parsing, and timeout protection previously found only in MavenWithFallbackDetector
MavenWithFallbackDetector.cs is deleted along with its dedicated test file MavenWithFallbackDetectorTests.cs
Corresponding tests are ported to MvnCliDetectorTests.cs and the MavenWithFallbackDetector registration is removed from the DI container

Reviewed changes

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

Show a summary per file

File	Description
`src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs`	Merges `MavenWithFallbackDetector` logic (static pom.xml fallback, env var toggle, telemetry, timeout guard) into the production detector; version bumped from 4 → 5
`src/Microsoft.ComponentDetection.Detectors/maven/MavenWithFallbackDetector.cs`	Deleted — logic moved into `MvnCliComponentDetector`
`src/Microsoft.ComponentDetection.Orchestrator/Extensions/ServiceCollectionExtensions.cs`	Removes the now-deleted `MavenWithFallbackDetector` singleton registration
`test/Microsoft.ComponentDetection.Detectors.Tests/MavenWithFallbackDetectorTests.cs`	Deleted — tests ported to `MvnCliDetectorTests.cs`
`test/Microsoft.ComponentDetection.Detectors.Tests/MvnCliDetectorTests.cs`	Adds mocks for new dependencies (`IEnvironmentVariableService`, `IFileUtilityService`) and ports relevant tests from the deleted `MavenWithFallbackDetectorTests.cs`; updates `MvnCliHappyPath` helper to configure the new mock setup

Copilot · 2026-03-04T23:21:17Z

test/Microsoft.ComponentDetection.Detectors.Tests/MvnCliDetectorTests.cs

+
+        // Verify telemetry shows fallback reason as other
+        detectorResult.AdditionalTelemetryDetails.Should().ContainKey("FallbackReason");
+        detectorResult.AdditionalTelemetryDetails["FallbackReason"].Should().Be("OtherMvnCliFailure");


The WhenMvnCliFailsWithNonAuthError_SetsFallbackReasonToOther_Async test does not assert that the FailedEndpoints telemetry key is absent when the Maven CLI failure is not due to authentication. The equivalent test in the now-deleted MavenWithFallbackDetectorTests.cs had this assertion: detectorResult.AdditionalTelemetryDetails.Should().NotContainKey("FailedEndpoints"). Without this assertion, if a bug caused FailedEndpoints to be set on non-auth errors, it would go undetected.

Suggested change

detectorResult.AdditionalTelemetryDetails["FallbackReason"].Should().Be("OtherMvnCliFailure");

detectorResult.AdditionalTelemetryDetails["FallbackReason"].Should().Be("OtherMvnCliFailure");

detectorResult.AdditionalTelemetryDetails.Should().NotContainKey("FailedEndpoints");

Copilot · 2026-03-04T23:21:17Z

test/Microsoft.ComponentDetection.Detectors.Tests/MvnCliDetectorTests.cs

+            x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),
+            Times.Once);
+    }
+


Several important test scenarios from the deleted MavenWithFallbackDetectorTests.cs are not ported to MvnCliDetectorTests.cs. These scenarios now apply to the promoted MvnCliComponentDetector but have no test coverage:

Transitive dependency preservation: WhenMavenCliSucceeds_PreservesTransitiveDependencies_Async — verifies that when Maven CLI succeeds, the dependency graph (parent → transitive) is preserved correctly.

Complete CLI failure with nested modules: WhenMvnCliFailsCompletely_AllNestedPomXmlsAreRestoredForStaticParsing_Async — verifies that when CLI fails for all pom.xml files, nested modules are restored for static parsing.

Partial CLI failure: WhenMvnCliPartiallyFails_NestedPomXmlsRestoredOnlyForFailedDirectories_Async — verifies that nested pom.xml files under directories that succeeded with CLI are not statically parsed, while directories where CLI failed are restored for static parsing.

DetectionMethod telemetry: No test verifies the DetectionMethod=MvnCliOnly telemetry entry when CLI succeeds for all files.

CD_MAVEN_DISABLE_CLI=false behavior: WhenDisableMvnCliEnvVarIsFalse_UsesMvnCliNormally_Async and related tests — verify that setting the env var to false or an invalid value still allows CLI to run normally.

These scenarios represent core functionality of the merged detector logic and should have corresponding tests.

Suggested change

[TestMethod]

public async Task WhenMavenCliSucceeds_PreservesTransitiveDependencies_Async()

{

// Arrange

const string bcdeMvnFileName = "bcde.mvndeps";

var cliOutput = @"

<dependencies>

<dependency>

<groupId>com.example</groupId>

<artifactId>parent-app</artifactId>

<version>1.0.0</version>

</dependency>

<dependency>

<groupId>com.example</groupId>

<artifactId>child-lib</artifactId>

<version>2.0.0</version>

</dependency>

</dependencies>";

this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)

.Returns(bcdeMvnFileName);

this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())

.ReturnsAsync(true);

this.mavenCommandServiceMock.Setup(x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))

.ReturnsAsync(new MavenCliResult(true, null));

this.fileUtilityServiceMock.Setup(x => x.Exists(It.IsAny<string>())).Returns(true);

this.fileUtilityServiceMock.Setup(x => x.ReadAllText(It.IsAny<string>())).Returns(cliOutput);

const string rootPom = @"<project>

<modelVersion>4.0.0</modelVersion>

<groupId>com.example</groupId>

<artifactId>parent-app</artifactId>

<version>1.0.0</version>

</project>";

var (scanResult, componentRecorder) = await this.DetectorTestUtility

.WithFile("pom.xml", rootPom)

.WithFile("pom.xml", cliOutput, searchPatterns: [bcdeMvnFileName])

.ExecuteDetectorAsync();

// Assert

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

var components = componentRecorder.GetDetectedComponents();

components.Should().HaveCount(2);

components.Should().Contain(c => c.Component is MavenComponent mc &&

mc.ArtifactId == "parent-app");

components.Should().Contain(c => c.Component is MavenComponent mc &&

mc.ArtifactId == "child-lib");

}

[TestMethod]

public async Task WhenMvnCliFailsCompletely_AllNestedPomXmlsAreRestoredForStaticParsing_Async()

{

// Arrange

const string bcdeMvnFileName = "bcde.mvndeps";

this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)

.Returns(bcdeMvnFileName);

this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())

.ReturnsAsync(true);

this.mavenCommandServiceMock.Setup(x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))

.ReturnsAsync(new MavenCliResult(false, "mvn failed"));

const string rootPom = @"<project>

<modelVersion>4.0.0</modelVersion>

<groupId>com.example</groupId>

<artifactId>root-app</artifactId>

<version>1.0.0</version>

<modules>

<module>module-a</module>

</modules>

</project>";

const string modulePom = @"<project>

<modelVersion>4.0.0</modelVersion>

<groupId>com.example</groupId>

<artifactId>module-a</artifactId>

<version>1.0.0</version>

</project>";

var (scanResult, _) = await this.DetectorTestUtility

.WithFile("pom.xml", rootPom)

.WithFile("module-a/pom.xml", modulePom)

.ExecuteDetectorAsync();

// Assert

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

// CLI should have been attempted for both root and nested pom.xml

this.mavenCommandServiceMock.Verify(

x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),

Times.Exactly(2));

}

[TestMethod]

public async Task WhenMvnCliPartiallyFails_NestedPomXmlsRestoredOnlyForFailedDirectories_Async()

{

// Arrange

const string bcdeMvnFileName = "bcde.mvndeps";

this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)

.Returns(bcdeMvnFileName);

this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())

.ReturnsAsync(true);

// First call succeeds, second fails

this.mavenCommandServiceMock.SetupSequence(

x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))

.ReturnsAsync(new MavenCliResult(true, null))

.ReturnsAsync(new MavenCliResult(false, "mvn failed"));

const string rootPom = @"<project>

<modelVersion>4.0.0</modelVersion>

<groupId>com.example</groupId>

<artifactId>root-app</artifactId>

<version>1.0.0</version>

<modules>

<module>module-a</module>

</modules>

</project>";

const string modulePom = @"<project>

<modelVersion>4.0.0</modelVersion>

<groupId>com.example</groupId>

<artifactId>module-a</artifactId>

<version>1.0.0</version>

</project>";

var (scanResult, _) = await this.DetectorTestUtility

.WithFile("pom.xml", rootPom)

.WithFile("module-a/pom.xml", modulePom)

.ExecuteDetectorAsync();

// Assert

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

this.mavenCommandServiceMock.Verify(

x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),

Times.Exactly(2));

}

[TestMethod]

public async Task WhenMvnCliSucceeds_EmitsMvnCliOnlyDetectionMethodTelemetry_Async()

{

// Arrange

const string cliOutput = @"<dependencies></dependencies>";

this.MvnCliHappyPath(cliOutput);

// Act

var (scanResult, _) = await this.DetectorTestUtility.ExecuteDetectorAsync();

// Assert

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

scanResult.AdditionalTelemetryDetails.Should().ContainKey("DetectionMethod");

scanResult.AdditionalTelemetryDetails["DetectionMethod"].Should().Be("MvnCliOnly");

}

[TestMethod]

public async Task WhenDisableMvnCliEnvVarIsFalse_UsesMvnCliNormally_Async()

{

// Arrange

const string cliOutput = @"<dependencies></dependencies>";

Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", "false");

try

{

this.MvnCliHappyPath(cliOutput);

// Act

var (scanResult, _) = await this.DetectorTestUtility.ExecuteDetectorAsync();

// Assert

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

this.mavenCommandServiceMock.Verify(

x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),

Times.AtLeastOnce);

}

finally

{

Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", null);

}

}

[TestMethod]

public async Task WhenDisableMvnCliEnvVarIsInvalid_UsesMvnCliNormally_Async()

{

// Arrange

const string cliOutput = @"<dependencies></dependencies>";

Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", "not-a-bool");

try

{

this.MvnCliHappyPath(cliOutput);

// Act

var (scanResult, _) = await this.DetectorTestUtility.ExecuteDetectorAsync();

// Assert

scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);

this.mavenCommandServiceMock.Verify(

x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),

Times.AtLeastOnce);

}

finally

{

Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", null);

}

}

Copilot · 2026-03-04T23:21:18Z

test/Microsoft.ComponentDetection.Detectors.Tests/MvnCliDetectorTests.cs

+    public async Task WhenDisableMvnCliTrue_UsesStaticParsing_Async()
+    {
+        // Arrange
+        this.environmentVariableServiceMock.Setup(x => x.IsEnvironmentVariableValueTrue("CD_MAVEN_DISABLE_CLI"))


The test uses the hardcoded string "CD_MAVEN_DISABLE_CLI" instead of the constant MvnCliComponentDetector.DisableMvnCliEnvVar. While the constant is internal and not directly accessible from the test assembly, the old MavenWithFallbackDetectorTests.cs used MavenWithFallbackDetector.DisableMvnCliEnvVar because that constant was also internal but accessible because tests were aware of it. Consider making DisableMvnCliEnvVar accessible via an [assembly: InternalsVisibleTo("Microsoft.ComponentDetection.Detectors.Tests")] attribute (as done in other parts of the codebase, e.g., src/Microsoft.ComponentDetection.Detectors/pip/IPyPiClient.cs:21) or changing the visibility to public, so the test references the constant directly instead of a magic string that can silently diverge.

Suggested change

this.environmentVariableServiceMock.Setup(x => x.IsEnvironmentVariableValueTrue("CD_MAVEN_DISABLE_CLI"))

this.environmentVariableServiceMock.Setup(x => x.IsEnvironmentVariableValueTrue(MvnCliComponentDetector.DisableMvnCliEnvVar))

github-actions · 2026-03-05T00:00:38Z

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

The detector detects more or fewer components than before
The detector generates different parent/child graph relationships than before
The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

Copilot

Pull request overview

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

Copilot · 2026-03-05T06:21:44Z

src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs

+        IFileUtilityService fileUtilityService,
        ILogger<MvnCliComponentDetector> logger)
    {
        this.ComponentStreamEnumerableFactory = componentStreamEnumerableFactory;
        this.Scanner = walkerFactory;
        this.mavenCommandService = mavenCommandService;
+        this.envVarService = envVarService;
+        this.fileUtilityService = fileUtilityService;


The IFileUtilityService dependency is injected and assigned to this.fileUtilityService in the constructor, but it is never actually used anywhere else in MvnCliComponentDetector. The old MavenWithFallbackDetector used it to read the generated deps file content after a successful Maven CLI run (fileUtilityService.Exists(depsFilePath) and fileUtilityService.ReadAllText(depsFilePath)), but the new implementation instead relies on ComponentStreamEnumerableFactory.GetComponentStreams() to find all generated dependency files in the source directory. This unused dependency should be removed from the constructor signature and class to avoid confusion.

Suggested change

IFileUtilityService fileUtilityService,

ILogger<MvnCliComponentDetector> logger)

{

this.ComponentStreamEnumerableFactory = componentStreamEnumerableFactory;

this.Scanner = walkerFactory;

this.mavenCommandService = mavenCommandService;

this.envVarService = envVarService;

this.fileUtilityService = fileUtilityService;

ILogger<MvnCliComponentDetector> logger)

{

this.ComponentStreamEnumerableFactory = componentStreamEnumerableFactory;

this.Scanner = walkerFactory;

this.mavenCommandService = mavenCommandService;

this.envVarService = envVarService;

Copilot · 2026-03-05T06:21:44Z

test/Microsoft.ComponentDetection.Detectors.Tests/MvnCliDetectorTests.cs

+    [TestMethod]
+    public async Task WhenMavenCliProducesNoOutput_FallsBackToStaticParsing_Async()
+    {
+        // Arrange
+        this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())
+            .ReturnsAsync(true);
+        this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)
+            .Returns("bcde.mvndeps");
+        this.mavenCommandServiceMock.Setup(x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new MavenCliResult(true, null));
+
+        // File exists but is empty
+        this.fileUtilityServiceMock.Setup(x => x.Exists(It.IsAny<string>())).Returns(true);
+        this.fileUtilityServiceMock.Setup(x => x.ReadAllText(It.IsAny<string>())).Returns(string.Empty);
+
+        var pomXmlContent = @"<?xml version=""1.0"" encoding=""UTF-8""?>
+<project xmlns=""http://maven.apache.org/POM/4.0.0"">
+    <modelVersion>4.0.0</modelVersion>
+    <groupId>com.test</groupId>
+    <artifactId>my-app</artifactId>
+    <version>1.0.0</version>
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-lang3</artifactId>
+            <version>3.12.0</version>
+        </dependency>
+    </dependencies>
+</project>";
+
+        // Act
+        var (detectorResult, componentRecorder) = await this.DetectorTestUtility
+            .WithFile("pom.xml", pomXmlContent)
+            .ExecuteDetectorAsync();
+
+        // Assert
+        detectorResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+
+        var detectedComponents = componentRecorder.GetDetectedComponents();
+        detectedComponents.Should().ContainSingle();
+
+        var mavenComponent = detectedComponents.First().Component as MavenComponent;
+        mavenComponent.Should().NotBeNull();
+        mavenComponent.GroupId.Should().Be("org.apache.commons");
+        mavenComponent.ArtifactId.Should().Be("commons-lang3");
+        mavenComponent.Version.Should().Be("3.12.0");
+    }


This test is copied from the now-deleted MavenWithFallbackDetectorTests but is no longer testing the correct behavior. In the old MavenWithFallbackDetector, the fallback to static parsing occurred when GenerateDependenciesFileAsync returned Success=true but fileUtilityService.ReadAllText(depsFilePath) returned an empty string. The new MvnCliComponentDetector does NOT use IFileUtilityService at all — it relies on ComponentStreamEnumerableFactory.GetComponentStreams() to find any generated bcde.mvndeps files. When CLI reports success but no bcde.mvndeps file exists in the test virtual filesystem, the new implementation does NOT fall back to static parsing (since no failedDirectories are recorded). As a result, this test likely passes with zero detected components, making the assertion detectedComponents.Should().ContainSingle() incorrect. The fileUtilityServiceMock setups are also dead code. This test should be removed or rewritten to reflect the actual behavior of the new implementation.

Copilot · 2026-03-05T06:21:44Z

src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs

+    /// This allows the MvnCliComponentDetector
    /// to safely share the same output file without race conditions.


The updated comment at lines 23-27 is now grammatically incomplete. The sentence "This allows the MvnCliComponentDetector" ends without finishing the thought, then the next line "to safely share the same output file without race conditions." starts with "to" without a subject, making the full sentence read: "This allows the MvnCliComponentDetector to safely share the same output file without race conditions." This is actually a dangling incomplete sentence in the XML doc comment. The comment should be rephrased to form a complete, readable sentence, such as: "This allows the MvnCliComponentDetector to safely share the same output file without race conditions."

Suggested change

/// This allows the MvnCliComponentDetector

/// to safely share the same output file without race conditions.

/// This design allows the MvnCliComponentDetector to safely share the same output file without race conditions.

- Add comprehensive directory scanning after Maven CLI execution to find all generated dependency files - Combines original direct file reading (for test compatibility) with ComponentStreamEnumerableFactory scanning (for submodule detection) - Maintains all existing functionality while fixing missing submodule dependencies - Minimal changes to preserve existing behavior and test compatibility

- Add expectedRemovedDetectors list to handle intentional detector removals - Skip assertions for expected removed detectors in ProcessDetectorVersions - Adjust detector count validation to account for expected removals - Resolves verification test failures caused by consolidating Maven detection into MvnCli

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Copilot · 2026-03-05T07:14:54Z

test/Microsoft.ComponentDetection.VerificationTests/ComponentDetectionIntegrationTests.cs

    private void ProcessDetectorVersions()
    {
+        // List of detectors that were intentionally removed
+        var expectedRemovedDetectors = new HashSet<string> { "MavenWithFallback" };


The expectedRemovedDetectors HashSet is defined independently in both CheckDetectorsRunTimesAndCounts (line 176) and ProcessDetectorVersions (line 247), with the same literal content { "MavenWithFallback" }. This duplication means any future updates to the list must be made in two places. Consider extracting it to a class-level field or property to avoid divergence.

Suggested change

private void ProcessDetectorVersions()

{

// List of detectors that were intentionally removed

var expectedRemovedDetectors = new HashSet<string> { "MavenWithFallback" };

private static readonly HashSet<string> ExpectedRemovedDetectors = new HashSet<string> { "MavenWithFallback" };

private void ProcessDetectorVersions()

{

// List of detectors that were intentionally removed

var expectedRemovedDetectors = ExpectedRemovedDetectors;

Copilot · 2026-03-05T07:14:55Z

test/Microsoft.ComponentDetection.VerificationTests/ComponentDetectionIntegrationTests.cs

    [TestMethod]
    public void CheckDetectorsRunTimesAndCounts()
    {
+        // List of detectors that were intentionally removed
+        var expectedRemovedDetectors = new HashSet<string> { "MavenWithFallback" };


The same expectedRemovedDetectors HashSet with identical content is also defined at line 176 in CheckDetectorsRunTimesAndCounts. Both definitions contain "MavenWithFallback" and would need to be updated together whenever the list changes.

Suggested change

[TestMethod]

public void CheckDetectorsRunTimesAndCounts()

{

// List of detectors that were intentionally removed

var expectedRemovedDetectors = new HashSet<string> { "MavenWithFallback" };

private static readonly string[] ExpectedRemovedDetectors = { "MavenWithFallback" };

[TestMethod]

public void CheckDetectorsRunTimesAndCounts()

{

// List of detectors that were intentionally removed

var expectedRemovedDetectors = new HashSet<string>(ExpectedRemovedDetectors);

Copilot · 2026-03-05T07:14:55Z

src/Microsoft.ComponentDetection.Detectors/maven/MvnCliComponentDetector.cs

@@ -89,74 +401,440 @@ await this.RemoveNestedPomXmls(processRequests).ForEachAsync(processRequest =>
                    SingleFileComponentRecorder = this.ComponentRecorder.CreateSingleFileComponentRecorder(
                        Path.Combine(Path.GetDirectoryName(componentStream.Location), MavenManifest)),
                };
-            })
-            .ToObservable();
+            });
+
+        // Combine dependency files from CLI success with pom.xml files from CLI failures
+        return results.Concat(allGeneratedDependencyFiles).ToObservable();


The RunMavenCliDetectionAsync method in the updated MvnCliComponentDetector constructs the return value by concatenating two sources: (1) the results ConcurrentBag, which is populated with ProcessRequests for every successfully-processed deps file (line ~312), and (2) allGeneratedDependencyFiles which scans the entire source directory for ALL bcde.mvndeps files (lines 383-404). These two sources will both yield the same deps file for each successful Maven CLI run, causing ProcessMvnCliResult (and thus ParseDependenciesFile) to be called twice for the same file. This will double-count components in mvnCliComponentCount and may cause duplicate graph entries depending on the recorder's behavior. The allGeneratedDependencyFiles scan appears intended to handle sub-module deps files generated by Maven that were not explicitly tracked in results, but it should be filtered to exclude paths already present in results, or results should not add items that will also be picked up by allGeneratedDependencyFiles.

Copilot · 2026-03-05T07:14:55Z

test/Microsoft.ComponentDetection.Detectors.Tests/MvnCliDetectorTests.cs

+    public async Task StaticParser_IgnoresDependenciesWithoutVersion_Async()
+    {
+        // Arrange
+        this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())
+            .ReturnsAsync(false);
+
+        var pomXmlContent = @"<?xml version=""1.0"" encoding=""UTF-8""?>
+<project xmlns=""http://maven.apache.org/POM/4.0.0"">
+    <modelVersion>4.0.0</modelVersion>
+    <groupId>com.test</groupId>
+    <artifactId>my-app</artifactId>
+    <version>1.0.0</version>
+    <dependencies>
+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-lang3</artifactId>
+        </dependency>
+    </dependencies>
+</project>";
+
+        // Act
+        var (detectorResult, componentRecorder) = await this.DetectorTestUtility
+            .WithFile("pom.xml", pomXmlContent)
+            .ExecuteDetectorAsync();
+
+        // Assert
+        detectorResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+        var detectedComponents = componentRecorder.GetDetectedComponents();
+        detectedComponents.Should().BeEmpty();
+    }


The new StaticParser_IgnoresDependenciesWithoutVersion_Async test only includes a single dependency without a version, asserting that zero components are detected. The equivalent test in the deleted MavenWithFallbackDetectorTests had two dependencies (one without version, one with a valid version), verifying that the valid one is still detected while the versionless one is skipped. This mixed case — where some dependencies have missing versions and others are valid — is now untested.

Copilot · 2026-03-05T07:14:55Z

test/Microsoft.ComponentDetection.VerificationTests/ComponentDetectionIntegrationTests.cs

+            // Account for expected removed detectors
+            var expectedMatchCount = oldMatches.Count - expectedRemovedDetectors.Count;


The expectedMatchCount calculation assumes exactly one log line per removed detector (oldMatches.Count - expectedRemovedDetectors.Count). However, each detector may have zero or more log entries (e.g., if MavenWithFallback ran but produced no output, or produced multiple entries). If MavenWithFallback contributed more than one match in the old log, this calculation will produce an over-permissive threshold (too few expected matches required), causing a legitimate lost-detector regression to be silently ignored.

Suggested change

// Account for expected removed detectors

var expectedMatchCount = oldMatches.Count - expectedRemovedDetectors.Count;

// Account for expected removed detectors by excluding their matches from the old count

var removedDetectorMatchCount = oldMatches.Cast<Match>()

.Count(m => m.Groups[2].Success && expectedRemovedDetectors.Contains(m.Groups[2].Value));

var expectedMatchCount = oldMatches.Count - removedDetectorMatchCount;

FernandoRojo · 2026-03-05T18:26:31Z

src/Microsoft.ComponentDetection.Detectors/maven/MavenCommandService.cs

        finally
        {
            semaphore.Release();
        }


Technically don't need this semaphore anymore, but i don't think there's any harm in keeping it there

FernandoRojo · 2026-03-05T18:39:15Z

test/Microsoft.ComponentDetection.VerificationTests/ComponentDetectionIntegrationTests.cs


-            newMatches.Should().HaveCountGreaterThanOrEqualTo(oldMatches.Count, "A detector was lost, make sure this was intentional.");
+            // Account for expected removed detectors
+            var expectedMatchCount = oldMatches.Count - expectedRemovedDetectors.Count;


There is an issue with adding this, as it would allow any detector to be removed after this and no longer fail this first check phase.

Since this is a test that runs as part of the PPE phase and not during the PR, i think it would be best to keep it as is and have that manual confirm step be the expectation.

We also just need to fix those PPE tests in general as they are flaky and pretty much always fail: https://dev.azure.com/cg-scus1/MyFirstProject/_build?definitionId=697&_a=summary

FernandoRojo · 2026-03-05T18:39:57Z

test/Microsoft.ComponentDetection.VerificationTests/ComponentDetectionIntegrationTests.cs

            if (newDetector == null)
            {
-                newDetector.Should().NotBeNull($"the detector {cd.DetectorId} was lost, verify this is expected behavior");
+                if (!expectedRemovedDetectors.Contains(cd.DetectorId))


This one is fine though

Move new maven detector out from experiment

76bb43d

zhenghao104 requested a review from a team as a code owner March 4, 2026 23:14

zhenghao104 requested review from Copilot and jasonpaulos March 4, 2026 23:14

Copilot started reviewing on behalf of zhenghao104 March 4, 2026 23:15 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Comment update

6dda964

Copilot AI review requested due to automatic review settings March 5, 2026 06:15

Copilot started reviewing on behalf of zhenghao104 March 5, 2026 06:16 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

zhenghao104 force-pushed the users/zhentan/move-maven-logic-into-mvncli branch from cdba85e to 351a80c Compare March 5, 2026 06:49

Copilot AI review requested due to automatic review settings March 5, 2026 07:08

Copilot started reviewing on behalf of zhenghao104 March 5, 2026 07:09 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

FernandoRojo reviewed Mar 5, 2026

View reviewed changes

	detectorResult.AdditionalTelemetryDetails["FallbackReason"].Should().Be("OtherMvnCliFailure");
	detectorResult.AdditionalTelemetryDetails["FallbackReason"].Should().Be("OtherMvnCliFailure");
	detectorResult.AdditionalTelemetryDetails.Should().NotContainKey("FailedEndpoints");

+    [TestMethod]
+    public async Task WhenMavenCliSucceeds_PreservesTransitiveDependencies_Async()
+    {
+        // Arrange
+        const string bcdeMvnFileName = "bcde.mvndeps";
+        var cliOutput = @"
+<dependencies>
+  <dependency>
+    <groupId>com.example</groupId>
+    <artifactId>parent-app</artifactId>
+    <version>1.0.0</version>
+  </dependency>
+  <dependency>
+    <groupId>com.example</groupId>
+    <artifactId>child-lib</artifactId>
+    <version>2.0.0</version>
+  </dependency>
+</dependencies>";
+        this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)
+            .Returns(bcdeMvnFileName);
+        this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())
+            .ReturnsAsync(true);
+        this.mavenCommandServiceMock.Setup(x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new MavenCliResult(true, null));
+        this.fileUtilityServiceMock.Setup(x => x.Exists(It.IsAny<string>())).Returns(true);
+        this.fileUtilityServiceMock.Setup(x => x.ReadAllText(It.IsAny<string>())).Returns(cliOutput);
+        const string rootPom = @"<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>com.example</groupId>
+  <artifactId>parent-app</artifactId>
+  <version>1.0.0</version>
+</project>";
+        var (scanResult, componentRecorder) = await this.DetectorTestUtility
+            .WithFile("pom.xml", rootPom)
+            .WithFile("pom.xml", cliOutput, searchPatterns: [bcdeMvnFileName])
+            .ExecuteDetectorAsync();
+        // Assert
+        scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+        var components = componentRecorder.GetDetectedComponents();
+        components.Should().HaveCount(2);
+        components.Should().Contain(c => c.Component is MavenComponent mc &&
+                                         mc.ArtifactId == "parent-app");
+        components.Should().Contain(c => c.Component is MavenComponent mc &&
+                                         mc.ArtifactId == "child-lib");
+    }
+    [TestMethod]
+    public async Task WhenMvnCliFailsCompletely_AllNestedPomXmlsAreRestoredForStaticParsing_Async()
+    {
+        // Arrange
+        const string bcdeMvnFileName = "bcde.mvndeps";
+        this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)
+            .Returns(bcdeMvnFileName);
+        this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())
+            .ReturnsAsync(true);
+        this.mavenCommandServiceMock.Setup(x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new MavenCliResult(false, "mvn failed"));
+        const string rootPom = @"<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>com.example</groupId>
+  <artifactId>root-app</artifactId>
+  <version>1.0.0</version>
+  <modules>
+    <module>module-a</module>
+  </modules>
+</project>";
+        const string modulePom = @"<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>com.example</groupId>
+  <artifactId>module-a</artifactId>
+  <version>1.0.0</version>
+</project>";
+        var (scanResult, _) = await this.DetectorTestUtility
+            .WithFile("pom.xml", rootPom)
+            .WithFile("module-a/pom.xml", modulePom)
+            .ExecuteDetectorAsync();
+        // Assert
+        scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+        // CLI should have been attempted for both root and nested pom.xml
+        this.mavenCommandServiceMock.Verify(
+            x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),
+            Times.Exactly(2));
+    }
+    [TestMethod]
+    public async Task WhenMvnCliPartiallyFails_NestedPomXmlsRestoredOnlyForFailedDirectories_Async()
+    {
+        // Arrange
+        const string bcdeMvnFileName = "bcde.mvndeps";
+        this.mavenCommandServiceMock.Setup(x => x.BcdeMvnDependencyFileName)
+            .Returns(bcdeMvnFileName);
+        this.mavenCommandServiceMock.Setup(x => x.MavenCLIExistsAsync())
+            .ReturnsAsync(true);
+        // First call succeeds, second fails
+        this.mavenCommandServiceMock.SetupSequence(
+                x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new MavenCliResult(true, null))
+            .ReturnsAsync(new MavenCliResult(false, "mvn failed"));
+        const string rootPom = @"<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>com.example</groupId>
+  <artifactId>root-app</artifactId>
+  <version>1.0.0</version>
+  <modules>
+    <module>module-a</module>
+  </modules>
+</project>";
+        const string modulePom = @"<project>
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>com.example</groupId>
+  <artifactId>module-a</artifactId>
+  <version>1.0.0</version>
+</project>";
+        var (scanResult, _) = await this.DetectorTestUtility
+            .WithFile("pom.xml", rootPom)
+            .WithFile("module-a/pom.xml", modulePom)
+            .ExecuteDetectorAsync();
+        // Assert
+        scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+        this.mavenCommandServiceMock.Verify(
+            x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),
+            Times.Exactly(2));
+    }
+    [TestMethod]
+    public async Task WhenMvnCliSucceeds_EmitsMvnCliOnlyDetectionMethodTelemetry_Async()
+    {
+        // Arrange
+        const string cliOutput = @"<dependencies></dependencies>";
+        this.MvnCliHappyPath(cliOutput);
+        // Act
+        var (scanResult, _) = await this.DetectorTestUtility.ExecuteDetectorAsync();
+        // Assert
+        scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+        scanResult.AdditionalTelemetryDetails.Should().ContainKey("DetectionMethod");
+        scanResult.AdditionalTelemetryDetails["DetectionMethod"].Should().Be("MvnCliOnly");
+    }
+    [TestMethod]
+    public async Task WhenDisableMvnCliEnvVarIsFalse_UsesMvnCliNormally_Async()
+    {
+        // Arrange
+        const string cliOutput = @"<dependencies></dependencies>";
+        Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", "false");
+        try
+        {
+            this.MvnCliHappyPath(cliOutput);
+            // Act
+            var (scanResult, _) = await this.DetectorTestUtility.ExecuteDetectorAsync();
+            // Assert
+            scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+            this.mavenCommandServiceMock.Verify(
+                x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),
+                Times.AtLeastOnce);
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", null);
+        }
+    }
+    [TestMethod]
+    public async Task WhenDisableMvnCliEnvVarIsInvalid_UsesMvnCliNormally_Async()
+    {
+        // Arrange
+        const string cliOutput = @"<dependencies></dependencies>";
+        Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", "not-a-bool");
+        try
+        {
+            this.MvnCliHappyPath(cliOutput);
+            // Act
+            var (scanResult, _) = await this.DetectorTestUtility.ExecuteDetectorAsync();
+            // Assert
+            scanResult.ResultCode.Should().Be(ProcessingResultCode.Success);
+            this.mavenCommandServiceMock.Verify(
+                x => x.GenerateDependenciesFileAsync(It.IsAny<ProcessRequest>(), It.IsAny<CancellationToken>()),
+                Times.AtLeastOnce);
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable("CD_MAVEN_DISABLE_CLI", null);
+        }
+    }

	this.environmentVariableServiceMock.Setup(x => x.IsEnvironmentVariableValueTrue("CD_MAVEN_DISABLE_CLI"))
	this.environmentVariableServiceMock.Setup(x => x.IsEnvironmentVariableValueTrue(MvnCliComponentDetector.DisableMvnCliEnvVar))

		/// This allows the MvnCliComponentDetector
		/// to safely share the same output file without race conditions.

	/// This allows the MvnCliComponentDetector
	/// to safely share the same output file without race conditions.
	/// This design allows the MvnCliComponentDetector to safely share the same output file without race conditions.

		// Account for expected removed detectors
		var expectedMatchCount = oldMatches.Count - expectedRemovedDetectors.Count;

-            // Account for expected removed detectors
-            var expectedMatchCount = oldMatches.Count - expectedRemovedDetectors.Count;
+            // Account for expected removed detectors by excluding their matches from the old count
+            var removedDetectorMatchCount = oldMatches.Cast<Match>()
+                .Count(m => m.Groups[2].Success && expectedRemovedDetectors.Contains(m.Groups[2].Value));
+            var expectedMatchCount = oldMatches.Count - removedDetectorMatchCount;

Conversation

zhenghao104 commented Mar 4, 2026

Uh oh!

codecov bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

FernandoRojo Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

FernandoRojo Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

FernandoRojo Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 4, 2026 •

edited

Loading

github-actions bot commented Mar 5, 2026 •

edited

Loading