Enhance customALGoFiles feature - #2273
Conversation
There was a problem hiding this comment.
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.filesToRemovesupport end-to-end (schema, defaults, resolution logic, docs, and release notes). - Update
CheckForUpdatesto read template repo settings viaReadSettingsand 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. |
|
Maria Zhelezova (@mazhelez) The failing PS5 tests should be fixed now. |
There was a problem hiding this comment.
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. AfilesToRemoveentry with a matching template file anddestinationFolder = '..'produces adestinationFullPathoutside the repository;CheckForUpdates.ps1later converts it to a relative path and passes it toRemove-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)
There was a problem hiding this comment.
🟡 Not ready to approve
Destination normalization rejects valid relative paths, and case-insensitive deduplication drops valid Linux files.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1:956
- The destination file is canonicalized but
$fileDestinationFolderis not. Consequently, valid relative folder paths such as.orscripts/../sharedare treated as traversal and skipped because the normalized file path cannot start with the unnormalized folder string. Canonicalize the folder first and check that it remains under$destinationFolder; then retain the existing filename-boundary check.
$fileDestinationFolder = Join-Path $destinationFolder $file.destinationFolder
$fileDestinationFolder = Join-Path $fileDestinationFolder '' # Ensure file destination folder has a trailing slash for correct path resolution
$fullFilePath.destinationFullPath = Join-Path $fileDestinationFolder $destinationName
$fullFilePath.destinationFullPath = [System.IO.Path]::GetFullPath($fullFilePath.destinationFullPath) # Canonicalize the destination full path to an absolute path
Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1:928
$fileDestinationFolderis compared before it is canonicalized, whiledestinationFullPathis canonicalized. A valid relative path containing.or an internal..(for example,destinationFolder: './scripts') therefore failsStartsWithand is skipped. Canonicalize the project and file destination folders first, then separately verify the project stays under the base folder and the file folder stays under the project folder before checkingdestinationName.
This issue also appears on line 952 of the same file.
$fileDestinationFolder = Join-Path $destinationFolder $project
$fileDestinationFolder = Join-Path $fileDestinationFolder $file.destinationFolder
$fileDestinationFolder = Join-Path $fileDestinationFolder '' # Ensure file destination folder has a trailing slash for correct path resolution
Actions/CheckForUpdates/CheckForUpdates.HelperFunctions.ps1:1141
Group-Objectgroups strings case-insensitively by default. On Linux, the new dual-template resolution can produce distinct destinations such asFoo.ps1from the custom template andfoo.ps1from the original template, but this line drops one even though both paths are valid and distinct. Deduplicate with an OS-appropriateStringComparer, matching the path comparison used byResolveFilePaths.
$filesToInclude = @($filesToInclude | Group-Object { $_.destinationFullPath } | Sort-Object -Property Name | ForEach-Object { $_.Group[0] })
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| $fullFilePath.destinationFullPath = Join-Path $fileDestinationFolder $destinationName | ||
| $fullFilePath.destinationFullPath = [System.IO.Path]::GetFullPath($fullFilePath.destinationFullPath) # Canonicalize the destination full path to an absolute path | ||
|
|
||
| # Check if the destination file is under the file destination folder | ||
| if (-not $fullFilePath.destinationFullPath.StartsWith($fileDestinationFolder, $pathComparison)) { |
There was a problem hiding this comment.
Copilot Can you please elaborate on this finding? Can you share a full example of what could go wrong?
There was a problem hiding this comment.
I think Copilot has an issue with pull requests from forks.
As far as I know, the problem is: a supposedly valid destination path (i.e., no breakout from the repo/project folder) can reference a symbolic link that points to an invalid location (for example, outside the repo/project folder).
This is not currently checked and when the file is written, it follows the symlink and writes to the linked path instead of staying inside the repo/project.
There was a problem hiding this comment.
Uff, that doesn't sound good.
Containment in the repo at least should be enforced. I suspect this hasn't been checked for before, but it'll be great if you could include it in your changes.
📦 Release Notes Update RequiredAL-Go v9.2 has been released, and your changes to RELEASENOTES.md appear to be under that version's section. Action needed: Please move your release notes entry to above the Example## Changes to be included in the next release
- Your change here ✅
## v9.2
- Already released changesThank you for contributing to AL-Go! 🙏 |
|
Copilot resolve the merge conflicts in this pull request |
fb60e90
|
Maria Zhelezova (@mazhelez) I’ve gone ahead and resolved the merge conflicts, since I suspect that Copilot struggled with the fact that the source branch comes from my fork. I saw that there were already approvals, but there are still two Copilot comments open:
|
Welcome back! I hope your vacation was splendid! On the comments: I resolved the first one. As for the second one, I'll need to take a proper look. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| $upperCaseFolder = Join-Path $templateFolder 'CaseFolder' | ||
| $lowerCaseFolder = Join-Path $templateFolder 'casefolder' | ||
| $upperCaseFile = Join-Path $upperCaseFolder 'script.ps1' | ||
| $lowerCaseFile = Join-Path $lowerCaseFolder 'script.ps1' |
There was a problem hiding this comment.
Ole Wunschmann (@OleWunschmann) can you please address this one? Suppress, if needed.
❔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:
customALGoFilessettings 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.filesToIncludeandfilesToExcludeare 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.destinationNameproperty is added tofilesToInclude, 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
GetPathStringComparisonandGetPathStringComparerutility functions to ensure path comparisons and deduplication are case-insensitive on Windows/macOS and case-sensitive on Linux, improving cross-platform reliability.ResolveFilePathsto 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.Documentation and schema updates:
filesToIncludeandfilesToExcludehave been updated to clarify their new behavior and the newdestinationNameproperty.These changes make custom template management more robust, predictable, and safer for consumer repositories.
Related to discussion: #2227
✅ Checklist