Improvements/build definition source gen #233

DecSmith42 · 2025-12-04T14:17:45Z

No description provided.

- BuildDefinition is now MinimalBuildDefinition - DefaultBuildDefinition is now BuildDefinition - BuildDefinitionAttribute and MinimalBuildDefinitionAttribute attributed builds now auto-inherit the respective base definition class if no base class is already set - Updated tests to reflect changes

Copilot

Pull request overview

This PR refactors the build definition architecture by renaming and reorganizing the core classes and attributes to create a clearer hierarchy. The changes rename BuildDefinition to MinimalBuildDefinition (the minimal base class), and DefaultBuildDefinition to BuildDefinition (the comprehensive base class with common targets). This improves naming clarity by making BuildDefinition the recommended starting point and MinimalBuildDefinition the lean alternative.

Key Changes

Class hierarchy refactoring: Introduced MinimalBuildDefinition as the base and made BuildDefinition inherit from it with additional features (ISetupBuildInfo, IValidateBuild, IDotnetUserSecrets)
Attribute updates: Created MinimalBuildDefinitionAttribute for the new naming, with BuildDefinitionAttribute now serving as a marker for both use cases
Source generator enhancements: Updated generators to recognize both attributes and intelligently choose base types based on attribute and existing inheritance

Reviewed changes

Copilot reviewed 75 out of 75 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`DecSm.Atom/Build/Definition/MinimalBuildDefinition.cs`	New minimal base class (formerly `BuildDefinition`) providing core `IBuildDefinition` implementation
`DecSm.Atom/Build/Definition/BuildDefinition.cs`	Renamed from `DefaultBuildDefinition`; now inherits from `MinimalBuildDefinition` with common build targets
`DecSm.Atom/Build/Definition/MinimalBuildDefinitionAttribute.cs`	New attribute with comprehensive documentation (content moved from old `BuildDefinitionAttribute`)
`DecSm.Atom/Build/Definition/BuildDefinitionAttribute.cs`	Simplified to empty attribute class
`DecSm.Atom.SourceGenerators/BuildDefinitionSourceGenerator.cs`	Enhanced to detect base types and choose appropriate inheritance (`MinimalBuildDefinition`, `BuildDefinition`, or `IBuildDefinition` interface)
`DecSm.Atom/Workflows/WorkflowGenerator.cs`	Updated cast from `BuildDefinition` to `MinimalBuildDefinition`
`_atom/Build.cs`, `Sample_*/Build.cs`	Updated to use `[BuildDefinition]` attribute without explicit base class inheritance
Test files	Comprehensive updates across all test builds to use `[MinimalBuildDefinition]` with appropriate base classes

Comments suppressed due to low confidence (4)

DecSm.Atom.Tests/BuildTests/BuildInfo/BuildInfoBuild.cs:20

Downcasting 'this' from IBuildInfoTarget to BuildInfoBuild introduces a dependency cycle between the two types.
DecSm.Atom.Tests/BuildTests/BuildInfo/BuildInfoBuild.cs:21
Downcasting 'this' from IBuildInfoTarget to BuildInfoBuild introduces a dependency cycle between the two types.
DecSm.Atom.Tests/BuildTests/BuildInfo/BuildInfoBuild.cs:22
Downcasting 'this' from IBuildInfoTarget to BuildInfoBuild introduces a dependency cycle between the two types.
DecSm.Atom.Tests/BuildTests/BuildInfo/BuildInfoBuild.cs:23
Downcasting 'this' from IBuildInfoTarget to BuildInfoBuild introduces a dependency cycle between the two types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-04T14:22:56Z

DecSm.Atom/Build/Definition/MinimalBuildDefinition.cs

+///         A class deriving from <c>BuildDefinition</c> (or <see cref="BuildDefinition" />) must be
+///         decorated with the <see cref="MinimalBuildDefinitionAttribute" /> to be recognized by the Atom framework.


The documentation mentions that a class deriving from <c>BuildDefinition</c> must be decorated with MinimalBuildDefinitionAttribute. This is confusing because:

The sentence refers to both <c>BuildDefinition</c> (the text) and <see cref="BuildDefinition" /> (the tag)

Classes inheriting from the BuildDefinition class should typically use [BuildDefinition] attribute, not [MinimalBuildDefinition]

The sentence should clarify that MinimalBuildDefinition (the current class) requires the [MinimalBuildDefinition] attribute, while BuildDefinition typically uses [BuildDefinition] attribute.

// Should be: A class deriving from <c>MinimalBuildDefinition</c> must be decorated with the <see cref="MinimalBuildDefinitionAttribute" /> or <see cref="BuildDefinitionAttribute" /> to be recognized by the Atom framework.

Copilot · 2025-12-04T14:22:57Z

DecSm.Atom/Build/Definition/MinimalBuildDefinitionAttribute.cs

+/// using DecSm.Atom.Hosting;
+/// [BuildDefinition] // Marks this class as the build definition
+/// [GenerateEntryPoint] // Often used with GenerateEntryPoint to create a Program.cs
+/// internal partial class MyBuild : DefaultBuildDefinition, IMyCustomTarget


The documentation example still references DefaultBuildDefinition, which has been renamed to BuildDefinition. The example code should be updated to use the current class name.

// Should be: [BuildDefinition] // Marks this class as the build definition [GenerateEntryPoint] // Often used with GenerateEntryPoint to create a Program.cs internal partial class MyBuild : BuildDefinition, IMyCustomTarget

Copilot · 2025-12-04T14:22:57Z

...initionSourceGeneratorTests.MinimalBuildDefinition_WithBaseType_GeneratesSource.verified.txt

+partial class MinimalTestDefinition : DecSm.Atom.Build.Definition.IBuildDefinition
+{
+    // Build has no defined targets
+
+    public MinimalTestDefinition(System.IServiceProvider services) : base(services) { }


The source generator is producing incorrect code. When a class has a base type (MinimalBuildDefinition), it should inherit from IBuildDefinition in the generated code. However, the constructor at line 22 calls base(services), which will fail because IBuildDefinition is an interface and doesn't have a constructor. The generated code should not include a constructor when the class already has a base type, or should only implement IBuildDefinition interface without the base constructor call.

Copilot · 2025-12-04T14:22:57Z

DecSm.Atom.SourceGenerators/BuildDefinitionSourceGenerator.cs

+        // If class has a base type, only implement IBuildDefinition
+        // If the class doesn't have a base type, inherit from BuildDefinition or MinimalBuildDefinition depending on the attribute
+
+        var typeToInherit = IBuildDefinitionFull;
+
+        if (classSymbol.BaseType?.Name is null or "object" or "Object" or "System.Object")
+            typeToInherit = classSymbol
+                .GetAttributes()
+                .Any(x => x.AttributeClass?.ToDisplayString() is MinimalBuildDefinitionAttributeFull)
+                ? MinimalBuildDefinitionFull
+                : BuildDefinitionFull;


The logic for determining typeToInherit is incomplete. When a class has a base type (line 381), typeToInherit is set to IBuildDefinitionFull (an interface), but the generated code at line 410 (outside this diff) always includes a constructor with base(services) call. This will cause a compilation error because interfaces don't have constructors.

The generator needs to conditionally generate the constructor only when typeToInherit is a concrete class (MinimalBuildDefinitionFull or BuildDefinitionFull), not when it's the IBuildDefinitionFull interface.

Suggested fix: Add logic to track whether a constructor should be generated:

var needsConstructor = classSymbol.BaseType?.Name is null or "object" or "Object" or "System.Object";

Then use this flag to conditionally generate the constructor.

Copilot · 2025-12-04T14:22:58Z

DecSm.Atom/Build/Definition/BuildDefinition.cs

-///         Developers should typically inherit from <see cref="DefaultBuildDefinition" /> for a more comprehensive
-///         set of pre-configured targets and options. However, <c>BuildDefinition</c> can be used as a
-///         more minimal base if finer-grained control or a very lean setup is desired.
+///         <c>DefaultBuildDefinition</c> is the recommended starting point for most Atom build projects.


The documentation still refers to DefaultBuildDefinition, which is the old name of this class. It should be updated to BuildDefinition to match the current implementation.

// Should be: <c>BuildDefinition</c> is the recommended starting point for most Atom build projects.

Copilot · 2025-12-04T14:22:58Z

DecSm.Atom/Build/Definition/BuildDefinition.cs

-///         decorated with the <see cref="BuildDefinitionAttribute" /> to be recognized by the Atom framework.
+///         The main build definition class in an Atom project (typically <c>_atom/Build.cs</c>) would inherit from
+///         <c>DefaultBuildDefinition</c>
+///         and be decorated with <see cref="MinimalBuildDefinitionAttribute" />.


The documentation states that the class should be decorated with MinimalBuildDefinitionAttribute, but this is incorrect. Based on the PR changes, classes inheriting from BuildDefinition should use the [BuildDefinition] attribute (which now maps to BuildDefinitionAttribute), not [MinimalBuildDefinition]. The [MinimalBuildDefinition] attribute is for classes that either have no base class or inherit from MinimalBuildDefinition.

// Should be: and be decorated with <see cref="BuildDefinitionAttribute" />.

Copilot · 2025-12-04T14:22:58Z

DecSm.Atom/Build/Definition/MinimalBuildDefinition.cs

+/// <remarks>
+///     <para>
+///         Developers should typically inherit from <see cref="BuildDefinition" /> for a more comprehensive
+///         set of pre-configured targets and options. However, <c>BuildDefinition</c> can be used as a


The documentation uses <c>BuildDefinition</c> ambiguously. In this context, it should refer to <c>MinimalBuildDefinition</c> since the sentence is describing when to use the current class (MinimalBuildDefinition) vs the comprehensive BuildDefinition class.

// Should be: set of pre-configured targets and options. However, <c>MinimalBuildDefinition</c> can be used as a

Copilot · 2025-12-04T14:22:59Z

DecSm.Atom/Build/Definition/MinimalBuildDefinition.cs

+/// [BuildDefinition]
+/// internal partial class MinimalBuild : BuildDefinition, IMyTargets


The example code uses [BuildDefinition] attribute but the class name is MinimalBuild and inherits from BuildDefinition. This is inconsistent with the typical usage pattern. For a minimal build example in the MinimalBuildDefinition class documentation, it should either:

Use [MinimalBuildDefinition] attribute with MinimalBuildDefinition base class, or

Use [MinimalBuildDefinition] attribute with no base class

Current example mixing [BuildDefinition] with a minimal example is confusing.

// Suggested example: [MinimalBuildDefinition] internal partial class MinimalBuild : MinimalBuildDefinition, IMyTargets

Copilot · 2025-12-04T14:22:59Z

DecSm.Atom/Build/Definition/BuildDefinition.cs

 /// [BuildDefinition]
-/// internal partial class MinimalBuild : BuildDefinition, IMyTargets
+/// [GenerateEntryPoint] // Generates Program.cs
+/// internal partial class Build : DefaultBuildDefinition


The example code still references DefaultBuildDefinition, which has been renamed to BuildDefinition. The example should be updated to reflect the current class name.

// Should be: internal partial class Build : BuildDefinition

Copilot · 2025-12-04T14:22:59Z

README.md

+            Logger.LogInformation("MySecret: {SecretValue}", MySecret);
+        });


Logger.LogInformation("MySecret: {SecretValue}", MySecret); logs the value of a secret defined via [SecretDefinition]. Even if redaction is claimed, this encourages emitting sensitive data to logs, and redaction can fail or be bypassed, exposing credentials in CI logs or local runs. Remove logging of secrets entirely and, if needed, log only metadata (e.g., that a secret was retrieved) without the secret value:

Logger.LogInformation("Secret 'my-secret' retrieved successfully"); // Do not log the secret value

DecSmith42 added 2 commits December 4, 2025 23:48

docs(readme): update README

4ad9e0d

Copilot AI review requested due to automatic review settings December 4, 2025 14:17

DecSmith42 enabled auto-merge December 4, 2025 14:17

Copilot started reviewing on behalf of DecSmith42 December 4, 2025 14:18 View session

Copilot finished reviewing on behalf of DecSmith42 December 4, 2025 14:21

Copilot AI reviewed Dec 4, 2025

View reviewed changes

DecSmith42 disabled auto-merge December 4, 2025 14:33

DecSmith42 merged commit 1bbc38f into main Dec 4, 2025
36 checks passed

DecSmith42 deleted the improvements/build-definition-source-gen branch December 4, 2025 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements/build definition source gen #233

Improvements/build definition source gen #233

Uh oh!

DecSmith42 commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Copilot AI Dec 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		/// A class deriving from <c>BuildDefinition</c> (or <see cref="BuildDefinition" />) must be
		/// decorated with the <see cref="MinimalBuildDefinitionAttribute" /> to be recognized by the Atom framework.

		/// [BuildDefinition]
		/// internal partial class MinimalBuild : BuildDefinition, IMyTargets

		Logger.LogInformation("MySecret: {SecretValue}", MySecret);
		});

Improvements/build definition source gen #233

Improvements/build definition source gen #233

Uh oh!

Conversation

DecSmith42 commented Dec 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant