Fail manifest publishing on blob collision errors

[Copilot]

PublishArtifactsInManifest could log immutable blob collision errors while still promoting the BAR build because wrapper-level errors were not considered after inner publish tasks completed.

• Failure propagation
  • Block channel assignment when the wrapper logger has errors.
  • Construct V3/V4 publish tasks with their own logger-backed asset publisher factories.

if (results.All(t => t) && !Log.HasLoggedErrors)

• Regression coverage
  • Verifies collision-style outer logged errors make wrapper execution fail.
  • Verifies asset publisher factories use inner task loggers.

To double check:

• The right tests are in and the right validation has happened. Guidance: https://github.com/dotnet/arcade/blob/main/Documentation/Validation.md

• Fixes PublishArtifactsInManifest can succeed after deterministic blob collision errors #17359

[Copilot]

Copilot wasn't able to review any files in this pull request.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Microsoft.DotNet.Build.Tasks.Feed.Tests/PublishArtifactsInManifestTests.cs:263

• The test uses reflection to access AssetPublisherFactory’s private "_log" field but doesn’t assert the field exists. If the implementation changes (e.g., rename/refactor), this will fail with a NullReferenceException rather than a clear assertion message.

            var logField = typeof(AssetPublisherFactory).GetField("_log", BindingFlags.NonPublic | BindingFlags.Instance);
            logField.Should().NotBeNull();

src/Microsoft.DotNet.Build.Tasks.Feed.Tests/PublishArtifactsInManifestTests.cs:294

• Test name says "DoesNotPromote" but the assertions don’t guard against a promotion attempt (e.g., an exception from PCS auth) adding additional error events. Tightening the assertion to ensure only the expected single error was logged makes this regression test more robust.

            result.Should().BeFalse();
            buildEngine.BuildErrorEvents.Should().ContainSingle(error =>
                error.Message.Contains("already exists with different contents"));

[Copilot]

Pull request overview

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

Suppressed comments (2)

src/Microsoft.DotNet.Build.Tasks.Feed.Tests/PublishArtifactsInManifestTests.cs:268

• This test relies on reflection to read the private field AssetPublisherFactory._log, which makes the test brittle (it will break on simple refactors/renames and is hard to understand). Consider exposing the logger via an internal/property on AssetPublisherFactory (e.g. internal TaskLoggingHelper Log => _log with InternalsVisibleTo for the test assembly) or otherwise providing a supported way to validate which logger the factory is using.

            task.ConfigureServices(collection);
            using var provider = collection.BuildServiceProvider();
            task.InvokeExecute(provider);

            var publishingTask = task.WhichPublishingTask(manifestFullPath);
            publishingTask.AssetPublisherFactory.Log.Should().BeSameAs(publishingTask.Log);
            publishingTask.AssetPublisherFactory.Log.Should().NotBeSameAs(task.Log);
        }

src/Microsoft.DotNet.Build.Tasks.Feed.Tests/PublishArtifactsInManifestTests.cs:271

• The test name claims "DoesNotPromote", but the assertions only verify the overall result is false and that an error was logged. Either add an explicit assertion that promotion was not attempted (if feasible), or rename the test to match what it actually asserts.

        [Fact]
        public async Task ExecuteAsyncReturnsFalseAndDoesNotPromoteWhenOuterLoggerHasErrors()

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/Microsoft.DotNet.Build.Tasks.Feed.Tests/PublishArtifactsInManifestTests.cs:239

• InvokeExecute runs the full task (ExecuteTask -> ExecuteAsync). In this test AssetManifestPaths is not set, so ExecuteAsync will throw (null deref on AssetManifestPaths.Select(...)) and log an exception before the assertions run. That makes the test brittle/noisy and couples it to the current implementation details of ExecuteAsync.

Set AssetManifestPaths to a deterministic dummy value so InvokeExecute fails in a controlled way (e.g., missing manifest file) without relying on an exception.

            var task = new PublishArtifactsInManifest()
            {
                BuildEngine = buildEngine,
                TargetChannels = GeneralTestingChannelId,
                AzdoApiToken = "test-token"
            };