Skip to content

Enhance customALGoFiles feature - #2273

Open
Ole Wunschmann (OleWunschmann) wants to merge 58 commits into
microsoft:mainfrom
OleWunschmann:enhance-customALGoFiles-feature
Open

Ole Wunschmann (OleWunschmann) wants to merge 58 commits into
microsoft:mainfrom
OleWunschmann:enhance-customALGoFiles-feature

Conversation

@OleWunschmann

@OleWunschmann Ole Wunschmann (OleWunschmann) commented Jun 3, 2026 •

Copy link
Copy Markdown
Contributor

❔What, Why & How

This pull request introduces significant improvements to the handling of custom template file inclusion and exclusion in AL-Go actions, focusing on robust path resolution, improved deduplication, and enhanced cross-platform compatibility. The changes also clarify and expand the documentation for settings, and add a new utility function for reading settings with custom templates.

Improvements to custom template file handling:

  • The current customALGoFiles settings from the custom template are now merged and used directly during updates, ensuring that the latest template configuration is always applied without waiting for a new update cycle.
  • Both filesToInclude and filesToExclude are now resolved against both the custom template and the original AL-Go template, ensuring that files added or removed upstream are properly propagated to consumer repositories.
  • A new destinationName property is added to filesToInclude, allowing files to be renamed when copied to the destination. This is reflected in both the schema and implementation.

Path Handling and Cross-Platform Support

  • Introduced GetPathStringComparison and GetPathStringComparer utility functions to ensure path comparisons and deduplication are case-insensitive on Windows/macOS and case-sensitive on Linux, improving cross-platform reliability.
  • Updated ResolveFilePaths to canonicalize all paths, enforce that destination folders/files are within allowed directories, and use the new path comparison utilities for accurate and secure file operations.
  • Updated CheckForUpdates.ps1 to skip files, whose source/destination paths physically resolve to other paths (to avoid issues with symlinks or junctions)

Documentation and schema updates:

  • The schema and documentation for filesToInclude and filesToExclude have been updated to clarify their new behavior and the new destinationName property.

These changes make custom template management more robust, predictable, and safer for consumer repositories.

Related to discussion: #2227

✅ Checklist

  • Add tests (E2E, unit tests)
  • Update RELEASENOTES.md
  • Update documentation (e.g. for new settings or scenarios)
  • Add telemetry

Copilot AI review requested due to automatic review settings June 3, 2026 15:24
@OleWunschmann
Ole Wunschmann (OleWunschmann) requested a review from a team as a code owner June 3, 2026 15:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR enhances the customALGoFiles behavior during “Update AL-Go System Files”, adding support for unconditional removals (filesToRemove) and improving custom-template behavior by merging template settings directly and resolving files from the original AL-Go template where applicable.

Changes:

  • Add customALGoFiles.filesToRemove support end-to-end (schema, defaults, resolution logic, docs, and release notes).
  • Update CheckForUpdates to read template repo settings via ReadSettings and merge template settings during file resolution.
  • Expand automated coverage (unit + e2e) for include/exclude/remove resolution and custom-template propagation.

Reviewed changes

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

Show a summary per file
File Description
e2eTests/scenarios/CustomTemplate/runtest.ps1 Extends e2e scenario to validate custom-template file include/exclude/remove propagation and workflow presence.
Tests/CheckForUpdates.Action.Test.ps1 Adds unit tests for destination-folder resolution and expanded GetFilesToUpdate behaviors (including filesToRemove).
Actions/CheckForUpdates/CheckForUpdates.ps1 Updates settings reading (incl. trigger) and wires template settings + filesToRemove into update/removal flow.
Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 Implements ResolveFilePathsInDestinationFolder and extends GetFilesToUpdate to produce include/exclude/remove lists.
Actions/.Modules/settings.schema.json Extends settings schema with customALGoFiles.filesToRemove and clarifying descriptions.
Actions/.Modules/ReadSettings.psm1 Adds default filesToRemove array under customALGoFiles.
Scenarios/settings.md Documents customALGoFiles.filesToRemove in settings reference.
Scenarios/CustomizingALGoForGitHub.md Adds conceptual docs + examples for original-template resolution and filesToRemove.
RELEASENOTES.md Documents enhanced customALGoFiles behavior and new filesToRemove.

Comment thread Tests/CheckForUpdates.Action.Test.ps1 Outdated
Comment thread Tests/CheckForUpdates.Action.Test.ps1 Outdated
Comment thread e2eTests/scenarios/CustomTemplate/runtest.ps1
Comment thread e2eTests/scenarios/CustomTemplate/runtest.ps1
Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 Outdated
Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1
Comment thread e2eTests/scenarios/CustomTemplate/runtest.ps1 Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 Outdated
Comment thread Scenarios/CustomizingALGoForGitHub.md Outdated
Comment thread Actions/.Modules/settings.schema.json Outdated
@OleWunschmann

Copy link
Copy Markdown
Contributor Author

Maria Zhelezova (@mazhelez) The failing PS5 tests should be fixed now.

Comment thread RELEASENOTES.md
Comment thread RELEASENOTES.md
Comment thread Actions/.Modules/settings.schema.json Outdated
Copilot AI review requested due to automatic review settings July 21, 2026 09:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (1)

Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1:1205

  • Removal destinations are not constrained to $baseFolder. A filesToRemove entry with a matching template file and destinationFolder = '..' produces a destinationFullPath outside the repository; CheckForUpdates.ps1 later converts it to a relative path and passes it to Remove-Item. Normalize every removal destination and reject paths that are not descendants of the repository root before returning this list.
        $filesToRemove += @(ResolveFilePaths -sourceFolder $originalTemplateFolder -destinationFolder $baseFolder -files $filesToRemoveUnresolved -projects $projects)
    }
    $filesToRemove += @(ResolveFilePaths -sourceFolder $templateFolder -originalSourceFolder $originalTemplateFolder -destinationFolder $baseFolder -files $filesToRemoveUnresolved -projects $projects)
    $filesToRemove += @(ResolveFilePathsInDestinationFolder -destinationFolder $baseFolder -files $filesToRemoveUnresolved -projects $projects)

Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 Outdated
Comment thread Actions/CheckForUpdates/CheckForUpdates.ps1 Outdated
Comment thread Scenarios/CustomizingALGoForGitHub.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The temporary settings refresh can follow repository-controlled symlinks and write outside the workspace.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (1)

Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Explicitly clearing the trigger causes documented trigger-based conditional settings to be ignored.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Preserve event trigger when refreshing conditional settings

Actions/​CheckForUpdates/​CheckForUpdates.HelperFunctions.ps1:1313

This refreshed read also suppresses trigger-based ConditionalSettings by explicitly passing an empty trigger. As a result, custom-template customALGoFiles entries conditioned on the current workflow_dispatch or schedule event are still omitted on the first update—the exact path this helper is intended to refresh. Let ReadSettings obtain the trigger from GITHUB_EVENT_NAME.

Medium severity Avoid disabling trigger-based settings during updates

Actions/​CheckForUpdates/​CheckForUpdates.ps1:55

Passing an empty trigger disables trigger-based ConditionalSettings, although ReadSettings previously used GITHUB_EVENT_NAME here and the settings documentation allows trigger conditions for any setting. An Update run can therefore ignore values such as customALGoFiles that are conditional on workflow_dispatch or schedule. Omit this argument to preserve the documented behavior.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Literal wildcard paths can fail during file reads, and a Linux case-sensitive path can bypass physical validation.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Preserve literal wildcard paths in all source readers

Actions/​CheckForUpdates/​CheckForUpdates.HelperFunctions.ps1:1124

This now preserves literal wildcard characters in originalSourceFullPath, but the update path still reads these values through wildcard-aware APIs (Get-ContentLF -Path in CheckForUpdates.ps1:190/helper line 710 and Yaml.Load, whose loader uses Get-Content -Path). Consequently a valid file such as the newly tested File[1].ps1 resolves here but fails when the action actually reads it. Carry literal-path semantics through all source readers (and add an action-level test), rather than only validating path resolution.

Medium severity Use platform-aware path comparison on Linux

Actions/​CheckForUpdates/​CheckForUpdates.ps1:163

On Linux, PowerShell's -ne is still case-insensitive, so an original source path that differs from the custom source only by casing is treated as the same path and bypasses the new physical-resolution check. Since those are distinct Linux paths, use the platform-aware comparer introduced by this PR before deciding to skip validation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Workflow regeneration can still apply trigger-conditional settings despite the newly documented context-independent behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity · 1 Low severity

Open (4)

$repoSettings = ReadSettings -buildMode '' -project '' -workflowName '' -userName '' -branchName '' | ConvertTo-HashTable -recurse
# Get repo settings independent of the build, project, workflow, user, branch, and trigger running this update.
# Repository-scoped and unconditional settings still apply.
$repoSettings = ReadSettings -buildMode '' -project '' -workflowName '' -userName '' -branchName '' -trigger '' | ConvertTo-HashTable -recurse

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by 0b9dc8c

Comment thread Tests/CheckForUpdates.Action.Test.ps1 Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Source folders are enumerated before literal and boundary validation, causing incorrect handling and premature traversal.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Validate source directory before enumeration and use LiteralPath

Actions/​CheckForUpdates/​CheckForUpdates.HelperFunctions.ps1:1114

This containment check runs only after Get-ChildItem -Path has already interpreted and traversed sourceFolder. Consequently, a valid literal folder such as scripts[1] is treated as a wildcard path and is not discovered, while an escaping or redirected folder can be enumerated before its files are rejected. Resolve and validate the source directory first, then enumerate it with -LiteralPath.

Low severity Clarify custom-template handling and settings merge behavior

Scenarios/​CustomizingALGoForGitHub.md:250

“Used as-is” is inaccurate for settings files: GetModifiedSettingsContent preserves the destination settings and only updates the $schema from the original source when the destination exists. Clarify that the custom-template copy is ignored while the normal settings merge behavior still applies.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread Actions/CheckForUpdates/CheckForUpdates.ps1
Comment thread e2eTests/scenarios/CustomTemplate/runtest.ps1
Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1 Outdated
Comment thread Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The Linux E2E expectation is incorrect, and destinationName lacks required filename validation.

Review effort: Balanced
Findings: 2 High severity · 2 Medium severity

Open (4)
Resolved since last review (4)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Validate destinationName as a cross-platform filename

Actions/​.Modules/​settings.schema.json:766

destinationName is documented as a filename, but the schema accepts separators and platform-invalid characters. Because the implementation passes it directly to Join-Path, the same value can create a subdirectory on Windows but a backslash-named file on Linux, or fail during the update. Validate it as one cross-platform filename in both the schema and runtime path handling.

Comment thread e2eTests/scenarios/CustomTemplate/runtest.ps1
@OleWunschmann

Copy link
Copy Markdown
Contributor Author

Copilot review overview

🟡 Changes recommended

The Linux E2E expectation is incorrect, and destinationName lacks required filename validation.

Review effort: Balanced Findings: 2 High severity · 2 Medium severity

Open (4)

Resolved since last review (4)

Previously missed (1)
In code that hasn't changed since last review

Medium severity Validate destinationName as a cross-platform filename
Actions/​.Modules/​settings.schema.json:766

destinationName is documented as a filename, but the schema accepts separators and platform-invalid characters. Because the implementation passes it directly to Join-Path, the same value can create a subdirectory on Windows but a backslash-named file on Linux, or fail during the update. Validate it as one cross-platform filename in both the schema and runtime path handling.

The previously missed issue Medium severity Validate destinationName as a cross-platform filename was not added by this Pull-Request and would also apply to sourceFolder and destinationFolder.
I would like to keep it as is, because it currently works just fine.

@OleWunschmann

Copy link
Copy Markdown
Contributor Author

Maria Zhelezova (@mazhelez) Alexander Holstrup (@aholstrup1) spetersenms
I think I've addressed all of Copilot's review comments (either with a commit or a comment).
Could you please take a look at the remaining unresolved comments?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants